Description
Joy Nelson
2019-09-25 22:45:51 UTC
Chiming in with a YES PLEASE from Huntsville-Madison County Public Library. Linking to bug 24599, as we need a spec for batch actions on holds, including cancelling them. (In reply to Joy Nelson from comment #0) > Allow staff a way to cancel more than one hold at a time. > Provide checkboxes on holds waiting over tab and allow staff to select > multiple holds and cancel those selected holds. +1 Not that I'm opposed to batch action on holds, but is that strictly required for this development? We'd likely go through and check everything in anyway since there might be other transfers, materials specified notes or whatever to deal with. (In reply to Benjamin Daeuber from comment #4) > Not that I'm opposed to batch action on holds, but is that strictly required > for this development? We'd likely go through and check everything in anyway > since there might be other transfers, materials specified notes or whatever > to deal with. Ah, never mind. I think I see what you mean here. The bulk action for canceling the holds. I was sidetracked by a discussion on triggering multiple holds at one time. Created attachment 118338 [details] [review] Bug 23678: Allow cancel holds in bulk This patch allows staff patrons to cancel multiple holds in bulk. To test: 1. Apply this patch 2. restart_all 3. In cataloge go to a book and place many holds CHECK => Holds table shows a column of checkboxes 4. Play with checkboxes (have some fun ;-P) CHECK => When you manually check all checkboxes, the checkbox in the header also gets checked. => When you uncheck one of the checkboxes, the one in the header also gets unchecked. => If no checkbox is checked and you check the one in the header, all checkboxes get checked. => If there are some checkboxes that are checked and others are not, when you click on the checkbox in the header all checkboxes get unchecked. => If all checkboxes are checked, when you uncheck the one in the header, all checkboxes get unchecked. => Every time you play with checkboxes, the number in the button "Cancel selected" changes. 5. Check some of the checkboxes and click on cancel selected. SUCCESS => A background job gets fired to cancel all selected holds. => A message should appear with a link to the job. 6. Wait a few seconds and click on the link SUCCESS => A message appears with the report of the execution of the background job. 7. Grab a patron and search to hold 8. Select multiple biblios and click on "place hold for <patron>" CHECK => After holds are confirmed, multiple holds table are shown.. one for each record. Checkboxes work exactly the same as before, but scoped for each individual table. Checkboxes from one table will not affect checkboxes from other tables. 9. Repeat steps 4 to 6. Hi Agustín - I tested this and it works great. However, the functionality is tied to cancelling holds on a queue for an individual title. I actually just submitted a bug for that - #27922. This bug states that it should allow cancellation of selected holds via checkboxes from the 'holds waiting over xx days' tab on waitingreserves.pl. So while the patch works great I don't think it address the original intention of this bug. (In reply to Barbara Johnson from comment #7) > Hi Agustín - I tested this and it works great. However, the functionality > is tied to cancelling holds on a queue for an individual title. I actually > just submitted a bug for that - #27922. > > This bug states that it should allow cancellation of selected holds via > checkboxes from the 'holds waiting over xx days' tab on waitingreserves.pl. > So while the patch works great I don't think it address the original > intention of this bug. Thanks Barbara.. I'm such a dummy! *** Bug 27922 has been marked as a duplicate of this bug. *** Created attachment 118553 [details] [review] Bug 23678: Allow cancel holds in bulk This patch allows staff patrons to cancel multiple holds in bulk. To test: 1. Apply this patch 2. restart_all 3. In cataloge go to a book and place many holds CHECK => Holds table shows a column of checkboxes 4. Play with checkboxes (have some fun ;-P) CHECK => When you manually check all checkboxes, the checkbox in the header also gets checked. => When you uncheck one of the checkboxes, the one in the header also gets unchecked. => If no checkbox is checked and you check the one in the header, all checkboxes get checked. => If there are some checkboxes that are checked and others are not, when you click on the checkbox in the header all checkboxes get unchecked. => If all checkboxes are checked, when you uncheck the one in the header, all checkboxes get unchecked. => Every time you play with checkboxes, the number in the button "Cancel selected" changes. 5. Check some of the checkboxes and click on cancel selected. SUCCESS => A background job gets fired to cancel all selected holds. => A message should appear with a link to the job. 6. Wait a few seconds and click on the link SUCCESS => A message appears with the report of the execution of the background job. 7. Grab a patron and search to hold 8. Select multiple biblios and click on "place hold for <patron>" CHECK => After holds are confirmed, multiple holds table are shown.. one for each record. Checkboxes work exactly the same as before, but scoped for each individual table. Checkboxes from one table will not affect checkboxes from other tables. 9. Repeat steps 4 to 6. 10. Check In some of the items so the get in Waiting state. 11. Update expirationdate os some of those holds and set it to ReservesMaxPickUpDelay + 1 days earlier NOTE => ReservesMaxPickUpDelay = 7 days by default, so sql syntax to update would be => update reserves set expirationdate = date_sub(expirationdate, interval 8 day) where reserve_id in (...) 12. Repeat steps 4 to 6 but in waitingreserves.pl, in both tabs. (In reply to Barbara Johnson from comment #7) > Hi Agustín - I tested this and it works great. However, the functionality > is tied to cancelling holds on a queue for an individual title. I actually > just submitted a bug for that - #27922. > > This bug states that it should allow cancellation of selected holds via > checkboxes from the 'holds waiting over xx days' tab on waitingreserves.pl. > So while the patch works great I don't think it address the original > intention of this bug. Hi Barbara, could you please try once again? Thanks Everything worked great. Also checked to make sure that if a title had several holds that using the bulk cancel did not impact the rest of the holds queue. Created attachment 120265 [details] [review] Bug 23678: (QA follow-up) Add missing template filter Requires another follow-up: FAIL koha-tmpl/intranet-tmpl/prog/en/modules/admin/background_jobs.tt FAIL filters missing_filter at line 180 ( Hold on <a href="/cgi-bin/koha/reserve/request.pl?biblionumber=[% m.biblio_id | uri %]">[% m.biblio_title | html %]</a> for <a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% m.patron_id | uri %]">[% m.patron_name %]</a> has successfully been cancelled.) Created attachment 123185 [details] [review] Bug 23678: Allow cancel holds in bulk This patch allows staff patrons to cancel multiple holds in bulk. To test: 1. Apply this patch 2. restart_all 3. In cataloge go to a book and place many holds CHECK => Holds table shows a column of checkboxes 4. Play with checkboxes (have some fun ;-P) CHECK => When you manually check all checkboxes, the checkbox in the header also gets checked. => When you uncheck one of the checkboxes, the one in the header also gets unchecked. => If no checkbox is checked and you check the one in the header, all checkboxes get checked. => If there are some checkboxes that are checked and others are not, when you click on the checkbox in the header all checkboxes get unchecked. => If all checkboxes are checked, when you uncheck the one in the header, all checkboxes get unchecked. => Every time you play with checkboxes, the number in the button "Cancel selected" changes. 5. Check some of the checkboxes and click on cancel selected. SUCCESS => A background job gets fired to cancel all selected holds. => A message should appear with a link to the job. 6. Wait a few seconds and click on the link SUCCESS => A message appears with the report of the execution of the background job. 7. Grab a patron and search to hold 8. Select multiple biblios and click on "place hold for <patron>" CHECK => After holds are confirmed, multiple holds table are shown.. one for each record. Checkboxes work exactly the same as before, but scoped for each individual table. Checkboxes from one table will not affect checkboxes from other tables. 9. Repeat steps 4 to 6. 10. Check In some of the items so the get in Waiting state. 11. Update expirationdate os some of those holds and set it to ReservesMaxPickUpDelay + 1 days earlier NOTE => ReservesMaxPickUpDelay = 7 days by default, so sql syntax to update would be => update reserves set expirationdate = date_sub(expirationdate, interval 8 day) where reserve_id in (...) 12. Repeat steps 4 to 6 but in waitingreserves.pl, in both tabs. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 123186 [details] [review] Bug 23678: (QA follow-up) Add missing template filter Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 123187 [details] [review] Bug 23678: (QA follow-up) Add missing filters Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (In reply to Katrin Fischer from comment #14) > Requires another follow-up: Done, thanks Created attachment 125404 [details] [review] Bug 23678: Allow cancel holds in bulk This patch allows staff patrons to cancel multiple holds in bulk. To test: 1. Apply this patch 2. restart_all 3. In cataloge go to a book and place many holds CHECK => Holds table shows a column of checkboxes 4. Play with checkboxes (have some fun ;-P) CHECK => When you manually check all checkboxes, the checkbox in the header also gets checked. => When you uncheck one of the checkboxes, the one in the header also gets unchecked. => If no checkbox is checked and you check the one in the header, all checkboxes get checked. => If there are some checkboxes that are checked and others are not, when you click on the checkbox in the header all checkboxes get unchecked. => If all checkboxes are checked, when you uncheck the one in the header, all checkboxes get unchecked. => Every time you play with checkboxes, the number in the button "Cancel selected" changes. 5. Check some of the checkboxes and click on cancel selected. SUCCESS => A background job gets fired to cancel all selected holds. => A message should appear with a link to the job. 6. Wait a few seconds and click on the link SUCCESS => A message appears with the report of the execution of the background job. 7. Grab a patron and search to hold 8. Select multiple biblios and click on "place hold for <patron>" CHECK => After holds are confirmed, multiple holds table are shown.. one for each record. Checkboxes work exactly the same as before, but scoped for each individual table. Checkboxes from one table will not affect checkboxes from other tables. 9. Repeat steps 4 to 6. 10. Check In some of the items so the get in Waiting state. 11. Update expirationdate os some of those holds and set it to ReservesMaxPickUpDelay + 1 days earlier NOTE => ReservesMaxPickUpDelay = 7 days by default, so sql syntax to update would be => update reserves set expirationdate = date_sub(expirationdate, interval 8 day) where reserve_id in (...) 12. Repeat steps 4 to 6 but in waitingreserves.pl, in both tabs. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 125405 [details] [review] Bug 23678: (QA follow-up) Add missing template filter Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 125406 [details] [review] Bug 23678: (QA follow-up) Add missing filters Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> This works as expected and passes QA scripts. I can't see any evidence of previous background jobs having associated tests so the lack of a unit test here seems reasonable.. but it did make me wonder. Passing QA and differing to RM regards tests. Created attachment 125470 [details] [review] Bug 23678: (QA follow-up) Use correct indentation Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 1. I don't understand why we pass patron_name when we have the patron_id (same for biblio_title). If you modify the patron's name, the name won't match. If you cannot do it now, you may need Koha::BackgroundJob->additional_report from "Bug 28445: Add the capability to provide more info to the background job detail view" 2. + push @messages, { + type => 'error', + code => 'hold_not_cancelled', + patron_id => defined $patron?$patron->borrowernumber:'', + patron_name => defined $patron?($patron->firstname?$patron->firstname.', ':'').$patron->surname:'', + biblio_id => defined $biblio?$biblio->biblionumber:'', + biblio_title => defined $biblio?$biblio->title:'', + hold_id => $hold_id, + error => defined $hold?($@ ? $@ : 0):'No hold with id '.$hold_id.' found', + }; Erk, spaces, indentation? I can fix that. Created attachment 125577 [details] [review] Bug 23678: (QA follow-up) Use correct indentation Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> JD amended patch: also Koha/BackgroundJob/BatchCancelHold.pm Created attachment 125578 [details] [review] Bug 23678: Allow cancel holds in bulk This patch allows staff patrons to cancel multiple holds in bulk. To test: 1. Apply this patch 2. restart_all 3. In cataloge go to a book and place many holds CHECK => Holds table shows a column of checkboxes 4. Play with checkboxes (have some fun ;-P) CHECK => When you manually check all checkboxes, the checkbox in the header also gets checked. => When you uncheck one of the checkboxes, the one in the header also gets unchecked. => If no checkbox is checked and you check the one in the header, all checkboxes get checked. => If there are some checkboxes that are checked and others are not, when you click on the checkbox in the header all checkboxes get unchecked. => If all checkboxes are checked, when you uncheck the one in the header, all checkboxes get unchecked. => Every time you play with checkboxes, the number in the button "Cancel selected" changes. 5. Check some of the checkboxes and click on cancel selected. SUCCESS => A background job gets fired to cancel all selected holds. => A message should appear with a link to the job. 6. Wait a few seconds and click on the link SUCCESS => A message appears with the report of the execution of the background job. 7. Grab a patron and search to hold 8. Select multiple biblios and click on "place hold for <patron>" CHECK => After holds are confirmed, multiple holds table are shown.. one for each record. Checkboxes work exactly the same as before, but scoped for each individual table. Checkboxes from one table will not affect checkboxes from other tables. 9. Repeat steps 4 to 6. 10. Check In some of the items so the get in Waiting state. 11. Update expirationdate os some of those holds and set it to ReservesMaxPickUpDelay + 1 days earlier NOTE => ReservesMaxPickUpDelay = 7 days by default, so sql syntax to update would be => update reserves set expirationdate = date_sub(expirationdate, interval 8 day) where reserve_id in (...) 12. Repeat steps 4 to 6 but in waitingreserves.pl, in both tabs. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Bug 23678: (QA follow-up) Add missing template filter Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Bug 23678: (QA follow-up) Add missing filters Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Bug 23678: (QA follow-up) Use correct indentation Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> JD amended patch: also Koha/BackgroundJob/BatchCancelHold.pm Patches squashed. Created attachment 125579 [details] [review] Bug 23678: Don't display code of the job type Created attachment 125589 [details] [review] Bug 23678: Allow cancel holds in bulk This patch allows staff patrons to cancel multiple holds in bulk. To test: 1. Apply this patch 2. restart_all 3. In cataloge go to a book and place many holds CHECK => Holds table shows a column of checkboxes 4. Play with checkboxes (have some fun ;-P) CHECK => When you manually check all checkboxes, the checkbox in the header also gets checked. => When you uncheck one of the checkboxes, the one in the header also gets unchecked. => If no checkbox is checked and you check the one in the header, all checkboxes get checked. => If there are some checkboxes that are checked and others are not, when you click on the checkbox in the header all checkboxes get unchecked. => If all checkboxes are checked, when you uncheck the one in the header, all checkboxes get unchecked. => Every time you play with checkboxes, the number in the button "Cancel selected" changes. 5. Check some of the checkboxes and click on cancel selected. SUCCESS => A background job gets fired to cancel all selected holds. => A message should appear with a link to the job. 6. Wait a few seconds and click on the link SUCCESS => A message appears with the report of the execution of the background job. 7. Grab a patron and search to hold 8. Select multiple biblios and click on "place hold for <patron>" CHECK => After holds are confirmed, multiple holds table are shown.. one for each record. Checkboxes work exactly the same as before, but scoped for each individual table. Checkboxes from one table will not affect checkboxes from other tables. 9. Repeat steps 4 to 6. 10. Check In some of the items so the get in Waiting state. 11. Update expirationdate os some of those holds and set it to ReservesMaxPickUpDelay + 1 days earlier NOTE => ReservesMaxPickUpDelay = 7 days by default, so sql syntax to update would be => update reserves set expirationdate = date_sub(expirationdate, interval 8 day) where reserve_id in (...) 12. Repeat steps 4 to 6 but in waitingreserves.pl, in both tabs. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Bug 23678: (QA follow-up) Add missing template filter Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Bug 23678: (QA follow-up) Add missing filters Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Bug 23678: (QA follow-up) Use correct indentation Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> JD amended patch: also Koha/BackgroundJob/BatchCancelHold.pm JD Amended patch: Full rebase and adjustements made on top of bug 26080. Created attachment 125590 [details] [review] Bug 23678: Don't display code of the job type Patches rebased and reworked on top of bug 26080 (PQA for a month already). It refactored some of the background job code. 99 error => defined $hold 100 ? ( $@ ? $@ : 0 ) 101 : 'No hold with id ' . $hold_id . ' found', Caught a non translatable error. Created attachment 125600 [details] [review] Bug 23678: Include file for batch_hold_cancel Use additional_report to retrieve patron and biblio's info. Tomas, have a look at the current version and the dependent bugs. I moved code out of bug 28445 to reuse it here. Created attachment 125627 [details] [review] Bug 23678: Allow cancel holds in bulk This patch allows staff patrons to cancel multiple holds in bulk. To test: 1. Apply this patch 2. restart_all 3. In cataloge go to a book and place many holds CHECK => Holds table shows a column of checkboxes 4. Play with checkboxes (have some fun ;-P) CHECK => When you manually check all checkboxes, the checkbox in the header also gets checked. => When you uncheck one of the checkboxes, the one in the header also gets unchecked. => If no checkbox is checked and you check the one in the header, all checkboxes get checked. => If there are some checkboxes that are checked and others are not, when you click on the checkbox in the header all checkboxes get unchecked. => If all checkboxes are checked, when you uncheck the one in the header, all checkboxes get unchecked. => Every time you play with checkboxes, the number in the button "Cancel selected" changes. 5. Check some of the checkboxes and click on cancel selected. SUCCESS => A background job gets fired to cancel all selected holds. => A message should appear with a link to the job. 6. Wait a few seconds and click on the link SUCCESS => A message appears with the report of the execution of the background job. 7. Grab a patron and search to hold 8. Select multiple biblios and click on "place hold for <patron>" CHECK => After holds are confirmed, multiple holds table are shown.. one for each record. Checkboxes work exactly the same as before, but scoped for each individual table. Checkboxes from one table will not affect checkboxes from other tables. 9. Repeat steps 4 to 6. 10. Check In some of the items so the get in Waiting state. 11. Update expirationdate os some of those holds and set it to ReservesMaxPickUpDelay + 1 days earlier NOTE => ReservesMaxPickUpDelay = 7 days by default, so sql syntax to update would be => update reserves set expirationdate = date_sub(expirationdate, interval 8 day) where reserve_id in (...) 12. Repeat steps 4 to 6 but in waitingreserves.pl, in both tabs. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Bug 23678: (QA follow-up) Add missing template filter Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Bug 23678: (QA follow-up) Add missing filters Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Bug 23678: (QA follow-up) Use correct indentation Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> JD amended patch: also Koha/BackgroundJob/BatchCancelHold.pm JD Amended patch: Full rebase and adjustements made on top of bug 26080. Created attachment 125628 [details] [review] Bug 23678: Don't display code of the job type Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 125629 [details] [review] Bug 23678: Include file for batch_hold_cancel Use additional_report to retrieve patron and biblio's info. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 125631 [details] [review] Bug 23678: Include file for batch_hold_cancel Use additional_report to retrieve patron and biblio's info. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Pushed to master for 21.11, thanks to everybody involved! *** Bug 28062 has been marked as a duplicate of this bug. *** Why are we storing selected holds in localStorage? If we leave the page and come back the checkboxes are selected again, I don't think it's the expected behaviour. Can someone confirm? (In reply to Jonathan Druart from comment #41) > Why are we storing selected holds in localStorage? > > If we leave the page and come back the checkboxes are selected again, I > don't think it's the expected behaviour. Can someone confirm? I would not expect it to keep the checkboxes if I left the page and came back. To me it sounds a bit dangerous actually as this might get missed for a single checkbox hiding way down in a list for example. Opened bug 38300. |