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

MMT-3891: Existing country should be displayed in draft form. #1307

Merged
merged 2 commits into from
Oct 3, 2024
Merged

Conversation

htranho
Copy link
Contributor

@htranho htranho commented Oct 1, 2024

Overview

What is the feature?

Existing country in address form doesn't displayed in the draft form and only short form of country names are written to json.

What is the Solution?

Issues were that Country selector:

  1. When it writes to json, the short form of the country name is written ('US' instead of 'United States', 'TZ' instead of 'Tanzania, United Republic of')
  2. When importing into the form (Edit a Collection), values like 'United States' in address field 'Country' will not be displayed in the form.

Those issues are fixed:

  1. Long form of country name is to be written to json.
  2. When edit an existing record, both short and long form of country name will be read and displayed.

What areas of the application does this impact?

List impacted areas.

Country selector

Reproduction steps

  1. Create a new draft, long form of country name is written out to json.
  2. Import an existing collection with 'US' in address field, that should work.

Attachments

N/A

Checklist

  • I have added automated tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.66%. Comparing base (e7e5afa) to head (d2f04f0).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1307   +/-   ##
=======================================
  Coverage   97.65%   97.66%           
=======================================
  Files         362      362           
  Lines        5584     5601   +17     
  Branches     1172     1177    +5     
=======================================
+ Hits         5453     5470   +17     
  Misses        130      130           
  Partials        1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -84,7 +85,8 @@ const CustomCountrySelectWidget = ({
// Selects a country and propagates onChange
const selectCountry = (val) => {
setSelectedCountry(val)
onChange(val.value)
// Propagates the label (long form of country name)
onChange(val.label)
Copy link
Collaborator

Choose a reason for hiding this comment

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

did you confirm with Erich or someone that the long form is what is expected in the json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only Scott replied to my question (Asked Eric, Scott and Tyler): 'I am thinking long form. Do you all agree?'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tyler also think that long form makes sense

Copy link
Contributor

@mandyparson mandyparson left a comment

Choose a reason for hiding this comment

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

Need to figure out snyk issue

@htranho htranho merged commit 63c299d into main Oct 3, 2024
5 of 7 checks passed
@htranho htranho deleted the MMT-3891 branch October 3, 2024 20:39
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