Skip to content

LG-10670: use different locations for troubleshooting links.#9014

Merged
dawei-nava merged 4 commits intomainfrom
dwang/lg-10670-doc-capture-handbook-link
Aug 21, 2023
Merged

LG-10670: use different locations for troubleshooting links.#9014
dawei-nava merged 4 commits intomainfrom
dwang/lg-10670-doc-capture-handbook-link

Conversation

@dawei-nava
Copy link
Contributor

@dawei-nava dawei-nava commented Aug 16, 2023

🎫 Ticket

LG-10670

🛠 Summary of changes

When clicking troubleshooting links on the document step, warning screen, review issues screen will generate analytics events with following properties:
name: 'External Direct'
properties.event_properties.step='document_capture'

And different properties.event_properties.location properties:
document_capture, post_submission_warning, post_submission_review

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Step 1: Login and go through doc auth agreement page
  • Step 2: Go to doc capture page, open new tab for both links in the troubleshooting section.
  • Step 3: Failed doc auth and redirected to warning screen, open new tab for both links in the troubleshooting section.
  • Step 4: Try again and directed to review screen, open new tab for both links in the troubleshooting section.
  • Step 5: query event log with provided properties as filter and verify events logged with corresponding location.

👀 Screenshots

If relevant, include a screenshot or screen capture of the changes.

Before:
After:

Document capture:
AddPhotoEvents

Warning:
WarningPageEvents

Reviewing:
ReviewPageEvents

@dawei-nava dawei-nava requested review from a team and night-jellyfish and removed request for a team August 16, 2023 15:46
@dawei-nava dawei-nava marked this pull request as ready for review August 16, 2023 16:55
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//

Copy link
Contributor

Choose a reason for hiding this comment

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

I do wonder if maybe some of these should be separate it blocks, as they seem pretty long for one test (and comments like this in particular feel like an it block).

But, I am less familiar with React and testing in React so maybe the length is necessary. It might be good to have someone more familiar with React to review the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kind of feels having some duplicate testing verification code, unfortunately did not find something like Rspec behaves like reusables.

For it blocks, all this verification is on the screen controlled by 2/3 flags, it just having quite a lot ui elements rendered.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi! I'll tag the @18F/identity-frontend group to do a review and get their opinion about the tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Update: I can't actually select @18F/identity-frontend as a reviewer. Not sure why. I'll investigate after my meeting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Couple ideas for shortening / making reusable:

  • Where we're not expecting content to exist, one assertion should suffice, rather than checking the heading, prompt, and button? i.e. if they're all statically rendered via InPersonCallToAction , checking one should be enough.
    • It's not really well supported in our toolchain, but this is where something like shallow-rendering and checking for the rendered existence of some child component would be helpful (e.g. Enzyme shallow rendering with Component finder)
  • Create functions to either assert, render common elements, or add common test cases
  • Have a single spec test the exhaustive set of content expected, then each variation only needs to a small part of that, if it's expected that the content would come together

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aduth good suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aduth refactored the test with reusable functions.

Copy link
Contributor

@night-jellyfish night-jellyfish left a comment

Choose a reason for hiding this comment

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

I followed the steps and tested the branch locally, and saw the expected change. 🎉

Screenshot 2023-08-16 at 5 22 54 PM

However, as I said in a comment, I do think it might be beneficial to have someone more familiar with React / JS testing to review the new specs before merging.

@dawei-nava dawei-nava force-pushed the dwang/lg-10670-doc-capture-handbook-link branch from d28eded to 8469b87 Compare August 18, 2023 19:56
@dawei-nava dawei-nava merged commit 0a15e75 into main Aug 21, 2023
@dawei-nava dawei-nava deleted the dwang/lg-10670-doc-capture-handbook-link branch August 21, 2023 14:56
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