|
Description
Emmi Takkinen
2019-06-26 10:50:36 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? 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? 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? Created attachment 91175 [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 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).
Created attachment 91281 [details] [review] Bug 23219: Cancel patrons holds when patron delete Fixed missing filter from "ItemsOnHold". 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> 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> 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> 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> 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. 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 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.
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> 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> 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> Nice work! Pushed to master for 19.11.00 *** Bug 24182 has been marked as a duplicate of this bug. *** |