https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25534
Katrin Fischer <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #39 from Katrin Fischer <[email protected]> --- 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! -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
