Changing address verification step to a form instead of buttons #1781
Conversation
There was a problem hiding this comment.
I had a few comments about comments. Also looks like you made CodeClimate angry.
One thing that we don't do here is display any kind of error when you don't make a choice on the form. I think that is okay, but we may want to run it by @mkhandekar.
It'd also be nice to see a feature spec for when you don't make a choice on that screen. Perhaps in the USPS verification selection examples or something?
| span = t('idv.messages.select_verification_form.phone_message') | ||
|
|
||
| // USPS verification should always be enabled in prod. | ||
| // This check is a convenience for lower envs. |
There was a problem hiding this comment.
We can probably drop the comment in here
There was a problem hiding this comment.
I think I may be more of a fan of comments than other people. My thinking was that since I had to stop and ask about it, leave a comment for the next person so they will know.
There was a problem hiding this comment.
We typically try to keep comments out the idp. Since there's a lot of code and a lot of churn, it is difficult to keep comments up to date. If you think there's a risk that someone will change the code and break something later, it may be better to add a tests. If you think the code is unclear, it may be better to refactor it so it is more clear.
This comment is an example of one that will go out of date and be confusing when we change this config in the lower envs.
| class: 'btn btn-primary col-6 mb2 p2 rounded-lg inline-block' | ||
|
|
||
| // Be sure to keep this outdented to the same level as the form above | ||
| // or terrible things will happen to your tests. |
There was a problem hiding this comment.
Same here. We can probs drop this comment
There was a problem hiding this comment.
I'm on the fence on this one. On the one hand, I tend to like to comment anyplace a simple mistake can lead to big headaches. On the other hand, this one was really just my silly mistake, and I can see an argument that the comment only says "malformed HTML is bad". So I could go either way.
| @@ -53,7 +53,7 @@ | |||
| expect(current_path).to eq verify_session_result_path | |||
| end | |||
|
|
|||
| scenario 'fincance shows failure flash message after max attempts', :email do | |||
| scenario 'finance shows failure flash message after max attempts', :email do | |||
| @@ -68,11 +68,19 @@ def click_idv_continue | |||
| end | |||
|
|
|||
| def click_idv_address_choose_phone | |||
| click_link t('idv.buttons.activate_by_phone') | |||
| # we're capturing the click on the label element via the unique "for" attribute | |||
There was a problem hiding this comment.
I think we should keep it here, because the click handler is a bit unique. There are multiple elements within the label, and clicking the label itself is the only thing that matches how the CSS is actually setting the hidden input to true. Since it's both not immediately clear and a relatively unfamiliar pattern, I think it's worth commenting.
There was a problem hiding this comment.
Having a slack conversation about giving the user an error if nothing is chosen: https://gsa-tts.slack.com/archives/C0NGESUN5/p1511195802000213
There was a problem hiding this comment.
@tbaxter-18f and I just chatted offline and decided that verification via text should be pre-selected as the vast majority of folks will want to choose that. That eliminates the need to provide an error if nothing is selected.
tbaxter-18f
changed the title
|
Because we're pre-selecting the text option, users may breeze through this page without reading fully. In case they find themselves somewhere they don't mean to be, we do want to make sure that users have a way to navigate freely between these three screens:
Please see this issue (https://github.com/18F/identity-private/issues/2138#issuecomment-315444285) to implement the 2 links at the bottom of the "verify by text" and verify by letter" pages. @tbaxter-18f @jmhooper I see that issue 2138 is in the backlog. I'd like to make sure to add to next sprint. |
tbaxter-18f
changed the title
**Why**: In order to ensure the user makes a selection and to ease the workflow by preselecting the most common choice.
|
After talking with @mkhandekar we decided to pre-select "by phone", which negates the possiblity of a null choice. So I think we should be good here without an additional test. |
|
@tbaxter-18f: Would you mind rebasing this or merging master in? Looks like you have a failing test, but it's fixed on master. |
**Why**: somewhere we picked up a YAML error.
**Why**: The logstash example configuration was accidentally removed in #1781 because the commits were not properly squashed. While testing this locally, I ran into some issues, so I updated the documentation to help others solve them.
To better match UX patterns used elsewhere.
https://github.com/18F/identity-private/issues/2112