Skip to content

LG 6201 audit/implement logs#6257

Merged
nprimak merged 3 commits intomainfrom
LG-6201/nprimak-audit-implement-logs
May 2, 2022
Merged

LG 6201 audit/implement logs#6257
nprimak merged 3 commits intomainfrom
LG-6201/nprimak-audit-implement-logs

Conversation

@nprimak
Copy link
Contributor

@nprimak nprimak commented Apr 26, 2022

LG-6201

What
Adding logs for personal_key step based on event log documentation in Figma
Also adding test coverage for all logging events.

Why
We need to add logging to the personal key step to match the existing logging in production.

Testing Instructions

  1. In application.yml.default, set idv_api_enabled to true
  2. Go through initial steps to create new account (set up password, click link in email) on localhost
  3. Navigate to localhost:3000/verify/v2
  4. Open dev tools Network tab, check that logger event from Figma doc is called (personal key visited)
  5. Click on download button, check that logger event (download personal key) is called
  6. Click on clipboard button, check that logger event (copy personal key) is called
  7. Click on print button, check that logger event (print personal key) is called
  8. Click continue button, check that logger event (show personal key modal) is called
  9. Click back button, check that logger event (hide personal key modal) is called
  10. Enter correct personal key (0000-0000-0000-0000) into input, press submit
  11. Check that logger event (personal key submitted) is called

@aduth
Copy link
Contributor

aduth commented Apr 27, 2022

This reminds me that we had actually implemented a React-based analytics context for document capture, which could be another solution for stubbing trackEvent. The way it works isn't currently very compatible with trackEvent, and it's specific to document-capture right now, but both of those things should probably be refactored anyways.

https://github.com/18F/identity-idp/blob/main/app/javascript/packages/document-capture/context/analytics.jsx

That being said, if this way of spying on trackEvent works, it feels pretty elegant 👍

import * as analytics from '@18f/identity-analytics';
sandbox.spy(analytics, 'trackEvent');

@nprimak
Copy link
Contributor Author

nprimak commented Apr 27, 2022

Could you share the error you were seeing? The way this was implemented was meant to work around the fact that TypeScript won't expect msSaveBlob to exist on navigator by default, but can create a type that extends the default navigator with the function, and check for it in hasProprietarySaveBlob.

Now the error isn't showing up anymore 😅 I guess it automagically fixed itself?

@nprimak nprimak marked this pull request as ready for review April 28, 2022 15:53
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason removing the basePath was the only thing I could figure out to do to finally get this test to pass (it would get stuck on the personal_key step and fail to get the continue button element, so it was unable proceed to the next step) . Of course once I removed basePath I got a typecheck error so I had to update the type definition as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like some unrelated commits got into the branch. Could you remove those? (e.g. rebase)

Copy link
Contributor

Choose a reason for hiding this comment

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

We could do it here or in a separate pull request, but I think for LG-6201 to be complete, we need to figure out how to avoid prefixing for these events, since otherwise we don't have strict parity with how it was working previously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lets do it in another pull request

changelog: Upcoming Features, Identity Verification, add logging
@aduth aduth force-pushed the LG-6201/nprimak-audit-implement-logs branch from 3a2a32c to 5b3e855 Compare May 2, 2022 16:26
aduth and others added 2 commits May 2, 2022 12:30
Co-Authored-By: Nadya Primak <nprimak@pluribusdigital.com>
Co-Authored-By: Nadya Primak <nprimak@pluribusdigital.com>
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 👍

@nprimak nprimak merged commit a34c459 into main May 2, 2022
@nprimak nprimak deleted the LG-6201/nprimak-audit-implement-logs branch May 2, 2022 18:01
peggles2 pushed a commit that referenced this pull request May 3, 2022
* LG-6201: Audit and implement missing event logs

changelog: Upcoming Features, Identity Verification, add logging

* Use getByText to get Continue button

Co-Authored-By: Nadya Primak <nprimak@pluribusdigital.com>

* Remove extra space

Co-Authored-By: Nadya Primak <nprimak@pluribusdigital.com>

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
peggles2 pushed a commit that referenced this pull request May 5, 2022
* LG-6201: Audit and implement missing event logs

changelog: Upcoming Features, Identity Verification, add logging

* Use getByText to get Continue button

Co-Authored-By: Nadya Primak <nprimak@pluribusdigital.com>

* Remove extra space

Co-Authored-By: Nadya Primak <nprimak@pluribusdigital.com>

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
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