Bug 25534 - Add ability to specifying and store a reason when cancelling a hold
Summary: Add ability to specifying and store a reason when cancelling a hold
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
: 20453 (view as bug list)
Depends on:
Blocks: 12656 25260 26281 27331 27103
  Show dependency treegraph
 
Reported: 2020-05-18 16:50 UTC by Kyle M Hall
Modified: 2021-01-04 13:44 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Text to go in the release notes:
This new feature adds an option to allow staff to add a reason for cancellation when cancelling a hold. The new 'CANCELLATION_REASON' authorized value is used to provide a configurable pick list of reasons. Optionally, the library may choose to define a 'HOLD_CANCELLATION' notice which will be sent whenever a cancellation reason is assigned.
Version(s) released in:
20.11.00


Attachments
Bug 25534: Update database (2.89 KB, patch)
2020-05-18 18:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Add ability to send an email specifying a reason when canceling a hold (12.73 KB, patch)
2020-05-18 18:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Update Schema Files [DO NOT PUSH] (2.78 KB, patch)
2020-05-18 18:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Update database (2.89 KB, patch)
2020-05-19 11:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Add ability to send an email specifying a reason when canceling a hold (12.52 KB, patch)
2020-05-19 11:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Update Schema Files [DO NOT PUSH] (2.78 KB, patch)
2020-05-19 11:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Use the cancelation reasion for the 'X' hold cancelation links (2.81 KB, patch)
2020-05-19 17:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Add reason to pendingreserves.pl (2.97 KB, patch)
2020-05-19 17:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Update database (2.89 KB, patch)
2020-05-19 18:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Add ability to send an email specifying a reason when canceling a hold (13.15 KB, patch)
2020-05-19 18:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Update Schema Files [DO NOT PUSH] (2.78 KB, patch)
2020-05-19 18:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Use the cancelation reasion for the 'X' hold cancelation links (2.81 KB, patch)
2020-05-19 18:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Add reason to pendingreserves.pl (2.97 KB, patch)
2020-05-19 18:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Update database (2.89 KB, patch)
2020-05-19 18:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Add ability to send an email specifying a reason when canceling a hold (13.14 KB, patch)
2020-05-19 18:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Update Schema Files [DO NOT PUSH] (2.78 KB, patch)
2020-05-19 18:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Use the cancelation reasion for the 'X' hold cancelation links (2.81 KB, patch)
2020-05-19 18:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Add reason to pendingreserves.pl (2.97 KB, patch)
2020-05-19 18:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Update database (2.96 KB, patch)
2020-05-19 20:15 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25534: Add ability to send an email specifying a reason when canceling a hold (13.22 KB, patch)
2020-05-19 20:15 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25534: Update Schema Files [DO NOT PUSH] (2.83 KB, patch)
2020-05-19 20:15 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25534: Use the cancelation reasion for the 'X' hold cancelation links (2.88 KB, patch)
2020-05-19 20:15 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25534: Add reason to pendingreserves.pl (3.04 KB, patch)
2020-05-19 20:15 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25534: Update database (3.01 KB, patch)
2020-05-20 14:24 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25534: Add ability to send an email specifying a reason when canceling a hold (13.27 KB, patch)
2020-05-20 14:24 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25534: Update Schema Files [DO NOT PUSH] (2.88 KB, patch)
2020-05-20 14:24 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25534: Use the cancelation reasion for the 'X' hold cancelation links (2.93 KB, patch)
2020-05-20 14:24 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25534: Add reason to pendingreserves.pl (3.09 KB, patch)
2020-05-20 14:24 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25534: Update database (3.01 KB, patch)
2020-08-18 12:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Update database (3.01 KB, patch)
2020-08-18 12:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Add ability to send an email specifying a reason when canceling a hold (12.95 KB, patch)
2020-08-18 12:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Update Schema Files [DO NOT PUSH] (2.90 KB, patch)
2020-08-18 12:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Use the cancelation reasion for the 'X' hold cancelation links (2.93 KB, patch)
2020-08-18 12:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Add reason to pendingreserves.pl (3.09 KB, patch)
2020-08-18 12:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Update database (3.01 KB, patch)
2020-08-20 17:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Add ability to send an email specifying a reason when canceling a hold (12.95 KB, patch)
2020-08-20 17:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Update Schema Files [DO NOT PUSH] (2.90 KB, patch)
2020-08-20 17:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Use the cancelation reasion for the 'X' hold cancelation links (2.93 KB, patch)
2020-08-20 17:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Add reason to pendingreserves.pl (3.09 KB, patch)
2020-08-20 17:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: (QA follow-up) Add missing TT filters (1.89 KB, patch)
2020-08-20 17:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: (QA follow-up) Fix typo (930 bytes, patch)
2020-08-20 17:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: (QA follow-up) Unit tests (6.41 KB, patch)
2020-08-20 17:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: (QA follow-up) Add sample notice (2.48 KB, patch)
2020-08-20 17:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: (QA follow-up) Add label to reason pulldown (1.25 KB, patch)
2020-08-20 17:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: (QA follow-up) Add AV category (3.30 KB, patch)
2020-08-20 17:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: (QA follow-up) Add colon to label on pendingreserves.pl (1.32 KB, patch)
2020-08-20 17:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: (QA follow-up) Add default values for new AV category (10.31 KB, patch)
2020-08-21 10:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: (QA follow-up) Add sample notices for translators (7.92 KB, patch)
2020-08-21 11:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Update database (3.07 KB, patch)
2020-08-22 09:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25534: Add ability to send an email specifying a reason when canceling a hold (13.03 KB, patch)
2020-08-22 09:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25534: Update Schema Files [DO NOT PUSH] (2.94 KB, patch)
2020-08-22 09:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25534: Use the cancelation reasion for the 'X' hold cancelation links (2.99 KB, patch)
2020-08-22 09:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25534: Add reason to pendingreserves.pl (3.14 KB, patch)
2020-08-22 09:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25534: (QA follow-up) Add missing TT filters (1.93 KB, patch)
2020-08-22 09:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25534: (QA follow-up) Fix typo (971 bytes, patch)
2020-08-22 09:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25534: (QA follow-up) Unit tests (6.46 KB, patch)
2020-08-22 09:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25534: (QA follow-up) Add sample notice (2.54 KB, patch)
2020-08-22 09:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25534: (QA follow-up) Add label to reason pulldown (1.29 KB, patch)
2020-08-22 09:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25534: (QA follow-up) Add AV category (3.36 KB, patch)
2020-08-22 09:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25534: (QA follow-up) Add colon to label on pendingreserves.pl (1.36 KB, patch)
2020-08-22 09:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25534: (QA follow-up) Add default values for new AV category (10.44 KB, patch)
2020-08-22 09:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25534: (QA follow-up) Add sample notices for translators (7.96 KB, patch)
2020-08-22 09:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25534: Use build_sample_item in tests (2.53 KB, patch)
2020-08-24 09:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25534: (QA follow-up) Update Koha::Hold::cancel POD (1.10 KB, patch)
2020-08-24 13:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: (QA follow-up) Removed doubled insert statements in de-DE/optional/auth_val.sql (1.36 KB, patch)
2020-08-24 13:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: (QA follow-up) Use modal for cancel links, hide reason unless priority is set to 'del' (9.26 KB, patch)
2020-08-24 14:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25534: Add missing td in tfoot in pendingreserves (1.18 KB, patch)
2020-11-27 08:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25534: Add missing td in tfoot in pendingreserves (841 bytes, patch)
2020-11-27 08:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25534: (QA follow-up 2) Add default values for new AV category (1.72 KB, patch)
2021-01-04 13:42 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2020-05-18 16:50:11 UTC
Some libraries would like to be able to cancel a hold with the option to specify a reason. Providing a reason would generate an email to that patron.
Comment 1 Kyle M Hall 2020-05-18 18:11:07 UTC
Created attachment 105034 [details] [review]
Bug 25534: Update database
Comment 2 Kyle M Hall 2020-05-18 18:11:15 UTC
Created attachment 105035 [details] [review]
Bug 25534: Add ability to send an email specifying a reason when canceling a hold

Some libraries would like to be able to cancel a hold with the option to
specify a reason. Providing a reason would generate an email to that
patron.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Restart all the things!
4) Create new AV category "HOLD_CANCELLATION", add some cancelation reasons
5) Add new Holds module notice "HOLD_CANCELLATION", add an email version.
   A quick test version would be "Reason: <<reserves.cancellation_reason>>"
6) Place a hold for a patron
7) On request.pl, select the 'del' option for the hold,
   not the reason pulldown appears.
8) Select a reason and choose "Update hold(s)"
9) Note a new message has been queue for the patron with the cancelation reason
11) Test again from circulation.pl
12) Test again from moremember.pl
10) Cancel a hold with no reason, note no email is generated
11) Delete you authorised values, not the feature is disabled
Comment 3 Kyle M Hall 2020-05-18 18:11:18 UTC
Created attachment 105036 [details] [review]
Bug 25534: Update Schema Files [DO NOT PUSH]
Comment 4 Kyle M Hall 2020-05-19 11:05:33 UTC
Created attachment 105077 [details] [review]
Bug 25534: Update database
Comment 5 Kyle M Hall 2020-05-19 11:05:42 UTC
Created attachment 105078 [details] [review]
Bug 25534: Add ability to send an email specifying a reason when canceling a hold

Some libraries would like to be able to cancel a hold with the option to
specify a reason. Providing a reason would generate an email to that
patron.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Restart all the things!
4) Create new AV category "HOLD_CANCELLATION", add some cancelation reasons
5) Add new Holds module notice "HOLD_CANCELLATION", add an email version.
   A quick test version would be "Reason: <<reserves.cancellation_reason>>"
6) Place a hold for a patron
7) On request.pl, select the 'del' option for the hold
8) Select a cancellation reason and choose "Update hold(s)"
9) Note a new message has been queue for the patron with the cancelation reason
11) Test again from circulation.pl
12) Test again from moremember.pl
10) Cancel a hold with no reason, note no email is generated
11) Delete your authorised values, not the feature is disabled
12) Reinstate the authorised values, but delete the notice,
    you should now be able to cancel a hold with a reason,
    but no email will be generated
Comment 6 Kyle M Hall 2020-05-19 11:05:44 UTC
Created attachment 105079 [details] [review]
Bug 25534: Update Schema Files [DO NOT PUSH]
Comment 7 Andrew Fuerste-Henry 2020-05-19 14:36:10 UTC
On request.pl, the notice doesn't send if you cancel the hold using the big red X button. This is the more obvious way to cancel a hold, so if it doesn't send a notice then folks are going to get confused.

Additionally, I've got "<<reserves.cancellation_reason>>" in my notice and it's putting in the authorised value code rather than the description, which is no good.
Comment 8 Andrew Fuerste-Henry 2020-05-19 14:52:27 UTC
Also need to account for cancelation via pendingreserves.pl
Comment 9 Kyle M Hall 2020-05-19 14:59:12 UTC
(In reply to Andrew Fuerste-Henry from comment #7)
> On request.pl, the notice doesn't send if you cancel the hold using the big
> red X button. This is the more obvious way to cancel a hold, so if it
> doesn't send a notice then folks are going to get confused.

Will fix.

> Additionally, I've got "<<reserves.cancellation_reason>>" in my notice and
> it's putting in the authorised value code rather than the description, which
> is no good.

I should have specified a TT sample notice in the test plan. Will fix.
Comment 10 Kyle M Hall 2020-05-19 17:25:13 UTC
Created attachment 105105 [details] [review]
Bug 25534: Use the cancelation reasion for the 'X' hold cancelation links
Comment 11 Kyle M Hall 2020-05-19 17:44:10 UTC
Created attachment 105106 [details] [review]
Bug 25534: Add reason to pendingreserves.pl
Comment 12 Kyle M Hall 2020-05-19 18:43:17 UTC
Created attachment 105108 [details] [review]
Bug 25534: Update database
Comment 13 Kyle M Hall 2020-05-19 18:43:27 UTC
Created attachment 105109 [details] [review]
Bug 25534: Add ability to send an email specifying a reason when canceling a hold

Some libraries would like to be able to cancel a hold with the option to
specify a reason. Providing a reason would generate an email to that
patron.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Restart all the things!
4) Create new AV category "HOLD_CANCELLATION", add some cancelation reasons
5) Add new Holds module notice "HOLD_CANCELLATION", add an email version.
   A quick test version would be "Reason: <<reserves.cancellation_reason>>"
--
[% USE AuthorisedValues %]
Reason: [% AuthorisedValues.GetByCode( 'CANCELLATION_REASON', [% hold.cancellation_reason %], 'IS_OPAC' ) %]

[% IF hold.cancellation_reason == "MY_AV_VALUE" %]
IF perhaps you'd like to have a much longer explanation than just the
one sentence in the AV description, you can use IF blocks using Template
Toolkit markup!
[% END %]
--
6) Place a hold for a patron
7) On request.pl, select the 'del' option for the hold
8) Select a cancellation reason and choose "Update hold(s)"
9) Note a new message has been queue for the patron with the cancelation reason
11) Test again from circulation.pl
12) Test again from moremember.pl
10) Cancel a hold with no reason, note no email is generated
11) Delete your authorised values, not the feature is disabled
12) Reinstate the authorised values, but delete the notice,
    you should now be able to cancel a hold with a reason,
    but no email will be generated
Comment 14 Kyle M Hall 2020-05-19 18:43:30 UTC
Created attachment 105110 [details] [review]
Bug 25534: Update Schema Files [DO NOT PUSH]
Comment 15 Kyle M Hall 2020-05-19 18:43:33 UTC
Created attachment 105111 [details] [review]
Bug 25534: Use the cancelation reasion for the 'X' hold cancelation links
Comment 16 Kyle M Hall 2020-05-19 18:43:37 UTC
Created attachment 105112 [details] [review]
Bug 25534: Add reason to pendingreserves.pl
Comment 17 Kyle M Hall 2020-05-19 18:57:36 UTC
Created attachment 105113 [details] [review]
Bug 25534: Update database
Comment 18 Kyle M Hall 2020-05-19 18:57:47 UTC
Created attachment 105114 [details] [review]
Bug 25534: Add ability to send an email specifying a reason when canceling a hold

Some libraries would like to be able to cancel a hold with the option to
specify a reason. Providing a reason would generate an email to that
patron.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Restart all the things!
4) Create new AV category "HOLD_CANCELLATION", add some cancelation reasons
5) Add new Holds module notice "HOLD_CANCELLATION", add an email version.
   A quick test version would be "Reason: <<reserves.cancellation_reason>>"
--
[% USE AuthorisedValues %]
Reason: [% AuthorisedValues.GetByCode( 'CANCELLATION_REASON', hold.cancellation_reason, 'IS_OPAC' ) %]

[% IF hold.cancellation_reason == "MY_AV_VALUE" %]
IF perhaps you'd like to have a much longer explanation than just the
one sentence in the AV description, you can use IF blocks using Template
Toolkit markup!
[% END %]
--
6) Place a hold for a patron
7) On request.pl, select the 'del' option for the hold
8) Select a cancellation reason and choose "Update hold(s)"
9) Note a new message has been queue for the patron with the cancelation reason
11) Test again from circulation.pl
12) Test again from moremember.pl
10) Cancel a hold with no reason, note no email is generated
11) Delete your authorised values, not the feature is disabled
12) Reinstate the authorised values, but delete the notice,
    you should now be able to cancel a hold with a reason,
    but no email will be generated
Comment 19 Kyle M Hall 2020-05-19 18:57:49 UTC
Created attachment 105115 [details] [review]
Bug 25534: Update Schema Files [DO NOT PUSH]
Comment 20 Kyle M Hall 2020-05-19 18:57:52 UTC
Created attachment 105116 [details] [review]
Bug 25534: Use the cancelation reasion for the 'X' hold cancelation links
Comment 21 Kyle M Hall 2020-05-19 18:57:56 UTC
Created attachment 105117 [details] [review]
Bug 25534: Add reason to pendingreserves.pl
Comment 22 Andrew Fuerste-Henry 2020-05-19 20:15:09 UTC
Created attachment 105118 [details] [review]
Bug 25534: Update database

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 23 Andrew Fuerste-Henry 2020-05-19 20:15:13 UTC
Created attachment 105119 [details] [review]
Bug 25534: Add ability to send an email specifying a reason when canceling a hold

Some libraries would like to be able to cancel a hold with the option to
specify a reason. Providing a reason would generate an email to that
patron.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Restart all the things!
4) Create new AV category "HOLD_CANCELLATION", add some cancelation reasons
5) Add new Holds module notice "HOLD_CANCELLATION", add an email version.
   A quick test version would be "Reason: <<reserves.cancellation_reason>>"
--
[% USE AuthorisedValues %]
Reason: [% AuthorisedValues.GetByCode( 'CANCELLATION_REASON', hold.cancellation_reason, 'IS_OPAC' ) %]

[% IF hold.cancellation_reason == "MY_AV_VALUE" %]
IF perhaps you'd like to have a much longer explanation than just the
one sentence in the AV description, you can use IF blocks using Template
Toolkit markup!
[% END %]
--
6) Place a hold for a patron
7) On request.pl, select the 'del' option for the hold
8) Select a cancellation reason and choose "Update hold(s)"
9) Note a new message has been queue for the patron with the cancelation reason
11) Test again from circulation.pl
12) Test again from moremember.pl
10) Cancel a hold with no reason, note no email is generated
11) Delete your authorised values, not the feature is disabled
12) Reinstate the authorised values, but delete the notice,
    you should now be able to cancel a hold with a reason,
    but no email will be generated

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 24 Andrew Fuerste-Henry 2020-05-19 20:15:16 UTC
Created attachment 105120 [details] [review]
Bug 25534: Update Schema Files [DO NOT PUSH]

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 25 Andrew Fuerste-Henry 2020-05-19 20:15:21 UTC
Created attachment 105121 [details] [review]
Bug 25534: Use the cancelation reasion for the 'X' hold cancelation links

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 26 Andrew Fuerste-Henry 2020-05-19 20:15:24 UTC
Created attachment 105122 [details] [review]
Bug 25534: Add reason to pendingreserves.pl

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 27 ByWater Sandboxes 2020-05-20 14:24:19 UTC
Created attachment 105147 [details] [review]
Bug 25534: Update database

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Comment 28 ByWater Sandboxes 2020-05-20 14:24:22 UTC
Created attachment 105148 [details] [review]
Bug 25534: Add ability to send an email specifying a reason when canceling a hold

Some libraries would like to be able to cancel a hold with the option to
specify a reason. Providing a reason would generate an email to that
patron.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Restart all the things!
4) Create new AV category "HOLD_CANCELLATION", add some cancelation reasons
5) Add new Holds module notice "HOLD_CANCELLATION", add an email version.
   A quick test version would be "Reason: <<reserves.cancellation_reason>>"
--
[% USE AuthorisedValues %]
Reason: [% AuthorisedValues.GetByCode( 'CANCELLATION_REASON', hold.cancellation_reason, 'IS_OPAC' ) %]

[% IF hold.cancellation_reason == "MY_AV_VALUE" %]
IF perhaps you'd like to have a much longer explanation than just the
one sentence in the AV description, you can use IF blocks using Template
Toolkit markup!
[% END %]
--
6) Place a hold for a patron
7) On request.pl, select the 'del' option for the hold
8) Select a cancellation reason and choose "Update hold(s)"
9) Note a new message has been queue for the patron with the cancelation reason
11) Test again from circulation.pl
12) Test again from moremember.pl
10) Cancel a hold with no reason, note no email is generated
11) Delete your authorised values, not the feature is disabled
12) Reinstate the authorised values, but delete the notice,
    you should now be able to cancel a hold with a reason,
    but no email will be generated

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Comment 29 ByWater Sandboxes 2020-05-20 14:24:24 UTC
Created attachment 105149 [details] [review]
Bug 25534: Update Schema Files [DO NOT PUSH]

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Comment 30 ByWater Sandboxes 2020-05-20 14:24:27 UTC
Created attachment 105150 [details] [review]
Bug 25534: Use the cancelation reasion for the 'X' hold cancelation links

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Comment 31 ByWater Sandboxes 2020-05-20 14:24:30 UTC
Created attachment 105151 [details] [review]
Bug 25534: Add reason to pendingreserves.pl

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Comment 32 Katrin Fischer 2020-08-16 09:43:48 UTC
Patches no longer apply cleanly, please rebase!
Comment 33 Kyle M Hall 2020-08-18 12:31:06 UTC
Created attachment 108476 [details] [review]
Bug 25534: Update database

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Comment 34 Kyle M Hall 2020-08-18 12:33:04 UTC
Created attachment 108477 [details] [review]
Bug 25534: Update database

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Comment 35 Kyle M Hall 2020-08-18 12:33:14 UTC
Created attachment 108478 [details] [review]
Bug 25534: Add ability to send an email specifying a reason when canceling a hold

Some libraries would like to be able to cancel a hold with the option to
specify a reason. Providing a reason would generate an email to that
patron.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Restart all the things!
4) Create new AV category "HOLD_CANCELLATION", add some cancelation reasons
5) Add new Holds module notice "HOLD_CANCELLATION", add an email version.
   A quick test version would be "Reason: <<reserves.cancellation_reason>>"
--
[% USE AuthorisedValues %]
Reason: [% AuthorisedValues.GetByCode( 'CANCELLATION_REASON', hold.cancellation_reason, 'IS_OPAC' ) %]

[% IF hold.cancellation_reason == "MY_AV_VALUE" %]
IF perhaps you'd like to have a much longer explanation than just the
one sentence in the AV description, you can use IF blocks using Template
Toolkit markup!
[% END %]
--
6) Place a hold for a patron
7) On request.pl, select the 'del' option for the hold
8) Select a cancellation reason and choose "Update hold(s)"
9) Note a new message has been queue for the patron with the cancelation reason
11) Test again from circulation.pl
12) Test again from moremember.pl
10) Cancel a hold with no reason, note no email is generated
11) Delete your authorised values, not the feature is disabled
12) Reinstate the authorised values, but delete the notice,
    you should now be able to cancel a hold with a reason,
    but no email will be generated

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Comment 36 Kyle M Hall 2020-08-18 12:33:18 UTC
Created attachment 108479 [details] [review]
Bug 25534: Update Schema Files [DO NOT PUSH]

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Comment 37 Kyle M Hall 2020-08-18 12:33:22 UTC
Created attachment 108480 [details] [review]
Bug 25534: Use the cancelation reasion for the 'X' hold cancelation links

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Comment 38 Kyle M Hall 2020-08-18 12:33:26 UTC
Created attachment 108481 [details] [review]
Bug 25534: Add reason to pendingreserves.pl

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Comment 39 Katrin Fischer 2020-08-18 22:48:23 UTC
Doing a first review here:

1) QA script:

 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc
   FAIL	  filters
		missing_filter at line 36 (                    <select name="rank-request" class="rank-request" data-hold-id="[% hold.reserve_id %]">)
		missing_filter at line 38 (                    <input type="hidden" name="rank-request" class="rank-request" value="[% hold.priority | html %]" data-hold-id="[% hold.reserve_id %]">)
		missing_filter at line 39 (                    <select name="rank-request" class="rank-request" disabled="disabled" data-hold-id="[% hold.reserve_id %]">)

2) Unit tests

There are changes on the ModReserveCancelAll and Hold:cancel methods. Can we please get some tests for the new behavior?

3) Docs

Maybe it would be worth mentioning that this allows to store a cancellation reason in the documentation? (Bug title + description for release notes). Currently only the email functionality is mentioned, but we also have an interesting db change.

4) Constants for translation?

+        var REASON_LABEL = _("Reason: ");

I believe this is not really needed here as the Javascript code is in a tt file and not js. Not a blocker, but was wondering.

5) Sample notice

Could we please create a nice sample notice for this? I think it would make using the feature easier on new installations especially since the TT syntax is still a little hard to figure out. As an existing notice also triggers activation, we can't add for existing installations.

6) Authorised value

We should add the authorised value to ease use of the feature. I believe this would be save for existing and new installations? Please also add a nice description to the template on what the category is used for (See: /cgi-bin/koha/admin/authorised_values.pl)

7) GUI

reserve.pl
I think it would be nice if the cancellation reason only activated once a "del" was selected. Otherwise people might expect it to do something without that or expect it to also work when they cancel using "X".

moremember.pl/circulation.pl
I think for context we should add "Cancellation reason:" here before the pull down too.

Why pendingreserves, but not holds queue?

pendingreserves: I'd remove the bold and add : (only one string to translate for translators :) )

8) Possible enhancements

I wonder if it would be nice to move the pull downs to a modal dialog later. So it will always show when you decide to cancel something. It would add an additional click, but libraries can opt-in to use the feature and might like the 'confirmation' step that reminds of setting the reason? 

It also might be nice to haven option to 'not send an email' in some cases. At the moment I think it's always or never? A checkbox for 'send email' might do the job and could be selected/preselected depending on library choice.

Overall no big issues - works well!
Comment 40 Kyle M Hall 2020-08-20 17:10:09 UTC
Created attachment 108746 [details] [review]
Bug 25534: Update database

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Comment 41 Kyle M Hall 2020-08-20 17:10:19 UTC
Created attachment 108747 [details] [review]
Bug 25534: Add ability to send an email specifying a reason when canceling a hold

Some libraries would like to be able to cancel a hold with the option to
specify a reason. Providing a reason would generate an email to that
patron.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Restart all the things!
4) Create new AV category "HOLD_CANCELLATION", add some cancelation reasons
5) Add new Holds module notice "HOLD_CANCELLATION", add an email version.
   A quick test version would be "Reason: <<reserves.cancellation_reason>>"
--
[% USE AuthorisedValues %]
Reason: [% AuthorisedValues.GetByCode( 'CANCELLATION_REASON', hold.cancellation_reason, 'IS_OPAC' ) %]

[% IF hold.cancellation_reason == "MY_AV_VALUE" %]
IF perhaps you'd like to have a much longer explanation than just the
one sentence in the AV description, you can use IF blocks using Template
Toolkit markup!
[% END %]
--
6) Place a hold for a patron
7) On request.pl, select the 'del' option for the hold
8) Select a cancellation reason and choose "Update hold(s)"
9) Note a new message has been queue for the patron with the cancelation reason
11) Test again from circulation.pl
12) Test again from moremember.pl
10) Cancel a hold with no reason, note no email is generated
11) Delete your authorised values, not the feature is disabled
12) Reinstate the authorised values, but delete the notice,
    you should now be able to cancel a hold with a reason,
    but no email will be generated

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Comment 42 Kyle M Hall 2020-08-20 17:10:22 UTC
Created attachment 108748 [details] [review]
Bug 25534: Update Schema Files [DO NOT PUSH]

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Comment 43 Kyle M Hall 2020-08-20 17:10:25 UTC
Created attachment 108749 [details] [review]
Bug 25534: Use the cancelation reasion for the 'X' hold cancelation links

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Comment 44 Kyle M Hall 2020-08-20 17:10:29 UTC
Created attachment 108750 [details] [review]
Bug 25534: Add reason to pendingreserves.pl

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Comment 45 Kyle M Hall 2020-08-20 17:10:32 UTC
Created attachment 108751 [details] [review]
Bug 25534: (QA follow-up) Add missing TT filters
Comment 46 Kyle M Hall 2020-08-20 17:10:35 UTC
Created attachment 108752 [details] [review]
Bug 25534: (QA follow-up) Fix typo
Comment 47 Kyle M Hall 2020-08-20 17:10:38 UTC
Created attachment 108753 [details] [review]
Bug 25534: (QA follow-up) Unit tests
Comment 48 Kyle M Hall 2020-08-20 17:10:41 UTC
Created attachment 108754 [details] [review]
Bug 25534: (QA follow-up) Add sample notice
Comment 49 Kyle M Hall 2020-08-20 17:10:44 UTC
Created attachment 108755 [details] [review]
Bug 25534: (QA follow-up) Add label to reason pulldown
Comment 50 Kyle M Hall 2020-08-20 17:10:48 UTC
Created attachment 108756 [details] [review]
Bug 25534: (QA follow-up) Add AV category
Comment 51 Kyle M Hall 2020-08-20 17:17:55 UTC
Created attachment 108757 [details] [review]
Bug 25534: (QA follow-up) Add colon to label on pendingreserves.pl
Comment 52 Kyle M Hall 2020-08-20 17:18:53 UTC
(In reply to Katrin Fischer from comment #39)
> Doing a first review here:
> 
> 1) QA script:

Fixed!

> 
> 2) Unit tests

Fixed!
 
> 3) Docs
> 
> Maybe it would be worth mentioning that this allows to store a cancellation
> reason in the documentation? (Bug title + description for release notes).
> Currently only the email functionality is mentioned, but we also have an
> interesting db change.

Added! Feel free to change them, I wasn't sure what I should add really.

> 4) Constants for translation?
> 
> +        var REASON_LABEL = _("Reason: ");
> 
> I believe this is not really needed here as the Javascript code is in a tt
> file and not js. Not a blocker, but was wondering.

I see that model being followed on many templates, added for consistency.

> 5) Sample notice

Sample notice added!
 
> 6) Authorised value

I've added the AV category, but no values. I suppose we could add values for new installations, but I don't think we'd want them for existing installs being upgraded. Let me know what you think.

> 
> 7) GUI
> 
> reserve.pl
> I think it would be nice if the cancellation reason only activated once a
> "del" was selected. Otherwise people might expect it to do something without
> that or expect it to also work when they cancel using "X".

It *does* work when canceling using the "X"! For that reason it is not hidden. If you think it's simpler to not allow a reason when using the X and have it hidden unless one of the pulldowns is marked "del" I can make that change too.

> moremember.pl/circulation.pl
> I think for context we should add "Cancellation reason:" here before the
> pull down too.

Done!

> 
> Why pendingreserves, but not holds queue?

Pending reserves has a cancellation function already while the holds queue does not.
 
> pendingreserves: I'd remove the bold and add : (only one string to translate
> for translators :) )

Done!
Comment 53 Katrin Fischer 2020-08-20 21:29:20 UTC
Hi Kyle,

thx for the follow-ups!
> 
> > 5) Sample notice
> 
> Sample notice added!

Can you please also include the translated installers (the .sql files)? We still use them at the moment and not sure when we can get rid of them all yet (for example UNIMARC still needs some work)
 
> > 6) Authorised value
> 
> I've added the AV category, but no values. I suppose we could add values for
> new installations, but I don't think we'd want them for existing installs
> being upgraded. Let me know what you think.

Agree!

> > 7) GUI
> > 
> > reserve.pl
> > I think it would be nice if the cancellation reason only activated once a
> > "del" was selected. Otherwise people might expect it to do something without
> > that or expect it to also work when they cancel using "X".
> 
> It *does* work when canceling using the "X"! For that reason it is not
> hidden. If you think it's simpler to not allow a reason when using the X and
> have it hidden unless one of the pulldowns is marked "del" I can make that
> change too.

I'll test, but then we should leave as is!
Comment 54 Kyle M Hall 2020-08-21 10:58:25 UTC
Created attachment 108805 [details] [review]
Bug 25534: (QA follow-up) Add default values for new AV category
Comment 55 Kyle M Hall 2020-08-21 11:24:36 UTC
Created attachment 108806 [details] [review]
Bug 25534: (QA follow-up) Add sample notices for translators
Comment 56 Katrin Fischer 2020-08-22 09:56:40 UTC
Created attachment 108891 [details] [review]
Bug 25534: Update database

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 57 Katrin Fischer 2020-08-22 09:56:45 UTC
Created attachment 108892 [details] [review]
Bug 25534: Add ability to send an email specifying a reason when canceling a hold

Some libraries would like to be able to cancel a hold with the option to
specify a reason. Providing a reason would generate an email to that
patron.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Restart all the things!
4) Create new AV category "HOLD_CANCELLATION", add some cancelation reasons
5) Add new Holds module notice "HOLD_CANCELLATION", add an email version.
   A quick test version would be "Reason: <<reserves.cancellation_reason>>"
--
[% USE AuthorisedValues %]
Reason: [% AuthorisedValues.GetByCode( 'CANCELLATION_REASON', hold.cancellation_reason, 'IS_OPAC' ) %]

[% IF hold.cancellation_reason == "MY_AV_VALUE" %]
IF perhaps you'd like to have a much longer explanation than just the
one sentence in the AV description, you can use IF blocks using Template
Toolkit markup!
[% END %]
--
6) Place a hold for a patron
7) On request.pl, select the 'del' option for the hold
8) Select a cancellation reason and choose "Update hold(s)"
9) Note a new message has been queue for the patron with the cancelation reason
11) Test again from circulation.pl
12) Test again from moremember.pl
10) Cancel a hold with no reason, note no email is generated
11) Delete your authorised values, not the feature is disabled
12) Reinstate the authorised values, but delete the notice,
    you should now be able to cancel a hold with a reason,
    but no email will be generated

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 58 Katrin Fischer 2020-08-22 09:56:50 UTC
Created attachment 108893 [details] [review]
Bug 25534: Update Schema Files [DO NOT PUSH]

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 59 Katrin Fischer 2020-08-22 09:56:55 UTC
Created attachment 108894 [details] [review]
Bug 25534: Use the cancelation reasion for the 'X' hold cancelation links

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 60 Katrin Fischer 2020-08-22 09:56:59 UTC
Created attachment 108895 [details] [review]
Bug 25534: Add reason to pendingreserves.pl

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 61 Katrin Fischer 2020-08-22 09:57:04 UTC
Created attachment 108896 [details] [review]
Bug 25534: (QA follow-up) Add missing TT filters

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 62 Katrin Fischer 2020-08-22 09:57:09 UTC
Created attachment 108897 [details] [review]
Bug 25534: (QA follow-up) Fix typo

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 63 Katrin Fischer 2020-08-22 09:57:14 UTC
Created attachment 108898 [details] [review]
Bug 25534: (QA follow-up) Unit tests

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 64 Katrin Fischer 2020-08-22 09:57:19 UTC
Created attachment 108899 [details] [review]
Bug 25534: (QA follow-up) Add sample notice

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 65 Katrin Fischer 2020-08-22 09:57:24 UTC
Created attachment 108900 [details] [review]
Bug 25534: (QA follow-up) Add label to reason pulldown

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 66 Katrin Fischer 2020-08-22 09:57:28 UTC
Created attachment 108901 [details] [review]
Bug 25534: (QA follow-up) Add AV category

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 67 Katrin Fischer 2020-08-22 09:57:33 UTC
Created attachment 108902 [details] [review]
Bug 25534: (QA follow-up) Add colon to label on pendingreserves.pl

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 68 Katrin Fischer 2020-08-22 09:57:38 UTC
Created attachment 108903 [details] [review]
Bug 25534: (QA follow-up) Add default values for new AV category

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 69 Katrin Fischer 2020-08-22 09:57:42 UTC
Created attachment 108904 [details] [review]
Bug 25534: (QA follow-up) Add sample notices for translators

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 70 Jonathan Druart 2020-08-24 09:51:32 UTC
1. Why do we have the dropdown list with the reason list at the bottom of this table: https://snipboard.io/i0RrM2.jpg
Would not it make sense to have it in the confirmation popup (moved to a modal)?

2. Should not we have the cancellation reason displayed in the "Holds history" view?
/cgi-bin/koha/members/holdshistory.pl?borrowernumber=51

3. POD for Koha::Hold->cancel needs to be adjusted

4. de-DE/optional/auth_val.sql contains twice the same insert statements

5. Tests must use build_sample_item (see bug 26250)
I will attach a follow-up for 5.
Comment 71 Jonathan Druart 2020-08-24 09:52:01 UTC
Created attachment 108977 [details] [review]
Bug 25534: Use build_sample_item in tests
Comment 72 Katrin Fischer 2020-08-24 10:50:56 UTC
(In reply to Jonathan Druart from comment #70)
> 1. Why do we have the dropdown list with the reason list at the bottom of
> this table: https://snipboard.io/i0RrM2.jpg
> Would not it make sense to have it in the confirmation popup (moved to a
> modal)?

That would make sense to me, but I had not marked it as a blocker (see comment#39)

> 2. Should not we have the cancellation reason displayed in the "Holds
> history" view?
> /cgi-bin/koha/members/holdshistory.pl?borrowernumber=51

I have filed a new bug for this, I think we could easily deal with it separately: 
Bug 26281 - Add cancellation reason to holds history 


> 3. POD for Koha::Hold->cancel needs to be adjusted

> 4. de-DE/optional/auth_val.sql contains twice the same insert statements

Oops, thx for catching that.
 
> 5. Tests must use build_sample_item (see bug 26250)
Comment 73 Kyle M Hall 2020-08-24 13:14:39 UTC
Created attachment 108985 [details] [review]
Bug 25534: (QA follow-up) Update Koha::Hold::cancel POD
Comment 74 Kyle M Hall 2020-08-24 13:14:48 UTC
Created attachment 108986 [details] [review]
Bug 25534: (QA follow-up) Removed doubled insert statements in de-DE/optional/auth_val.sql
Comment 75 Jonathan Druart 2020-08-24 13:25:57 UTC
What about the popup Kyle? I don't think it makes sense to have the dropdown list alone at the bottom at the table. It's not obvious at all that it will be used when the red cross is clicked.
Comment 76 Kyle M Hall 2020-08-24 13:26:55 UTC
(In reply to Jonathan Druart from comment #75)
> What about the popup Kyle? I don't think it makes sense to have the dropdown
> list alone at the bottom at the table. It's not obvious at all that it will
> be used when the red cross is clicked.

I'll take a look and see what I can do!
Comment 77 Kyle M Hall 2020-08-24 14:52:55 UTC
Created attachment 108990 [details] [review]
Bug 25534: (QA follow-up) Use modal for cancel links, hide reason unless priority is set to 'del'
Comment 78 Jonathan Druart 2020-08-25 12:27:06 UTC
We don't add the 2 HOLD_CANCELLATION AVs for existing installations, is that expected?
Comment 79 Jonathan Druart 2020-08-25 12:50:59 UTC
Also wondering, is that ok to send the HOLD_CANCELLATION email to the patron without their agreement? I mean, there is no way for them to stop it if they wish.
Comment 80 Kyle M Hall 2020-08-25 12:58:48 UTC
(In reply to Jonathan Druart from comment #78)
> We don't add the 2 HOLD_CANCELLATION AVs for existing installations, is that
> expected?

That is expected, we don't want to change the behavior of existing installations. If you think we should, I can add that!
Comment 81 Kyle M Hall 2020-08-25 13:00:35 UTC
(In reply to Jonathan Druart from comment #79)
> Also wondering, is that ok to send the HOLD_CANCELLATION email to the patron
> without their agreement? I mean, there is no way for them to stop it if they
> wish.

We send other emails without an agreement. I think like this is one of a very few that shouldn't be opt out, just as you wouldn't be able to opt out of emails from a store that was sending you a cancellation notice for your order.
Comment 82 Jonathan Druart 2020-08-25 13:12:15 UTC
Yup, I am inclined to push as it. We will open follow-up bug reports if needed.
Comment 83 Jonathan Druart 2020-08-25 13:16:18 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 84 Katrin Fischer 2020-08-26 07:33:39 UTC
(In reply to Kyle M Hall from comment #81)
> (In reply to Jonathan Druart from comment #79)
> > Also wondering, is that ok to send the HOLD_CANCELLATION email to the patron
> > without their agreement? I mean, there is no way for them to stop it if they
> > wish.
> 
> We send other emails without an agreement. I think like this is one of a
> very few that shouldn't be opt out, just as you wouldn't be able to opt out
> of emails from a store that was sending you a cancellation notice for your
> order.

I think it would be a nice enhancement actually - sending emails without agreement can be quite problematic in some contexts. This is an optional feature now, but it's one of the reasons I filed bug 26282. We could also think of making it depending on the holds pickup notice being checked, but a separate option might be better long term.
Comment 85 Jonathan Druart 2020-08-26 07:57:42 UTC
(In reply to Katrin Fischer from comment #84)
> (In reply to Kyle M Hall from comment #81)
> > (In reply to Jonathan Druart from comment #79)
> > > Also wondering, is that ok to send the HOLD_CANCELLATION email to the patron
> > > without their agreement? I mean, there is no way for them to stop it if they
> > > wish.
> > 
> > We send other emails without an agreement. I think like this is one of a
> > very few that shouldn't be opt out, just as you wouldn't be able to opt out
> > of emails from a store that was sending you a cancellation notice for your
> > order.
> 
> I think it would be a nice enhancement actually - sending emails without
> agreement can be quite problematic in some contexts. This is an optional
> feature now, but it's one of the reasons I filed bug 26282. We could also
> think of making it depending on the holds pickup notice being checked, but a
> separate option might be better long term.

Without bug 26281 it's not optional. On new install the AVs will get created and clicking the cancel link with a reason will send an email.
Does it mean we must implement 26281 for 20.11?
Comment 86 Kyle M Hall 2020-08-27 11:48:20 UTC
> Without bug 26281 it's not optional. On new install the AVs will get created
> and clicking the cancel link with a reason will send an email.
> Does it mean we must implement 26281 for 20.11?

I see no reason not to!
Comment 87 Jonathan Druart 2020-08-27 12:27:13 UTC
(In reply to Kyle M Hall from comment #86)
> > Without bug 26281 it's not optional. On new install the AVs will get created
> > and clicking the cancel link with a reason will send an email.
> > Does it mean we must implement 26281 for 20.11?
> 
> I see no reason not to!

I actually meant bug 26282. Can you then provide a patch please? :)
Comment 88 Lucas Gass 2020-09-04 20:15:16 UTC
enhancement will not be backported to 20.05.x
Comment 89 Aleisha Amohia 2020-09-17 21:20:22 UTC
*** Bug 20453 has been marked as a duplicate of this bug. ***
Comment 90 Jonathan Druart 2020-11-27 08:26:32 UTC
Created attachment 114026 [details] [review]
Bug 25534: Add missing td in tfoot in pendingreserves
Comment 91 Jonathan Druart 2020-11-27 08:28:59 UTC
Created attachment 114027 [details] [review]
Bug 25534: Add missing td in tfoot in pendingreserves
Comment 92 Martin Renvoize 2020-11-27 10:37:50 UTC
Bug description and text for release notes updated to reflect the removal of the sample notice by bug 27103.

Full functionality will be restored with bug 26281 and the 21.11.01 release.
Comment 93 Jonathan Druart 2020-11-27 12:03:18 UTC
(In reply to Jonathan Druart from comment #91)
> Created attachment 114027 [details] [review] [review]
> Bug 25534: Add missing td in tfoot in pendingreserves

Pushed to master.
Comment 94 Andrew Fuerste-Henry 2020-12-14 15:41:51 UTC
Enhancement, not backported to 20.05
Comment 95 Jonathan Druart 2021-01-04 13:42:29 UTC
Created attachment 114788 [details] [review]
Bug 25534: (QA follow-up 2) Add default values for new AV category

Fix wrong conflict resolution
Comment 96 Jonathan Druart 2021-01-04 13:44:00 UTC
(In reply to Jonathan Druart from comment #95)
> Created attachment 114788 [details] [review] [review]
> Bug 25534: (QA follow-up 2) Add default values for new AV category
> 
> Fix wrong conflict resolution

Moved to bug 27331.