Bug 25534 adds the option to send hold cancellation notices when a reason was given. It would be nice if you could give a reason but still decide not to send to the patron. Say the patron called and will not be able to pick up numerous holds for some time. You cancel them all with "patron requested cancellation" but don't want to send multiple notices. I could imagine a checkbox next to the reason to activate/deactivate sending the notice.
Created attachment 109286 [details] [review] Bug 26282: Allow staff to decide if a hold cancellation notice will be sent when cancelling a hold Bug 25534 adds the option to send hold cancellation notices when a reason was given. It would be nice if you could give a reason but still decide not to send to the patron. Say the patron called and will not be able to pick up numerous holds for some time. You cancel them all with "patron requested cancellation" but don't want to send multiple notices. I could imagine a checkbox next to the reason to activate/deactivate sending the notice. Test Plan: 1) Apply this patch 2) Visit each area in Koha where a hold can be canceled with a reason 3) Note the new 'Notify patron' checkbox 4) Test canceling a hold with and without this checkbox checked 5) Verify leaving it unchecked does not trigger a notice to be sent to the patron
> 2) Visit each area in Koha where a hold can be canceled with a reason 3 places according to the patch code. A. pendingreserves B. Record page => Holds (it's the list of holds bellow the patron search field) C. Same template but I don't get where this code is displayed Missing place found: in the patron's page. Hint for other places: due to clubs, are there other places to cancel holds?
Considering this one a bug fix.
Kyle, this is blocker for the release. Can you have another try please?
Created attachment 113960 [details] [review] Bug 26282: (QA follow-up) Implement for moremember.pl and circulation.pl
Potential issue found: "No reason given" => no notification sent
Created attachment 113991 [details] [review] Bug 26282: (QA follow-up) Always notify patron if notify checkbox is checked
(In reply to Kyle M Hall from comment #7) > Created attachment 113991 [details] [review] [review] > Bug 26282: (QA follow-up) Always notify patron if notify checkbox is checked I am not sure we want that. I'd prefer to adjust the label of the checkbox instead (or better, disable the checkbox if no option is selected). What do you think?
(In reply to Jonathan Druart from comment #8) > (In reply to Kyle M Hall from comment #7) > > Created attachment 113991 [details] [review] [review] [review] > > Bug 26282: (QA follow-up) Always notify patron if notify checkbox is checked > > I am not sure we want that. I'd prefer to adjust the label of the checkbox > instead (or better, disable the checkbox if no option is selected). > What do you think? That is how it functioned previously. I'm not sure what you mean by adjusting the label of the checkbox. A reason with a value of "" or "0" is certainly valid in Koha, so we shouldn't prevent sending a notice just because that's how the library has set up the AV values. The default AV values support this, since one of them is already failing to send a notice. IMO it makes for extra unnecessary steps to require a librarian to both select a reason *and* check the checkbox. When the library could have their 'default' reason be the first reason. It feels like design over usability.
(In reply to Jonathan Druart from comment #8) > (In reply to Kyle M Hall from comment #7) > > Created attachment 113991 [details] [review] [review] [review] > > Bug 26282: (QA follow-up) Always notify patron if notify checkbox is checked > > I am not sure we want that. I'd prefer to adjust the label of the checkbox > instead (or better, disable the checkbox if no option is selected). > What do you think? I think both (having a cancellation reason and choosing to notify) should be independent. I'm not sure why setting a reason should put a constraint on notifying the patron the hold has been cancelled.
(In reply to Kyle M Hall from comment #9) > (In reply to Jonathan Druart from comment #8) > > (In reply to Kyle M Hall from comment #7) > > > Created attachment 113991 [details] [review] [review] [review] [review] > > > Bug 26282: (QA follow-up) Always notify patron if notify checkbox is checked > > > > I am not sure we want that. I'd prefer to adjust the label of the checkbox > > instead (or better, disable the checkbox if no option is selected). > > What do you think? > > That is how it functioned previously. I'm not sure what you mean by > adjusting the label of the checkbox. Previously what? On master there is: if ( $params->{cancellation_reason} ) { then send the notice So if no reason is chosen, no notice is sent. In my understanding, the limit of this bug report is to add the checkbox, not to change this behaviour. My suggestion was to modify the label "Notify patron" with something like "Notify patron if a reason is selected". It's not optimal, and that's why I suggested to disable the checkbox until an option was selected from the "reason" dropdown list.
(In reply to Jonathan Druart from comment #11) > (In reply to Kyle M Hall from comment #9) > > (In reply to Jonathan Druart from comment #8) > > > (In reply to Kyle M Hall from comment #7) > > > > Created attachment 113991 [details] [review] [review] [review] [review] [review] > > > > Bug 26282: (QA follow-up) Always notify patron if notify checkbox is checked > > > > > > I am not sure we want that. I'd prefer to adjust the label of the checkbox > > > instead (or better, disable the checkbox if no option is selected). > > > What do you think? > > > > That is how it functioned previously. I'm not sure what you mean by > > adjusting the label of the checkbox. > > Previously what? > > On master there is: > if ( $params->{cancellation_reason} ) { > then send the notice > > So if no reason is chosen, no notice is sent. > > In my understanding, the limit of this bug report is to add the checkbox, > not to change this behaviour. > > My suggestion was to modify the label "Notify patron" with something like > "Notify patron if a reason is selected". > It's not optimal, and that's why I suggested to disable the checkbox until > an option was selected from the "reason" dropdown list. I do agree with you Jonathan - this is change in behaviour, and disabling checkbox when no reason is selected is definitely solution for this.
This will also need some handling added to misc/cronjobs/holds/cancel_expired_holds.pl to re-enable the option to send notices when holds are automatically cancelled on expiration as per bug 12656
Created attachment 114007 [details] [review] Bug 26282: Allow staff to decide if a hold cancellation notice will be sent when cancelling a hold Bug 25534 adds the option to send hold cancellation notices when a reason was given. It would be nice if you could give a reason but still decide not to send to the patron. Say the patron called and will not be able to pick up numerous holds for some time. You cancel them all with "patron requested cancellation" but don't want to send multiple notices. I could imagine a checkbox next to the reason to activate/deactivate sending the notice. Test Plan: 1) Apply this patch 2) Visit each area in Koha where a hold can be canceled with a reason 3) Note the new 'Notify patron' checkbox 4) Test canceling a hold with and without this checkbox checked 5) Verify leaving it unchecked does not trigger a notice to be sent to the patron Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 114008 [details] [review] Bug 26282: (QA follow-up) Implement for moremember.pl and circulation.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 114009 [details] [review] Bug 26282: (QA follow-up) Always notify patron if notify checkbox is checked Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 114010 [details] [review] Bug 26282: (QA follow-up) Allow notification via cancel_expired_holds Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
As this is an entirely new notice this cycle, I'd be tempted to update the default to check for the presence of 'reason' and give a slightly different form if it is not present.... as a side note, if we're going to allow notifications without a reason, then perhaps rather than a checkbox we should instead have 'Confirm', 'Confirm and notify` and `Cancel` buttons/links to close the modal?
We are D-1 before the release, I'd like a patch as small as possible. And I won't have time to test it...
Created attachment 114011 [details] [review] Bug 26282: Unit tests This patch adds tests for the behaviour change. It tests if letter processing takes place when notify_patron is passed. It tests all combinations. Bonus: it tweaks the POD to acknowledge this new parameter. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Hold.t => FAIL: Tests fail! 3. Apply the rest of the patches 4. Repeat 2 => SUCCESS: Tests pass! No warnings! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 114012 [details] [review] Bug 26282: Allow staff to decide if a hold cancellation notice will be sent when cancelling a hold Bug 25534 adds the option to send hold cancellation notices when a reason was given. It would be nice if you could give a reason but still decide not to send to the patron. Say the patron called and will not be able to pick up numerous holds for some time. You cancel them all with "patron requested cancellation" but don't want to send multiple notices. I could imagine a checkbox next to the reason to activate/deactivate sending the notice. Test Plan: 1) Apply this patch 2) Visit each area in Koha where a hold can be canceled with a reason 3) Note the new 'Notify patron' checkbox 4) Test canceling a hold with and without this checkbox checked 5) Verify leaving it unchecked does not trigger a notice to be sent to the patron Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 114013 [details] [review] Bug 26282: (QA follow-up) Implement for moremember.pl and circulation.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 114014 [details] [review] Bug 26282: (QA follow-up) Always notify patron if notify checkbox is checked Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 114015 [details] [review] Bug 26282: (QA follow-up) Allow notification via cancel_expired_holds Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 114016 [details] [review] Bug 26282: (QA follow-up) Update POD Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
The letter is: """ Your hold for De Crasi Aristophea was canceled for the following reason: """ if you don't select a reason...
So, to resume a private discussion. We are trying to modify a behaviour that is not advertise in comment 0, neither on bug 25534 and so it's not the point of this bug report. I still don't understand Kyle's "That is how it functioned previously". My suggestion is still the same as comment 8, we remove the last 3 patches, adjust the checkbox's label with "only if a reason is selected". However the best would be to disable the checkbox until an option is selected, to not introduce new strings (release planned tomorrow...).
Created attachment 114028 [details] [review] Bug 26282: Disable the checkbox if no reason selected On this bug report we want to explicitly tell the librarian than a notice will be generate if a cancellation reason is selected. If the new checkbox we have a situation if the checkbox is selected and no reason selected => no notice will be generated This patch is not the optimal solution but answers the original need, without string changes (today is release day...) and without too many modifications in the code. Additional follow-up bug report must be opened to deal properly with this specific use case.
I am really happy to have to deal with a checkbox on release day!
Created attachment 114030 [details] [review] Bug 26282: Unit tests This patch adds tests for the behaviour change. It tests if letter processing takes place when notify_patron is passed. It tests all combinations. Bonus: it tweaks the POD to acknowledge this new parameter. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Hold.t => FAIL: Tests fail! 3. Apply the rest of the patches 4. Repeat 2 => SUCCESS: Tests pass! No warnings! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 114031 [details] [review] Bug 26282: Allow staff to decide if a hold cancellation notice will be sent when cancelling a hold Bug 25534 adds the option to send hold cancellation notices when a reason was given. It would be nice if you could give a reason but still decide not to send to the patron. Say the patron called and will not be able to pick up numerous holds for some time. You cancel them all with "patron requested cancellation" but don't want to send multiple notices. I could imagine a checkbox next to the reason to activate/deactivate sending the notice. Test Plan: 1) Apply this patch 2) Visit each area in Koha where a hold can be canceled with a reason 3) Note the new 'Notify patron' checkbox 4) Test canceling a hold with and without this checkbox checked 5) Verify leaving it unchecked does not trigger a notice to be sent to the patron Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 114032 [details] [review] Bug 26282: (QA follow-up) Implement for moremember.pl and circulation.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 114033 [details] [review] Bug 26282: Disable the checkbox if no reason selected On this bug report we want to explicitly tell the librarian than a notice will be generate if a cancellation reason is selected. If the new checkbox we have a situation if the checkbox is selected and no reason selected => no notice will be generated This patch is not the optimal solution but answers the original need, without string changes (today is release day...) and without too many modifications in the code. Additional follow-up bug report must be opened to deal properly with this specific use case. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 114034 [details] [review] Bug 26282: (QA follow-up) Update test Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Hi Josef, I was in a chat with Martin. We were actually thinking about removing the notice template HOLD_CANCELLATION from the DB, for 20.11.00. That will disable the feature and let people create the notice template if needed. Then we will implement the real fix properly for 20.11.01.
Created attachment 114035 [details] [review] Bug 26282: (ALTERNATIVE) Remove HOLD_CANCELLATION notice We are really close to release, and the 'proper' fix to make the sending of hold cancellation notices optional will introduce string changes. This patch takes the alternative approach of removing the default notice template there by disabling the feature for 20.11.00 release and giving us time to properly impliment a solution and re-instate the default notice template for the 20.11.01 release.
I think this final alternative is the best approach given the release timing... If we all agree, I will happily transplant the other patches here to a new bug for 20.11.01 and go back and update the release note text for the two affected bugs now.
(In reply to Martin Renvoize from comment #37) > I think this final alternative is the best approach given the release > timing... > > If we all agree, I will happily transplant the other patches here to a new > bug for 20.11.01 and go back and update the release note text for the two > affected bugs now. I do agree thanks Martin and Jonathan!
Created attachment 114247 [details] [review] Bug 26282: Allow staff to decide if a hold cancellation notice will be sent when cancelling a hold Bug 25534 adds the option to send hold cancellation notices when a reason was given. It would be nice if you could give a reason but still decide not to send to the patron. Say the patron called and will not be able to pick up numerous holds for some time. You cancel them all with "patron requested cancellation" but don't want to send multiple notices. I could imagine a checkbox next to the reason to activate/deactivate sending the notice. Test Plan: 1) Apply this patch 2) Visit each area in Koha where a hold can be canceled with a reason 3) Note the new 'Notify patron' checkbox 4) Test canceling a hold with and without this checkbox checked 5) Verify leaving it unchecked does not trigger a notice to be sent to the patron Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 114248 [details] [review] Bug 26282: (QA follow-up) Implement for moremember.pl and circulation.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 114249 [details] [review] Bug 26282: (QA follow-up) Always notify patron if notify checkbox is checked Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 114250 [details] [review] Bug 26282: (QA follow-up) Allow notification via cancel_expired_holds Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 114251 [details] [review] Bug 26282: (QA follow-up) Update POD Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 114252 [details] [review] Bug 26282: (QA follow-up) Unit tests This patch adds tests for the behaviour change. It tests if letter processing takes place when notify_patron is passed. It tests all combinations. Bonus: it tweaks the POD to acknowledge this new parameter. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Hold.t => FAIL: Tests fail! 3. Apply the rest of the patches 4. Repeat 2 => SUCCESS: Tests pass! No warnings! 5. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 114253 [details] [review] Bug 26282: (QA follow-up) Update test Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 114254 [details] Button approach
Created attachment 114255 [details] Checkbox aproach
Two more followups to come.. one to restore the notices we nuked with bug 27103 and one to convert from a checkbox to a button.
Created attachment 114259 [details] [review] Bug 26282: (QA follow-up) Restore default notice
OK, I don't think I'll manage to get to switching the checkbox implementation to a double button one.. Perhaps we can do that in a distinct bug along with cleaning up some of the forms. So.. I reckon this is read for SO/QA again
(In reply to Martin Renvoize from comment #50) > OK, I don't think I'll manage to get to switching the checkbox > implementation to a double button one.. > > Perhaps we can do that in a distinct bug along with cleaning up some of the > forms. > > So.. I reckon this is read for SO/QA again The checkbox is displayed even if the letter template does not exist. Should not we hide it? Should not we insert the template for new installs?
Good call.. seems I missed some bits from that final commit.. I'll dig them out tomorrow.. As for bit displaying the checkbox.. are there precidents for that elsewhere? I'm happy to do it, but am also keen to have consistency
It's how it works right now with the feature. It's disabled if the letter does not exist. And yes, behaviours being disabled if the letter template does not exist is what we are doing in other areas (claims, out of the top of my head).
Slowly working on this here: https://gitlab.com/mrenvoize/Koha/-/commits/bug_26282 It's slow going working through the various locations one can cancel a reservation from and updating the checkbox to optionally display/hide based on the existence of a notice template.
Created attachment 115193 [details] [review] Bug 26282: Allow staff to decide if a hold cancellation notice will be sent when cancelling a hold Bug 25534 adds the option to send hold cancellation notices when a reason was given. It would be nice if you could give a reason but still decide not to send to the patron. Say the patron called and will not be able to pick up numerous holds for some time. You cancel them all with "patron requested cancellation" but don't want to send multiple notices. I could imagine a checkbox next to the reason to activate/deactivate sending the notice. Test Plan: 1) Apply this patch 2) Visit each area in Koha where a hold can be canceled with a reason 3) Note the new 'Notify patron' checkbox 4) Test canceling a hold with and without this checkbox checked 5) Verify leaving it unchecked does not trigger a notice to be sent to the patron Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 115194 [details] [review] Bug 26282: (QA follow-up) Implement for moremember.pl and circulation.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 115195 [details] [review] Bug 26282: (QA follow-up) Always notify patron if notify checkbox is checked Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 115196 [details] [review] Bug 26282: (QA follow-up) Allow notification via cancel_expired_holds Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 115197 [details] [review] Bug 26282: (QA follow-up) Update POD Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 115198 [details] [review] Bug 26282: (QA follow-up) Unit tests This patch adds tests for the behaviour change. It tests if letter processing takes place when notify_patron is passed. It tests all combinations. Bonus: it tweaks the POD to acknowledge this new parameter. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Hold.t => FAIL: Tests fail! 3. Apply the rest of the patches 4. Repeat 2 => SUCCESS: Tests pass! No warnings! 5. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 115199 [details] [review] Bug 26282: (QA follow-up) Update test Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 115200 [details] [review] Bug 26282: (QA follow-up) Restore default notice
Created attachment 115201 [details] [review] Bug 26282: Update pendingreserves to use buttons This patch updates circ/pendingreserves to use two buttons; "Cancel hold" and "Cancel hold and notify" for the pending reserves page. It will only display the notify option when a notice template exists.
Work to date uploaded.. still needs more followups but I wanted to make sure BZ was in sync with my gitlab before stepping away again
Kyle.. do you think you might have a chance to get back to this.. I'm not sure I will any time soon.
Wasn't this a "have to" for 20.11.01?... Looks like we completely forgot about it.
(In reply to Martin Renvoize from comment #64) > Work to date uploaded.. still needs more followups but I wanted to make sure > BZ was in sync with my gitlab before stepping away again What is still on the todo list?
(In reply to Kyle M Hall from comment #67) > (In reply to Martin Renvoize from comment #64) > > Work to date uploaded.. still needs more followups but I wanted to make sure > > BZ was in sync with my gitlab before stepping away again > > What is still on the todo list? I guess it's coming from comments 51-53.
Created attachment 122448 [details] [review] Bug 26282: Allow staff to decide if a hold cancellation notice will be sent when cancelling a hold Bug 25534 adds the option to send hold cancellation notices when a reason was given. It would be nice if you could give a reason but still decide not to send to the patron. Say the patron called and will not be able to pick up numerous holds for some time. You cancel them all with "patron requested cancellation" but don't want to send multiple notices. I could imagine a checkbox next to the reason to activate/deactivate sending the notice. Test Plan: 1) Apply this patch 2) Visit each area in Koha where a hold can be canceled with a reason 3) Note the new 'Notify patron' checkbox 4) Test canceling a hold with and without this checkbox checked 5) Verify leaving it unchecked does not trigger a notice to be sent to the patron Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 122449 [details] [review] Bug 26282: (QA follow-up) Implement for moremember.pl and circulation.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 122450 [details] [review] Bug 26282: (QA follow-up) Always notify patron if notify checkbox is checked Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 122451 [details] [review] Bug 26282: (QA follow-up) Allow notification via cancel_expired_holds Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 122452 [details] [review] Bug 26282: (QA follow-up) Update POD Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 122453 [details] [review] Bug 26282: (QA follow-up) Unit tests This patch adds tests for the behaviour change. It tests if letter processing takes place when notify_patron is passed. It tests all combinations. Bonus: it tweaks the POD to acknowledge this new parameter. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Hold.t => FAIL: Tests fail! 3. Apply the rest of the patches 4. Repeat 2 => SUCCESS: Tests pass! No warnings! 5. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 122454 [details] [review] Bug 26282: (QA follow-up) Update test Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 122455 [details] [review] Bug 26282: (QA follow-up) Restore default notice
Created attachment 122456 [details] [review] Bug 26282: Show reason only if notice exists
Created attachment 122550 [details] [review] Bug 26282: Update Holds to pull
Doesn't apply anymore :|
Created attachment 138407 [details] [review] Bug 26282: Allow staff to decide if a hold cancellation notice will be sent when cancelling a hold Bug 25534 adds the option to send hold cancellation notices when a reason was given. It would be nice if you could give a reason but still decide not to send to the patron. Say the patron called and will not be able to pick up numerous holds for some time. You cancel them all with "patron requested cancellation" but don't want to send multiple notices. I could imagine a checkbox next to the reason to activate/deactivate sending the notice. Test Plan: 1) Apply this patch 2) Visit each area in Koha where a hold can be canceled with a reason 3) Note the new 'Notify patron' checkbox 4) Test canceling a hold with and without this checkbox checked 5) Verify leaving it unchecked does not trigger a notice to be sent to the patron Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 138408 [details] [review] Bug 26282: (QA follow-up) Implement for moremember.pl and circulation.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 138409 [details] [review] Bug 26282: (QA follow-up) Always notify patron if notify checkbox is checked Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 138410 [details] [review] Bug 26282: Allow staff to decide if a hold cancellation notice will be sent when cancelling a hold Bug 25534 adds the option to send hold cancellation notices when a reason was given. It would be nice if you could give a reason but still decide not to send to the patron. Say the patron called and will not be able to pick up numerous holds for some time. You cancel them all with "patron requested cancellation" but don't want to send multiple notices. I could imagine a checkbox next to the reason to activate/deactivate sending the notice. Test Plan: 1) Apply this patch 2) Visit each area in Koha where a hold can be canceled with a reason 3) Note the new 'Notify patron' checkbox 4) Test canceling a hold with and without this checkbox checked 5) Verify leaving it unchecked does not trigger a notice to be sent to the patron Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138411 [details] [review] Bug 26282: (QA follow-up) Implement for moremember.pl and circulation.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138412 [details] [review] Bug 26282: (QA follow-up) Always notify patron if notify checkbox is checked Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138413 [details] [review] Bug 26282: (QA follow-up) Allow notification via cancel_expired_holds Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138414 [details] [review] Bug 26282: (QA follow-up) Update POD Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138415 [details] [review] Bug 26282: (QA follow-up) Unit tests This patch adds tests for the behaviour change. It tests if letter processing takes place when notify_patron is passed. It tests all combinations. Bonus: it tweaks the POD to acknowledge this new parameter. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Hold.t => FAIL: Tests fail! 3. Apply the rest of the patches 4. Repeat 2 => SUCCESS: Tests pass! No warnings! 5. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138416 [details] [review] Bug 26282: (QA follow-up) Update test Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138417 [details] [review] Bug 26282: (QA follow-up) Restore default notice Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138418 [details] [review] Bug 26282: Show reason only if notice exists Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 138419 [details] [review] Bug 26282: Update Holds to pull Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Happy birthday, here is a rebase [U+1F381] ^o^ I compared the old and new patches to double check that I didn't change anything unknowingly. However failed QA because a test is failing and it's also failing when rebasing on an old(2021-08) master. So it's not from the rebase. #notmyjoblol root@kohadevbox:koha((e781542349...))$ prove t/db_dependent/Koha/Hold.t t/db_dependent/Koha/Hold.t .. 3/4 # Failed test 'GetPreparedLetter not called if notify_patron passed and no cancellation_reason passed' # at t/db_dependent/Koha/Hold.t line 290. # got: '1' # expected: '0' # Looks like you failed 1 test of 12. t/db_dependent/Koha/Hold.t .. 4/4 # Failed test 'cancel() tests' # at t/db_dependent/Koha/Hold.t line 297. # Looks like you failed 1 test of 4. t/db_dependent/Koha/Hold.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/4 subtests Test Summary Report ------------------- t/db_dependent/Koha/Hold.t (Wstat: 256 Tests: 4 Failed: 1) Failed test: 4 Non-zero exit status: 1 Files=1, Tests=4, 10 wallclock secs ( 0.04 usr 0.00 sys + 7.23 cusr 1.52 csys = 8.79 CPU) Result: FAIL
Created attachment 138469 [details] [review] Bug 26282: (QA follow-up) Remove invalid test A cancellation reason is not required, making this test invalid.
Created attachment 141210 [details] [review] Bug 26282: (QA follow-up) Fix typo recieve
I tried testing this, but have run into 2 dead ends: * Manually added a HOLD_CANCELLATION notice * Added email address to my patron, created a bunch of holds * Verified the AV exists and has values * Verified my patron has a primary email set 1) From the patron account * Switch No to Yes for cancelling a hold * Choose reason, leave notify patron checked * Cancelled hold * Hold was cancelled, but: No notice was created 2) Record detail page, holds tab: * Click on "garbage can" - modal comes up without cancellation reason pull down or notify option
(In reply to Katrin Fischer from comment #96) > I tried testing this, but have run into 2 dead ends: > > * Manually added a HOLD_CANCELLATION notice > * Added email address to my patron, created a bunch of holds > * Verified the AV exists and has values > * Verified my patron has a primary email set > > 1) From the patron account > * Switch No to Yes for cancelling a hold > * Choose reason, leave notify patron checked > * Cancelled hold > * Hold was cancelled, but: No notice was created > > 2) Record detail page, holds tab: > * Click on "garbage can" - modal comes up without cancellation reason pull > down or notify option I'd love to see this moving. We initially wanted to fix it right away after introducing the feature...
Until we find such time as to resurrect this, I just wanted to suggest that one could create one or more "SOMETYPE_NONOTICE" and wrap your notice in something like: [%- UNLESS reserves.cancellation_reason.match("NONOTICE") %] [%- END %] That cancelation notice type will end up being blank and thus won't get sent!
(In reply to Kyle M Hall from comment #98) > Until we find such time as to resurrect this, I just wanted to suggest that > one could create one or more "SOMETYPE_NONOTICE" and wrap your notice in > something like: > > [%- UNLESS reserves.cancellation_reason.match("NONOTICE") %] > > [%- END %] > > That cancelation notice type will end up being blank and thus won't get sent! Thx for the workaround :)
This feature would be great, we'd be eager to test if this gets moving again.
Created attachment 171577 [details] [review] Bug 26282: Allow staff to decide if a hold cancellation notice will be sent when cancelling a hold Bug 25534 adds the option to send hold cancellation notices when a reason was given. It would be nice if you could give a reason but still decide not to send to the patron. Say the patron called and will not be able to pick up numerous holds for some time. You cancel them all with "patron requested cancellation" but don't want to send multiple notices. I could imagine a checkbox next to the reason to activate/deactivate sending the notice. Test Plan: 1) Apply this patch 2) Visit each area in Koha where a hold can be canceled with a reason 3) Note the new 'Notify patron' checkbox 4) Test canceling a hold with and without this checkbox checked 5) Verify leaving it unchecked does not trigger a notice to be sent to the patron Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 171578 [details] [review] Bug 26282: (QA follow-up) Implement for moremember.pl and circulation.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 171579 [details] [review] Bug 26282: (QA follow-up) Always notify patron if notify checkbox is checked Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 171580 [details] [review] Bug 26282: (QA follow-up) Allow notification via cancel_expired_holds Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 171581 [details] [review] Bug 26282: (QA follow-up) Update POD Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 171582 [details] [review] Bug 26282: (QA follow-up) Unit tests This patch adds tests for the behaviour change. It tests if letter processing takes place when notify_patron is passed. It tests all combinations. Bonus: it tweaks the POD to acknowledge this new parameter. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Hold.t => FAIL: Tests fail! 3. Apply the rest of the patches 4. Repeat 2 => SUCCESS: Tests pass! No warnings! 5. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 171583 [details] [review] Bug 26282: (QA follow-up) Update test Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 171584 [details] [review] Bug 26282: (QA follow-up) Restore default notice Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 171585 [details] [review] Bug 26282: Show reason only if notice exists Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 171586 [details] [review] Bug 26282: Update Holds to pull Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 171587 [details] [review] Bug 26282: (QA follow-up) Remove invalid test A cancellation reason is not required, making this test invalid.
Created attachment 171588 [details] [review] Bug 26282: (QA follow-up) Fix typo recieve
Created attachment 171589 [details] [review] Bug 26282: (QA follow-up) Tidy code for qa script
Created attachment 171590 [details] [review] Bug 26282: (QA follow-up) Fix typo cancelation
Created attachment 171592 [details] [review] Bug 26282: Allow staff to decide if a hold cancellation notice will be sent when cancelling a hold Bug 25534 adds the option to send hold cancellation notices when a reason was given. It would be nice if you could give a reason but still decide not to send to the patron. Say the patron called and will not be able to pick up numerous holds for some time. You cancel them all with "patron requested cancellation" but don't want to send multiple notices. I could imagine a checkbox next to the reason to activate/deactivate sending the notice. Test Plan: 1) Apply this patch 2) Visit each area in Koha where a hold can be canceled with a reason 3) Note the new 'Notify patron' checkbox 4) Test canceling a hold with and without this checkbox checked 5) Verify leaving it unchecked does not trigger a notice to be sent to the patron Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 171593 [details] [review] Bug 26282: (QA follow-up) Tidy code for qa script
Patches squashed for readability. Final tidy kept separate to prevent future rebase issues.
This poor bug. I really wanted to sign off, but something, probably Bootstrap5, has made the cancellation popup in reserves/request.tt too narrow to reasonably display the checkbox unless your cancellation reasons are quite terse. With the default data, I get Cancellation reason [ No reason given ⌄ ] [ ] Notify patron and a checkbox on one line and the label on another is ungood.
This is indeed a poor bug and major too. :(
Handy solution to the wrapping between checkbox and label back in comment 18 - use two buttons, Cancel and Cancel and notify, rather than one button with different behavior based on a checkbox.
> Say the patron called and will not be able to pick up numerous holds for > some time. You cancel them all with "patron requested cancellation" but > don't want to send multiple notices. Good reason to make that one digestable in the future? Plugging Bug 37760 here. :)