Skip to content

LG-11165: Create separate "submitted" event for WebAuthn registrations#9417

Merged
jmdembe merged 45 commits intomainfrom
jd-LG-11165-webauth-setup-analytics
Nov 7, 2023
Merged

LG-11165: Create separate "submitted" event for WebAuthn registrations#9417
jmdembe merged 45 commits intomainfrom
jd-LG-11165-webauth-setup-analytics

Conversation

@jmdembe
Copy link
Contributor

@jmdembe jmdembe commented Oct 19, 2023

🎫 Ticket

LG-11165: Create separate "submitted" event for WebAuthn registrations

🛠 Summary of changes

This ticket creates and modifies events associated with Webauthn Setup.

Webauthn Setup Visited: Now tracks page visits only;
Webauthn Setup Submitted: Tracks successful submission and any error message

jmdembe and others added 5 commits October 17, 2023 14:25
@jmdembe jmdembe marked this pull request as ready for review October 20, 2023 14:11
@jmdembe jmdembe requested a review from a team October 25, 2023 14:20
@aduth
Copy link
Contributor

aduth commented Oct 25, 2023

I just tested this by trying to add a Face/Touch credential from my account dashboard and cancelling when prompted for my credential, and I didn't see a submitted event logged.

I think the problem is we don't actually submit the form when handling frontend errors, we just reload with a new query parameter, which probably explains why we were handling errors in the #new action.

Could you see what we'd need to do to handle those errors? Arguably those are the ones we care about the most, so it'd be important to capture, and I think we should have test coverage for them as well.

Copy link
Contributor

@kevinsmaster5 kevinsmaster5 left a comment

Choose a reason for hiding this comment

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

Looks good to me.
I was able to validate that this comment: #9417 (comment) is no longer an issue. Cancelling when prompted will log the error now.

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 👍 Left a couple minor nitpicks.

Per the ticket, can we also make sure to have the relevant dashboards ready for the new event before this goes live in production?

@jmdembe jmdembe merged commit df26d60 into main Nov 7, 2023
@jmdembe jmdembe deleted the jd-LG-11165-webauth-setup-analytics branch November 7, 2023 18:30
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.

4 participants