Bug 26282 - Allow staff to decide if a hold cancellation notice will be sent when cancelling a hold
Summary: Allow staff to decide if a hold cancellation notice will be sent when cancell...
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Tomás Cohen Arazi
URL:
Keywords: rel_20_11_candidate
Depends on: 27103
Blocks: 27262
  Show dependency treegraph
 
Reported: 2020-08-22 09:54 UTC by Katrin Fischer
Modified: 2021-08-23 14:26 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 26282: Allow staff to decide if a hold cancellation notice will be sent when cancelling a hold (8.44 KB, patch)
2020-08-28 14:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Implement for moremember.pl and circulation.pl (2.40 KB, patch)
2020-11-24 16:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Always notify patron if notify checkbox is checked (921 bytes, patch)
2020-11-25 15:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26282: Allow staff to decide if a hold cancellation notice will be sent when cancelling a hold (8.49 KB, patch)
2020-11-26 13:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Implement for moremember.pl and circulation.pl (2.44 KB, patch)
2020-11-26 13:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Always notify patron if notify checkbox is checked (970 bytes, patch)
2020-11-26 13:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Allow notification via cancel_expired_holds (2.65 KB, patch)
2020-11-26 13:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26282: Unit tests (4.34 KB, patch)
2020-11-26 15:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26282: Allow staff to decide if a hold cancellation notice will be sent when cancelling a hold (8.56 KB, patch)
2020-11-26 15:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Implement for moremember.pl and circulation.pl (2.50 KB, patch)
2020-11-26 15:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Always notify patron if notify checkbox is checked (1.00 KB, patch)
2020-11-26 15:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Allow notification via cancel_expired_holds (2.70 KB, patch)
2020-11-26 15:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Update POD (744 bytes, patch)
2020-11-26 15:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26282: Disable the checkbox if no reason selected (5.73 KB, patch)
2020-11-27 08:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26282: Unit tests (4.40 KB, patch)
2020-11-27 09:08 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 26282: Allow staff to decide if a hold cancellation notice will be sent when cancelling a hold (8.66 KB, patch)
2020-11-27 09:08 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Implement for moremember.pl and circulation.pl (2.55 KB, patch)
2020-11-27 09:08 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 26282: Disable the checkbox if no reason selected (5.81 KB, patch)
2020-11-27 09:08 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Update test (1.03 KB, patch)
2020-11-27 09:08 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 26282: (ALTERNATIVE) Remove HOLD_CANCELLATION notice (20.06 KB, patch)
2020-11-27 09:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26282: Allow staff to decide if a hold cancellation notice will be sent when cancelling a hold (8.60 KB, patch)
2020-12-08 11:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Implement for moremember.pl and circulation.pl (2.55 KB, patch)
2020-12-08 11:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Always notify patron if notify checkbox is checked (1.05 KB, patch)
2020-12-08 11:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Allow notification via cancel_expired_holds (2.75 KB, patch)
2020-12-08 11:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Update POD (862 bytes, patch)
2020-12-08 11:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Unit tests (4.48 KB, patch)
2020-12-08 11:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Update test (1.14 KB, patch)
2020-12-08 11:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Button approach (86.28 KB, image/png)
2020-12-08 11:43 UTC, Martin Renvoize
Details
Checkbox aproach (27.25 KB, image/png)
2020-12-08 11:44 UTC, Martin Renvoize
Details
Bug 26282: (QA follow-up) Restore default notice (1.48 KB, patch)
2020-12-08 14:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26282: Allow staff to decide if a hold cancellation notice will be sent when cancelling a hold (8.60 KB, patch)
2021-01-15 13:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Implement for moremember.pl and circulation.pl (2.55 KB, patch)
2021-01-15 13:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Always notify patron if notify checkbox is checked (1.05 KB, patch)
2021-01-15 13:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Allow notification via cancel_expired_holds (2.75 KB, patch)
2021-01-15 13:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Update POD (862 bytes, patch)
2021-01-15 13:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Unit tests (4.50 KB, patch)
2021-01-15 13:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Update test (1.14 KB, patch)
2021-01-15 13:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Restore default notice (1.48 KB, patch)
2021-01-15 13:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26282: Update pendingreserves to use buttons (4.17 KB, patch)
2021-01-15 13:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26282: Allow staff to decide if a hold cancellation notice will be sent when cancelling a hold (8.62 KB, patch)
2021-06-25 17:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Implement for moremember.pl and circulation.pl (2.57 KB, patch)
2021-06-25 17:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Always notify patron if notify checkbox is checked (1.07 KB, patch)
2021-06-25 17:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Allow notification via cancel_expired_holds (2.77 KB, patch)
2021-06-25 17:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Update POD (878 bytes, patch)
2021-06-25 17:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Unit tests (4.52 KB, patch)
2021-06-25 17:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Update test (1.16 KB, patch)
2021-06-25 17:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26282: (QA follow-up) Restore default notice (1.40 KB, patch)
2021-06-25 17:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26282: Show reason only if notice exists (5.12 KB, patch)
2021-06-25 17:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26282: Update Holds to pull (3.21 KB, patch)
2021-07-02 15:08 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2020-08-22 09:54:09 UTC
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.
Comment 1 Kyle M Hall 2020-08-28 14:16:57 UTC
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
Comment 2 Victor Grousset/tuxayo 2020-11-08 17:59:46 UTC
> 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?
Comment 3 Jonathan Druart 2020-11-09 09:43:47 UTC
Considering this one a bug fix.
Comment 4 Jonathan Druart 2020-11-13 14:04:25 UTC
Kyle, this is blocker for the release. Can you have another try please?
Comment 5 Kyle M Hall 2020-11-24 16:24:11 UTC
Created attachment 113960 [details] [review]
Bug 26282: (QA follow-up) Implement for moremember.pl and circulation.pl
Comment 6 Victor Grousset/tuxayo 2020-11-25 14:42:07 UTC
Potential issue found: "No reason given" => no notification sent
Comment 7 Kyle M Hall 2020-11-25 15:09:28 UTC
Created attachment 113991 [details] [review]
Bug 26282: (QA follow-up) Always notify patron if notify checkbox is checked
Comment 8 Jonathan Druart 2020-11-25 15:13:45 UTC
(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?
Comment 9 Kyle M Hall 2020-11-25 15:32:58 UTC
(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.
Comment 10 Tomás Cohen Arazi 2020-11-25 15:33:33 UTC
(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.
Comment 11 Jonathan Druart 2020-11-25 16:28:18 UTC
(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.
Comment 12 Josef Moravec 2020-11-25 19:18:11 UTC
(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.
Comment 13 Martin Renvoize 2020-11-26 11:42:17 UTC
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
Comment 14 Martin Renvoize 2020-11-26 13:28:11 UTC
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>
Comment 15 Martin Renvoize 2020-11-26 13:28:14 UTC
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>
Comment 16 Martin Renvoize 2020-11-26 13:28:18 UTC
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>
Comment 17 Martin Renvoize 2020-11-26 13:28:21 UTC
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>
Comment 18 Martin Renvoize 2020-11-26 13:32:06 UTC
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?
Comment 19 Jonathan Druart 2020-11-26 13:54:06 UTC
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...
Comment 20 Tomás Cohen Arazi 2020-11-26 15:11:20 UTC
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>
Comment 21 Tomás Cohen Arazi 2020-11-26 15:11:30 UTC
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>
Comment 22 Tomás Cohen Arazi 2020-11-26 15:11:35 UTC
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>
Comment 23 Tomás Cohen Arazi 2020-11-26 15:11:40 UTC
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>
Comment 24 Tomás Cohen Arazi 2020-11-26 15:11:45 UTC
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>
Comment 25 Tomás Cohen Arazi 2020-11-26 15:11:53 UTC
Created attachment 114016 [details] [review]
Bug 26282: (QA follow-up) Update POD

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Jonathan Druart 2020-11-26 15:25:32 UTC
The letter is:
"""
Your hold for De Crasi Aristophea was canceled for the following reason: 
"""
if you don't select a reason...
Comment 27 Jonathan Druart 2020-11-26 15:50:39 UTC
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...).
Comment 28 Jonathan Druart 2020-11-27 08:35:18 UTC
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.
Comment 29 Jonathan Druart 2020-11-27 08:35:57 UTC
I am really happy to have to deal with a checkbox on release day!
Comment 30 Josef Moravec 2020-11-27 09:08:31 UTC
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>
Comment 31 Josef Moravec 2020-11-27 09:08:35 UTC
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>
Comment 32 Josef Moravec 2020-11-27 09:08:40 UTC
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>
Comment 33 Josef Moravec 2020-11-27 09:08:44 UTC
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>
Comment 34 Josef Moravec 2020-11-27 09:08:49 UTC
Created attachment 114034 [details] [review]
Bug 26282: (QA follow-up) Update test

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 35 Jonathan Druart 2020-11-27 09:11:19 UTC
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.
Comment 36 Martin Renvoize 2020-11-27 09:27:34 UTC
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.
Comment 37 Martin Renvoize 2020-11-27 09:29:20 UTC
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.
Comment 38 Josef Moravec 2020-11-27 10:08:22 UTC
(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!
Comment 39 Martin Renvoize 2020-12-08 11:38:03 UTC
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>
Comment 40 Martin Renvoize 2020-12-08 11:38:07 UTC
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>
Comment 41 Martin Renvoize 2020-12-08 11:38:12 UTC
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>
Comment 42 Martin Renvoize 2020-12-08 11:38:16 UTC
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>
Comment 43 Martin Renvoize 2020-12-08 11:38:20 UTC
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>
Comment 44 Martin Renvoize 2020-12-08 11:38:24 UTC
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>
Comment 45 Martin Renvoize 2020-12-08 11:38:28 UTC
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>
Comment 46 Martin Renvoize 2020-12-08 11:43:19 UTC
Created attachment 114254 [details]
Button approach
Comment 47 Martin Renvoize 2020-12-08 11:44:44 UTC
Created attachment 114255 [details]
Checkbox aproach
Comment 48 Martin Renvoize 2020-12-08 11:47:31 UTC
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.
Comment 49 Martin Renvoize 2020-12-08 14:40:11 UTC
Created attachment 114259 [details] [review]
Bug 26282: (QA follow-up) Restore default notice
Comment 50 Martin Renvoize 2020-12-17 14:52:55 UTC
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
Comment 51 Jonathan Druart 2020-12-17 15:50:37 UTC
(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?
Comment 52 Martin Renvoize 2020-12-17 16:40:35 UTC
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
Comment 53 Jonathan Druart 2020-12-18 08:35:19 UTC
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).
Comment 54 Martin Renvoize 2021-01-15 13:14:28 UTC
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.
Comment 55 Martin Renvoize 2021-01-15 13:21:08 UTC
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>
Comment 56 Martin Renvoize 2021-01-15 13:21:12 UTC
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>
Comment 57 Martin Renvoize 2021-01-15 13:21:16 UTC
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>
Comment 58 Martin Renvoize 2021-01-15 13:21:20 UTC
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>
Comment 59 Martin Renvoize 2021-01-15 13:21:24 UTC
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>
Comment 60 Martin Renvoize 2021-01-15 13:21:28 UTC
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>
Comment 61 Martin Renvoize 2021-01-15 13:21:32 UTC
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>
Comment 62 Martin Renvoize 2021-01-15 13:21:35 UTC
Created attachment 115200 [details] [review]
Bug 26282: (QA follow-up) Restore default notice
Comment 63 Martin Renvoize 2021-01-15 13:21:39 UTC
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.
Comment 64 Martin Renvoize 2021-01-15 13:22:36 UTC
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
Comment 65 Martin Renvoize 2021-06-15 13:55:30 UTC
Kyle.. do you think you might have a chance to get back to this.. I'm not sure I will any time soon.
Comment 66 Jonathan Druart 2021-06-15 14:21:07 UTC
Wasn't this a "have to" for 20.11.01?...

Looks like we completely forgot about it.
Comment 67 Kyle M Hall 2021-06-17 14:07:06 UTC
(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?
Comment 68 Jonathan Druart 2021-06-17 14:15:19 UTC
(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.
Comment 69 Kyle M Hall 2021-06-25 17:02:07 UTC
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>
Comment 70 Kyle M Hall 2021-06-25 17:02:26 UTC
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>
Comment 71 Kyle M Hall 2021-06-25 17:02:30 UTC
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>
Comment 72 Kyle M Hall 2021-06-25 17:02:35 UTC
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>
Comment 73 Kyle M Hall 2021-06-25 17:02:39 UTC
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>
Comment 74 Kyle M Hall 2021-06-25 17:02:43 UTC
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>
Comment 75 Kyle M Hall 2021-06-25 17:02:47 UTC
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>
Comment 76 Kyle M Hall 2021-06-25 17:02:51 UTC
Created attachment 122455 [details] [review]
Bug 26282: (QA follow-up) Restore default notice
Comment 77 Kyle M Hall 2021-06-25 17:02:55 UTC
Created attachment 122456 [details] [review]
Bug 26282: Show reason only if notice exists
Comment 78 Kyle M Hall 2021-07-02 15:08:23 UTC
Created attachment 122550 [details] [review]
Bug 26282: Update Holds to pull