Bug 29157 - Cannot set date/date-time attributes to NULL
Summary: Cannot set date/date-time attributes to NULL
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Joonas Kylmälä
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-10-01 23:03 UTC by Eric Phetteplace
Modified: 2021-10-26 01:04 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
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 (1.01 KB, patch)
2021-10-01 23:03 UTC, Eric Phetteplace
Details | Diff | Splinter Review
Bug 29157: Regression tests (1.13 KB, patch)
2021-10-07 18:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29157: Date and Datetime attributes cannot be set NULL (947 bytes, patch)
2021-10-07 18:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29157: Regression tests (1.18 KB, patch)
2021-10-09 20:14 UTC, David Nind
Details | Diff | Splinter Review
Bug 29157: Date and Datetime attributes cannot be set NULL (996 bytes, patch)
2021-10-09 20:14 UTC, David Nind
Details | Diff | Splinter Review
Bug 29157: Regression tests (6.77 KB, patch)
2021-10-12 20:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29157: Date and Datetime attributes cannot be set NULL (1.45 KB, patch)
2021-10-12 20:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29157: Regression tests (6.82 KB, patch)
2021-10-13 15:28 UTC, Eric Phetteplace
Details | Diff | Splinter Review
Bug 29157: Date and Datetime attributes cannot be set NULL (1.50 KB, patch)
2021-10-13 15:29 UTC, Eric Phetteplace
Details | Diff | Splinter Review
Bug 29157: Regression tests (6.97 KB, patch)
2021-10-13 20:59 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29157: Date and Datetime attributes cannot be set NULL (1.64 KB, patch)
2021-10-13 21:00 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.