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

MPDX-8022 - Fix Send Newsletter: Contact Status Display Value #977

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

wrandall22
Copy link
Contributor

Description

This PR updates the display of contact status to the friendly name instead of the enum id.

Checklist:

  • I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
  • I have applied the appropriate labels. (Add the label "On Staging" to get the branch automatically merged into staging.)
  • I have requested a review from another person on the project

@wrandall22 wrandall22 added the Preview Environment Add this label to create an Amplify Preview label Jul 12, 2024
@wrandall22 wrandall22 requested a review from dr-bizz July 12, 2024 14:00
Copy link
Contributor

@wrandall22
Copy link
Contributor Author

I tried using Ergonomock for the test here, but I couldn't get it to use my value overrides on contact status, so I just mocked the hook instead.

Copy link
Contributor

Bundle sizes [mpdx-react]

Compared against 6764d70

No significant changes found

Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

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

This looks great. The only update I see needing to be done is you could use the UseApiConstants hook to grab the constants here instead of adding constants to your query. src/components/Constants/UseApiConstants.tsx

Comment on lines +23 to +25
constant {
...Status
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use the UseApiConstants hook to grab the constants here instead of adding constants to your query. src/components/Constants/UseApiConstants.tsx

Copy link
Contributor Author

@wrandall22 wrandall22 Jul 12, 2024

Choose a reason for hiding this comment

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

I thought about doing that. The reason I didn't was performance. If I understand GraphQL correctly, this should grab only the statuses from the API (which probably takes it from the database) while not requesting the rest of the constants, thus saving 12 more database calls on the back-end. This way of doing it would also mean 1 HTTP request instead of 2. Perhaps it is not worth saving that processing?

Copy link
Contributor

@dr-bizz dr-bizz Jul 12, 2024

Choose a reason for hiding this comment

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

That is correct. It would be faster to just grab just the statuses, unless UseApiConstants has been called before causing it to be cached. Otherwise, you're correct.

It's hard to know if it's been called before, so I'm good with you leaving it the way it is if you want.

@dr-bizz
Copy link
Contributor

dr-bizz commented Jul 12, 2024

When you start using the useApiConstants hook, that should help your CodeCov score. If it's not totally passing. I think you might be okay with merging it as you've tested the main function of the code.

@dr-bizz dr-bizz self-requested a review July 12, 2024 20:15
@dr-bizz
Copy link
Contributor

dr-bizz commented Jul 12, 2024

Approving to not be a blocker

@wrandall22 wrandall22 merged commit e77bde9 into main Jul 15, 2024
18 of 19 checks passed
@wrandall22 wrandall22 deleted the MPDX-8022-fix-newsletter-status branch July 15, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Preview Environment Add this label to create an Amplify Preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants