Skip to content

LG-11006: "User registration complete" event not logged for direct sign-up#9403

Merged
jc-gsa merged 6 commits intomainfrom
LG-11006-user-registration-complete-event
Oct 24, 2023
Merged

LG-11006: "User registration complete" event not logged for direct sign-up#9403
jc-gsa merged 6 commits intomainfrom
LG-11006-user-registration-complete-event

Conversation

@jc-gsa
Copy link
Contributor

@jc-gsa jc-gsa commented Oct 17, 2023

🎫 Ticket

LG-11006

🛠 Summary of changes

Adds the event "User registration: complete" to user registration flows where previously missing.

@jc-gsa jc-gsa requested a review from a team October 18, 2023 15:17
changelog: Internal, Authentication, Record event user_registration_complete
@jc-gsa jc-gsa force-pushed the LG-11006-user-registration-complete-event branch from b0a920f to ed09b57 Compare October 18, 2023 15:23
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

I think this direction makes sense as far as treating the MFA selections setup as funneling toward the completions screen, which is how we're using it today.

In the future we might want to be able to support sending people to the MFA selection setup outside these flows (I recall this coming up in the designs around #8965 and why we couldn't link "choose another authentication method" in the error alert). Maybe we'd want to use the newly-introduced user_session[:in_account_creation_flow] in the logic to support that.

This also has me realizing that our idea of "setup completion" is rather inconsistent, since it's less to do with account setup, and more to do with whether someone needs to see a consent screen, which can happen outside initial account setup. That caught me off guard here since I worried about sending people to the "setup completion" in a flow like the AAL2 strict upgrade, but I think this is consistent with how we're currently treating "completions".

Those are things we can follow-up with separately and wouldn't block this, but I think this work helps highlight a lot of existing inconsistency.

@jc-gsa jc-gsa force-pushed the LG-11006-user-registration-complete-event branch from 8ffef01 to 730bcc7 Compare October 20, 2023 18:07
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Can we add some regression test coverage for the event being logged for sign-in?

Edit: Also, would help for review to complete the PR template details.

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jc-gsa jc-gsa force-pushed the LG-11006-user-registration-complete-event branch from 682cdc2 to 6983ba9 Compare October 24, 2023 16:58
@jc-gsa jc-gsa merged commit 35a1f0d into main Oct 24, 2023
@jc-gsa jc-gsa deleted the LG-11006-user-registration-complete-event branch October 24, 2023 19:09
@mdiarra3 mdiarra3 mentioned this pull request Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants