-
-
Notifications
You must be signed in to change notification settings - Fork 492
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
Validate issued_at field to prevent future dates further than 1 year #4749
Validate issued_at field to prevent future dates further than 1 year #4749
Conversation
f6e3073
to
02b8223
Compare
@tatheerf02 Let's change that from "Cannot be further than 1 year" to "cannot be more than 1 year in the future". |
02b8223
to
53d4286
Compare
@cielf I've changed the error message and updated the screenshots in the description 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from a functional pov; I found an unrelated issue (which is also in main).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. The calendar spec had to change because it had a year too far in the future, which would prevent the object from saving now.
@tatheerf02: Your PR |
Resolves #4742
Description
Add a new validation to the
issued_at
field which is present on the donation, distribution, and purchase models. The new validation ensures that the field can not be set to a date that is further than one year into the future, as this causes unexpected behaviour when the models are later filtered.An alternative solution could be to add this validation to the models themselves, but putting it in the centralized location was an existing pattern and made more sense.
Tradeoff: Are there valid use cases where we might want to have set this date to further than one year ahead?
Type of change
How Has This Been Tested?
Added new unit tests following existing patterns for all affected models and ensured they passed
Screenshots
Note: Although not pictured, I also tested that the validation is run when the date on a purchase, donation, or distribution is edited after creation.
📸 Validation error shown on new donation
📸 Validation error shown on new purchase
📸 Validation error shown on new distribution