Fix unexpected issued_at behavior when date field empty. #4849
Conversation
coalest
force-pushed
the
4313-bug-you-can-enter-an-issued_at-date-without-a-time-hijinks-ensue
branch
from
30510be to
5507b87
Compare
Previously before_save and before_create callbacks set the value for issued_at created_at end of day if no datetime was provided. This prevented records being saved with no issue date. But it caused unexpected behavior when creating or updating a donation/purchase/distribution with an empty issued_at field, silently changing the issued_at date to something potentially incorrect.
coalest
force-pushed
the
4313-bug-you-can-enter-an-issued_at-date-without-a-time-hijinks-ensue
branch
from
5507b87 to
9f306ac
Compare
| def issued_at_cannot_be_before_2000 | ||
| if issued_at.present? && issued_at < Date.new(2000, 1, 1) | ||
| errors.add(:issued_at, "Cannot be before 2000") | ||
| errors.add(:issued_at, "cannot be before 2000") |
There was a problem hiding this comment.
Purely cosmetic change.
Error message before: "Issue date Cannot be before 2000"
Error message after: "Issue date cannot be before 2000"
|
Pushed another commit that seems unrelated, but is kinda related. If you edit a distribution and the validation fails, the partner dropdown gets messed up.
(Yes and No appear as only options instead of partner names) |
|
Hey @coalest -- as a matter of course, I'd prefer if you kept the changes on a PR tight to the issue you have claimed on a go-forward basis. Thanks. (It just makes the testing simpler) |
|
Well, this prevents the issue, so LGTM. Over to @dorner. (though, noting that there are conficts to be resolved) |
…hout-a-time-hijinks-ensue
|
Fixed the merge confict, so ready for review again |
|
Still LGTM |
| @@ -18,6 +18,7 @@ | |||
| organization_name: organization.id, | |||
| distribution: { | |||
| partner_id: partner.id, | |||
| issued_at: Date.yesterday, | |||
There was a problem hiding this comment.
Why do we need these specifications when you added it to the factory already?
Or should yesterday be the default?
There was a problem hiding this comment.
I added values to the factories so creating these records will always have an issued_at field, but here these are parameters for a request. If I omit this parameter we are trying to create a distribution with no issued_at field which is now invalid after these changes.
When you go to create a new distribution (although same for purchase/donation), a default value for issued_at is displayed. I could add a default value for issued_at in the controller, but then we are in a similar place as before (someone emptying the field, clicking save, and unexpected behavior occurring).
If someone decides to empty the issue date field, I think it would be better to respond with a validation error and make them put in a date. But that's just an assumption on my part based on the issue description, so I can change if you think coercing missing issue dates into a default would be better.
The date yesterday is arbitrary here, it could be any valid date/time.
| @@ -211,8 +211,9 @@ | |||
| describe "POST #create" do | |||
| let!(:storage_location) { create(:storage_location, organization: organization) } | |||
| let!(:partner) { create(:partner, organization: organization) } | |||
| let(:issued_at) { Time.current } | |||
There was a problem hiding this comment.
Ditto here - we shouldn't need to specify this.
There was a problem hiding this comment.
Same as above. These are params for the request. issued_at now must be present. Unless we decide to coerce blank values into a default date/time. But if we do I think we need to communicate that to the user somehow.
|
@dorner By the way, is there any danger here in regards to event sourcing. Since this is a new validation that didn't exist for old events, is it possible that we might have past events with a |
|
@dorner Re-requesting a review. I didn't make any changes, just replied to your comments. Hopefully you have enough context now |
|
Thanks for the clarifications! |
|
@coalest: Your PR |
Resolves #4313
Description
Previously we had two callbacks on these models with an
issued_atfield. Thesebefore_saveandbefore_createcallbacks set the value forissued_atto be based oncreated_atifissued_atwas missing. This prevented records from being saved with no issue date. But it causes the unexpected behavior mentioned in the issue when creating or updating a donation/purchase/distribution with an emptyissued_atfield, silently changing theissued_atdate to something potentially incorrect.Type of change
How Has This Been Tested?
Added new request and service specs.
Screenshots
These date times are still filled in automatically with default values like before. The difference is that if a user manually deletes the date, an error will be shown like our other validation errors.
