Add feature flag to control verify by mail availability by el-mapache · Pull Request #1455 · 18F/identity-idp

el-mapache · 2017-05-25T13:44:05Z

This will let us deploy the application to production while smoothing out the LOA3 mail verification flow. The flow will be available in our local and QA environments.

This PR:

Always redirects the user to the phone verification option after they confirm their financial information
Removes the link to the verify by mail page from the phone verification page
Redirects users who attempt to manually visit the verification options page, usps confirmation code entry page, or usps_controller index to the account page

monfresh · 2017-05-30T18:01:00Z

lib/feature_management.rb

What do you think about tying this feature flag to a Figaro entry, similarly to our enable_identity_verification flag so that we can easily turn it on when we're ready without having to modify the IdP code?

Also, did you mean to remove the reveal_usps_code? flag? IIUC, they serve 2 separate purposes. One is to facilitate testing of the USPS flow feature by prefilling the code in the form, and should only be enabled in certain environments. The other is to globally turn on or off the whole USPS feature. If the USPS feature is ON, we still want the code prefilling to be OFF on some servers. In this PR, if I'm reading things correctly, turning on the USPS feature will automatically also prefill the USPS code.

Yes, but I'll add a separate method since combing them will defeat the purpose of being able to toggle the feature in production without prefilling the code.

Re: the first part of the comment, I thought our feature flags were entirely handled in the feature management class. I'll look at the enable_identity_verification flag for insight!

If you look at the FeatureManagement class, you'll notice that pretty much all the methods depend on a Figaro key. The class mainly provides descriptive method names and allows us to add other conditions besides Figaro values.

monfresh · 2017-05-30T18:07:11Z

app/controllers/concerns/idv_step_concern.rb

What do you think about restricting access at the route level instead, similarly to how we turned off LOA3? https://github.com/18F/identity-idp/blob/master/config/routes.rb#L115

Ah I didn't see that. I'll change it, I like that much better.

monfresh · 2017-05-30T18:13:11Z

app/controllers/users/verify_account_controller.rb

We can eliminate the need of this additional condition by restricting at the route level.

monfresh · 2017-05-30T18:15:51Z

app/controllers/verify/finance_controller.rb

This change seems unrelated to the feature flag. Can we move this to a separate PR and open a new issue if there isn't one, assuming it's a bug that this always goes to the address path.

Is it unrelated? Currently the user won't be redirected to a valid route if the feature flag isn't enabled. Are you saying we ignore that and just accept that the code won't work properly with the feature flag disabled, and address it in a separate PR? Or will moving the USPS feature flag check into the routes address this problem?

I might be missing the point

Maybe I'm misunderstanding, but I don't think the verify/address page should be restricted since it allows the user to choose between USPS and Phone verification. If the USPS feature is ON, then the USPS button should show up on the verify/address page, otherwise, it should not be shown. In other words, unless this controller was doing something wrong, it should not be changed. What should be changed is the logic in the view to determine when to show the USPS button since address verification can be performed in one of two ways currently. It's the USPS verification that needs to be toggled on or off, not address verification as a whole, right?

Ok, I understand your point now. We want to err on the side of presenting more information about the process to the user, so I'll add the page back and remove the mail verification option from the view.

el-mapache · 2017-05-30T20:45:26Z

@monfresh updated with your feedback thus far

monfresh · 2017-05-30T21:47:32Z

spec/controllers/verify/finance_controller_spec.rb

I don't think we need to change anything in this spec since we didn't change the controller. It's expected to always redirect to verify/address, whether the feature is enabled or not.

monfresh · 2017-05-30T21:51:46Z

spec/features/openid_connect/openid_connect_spec.rb

What do you think about removing these changes since they are cosmetic, to keep this PR focused on the addition of the feature flag?

Pretty sure the above line is required for the test to pass. If I remove it , it fails. Or is there a better way to do this?

I guess you meant this blank line?

I meant that both of the changes in this file are not related to this PR. They are only cosmetic changes. One removes the parentheses around true, and the other is removing a blank line.

monfresh · 2017-05-30T21:53:36Z

spec/routing/usps_verification_routing.rb

s/does not route/routes

monfresh · 2017-05-30T21:55:04Z

app/controllers/verify/finance_controller.rb

Any particular reason for the change from _url to _path?

Looks like a lazy copy from the old code, one of the methods had url instead of path

monfresh · 2017-05-30T21:56:47Z

spec/controllers/users/verify_account_controller_spec.rb

I don't think this is needed, right?

el-mapache · 2017-05-31T13:50:42Z

@monfresh updated again

monfresh · 2017-05-31T14:29:43Z

app/controllers/verify/finance_controller.rb

Can we leave this file unmodified since this change is not necessary to implement this feature?

monfresh

lgtm. Thanks!

**Why**: The entire flow will be conditionally available, so tying the code visibility to the availability of the feature makes sense Gate usps pages, verify/phone view + specs update **Why**: Users shouldnt have the option to verify by mail if the feature is turned off, and they shouldnt be able to directly access the pages Address PR feedback **Why**: There are improvements to be made * Use Figaro config setting * Change name to better match identity_verification flag * Adds route spec * Removes checks on controller level

el-mapache · 2017-06-01T15:20:38Z

thank you!

monfresh · 2017-06-01T16:43:39Z

Crap. I forgot we can't merge this until we submit a PR to the devops repo to add this new Figaro flag to dev.

el-mapache · 2017-06-01T16:47:41Z

Ugh thats right, should we revert?

monfresh · 2017-06-01T16:50:32Z

No, it's fine. I'm working on the devops PR now.

el-mapache self-assigned this May 25, 2017

el-mapache added the status - work in progress label May 25, 2017

el-mapache force-pushed the ab-add-activate-by-mail-flag branch from 7bc848b to a35ab97 Compare May 25, 2017 14:52

el-mapache added status - ready for review and removed status - work in progress labels May 25, 2017

monfresh reviewed May 30, 2017

View reviewed changes

el-mapache force-pushed the ab-add-activate-by-mail-flag branch from 191c650 to bc983b1 Compare May 30, 2017 20:35

monfresh reviewed May 30, 2017

View reviewed changes

spec/routing/usps_verification_routing.rb Outdated

Copy link

Contributor

monfresh May 30, 2017

Choose a reason for hiding this comment

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

s/does not route/routes

monfresh reviewed May 30, 2017

View reviewed changes

spec/controllers/users/verify_account_controller_spec.rb Outdated

Copy link

Contributor

monfresh May 30, 2017

Choose a reason for hiding this comment

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

I don't think this is needed, right?

el-mapache force-pushed the ab-add-activate-by-mail-flag branch from d2b3b69 to 8a13108 Compare May 31, 2017 13:45

monfresh reviewed May 31, 2017

View reviewed changes

monfresh approved these changes Jun 1, 2017

View reviewed changes

el-mapache force-pushed the ab-add-activate-by-mail-flag branch from c51f20b to de4befd Compare June 1, 2017 15:20

el-mapache merged commit 8efadb3 into master Jun 1, 2017

el-mapache deleted the ab-add-activate-by-mail-flag branch June 1, 2017 16:11

Conversation

el-mapache commented May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

el-mapache May 30, 2017 • 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!

el-mapache commented May 30, 2017

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!

el-mapache commented May 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monfresh left a comment

Choose a reason for hiding this comment

Uh oh!

el-mapache commented Jun 1, 2017

Uh oh!

monfresh commented Jun 1, 2017

Uh oh!

el-mapache commented Jun 1, 2017

Uh oh!

monfresh commented Jun 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

el-mapache commented May 25, 2017 •

edited

Loading

el-mapache May 30, 2017 •

edited

Loading