Skip to content

Fix verification by mail redirect logic#1569

Merged
monfresh merged 1 commit intomasterfrom
bsh-usps-verification
Jul 28, 2017
Merged

Fix verification by mail redirect logic#1569
monfresh merged 1 commit intomasterfrom
bsh-usps-verification

Conversation

@gemfarmer
Copy link
Contributor

@gemfarmer gemfarmer commented Jul 24, 2017

For issue https://github.com/18F/identity-private/issues/1890

Fixes an issue that was redirecting users to /verify instead of /account after clicking the "Send another letter" button.

There are now two tests to check the verification letter flow. One if the user doesn't log out before entering their confirmation letter (this probably will never happen in real life) and one if they do log out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Code Climate says this is not tested. Can you double check that the specs are performing the correct steps? I would expect at least one of those specs (if not both) to have an expectation that the user ends up on the account_path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm. Yeah, I'll check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the flow just wasn't completed. I updated the specs and Code Climate should be happy now

@gemfarmer gemfarmer force-pushed the bsh-usps-verification branch from 637e746 to 71312bd Compare July 24, 2017 16:18
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this was like this before this PR, but to make sure we're testing things exactly like the user would experience, can we change sign_in_live_with_2fa(user) to this instead?

click_link t('links.sign_in')
fill_in_credentials_and_submit(user.email, user.password)
click_submit_default

The difference is that the latter preserves the request_id in the URL, whereas the former doesn't, and we have logic in the app that does different things depending on whether or not the request_id is present.

Copy link
Contributor

Choose a reason for hiding this comment

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

We'll also need allow(FeatureManagement).to receive(:prefill_otp_codes?).and_return(true) if the test doesn't already use that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

It looks like we are prefilling before each test, so I won't add anything new on that front!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But wait, isn't the point of reusing methods like sign_in_live_with_2fa(user) to avoid duplication of processes that are the same? How is this different?

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is sign_in_live_with_2fa(user) visits new_user_session_path explicitly, instead of clicking the "Sign In" button, which drops the request_id from the URL. To avoid duplication, we can create a new helper method.

Fixes an issue that was redirecting users to /verify instead of /account after clicking the "Send another letter" button.
@gemfarmer gemfarmer force-pushed the bsh-usps-verification branch from 2b15136 to a86a124 Compare July 25, 2017 22:55
@gemfarmer
Copy link
Contributor Author

@monfresh made those final changes. Ready for review! I'm headed out on vacation, so merge if you think it is ready :)

Copy link
Contributor

@monfresh monfresh left a comment

Choose a reason for hiding this comment

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

LGTM

@monfresh monfresh merged commit 1ccd05d into master Jul 28, 2017
@monfresh monfresh deleted the bsh-usps-verification branch July 28, 2017 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants