Skip to content

Document analytics events, batch 18 (LG-5937)#6368

Merged
zachmargolis merged 12 commits intomainfrom
margolis-analytics-events-18
May 18, 2022
Merged

Document analytics events, batch 18 (LG-5937)#6368
zachmargolis merged 12 commits intomainfrom
margolis-analytics-events-18

Conversation

@zachmargolis
Copy link
Contributor

What it says on the tin.

In this batch:

  • MULTI_FACTOR_AUTH_ENTER_PIV_CAC
  • MULTI_FACTOR_AUTH_ENTER_TOTP_VISIT
  • MULTI_FACTOR_AUTH_ENTER_PERSONAL_KEY_VISIT
  • MULTI_FACTOR_AUTH_ENTER_BACKUP_CODE_VISIT
  • MULTI_FACTOR_AUTH_ENTER_WEBAUTHN_VISIT

@zachmargolis zachmargolis requested review from a team, sheldon-b and tomas-nava May 17, 2022 22:43
analytics.track_event(
Analytics::MULTI_FACTOR_AUTH_ENTER_PERSONAL_KEY_VISIT, context: context
)
analytics.multi_factor_auth_enter_totp_visit(context: context)
Copy link
Contributor

Choose a reason for hiding this comment

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

multi_factor_auth_enter_personal_key_visit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops, great catch, thanks

zachmargolis and others added 2 commits May 18, 2022 09:03
…cation_controller_spec.rb

Co-authored-by: Tomas Apodaca <thomas.apodaca@gsa.gov>
@zachmargolis zachmargolis merged commit 748784b into main May 18, 2022
@zachmargolis zachmargolis deleted the margolis-analytics-events-18 branch May 18, 2022 16:43
return unless FeatureManagement.prefill_otp_codes?
@code = ROTP::TOTP.new(current_user.auth_app_configurations.first.otp_secret_key).now
analytics.track_event(Analytics::MULTI_FACTOR_AUTH_ENTER_TOTP_VISIT)
analytics.multi_factor_auth_enter_totp_visit
Copy link
Contributor

Choose a reason for hiding this comment

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

context is a required parameter for multi_factor_auth_enter_totp_visit, so this errors.

ArgumentError at /login/two_factor/authenticator
missing keyword: :context

Fortunately, this code is guarded by FeatureManagement.prefill_otp_codes?, so it's really only an issue in local development.

Copy link
Contributor

Choose a reason for hiding this comment

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

Although... now that I think of it, we probably want to be logging this event in all cases, not only prefill_otp_codes? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Follow-up at #6522

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