Refactor test card

[mpbrown]

FRONTEND PULL REQUEST

Related Issue

• Refactor test card (COVID/flu) #6079

Changes Proposed

• Adds a new TestCard component used instead of the existing QueueItem if the testCardRefactorEnabled feature flag is active.
• TestCard uses a collapisble design which will display the TestCardForm as a child component when the card is expanded. TestCardForm is where the majority of the conduct tests business logic is used while TestCard primarily serves as the header and overall form container.
• Currently the test cards are expanded by default to make it an easier transition for users unfamiliar with the new collapsible design, but this should be updated in the future.

AOE

• Test questionnaire modal will no longer appear when starting a patient test from either the search bar or the patients page.
• AOE questions will now be shown directly on the TestCardForm. The COVID AOE questions are used by default, but other disease specific AOE form components can be swapped when additional supported diseases are added.
• Test result delivery preferences will no longer be included when saving AOE responses. The backend defaults to using whatever preferences the patient already has saved on their profile.

Test Results

• The input groups for marking test results no longer use their own internal submit button.
• The radio button options for results will now show as asterisk if it is required, such as when either some or all multiplex results are reburied for submission.
• Error messages for missing test results are shown after the user attempts to submit the form. The submit button is no longer disabled for this reason.

Timer

• Test timer design has been updated which will be present on both the new TestCard and existing QueueItem.

Additional Information

• TestCardForm uses useReducer to manage the form state instead of multiple useState hooks. This was chosen because some state properties depend on each other or on the previous state and encapsulating this state management in a reducer with discrete dispatch actions improves the testability and readability of the main TestCardForm.
• TestCardForm.utils contains some custom hooks and helper functions with the goal of balancing just enough abstraction to improve readability and understanding of how the test card works overall.

Testing

• Test out the new changes on dev7.
• Please specifically test out different states of the test card and try making fast changes or unexpected inputs to test the behavior of the form saving live changes and receiving any updates from the server.
  • Keep in mind that if someone else is testing on the same patient at the same time that may lead to unexpected but intentional behavior where an update from the server might overwrite a change you previously made.
• Unit test suite for the new TestCard and TestCardForm is still in progress.

Screenshots / Demos

Multiplex with all results

Multiplex missing results

Flu only device and custom backdate

Incomplete AOE questions confirmation modal before submitting

Invalid test date

Collapsed view

[mpbrown]

FYI on dev7 I'm experiencing an unexpected 403 forbidden error when I try answering an AOE question, but this might just be related to needing to update my user permissions for my logged in organization on dev7 since answering and saving AOE works on local deployment.

[nathancrtr]

I didn't see the ticket or design specify whether the test cards should default to the collapsed state or the expanded one. It looks like they're expanded by default? Just wanting to call this out to confirm 👍

[DanielSass]

I know I've had to disable this linting error before, but with how consistently we're ignoring it - is there a more correct way of doing this? Or can we update the linter to not complain about this if its not actually an issue?

[mpbrown]

Thanks for mentioning this! After I looked into it a bit more, the React team strongly recommends keeping the rule and reworking the code to handle it properly. I'm experimenting with the code and we may be able to remove some of these useEffects entirely, so I'll create a follow up PR for this.

Some of this effect code will benefit significantly when React officially releases the new useEffectEvent hook on a stable version. For example, we need to control for race conditions between the frontend saving edits and the periodic live sync with any updates from the backend.

When we get new values from the backend, we have to check whether the mutations are still loading and although those loading booleans are reactive values that React says should be included in the dependency array, we don't want those in the array because then the effect will be triggered whenever those change and potentially cause sync issues.

useEffect(() => {
    // don't update if there are unsaved dirty changes or if still awaiting saved edits
    if (state.dirty || editQueueItemMutationLoading || updateAoeMutationLoading)
      return;
    dispatch({
      type: TestFormActionCase.UPDATE_WITH_CHANGES_FROM_SERVER,
      payload: testOrder,
    });
    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, [testOrder]);

So once useEffectEvent is officially added, we'll be able to separate reactive values that are actual effect dependencies from ones that are only used for conditional logic.

[nathancrtr]

It doesn't feel important to correct this under this particular ticket, but just calling out a little wonky layout behavior:

This is in the case where a facility has its "default" device removed while a test card is open. And ditto, I guess, for doing the same with a device's default specimen type:

Again, really nitpicky and best looked at elsewhere, IMO, just shouting it out for now.

[zdeveloper]

It doesn't feel important to correct this under this particular ticket, but just calling out a little wonky layout behavior:

This is in the case where a facility has its "default" device removed while a test card is open. And ditto, I guess, for doing the same with a device's default specimen type: Again, really nitpicky and best looked at elsewhere, IMO, just shouting it out for now.

Yeah I agree, but this is a super edge case, especially with relying on automatic livdsync, I can see us never really deleting a device or specimen since we already corrected all of our devices info.

[emyl3]

Nice work, Mike! Left my first bunch of comments.

Do you have access to Deque's axe dev tool? I think it will be helpful to run particularly against multiple test cards in the conduct test page.

[emyl3]

can we pass in the patient's name so if there are multiple test timer buttons on a conduct page we know which person's test the timer is for?

[mpbrown]

I agree this should be added on the aria label, although I think we should create a separate ticket for it since it'll also affect the existing queue item component which will still be used in prod until the new test card feature flag is enabled

[mpbrown]

@emyl3 thanks for the reminder about the axe devtools extension! I pushed some updates that improve accessibility. Once the PR is approved, the design team will be conducting usability testing using the demo environment. The new test card feature flag will remain disabled in the prod environment

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

81.0% Coverage
2.8% Duplication

[emyl3]

Something we can also flag for design is marking the labels as required when we select "Mark test as inconclusive" is misleading

[DanielSass]

that's a good catch 🔍

[mpbrown]

We're likely going to remove that inconclusive checkbox altogether in favor of simplifying the result validation rules for multiplex devices. That change should be in the upcoming PR for supporting HIV on the test card

[emyl3]

Looks like the <Card> element is a <li> and needs to be nested under <ul>

[mpbrown]

Noticed this on the axe scan, but holding off on changing it until the follow-up PR because a few issues popped up with nesting the queue under a single ul so additional refactor and updating some tests would be needed

[emyl3]

flagged a couple more things but am approving since it sounds like we'll be doing more work on this later and we need to get this out for design/research soon! functionality-wise looks good