Bug 10459

Forgot deletedborrowers..please wait

Thanks, Marc for signing off.

Temporarily switching status to check a possible problem..

Had this error on 3.14.x: Column 'timestamp' in order clause is ambiguous at /usr/share/koha/prodclone/C4/Items.pm line 1209

Had to reset the status. Sent a mail to the dev list about the ambiguous timestamps in Koha. My patch now proposes bortimestamp.

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.

Restored the patch signed off by Marc, but I will still add a patch to update some queries to prevent ambiguous column-errors.

Should this be signed off?

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 :)

(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..

Hi Marcel, any update on this? I'd really like to see it happen.

(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.

(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.

Rebased existing code. Now... let's upgrade it to current standards.

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.

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.

Moving complexity higher in view of former comment

(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.

(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..

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 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

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?

(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 :)

(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.

(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.

(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"

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

(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.

Okay, Joy, go sign off crazy. ;)

Signed off!  Thanks!!!
joy

(In reply to M. Tompsett from comment #44)
> Okay, Joy, go sign off crazy. ;)

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.

(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

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?)

(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.

(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.

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.

(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

Hence the third patch which makes it updated_on, David Cook.
Still ready for QA, like comment #56 says.

(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!

(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 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...

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>

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>

Created attachment 52115 [details] [review]
Bug 10459: Add tests for borrowers.updated_on

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

I have squashed the 2 first patches (Mark, I hope it's ok for you) and added tests.

(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.

(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).

Pushed to master for 16.11, thanks Marcel, Mark and Jonathan!

Kyle, upgrade the DBIx schema!

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.

Created attachment 52294 [details] [review]
Bug 10459: (followup) DBIx updates

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Moving back to the @RM queue.

Guys, push the followup!

I can't evaluate this patch level of dangerousness for 16.05 branch, especially with Thomas last comment. Any advice?

(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.

Pushed in 16.05. Will be in 16.05.01.

I think we're all fixed up on this one!

Patches pushed to 3.22.x, will be in 3.22.9

@RMaints, have a look at bug 17914, to avoid a warning when updating.

(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?