Bug 40530 - Show hold cancellation reason in patron holds history
Summary: Show hold cancellation reason in patron holds history
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Kyle M Hall (khall)
QA Contact: Tomás Cohen Arazi (tcohen)
URL:
Keywords:
Depends on: 35560 40542
Blocks: 40543
  Show dependency treegraph
 
Reported: 2025-07-28 13:41 UTC by Andrew Fuerste-Henry
Modified: 2025-08-01 19:25 UTC (History)
7 users (show)

See Also:
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:


Attachments
Bug 40530: Show hold cancellation reason in patron holds history (2.83 KB, patch)
2025-07-28 14:38 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 40530: Show hold cancellation reason in patron holds history (2.84 KB, patch)
2025-07-28 16:25 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 40530: Show hold cancellation reason in patron holds history (2.84 KB, patch)
2025-07-28 18:01 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 40530: Show hold cancellation reason in patron holds history (2.87 KB, patch)
2025-07-29 00:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 40530: (follow-up) Remove spaces before and after the reason (1.30 KB, patch)
2025-07-29 00:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 40530: Show hold cancellation reason in patron holds history (2.93 KB, patch)
2025-07-29 14:24 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40530: (follow-up) Remove spaces before and after the reason (1.36 KB, patch)
2025-07-29 14:24 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40530: Use +strings and simplify JS (2.94 KB, patch)
2025-07-29 14:24 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40530: (follow-up) Remove spaces before and after the reason (1.43 KB, patch)
2025-07-29 15:14 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 40530: Use +strings and simplify JS (3.01 KB, patch)
2025-07-29 15:14 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 40530: Show hold cancellation reason in patron holds history (2.93 KB, patch)
2025-07-31 19:33 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40530: (follow-up) Remove spaces before and after the reason (1.41 KB, patch)
2025-07-31 19:33 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40530: Use +strings and simplify JS (2.99 KB, patch)
2025-07-31 19:33 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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