Add logging ahead of removing Analytics logic by zachmargolis · Pull Request #7864 · 18F/identity-idp

zachmargolis · 2023-02-21T20:16:53Z

This is a redo of #7833, but first:

Adds tests ahead of time
Finds 4 new callsites that were missing analytics hookds
- Before, only two "submit" events in register_step_from_analytics_submit_event.rb were included
- Now, 4 more that I missed in a second file register_step_from_analytics_view_event.rb are included too
In terms of the database, this should be a no-op in production, it does not touch anything, but it adds Rails.logger.info logging we can check to verify (basically the original and proposed counts at each spot should match)
- We can run a CW query along the lines of this to see if there are any differences and dig in
```
fields @message
| filter name = 'event_to_doc_auth_log_token'
| stats count(*) by source, token, action
```

zachmargolis · 2023-02-21T20:20:13Z

app/controllers/idv/gpo_controller.rb

+      # Funnel::DocAuth::RegisterStep.new(current_user.id, current_sp&.issuer).
+      #   call(:usps_address, :view, true)


I am open to dropping the commented out lines, but figured it was easier to leave them in. Could also have gone with a feature flags approach, but that seemed like a lot more temporary code and more complex testing

zachmargolis · 2023-02-21T20:21:00Z

app/services/funnel/doc_auth/register_step_from_analytics_view_event.rb

@@ -10,7 +10,19 @@ class RegisterStepFromAnalyticsViewEvent

      def self.call(user_id, issuer, event, result)


this is the file I missed entirely last time 😬 as far as I can tell it's just these 2! shouldn't be a mystery 3rd one

changelog: Internal, Logging, Add logging ahead of streamlining analytics events

app/controllers/idv/phone_controller.rb

Use correct success value

soniaconnolly

LGTM, with the caveat that I don't have a larger context to catch hidden issues.

jskinne3

Thanks for your explanation; LGTM

@matthinz

* LG-8796: Add recaptcha disclosure form at bottom of phone setup screen for new and existing accounts (#7827) * changelog: Upcoming Features, Authentication, Add recaptcha disclosure form at bottom of phone setup screen * styling fixes and dont hardcode login.gov * hide and show other movile service * add captcha submit button to add new phone * add to hide grecaptcha-badge * use recaptcha disclosure shared * remove empty space * lint recaptcha badge * Skip devices database query when identifier is null (#7856) changelog: Internal, Performance, Skip devices database query when identifier is null * Removed analytics check from password visibility toggle spec. (#7860) The test was flaky and the flag is unused, per Andrew Duthie. [skip changelog] * Removed use of Faker email creation in user factory primary email (#7861) * Removed use of Faker email creation in user factory primary email We suspect that using Faker email, rather than explicitly setting sequential emails, was occasionally creating collisions and leading to flaky test failures. [skip changelog] * Add 2023 SAML certs (#7862) (symlink 2022 ones to have non-blank files there) changelog: Internal, Certificates, 2023 SAML Certificate rotation * LG-8215 Update Event: IDV Reproof using Address verify by Mail (#7859) changelog: Internal, Attempts API, Update in tracking event * LG-8604 - Optimize s3 by streaming data w/ feature flag (#7843) * WIP - optimize s3 by streaming data changelog: Internal, Attempts API, Scalability Optimization * shifted a line - rubocop apparently wasn't picky about this * added feature flag. updated spec to reflect nested conditionals * cleaned up some comments. * broke out s3 response code into its own method. * cleaned up code based on feedback * updated default buffer size and made some clarifying changes based on feedback changelog: Internal, Attempts Api, Optimization: S3 Streaming * cleaned up more test descriptions. changelog: Internal, Attempts Api, Feature: S3 Streaming * Add logging ahead of removing Analytics logic (#7864) * Add Rails.logger.info logging near proposed call sites changelog: Internal, Logging, Add logging ahead of streamlining analytics events * Remove unused Authentication Presenter data (#7868) * Remove unused Authentication Presenter data changelog: Internal, Maintenance, Remove unused methods and variables * remove unused pivcac email data * remove more unused methods * Disable notify for good_job (#7870) changelog: Internal, Background Jobs, Update GoodJob and disable NOTIFY * LG-8423 log cancel analytics from barcode page (#7838) * analytics logged when user cancels from barcode pg * update event and add spec for for event in sessions controller * event and spec added for cancellations controller * changelog: Internal, Analytics Events, add cancel from barcode page analytics event * fix lint issue w/ spec * change step to barcode to make unique * use cancellation go back w/ extra attributes when user comes from barcode pg * remove new event and update session controller * update spec * change enrollment key * update cancel spec * check for enrollment and merge extra attributes and update tests * use pending enrollment and remove sp as it is redundant * update spec to create pending enrollment * LG-8901: New Hybrid Handoff (upload step) with feature flag (#7849) * Adding initial fields and new icons * Deleting useless hybrid handoff mobile tests * Pulling in send_link step functionality to upload_step * Adding feature flag for new combined hybrid handoff step * Adding additional checking for 50/50 conditions -- What As @matthinz pointed out, the current code does not account well for the following 50/50 states: 1. User is shown the new combined upload view, but submits the phone number to an instance with the feature flag off; 2. User is shown the original upload view, but clicks the phone button submitting to an instance where the feature flag is now on We handle this case by adding a request param _only_ to the form rendered by the new combined view. That view is only shown to users who have the feature flag enabled. The upload step will now check for that param -- called :combined -- and can determine regardless of the current state of the feature flag, that the user actually has submitted a phone number to the upload step itself, and that the send link step should be skipped. Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov> Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> changelog: User-Facing Improvements, Hybrid Handoff Screen, Combining upload and send link steps into one screen * Unpack a dynamic send to make code easier to trace (#7869) * Remove now-unused constant changelog: Internal, Refactor, Small update to remove dynamic send in 2FA mixin * LG-8875 / LG-8877: Try to avoid race condition in document-capture-welcome (#7842) * "Convert" document-capture-welcome to typescript (It was already valid TS) * Rework camera check to block form submission Don't let the form submit until the promise we use to check for the presence / nonpresence of a camera has resolved. * Disable submit buttons while we wait * TEMP: Allow injecting delay into hasCamera() Set HAS_CAMERA_DELAY_MS in localStorage to configure. * Update app/javascript/packs/document-capture-welcome.ts Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> * Use SpinnerButtonComponent for welcome/agreement * Simplify cameraCheckPromise usage Eliminate additional local variable and just race the promises * Wrap agreement spinner button in <div> Adding margin-top-4 to button does not get applied to the spinner dots * changelog: User-Facing Improvements, Identity verification, help prevent mobile users from ending up in hybrid handoff flow * Log event from frontend for mobile/camera checks Ideally, we want to see if this code actually fixes anything. (Relates to LG-8877) Co-authored-by: Eric Gade <eric.gade@gsa.gov> * Refactor perf measuring slightly Pull out to a function that just augments the promise with perf data * Add spin_on_click to SpinnerButtonComponent Allow setting spin-on-click as needed. * Toggle spinner on form submission Just in case there is a form validation issue, don't start spinning until we're sure we can actually make the submission. * await trackEvent that could occur during form submission * Add idv_mobile_device_and_camera_check to AnalyticsEvents Document the event and make it so it's not prefixed with "Frontend:" * Revert "TEMP: Allow injecting delay into hasCamera()" This reverts commit 96cbc3e. * Don't use performance.measure() Complicated API, don't need it for what we're doing. --------- Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> Co-authored-by: Eric Gade <eric.gade@gsa.gov> * LG-7836 Automatically refresh USPS auth token when necessary (#7808) * implement retry behavior with repetitive code * remove begin/rescue blocks and get failing tests * add retry options to faraday. tests still fail. * refactor implementation & spec * changelog:Bug Fixes, In-person proofing, refresh USPS auth token when request fails * attempt broader retry behavior. tests still fail. * attempt to refresh token before pinging USPS api. more tests fail * got first test to pass, need to get other expired token tests passing * get all expired token tests to pass * DRY up proofer test setup * add buffer to expires_at conditional * fix broken tests by adding redis cache setup * fix broken enrollment helper tests by adding caching setup * respond to feedback * remove no-op * Fixing missed grid gap sizing (follow up to LG-8901) (#7873) * Fixing missed grid gap sizing h/t @aduth changelog: User-Facing Improvements, Hybrid Handoff Screen, Combining upload and send link steps into one screen * LG-9025 Go to new SsnController from hybrid flow when feature flag is set (#7875) From the DocumentCaptureStep, we only want to redirect to the new SsnController if we are in the DocAuthFlow. This step is reused from the Hybrid Flow, and we don't want to go to the SsnController on mobile. In hybrid flow, we don't return to the DocumentCaptureStep, so redirect to SsnController from LinkSentStep when the feature flag is true as well. Duplicate the hybrid flow test with doc_auth_ssn_controller_enabled flag set to true and make sure it passes. Note that on my machine even on main without the feature flag I have to set a binding.pry and delay the mobile browser part of the test to get it to pass. changelog: Internal, refactor, fix hybrid flow with new SsnController (feature flagged) Co-authored-by: eric-gade <eric.gade@gsa.gov> --------- Co-authored-by: Malick Diarra <malick.diarra@gsa.gov> Co-authored-by: Mitchell Henke <mitchell.henke@gsa.gov> Co-authored-by: John Maxwell <john.maxwell@gsa.gov> Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> Co-authored-by: Osman Latif <109746710+olatifflexion@users.noreply.github.com> Co-authored-by: Rwolfe-Nava <87499456+Rwolfe-Nava@users.noreply.github.com> Co-authored-by: Shannon A <20867088+svalexander@users.noreply.github.com> Co-authored-by: Eric Gade <105373963+eric-gade@users.noreply.github.com> Co-authored-by: Matt Hinz <matt.hinz@gsa.gov> Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> Co-authored-by: Eric Gade <eric.gade@gsa.gov> Co-authored-by: eileen-nava <80347702+eileen-nava@users.noreply.github.com> Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov>

- Builds on logging added in #7864, and is a redo of #7685 now that we have logging to ensure better coverage - Specs were added in #7864

- Builds on logging added in #7864, and is a redo of #7685 now that we have logging to ensure better coverage - Specs were added in #7864 changelog: Internal, Logging, Move doc auth logging out of shared analytics class

zachmargolis added 5 commits February 21, 2023 10:42

Add Rails.logger.info logging near proposed call sites

4f55762

update spec coverage

1603d27

specs

8107f9b

simplify tests

1b35fbd

specs

0613414

zachmargolis requested review from a team February 21, 2023 20:16

zachmargolis commented Feb 21, 2023

View reviewed changes

Add changelog

28ae7e6

changelog: Internal, Logging, Add logging ahead of streamlining analytics events

soniaconnolly reviewed Feb 21, 2023

View reviewed changes

app/controllers/idv/phone_controller.rb Outdated Show resolved Hide resolved

Update app/controllers/idv/phone_controller.rb

0fb282d

Use correct success value

soniaconnolly approved these changes Feb 21, 2023

View reviewed changes

jskinne3 approved these changes Feb 22, 2023

View reviewed changes

zachmargolis merged commit 38b37f8 into main Feb 22, 2023

zachmargolis deleted the margolis-add-doc-auth-log-token-logging branch February 22, 2023 15:49

solipet mentioned this pull request Feb 23, 2023

Deploy RC 256 to Prod #7878

Merged

zachmargolis added a commit that referenced this pull request Feb 27, 2023

Move DocAuthLog registration out of shared analytics class

5b87857

- Builds on logging added in #7864, and is a redo of #7685 now that we have logging to ensure better coverage - Specs were added in #7864

zachmargolis mentioned this pull request Feb 27, 2023

Move DocAuthLog registration out of shared analytics class #7898

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logging ahead of removing Analytics logic#7864

Add logging ahead of removing Analytics logic#7864
zachmargolis merged 7 commits intomainfrom
margolis-add-doc-auth-log-token-logging

zachmargolis commented Feb 21, 2023

Uh oh!

zachmargolis Feb 21, 2023

Uh oh!

zachmargolis Feb 21, 2023

Uh oh!

Uh oh!

soniaconnolly left a comment

Uh oh!

jskinne3 left a comment •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		# Funnel::DocAuth::RegisterStep.new(current_user.id, current_sp&.issuer).
		# call(:usps_address, :view, true)

		@@ -10,7 +10,19 @@ class RegisterStepFromAnalyticsViewEvent

		def self.call(user_id, issuer, event, result)

Conversation

zachmargolis commented Feb 21, 2023

Uh oh!

zachmargolis Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

zachmargolis Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

soniaconnolly left a comment

Choose a reason for hiding this comment

Uh oh!

jskinne3 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jskinne3 left a comment •

edited

Loading