Bug 17782

Summary: Patron updated_on field should be set to current timestamp when borrower is deleted
Product: Koha Reporter: Nick Clemens <nick>
Component: PatronsAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: claire_gravely, george, gmcharlt, jonathan.druart+koha, julian.maurice, katrin.fischer, kyle.m.hall, kyle, mtj
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 18003    
Attachments: Bug 17782 - Patron updated_on field should be set to current timestamp when borrower is deleted
Bug 17782 - Patron updated_on field should be set to current timestamp when borrower is deleted
Bug 17782 - Patron updated_on field should be set to current timestamp when borrower is deleted
Bug 17782 - (QA Followup)
Bug 17782: Try to fix tests
Bug 17782 - Allow discard_changes to work for Deletedborrower class
Bug 17782 - Patron updated_on field should be set to current timestamp when borrower is deleted
Bug 17782 - (QA Followup)
Bug 17782: Fix tests
[3.22.x] Bug 17782: deletedborrowers.updated_on should be set to current timestamp

Description Nick Clemens 2016-12-15 14:17:50 UTC
Currently the timestamp is not udpated when a patron is deleted. This is because we insert the db value of updated_on from the patron when we move the borrower.  Clearing the value beforehand should resolve this
Comment 1 Nick Clemens 2016-12-15 14:20:17 UTC
Created attachment 58203 [details] [review]
Bug 17782 - Patron updated_on field should be set to current timestamp when borrower is deleted

To test:
01 Find a patron
02 Get the updated_on value from the db in borrowers table
03 Delete the patron
04 Get the update_on value from the db on deletedborrowers table
05 Values from 02 and 04 are the same
06 Apply patch
07 Repeat 01-04
08 Values should now be different
Comment 2 Jonathan Druart 2017-01-09 10:47:15 UTC
Nick, is this ready to be tested?
Comment 3 Nick Clemens 2017-01-10 19:19:43 UTC
(In reply to Jonathan Druart from comment #2)
> Nick, is this ready to be tested?

Yes, should be, forgot to change status
Comment 4 Claire Gravely 2017-01-11 08:14:46 UTC
Hi Nick

After applying the patch, the updated_on field in deletedborrowers hasn't been populated after deleting the patron. They appear in the table, but the field is blank.
Comment 5 Jonathan Druart 2017-01-11 08:42:55 UTC
Nick, I did not test, but it does not work that way. If you want DBIx::Class to use the default value you must not pass the key.
So:
  delete $patron_infos->{updated_on};
should work.
Comment 6 Nick Clemens 2017-01-13 00:43:17 UTC
Created attachment 58890 [details] [review]
Bug 17782 - Patron updated_on field should be set to current timestamp when borrower is deleted

To test:
01 Find a patron
02 Get the updated_on value from the db in borrowers table
03 Delete the patron
04 Get the update_on value from the db on deletedborrowers table
05 Values from 02 and 04 are the same
06 Apply patch
07 Repeat 01-04
08 Values should now be different
Comment 7 Nick Clemens 2017-01-13 00:43:51 UTC
I thought it worked the other way, this is better though :-)

Updated patch

(In reply to Jonathan Druart from comment #5)
> Nick, I did not test, but it does not work that way. If you want DBIx::Class
> to use the default value you must not pass the key.
> So:
>   delete $patron_infos->{updated_on};
> should work.
Comment 8 Claire Gravely 2017-01-13 07:09:54 UTC
Created attachment 58913 [details] [review]
Bug 17782 - Patron updated_on field should be set to current timestamp when borrower is deleted

To test:
01 Find a patron
02 Get the updated_on value from the db in borrowers table
03 Delete the patron
04 Get the update_on value from the db on deletedborrowers table
05 Values from 02 and 04 are the same
06 Apply patch
07 Repeat 01-04
08 Values should now be different

Signed-off-by: Claire Gravely <claire_gravely@hotmail.com>
Comment 9 Jonathan Druart 2017-01-13 10:43:59 UTC
Please provide tests (t/db_dependent/Koha/Patrons.t -> move_to_deleted).
Note that at one test does not pass on my devbox:

    #   Failed test 'Koha::Patron->move_to_deleted should have correctly moved the patron to the deleted table'
    #   at t/db_dependent/Koha/Patrons.t line 331.
    #     Structures begin differing at:
    #          $got->{updated_on} = '2017-01-13 11:42:27'
    #     $expected->{updated_on} = '2017-01-13 10:42:26'

system date is 2017-01-13 11:42:27
I do not understand what's going on actually.
Comment 10 George Williams (NEKLS) 2017-01-18 06:06:16 UTC
This failed the test when I tried to sign off this evening.  The timestamp didn't change on the second deletion.
Comment 11 Nick Clemens 2017-01-25 16:00:59 UTC
Created attachment 59558 [details] [review]
Bug 17782 - (QA Followup)

Fix tests

prove t/db_dependent/Koha/Patrons.t
Comment 12 Jonathan Druart 2017-01-26 11:27:19 UTC
Created attachment 59573 [details] [review]
Bug 17782: Try to fix tests

Previous code was wrong, $retrieved_patron->{updated_on} is always
undef, $retrieved_patron is a Koha::Object
Comment 13 Kyle M Hall 2017-01-26 13:27:38 UTC
Created attachment 59583 [details] [review]
Bug 17782 - Allow discard_changes to work for Deletedborrower class
Comment 14 Jonathan Druart 2017-01-27 09:15:39 UTC
Created attachment 59605 [details] [review]
Bug 17782 - Patron updated_on field should be set to current timestamp when borrower is deleted

To test:
01 Find a patron
02 Get the updated_on value from the db in borrowers table
03 Delete the patron
04 Get the update_on value from the db on deletedborrowers table
05 Values from 02 and 04 are the same
06 Apply patch
07 Repeat 01-04
08 Values should now be different

Signed-off-by: Claire Gravely <claire_gravely@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Jonathan Druart 2017-01-27 09:15:45 UTC
Created attachment 59606 [details] [review]
Bug 17782 - (QA Followup)

Fix tests

prove t/db_dependent/Koha/Patrons.t

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 16 Jonathan Druart 2017-01-27 09:15:50 UTC
Created attachment 59607 [details] [review]
Bug 17782: Fix tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Jonathan Druart 2017-01-27 09:26:03 UTC
Let's deal with deletedborrowers.borrowernumber as a PK on another bug report.
Comment 18 Jonathan Druart 2017-01-27 10:45:20 UTC
(In reply to Jonathan Druart from comment #17)
> Let's deal with deletedborrowers.borrowernumber as a PK on another bug
> report.

See bug 18003.
Comment 19 Kyle M Hall 2017-02-07 17:54:56 UTC
Pushed to master for 17.05, thanks Nick, Jonathan!
Comment 20 Katrin Fischer 2017-02-07 21:25:17 UTC
This patch has been pushed to 16.11.x and will be in 16.11.04.

Note: updated_on is also present in 3.22.x, so possibly needs backporting all the way down.
Comment 21 Julian Maurice 2017-02-09 09:52:32 UTC
Created attachment 60047 [details] [review]
[3.22.x] Bug 17782: deletedborrowers.updated_on should be set to current timestamp
Comment 22 Julian Maurice 2017-02-09 09:52:53 UTC
Pushed to 3.22.x for 3.22.17
Comment 23 Mason James 2017-02-23 20:39:36 UTC
Pushed to 16.05.x, for 16.05.10 release