Conversation
el-mapache
force-pushed
the
ab-address-verify-views
branch
4 times, most recently
from
c5ff55e to
2452c19
Compare
el-mapache
added
status - ready for review
and removed
status - work in progress
labels
el-mapache
force-pushed
the
ab-address-verify-views
branch
from
2452c19 to
bd32684
Compare
There was a problem hiding this comment.
It was duplicated on line 134
spec/features/idv/flow_spec.rb
Outdated
There was a problem hiding this comment.
debugger statement left in :)
There was a problem hiding this comment.
whoops! Good catch 😄
spec/features/idv/flow_spec.rb
Outdated
spec/features/idv/flow_spec.rb
Outdated
There was a problem hiding this comment.
I don't think we need a newline here
spec/features/idv/flow_spec.rb
Outdated
There was a problem hiding this comment.
I don't think we need a newline here. Usually, I do 3 phases: 1. setup 2. exercise 3. expectations
So signing in the user and setting the session can be phase 1
visit verify route, filling out form, clicking continue, cancel stuff can be phase 2
and then expectation is phase 3
One newline between each phase for clarity. otherwise, no newlines (generally...there are always exceptions, of course)
There was a problem hiding this comment.
I personally don't like having the first five lines of the code all jammed up there with no spaces, but I am totally willing to adhere to the conventions we already have.
spec/features/saml/loa3_sso_spec.rb
Outdated
There was a problem hiding this comment.
why not just look for current_path to eq profile_path ?
There was a problem hiding this comment.
Running the specs in a JS context changes the entire url (from example.com to a port number). Maybe we can just match on the path?
edit: we can
There was a problem hiding this comment.
no matter what the entire URL is, expect(current_path).to eq profile_path should work
spec/support/features/idv_helper.rb
Outdated
There was a problem hiding this comment.
possible to look for it within a different div for more exactness? eg:
within('.cool-div') do
click_on t('idv.buttons.cancel')
end
There was a problem hiding this comment.
definitely, I didn't know that even existed.
There was a problem hiding this comment.
cool, I think that would be preferable to the comments here
el-mapache
force-pushed
the
ab-address-verify-views
branch
from
bd32684 to
8a53f67
Compare
|
@jessieay I think I addressed your feedback. I must have amended at some point with the new commit, sorry about that! |
jessieay
left a comment
jessieay
left a comment
There was a problem hiding this comment.
Updates look good! I am not seeing a linked github issue? Is there one so I can confirm that this is doing what the ticket is asking for?
|
@jessieay hooray! should be referenced now. |
There was a problem hiding this comment.
what if there is no service provider present?
There was a problem hiding this comment.
This flow only applies to identity verification, so I think there always is?
There was a problem hiding this comment.
ahh ok. can I get a quick confirm from @pkarman or someone else? I know that I have gone through the LOA3 flow without an SP several times, but I am a developer so I know that my behavior is probably abnormal
There was a problem hiding this comment.
It's possible to hit the /verify endpoint w/o having been referred by a SP. You can log in directly afaik.
There was a problem hiding this comment.
I actually did not know that was possible. Should I have a fallback to APP_NAME if sp_name is nil, then?
There was a problem hiding this comment.
This is the magic ✨ of code review!
The text reads:
To protect you from identity fraud, you can't use your account at %{sp_name} until you activate it by entering a confirmation code.
Would this make sense with an sp name of login.gov ? Maybe! I'd double check with our designer folks.
There was a problem hiding this comment.
I don't think it would, since the user can definitely use login.gov without having their identity verified, right? Does it make sense to have this route only accessible during LOA3 verification? Or do we want to be able to verify a user's identity directly using login.gov in the future?
In that case, I think the copy will probably need to change somewhat.
There was a problem hiding this comment.
For now, I think the easiest route would be to show different copy depending on whether an sp is present or not. Check out DecoratedSession and how we are using that for rendering different content for sp and non-sp sessions. (basically, you define the same method on ServiceProviderSessionDecorator and SessionDecorator and have them return different content)
let me know if you'd like to pair on this! should be pretty fast and then we can get this merged :) :) 🌈
There was a problem hiding this comment.
word, I am just writing some specs for this, hope to push it up shortly.
jessieay
left a comment
jessieay
left a comment
There was a problem hiding this comment.
One final comment about copy that seems very sp-specific. Otherwise, let's 
|
I suspect that a rebase and squash might change this slightly. Happy to re-view after the that. |
**Why**: To keep up with changes to our designs
jessieay
force-pushed
the
ab-address-verify-views
branch
from
7045852 to
27a0f48
Compare
jessieay
left a comment
jessieay
left a comment
There was a problem hiding this comment.
Confirmed locally that the views look good!
|
thanks @jessieay!! |
**Why**: To keep up with changes to our designs
4 participants
Views for LOA3 address/identity verification. The views are hooked up to the new controllers @pkarman set up in , but the additional OTP code entry hasn't been implemented yet.
Screenshots