Summary: | borrowers.updated_on cannot be null on fresh install, but can be null with upgrade | ||
---|---|---|---|
Product: | Koha | Reporter: | Kyle M Hall (khall) <kyle> |
Component: | Architecture, internals, and plumbing | Assignee: | Julian Maurice <julian.maurice> |
Status: | CLOSED FIXED | QA Contact: | Marcel de Rooy <m.de.rooy> |
Severity: | normal | ||
Priority: | P5 - low | CC: | fridolin.somers, jonathan.druart, julian.maurice, kyle, m.de.rooy, tomascohen, victor |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
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
|
|
Circulation function: | |||
Bug Depends on: | 10459 | ||
Bug Blocks: | 30486 | ||
Attachments: |
Bug 27253: Fix definition of updated_on in borrowers and deletedborrowers
Bug 27253: Fix definition of updated_on in borrowers and deletedborrowers Bug 27253: Fix definition of updated_on in borrowers and deletedborrowers Bug 27253: (follow-up) Fix UNIXTIME(0) in db_revs/211200037.pl |
Description
Kyle M Hall (khall)
2020-12-16 14:39:58 UTC
What about MAX(date_renewed, dateenrolled, last_seen)? (In reply to Jonathan Druart from comment #1) > What about MAX(date_renewed, dateenrolled, last_seen)? +1 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 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> QA: Looking here (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? $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; 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> 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 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 Pushed to master for 22.05, thanks to everybody involved [U+1F984] (In reply to Fridolin Somers from comment #11) > Pushed to master for 22.05, thanks to everybody involved [U+1F984] Sorry I forgot yesterday to update status May I revert ? 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? (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 Reading helps - thx Marcel. 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 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 (In reply to Fridolin Somers from comment #13) > May I revert ? Push the follow-up please. (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 (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 :) (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 Pushed to 21.11.x for 21.11.06 Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed. |