Adds USPS completion page and wires up USPS confirmation form by el-mapache · Pull Request #1421 · 18F/identity-idp

el-mapache · 2017-05-09T18:35:55Z

This PR adds a confirmation page and fixes up the confirmation form in which the user enters the OTP code they received in the mail, when selecting USPS verification.

Confirmation screen:

Code confirmation form:

Manual testing is a bit cumbersome:

1). Sign in / create an account via a service provider (LOA3)
2). Fill in all the personal info, select USPS delivery
3). Enter your password, confirm your personal key
4). Manually go to /account (necessary because of an existing bug)
5). Click the pending profile alert
6). In your rails console, find your user, decrypt your pii and copy the otp code
7. Enter that code into the form on step 6
8. View the confirmation page

I'm marking this as ready for review although I suspect there will be some cleanup to do

EDIT: I added a screenshot for the confirmation page. I also tried to make it clearer that two pages were updated.

esgoodman · 2017-05-09T20:14:31Z

@rtwell I think we need to remove either the top flash banner from this OR remove the descriptive copy. Seeing the screenshot, I am tempted to remove the descriptive copy and move the flash banner down (see below), but that leaves us without an H1 for the page. What are your thoughts?

rtwell · 2017-05-09T20:38:08Z

i like the H1 and no banner, personally. it also lines up with the comps for this: https://github.com/18F/identity-private/issues/1443

fyi, @hursey013 and i are going to open an issue to revisit our type scale (hopefully in sprint 32?), so don't let the uncomfortable type sizes deter. done is better than perfect!

el-mapache · 2017-05-09T20:38:51Z

@rtwell sweet, I will update the PR!

rtwell · 2017-05-09T20:43:37Z

also, don't we need to tell folks what attributes will be shared here?

el-mapache · 2017-05-09T20:49:37Z

I don't think so? It wasn't on any of the mocks.

Edit: That I saw

rtwell · 2017-05-09T20:50:42Z

the more i think about this screen, the more i dont see why we cant use the same screen, attributes and all, from https://github.com/18F/identity-private/issues/1443.

these users are now no different than a user that successfully verified online, correct?

el-mapache · 2017-05-09T20:56:41Z

I think this screen is shown post account creation, so the user will have already been notified about what personal information is going to be shared with the partner agencies?

rtwell · 2017-05-09T21:12:19Z

hmmmm we should confirm that. @andrewhughey @esgoodman can you advise?

esgoodman · 2017-05-09T21:26:09Z

The issue is not so much when the account is created but when the attributes get shared with our partner. In the verify-by-mail case, the user can't sign in -- and hence the attributes are not going to be shared -- until the account is verified by entering a valid confirmation code from the letter.

TLDR; we should have the attributes on this page as in #1143. We will use the body text from #1143 as suggested.

@rtwell, can you confirm the final styles for @el-mapache? #1143 has a bunch of different mockups.

andrewhughey · 2017-05-10T14:10:37Z

I'm pretty comfortable with how this has been implemented given how the issue was originally outlined. I'd rather create new issues to work through the design concerns above than keep this PR open.

Let's focus here on the code review and we'll handle the rest elsewhere. Thanks @el-mapache!

monfresh · 2017-05-11T02:52:12Z

It looks like there is some overlap here with #1369. I would suggest waiting until that gets merged, then rebasing this PR against that.

monfresh · 2017-05-11T02:55:35Z

app/controllers/sign_up/completions_controller.rb

What do you think about making these analytics changes in a separate PR? They don't seem directly related to adding a USPS confirmation page, and I think this requires more thought about what distinction we want to make here. IdV completion could immediately follow user registration for example.

Gotcha. Makes sense to maintain two separate events. I'll take it out

monfresh · 2017-05-11T02:57:22Z

app/controllers/users/verify_account_controller.rb

I believe this logic will need to be moved to the VerifyProfileConcern once #1369 is merged.

Also, this assumes that the initial SP request is still in the session, but in all likelihood, it will not be, since by the time the user receives the letter, the session will have expired, so when they sign back in, session[:sp] will not be present, and redirecting them to sign_up_completed_url will take them to the account page.

In the USPS confirmation flow, we provide the user with a URL that points back to the service provider, so in theory there should be an sp session preset at all times. Obviously the user could elect not to enter the URL and instead go directly to the site. I spoke with Liz about it and she seemed to think it was unlikely the user wouldn't follow the instructions on the letter.

But, we won't know for sure until we test is out with users.

Yeah, on my way in to work, I realized I was wrong about this scenario not being likely. Even if the instructions didn't include a URL, the user could potentially go to the SP directly, then after they sign in on login.gov, they will be prompted to confirm their account, and at that point, the SP info will be in the session.

I think it's still worth it to have tests for both scenarios: one where the SP info is in the session, and one where it is not.

Also, I'm curious how the "URL that points back to the service provider" works. Where can I see an example of that?

Sorry that was worded really badly; but the idea is that the confirmation letter is going to direct the user to go back to the service provider's URL. It should be in the invision docs linked in the github issue

Would the no sp session scenario be better off in a separate PR? To generate the redirect url and service provider name we'll need additional behavior, I guess using the identity model? That is, assuming that we always want to show the user the completion page.

Yes, a separate PR is fine. I'll open an issue to discuss how we want to handle this scenario.

monfresh · 2017-05-11T02:58:14Z

app/decorators/user_decorator.rb

This method doesn't seem to be used anywhere.

monfresh · 2017-05-11T03:02:36Z

config/locales/errors/en.yml

Are we explicitly making a distinction between the USPS confirmation code and other security codes? For example, we have otp_incorrect: Security code is incorrect, but here we are changing the language. Is that intentional?

I did change the language intentionally. The new designs, which I'll be linking in a moment, called for it.

monfresh · 2017-05-11T03:05:42Z

config/routes.rb

Are these being used?

Nope, will remove!

monfresh · 2017-05-11T03:07:19Z

spec/controllers/users/verify_account_controller_spec.rb

Did you mean sign_up_completed_url?

monfresh · 2017-05-11T03:15:45Z

spec/features/idv/flow_spec.rb

Did you mean sign_up_completed_path?

monfresh · 2017-05-11T03:19:35Z

spec/features/idv/flow_spec.rb

Could you explain the reason for modifying the loa3_sp_session method? It looks like you're using the same issuer it was using before. If the reason was to set a request_url in the session in order to test that when you click "continue" on the completions page it takes you back to the SP, may I suggest that we don't need that test since the completions page and controller are already tested? The main thing we're concerned with here is to make sure that USPS confirmation takes the user to the completions page.

This test assumes that the USPS confirmation is performed in a session where SP info is still present, which is not a likely scenario. What do you think about modifying this spec such that the SP session is not present?

Per my above comment, its expected to be a likely scenario. I agree it would be good to have a fallback but am not 100% sure what that would look like right now.

I modified it for test purposes, I'll remove the parameter.

monfresh · 2017-05-11T21:38:34Z

For the LOA3 spec, I would recommend simulating an actual LOA3 request and putting the test in spec/features/saml/loa3. That way, it's a true integration test.

el-mapache · 2017-05-15T20:17:34Z

@monfresh I've rebased this onto 1369. I updated the USPS loa3 spec at features/saml/loa3_sso_spec to continue through the confirmation page and back to to original request URL.

I added a next_step method to the verify/account controller. In the case of openid the user needs to be redirected to whatever path is generated by after_sign_in_for, and in all other cases (from what I can tell) the user should get redirected to the sign_up_completed route.

If you think we can better address this by extending the after_sign_in_for method and/or adding a concern for openid, let me know.

monfresh · 2017-05-16T01:00:42Z

app/services/idv/session.rb

Did you mean to add this here? Perhaps this was an unintentional consequence of the rebase with master? This line doesn't exist in master.

I see now, it did exist in master when I opened the PR, but was moved in 1369 to the profile maker class. I'll remove it!

monfresh · 2017-05-16T02:21:24Z

spec/features/saml/loa3_sso_spec.rb

Since this test is going through SamlIdpController, it should not be necessary to stub the session. In fact, it's discouraged because it might be masking a bug. We want to simulate the user experience as closely as possible.

Interesting, I added this because I could not get the test to pass at all. The value of sp_session was always an empty object.

monfresh · 2017-05-16T02:23:44Z

spec/features/idv/flow_spec.rb

What do you think about moving this spec to spec/features/saml and using a real SAML request as opposed to stubbing the session? This will ensure a more authentic end to end test.

We would also need to have the same test in spec/features/openid_connect to make sure it works there too.

The spec should be in openid_connect already, on line 280.

I'm slightly confused, the spec already exists in saml, it is the test you commented on ☝️ . Do you want me to completely remove this scenario from flow_spec and just rely on the 'live' tests?

Oh. I didn't realize that. If it's exactly the same test, then yes, we only want the "live" one.

The two specs aren't exactly the same, as the flow_spec example verifies immediately after the profile is created. I don't think that matters much though, as that scenario is guaranteed to never happen in real life.

EDIT: On second though, it is probably worth it to test going through the IdV flow and clicking on 'send letter'. Both the saml and openid specs manually set the deactivation_reason attribute and don't hit that page.

Agreed, but I think we only need one test that uses SAML and goes through everything as a user would.

monfresh · 2017-05-16T02:34:22Z

config/locales/errors/en.yml

I recently learned about a neat trick where you can reference an existing translation inside another one. Since "Resend confirmation instructions" is defined in idv.messages.usps.send_again, you can refer to it by add a colon before it, like this:

You can click :idv.messages.usps.send_again to get another one.

Magic!

No way!! I was thinking it was odd that there wasn't anyway to reuse these strings.

I actually can't get this to work. From previous searches I've seen using & as an alias, but I can't find references to prefixing a translation key with a : to share them.

Strange. It worked for me. I tested by running a spec that loads this page and I added a screenshot_and_save_page, and I saw the translated text.

and you put in exactly what you pasted here? I see the string as presented here, with the : prefix, uninterpolated

My bad. I was looking at the wrong page. I just realized this change is not related to this PR. It looks like it's just cleaning up whitespace. It turns out that to use this magic trick, the only thing you can put as the value is the key you are referencing, like this:

confirmation_period_expired: :idv.messages.usps.send_again

So, it doesn't look like we can use it here 😢

oh bummer 😞

el-mapache · 2017-05-16T19:56:18Z

@monfresh I removed the test from flow_spec and put it into saml/loa3_sso_spec.

monfresh · 2017-05-17T02:07:38Z

app/controllers/users/verify_account_controller.rb

I don't understand why OpenID Connect users would not be presented with the completions page. Could you explain that to me please?

@monfresh the openid connect flow takes the user to a different page after they verify their code, which is determined by the after_sign_in_path_for method. Ideally I'd like to just use that everywhere, but I don't want to refactor that method in the context of this PR.

We may also need to update the openid connect flow, as we now have different styles for the 'shared attributes' pages. This flow also bypasses the current completions page.

For reference, here is a screenshot from openid_connect spec on line 280:

As for why users are taken to a different page, I'm not sure! I guess the requirements weren't exactly in sync? We hadn't really nailed down the LOA3 verification flow either. In any case, it is probably worth opening up a ticket to address this.

My understanding is that we are getting rid of this screen, so might as well do it here, at least for this scenario. By "getting rid of it", I mean always redirect to sign_up_completed_url and don't include any logic for OIDC.

monfresh · 2017-05-17T02:21:58Z

spec/features/saml/loa3_sso_spec.rb

Did you mean to stop the test here? Don't we want to test all the way through to seeing the completions page and then continuing to the SP?

I stopped it because the user will never be able to go all the way through the flow when they first register. There is a second test beginning at line 141 that checks that the user is able to return and finish the verification process.

Sure, I get that, but they are still able to do more on the site, right? Once on the verify/confirmations page, they will acknowledge their personal key, and after that, they should be sent to the account page, where they should see a message about needing to confirm their USPS code.

I suggest we merge #1424 (I approved it last night. It just needs to be squashed), then rebase this PR against master so we can properly test the whole flow as the user would experience it.

monfresh · 2017-05-17T20:28:40Z

@el-mapache Looks like your rebase with master is causing these latest test failures. It's just some I18n stuff that needs to be updated in the tests.

el-mapache · 2017-05-17T20:53:03Z

@monfresh errors fixed!

monfresh · 2017-05-17T23:19:31Z

This is starting to shape up nicely! 2 more tests and we should be good to go:

Clicking "Send another letter" when prompted to enter USPS code. Is it expected that the user has to enter their password and then get a new personal key in this scenario?
Clicking "I can't get mail at this address". Currently, in this PR, clicking that link refreshes the account/verify page. I'm assuming it should be going to the phone activation page instead.

el-mapache · 2017-05-17T23:54:08Z

@monfresh So, those two routes don't actually behave as expected right now. The original issue included making these pages behave properly but it seemed like too much to tackle. I believe there are two additional issues open to handle those two links.

monfresh · 2017-05-18T00:24:18Z

Gotcha. In that case, what do you think about removing those links from this PR since they are not hooked up right?

el-mapache · 2017-05-18T00:26:57Z

Yeah, I think that's reasonable

monfresh · 2017-05-18T14:50:07Z

Approved! Please remember to squash before merging.

**Why**: To better match our designs. Add verify/completion controller, route, view+test **Why**: To Let the user know their account was verified and activated successfully, and to provide a simple mechanism for the user to return to the service provider Removes uneeded duplicate controller **Why**: Made changes to `completions_controller` to accomodate either loa3 or loa1 signups Remove vestigal test code, undeed route logic **Why**: USPS test now ends once user has gotten to the completion page, and no longer attempts to click the 'continue to partner agency' link. Route logic was out of scope and is handled in another pr Move usps spec to features/saml **Why**: It doesn't stub out the session and is more of an integration test Rebase with master, extend loa3 signup feature **Why**: Get up-to-date with new code, make loa3 feature spec more complete (follow user all the way from signup to their profile) Remove links to pages being reworked/created **Why**: They don't go anywhere yet

el-mapache self-assigned this May 9, 2017

el-mapache added the status - ready for review label May 9, 2017

el-mapache force-pushed the ab-restyle-usps-confirmation branch from 4103878 to 3ae6947 Compare May 9, 2017 18:40

el-mapache force-pushed the ab-restyle-usps-confirmation branch from 3ae6947 to 4c18d42 Compare May 9, 2017 20:28

el-mapache force-pushed the ab-restyle-usps-confirmation branch 2 times, most recently from 9cd6340 to b414d4a Compare May 10, 2017 20:14

monfresh requested changes May 11, 2017

View reviewed changes

monfresh reviewed May 11, 2017

View reviewed changes

app/decorators/user_decorator.rb Outdated

Copy link

Contributor

monfresh May 11, 2017

Choose a reason for hiding this comment

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

This method doesn't seem to be used anywhere.

monfresh reviewed May 11, 2017

View reviewed changes

spec/features/idv/flow_spec.rb Outdated

Copy link

Contributor

monfresh May 11, 2017

Choose a reason for hiding this comment

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

Did you mean sign_up_completed_path?

el-mapache reacted with thumbs up emoji

monfresh reviewed May 11, 2017

View reviewed changes

el-mapache changed the title ~~Adds USPS Confirmation page~~ Adds USPS Confirmation page and wires up USPS confirmation May 11, 2017

el-mapache changed the title ~~Adds USPS Confirmation page and wires up USPS confirmation~~ Adds USPS completion page and wires up USPS confirmation form May 11, 2017

el-mapache force-pushed the ab-restyle-usps-confirmation branch from 2d858f9 to bb51a2f Compare May 11, 2017 19:24

el-mapache force-pushed the ab-restyle-usps-confirmation branch from bb51a2f to 612fda8 Compare May 15, 2017 18:17

el-mapache force-pushed the ab-restyle-usps-confirmation branch 2 times, most recently from d3758f3 to a59352f Compare May 15, 2017 20:00

monfresh reviewed May 16, 2017

View reviewed changes

el-mapache force-pushed the ab-restyle-usps-confirmation branch from 6377363 to f868e88 Compare May 16, 2017 16:45

el-mapache force-pushed the ab-restyle-usps-confirmation branch 2 times, most recently from bdcac61 to 31d0488 Compare May 16, 2017 22:59

monfresh reviewed May 17, 2017

View reviewed changes

monfresh mentioned this pull request May 17, 2017

Add feature flag to prepopulate USPS code #1434

Merged

el-mapache force-pushed the ab-restyle-usps-confirmation branch from 31d0488 to 630af13 Compare May 17, 2017 18:25

monfresh force-pushed the ab-restyle-usps-confirmation branch from def689a to 9080bc6 Compare May 17, 2017 19:51

el-mapache force-pushed the ab-restyle-usps-confirmation branch from 9080bc6 to 218c1f4 Compare May 17, 2017 20:05

el-mapache force-pushed the ab-restyle-usps-confirmation branch from 218c1f4 to 806a0bc Compare May 17, 2017 20:43

monfresh approved these changes May 18, 2017

View reviewed changes

el-mapache force-pushed the ab-restyle-usps-confirmation branch from 8c49406 to 0dca6af Compare May 18, 2017 15:01

el-mapache merged commit 85b842a into master May 18, 2017

el-mapache deleted the ab-restyle-usps-confirmation branch May 18, 2017 15:13

Conversation

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

Uh oh!

Uh oh!

esgoodman commented May 9, 2017

Uh oh!

rtwell commented May 9, 2017

Uh oh!

el-mapache commented May 9, 2017

Uh oh!

rtwell commented May 9, 2017

Uh oh!

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

Uh oh!

Uh oh!

rtwell commented May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

rtwell commented May 9, 2017

Uh oh!

esgoodman commented May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewhughey commented May 10, 2017

Uh oh!

monfresh commented May 11, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

el-mapache May 11, 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!

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!

monfresh commented May 11, 2017

Uh oh!

el-mapache commented May 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

el-mapache commented May 9, 2017 •

edited

Loading

el-mapache commented May 9, 2017 •

edited

Loading

rtwell commented May 9, 2017 •

edited

Loading

el-mapache commented May 9, 2017 •

edited

Loading

esgoodman commented May 9, 2017 •

edited

Loading

el-mapache May 11, 2017 •

edited

Loading

el-mapache May 16, 2017 •

edited

Loading

el-mapache May 16, 2017 •

edited

Loading