Bug 17782 - Patron updated_on field should be set to current timestamp when borrower is deleted
Summary: Patron updated_on field should be set to current timestamp when borrower is d...
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 18003
  Show dependency treegraph
 
Reported: 2016-12-15 14:17 UTC by Nick Clemens
Modified: 2017-02-23 20:39 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17782 - Patron updated_on field should be set to current timestamp when borrower is deleted (1.12 KB, patch)
2016-12-15 14:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17782 - Patron updated_on field should be set to current timestamp when borrower is deleted (1.12 KB, patch)
2017-01-13 00:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17782 - Patron updated_on field should be set to current timestamp when borrower is deleted (1.18 KB, patch)
2017-01-13 07:09 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 17782 - (QA Followup) (1.86 KB, patch)
2017-01-25 16:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17782: Try to fix tests (2.83 KB, patch)
2017-01-26 11:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17782 - Allow discard_changes to work for Deletedborrower class (905 bytes, patch)
2017-01-26 13:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17782 - Patron updated_on field should be set to current timestamp when borrower is deleted (1.25 KB, patch)
2017-01-27 09:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17782 - (QA Followup) (1.93 KB, patch)
2017-01-27 09:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17782: Fix tests (2.01 KB, patch)
2017-01-27 09:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
[3.22.x] Bug 17782: deletedborrowers.updated_on should be set to current timestamp (723 bytes, patch)
2017-02-09 09:52 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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