Description
Andrew Fuerste-Henry
2025-07-28 13:41:52 UTC
Created attachment 184751 [details] [review] Bug 40530: Show hold cancellation reason in patron holds history Bug 35560 includes a FIXME noting that it does not properly maintain the display of hold cancellation reasons introduced in bug 26281. Test Plan: 1) Place a hold and cancel it with a reason 2) Browse to the patron's hold history, note the FIXME shown instead of the reason 3) Apply this patch 4) Reload the patch 5) You should now see the cancellation reason instead of FIXME! Oops, sorry about that, that was definitely a FIXME/TODO for me when I wrote bug 35560. Kyle, I am not convinced by the use of a fetch to retrieve the AV's description. IMO it's going to delay the display. Also I am wondering if it's not a case for strings_map. We have two occurrences already: Koha/Preservation/Train/Item/Attribute.pm Koha/Acquisition/Bookseller/Issue.pm Created attachment 184753 [details] [review] Bug 40530: Show hold cancellation reason in patron holds history Bug 35560 includes a FIXME noting that it does not properly maintain the display of hold cancellation reasons introduced in bug 26281. Test Plan: 1) Place a hold and cancel it with a reason 2) Browse to the patron's hold history, note the FIXME shown instead of the reason 3) Apply this patch 4) Reload the patch 5) You should now see the cancellation reason instead of FIXME! > Kyle, I am not convinced by the use of a fetch to retrieve the AV's
> description. IMO it's going to delay the display.
I don't think we need to worry about that. I tested it with 150 Hold Cancelation reasons ( aka an absurd number ) and it took all of 34 miliseconds to get a response. Doing this request on the front end will keep the page load time from being affected as it would be from doing it in the perl script.
Created attachment 184756 [details] [review] Bug 40530: Show hold cancellation reason in patron holds history Bug 35560 includes a FIXME noting that it does not properly maintain the display of hold cancellation reasons introduced in bug 26281. Test Plan: 1) Place a hold and cancel it with a reason 2) Browse to the patron's hold history, note the FIXME shown instead of the reason 3) Apply this patch 4) Reload the patch 5) You should now see the cancellation reason instead of FIXME! Created attachment 184761 [details] [review] Bug 40530: Show hold cancellation reason in patron holds history Bug 35560 includes a FIXME noting that it does not properly maintain the display of hold cancellation reasons introduced in bug 26281. Test Plan: 1) Place a hold and cancel it with a reason 2) Browse to the patron's hold history, note the FIXME shown instead of the reason 3) Apply this patch 4) Reload the patch 5) You should now see the cancellation reason instead of FIXME! Signed-off-by: David Nind <david@davidnind.com> Created attachment 184762 [details] [review] Bug 40530: (follow-up) Remove spaces before and after the reason Signed-off-by: David Nind <david@davidnind.com> I added a follow-up to remove the space in the brackets before and after the cancellation reason: - Before: Cancelled ( Item could not be located on shelves ) - After: Cancelled (Item could not be located on shelves) Notes: 1. The OPAC correctly displayed the cancellation reason before the patch, when OPACHoldsHistory is enabled. 2. If no reason is given, the status message is just "Cancelled". I agree with Jonathan here. I'll provide a tiny follow-up implementing what he proposes. We can ditch it if there's no consensus Created attachment 184812 [details] [review] Bug 40530: Show hold cancellation reason in patron holds history Bug 35560 includes a FIXME noting that it does not properly maintain the display of hold cancellation reasons introduced in bug 26281. Test Plan: 1) Place a hold and cancel it with a reason 2) Browse to the patron's hold history, note the FIXME shown instead of the reason 3) Apply this patch 4) Reload the patch 5) You should now see the cancellation reason instead of FIXME! Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 184813 [details] [review] Bug 40530: (follow-up) Remove spaces before and after the reason Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 184814 [details] [review] Bug 40530: Use +strings and simplify JS Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 184816 [details] [review] Bug 40530: (follow-up) Remove spaces before and after the reason Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 184817 [details] [review] Bug 40530: Use +strings and simplify JS Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Sorry guys, with Bug 40542 this one doesn't apply cleanly. Can we get a rebase? Created attachment 184945 [details] [review] Bug 40530: Show hold cancellation reason in patron holds history Bug 35560 includes a FIXME noting that it does not properly maintain the display of hold cancellation reasons introduced in bug 26281. Test Plan: 1) Place a hold and cancel it with a reason 2) Browse to the patron's hold history, note the FIXME shown instead of the reason 3) Apply this patch 4) Reload the patch 5) You should now see the cancellation reason instead of FIXME! Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 184946 [details] [review] Bug 40530: (follow-up) Remove spaces before and after the reason Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 184947 [details] [review] Bug 40530: Use +strings and simplify JS Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Nice work everyone! Pushed to main for 25.11 |