Bug 27253 - borrowers.updated_on cannot be null on fresh install, but can be null with upgrade
Summary: borrowers.updated_on cannot be null on fresh install, but can be null with up...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Julian Maurice
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 10459
Blocks: 30486
  Show dependency treegraph
 
Reported: 2020-12-16 14:39 UTC by Kyle M Hall
Modified: 2023-06-08 22:26 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.06


Attachments
Bug 27253: Fix definition of updated_on in borrowers and deletedborrowers (4.11 KB, patch)
2022-04-01 07:37 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 27253: Fix definition of updated_on in borrowers and deletedborrowers (4.16 KB, patch)
2022-04-04 12:13 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 27253: Fix definition of updated_on in borrowers and deletedborrowers (4.25 KB, patch)
2022-04-15 07:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27253: (follow-up) Fix UNIXTIME(0) in db_revs/211200037.pl (2.39 KB, patch)
2022-04-25 10:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2020-12-16 14:39:58 UTC
Simply put, the borrowers table column updated_on in kohastructure.sql specifies  that it cannot be null, but the updatedatabase.pl it can. It appears the NOT got lost when the column as renamed 'updated_on' from 'timestamp' in a followup on bug 10459.

Fixing it is easy, the question is, what do we do with currently NULL updated_on's? My though is to set them to the unix epoch ( 1970-01-01 ).
Comment 1 Jonathan Druart 2020-12-21 10:28:17 UTC
What about MAX(date_renewed, dateenrolled, last_seen)?
Comment 2 Katrin Fischer 2021-01-21 20:15:05 UTC
(In reply to Jonathan Druart from comment #1)
> What about MAX(date_renewed, dateenrolled, last_seen)?

+1
Comment 3 Julian Maurice 2022-04-01 07:37:01 UTC
Created attachment 132819 [details] [review]
Bug 27253: Fix definition of updated_on in borrowers and deletedborrowers

Test plan:
1. First you have to be in a state where updated_on is NULL-able. You
   can do that by either:
   a) do a fresh install of Koha 16.05 and update to master, or
   b) execute the following SQL queries:

     ALTER TABLE borrowers MODIFY updated_on timestamp NULL
     DEFAULT current_timestamp() ON UPDATE current_timestamp()
     COMMENT 'time of last change could be useful for synchronization
     with external systems (among others)'

     ALTER TABLE deletedborrowers MODIFY updated_on timestamp NULL
     DEFAULT current_timestamp() ON UPDATE current_timestamp()
     COMMENT 'time of last change could be useful for synchronization
     with external systems (among others)'
2. Create two borrowers (let's name them X and Y)
3. Delete borrower Y
4. Set the column updated_on to NULL for both borrowers by executing the
   following SQL query:
   UPDATE borrowers SET updated_on = NULL WHERE borrowernumber =
   <borrowernumber of X>
   UPDATE deletedborrowers SET updated_on = NULL WHERE borrowernumber =
   <borrowernumber of Y>
5. Apply patch and run updatedatabase
6. Verify that borrowers.updated_on and deletedborrowers.updated_on are
   not NULL-able.
   Verify that updated_on for X and Y have taken the value of
   dateenrolled.
7. Repeat step 2 to 6 but this time renew the patron and/or log in with
   its account in order to set the columns borrowers.date_renewed and
   borrowers.lastseen before executing updatedatabase
   borrowers.updated_on should take the greatest value among
   dateenrolled, date_renewed, and lastseen
Comment 4 Owen Leonard 2022-04-04 12:13:14 UTC
Created attachment 132934 [details] [review]
Bug 27253: Fix definition of updated_on in borrowers and deletedborrowers

Test plan:
1. First you have to be in a state where updated_on is NULL-able. You
   can do that by either:
   a) do a fresh install of Koha 16.05 and update to master, or
   b) execute the following SQL queries:

     ALTER TABLE borrowers MODIFY updated_on timestamp NULL
     DEFAULT current_timestamp() ON UPDATE current_timestamp()
     COMMENT 'time of last change could be useful for synchronization
     with external systems (among others)'

     ALTER TABLE deletedborrowers MODIFY updated_on timestamp NULL
     DEFAULT current_timestamp() ON UPDATE current_timestamp()
     COMMENT 'time of last change could be useful for synchronization
     with external systems (among others)'
2. Create two borrowers (let's name them X and Y)
3. Delete borrower Y
4. Set the column updated_on to NULL for both borrowers by executing the
   following SQL query:
   UPDATE borrowers SET updated_on = NULL WHERE borrowernumber =
   <borrowernumber of X>
   UPDATE deletedborrowers SET updated_on = NULL WHERE borrowernumber =
   <borrowernumber of Y>
5. Apply patch and run updatedatabase
6. Verify that borrowers.updated_on and deletedborrowers.updated_on are
   not NULL-able.
   Verify that updated_on for X and Y have taken the value of
   dateenrolled.
7. Repeat step 2 to 6 but this time renew the patron and/or log in with
   its account in order to set the columns borrowers.date_renewed and
   borrowers.lastseen before executing updatedatabase
   borrowers.updated_on should take the greatest value among
   dateenrolled, date_renewed, and lastseen

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 5 Marcel de Rooy 2022-04-15 07:31:31 UTC
QA: Looking here
Comment 6 Marcel de Rooy 2022-04-15 07:34:42 UTC
(In reply to Kyle M Hall from comment #0)
> Simply put, the borrowers table column updated_on in kohastructure.sql
> specifies  that it cannot be null, but the updatedatabase.pl it can. It
> appears the NOT got lost when the column as renamed 'updated_on' from
> 'timestamp' in a followup on bug 10459.
> 
> Fixing it is easy, the question is, what do we do with currently NULL
> updated_on's? My though is to set them to the unix epoch ( 1970-01-01 ).

(In reply to Jonathan Druart from comment #1)
> What about MAX(date_renewed, dateenrolled, last_seen)?

I think it is an arbitrary choice.
For my part NOW() would have been good either (and much easier). After all, we are updating those borrowers .. now, right?
Comment 7 Marcel de Rooy 2022-04-15 07:37:57 UTC
$DBversion = "16.06.00.002";
if ( CheckVersion($DBversion) ) {
    unless ( column_exists('borrowers', 'updated_on') ) {
        $dbh->do(q{
            ALTER TABLE borrowers
                ADD COLUMN updated_on timestamp NULL DEFAULT CURRENT_TIMESTAMP
                ON UPDATE CURRENT_TIMESTAMP
                AFTER privacy_guarantor_checkouts;
        });
        $dbh->do(q{
            ALTER TABLE deletedborrowers
                ADD COLUMN updated_on timestamp NULL DEFAULT CURRENT_TIMESTAMP
                ON UPDATE CURRENT_TIMESTAMP
                AFTER privacy_guarantor_checkouts;
Comment 8 Marcel de Rooy 2022-04-15 07:39:56 UTC
Created attachment 133342 [details] [review]
Bug 27253: Fix definition of updated_on in borrowers and deletedborrowers

Test plan:
1. First you have to be in a state where updated_on is NULL-able. You
   can do that by either:
   a) do a fresh install of Koha 16.05 and update to master, or
   b) execute the following SQL queries:

     ALTER TABLE borrowers MODIFY updated_on timestamp NULL
     DEFAULT current_timestamp() ON UPDATE current_timestamp()
     COMMENT 'time of last change could be useful for synchronization
     with external systems (among others)'

     ALTER TABLE deletedborrowers MODIFY updated_on timestamp NULL
     DEFAULT current_timestamp() ON UPDATE current_timestamp()
     COMMENT 'time of last change could be useful for synchronization
     with external systems (among others)'
2. Create two borrowers (let's name them X and Y)
3. Delete borrower Y
4. Set the column updated_on to NULL for both borrowers by executing the
   following SQL query:
   UPDATE borrowers SET updated_on = NULL WHERE borrowernumber =
   <borrowernumber of X>
   UPDATE deletedborrowers SET updated_on = NULL WHERE borrowernumber =
   <borrowernumber of Y>
5. Apply patch and run updatedatabase
6. Verify that borrowers.updated_on and deletedborrowers.updated_on are
   not NULL-able.
   Verify that updated_on for X and Y have taken the value of
   dateenrolled.
7. Repeat step 2 to 6 but this time renew the patron and/or log in with
   its account in order to set the columns borrowers.date_renewed and
   borrowers.lastseen before executing updatedatabase
   borrowers.updated_on should take the greatest value among
   dateenrolled, date_renewed, and lastseen

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Jonathan Druart 2022-04-22 11:41:11 UTC
FROM_UNIXTIME(0) does not seem to work

MariaDB [koha_kohadev]> update borrowers set updated_on="1970-01-01 00:00:01" where borrowernumber=1;
Query OK, 1 row affected (0.011 sec)
Rows matched: 1  Changed: 1  Warnings: 0

MariaDB [koha_kohadev]> update borrowers set updated_on="1970-01-01 00:00:00" where borrowernumber=1;
Query OK, 1 row affected, 1 warning (0.012 sec)
Rows matched: 1  Changed: 1  Warnings: 1

MariaDB [koha_kohadev]> select updated_on from borrowers where borrowernumber=1;
+---------------------+
| updated_on          |
+---------------------+
| 0000-00-00 00:00:00 |
+---------------------+
1 row in set (0.001 sec)

MariaDB [koha_kohadev]> show warnings;
+---------+------+-----------------------------------------------------+
| Level   | Code | Message                                             |
+---------+------+-----------------------------------------------------+
| Warning | 1264 | Out of range value for column 'updated_on' at row 1 |
+---------+------+-----------------------------------------------------+
1 row in set (0.000 sec)

MariaDB [koha_kohadev]> update borrowers set updated_on="1970-01-01 00:00:01" where borrowernumber=1;
Query OK, 1 row affected (0.011 sec)
Rows matched: 1  Changed: 1  Warnings: 0
Comment 10 Owen Leonard 2022-04-22 11:44:28 UTC
I get this error:

ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Incorrect datetime value: '1970-01-01 00:00:00' for column `koha_kohadev`.`borrowers`.`updated_on` at row 4739 at /kohadevbox/koha/C4/Installer.pm line 738
Comment 11 Fridolin Somers 2022-04-22 18:52:46 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 12 Fridolin Somers 2022-04-22 18:53:44 UTC
(In reply to Fridolin Somers from comment #11)
> Pushed to master for 22.05, thanks to everybody involved 🦄

Sorry I forgot yesterday to update status
Comment 13 Fridolin Somers 2022-04-22 18:55:19 UTC
May I revert ?
Comment 14 Katrin Fischer 2022-04-24 09:24:38 UTC
Hi,

we recently had some issues with updated timestamps on updates. Can you confirm that this will only change records where the updated_on was NULL before?
Comment 15 Marcel de Rooy 2022-04-24 11:45:31 UTC
(In reply to Katrin Fischer from comment #14)
> Hi,
> 
> we recently had some issues with updated timestamps on updates. Can you
> confirm that this will only change records where the updated_on was NULL
> before?

+            WHERE updated_on IS NULL
Comment 16 Katrin Fischer 2022-04-24 11:54:33 UTC
Reading helps - thx Marcel.
Comment 17 Marcel de Rooy 2022-04-25 10:26:44 UTC
MariaDB [koha_myclone]> update borrowers set updated_on=COALESCE( NULL, FROM_UNIXTIME(0) ) where borrowernumber=51;
ERROR 1292 (22007): Incorrect datetime value: '1970-01-01 00:00:00' for column `koha_myclone`.`borrowers`.`updated_on` at row 1

MariaDB [koha_myclone]> update borrowers set updated_on=COALESCE( NULL, FROM_UNIXTIME(1) ) where borrowernumber=51;
Query OK, 1 row affected (0.008 sec)
Rows matched: 1  Changed: 1  Warnings: 0

So I would propose to switch to UNIXTIME(1).
Note that I would consider this a bug in SQL.

Will add a follow-up
Comment 18 Marcel de Rooy 2022-04-25 10:41:15 UTC
Created attachment 133735 [details] [review]
Bug 27253: (follow-up) Fix UNIXTIME(0) in db_revs/211200037.pl

MariaDB [koha_myclone]> update borrowers set updated_on=COALESCE( NULL, FROM_UNIXTIME(0) ) where borrowernumber=51;
ERROR 1292 (22007): Incorrect datetime value: '1970-01-01 00:00:00' for column `koha_myclone`.`borrowers`.`updated_on` at row 1

MariaDB [koha_myclone]> update borrowers set updated_on=COALESCE( NULL, FROM_UNIXTIME(1) ) where borrowernumber=51;
Query OK, 1 row affected (0.008 sec)
Rows matched: 1  Changed: 1  Warnings: 0

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested by switching updated_on to datetime. Remove NOT NULL, etc.
Copied dbrev to atomicupdate folder.
Resulted in:
    Updated all NULL values of borrowers.updated_on to GREATEST(date_renewed, dateenrolled, lastseen): 51 rows updated
Comment 19 Marcel de Rooy 2022-04-25 10:43:52 UTC
(In reply to Fridolin Somers from comment #13)
> May I revert ?

Push the follow-up please.
Comment 20 Julian Maurice 2022-04-25 12:01:54 UTC
(In reply to Marcel de Rooy from comment #17)
> So I would propose to switch to UNIXTIME(1).
> Note that I would consider this a bug in SQL.

It's weird that '1970-01-01 00:00:00' is not a valid timestamp but it's not really a bug IMO. MySQL documentation says that:

The TIMESTAMP data type is used for values that contain both date and time parts. TIMESTAMP has a range of '1970-01-01 00:00:01' UTC to '2038-01-19 03:14:07' UTC.

https://dev.mysql.com/doc/refman/8.0/en/datetime.html
Comment 21 Marcel de Rooy 2022-04-25 12:07:36 UTC
(In reply to Julian Maurice from comment #20)
> (In reply to Marcel de Rooy from comment #17)
> > So I would propose to switch to UNIXTIME(1).
> > Note that I would consider this a bug in SQL.
> 
> It's weird that '1970-01-01 00:00:00' is not a valid timestamp but it's not
> really a bug IMO. MySQL documentation says that:
> 
> The TIMESTAMP data type is used for values that contain both date and time
> parts. TIMESTAMP has a range of '1970-01-01 00:00:01' UTC to '2038-01-19
> 03:14:07' UTC.
> 
> https://dev.mysql.com/doc/refman/8.0/en/datetime.html

Ok no bug, inconsistency then :)
Comment 22 Fridolin Somers 2022-04-25 19:25:08 UTC
(In reply to Marcel de Rooy from comment #19)
> (In reply to Fridolin Somers from comment #13)
> > May I revert ?
> 
> Push the follow-up please.

Done
Comment 23 Kyle M Hall 2022-04-29 17:29:50 UTC
Pushed to 21.11.x for 21.11.06
Comment 24 Victor Grousset/tuxayo 2022-06-16 03:21:48 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.