Bug 10459 - borrowers should have a timestamp
Summary: borrowers should have a timestamp
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Marcel de Rooy
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 17914
  Show dependency treegraph
 
Reported: 2013-06-13 09:23 UTC by Laurence Rault
Modified: 2017-12-07 22:16 UTC (History)
14 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10459: Borrowers should have a timestamp (2.74 KB, patch)
2014-11-10 13:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10459: Borrowers should have a timestamp (3.56 KB, patch)
2014-11-10 13:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED_OFF] Bug 10459: Borrowers should have a timestamp (3.73 KB, patch)
2014-11-10 17:26 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 10459: Borrowers should have a timestamp (3.85 KB, patch)
2014-11-12 15:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10459: Borrowers should have a timestamp (3.85 KB, patch)
2014-11-12 15:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10459: Borrowers should have a timestamp (3.73 KB, patch)
2014-11-13 08:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10459: Borrowers should have a timestamp (3.85 KB, patch)
2016-04-20 19:08 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 10459: Follow up to update to atomic update methodology (2.16 KB, patch)
2016-04-20 19:23 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10459: Borrowers should have a timestamp (3.92 KB, patch)
2016-04-20 19:39 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10459: Follow up to update to atomic update methodology (2.26 KB, patch)
2016-04-20 19:39 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 10459: Borrowers should have a timestamp (3.96 KB, patch)
2016-04-20 19:42 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 10459: Follow up to update to atomic update methodology (2.30 KB, patch)
2016-04-20 19:42 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 10459 renamed timestamp to updated_on (3.35 KB, patch)
2016-05-26 01:58 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 10459: Borrowers should have a timestamp (3.75 KB, patch)
2016-05-26 02:02 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 10459: Follow up to update to atomic update methodology (2.20 KB, patch)
2016-05-26 02:02 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 10459 renamed timestamp to updated_on (3.35 KB, patch)
2016-05-26 02:03 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 10459: Borrowers should have a timestamp (3.81 KB, patch)
2016-05-26 15:10 UTC, Joy Nelson
Details | Diff | Splinter Review
Bug 10459: Follow up to update to atomic update methodology (2.26 KB, patch)
2016-05-26 15:10 UTC, Joy Nelson
Details | Diff | Splinter Review
Bug 10459 renamed timestamp to updated_on (3.41 KB, patch)
2016-05-26 15:11 UTC, Joy Nelson
Details | Diff | Splinter Review
Bug 10459: Follow up to update to atomic update methodology (2.35 KB, patch)
2016-05-27 09:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10459 renamed timestamp to updated_on (3.50 KB, patch)
2016-05-27 09:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10459: Borrowers should have a timestamp (4.06 KB, patch)
2016-06-07 09:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10459 renamed timestamp to updated_on (3.53 KB, patch)
2016-06-07 09:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10459: Add tests for borrowers.updated_on (2.03 KB, patch)
2016-06-07 09:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10459: (followup) DBIx updates (2.98 KB, patch)
2016-06-12 14:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Laurence Rault 2013-06-13 09:23:34 UTC
We have a Drupal portal related with a Koha, with new borrowers exported from koha to Drupal every night

It would be great if borrowers table had a timestamp, so that only modified borrowers can be exported to portal during update process.
Comment 1 Marcel de Rooy 2014-11-10 13:21:46 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2014-11-10 13:25:29 UTC
Forgot deletedborrowers..please wait
Comment 3 Marcel de Rooy 2014-11-10 13:34:12 UTC Comment hidden (obsolete)
Comment 4 Marc Véron 2014-11-10 17:26:24 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2014-11-11 13:40:19 UTC
Thanks, Marc for signing off.

Temporarily switching status to check a possible problem..
Comment 6 Marcel de Rooy 2014-11-12 15:21:57 UTC
Had this error on 3.14.x: Column 'timestamp' in order clause is ambiguous at /usr/share/koha/prodclone/C4/Items.pm line 1209
Comment 7 Marcel de Rooy 2014-11-12 15:23:22 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2014-11-12 15:24:21 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2014-11-12 15:26:49 UTC
Had to reset the status. Sent a mail to the dev list about the ambiguous timestamps in Koha. My patch now proposes bortimestamp.
Comment 10 Marcel de Rooy 2014-11-13 07:45:04 UTC
From Chris and Galen on the mailing list:

> I think we don't need to make columns unique across the whole db just when
> selecting do select borrowers.timestamp as something.
> DBIx::Class helps us with this also


> I agree with Chris.  In legacy code, doing a "select *" from a join on
> multiple tables is should be discouraged, so using the addition of a
> new column to locate cases of these to stamp out is preferable.  The
> alternative of using a distinct column name has the problem of making
> the writing of more general templates and classes more difficult.
Comment 11 Marcel de Rooy 2014-11-13 07:48:23 UTC
Restored the patch signed off by Marc, but I will still add a patch to update some queries to prevent ambiguous column-errors.
Comment 12 Marcel de Rooy 2014-11-13 08:36:50 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2015-01-08 22:25:17 UTC
Should this be signed off?
Comment 14 Katrin Fischer 2015-01-08 22:28:13 UTC
I am looking for a way to determine easily when a patron was deleted, I think a timestamp in deletedborrowers would help a lot with that. So keen on seeing this move :)
Comment 15 Marcel de Rooy 2015-01-09 09:30:45 UTC
(In reply to Katrin Fischer from comment #13)
> Should this be signed off?

No. I will submit another patch as discussed on the dev list some time ago..
Comment 16 Katrin Fischer 2015-03-16 16:32:47 UTC
Hi Marcel, any update on this? I'd really like to see it happen.
Comment 17 Marcel de Rooy 2015-03-18 12:34:00 UTC
(In reply to Katrin Fischer from comment #16)
> Hi Marcel, any update on this? I'd really like to see it happen.

I will keep it on my list :) but the priority is another topic..
If someone else want to do it before me, please go ahead.
Comment 18 Marcel de Rooy 2015-03-19 10:54:04 UTC
(In reply to M. de Rooy from comment #15)
> No. I will submit another patch as discussed on the dev list some time ago..

For documentation: you can find the discussion in the dev list archives of November 2014.
Comment 19 M. Tompsett 2016-04-20 19:08:07 UTC Comment hidden (obsolete)
Comment 20 M. Tompsett 2016-04-20 19:10:02 UTC
Rebased existing code. Now... let's upgrade it to current standards.
Comment 21 M. Tompsett 2016-04-20 19:23:57 UTC Comment hidden (obsolete)
Comment 22 Bernardo Gonzalez Kriegel 2016-04-20 19:39:15 UTC Comment hidden (obsolete)
Comment 23 Bernardo Gonzalez Kriegel 2016-04-20 19:39:24 UTC Comment hidden (obsolete)
Comment 24 M. Tompsett 2016-04-20 19:40:40 UTC
kohastructure has way too many 'timestamp' named fields, so I just left this.

TEST PLAN
---------
1) back up db
2) apply patch
3) run upgrade
4) confirm that timestamp is now added to borrowers and deletedborrowers.
   -- it should be
5) drop your db, create it empty
6) run web install
7) confirm that timestamp is in borrowers and deleteborrowers
8) run koha qa test tools.
Comment 25 M. Tompsett 2016-04-20 19:42:18 UTC Comment hidden (obsolete)
Comment 26 M. Tompsett 2016-04-20 19:42:32 UTC Comment hidden (obsolete)
Comment 27 Marcel de Rooy 2016-04-21 06:32:59 UTC
Thanks for reviving this.
The reason I left this pending however, is probably as valid as then.
If you call the field timestamp, we will have clashes in SQL joins here and there in the codebase where another table also has the same field timestamp.
Especially think about the SELECT * statements with timestamp in another clause as well. So to prevent the ambiguous column name errors, we need to track these cases, test.. and hope we found them all.
An older alternative to add bortimestamp, introducing a new name, did not meet much approval.
Comment 28 Marcel de Rooy 2016-04-21 09:51:36 UTC
Moving complexity higher in view of former comment
Comment 29 M. Tompsett 2016-04-21 12:37:18 UTC
(In reply to Marcel de Rooy from comment #27)
> Thanks for reviving this.
> The reason I left this pending however, is probably as valid as then.
> If you call the field timestamp, we will have clashes in SQL joins here and
> there in the codebase where another table also has the same field timestamp.
> Especially think about the SELECT * statements with timestamp in another
> clause as well. So to prevent the ambiguous column name errors, we need to
> track these cases, test.. and hope we found them all.
> An older alternative to add bortimestamp, introducing a new name, did not
> meet much approval.

The problem is there are so many already with timestamp, I was thinking a "rename the timestamp fields" bug would be a reasonable next step. I don't see an immediate need to push this through right now. If there is a this bug blocks that renaming bug, that would be a good next step.

Part of the discussion needs to be how to name timestamps.
Because while thinking about that very problem, I found:
DROP TABLE IF EXISTS `borrower_debarments`;
CREATE TABLE borrower_debarments ( -- tracks restrictions on the patron's record
  borrower_debarment_id int(11) NOT NULL AUTO_INCREMENT, -- unique key for the restriction
  borrowernumber int(11) NOT NULL, -- foreign key for borrowers.borrowernumber for patron who is restricted
  expiration date DEFAULT NULL, -- expiration date of the restriction
  `type` enum('SUSPENSION','OVERDUES','MANUAL','DISCHARGE') NOT NULL DEFAULT 'MANUAL', -- type of restriction
  `comment` text, -- comments about the restriction
  manager_id int(11) DEFAULT NULL, -- foreign key for borrowers.borrowernumber for the librarian managing the restriction
  created timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, -- date the restriction was added
  updated timestamp NULL DEFAULT NULL, -- date the restriction was updated
  PRIMARY KEY (borrower_debarment_id),
  KEY borrowernumber (borrowernumber),
  CONSTRAINT `borrower_debarments_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`)
    ON DELETE CASCADE ON UPDATE CASCADE
) ENGINE=InnoDB  DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci;

I think there is a logic error on the created timestamp. That is, I think the ON UPDATE clause should be on the updated timestamp.

In short, timestamps that do exist in Koha are messy.
Comment 30 Marcel de Rooy 2016-04-21 12:50:43 UTC
(In reply to M. Tompsett from comment #29)
> Because while thinking about that very problem, I found:
> DROP TABLE IF EXISTS `borrower_debarments`;
> CREATE TABLE borrower_debarments ( -- tracks restrictions on the patron's
> record
>   borrower_debarment_id int(11) NOT NULL AUTO_INCREMENT, -- unique key for
> the restriction
>   borrowernumber int(11) NOT NULL, -- foreign key for
> borrowers.borrowernumber for patron who is restricted
>   expiration date DEFAULT NULL, -- expiration date of the restriction
>   `type` enum('SUSPENSION','OVERDUES','MANUAL','DISCHARGE') NOT NULL DEFAULT
> 'MANUAL', -- type of restriction
>   `comment` text, -- comments about the restriction
>   manager_id int(11) DEFAULT NULL, -- foreign key for
> borrowers.borrowernumber for the librarian managing the restriction
>   created timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE
> CURRENT_TIMESTAMP, -- date the restriction was added
>   updated timestamp NULL DEFAULT NULL, -- date the restriction was updated
>   PRIMARY KEY (borrower_debarment_id),
>   KEY borrowernumber (borrowernumber),
>   CONSTRAINT `borrower_debarments_ibfk_1` FOREIGN KEY (`borrowernumber`)
> REFERENCES `borrowers` (`borrowernumber`)
>     ON DELETE CASCADE ON UPDATE CASCADE
> ) ENGINE=InnoDB  DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci;
> 
> I think there is a logic error on the created timestamp. That is, I think
> the ON UPDATE clause should be on the updated timestamp.

Good catch. Proving at the same time that we did not adhere to just timestamp everywhere in Koha :)
And I am not saying that we should, but others did..
Comment 31 Kyle M Hall 2016-04-28 10:58:21 UTC
I really dislike having a field name being the same as a mysql data type. I think it introduces ambiguity. Would you or anyone else be opposed to naming it "updated_on"?
Comment 32 Marcel de Rooy 2016-04-28 11:08:07 UTC
(In reply to Kyle M Hall from comment #31)
> I really dislike having a field name being the same as a mysql data type. I
> think it introduces ambiguity. Would you or anyone else be opposed to naming
> it "updated_on"?

Have a look the discussion from some time ago.
I introduced bortimestamp (an ugly but unique name so no ambiguous column problems), but the majority pressed me back to use timestamp and first resolving possible clashes in joins here and there..

Moving this back to In Discussion
Comment 33 Katrin Fischer 2016-04-28 11:09:01 UTC
It looks like we have a 'ton' of timestamp in the database. Apart from that there seems to be no existing other schema for timestamps columns that I could spot. I am not opposed to changing it - I think that's what Marcel wanted to do?
Comment 34 Marcel de Rooy 2016-04-28 12:18:17 UTC
(In reply to Katrin Fischer from comment #33)
> It looks like we have a 'ton' of timestamp in the database. Apart from that
> there seems to be no existing other schema for timestamps columns that I
> could spot. I am not opposed to changing it - I think that's what Marcel
> wanted to do?

This patch cannot go further in this form.
So better move it to In discussion :)
Comment 35 Kyle M Hall 2016-05-02 12:22:22 UTC
(In reply to Katrin Fischer from comment #33)
> It looks like we have a 'ton' of timestamp in the database. Apart from that
> there seems to be no existing other schema for timestamps columns that I
> could spot. I am not opposed to changing it - I think that's what Marcel
> wanted to do?

Indeed! I think we should standardize on a single format. I think "updated_on" for timestamps that are set on each update and "created_on" for timestamps that are only set at the time of creation would be appropriate. We could have a vote at the next dev meeting to formalize this ( or another proposed format ) and add it to the coding guidelines.
Comment 36 M. Tompsett 2016-05-02 12:33:27 UTC
(In reply to Kyle M Hall from comment #35)
> I think we should standardize on a single format.

+1

> I think "updated_on" for timestamps that are set on each update
> and "created_on" for timestamps ... only set at the time of creation

+1


> We could have a vote at the next dev meeting to formalize this ( or another
> proposed format ) and add it to the coding guidelines.

+1 -- Just incase I don't make it to the next dev meeting.
Comment 37 Marc Véron 2016-05-02 12:41:09 UTC
(In reply to Kyle M Hall from comment #35)
> (In reply to Katrin Fischer from comment #33)
> > It looks like we have a 'ton' of timestamp in the database. Apart from that
> > there seems to be no existing other schema for timestamps columns that I
> > could spot. I am not opposed to changing it - I think that's what Marcel
> > wanted to do?
> 
> Indeed! I think we should standardize on a single format. I think
> "updated_on" for timestamps that are set on each update and "created_on" for
> timestamps that are only set at the time of creation would be appropriate.
> We could have a vote at the next dev meeting to formalize this ( or another
> proposed format ) and add it to the coding guidelines.

+1 for standardizing, "created_on", "updated_on"
Comment 38 Joy Nelson 2016-05-25 22:51:34 UTC
I appreciate the discussion on this issue to standardize the naming.  Is this ticket now about renaming all timestamps or still adding a timestamp on the borrower table?  Do we need to split this ticket into two issues?

Having a date (of any name) in the borrowers table will be game changing for those of us who troubleshoot patron data day in and day out.   I'd love to see this move forward with a resolution.

I vote for created_on/updated_on as a good path unless the majority wants timestamp. Then I vote for timestamp.   I just want the field added.  :-)

I will be at the Hackfest in Greece and happy to help write a patch, test...whatever it takes to get this in!

Cheers,
Joy
Comment 39 M. Tompsett 2016-05-26 01:05:39 UTC
(In reply to Joy Nelson from comment #38)
> I appreciate the discussion on this issue to standardize the naming.  Is
> this ticket now about renaming all timestamps or still adding a timestamp on
> the borrower table?  Do we need to split this ticket into two issues?

Yes, it can be split into two issues. The current two patches should be standardized to created_on instead of timestamp with a third patch.


> I will be at the Hackfest in Greece and happy to help write a patch,
> test...whatever it takes to get this in!

Give me a bit... I'll add a third commit to meet standard.
Comment 40 M. Tompsett 2016-05-26 01:58:21 UTC Comment hidden (obsolete)
Comment 41 M. Tompsett 2016-05-26 02:02:52 UTC Comment hidden (obsolete)
Comment 42 M. Tompsett 2016-05-26 02:02:59 UTC Comment hidden (obsolete)
Comment 43 M. Tompsett 2016-05-26 02:03:05 UTC Comment hidden (obsolete)
Comment 44 M. Tompsett 2016-05-26 02:03:50 UTC
Okay, Joy, go sign off crazy. ;)
Comment 45 Joy Nelson 2016-05-26 15:10:21 UTC Comment hidden (obsolete)
Comment 46 Joy Nelson 2016-05-26 15:10:49 UTC Comment hidden (obsolete)
Comment 47 Joy Nelson 2016-05-26 15:11:03 UTC Comment hidden (obsolete)
Comment 48 Joy Nelson 2016-05-26 15:12:27 UTC
Signed off!  Thanks!!!
joy

(In reply to M. Tompsett from comment #44)
> Okay, Joy, go sign off crazy. ;)
Comment 49 M. Tompsett 2016-05-26 16:12:20 UTC
I went with an updated_on, as created_on is static, and the point was for diagnosis to know when something was last done with the borrower record. And I couldn't figure out how to get two time stamps using MySQL to auto-fill (a created_on and an updated_on) in the same table.
Comment 50 Marcel de Rooy 2016-05-27 09:59:01 UTC Comment hidden (obsolete)
Comment 51 Marcel de Rooy 2016-05-27 09:59:06 UTC Comment hidden (obsolete)
Comment 52 Marcel de Rooy 2016-05-27 10:07:11 UTC
(In reply to M. Tompsett from comment #49)
> I went with an updated_on, as created_on is static, and the point was for
> diagnosis to know when something was last done with the borrower record. And
> I couldn't figure out how to get two time stamps using MySQL to auto-fill (a
> created_on and an updated_on) in the same table.

What about filling created_on once in AddMember ?
Mysql accepts: 
  `stamp1` timestamp NULL DEFAULT NULL,
  `stamp2` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP
Comment 53 Marcel de Rooy 2016-05-27 10:51:09 UTC
And another question:
We may expect some libraries to have inserted a custom timestamp column already in the borrowers table. They will run into the same problem as Mark described above here. Would it make sense to check in the database revision if that is the case and skip the dbrev with a warning? (They could rename the field manually?)
Comment 54 Bernardo Gonzalez Kriegel 2016-05-27 11:20:42 UTC
(In reply to Marcel de Rooy from comment #52)
> What about filling created_on once in AddMember ?
> Mysql accepts: 
>   `stamp1` timestamp NULL DEFAULT NULL,
>   `stamp2` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE
> CURRENT_TIMESTAMP

Which MySQL version?
For example this two columns in virtualshelves give problems on 5.7:

  `lastmodified` timestamp NOT NULL default CURRENT_TIMESTAMP on update CURRENT_TIMESTAMP, -- date and time the list was last modified
  `created_on` TIMESTAMP NOT NULL, -- creation time

loading kohastructure fails with 
line:  2212   `created_on` TIMESTAMP NOT NULL, -- creation time
error: (Invalid default value for 'created_on'): CREATE TABLE `virtualshelves`

It seems that we will find some problems because of the new version default sql_mode. It's something that we need to address. 

ATM neither 16.05 nor 3.22 run on Ubuntu 16.04.
Comment 55 M. Tompsett 2016-05-27 12:55:09 UTC
(In reply to Marcel de Rooy from comment #52)
> What about filling created_on once in AddMember ?
> Mysql accepts: 
>   `stamp1` timestamp NULL DEFAULT NULL,
>   `stamp2` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE
> CURRENT_TIMESTAMP

That's a possibility, but the idea was just a structural change.
The updated_on is sufficient for the bug's original intention.


(In reply to Bernardo Gonzalez Kriegel from comment #54)
> Which MySQL version?

Whatever is current with Debian 8, I believe.


(In reply to Bernardo Gonzalez Kriegel from comment #54)
> For example this two columns in virtualshelves give problems on 5.7:
> 
>   `lastmodified` timestamp NOT NULL default CURRENT_TIMESTAMP on update
> CURRENT_TIMESTAMP, -- date and time the list was last modified
>   `created_on` TIMESTAMP NOT NULL, -- creation time

Well, obviously, because NOT NULL means you need a default value. MySQL is trying to avoid the NULL vs. 0000-00-00 issue.

That's why Marcel was suggesting NULL default NULL above.

My comment was aimed at the disappointment that the following:
`updated_on` timestamp NOT NULL default CURRENT_TIMESTAMP on update CURRENT_TIMESTAMP, -- date and time the list was last modified
`created_on` TIMESTAMP NOT NULL default CURRENT_TIMESTAMP, -- creation time

Which probably is the equivalent to the less explicit version BKG gave above. QA-wise, this should be just fine. I'd push a secondary patch to add created_on with an update in the appropriate Koha perl code in another bug.
Comment 56 Brendan Gallagher 2016-06-03 07:24:26 UTC
This should be ready for QA - the created_on addition should be done in another bug - since that is a different issue than this bug.
Comment 57 David Cook 2016-06-06 00:00:56 UTC
(In reply to Kyle M Hall from comment #31)
> I really dislike having a field name being the same as a mysql data type. I
> think it introduces ambiguity. Would you or anyone else be opposed to naming
> it "updated_on"?

In addition to what Kyle has said, it appears that our own coding guidelines say that we shouldn't be using SQL92 keywords like "timestamp" as column names:

https://wiki.koha-community.org/wiki/Coding_Guidelines#SQL2:_SQL92_keywords
Comment 58 M. Tompsett 2016-06-06 01:16:18 UTC
Hence the third patch which makes it updated_on, David Cook.
Still ready for QA, like comment #56 says.
Comment 59 Marcel de Rooy 2016-06-06 08:08:38 UTC
(In reply to M. Tompsett from comment #58)
> Hence the third patch which makes it updated_on, David Cook.
> Still ready for QA, like comment #56 says.

Mark, if you remove my name from that old first patch and squash everything into one new one, I will pass QA on it. Somehow keep the accumulated signoffs!
Comment 60 David Cook 2016-06-07 03:57:53 UTC
(In reply to M. Tompsett from comment #58)
> Hence the third patch which makes it updated_on, David Cook.
> Still ready for QA, like comment #56 says.

Yes, I was just adding the link for the sake of the conversation.
Comment 61 Jonathan Druart 2016-06-07 08:45:55 UTC
Comment on attachment 51867 [details] [review]
Bug 10459: Follow up to update to atomic update methodology

Review of attachment 51867 [details] [review]:
-----------------------------------------------------------------

There is no point to move updatedb entry content to an atomic update file in another patch.
The goal of this workflow was to avoid conflict on the updatedatabase.pl file.
With this patch, we now get 2 conflicts to solve...
Comment 62 Jonathan Druart 2016-06-07 09:21:35 UTC
Created attachment 52113 [details] [review]
Bug 10459: Borrowers should have a timestamp

This patch adds a timestamp column to the borrowers table in kohastructure
and updatedatabase. (And also to the deletedborrowers table.)
A timestamp may be useful in synchronizing with external systems (among other
reasons).

Test plan:
Run updatestructure on an existing database, or install a new one.
Verify that the borrowers table has a timestamp now.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested updatestructure and running kohastructure.sql.
Passed t/db_dependent/Members.t.

updatedatabase.pl did not apply. I edited and then run it. Columns were added as expected.
Signed-off-by: Marc Veron <veron@veron.ch>

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Joy Nelson <joy@bywatersolutions.com>

Bug 10459: Follow up to update to atomic update methodology

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
New column created, no errors.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Joy Nelson <joy@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 63 Jonathan Druart 2016-06-07 09:21:39 UTC
Created attachment 52114 [details] [review]
Bug 10459 renamed timestamp to updated_on

Signed-off-by: Joy Nelson <joy@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 64 Jonathan Druart 2016-06-07 09:21:47 UTC
Created attachment 52115 [details] [review]
Bug 10459: Add tests for borrowers.updated_on

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 65 Jonathan Druart 2016-06-07 09:22:35 UTC
I have squashed the 2 first patches (Mark, I hope it's ok for you) and added tests.
Comment 66 M. Tompsett 2016-06-07 12:37:54 UTC
(In reply to Jonathan Druart from comment #65)
> I have squashed the 2 first patches (Mark, I hope it's ok for you) and added
> tests.

I generally don't squash things of different authors, but it's okay.
Comment 67 Jonathan Druart 2016-06-07 14:42:55 UTC
(In reply to M. Tompsett from comment #66)
> (In reply to Jonathan Druart from comment #65)
> > I have squashed the 2 first patches (Mark, I hope it's ok for you) and added
> > tests.
> 
> I generally don't squash things of different authors, but it's okay.

Me neither but as I said previously there is no sense to move code from updatedatabase.pl to the atomic update dir in the same patch set (2 conflicts instead of 1).
Comment 68 Kyle M Hall 2016-06-10 17:22:54 UTC
Pushed to master for 16.11, thanks Marcel, Mark and Jonathan!
Comment 69 Tomás Cohen Arazi 2016-06-12 13:53:55 UTC
Kyle, upgrade the DBIx schema!
Comment 70 Tomás Cohen Arazi 2016-06-12 14:19:09 UTC
While I agree with the timestamp, I must say systems syncing borrowers should use borrowers.borrower_sync which links to the table borrower_sync, which is well engineered to be reused outside the limited scope of the NorwegianPatronDB thing.

We should even extend plugins to allow for different sync implementations.
Comment 71 Tomás Cohen Arazi 2016-06-12 14:24:41 UTC
Created attachment 52294 [details] [review]
Bug 10459: (followup) DBIx updates

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 72 Tomás Cohen Arazi 2016-06-14 18:02:15 UTC
Moving back to the @RM queue.
Comment 73 Tomás Cohen Arazi 2016-06-14 18:02:47 UTC
Guys, push the followup!
Comment 74 Frédéric Demians 2016-06-15 06:25:27 UTC
I can't evaluate this patch level of dangerousness for 16.05 branch, especially with Thomas last comment. Any advice?
Comment 75 Marcel de Rooy 2016-06-15 11:07:32 UTC
(In reply to Frédéric Demians from comment #74)
> I can't evaluate this patch level of dangerousness for 16.05 branch,
> especially with Thomas last comment. Any advice?

Not a matter of dangerous. Just about location of the timestamp.
Such a change could be backported as well.
Comment 76 Frédéric Demians 2016-06-15 12:19:01 UTC
Pushed in 16.05. Will be in 16.05.01.
Comment 77 Kyle M Hall 2016-06-17 14:41:31 UTC
I think we're all fixed up on this one!
Comment 78 Julian Maurice 2016-07-01 06:22:02 UTC
Patches pushed to 3.22.x, will be in 3.22.9
Comment 79 Jonathan Druart 2017-01-16 11:02:01 UTC
@RMaints, have a look at bug 17914, to avoid a warning when updating.
Comment 80 Marcel de Rooy 2017-01-16 11:59:44 UTC
(In reply to Jonathan Druart from comment #79)
> @RMaints, have a look at bug 17914, to avoid a warning when updating.

Backports with db revs always generate warnings?