Created attachment 125644 [details] [review] diff of patron record after REST PUT action I noticed when trying to update patrons with a Python script that Koha set everyone's birthday to the current date. For each patron, I sent a Python dict where patron['date_of_birth'] was None (Python's null value). I can send an ISO 8601 date and DOB updates appropriately but I cannot find a way to leave the date empty, which is desirable (we don't care about age). Here's what I tried: date_of_birth = '' (empty string) date_of_birth = "null" date_of_birth = "undef" All these receive the same formatting error response: 400 Response {'errors': [{'message': '/anyOf/0 Does not match date format.', 'path': '/body/date_of_birth'}], 'status': 400} As I said, setting birthdate to None (which translates to `null` in JSON) updates the DOB to the current date, which I don't think makes sense since it's a nullable field in the database. I'll attach a diff of a minimal test patron where I added a "1" to their cardnumber and did nothing else. The steps were: - create a minimal patron record (e.g. with email, userid, names) - HTTP GET $API/patrons/username=$USERNAME (I did not test HTTP GET $API/patrons/$BORROWERNUMBER but I assume it works the same) - update cardnumber e.g. patron['cardnumber'] = patron['cardnumber'] + 1 - PUT $API/patrons/$BORROWERNUMBER with json=patron A few fields other than cardnumber changed. Both last_seen and updated_on make sense, I find it odd date_renewed was set to the current date since I was not renewing the patron but that's at least not problematic in our use case. It's the DOB I'd like to avoid updating, though. Let me know if I can provide more example data.
Small update to this - looks like DOB is not one of the fields that can be bulk updated with /cgi-bin/koha/tools/modborrowers.pl either so that makes the problem more annoying. We can't bulk update a bunch of users and then erase all the DOB data afterwards. I know ByWater could run a SQL query for us but it just adds one extra step.
I confirm the bug exists. Will take a look tomorrow. It affect date/date-time attributes.
Created attachment 125909 [details] [review] Bug 29157: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 125910 [details] [review] Bug 29157: Date and Datetime attributes cannot be set NULL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 125994 [details] [review] Bug 29157: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 125995 [details] [review] Bug 29157: Date and Datetime attributes cannot be set NULL Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com>
Tested by: 1. Applying regression tests patch 2. Run tests - these should fail: prove t/db_dependent/api/v1/patrons.t 3. Apply second patch 4. Run tests again - they should now pass 5. Sign-off! Hope that is sufficient.
(In reply to David Nind from comment #7) > Tested by: > > 1. Applying regression tests patch > 2. Run tests - these should fail: prove t/db_dependent/api/v1/patrons.t > 3. Apply second patch > 4. Run tests again - they should now pass > 5. Sign-off! > > Hope that is sufficient. Thanks, I forgot to add *exactly* that!
The unit test is in the wrong place, it should test the attributes_from_api function directly, please add it to t/db_dependent/Koha/Object.t. Otherwise the fix looks good.
(In reply to Joonas Kylmälä from comment #9) > The unit test is in the wrong place, it should test the attributes_from_api > function directly, please add it to t/db_dependent/Koha/Object.t. Otherwise > the fix looks good. Fair enough. I will of course leave the tests I added first, so we can track breaking this in another place of the stack.
Created attachment 126128 [details] [review] Bug 29157: Regression tests This patch implements regression tests for the filed bug. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 126129 [details] [review] Bug 29157: Date and Datetime attributes cannot be set NULL This patch makes Koha::Object->attribute_from_api allow setting attributes the undef value. The original implementation passed the value directly to dt_from_string, which made the attribute be set the current date. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Object.t \ t/db_dependent/api/v1/patrons.t => FAIL: Tests fail! Our code is buggy! 3. Apply this patch 4. Repeat 2 => SUCCESS! Fix fixed this thing! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 126220 [details] [review] Bug 29157: Regression tests I don't think this needs a third sign-off but I wanted to give it a try. Looks good! Thanks for getting to this so quickly!
Created attachment 126221 [details] [review] Bug 29157: Date and Datetime attributes cannot be set NULL Signing off on the other patch.
Created attachment 126237 [details] [review] Bug 29157: Regression tests This patch implements regression tests for the filed bug. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Eric Phetteplace <phette23@gmail.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 126238 [details] [review] Bug 29157: Date and Datetime attributes cannot be set NULL This patch makes Koha::Object->attribute_from_api allow setting attributes the undef value. The original implementation passed the value directly to dt_from_string, which made the attribute be set the current date. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Object.t \ t/db_dependent/api/v1/patrons.t => FAIL: Tests fail! Our code is buggy! 3. Apply this patch 4. Repeat 2 => SUCCESS! Fix fixed this thing! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Eric Phetteplace <phette23@gmail.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Pushed to master for 21.11, thanks to everybody involved!
Pushed to 21.05.x for 21.05.05
Pushed to 20.11.x for 20.11.11
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.