Bug 29157

Summary: Cannot set date/date-time attributes to NULL
Product: Koha Reporter: Eric Phetteplace <ephetteplace>
Component: REST APIAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Joonas Kylmälä <joonas.kylmala>
Severity: normal    
Priority: P5 - low CC: david, fridolin.somers, jonathan.druart, joonas.kylmala, katrin.fischer, kyle, martin.renvoize, tomascohen, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28585
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.05,20.11.11
Attachments: diff of patron record after REST PUT action
Bug 29157: Regression tests
Bug 29157: Date and Datetime attributes cannot be set NULL
Bug 29157: Regression tests
Bug 29157: Date and Datetime attributes cannot be set NULL
Bug 29157: Regression tests
Bug 29157: Date and Datetime attributes cannot be set NULL
Bug 29157: Regression tests
Bug 29157: Date and Datetime attributes cannot be set NULL
Bug 29157: Regression tests
Bug 29157: Date and Datetime attributes cannot be set NULL

Description Eric Phetteplace 2021-10-01 23:03:05 UTC
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.
Comment 1 Eric Phetteplace 2021-10-01 23:43:10 UTC
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.
Comment 2 Tomás Cohen Arazi 2021-10-03 23:13:32 UTC
I confirm the bug exists. Will take a look tomorrow. It affect date/date-time attributes.
Comment 3 Tomás Cohen Arazi 2021-10-07 18:42:58 UTC
Created attachment 125909 [details] [review]
Bug 29157: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Tomás Cohen Arazi 2021-10-07 18:43:02 UTC
Created attachment 125910 [details] [review]
Bug 29157: Date and Datetime attributes cannot be set NULL

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 David Nind 2021-10-09 20:14:23 UTC
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>
Comment 6 David Nind 2021-10-09 20:14:28 UTC
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>
Comment 7 David Nind 2021-10-09 20:16:46 UTC
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.
Comment 8 Tomás Cohen Arazi 2021-10-09 20:26:08 UTC
(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!
Comment 9 Joonas Kylmälä 2021-10-10 09:37:24 UTC
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.
Comment 10 Tomás Cohen Arazi 2021-10-12 16:48:40 UTC
(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.
Comment 11 Tomás Cohen Arazi 2021-10-12 20:43:35 UTC
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>
Comment 12 Tomás Cohen Arazi 2021-10-12 20:43:40 UTC
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>
Comment 13 Eric Phetteplace 2021-10-13 15:28:26 UTC
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!
Comment 14 Eric Phetteplace 2021-10-13 15:29:22 UTC
Created attachment 126221 [details] [review]
Bug 29157: Date and Datetime attributes cannot be set NULL

Signing off on the other patch.
Comment 15 Joonas Kylmälä 2021-10-13 20:59:59 UTC
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>
Comment 16 Joonas Kylmälä 2021-10-13 21:00:03 UTC
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>
Comment 17 Jonathan Druart 2021-10-15 09:29:38 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 18 Kyle M Hall 2021-10-22 15:01:35 UTC
Pushed to 21.05.x for 21.05.05
Comment 19 Fridolin Somers 2021-10-23 01:46:13 UTC
Pushed to 20.11.x for 20.11.11
Comment 20 Victor Grousset/tuxayo 2021-10-26 01:04:04 UTC
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.