Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update screen when user verifies directly on login #1774

Merged
merged 1 commit into from
Nov 15, 2017
Merged

Update screen when user verifies directly on login #1774

merged 1 commit into from
Nov 15, 2017

Conversation

nickbristow
Copy link
Contributor

Why:
If a user navigates directly to login.gov, they
will now see a screen after verifying, that displays
the different applications they have authenticated with

@nickbristow nickbristow changed the title Update screen when user verifies directly on login Update screen when user verifies directly on login[wip] Nov 7, 2017
@nickbristow nickbristow force-pushed the new_verified_screen_no_sp_session branch 7 times, most recently from eb50c36 to e736c38 Compare November 7, 2017 22:44
@jmhooper
Copy link
Member

jmhooper commented Nov 8, 2017

@nickbristow: Can we get screenshots?

@jmhooper
Copy link
Member

jmhooper commented Nov 8, 2017

Oops, didn't realize this was WIP. Carry on :)

@nickbristow nickbristow force-pushed the new_verified_screen_no_sp_session branch 5 times, most recently from e3f83ed to eb587f0 Compare November 8, 2017 22:20
@nickbristow
Copy link
Contributor Author

Going directly to login, with no session data, shows a user with multple accounts this screen.
login_gov_-_you_have_created_your_account_with_login_gov

User who has only logged into one application
login_gov_-_you_have_created_your_account_with_login_gov

Note: The mockups show the links as being the agency name. I don't believe we are storing the full agency name anywhere? Which is what the mockups have.

@jmhooper

@nickbristow nickbristow force-pushed the new_verified_screen_no_sp_session branch 6 times, most recently from cc40cf8 to eac8f94 Compare November 13, 2017 22:32
@nickbristow nickbristow requested a review from jmhooper November 13, 2017 22:46
@nickbristow nickbristow changed the title Update screen when user verifies directly on login[wip] Update screen when user verifies directly on login Nov 13, 2017
jmhooper
jmhooper reviewed Nov 14, 2017
View reviewed changes
app/controllers/sign_up/completions_controller.rb Outdated
service_provider_attributes
if show_completions_page?
track_agency_handoff(
Analytics::USER_REGISTRATION_AGENCY_HANDOFF_PAGE_VISIT
)
else
redirect_to new_user_session_url
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I know this is what was there before, but it was confusing to me for a bit and maybe we should change it to root_url or account_url? It works and I don't have a strong feeling so I'll leave up to you.

jmhooper
jmhooper reviewed Nov 14, 2017
View reviewed changes
app/decorators/user_decorator.rb Outdated
@@ -121,6 +121,10 @@ def recent_events
(events + identities).sort { |thing_a, thing_b| thing_b.happened_at <=> thing_a.happened_at }
end

def identities
user.identities.order('last_authenticated_at DESC').map(&:decorate)
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on adding a limit to this? Could get out of hand if the user has like 20 SPs they've signed into? We may want to show the last n identities instead of all of them. (cc @mkhandekar)

Also, you can use .order(last_authenticated_at: :desc) which does the same thing but imo is a little neater. That's an opinion though, so ultimately up to you.

@nickbristow nickbristow force-pushed the new_verified_screen_no_sp_session branch from eac8f94 to d123abb Compare November 14, 2017 18:50
jmhooper
jmhooper reviewed Nov 14, 2017
View reviewed changes
Copy link
Member

@jmhooper jmhooper left a comment

Choose a reason for hiding this comment

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

We should probably add unit tests for Identity#agency_name, UserDecorator#identities, and the new SignUpCompletionsShow methods. At least the ones that do more than return a hardcoded string.

@nickbristow nickbristow force-pushed the new_verified_screen_no_sp_session branch 2 times, most recently from 68d15f7 to c678a1f Compare November 14, 2017 20:49
@nickbristow nickbristow force-pushed the new_verified_screen_no_sp_session branch 10 times, most recently from 84e80b3 to 17c1f3c Compare November 15, 2017 18:49
Update screen when user verifies directly on login
56f7bb8 
**Why**:
If a user navigates directly to login.gov, they
will now see a screen after verifying, that displays
the different applications they have authenticated with
@nickbristow nickbristow force-pushed the new_verified_screen_no_sp_session branch from 17c1f3c to 56f7bb8 Compare November 15, 2017 20:55
@nickbristow nickbristow merged commit b6ab6d0 into master Nov 15, 2017
@nickbristow nickbristow deleted the new_verified_screen_no_sp_session branch November 15, 2017 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmhooper jmhooper jmhooper approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nickbristow @jmhooper