Bug 23219 - Show a warning about cancelling their holds before a patron is deleted
Summary: Show a warning about cancelling their holds before a patron is deleted
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Emmi Takkinen
QA Contact: Testopia
URL:
Keywords: Manual, release-notes-needed
Depends on:
Blocks:
 
Reported: 2019-06-26 10:50 UTC by Emmi Takkinen
Modified: 2019-12-05 16:15 UTC (History)
5 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:
19.11.00


Attachments
Bug 23219: Cancel patrons holds when patron delete (4.46 KB, patch)
2019-07-02 08:09 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 23219: Cancel patrons holds when patron delete (4.46 KB, patch)
2019-07-04 05:30 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 23219: Cancel patrons holds when patron delete (4.52 KB, patch)
2019-07-05 12:06 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23219: Cancel patrons holds when patron delete (4.57 KB, patch)
2019-07-05 14:10 UTC, Nadine Pierre
Details | Diff | Splinter Review
Bug 23219: Cancel patrons holds when patron delete (4.61 KB, patch)
2019-07-05 18:13 UTC, Arthur Bousquet
Details | Diff | Splinter Review
Bug 23219: Cancel patrons holds when patron delete (4.69 KB, patch)
2019-07-06 15:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23219: (QA Follow up) Cancel patrons holds when patron (1.75 KB, patch)
2019-07-09 06:37 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 23219: Cancel patrons holds when patron delete (4.69 KB, patch)
2019-07-12 06:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23219: (QA Follow up) Cancel patrons holds when patron delete (1.80 KB, patch)
2019-07-12 06:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23219: (QA follow-up) Change wording of test slightly (1.28 KB, patch)
2019-07-12 06:17 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emmi Takkinen 2019-06-26 10:50:36 UTC
Currently it's possible to delete patron with holds. This should be prevented like deleting patrons with checkouts. In this way it would be in-line to checkout and account handling.
Comment 1 Katrin Fischer 2019-06-26 10:53:31 UTC
I think this was discussed a long time ago and we had decided that holds should be cancel. Not sure if there is consensus here - cancelling a lot of holds manually could be time consuming. Maybe a confirmation question with an option to cancel them?
Comment 2 Owen Leonard 2019-06-26 11:46:10 UTC
I would think that the problem would be limited to waiting holds. If a patron has a hold which is unfulfilled why not automatically cancel it?
Comment 3 Emmi Takkinen 2019-06-26 11:58:16 UTC
We have noticed that when you delete patron with holds the holds just disappear and there is no data in old_reserves. Maybe other solution is to move holds to the old_reserves when deleting?
Comment 4 Emmi Takkinen 2019-07-02 08:09:22 UTC Comment hidden (obsolete)
Comment 5 Owen Leonard 2019-07-03 13:43:37 UTC
Everything looks like it's working as it should. There's just one minor complaint from the QA tool:

 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/members/deletemem.tt
   FAIL   filters
                missing_filter at line 41 (            <h3>Patron has [% ItemsOnHold %] hold(s).
Comment 6 Emmi Takkinen 2019-07-04 05:30:19 UTC
Created attachment 91281 [details] [review]
Bug 23219: Cancel patrons holds when patron delete

Fixed missing filter from "ItemsOnHold".
Comment 7 Owen Leonard 2019-07-05 12:06:29 UTC
Created attachment 91331 [details] [review]
Bug 23219: Cancel patrons holds when patron delete

Currently deleting a patron deletes all their holds and leaves
no record to the "old_reserves" table.

 Steps to reproduce:
- Create a patron
- Add holds for patron
- Holds are recorded to "reserves" table
- Delete patron
- Confirm delete
=>Patron and all holds are deleted and no record of holds is
left in "old_reserves" table

This patch displays alert text notifying user that deleting patron
cancels all their holds. Holds are cancelled instead of deleting them.
This patch also writes stringified datetime to holds cancel log instead
of whole datetime object.

To test:
- Apply this patch
- Create a patron
- Add holds for patron
- Holds are recorded to "reserves" table
- Delete patron
- Alert text of holds is displayed
- Confirm patron delete
=> Patron is deleted, their holds are cancelled and moved to
"old_reserves" table

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 8 Nadine Pierre 2019-07-05 14:10:57 UTC
Created attachment 91333 [details] [review]
Bug 23219: Cancel patrons holds when patron delete

Currently deleting a patron deletes all their holds and leaves
no record to the "old_reserves" table.

 Steps to reproduce:
- Create a patron
- Add holds for patron
- Holds are recorded to "reserves" table
- Delete patron
- Confirm delete
=>Patron and all holds are deleted and no record of holds is
left in "old_reserves" table

This patch displays alert text notifying user that deleting patron
cancels all their holds. Holds are cancelled instead of deleting them.
This patch also writes stringified datetime to holds cancel log instead
of whole datetime object.

To test:
- Apply this patch
- Create a patron
- Add holds for patron
- Holds are recorded to "reserves" table
- Delete patron
- Alert text of holds is displayed
- Confirm patron delete
=> Patron is deleted, their holds are cancelled and moved to
"old_reserves" table

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Nadine Pierre <nadine.pierre@inLibro.com>
Comment 9 Arthur Bousquet 2019-07-05 18:13:52 UTC
Created attachment 91351 [details] [review]
Bug 23219: Cancel patrons holds when patron delete

Currently deleting a patron deletes all their holds and leaves
no record to the "old_reserves" table.

 Steps to reproduce:
- Create a patron
- Add holds for patron
- Holds are recorded to "reserves" table
- Delete patron
- Confirm delete
=>Patron and all holds are deleted and no record of holds is
left in "old_reserves" table

This patch displays alert text notifying user that deleting patron
cancels all their holds. Holds are cancelled instead of deleting them.
This patch also writes stringified datetime to holds cancel log instead
of whole datetime object.

To test:
- Apply this patch
- Create a patron
- Add holds for patron
- Holds are recorded to "reserves" table
- Delete patron
- Alert text of holds is displayed
- Confirm patron delete
=> Patron is deleted, their holds are cancelled and moved to
"old_reserves" table

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Nadine Pierre <nadine.pierre@inLibro.com>
Signed-off-by: Arthur Bousquet <arthur.bousquet@inlibro.com>
Comment 10 Katrin Fischer 2019-07-06 15:23:58 UTC
Created attachment 91395 [details] [review]
Bug 23219: Cancel patrons holds when patron delete

Currently deleting a patron deletes all their holds and leaves
no record to the "old_reserves" table.

 Steps to reproduce:
- Create a patron
- Add holds for patron
- Holds are recorded to "reserves" table
- Delete patron
- Confirm delete
=>Patron and all holds are deleted and no record of holds is
left in "old_reserves" table

This patch displays alert text notifying user that deleting patron
cancels all their holds. Holds are cancelled instead of deleting them.
This patch also writes stringified datetime to holds cancel log instead
of whole datetime object.

To test:
- Apply this patch
- Create a patron
- Add holds for patron
- Holds are recorded to "reserves" table
- Delete patron
- Alert text of holds is displayed
- Confirm patron delete
=> Patron is deleted, their holds are cancelled and moved to
"old_reserves" table

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Nadine Pierre <nadine.pierre@inLibro.com>
Signed-off-by: Arthur Bousquet <arthur.bousquet@inlibro.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Katrin Fischer 2019-07-06 15:25:32 UTC
Hi Emmi, this works really well, but according to our coding guidelines any change to one of the C4 or Koha routines should be accompanied by a unit test testing the changed/new behaviour. Would you be able to provide a test? Let us know if we can help. With the test, you can switch back to Signed off straight away.
Comment 12 Emmi Takkinen 2019-07-09 06:37:19 UTC
Created attachment 91423 [details] [review]
Bug 23219: (QA Follow up) Cancel patrons holds when patron

Adds test for patrons holds cancelation when patron is deleted.

Test plan:
Run t/db_dependent/Koha/Patrons.t
Comment 13 Katrin Fischer 2019-07-12 06:13:36 UTC
Hi Emmi,

for the tests - should this line not be removed now or changed? As we don't want to delete but cancel:

     is( Koha::Holds->search( { borrowernumber => $patron->{borrowernumber} } )->count, 0, q|Koha::Patron->delete should have deleted patron's holds| );

I'll change the wording in a follow-up.
Comment 14 Katrin Fischer 2019-07-12 06:17:33 UTC
Created attachment 91473 [details] [review]
Bug 23219: Cancel patrons holds when patron delete

Currently deleting a patron deletes all their holds and leaves
no record to the "old_reserves" table.

 Steps to reproduce:
- Create a patron
- Add holds for patron
- Holds are recorded to "reserves" table
- Delete patron
- Confirm delete
=>Patron and all holds are deleted and no record of holds is
left in "old_reserves" table

This patch displays alert text notifying user that deleting patron
cancels all their holds. Holds are cancelled instead of deleting them.
This patch also writes stringified datetime to holds cancel log instead
of whole datetime object.

To test:
- Apply this patch
- Create a patron
- Add holds for patron
- Holds are recorded to "reserves" table
- Delete patron
- Alert text of holds is displayed
- Confirm patron delete
=> Patron is deleted, their holds are cancelled and moved to
"old_reserves" table

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Nadine Pierre <nadine.pierre@inLibro.com>
Signed-off-by: Arthur Bousquet <arthur.bousquet@inlibro.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Katrin Fischer 2019-07-12 06:17:37 UTC
Created attachment 91474 [details] [review]
Bug 23219: (QA Follow up) Cancel patrons holds when patron delete

Adds test for patrons holds cancelation when patron is deleted.

Test plan:
Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Katrin Fischer 2019-07-12 06:17:41 UTC
Created attachment 91475 [details] [review]
Bug 23219: (QA follow-up) Change wording of test slightly

We no longer delete, but cancel the holds when deleting a patron.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 17 Martin Renvoize 2019-07-15 11:51:24 UTC
Nice work!

Pushed to master for 19.11.00
Comment 18 Nick Clemens 2019-12-05 16:15:37 UTC
*** Bug 24182 has been marked as a duplicate of this bug. ***