Skip to content
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

[CST] Override the display name and status of "PMR Pending" requests. #32256

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LouisFettet
Copy link
Contributor

@LouisFettet LouisFettet commented Oct 3, 2024

Summary

The change here for the end-user is very simple:

  1. Instead of showing PMR Pending requests as first-party requests, we show them as third-party requests.
  2. Instead of labeling these requests "PMR Pending", we show "Private Medical Record".

The changes under the hood are:

  • Clean up and add transform logic to the serialize reducer.
    • Move around some of the existing code in the reducer so that all the logic in the serializeClaim function uses other functions. These changes should be aesthetic only and shouldn't impact the result of the function.
    • Replace display names upon receiving them from the Lighthouse API, before they're ever passed to containers or components.
    • Replace the status of PMR Pending requests in the same way. This logic is wrapped in a <tempfix> comment block to make it clear that this is not a permanent solution to the problem.
  • Create a helper function to replace the display names of tracked items. Currently it only supports replacing "PMR Pending" with "Private Medical Record", but we know there are likely some other types of requests that we'll want to make more human-readable in the future as well.
  • Added tests to cover all changes.

Related issue(s)

Testing done

@jerekshoe added a PMR Pending tracked item on claim 600555011 on user 23 (Kyle Cole) in staging.

  • On the Claim Details page:
    • On the Status tab:
      • There should no longer be a "warning" alert under "What you need to do"
      • The tracked item under recent activity should show an "information" alert and use the new display name.
    • On the Files tab, the "warning" alert should be replaced with an "information" alert and the new display name is used.
  • On the Document Request page, the "optional" text is rendered in the description and the new display name is used.

It's possible that this tracked item appears on other pages that I'm unaware of, but the idea is that it should render properly as a third-party request with the updated display name across the app (since we're modifying it as soon as we receive data from the Lighthouse API).

A search for PMR Pending of the app returned no results, so it doesn't seem like there is any existing logic depending on the original display name or status (which should make this a safe change).

Screenshots

Before After
Claim status - What you need to do Screenshot 2024-10-03 at 1 35 38 PM Screenshot 2024-10-03 at 1 36 03 PM
Claim status - Recent activity Screenshot 2024-10-03 at 1 36 40 PM Screenshot 2024-10-03 at 1 37 09 PM
Claim files - Additional evidence Screenshot 2024-10-03 at 2 18 09 PM Screenshot 2024-10-03 at 2 18 45 PM
Document request Screenshot 2024-10-03 at 1 37 41 PM Screenshot 2024-10-03 at 1 38 13 PM

What areas of the site does it impact?

Claim Status Tool

Acceptance criteria

Quality Assurance & Testing

  • I fixed|updated|added unit tests and integration tests for each feature (if applicable).
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
  • Linting warnings have been addressed
  • Documentation has been updated
    • Not necessary.
  • Screenshot of the developed feature is added
  • Accessibility testing has been performed
    • Not necessary as only the data has changed (not any of the rendering logic).

Error Handling

  • Browser console contains no warnings or errors.
  • Events are being sent to the appropriate logging solution
    • Not necessary.
  • Feature/bug has a monitor built into Datadog or Grafana (if applicable)
    • Not necessary.

Authentication

  • Did you login to a local build and verify all authenticated routes work as expected with a test user
    • Not necessary.

Requested Feedback

See self-review!

Signed-off-by: Louis Fettet <louis@adhocteam.us>
Copy link
Contributor Author

@LouisFettet LouisFettet left a comment

Choose a reason for hiding this comment

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

Self-review!

Comment on lines -33 to -39
return items.map(item => {
const newItem = { ...item };
const associatedDocs = getDocsAssociatedWithTrackedItem(newItem, docs);
newItem.documents = associatedDocs || [];
newItem.date = getTrackedItemDate(item);
return newItem;
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleaned this up a little to make it more concise. Notable changes are:

  1. I removed the || [] for assigning documents, since I don't believe we can get here with a falsy return value for getDocsAssoociatedWithTrackedItems (since docs need to be an array already to run filter on them). For additional safety, I went ahead and added a default value (empty array) for both supportingDocuments and trackedItems at the beginning of the serializeClaim function.
  2. I moved setting date on the tracked item into the transform function, as I don't believe it's related to associating docs with the tracked item.

Comment on lines +45 to +49
const transformUnassociatedDocs = docs =>
docs.map(doc => ({
...doc,
date: doc.uploadDate,
}));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was moved into its own function from the original return of serializeClaim.

const transformAssociatedTrackedItems = items =>
items.map(item => ({
...item,
// <tempfix>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have an established pattern I should use here for marking code as temporary? Ideally this would be some sort of consistent token we can search for during clean-up work.

Comment on lines +97 to +98
supportingDocuments: transformedUnassociatedDocuments,
trackedItems: transformedAssociatedTrackedItems,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These variables are a bit wordy; if we don't find the labeling helpful, we can just run the relevant functions like so:

      supportingDocuments: transformUnassociatedDocs(
        filterAssociatedDocs(supportingDocuments),
      ),
      trackedItems: transformAssociatedTrackedItems(
        associatedTrackedItems,
      ),

The result is the same regardless.

Comment on lines +156 to +198
// <tempfix>
// See notes on PMR Pending in serialize.js
it('should manually override the status of PMR Pending requests', () => {
const claim = {
id: 1,
type: 'claim',
attributes: {
trackedItems: [
{
id: 1,
displayName: 'PMR Pending',
status: 'NEEDED_FROM_YOU',
},
],
},
};
const serializedClaim = serializeClaim(claim);
const trackedItem1 = serializedClaim.attributes.trackedItems.find(
d => d.id === 1,
);
expect(trackedItem1.status).to.eql('NEEDED_FROM_OTHERS');
});
// </tempfix>

it('should replace the display name of some types of tracked items', () => {
const claim = {
id: 1,
type: 'claim',
attributes: {
trackedItems: [
{
id: 1,
displayName: 'PMR Pending',
},
],
},
};
const serializedClaim = serializeClaim(claim);
const trackedItem1 = serializedClaim.attributes.trackedItems.find(
d => d.id === 1,
);
expect(trackedItem1.displayName).to.eql('Private Medical Record');
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests are basically doing the same thing, but one tests the status while the other tests the displayName. Since testing the status is temporary, I kept them separate so that we can just remove that whole test case down the road.

@LouisFettet LouisFettet marked this pull request as ready for review October 3, 2024 20:21
@LouisFettet LouisFettet requested review from a team as code owners October 3, 2024 20:21
@jacobworrell
Copy link

@LouisFettet looks good!

Copy link
Contributor

@rmessina1010 rmessina1010 left a comment

Choose a reason for hiding this comment

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

LGTM

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