Bug 40530

Summary: Show hold cancellation reason in patron holds history
Product: Koha Reporter: Andrew Fuerste-Henry <andrew>
Component: Hold requestsAssignee: Kyle M Hall (khall) <kyle>
Status: Pushed to main --- QA Contact: Tomás Cohen Arazi (tcohen) <tomascohen>
Severity: normal    
Priority: P5 - low CC: david, gmcharlt, jonathan.druart, kyle, lisette, lucas, tomascohen
Version: Main   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
This fixes the status message when a hold is cancelled on the patron's holds history page in the staff interface. It displayed "Cancelled(FIXME)", instead of the actual reason. (This is related to bug 35560 - Use the REST API for holds history, added in Koha 25.05.00 and 24.11.05.)
Version(s) released in:
25.11.00
Circulation function:
Bug Depends on: 35560, 40542    
Bug Blocks: 40543    
Attachments: Bug 40530: Show hold cancellation reason in patron holds history
Bug 40530: Show hold cancellation reason in patron holds history
Bug 40530: Show hold cancellation reason in patron holds history
Bug 40530: Show hold cancellation reason in patron holds history
Bug 40530: (follow-up) Remove spaces before and after the reason
Bug 40530: Show hold cancellation reason in patron holds history
Bug 40530: (follow-up) Remove spaces before and after the reason
Bug 40530: Use +strings and simplify JS
Bug 40530: (follow-up) Remove spaces before and after the reason
Bug 40530: Use +strings and simplify JS
Bug 40530: Show hold cancellation reason in patron holds history
Bug 40530: (follow-up) Remove spaces before and after the reason
Bug 40530: Use +strings and simplify JS

Description Andrew Fuerste-Henry 2025-07-28 13:41:52 UTC
Bug 35560 includes a FIXME noting that it does not properly maintain the display of hold cancellation reasons introduced in bug 26281. 

To recreate:
- place two holds for a patron
- cancel one hold without choosing a reason
- cancel the other hold with a reason
- load your patron's hold history
- the hold cancelled without a reason shows status "Cancelled"
- the hold cancelled with a reason shows status "Cancelled(FIXME)"

https://github.com/Koha-Community/Koha/blame/3216152520ac189bf25b0a33842bef8706e52b34/koha-tmpl/intranet-tmpl/prog/en/modules/members/holdshistory.tt#L323
Comment 1 Kyle M Hall (khall) 2025-07-28 14:38:47 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!
Comment 2 Jonathan Druart 2025-07-28 14:54:48 UTC
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
Comment 3 Kyle M Hall (khall) 2025-07-28 16:25:31 UTC
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!
Comment 4 Kyle M Hall (khall) 2025-07-28 16:29:52 UTC
> 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.
Comment 5 Kyle M Hall (khall) 2025-07-28 18:01:08 UTC
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!
Comment 6 David Nind 2025-07-29 00:37:41 UTC
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>
Comment 7 David Nind 2025-07-29 00:37:45 UTC
Created attachment 184762 [details] [review]
Bug 40530: (follow-up) Remove spaces before and after the reason

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2025-07-29 00:51:28 UTC
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".
Comment 9 Tomás Cohen Arazi (tcohen) 2025-07-29 13:10:39 UTC
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
Comment 10 Tomás Cohen Arazi (tcohen) 2025-07-29 14:24:36 UTC
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>
Comment 11 Tomás Cohen Arazi (tcohen) 2025-07-29 14:24:40 UTC
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>
Comment 12 Tomás Cohen Arazi (tcohen) 2025-07-29 14:24:43 UTC
Created attachment 184814 [details] [review]
Bug 40530: Use +strings and simplify JS

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Kyle M Hall (khall) 2025-07-29 15:14:48 UTC
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>
Comment 14 Kyle M Hall (khall) 2025-07-29 15:14:58 UTC
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>
Comment 15 Lucas Gass (lukeg) 2025-07-31 18:12:41 UTC
Sorry guys, with Bug 40542 this one doesn't apply cleanly. Can we get a rebase?
Comment 16 Tomás Cohen Arazi (tcohen) 2025-07-31 19:33:24 UTC
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>
Comment 17 Tomás Cohen Arazi (tcohen) 2025-07-31 19:33:28 UTC
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>
Comment 18 Tomás Cohen Arazi (tcohen) 2025-07-31 19:33:31 UTC
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>
Comment 19 Lucas Gass (lukeg) 2025-08-01 19:25:22 UTC
Nice work everyone!

Pushed to main for 25.11