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.

Sign up for GitHub

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

Jump to bottom

dan/6291 Unknown Fields #6683

Merged
merged 4 commits into from
Oct 11, 2023
Merged

dan/6291 Unknown Fields #6683

merged 4 commits into from
Oct 11, 2023

Conversation

DanielSass
Copy link
Collaborator

@DanielSass DanielSass commented Oct 2, 2023 •
edited
Loading

BACKEND PULL REQUEST

Related Issue

Changes Proposed

Mostly frontend changes, with a few necessary backend updates:

  • Update csv + fhir exports to default unknown fields per the linked requirements
  • Update resolvers / data handlers to account for empty phone number lists

FRONTEND PULL REQUEST

Related Issue

Changes Proposed

  • Add checkboxes on the person form for address + phone number to account for unknown values.
  • Conditional validation based on the new checkboxes.

Testing

  • Add new patient, edit patient and self registration are the relevant screens I'm aware of.

Screenshots / Demos

  • todo

@DanielSass DanielSass marked this pull request as ready for review October 3, 2023 15:14
@DanielSass DanielSass force-pushed the dan/6291-unknown-fields-fix branch from 1f540da to 9ac4e8c Compare October 4, 2023 14:15
johanna-skylight
johanna-skylight reviewed Oct 4, 2023
View reviewed changes
Copy link
Contributor

@johanna-skylight johanna-skylight left a comment

Choose a reason for hiding this comment

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

I tested this in local and it worked as expected 💯 . I left some comments. Thanks!

frontend/src/app/patients/Components/PersonForm.tsx Outdated
onPersonChange("unknownAddress")(e.target.checked);
}}
/>
{!patient.unknownAddress && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this block of code be moved to its own component?. Similar how the phone number is its own component.

frontend/src/app/patients/Components/ManageEmails.tsx Outdated
@@ -134,7 +134,7 @@ const ManageEmails: React.FC<Props> = ({
<TextInput
name={`email-${idx}`}
idString={`email-${idx}`}
className="flex-fill emailFormElement"
className="flex-fill emailFormElement, margin-top-0"
Copy link
Contributor

Choose a reason for hiding this comment

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

The , should not be part of the class attribute

@DanielSass DanielSass force-pushed the dan/6291-unknown-fields-fix branch from a7155e5 to 7597e7c Compare October 5, 2023 14:29
@DanielSass
unknown address
255548c 
unknown phone
bugfix
additional tests
linting fixes

removing act usage per johanna's work
@DanielSass DanielSass force-pushed the dan/6291-unknown-fields-fix branch from 7597e7c to 255548c Compare October 5, 2023 14:34
@nathancrtr
Copy link
Contributor

Do we care that selecting and un-selecting the "unknown" checkbox will clear out the values that were there before?

Screen.Recording.2023-10-06.at.3.50.34.PM.mov

@nathancrtr
Copy link
Contributor

Left a couple of minor comments but this looks really good! Everything works as expected running on my local machine. I know wrestling with the yup validation schemas isn't easy but your changes are clean and easy to follow 👍

@DanielSass
Copy link
Collaborator Author

Do we care that selecting and un-selecting the "unknown" checkbox will clear out the values that were there before?

Yeah obviously this is what made sense to me, but I don't feel strongly either way.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

85.4% 85.4% Coverage
0.0% 0.0% Duplication

johanna-skylight
johanna-skylight approved these changes Oct 10, 2023
View reviewed changes
Copy link
Contributor

@johanna-skylight johanna-skylight left a comment

Choose a reason for hiding this comment

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

Looks good!

nathancrtr
nathancrtr approved these changes Oct 10, 2023
View reviewed changes
Copy link
Contributor

@nathancrtr nathancrtr left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for this one, I know wrangling with the FE validation isn't easy but this feels like a clean solution.

@DanielSass DanielSass added this pull request to the merge queue Oct 11, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 11, 2023
@DanielSass DanielSass added this pull request to the merge queue Oct 11, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 11, 2023
@DanielSass DanielSass added this pull request to the merge queue Oct 11, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 11, 2023
@DanielSass DanielSass added this pull request to the merge queue Oct 11, 2023
Merged via the queue into main with commit f5fc5ec Oct 11, 2023
33 checks passed
@DanielSass DanielSass deleted the dan/6291-unknown-fields-fix branch October 11, 2023 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johanna-skylight johanna-skylight johanna-skylight approved these changes

@nathancrtr nathancrtr nathancrtr approved these changes

@zdeveloper zdeveloper Awaiting requested review from zdeveloper

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DanielSass @nathancrtr @johanna-skylight