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.
Created attachment 105034 [details] [review] Bug 25534: Update database
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
Created attachment 105036 [details] [review] Bug 25534: Update Schema Files [DO NOT PUSH]
Created attachment 105077 [details] [review] Bug 25534: Update database
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
Created attachment 105079 [details] [review] Bug 25534: Update Schema Files [DO NOT PUSH]
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.
Also need to account for cancelation via pendingreserves.pl
(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.
Created attachment 105105 [details] [review] Bug 25534: Use the cancelation reasion for the 'X' hold cancelation links
Created attachment 105106 [details] [review] Bug 25534: Add reason to pendingreserves.pl
Created attachment 105108 [details] [review] Bug 25534: Update database
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
Created attachment 105110 [details] [review] Bug 25534: Update Schema Files [DO NOT PUSH]
Created attachment 105111 [details] [review] Bug 25534: Use the cancelation reasion for the 'X' hold cancelation links
Created attachment 105112 [details] [review] Bug 25534: Add reason to pendingreserves.pl
Created attachment 105113 [details] [review] Bug 25534: Update database
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
Created attachment 105115 [details] [review] Bug 25534: Update Schema Files [DO NOT PUSH]
Created attachment 105116 [details] [review] Bug 25534: Use the cancelation reasion for the 'X' hold cancelation links
Created attachment 105117 [details] [review] Bug 25534: Add reason to pendingreserves.pl
Created attachment 105118 [details] [review] Bug 25534: Update database Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
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>
Created attachment 105120 [details] [review] Bug 25534: Update Schema Files [DO NOT PUSH] Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
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>
Created attachment 105122 [details] [review] Bug 25534: Add reason to pendingreserves.pl Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
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>
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>
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>
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>
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>
Patches no longer apply cleanly, please rebase!
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>
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>
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>
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>
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>
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>
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!
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>
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>
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>
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>
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>
Created attachment 108751 [details] [review] Bug 25534: (QA follow-up) Add missing TT filters
Created attachment 108752 [details] [review] Bug 25534: (QA follow-up) Fix typo
Created attachment 108753 [details] [review] Bug 25534: (QA follow-up) Unit tests
Created attachment 108754 [details] [review] Bug 25534: (QA follow-up) Add sample notice
Created attachment 108755 [details] [review] Bug 25534: (QA follow-up) Add label to reason pulldown
Created attachment 108756 [details] [review] Bug 25534: (QA follow-up) Add AV category
Created attachment 108757 [details] [review] Bug 25534: (QA follow-up) Add colon to label on pendingreserves.pl
(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!
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!
Created attachment 108805 [details] [review] Bug 25534: (QA follow-up) Add default values for new AV category
Created attachment 108806 [details] [review] Bug 25534: (QA follow-up) Add sample notices for translators
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>
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>
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>
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>
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>
Created attachment 108896 [details] [review] Bug 25534: (QA follow-up) Add missing TT filters Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 108897 [details] [review] Bug 25534: (QA follow-up) Fix typo Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 108898 [details] [review] Bug 25534: (QA follow-up) Unit tests Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 108899 [details] [review] Bug 25534: (QA follow-up) Add sample notice Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
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>
Created attachment 108901 [details] [review] Bug 25534: (QA follow-up) Add AV category Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
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>
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>
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>
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.
Created attachment 108977 [details] [review] Bug 25534: Use build_sample_item in tests
(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)
Created attachment 108985 [details] [review] Bug 25534: (QA follow-up) Update Koha::Hold::cancel POD
Created attachment 108986 [details] [review] Bug 25534: (QA follow-up) Removed doubled insert statements in de-DE/optional/auth_val.sql
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.
(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!
Created attachment 108990 [details] [review] Bug 25534: (QA follow-up) Use modal for cancel links, hide reason unless priority is set to 'del'
We don't add the 2 HOLD_CANCELLATION AVs for existing installations, is that expected?
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.
(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!
(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.
Yup, I am inclined to push as it. We will open follow-up bug reports if needed.
Pushed to master for 20.11, thanks to everybody involved!
(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.
(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?
> 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!
(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? :)
enhancement will not be backported to 20.05.x
*** Bug 20453 has been marked as a duplicate of this bug. ***
Created attachment 114026 [details] [review] Bug 25534: Add missing td in tfoot in pendingreserves
Created attachment 114027 [details] [review] Bug 25534: Add missing td in tfoot in pendingreserves
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.
(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.
Enhancement, not backported to 20.05
Created attachment 114788 [details] [review] Bug 25534: (QA follow-up 2) Add default values for new AV category Fix wrong conflict resolution
(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.
The Template Toolkit notice example given above had the wrong authorised value category value. This will work: [% USE AuthorisedValues %] Reason: [% AuthorisedValues.GetByCode( 'HOLD_CANCELLATION', hold.cancellation_reason, 'IS_OPAC' ) %] [% IF hold.cancellation_reason == "YOUR_CODE" %] 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 %]