-
Notifications
You must be signed in to change notification settings - Fork 112
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
Remove track_mfa_submit analytics pass-through method #10679
Conversation
changelog: Internal, Analytics, Remove track_mfa_submit analytics pass-through method
@@ -122,41 +122,43 @@ | |||
end | |||
|
|||
describe '#create' do | |||
let(:parsed_phone) { Phonelib.parse(controller.current_user.default_phone_configuration.phone) } | |||
let(:user) { create(:user, :with_phone) } |
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.
The reason for this change is that have_logged_event
coming after the controller action works differently than how analytics was previously stubbed with expect
before the action, and certain properties rely on the state of the user before the action. Some values like controller.current_user
are unavailable after the action is run.
phone_fingerprint: phone_fingerprint, | ||
frontend_error:, | ||
**extra, | ||
}.compact, |
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.
The reason for this change is that have_logged_event
is much stricter about comparing logged properties, and was failing on method-specific properties like auth_app_configuration_id
being nil
when logged in another method's controller.
I still think we should probably do an automatic .compact
on all properties logged, but that's a future problem to address.
changelog: Internal, Analytics, Remove track_mfa_submit analytics pass-through method
🛠 Summary of changes
Removes
Analytics#track_mfa_submit
, updating call-sites to callAnalyticsEvents#multi_factor_auth
directly.Why?
pii_like_keypaths
for specific personal key verification properties, which is reasonable to expect to be passed in the one place it's necessary (PersonalKeyVerificationController
)AnalyticsEvents
module method or actual event namehave_logged_event
, which is the preferred way to ensure that events are logged as expected, contains no PII, and all properties are documentederror_details
now that it is correctly flagged as undocumented📜 Testing Plan
Verify that the build passes.
Verify that when signing in with 2FA, the "Multi-Factor Authentication" event is still logged as expected:
make watch_events
in a separate terminal process from the IdP server