-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LG-12757: Add a "cancel" confirmation step to “Connect your verified information to [Partner Agency]” #10405
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see we have an existing SignUp::CancellationsController
. Is that something we can reuse instead of adding a new controller?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I forgot that the SignUp::CancellationsController
involves cancelling account creation altogether, i.e. destroying the new account. I don't think we'd want that for the consent screen. I'd wonder if we could at least preserve the naming convention of "cancel".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preserving the 'cancel' convention makes sense.
5adcbd6
to
c843c28
Compare
2bb17d6
to
b9a82bd
Compare
config/locales/account/en.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These strings were hard to place. This page is shown in two scenarios--when the user signs up and wishes to redirect and when the user signs in for the first time and does hits cancel and not "Agree and continue". I can see how it would fit in sign_up
, but its not exclusive to the sign up process
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think it's a little strange as well and I might wonder if a new "completion(s)" section could work better, but we're already using sign_up
for other strings on the page, and technically the controller is SignUp::CompletionsController
so there's some precedent for sign-up, even if it's not strictly always how the user experiences it
config/locales/account/en.yml
Outdated
bullet2_html: You’ll still have a %{app_name} account which you can manage or | ||
delete on your %{link_html}. | ||
exit: Exit %{app_name} | ||
header_html: <p>If you exit %{app_name} now:</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than embed the paragraph in the HTML, I'd expect it to be part of the view. But it looks like we already have a paragraph in the view markup as well, so we probably don't need it at all?
header_html: <p>If you exit %{app_name} now:</p> | |
header_html: If you exit %{app_name} now: |
config/locales/account/en.yml
Outdated
@@ -65,6 +65,15 @@ en: | |||
enter your email and password again. | |||
piv_cac: Sign in with your government employee ID | |||
tab_navigation: Account creation tabs | |||
login_exit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on previous conversation, I might suggest using "cancel" terminology instead of "exit" for consistency
843c79f
to
4972563
Compare
analytics.return_to_sp_cancelled(redirect_url: redirect_url, **location_params) | ||
redirect_to(redirect_url, allow_other_host: true) | ||
@redirect_url = sp_return_url_resolver.return_to_sp_url | ||
analytics.return_to_sp_cancelled(redirect_url: @redirect_url, **location_params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably not log this event until the user is actually redirected to the partner application. Currently it's being logged when they view the confirmation screen.
As mentioned when we were pairing, I'm wondering if it'd be better to create a separate controller/action and leave the redirect controller responsible only for redirecting, not for showing any user-facing views. That way, we can keep the analytics here but assume they'd only reach this action when it's time for them to be redirected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably not log this event until the user is actually redirected to the partner application. Currently it's being logged when they view the confirmation screen.
Additionally, we may want a separate logging event for when the user visits the confirmation screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense.
I remember at the very end of our pair session that we could try using a query parameter to redirect and that is why I explored that route. I'll go back to what we first talked about.
ecfeec1
to
1307dc6
Compare
f1e5dd6
to
197a9f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionally it's working well 👍 Noted a few lingering suggested adjustments.
5e83d63
to
2533ec1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple minor test-related comments, but otherwise LGTM! 👍
5fcc364
to
6624ceb
Compare
Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
08910df
to
c3c1e26
Compare
🎫 Ticket
Link to the relevant ticket:
LG-12757
🛠 Summary of changes
This pull request adds an interstitial page confirming if the user wants to exit the app to their service provider, or continue with setup
📜 Testing Plan
Required: OIDC app to simulate login through service provider
👀 Screenshots
After: