LG-6066: Stub out React entry-point for new IdV API routes by aduth · Pull Request #6177 · 18F/identity-idp

aduth · 2022-04-07T16:00:36Z

Why: To give us a foundation to start building out the new API-driven identity verification front-end.

Screenshot:

app/controllers/idv_controller.rb

config/routes.rb

zachmargolis · 2022-04-07T16:15:30Z

app/controllers/concerns/feature_flagged_concern.rb

we do this very often! I like the idea of DRY-ing it

I'm slightly wary of the dynamic send 😬

I'm slightly wary of the dynamic send 😬

Any ideas for an alternative? Allowlist? Method on IdentityConfig like IdentityConfig.enabled?(key) ?

Edit: Maybe checking keys on IdentityStore.store and gracefully falling back to false ?

Since it's a struct you can use brackets:

IdentityConfig.store[:feature_name_symbol]

Ope, brackets throws an error but apparently .dig does not!

IdentityConfig.store.dig(:feature_symbole_name)

After a bit of chatting with @mitchellhenke to better understand the concern, I came up with an alternative in def6403 which I think is a little more balanced in terms of convenience vs. explicitness.

jmhooper · 2022-04-07T16:52:12Z

app/controllers/idv_controller.rb

oooh, neato

mitchellhenke · 2022-04-08T14:11:35Z

scripts/changelog_check.rb

aduth · 2022-04-11T12:28:53Z

config/routes.rb

I had to bring back the feature flag guarding for this route, since we're going to have some conflicting route definitions between the current and in-development flows. This only happened now because originally I had mistakenly created the first route for /verify/password (currently /verify/review), but the ticket was meant for personal key (both before and after being /verify/personal_key).

what if we add a /v2 in there or some other prefix so that we don't have conflicting routes?

We could. I was also contemplating if there's a better name than "verify", since... it's pretty overused at this point. I couldn't come up with something I loved, though maybe something incorporating "identity" and/or "proofing".

Went with "v2" in 6a81f95. We can revisit later if we come up with a better idea.

by popular demand

**Why**: Until it's ready to at least be remotely functional in local development

**Why**: Because that's the goal of LG-6066

**Why**: So that someone unfamiliar can have a basic understanding of its purpose

**Why**: Avoid issues with environment-specific configuration

**Why**: Less magical and open with forwarding of config keys, more explicit with callable behavior

[skip changelog]

zachmargolis

LGTM

zachmargolis · 2022-04-13T18:20:46Z

app/controllers/verify_controller.rb

+class VerifyController < ApplicationController
+  include RenderConditionConcern
+
+  render_if -> { IdentityConfig.store.idv_api_enabled }, only: [:show]


still love this, another idea I have for the name:

Suggested change

render_if -> { IdentityConfig.store.idv_api_enabled }, only: [:show]

check_or_render_not_found -> { IdentityConfig.store.idv_api_enabled }, only: [:show]

still love this, another idea I have for the name:

Would the idea be to emphasize the actual behavior of render_not_found?

I could also imagine:

Suggested change

render_if -> { IdentityConfig.store.idv_api_enabled }, only: [:show]

render_not_found_unless -> { IdentityConfig.store.idv_api_enabled }, only: [:show]

Or:

Suggested change

render_if -> { IdentityConfig.store.idv_api_enabled }, only: [:show]

render_not_found_if -> { !IdentityConfig.store.idv_api_enabled }, only: [:show]

I also sort of wonder if it would be so bad to just have the complete logic here, if the concern is concealing too much.

Suggested change

render_if -> { IdentityConfig.store.idv_api_enabled }, only: [:show]

before_action { render_not_found if !IdentityConfig.store.idv_api_enabled }, only: [:show]

Went with proposed rename to keep this moving along. Can always come back to it later.

Makes sense! We have a bunch of similar code we could refactor all at once too -- did we file a followup ticket?

We have a bunch of similar code we could refactor all at once too -- did we file a followup ticket?

We did now! See LG-6157.

zachmargolis · 2022-04-13T18:23:47Z

spec/controllers/concerns/render_condition_concern_spec.rb

+    routes.draw do
+      get 'index' => 'anonymous#index'
+      get 'show' => 'anonymous#show'
+    end


do we need this? I think anonymous controllers "just work" without it usually

It works by default for index, but not for show. Not sure why, tbh 🤷 I put both just for completeness, even though index was technically optional.

1) RenderConditionConcern#show renders 404 Failure/Error: subject(:response) { get :show } ActionController::UrlGenerationError: No route matches {:action=>"show", :controller=>"anonymous"} # ./spec/controllers/concerns/render_condition_concern_spec.rb:52:in `block (3 levels) in <top (required)>' # ./spec/controllers/concerns/render_condition_concern_spec.rb:55:in `block (3 levels) in <top (required)>'

huh, weird :/

A bit of light sleuthing, maybe index is available since it's set up as the default action handler.

https://github.com/rspec/rspec-rails/blob/dc0e2b5/lib/rspec/rails/example/controller_example_group.rb#L94-L101

https://github.com/rails/rails/blob/51b4370/actionpack/lib/action_dispatch/routing/mapper.rb#L157-L159

spec/controllers/verify_controller_spec.rb

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

https: //github.com//pull/6177#discussion_r849771392 Co-Authored-By: Zach Margolis <zbmargolis@gmail.com>

aduth requested review from peggles2 and solipet April 7, 2022 16:01

aduth commented Apr 7, 2022

View reviewed changes

app/controllers/idv_controller.rb Outdated Show resolved Hide resolved

aduth commented Apr 7, 2022

View reviewed changes

config/routes.rb Outdated Show resolved Hide resolved

zachmargolis reviewed Apr 7, 2022

View reviewed changes

config/routes.rb Outdated Show resolved Hide resolved

zachmargolis reviewed Apr 7, 2022

View reviewed changes

jmhooper reviewed Apr 7, 2022

View reviewed changes

app/controllers/idv_controller.rb Outdated

Copy link

Contributor

jmhooper Apr 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oooh, neato

aduth marked this pull request as ready for review April 8, 2022 13:07

mitchellhenke reviewed Apr 8, 2022

View reviewed changes

scripts/changelog_check.rb Outdated

Copy link

Contributor

mitchellhenke Apr 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼

aduth commented Apr 11, 2022

View reviewed changes

aduth force-pushed the aduth-verify-api-route branch from 6a81f95 to bee7044 Compare April 12, 2022 14:49

aduth mentioned this pull request Apr 12, 2022

Implement client session secret store #6183

Merged

aduth changed the title ~~Stub out React entry-point for new IdV API routes~~ LG-6066: Stub out React entry-point for new IdV API routes Apr 12, 2022

aduth mentioned this pull request Apr 12, 2022

LG-6066: Create StepIndicator React component for proofing flow #6187

Merged

aduth force-pushed the aduth-verify-api-route branch from bee7044 to fc7fb1b Compare April 13, 2022 13:40

aduth added 15 commits April 13, 2022 12:22

Stub out React entry-point for new IdV API routes

b8a8c7a

Create new VerifyController for IdV API

f2fd969

Clean-up IdvController

a3a4051

Bring back FeatureFlaggedConcern

44ed45c

by popular demand

Create feature_flagged_concern_spec.rb

2e2cb0b

Create verify_controller_spec.rb

814682c

Create basic React root element

4d5f08d

Disable idv_api_enabled for all environments by default

a1adf29

**Why**: Until it's ready to at least be remotely functional in local development

Route personal_key instead of password

6d8da0a

**Why**: Because that's the goal of LG-6066

Add README for verify-flow package

60a22b1

**Why**: So that someone unfamiliar can have a basic understanding of its purpose

Always define config value context for specs

73bb716

**Why**: Avoid issues with environment-specific configuration

Always draw route in spec

b234eff

Replace FeatureFlaggedConcern with RenderConditionConcern

ed45b85

**Why**: Less magical and open with forwarding of config keys, more explicit with callable behavior

Inline before_action block

851439d

Add changelog

fc67389

[skip changelog]

Namespace new IdV flow routes as "v2"

446e4b5

aduth force-pushed the aduth-verify-api-route branch from fc7fb1b to 446e4b5 Compare April 13, 2022 16:22

zachmargolis approved these changes Apr 13, 2022

View reviewed changes

aduth and others added 2 commits April 13, 2022 15:43

Remove unnecessary route drawing

a79a59a

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

Rename "render_if" to "check_or_render_not_found'

23ec441

https: //github.com//pull/6177#discussion_r849771392 Co-Authored-By: Zach Margolis <zbmargolis@gmail.com>

aduth merged commit 6a64ed3 into main Apr 14, 2022

aduth deleted the aduth-verify-api-route branch April 14, 2022 12:21

This was referenced Apr 18, 2022

Enhance FormSteps to manage history using path fragment #6213

Merged

Lg 6114 personal key #6237

Merged

	render_if -> { IdentityConfig.store.idv_api_enabled }, only: [:show]
	check_or_render_not_found -> { IdentityConfig.store.idv_api_enabled }, only: [:show]

	render_if -> { IdentityConfig.store.idv_api_enabled }, only: [:show]
	render_not_found_unless -> { IdentityConfig.store.idv_api_enabled }, only: [:show]

	render_if -> { IdentityConfig.store.idv_api_enabled }, only: [:show]
	render_not_found_if -> { !IdentityConfig.store.idv_api_enabled }, only: [:show]

	render_if -> { IdentityConfig.store.idv_api_enabled }, only: [:show]
	before_action { render_not_found if !IdentityConfig.store.idv_api_enabled }, only: [:show]

Conversation

aduth commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aduth commented Apr 7, 2022 •

edited

Loading

aduth Apr 8, 2022 •

edited

Loading

aduth Apr 13, 2022 •

edited

Loading