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

1281 update shipping info #1506

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

1281 update shipping info #1506

wants to merge 3 commits into from

Conversation

marySalvi
Copy link
Collaborator

@marySalvi marySalvi commented Jan 9, 2025

Closes #1281
Included in this PR

  • An additional user input regarding if the samples will be shipped (EMSL)
  • Adds the 'country' field to the shipping info
  • Makes only the country field required
  • Adds a tooltip explaining that the sender's info is needed and updates help text

@marySalvi marySalvi requested a review from naglepuff January 9, 2025 19:25
Copy link
Collaborator

@naglepuff naglepuff left a comment

Choose a reason for hiding this comment

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

Front end validation looks good to me, but there were a couple of interactions with saving/loading that should be addressed.

@@ -47,6 +47,7 @@ class NmcdAddress(BaseModel):
city: str
state: str
postalCode: str
country: str
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
country: str
country: str = ""

I came across a 500 status code when loading the submission landing page. Looks like there's some backend validation that is not happy.

Since country is a new required field, we need to stay backwards-compatible with existing submissions. There are a couple of options.

  1. We could have the serializer add a default value (empty string?)
  2. We could write a migration to add a default value

I don't mind sticking with option 1, but we also need to make sure that any consumers of this data are aware that country might be missing (I'm not sure how robust the submission to mongo pipeline is about this).

I also don't know what the default value should be. The empty string allows us to sneak existing submissions past the pydantic classes, but these submissions are still technically invalid. It's probably fine though.

@@ -110,6 +111,7 @@ const addressFormDefault = {
const contextFormDefault = {
dataGenerated: undefined as undefined | boolean,
awardDois: [] as string[] | null,
ship: undefined as undefined | boolean,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this also needs to be added to the pydantic class in schemas_submission.py. Otherwise users will have to select an option for this each time they open the submission.

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.

Update shipping information for EMSL samples
2 participants