Bug 22456 - Allow patrons to cancel their waiting holds
Summary: Allow patrons to cancel their waiting holds
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement with 20 votes (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Kyle M Hall
URL:
Keywords: Manual
: 24730 (view as bug list)
Depends on:
Blocks: 30500 31303 31343 31395
  Show dependency treegraph
 
Reported: 2019-03-05 14:12 UTC by Christofer Zorn
Modified: 2022-08-18 17:20 UTC (History)
14 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Large patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement allows patrons to cancel holds waiting for pickup from their account (summary > holds > request cancellation button), instead of having to contact the library (for example, by email, phone, or in person). This is enabled by setting rules by library, patron type, and item type. The rules are set from a new "Default waiting hold cancellation policy" section under Administration > Patrons and circulation > Circulation and fines rules. Cancellation requests are listed in a new tab "Holds with cancellation requests" under Circulation > Holds > Holds awaiting pickup.
Version(s) released in:
22.11.00


Attachments
Bug 22456: Add cancellation request table (4.75 KB, patch)
2022-05-18 20:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add waiting_hold_cancellation circulation rule (9.13 KB, patch)
2022-05-18 20:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: DBIC update (6.65 KB, patch)
2022-05-18 20:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add Koha::Hold::CancellationRequest(s) class(es) (8.40 KB, patch)
2022-05-18 20:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add cancellation request methods to Koha::Hold (10.79 KB, patch)
2022-05-18 20:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Allow requesting cancellation from the OPAC (6.10 KB, patch)
2022-05-18 20:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Staff interface handling WIP (2.89 KB, patch)
2022-05-18 20:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add cancellation request table (4.75 KB, patch)
2022-05-24 17:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add waiting_hold_cancellation circulation rule (9.13 KB, patch)
2022-05-24 17:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: DBIC update (6.86 KB, patch)
2022-05-24 17:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add Koha::Hold::CancellationRequest(s) class(es) (17.70 KB, patch)
2022-05-24 17:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add cancellation request methods to Koha::Hold (12.98 KB, patch)
2022-05-24 17:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Allow requesting cancellation from the OPAC (6.69 KB, patch)
2022-05-24 17:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add Koha::Holds->filter_by_has_pending_cancellation_requests (4.25 KB, patch)
2022-05-24 17:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Staff interface handling (10.67 KB, patch)
2022-05-24 17:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add cancellation request methods to Koha::Hold (13.16 KB, patch)
2022-05-25 16:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Allow requesting cancellation from the OPAC (6.69 KB, patch)
2022-05-25 16:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add Koha::Holds->filter_by_has_pending_cancellation_requests (4.25 KB, patch)
2022-05-25 16:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Staff interface handling (10.67 KB, patch)
2022-05-25 16:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add cancellation request table (2.95 KB, patch)
2022-05-26 19:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add waiting_hold_cancellation circulation rule (9.13 KB, patch)
2022-05-26 19:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: DBIC update (2.38 KB, patch)
2022-05-26 19:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add Koha::Hold::CancellationRequest(s) class(es) (5.94 KB, patch)
2022-05-26 19:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add cancellation request methods to Koha::Hold (10.60 KB, patch)
2022-05-26 19:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add Koha::Holds->filter_by_has_cancellation_requests (3.69 KB, patch)
2022-05-26 19:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Allow requesting cancellation from the OPAC (6.07 KB, patch)
2022-05-26 19:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Staff interface handling (8.08 KB, patch)
2022-05-26 19:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add cancellation request table (3.00 KB, patch)
2022-05-26 20:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 22456: Add waiting_hold_cancellation circulation rule (9.17 KB, patch)
2022-05-26 20:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 22456: DBIC update (2.42 KB, patch)
2022-05-26 20:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 22456: Add Koha::Hold::CancellationRequest(s) class(es) (5.99 KB, patch)
2022-05-26 20:54 UTC, David Nind
Details | Diff | Splinter Review
Bug 22456: Add cancellation request methods to Koha::Hold (10.65 KB, patch)
2022-05-26 20:54 UTC, David Nind
Details | Diff | Splinter Review
Bug 22456: Add Koha::Holds->filter_by_has_cancellation_requests (3.74 KB, patch)
2022-05-26 20:54 UTC, David Nind
Details | Diff | Splinter Review
Bug 22456: Allow requesting cancellation from the OPAC (6.11 KB, patch)
2022-05-26 20:54 UTC, David Nind
Details | Diff | Splinter Review
Bug 22456: Staff interface handling (8.13 KB, patch)
2022-05-26 20:54 UTC, David Nind
Details | Diff | Splinter Review
Bug 22456: (follow-up) Cancel holds at checkin (1.48 KB, patch)
2022-05-27 13:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Allow requesting cancellation from the OPAC (6.13 KB, patch)
2022-06-08 19:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Staff interface handling (8.13 KB, patch)
2022-06-08 19:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: (follow-up) Cancel holds at checkin (1.48 KB, patch)
2022-06-08 19:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add cancellation request table (3.04 KB, patch)
2022-06-08 20:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add waiting_hold_cancellation circulation rule (9.22 KB, patch)
2022-06-08 20:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: DBIC update (2.47 KB, patch)
2022-06-08 20:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add Koha::Hold::CancellationRequest(s) class(es) (6.04 KB, patch)
2022-06-08 20:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add cancellation request methods to Koha::Hold (10.70 KB, patch)
2022-06-08 20:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add Koha::Holds->filter_by_has_cancellation_requests (3.79 KB, patch)
2022-06-08 20:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Allow requesting cancellation from the OPAC (6.18 KB, patch)
2022-06-08 20:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Staff interface handling (8.18 KB, patch)
2022-06-08 20:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: (follow-up) Cancel holds at checkin (1.53 KB, patch)
2022-06-08 20:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add cancellation request table (3.08 KB, patch)
2022-07-11 20:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add waiting_hold_cancellation circulation rule (9.22 KB, patch)
2022-07-11 20:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[DO NOT PUSH] Bug 22456: DBIC update (2.56 KB, patch)
2022-07-11 20:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add Koha::Hold::CancellationRequest(s) class(es) (3.50 KB, patch)
2022-07-11 20:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add cancellation request methods to Koha::Hold (10.70 KB, patch)
2022-07-11 20:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add Koha::Holds->filter_by_has_cancellation_requests (3.79 KB, patch)
2022-07-11 20:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Allow requesting cancellation from the OPAC (6.18 KB, patch)
2022-07-11 20:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Staff interface handling (8.18 KB, patch)
2022-07-11 20:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: (follow-up) Cancel holds at checkin (1.53 KB, patch)
2022-07-11 20:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: (QA follow-up) Consider cancellation requested as cancelled in OPAC (6.89 KB, patch)
2022-07-11 20:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: (QA follow-up) Consider cancellation requested as cancelled in OPAC (6.87 KB, patch)
2022-07-11 20:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22456: Add cancellation request table (3.14 KB, patch)
2022-07-28 17:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22456: Add waiting_hold_cancellation circulation rule (9.29 KB, patch)
2022-07-28 17:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22456: DBIC update (2.61 KB, patch)
2022-07-28 17:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22456: Add Koha::Hold::CancellationRequest(s) class(es) (3.56 KB, patch)
2022-07-28 17:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22456: Add cancellation request methods to Koha::Hold (10.76 KB, patch)
2022-07-28 17:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22456: Add Koha::Holds->filter_by_has_cancellation_requests (3.85 KB, patch)
2022-07-28 17:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22456: Allow requesting cancellation from the OPAC (6.24 KB, patch)
2022-07-28 17:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22456: Staff interface handling (8.39 KB, patch)
2022-07-28 17:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22456: (follow-up) Cancel holds at checkin (1.60 KB, patch)
2022-07-28 17:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22456: (QA follow-up) Consider cancellation requested as cancelled in OPAC (6.94 KB, patch)
2022-07-28 17:12 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Christofer Zorn 2019-03-05 14:12:30 UTC
Good morning!

I'm not sure if I should have piggybacked this off of the previous ticket, but I'd like to open the discussion on bug 18856 again. We'd like the ability to have patrons cancel their holds once they have been pulled by staff and are awaiting pick up.

Our previous catalogue (Bibliocommons) allowed the cancellation of holds once it as captured. This feature was well like and used by our patrons who had determined they didn't need/want the item any longer. This gave the perception that they were freeing up the item for the next person. We have fielded a number of grumpy comments since our migration about the inability to perform this functionality.

From my perspective, if we're allowing patrons to place holds and cancel them before they're captured, we should offer the ability to cancel them up until they're picked up. At the very least, this should be a system option to allow the Library to choose whether they'd like to enable this functionality.

Even if the implemented functionality is a placebo for the user ... offer them the ability to cancel the hold and have it remove from their account. As long as optically it appears they've cancelled the hold, there is no harm. If desired, a report could be run to allow staff to pull those items from the shelf if needed.

I'm more than happy to elaborate if needed on this, but I feel that this is a feature that is currently missing from Koha that would be a boon to customer service.

Thanks

Christofer
Comment 1 Margaret Hade 2019-03-26 21:31:07 UTC
Hi Chris,

You should know that this enhancement runs counter to this other recently resolved bug: Bug 18856 - "Cancel Waiting Hold in OPAC does not give useful message". The functionality you are asking for was recently removed due to its tendency to leave items stranded. 

So, would how you like this to work? Would this be a new system pref? How would the staff side work once the hold has been canceled by the patron? How would you be alerted to this newly canceled hold?
Comment 2 Katrin Fischer 2019-03-27 06:59:30 UTC
(In reply to Margaret from comment #1)
> Hi Chris,
> 
> You should know that this enhancement runs counter to this other recently
> resolved bug: Bug 18856 - "Cancel Waiting Hold in OPAC does not give useful
> message". The functionality you are asking for was recently removed due to
> its tendency to leave items stranded. 
> 
> So, would how you like this to work? Would this be a new system pref? How
> would the staff side work once the hold has been canceled by the patron? How
> would you be alerted to this newly canceled hold?

+1 I think there are libraries that want the current behaviour and we need to think about the workflow. Maybe a pref and a message on pick-up list?
Comment 3 Christofer Zorn 2019-03-27 12:06:12 UTC
Thank you Katrin ...

I would see this as a system preference. When enabled it would allow patrons to cancel the waiting hold from the Opac. From my perspective, as long as the reserves / old_reserves table was updated holding the date of the cancellation, we could then run a report that would list the items that were cancelled in the last 24 hours or so. Then staff could pull them from the hold shelf. When the customer cancels the hold the opac display would toggle the button text from "Cancel" to "Cancelled". The item would remain in their queue until the staff pulled the book and checked it in, based on the report.

Our Library is new to Koha and what I love is the flexibility (we have done things in this system we could only have dreamed of in our previous one). However, choice is what makes this system great. While I respect that this request runs counter to the most recent change and libraries like the status quo, I feel giving us the option to use the system the way we need to is what Koha is about.

Thank you again!
Comment 4 Chris Slone 2020-02-27 14:27:52 UTC
Our library would also like to add support for this feature being added to Koha as well. We have many patrons who  have expressed the desire for the ability to cancel an awaiting hold.

Chris Slone
Camden County Library System
Comment 5 Katrin Fischer 2020-03-15 22:15:32 UTC
*** Bug 24730 has been marked as a duplicate of this bug. ***
Comment 6 Christopher Brannon 2020-03-17 04:23:43 UTC
This could be resolved in a combination of ways.  One way would be to send an e-mail to specific addresses when the hold is cancelled.  Another would be to have an alert on the staff screen, along with the moderation alerts.

It should give several pieces of information about the item, the patron it was on hold for, when it was placed on hold, when it was to expire, and when it was cancelled.  Most of this information is for tracking the item down, depending on how the library shelved it.

I am for having a way for the patron to cancel the hold.  And I don't think it is optics.  Before, if the hold was cancelled, it was on the hold shelf and just sat there.  That was the optics.  Here, if staff are notified in some way, it moves back into circulation faster.
Comment 7 Barbara Johnson 2021-05-20 14:55:54 UTC
I support this feature as well.  Just last night a Library Board member asked me why she couldn't cancel a hold that was waiting for her on the hold shelf.  It should definitely be a system pref because I'm sure some libraries would not want to enable this behavior.  I like the idea of having a link to the cancelled holds appear on the staff home page along with the other things (pending suggestions, patrons requesting modifications, etc.) staff need to act on because that would make it front and center.
Comment 8 Katrin Fischer 2022-02-26 23:31:11 UTC
I like the idea of having it on the dashboard, but if we put them there, I'd also like to see them on the holds awaiting pickup report somehow, because this is where they probably are still :)

I wonder database wise how we should deal with these. We could just show 'cancelled in last... hours' - but then the library will be closed etc. and it won't work as nicely and you will never see what your coworkers have already taken care of...

Some way to actually process them might be nice. Maybe a checkin could remove them from the list?
Comment 9 Christopher Brannon 2022-02-28 15:51:45 UTC
(In reply to Katrin Fischer from comment #8)
> I like the idea of having it on the dashboard, but if we put them there, I'd
> also like to see them on the holds awaiting pickup report somehow, because
> this is where they probably are still :)
> 
> I wonder database wise how we should deal with these. We could just show
> 'cancelled in last... hours' - but then the library will be closed etc. and
> it won't work as nicely and you will never see what your coworkers have
> already taken care of...
> 
> Some way to actually process them might be nice. Maybe a checkin could
> remove them from the list?

I like both ideas.  Where ever it shows (new page via dashboard alerts/holds awaiting pickup), I think both cancelled and expired holds that do not have a datelastseen less than the cancellationdate and expirationdate (and the eventual patron_expiration_date in 22.05) should show as needing attention.  This would eliminate the need for any new fields.

I don't know if it would slow things down, but it might even be worth adding the logic to other places where that holds shows, with a badge or something, showing that this hold is expired/cancelled, and waiting to move on.  Just depends on how much of a delay it would add to the process of the page, if it is checking this for all holds displayed on the page.
Comment 10 Tomás Cohen Arazi 2022-04-19 02:55:47 UTC
I have started coding this. The first thing I did was making it an entry in the circulation rules page so this can be set per library and per patron category.

More on this tomorrow :-D
Comment 11 Lari Taskula 2022-05-11 23:29:54 UTC
(In reply to Tomás Cohen Arazi from comment #10)
> I have started coding this. The first thing I did was making it an entry in
> the circulation rules page so this can be set per library and per patron
> category.
> 
> More on this tomorrow :-D

Any updates on this?
Comment 12 Tomás Cohen Arazi 2022-05-16 18:57:58 UTC
(In reply to Katrin Fischer from comment #8)
> I like the idea of having it on the dashboard, but if we put them there, I'd
> also like to see them on the holds awaiting pickup report somehow, because
> this is where they probably are still :)
> 
> I wonder database wise how we should deal with these. We could just show
> 'cancelled in last... hours' - but then the library will be closed etc. and
> it won't work as nicely and you will never see what your coworkers have
> already taken care of...

I've added a hold_cancellation_requests table to store all we might need about the cancellation request. Patron's will be allowed per-circ rules to request cancellation and visual indicators will let librarians know if there are waiting holds requiring attention, haven't got there yet. It will be API-driven as much as possible.
Comment 13 Tomás Cohen Arazi 2022-05-18 20:40:15 UTC
Created attachment 135165 [details] [review]
Bug 22456: Add cancellation request table

This patch adds a new table, for storing holds cancellation requests,
and managing them afterwards.

The *hold_id* attribute is not made a FK on the reserves table because
of the old_reserves thing we still do (see bug 25260 for a discussion
about it).
Comment 14 Tomás Cohen Arazi 2022-05-18 20:40:23 UTC
Created attachment 135166 [details] [review]
Bug 22456: Add waiting_hold_cancellation circulation rule

This patch adds handling for the waiting_hold_cancellation circulation
rule. It is set no 'No' by default in the atomic update, if not
previously set.

Handling in the rules editor is added, in its own section.

To test:
1. Apply this patch
2. Run:
   $ updatedatabase
=> SUCCESS: All good
3. Verify that the syspref is set:
   $ koha-mysql kohadev
   > SELECT * FROM circulation_rules WHERE rule_name='waiting_hold_cancellation';
=> SUCCESS: Set to 0
4. Play with the rules editor, changing things back and forth, things
   should work, including library-specific and global/defualt settings.
=> SUCCESS: It works
=> SUCCESS: Texts are idiomatic
5. Sign off :-D
Comment 15 Tomás Cohen Arazi 2022-05-18 20:40:33 UTC
Created attachment 135167 [details] [review]
Bug 22456: DBIC update
Comment 16 Tomás Cohen Arazi 2022-05-18 20:40:41 UTC
Created attachment 135168 [details] [review]
Bug 22456: Add Koha::Hold::CancellationRequest(s) class(es)

This patch adds the Koha::Object-derived classes for cancellation
requests.

Tests are added for the overloaded ->store method.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold/CancellationRequest.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 17 Tomás Cohen Arazi 2022-05-18 20:40:48 UTC
Created attachment 135169 [details] [review]
Bug 22456: Add cancellation request methods to Koha::Hold

This patch adds the following helper methods for dealing with hold
cancellation requests to the Koha::Hold class:

 * cancellation_requests
 * add_cancellation_request
 * cancellation_requestable_from_opac
 * cancellation_requested_by_owner

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 18 Tomás Cohen Arazi 2022-05-18 20:40:56 UTC
Created attachment 135170 [details] [review]
Bug 22456: Allow requesting cancellation from the OPAC
Comment 19 Tomás Cohen Arazi 2022-05-18 20:41:02 UTC
Created attachment 135171 [details] [review]
Bug 22456: Staff interface handling WIP
Comment 20 Tomás Cohen Arazi 2022-05-18 20:45:03 UTC
Ok, I submitted so y'all can see where this is headed to.

What is done:
- Circulation rules page setting the ability to request cancellation of waiting holds per library, category and item type.
- Barebones, internals (db table, classes, tests, etc). All based on circ rules
- OPAC handling for this
- A mockup of how we would handle this in staff (see Holds awaiting pickup)

The idea is that the new tab in 'Holds awaiting pickup' will provide buttons for accepting or rejecting the cancellation request.

Once that's done, we should move into making some actions trigger acceptance/rejection automatically, if required.
Comment 21 Tomás Cohen Arazi 2022-05-23 22:08:35 UTC
I'm moving development to the Gitlab branch linked below:

https://gitlab.com/thekesolutions/Koha/-/commits/bug_22456
Comment 22 Tomás Cohen Arazi 2022-05-24 17:17:37 UTC
Created attachment 135319 [details] [review]
Bug 22456: Add cancellation request table

This patch adds a new table, for storing holds cancellation requests,
and managing them afterwards.

The *hold_id* attribute is not made a FK on the reserves table because
of the old_reserves thing we still do (see bug 25260 for a discussion
about it).
Comment 23 Tomás Cohen Arazi 2022-05-24 17:17:43 UTC
Created attachment 135320 [details] [review]
Bug 22456: Add waiting_hold_cancellation circulation rule

This patch adds handling for the waiting_hold_cancellation circulation
rule. It is set no 'No' by default in the atomic update, if not
previously set.

Handling in the rules editor is added, in its own section.

To test:
1. Apply this patch
2. Run:
   $ updatedatabase
=> SUCCESS: All good
3. Verify that the syspref is set:
   $ koha-mysql kohadev
   > SELECT * FROM circulation_rules WHERE rule_name='waiting_hold_cancellation';
=> SUCCESS: Set to 0
4. Play with the rules editor, changing things back and forth, things
   should work, including library-specific and global/defualt settings.
=> SUCCESS: It works
=> SUCCESS: Texts are idiomatic
5. Sign off :-D
Comment 24 Tomás Cohen Arazi 2022-05-24 17:17:49 UTC
Created attachment 135321 [details] [review]
Bug 22456: DBIC update
Comment 25 Tomás Cohen Arazi 2022-05-24 17:17:55 UTC
Created attachment 135322 [details] [review]
Bug 22456: Add Koha::Hold::CancellationRequest(s) class(es)

This patch adds the Koha::Object-derived classes for cancellation
requests.

Tests are added for the overloaded ->store method.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold/CancellationRequest.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 26 Tomás Cohen Arazi 2022-05-24 17:18:01 UTC
Created attachment 135323 [details] [review]
Bug 22456: Add cancellation request methods to Koha::Hold

This patch adds the following helper methods for dealing with hold
cancellation requests to the Koha::Hold class:

 * cancellation_requests
 * add_cancellation_request
 * cancellation_requestable_from_opac
 * cancellation_requested_by_owner

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 27 Tomás Cohen Arazi 2022-05-24 17:18:07 UTC
Created attachment 135324 [details] [review]
Bug 22456: Allow requesting cancellation from the OPAC
Comment 28 Tomás Cohen Arazi 2022-05-24 17:18:13 UTC
Created attachment 135325 [details] [review]
Bug 22456: Add Koha::Holds->filter_by_has_pending_cancellation_requests

This patch adds that method. Which is covered by new tests.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Holds.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 29 Tomás Cohen Arazi 2022-05-24 17:18:18 UTC
Created attachment 135326 [details] [review]
Bug 22456: Staff interface handling

This patch introduces a new tab to the 'Holds awaiting pickup' screen
for holds that have cancellation requests. This can be accepted (by
cancelling the hold) or rejected.

To test:
1. Set circulation rules for cancellation requests, allowing them for
   certain item type.
2. Have some waiting holds for a patron on an item type that allows
   requesting cancellation.
3. Visit 'Holds awaiting pickup'
=> SUCCESS: No holds have cancellation requests
4. In the OPAC, on the user details, notice some holds have the ability
   to be requested cancellation.
5. Choose to request cancellation
=> SUCCESS: Request is placed, the button is disabled
6. In 'Holds awaiting pickup' you can see them
7. Play with rejecting/accepting
=> SUCCESS: It works!
8. Things are reflected in the OPAC side
Comment 30 David Nind 2022-05-25 00:28:31 UTC
Hi Tomás.

I have tested this, and it all works fine except for these two things:

1. Test fails for prove t/db_dependent/Koha/Hold.t

2. I'm not sure of the workflow in the staff interface or the reason why there is a "Reject cancellation" option

1. Test fails for prove t/db_dependent/Koha/Hold.t
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

prove t/db_dependent/Koha/Hold.t
t/db_dependent/Koha/Hold.t .. 4/8 DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`hold_cancellation_requests`, CONSTRAINT `hc_resolver_ibfk` FOREIGN KEY (`resolver_id`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE CASCADE) at /kohadevbox/koha/Koha/Object.pm line 170
        # Looks like you planned 2 tests but ran 1.

    #   Failed test 'cancellation_requests handling'
    #   at t/db_dependent/Koha/Hold.t line 650.
    # Looks like you failed 1 test of 7.
t/db_dependent/Koha/Hold.t .. 5/8 
#   Failed test 'cancel() tests'
#   at t/db_dependent/Koha/Hold.t line 653.
Broken FK constraint# Looks like your test exited with 255 just after 5.
t/db_dependent/Koha/Hold.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 4/8 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/Hold.t (Wstat: 65280 Tests: 5 Failed: 1)
  Failed test:  5
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 8 tests but ran 5.
Files=1, Tests=5, 12 wallclock secs ( 0.03 usr  0.01 sys +  9.59 cusr  1.79 csys = 11.42 CPU)
Result: FAIL

2. I'm not sure of the workflow in the staff interface or the reason why there is a "Reject cancellation" option
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

If I:

1. Request cancellation in the OPAC (Request cancellation button) it changes to a non-clickable button (Cancellation requested).

2. Reject cancellation in the staff interface (Holds awaiting pickup > Holds with cancellation requests tab, and select "Reject cancellation".

The result is:
- In OPAC the button for the hold changes to "Cancellation rejected" (non-clickable)
- In the staff interface it goes back into the "Holds waiting" tab
- I have to click "Cancel hold", then it is removed from the OPAC and holds awaiting pickup > Holds waiting tab
Comment 31 Tomás Cohen Arazi 2022-05-25 16:06:17 UTC
Created attachment 135349 [details] [review]
Bug 22456: Add cancellation request methods to Koha::Hold

This patch adds the following helper methods for dealing with hold
cancellation requests to the Koha::Hold class:

 * cancellation_requests
 * add_cancellation_request
 * cancellation_requestable_from_opac
 * cancellation_requested_by_owner

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 32 Tomás Cohen Arazi 2022-05-25 16:06:23 UTC
Created attachment 135350 [details] [review]
Bug 22456: Allow requesting cancellation from the OPAC
Comment 33 Tomás Cohen Arazi 2022-05-25 16:06:29 UTC
Created attachment 135351 [details] [review]
Bug 22456: Add Koha::Holds->filter_by_has_pending_cancellation_requests

This patch adds that method. Which is covered by new tests.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Holds.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 34 Tomás Cohen Arazi 2022-05-25 16:06:36 UTC
Created attachment 135352 [details] [review]
Bug 22456: Staff interface handling

This patch introduces a new tab to the 'Holds awaiting pickup' screen
for holds that have cancellation requests. This can be accepted (by
cancelling the hold) or rejected.

To test:
1. Set circulation rules for cancellation requests, allowing them for
   certain item type.
2. Have some waiting holds for a patron on an item type that allows
   requesting cancellation.
3. Visit 'Holds awaiting pickup'
=> SUCCESS: No holds have cancellation requests
4. In the OPAC, on the user details, notice some holds have the ability
   to be requested cancellation.
5. Choose to request cancellation
=> SUCCESS: Request is placed, the button is disabled
6. In 'Holds awaiting pickup' you can see them
7. Play with rejecting/accepting
=> SUCCESS: It works!
8. Things are reflected in the OPAC side
Comment 35 Tomás Cohen Arazi 2022-05-25 16:13:36 UTC
(In reply to David Nind from comment #30)
> Hi Tomás.
> 
> I have tested this, and it all works fine except for these two things:
> 
> 1. Test fails for prove t/db_dependent/Koha/Hold.t

Fixed!

> 2. I'm not sure of the workflow in the staff interface or the reason why
> there is a "Reject cancellation" option
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> If I:
> 
> 1. Request cancellation in the OPAC (Request cancellation button) it changes
> to a non-clickable button (Cancellation requested).
> 
> 2. Reject cancellation in the staff interface (Holds awaiting pickup > Holds
> with cancellation requests tab, and select "Reject cancellation".
> 
> The result is:
> - In OPAC the button for the hold changes to "Cancellation rejected"
> (non-clickable)
> - In the staff interface it goes back into the "Holds waiting" tab
> - I have to click "Cancel hold", then it is removed from the OPAC and holds
> awaiting pickup > Holds waiting tab

Yes, I agree this is a bit weird. I tried to submit this as early as possible to get some feedback. I'm not sure this moderation step is required. The original proposal was to just have a way to let the librarian know the user requested cancellation. I can remove the whole 'cancellation request status' thing. The main reasoning was that it was not clear to me how should cancellation requests count towards max reserves and friends (until they actually get acknowledged). We could just mark them as cancelled, actually. But just mark them as user-cancelled... 
I'm open to ideas.
Comment 36 Tomás Cohen Arazi 2022-05-26 19:22:22 UTC
Created attachment 135396 [details] [review]
Bug 22456: Add cancellation request table

This patch adds a new table, for storing holds cancellation requests,
and managing them afterwards.

The *hold_id* attribute is not made a FK on the reserves table because
of the old_reserves thing we still do (see bug 25260 for a discussion
about it).
Comment 37 Tomás Cohen Arazi 2022-05-26 19:22:32 UTC
Created attachment 135397 [details] [review]
Bug 22456: Add waiting_hold_cancellation circulation rule

This patch adds handling for the waiting_hold_cancellation circulation
rule. It is set no 'No' by default in the atomic update, if not
previously set.

Handling in the rules editor is added, in its own section.

To test:
1. Apply this patch
2. Run:
   $ updatedatabase
=> SUCCESS: All good
3. Verify that the syspref is set:
   $ koha-mysql kohadev
   > SELECT * FROM circulation_rules WHERE rule_name='waiting_hold_cancellation';
=> SUCCESS: Set to 0
4. Play with the rules editor, changing things back and forth, things
   should work, including library-specific and global/defualt settings.
=> SUCCESS: It works
=> SUCCESS: Texts are idiomatic
5. Sign off :-D
Comment 38 Tomás Cohen Arazi 2022-05-26 19:22:39 UTC
Created attachment 135398 [details] [review]
Bug 22456: DBIC update
Comment 39 Tomás Cohen Arazi 2022-05-26 19:22:45 UTC
Created attachment 135399 [details] [review]
Bug 22456: Add Koha::Hold::CancellationRequest(s) class(es)

This patch adds the Koha::Object-derived classes for cancellation
requests.

Tests are added for the overloaded ->store method.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold/CancellationRequest.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 40 Tomás Cohen Arazi 2022-05-26 19:22:52 UTC
Created attachment 135400 [details] [review]
Bug 22456: Add cancellation request methods to Koha::Hold

This patch adds the following helper methods for dealing with hold
cancellation requests to the Koha::Hold class:

 * cancellation_requests
 * add_cancellation_request
 * cancellation_requestable_from_opac
 * cancellation_requested_by_owner

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 41 Tomás Cohen Arazi 2022-05-26 19:22:58 UTC
Created attachment 135401 [details] [review]
Bug 22456: Add Koha::Holds->filter_by_has_cancellation_requests

This patch adds that method. Which is covered by new tests.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Holds.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 42 Tomás Cohen Arazi 2022-05-26 19:23:04 UTC
Created attachment 135402 [details] [review]
Bug 22456: Allow requesting cancellation from the OPAC
Comment 43 Tomás Cohen Arazi 2022-05-26 19:23:11 UTC
Created attachment 135403 [details] [review]
Bug 22456: Staff interface handling

This patch introduces a new tab to the 'Holds awaiting pickup' screen
for holds that have cancellation requests. This can be accepted (by
cancelling the hold) or rejected.

To test:
1. Set circulation rules for cancellation requests, allowing them for
   certain item type.
2. Have some waiting holds for a patron on an item type that allows
   requesting cancellation.
3. Visit 'Holds awaiting pickup'
=> SUCCESS: No holds have cancellation requests
4. In the OPAC, on the user details, notice some holds have the ability
   to be requested cancellation.
5. Choose to request cancellation
=> SUCCESS: Request is placed, the button is disabled
6. In 'Holds awaiting pickup' you can see them
7. Play with rejecting/accepting
=> SUCCESS: It works!
8. Things are reflected in the OPAC side
Comment 44 David Nind 2022-05-26 20:53:39 UTC
Created attachment 135404 [details] [review]
Bug 22456: Add cancellation request table

This patch adds a new table, for storing holds cancellation requests,
and managing them afterwards.

The *hold_id* attribute is not made a FK on the reserves table because
of the old_reserves thing we still do (see bug 25260 for a discussion
about it).

Signed-off-by: David Nind <david@davidnind.com>
Comment 45 David Nind 2022-05-26 20:53:46 UTC
Created attachment 135405 [details] [review]
Bug 22456: Add waiting_hold_cancellation circulation rule

This patch adds handling for the waiting_hold_cancellation circulation
rule. It is set no 'No' by default in the atomic update, if not
previously set.

Handling in the rules editor is added, in its own section.

To test:
1. Apply this patch
2. Run:
   $ updatedatabase
=> SUCCESS: All good
3. Verify that the syspref is set:
   $ koha-mysql kohadev
   > SELECT * FROM circulation_rules WHERE rule_name='waiting_hold_cancellation';
=> SUCCESS: Set to 0
4. Play with the rules editor, changing things back and forth, things
   should work, including library-specific and global/defualt settings.
=> SUCCESS: It works
=> SUCCESS: Texts are idiomatic
5. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 46 David Nind 2022-05-26 20:53:53 UTC
Created attachment 135406 [details] [review]
Bug 22456: DBIC update

Signed-off-by: David Nind <david@davidnind.com>
Comment 47 David Nind 2022-05-26 20:54:00 UTC
Created attachment 135407 [details] [review]
Bug 22456: Add Koha::Hold::CancellationRequest(s) class(es)

This patch adds the Koha::Object-derived classes for cancellation
requests.

Tests are added for the overloaded ->store method.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold/CancellationRequest.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 48 David Nind 2022-05-26 20:54:07 UTC
Created attachment 135408 [details] [review]
Bug 22456: Add cancellation request methods to Koha::Hold

This patch adds the following helper methods for dealing with hold
cancellation requests to the Koha::Hold class:

 * cancellation_requests
 * add_cancellation_request
 * cancellation_requestable_from_opac
 * cancellation_requested_by_owner

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 49 David Nind 2022-05-26 20:54:13 UTC
Created attachment 135409 [details] [review]
Bug 22456: Add Koha::Holds->filter_by_has_cancellation_requests

This patch adds that method. Which is covered by new tests.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Holds.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 50 David Nind 2022-05-26 20:54:20 UTC
Created attachment 135410 [details] [review]
Bug 22456: Allow requesting cancellation from the OPAC

Signed-off-by: David Nind <david@davidnind.com>
Comment 51 David Nind 2022-05-26 20:54:27 UTC
Created attachment 135411 [details] [review]
Bug 22456: Staff interface handling

This patch introduces a new tab to the 'Holds awaiting pickup' screen
for holds that have cancellation requests. This can be accepted (by
cancelling the hold) or rejected.

To test:
1. Set circulation rules for cancellation requests, allowing them for
   certain item type.
2. Have some waiting holds for a patron on an item type that allows
   requesting cancellation.
3. Visit 'Holds awaiting pickup'
=> SUCCESS: No holds have cancellation requests
4. In the OPAC, on the user details, notice some holds have the ability
   to be requested cancellation.
5. Choose to request cancellation
=> SUCCESS: Request is placed, the button is disabled
6. In 'Holds awaiting pickup' you can see them
7. Play with rejecting/accepting
=> SUCCESS: It works!
8. Things are reflected in the OPAC side

Signed-off-by: David Nind <david@davidnind.com>
Comment 52 David Nind 2022-05-26 22:22:14 UTC
I've created some draft documentation for adding to the manual when pushed (anyone should be able to edit this, it is a google doc..):

https://docs.google.com/document/d/1RMRvURF5VJy8YkUBuPSTilaL90JPVP23UaXln7paiWM/edit?usp=sharing
Comment 53 Tomás Cohen Arazi 2022-05-27 13:41:26 UTC
Created attachment 135430 [details] [review]
Bug 22456: (follow-up) Cancel holds at checkin

This patch makes checkin process the cancellation requested holds
actually cancel them before moving forward.

To test:
1. Have a waiting hold with a patron generated cancellation request
2. At circulation, check the item in
=> SUCCESS: The workflow follows as if the cancellation requested hold
was already cancelled
=> SUCCESS: The hold is actually cancelled
=> SUCCESS: Not shown in the OPAC anymore

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 54 Tomás Cohen Arazi 2022-06-08 19:42:59 UTC
Created attachment 135830 [details] [review]
Bug 22456: Allow requesting cancellation from the OPAC

Signed-off-by: David Nind <david@davidnind.com>
Comment 55 Tomás Cohen Arazi 2022-06-08 19:43:08 UTC
Created attachment 135831 [details] [review]
Bug 22456: Staff interface handling

This patch introduces a new tab to the 'Holds awaiting pickup' screen
for holds that have cancellation requests. This can be accepted (by
cancelling the hold) or rejected.

To test:
1. Set circulation rules for cancellation requests, allowing them for
   certain item type.
2. Have some waiting holds for a patron on an item type that allows
   requesting cancellation.
3. Visit 'Holds awaiting pickup'
=> SUCCESS: No holds have cancellation requests
4. In the OPAC, on the user details, notice some holds have the ability
   to be requested cancellation.
5. Choose to request cancellation
=> SUCCESS: Request is placed, the button is disabled
6. In 'Holds awaiting pickup' you can see them
7. Play with rejecting/accepting
=> SUCCESS: It works!
8. Things are reflected in the OPAC side

Signed-off-by: David Nind <david@davidnind.com>
Comment 56 Tomás Cohen Arazi 2022-06-08 19:43:15 UTC
Created attachment 135832 [details] [review]
Bug 22456: (follow-up) Cancel holds at checkin

This patch makes checkin process the cancellation requested holds
actually cancel them before moving forward.

To test:
1. Have a waiting hold with a patron generated cancellation request
2. At circulation, check the item in
=> SUCCESS: The workflow follows as if the cancellation requested hold
was already cancelled
=> SUCCESS: The hold is actually cancelled
=> SUCCESS: Not shown in the OPAC anymore

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 57 Tomás Cohen Arazi 2022-06-08 20:49:54 UTC
Created attachment 135838 [details] [review]
Bug 22456: Add cancellation request table

This patch adds a new table, for storing holds cancellation requests,
and managing them afterwards.

The *hold_id* attribute is not made a FK on the reserves table because
of the old_reserves thing we still do (see bug 25260 for a discussion
about it).

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: David Nind <david@davidnind.com>
Comment 58 Tomás Cohen Arazi 2022-06-08 20:50:04 UTC
Created attachment 135839 [details] [review]
Bug 22456: Add waiting_hold_cancellation circulation rule

This patch adds handling for the waiting_hold_cancellation circulation
rule. It is set no 'No' by default in the atomic update, if not
previously set.

Handling in the rules editor is added, in its own section.

To test:
1. Apply this patch
2. Run:
   $ updatedatabase
=> SUCCESS: All good
3. Verify that the syspref is set:
   $ koha-mysql kohadev
   > SELECT * FROM circulation_rules WHERE rule_name='waiting_hold_cancellation';
=> SUCCESS: Set to 0
4. Play with the rules editor, changing things back and forth, things
   should work, including library-specific and global/defualt settings.
=> SUCCESS: It works
=> SUCCESS: Texts are idiomatic
5. Sign off :-D

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: David Nind <david@davidnind.com>
Comment 59 Tomás Cohen Arazi 2022-06-08 20:50:11 UTC
Created attachment 135840 [details] [review]
Bug 22456: DBIC update

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: David Nind <david@davidnind.com>
Comment 60 Tomás Cohen Arazi 2022-06-08 20:50:17 UTC
Created attachment 135841 [details] [review]
Bug 22456: Add Koha::Hold::CancellationRequest(s) class(es)

This patch adds the Koha::Object-derived classes for cancellation
requests.

Tests are added for the overloaded ->store method.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold/CancellationRequest.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: David Nind <david@davidnind.com>
Comment 61 Tomás Cohen Arazi 2022-06-08 20:50:24 UTC
Created attachment 135842 [details] [review]
Bug 22456: Add cancellation request methods to Koha::Hold

This patch adds the following helper methods for dealing with hold
cancellation requests to the Koha::Hold class:

 * cancellation_requests
 * add_cancellation_request
 * cancellation_requestable_from_opac
 * cancellation_requested_by_owner

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: David Nind <david@davidnind.com>
Comment 62 Tomás Cohen Arazi 2022-06-08 20:50:30 UTC
Created attachment 135843 [details] [review]
Bug 22456: Add Koha::Holds->filter_by_has_cancellation_requests

This patch adds that method. Which is covered by new tests.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Holds.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Comment 63 Tomás Cohen Arazi 2022-06-08 20:50:37 UTC
Created attachment 135844 [details] [review]
Bug 22456: Allow requesting cancellation from the OPAC

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: David Nind <david@davidnind.com>
Comment 64 Tomás Cohen Arazi 2022-06-08 20:50:43 UTC
Created attachment 135845 [details] [review]
Bug 22456: Staff interface handling

This patch introduces a new tab to the 'Holds awaiting pickup' screen
for holds that have cancellation requests. This can be accepted (by
cancelling the hold) or rejected.

To test:
1. Set circulation rules for cancellation requests, allowing them for
   certain item type.
2. Have some waiting holds for a patron on an item type that allows
   requesting cancellation.
3. Visit 'Holds awaiting pickup'
=> SUCCESS: No holds have cancellation requests
4. In the OPAC, on the user details, notice some holds have the ability
   to be requested cancellation.
5. Choose to request cancellation
=> SUCCESS: Request is placed, the button is disabled
6. In 'Holds awaiting pickup' you can see them
7. Play with rejecting/accepting
=> SUCCESS: It works!
8. Things are reflected in the OPAC side

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: David Nind <david@davidnind.com>
Comment 65 Tomás Cohen Arazi 2022-06-08 20:50:50 UTC
Created attachment 135846 [details] [review]
Bug 22456: (follow-up) Cancel holds at checkin

This patch makes checkin process the cancellation requested holds
actually cancel them before moving forward.

To test:
1. Have a waiting hold with a patron generated cancellation request
2. At circulation, check the item in
=> SUCCESS: The workflow follows as if the cancellation requested hold
was already cancelled
=> SUCCESS: The hold is actually cancelled
=> SUCCESS: Not shown in the OPAC anymore

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 66 Lari Taskula 2022-07-05 14:41:39 UTC
Comment on attachment 135839 [details] [review]
Bug 22456: Add waiting_hold_cancellation circulation rule

Review of attachment 135839 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
@@ +836,4 @@
>      </div>
>      [% END %]
>  
> +    <div id="waiting-hold-cancel-category" class="container">

I'm curious why this configuration was added in its own section if it contains the same three base parameters branchcode, patron category and itemtype as the default circulation rules table? Not that anyone likes worsening the horizontal scrolling but perhaps you had another reason in mind.
Comment 67 Lari Taskula 2022-07-05 14:41:43 UTC
Comment on attachment 135841 [details] [review]
Bug 22456: Add Koha::Hold::CancellationRequest(s) class(es)

Review of attachment 135841 [details] [review]:
-----------------------------------------------------------------

::: Koha/Hold/CancellationRequest.pm
@@ +40,5 @@
> +sub store {
> +    my ($self) = @_;
> +
> +    if ( !$self->in_storage ) { # new request
> +        $self->creation_date( \'NOW()' )

Why do we do store() overloading instead of defining "NOT NULL DEFAULT CURRENT_TIMESTAMP" at table creation?
Comment 68 Lari Taskula 2022-07-05 14:41:48 UTC
Comment on attachment 135845 [details] [review]
Bug 22456: Staff interface handling

Review of attachment 135845 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/waitingreserves.tt
@@ +50,5 @@
>          </h2>
> +    [% IF reject_result_error %]
> +        <div class="error message">
> +        [% IF reject_result_error.invalid_status %]
> +            <p>There was a problem rejecting the cancellation request. Cannot reject an already completed cancellation request.</p>

Looks like comment #35 decided to remove this functionality, and this seems now dead code.

Test plan should also be adjusted (it still mentions rejecting/accepting the request).
Comment 69 Lari Taskula 2022-07-05 14:47:07 UTC
I think it would be user friendly to let patron uncancel the cancellation request in OPAC, just because even with the confirmation dialog box someone may accidentally cancel a wrong hold or start regretting their action later.
Comment 70 Tomás Cohen Arazi 2022-07-05 15:13:03 UTC
(In reply to Lari Taskula from comment #66)
> Comment on attachment 135839 [details] [review] [review]
> Bug 22456: Add waiting_hold_cancellation circulation rule
> 
> Review of attachment 135839 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
> @@ +836,4 @@
> >      </div>
> >      [% END %]
> >  
> > +    <div id="waiting-hold-cancel-category" class="container">
> 
> I'm curious why this configuration was added in its own section if it
> contains the same three base parameters branchcode, patron category and
> itemtype as the default circulation rules table? Not that anyone likes
> worsening the horizontal scrolling but perhaps you had another reason in
> mind.

I hate the big table, and of course hate worsening it. I feel like this deserves its own section, and we could eventually group holds related policy in a single place as well, but out of the scope.

So, this is just personal preference.
Comment 71 Tomás Cohen Arazi 2022-07-05 15:14:32 UTC
(In reply to Lari Taskula from comment #67)
> Comment on attachment 135841 [details] [review] [review]
> Bug 22456: Add Koha::Hold::CancellationRequest(s) class(es)
> 
> Review of attachment 135841 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/Hold/CancellationRequest.pm
> @@ +40,5 @@
> > +sub store {
> > +    my ($self) = @_;
> > +
> > +    if ( !$self->in_storage ) { # new request
> > +        $self->creation_date( \'NOW()' )
> 
> Why do we do store() overloading instead of defining "NOT NULL DEFAULT
> CURRENT_TIMESTAMP" at table creation?

Good catch! This was carried from the first iteration of this dev, in which I added a cancellation request approval workflow, which included an 'updated_on' column which had the trigger set, and as you cannot set it for two columns...

Will clean this in a follow-up patch later today!

Thanks!
Comment 72 Tomás Cohen Arazi 2022-07-05 15:15:17 UTC
Thanks for your review, Lari. Will address those shortly.
Comment 73 Christopher Brannon 2022-07-06 14:20:02 UTC
(In reply to Lari Taskula from comment #69)
> I think it would be user friendly to let patron uncancel the cancellation
> request in OPAC, just because even with the confirmation dialog box someone
> may accidentally cancel a wrong hold or start regretting their action later.

Is that even possible if holds are automatically triggered for the next hold after cancellation? (maybe I'm not completely understanding the process).

Anyway, IMO, I think it is overkill to have a backup for a backup.  The confirmation should be the end-all.  An undo seems like it is out of the scope of this project anyway.
Comment 74 Lari Taskula 2022-07-06 21:16:51 UTC
(In reply to Christopher Brannon from comment #73)
> Is that even possible if holds are automatically triggered for the next hold
> after cancellation? (maybe I'm not completely understanding the process).

While the hold is in pending cancellation request status (not yet processed), it should be possible. The cancellation request entry would simply be deleted from the newly introduced "hold_cancellation_requests" table and the hold would continue to exist normally.

> An undo seems like it is out of the scope of this project anyway.

Perhaps. But I don't like a disabled button in OPAC saying "Cancellation requested" and the fact that if a patron wanted to revert the action there is nothing they can do other than giving the library a phone call and cause a lot of annoying manual work for the librarians, or an angry patron if nothing could be done.
Comment 75 Tomás Cohen Arazi 2022-07-06 21:50:27 UTC
(In reply to Lari Taskula from comment #74)
> (In reply to Christopher Brannon from comment #73)
> > Is that even possible if holds are automatically triggered for the next hold
> > after cancellation? (maybe I'm not completely understanding the process).
> 
> While the hold is in pending cancellation request status (not yet
> processed), it should be possible. The cancellation request entry would
> simply be deleted from the newly introduced "hold_cancellation_requests"
> table and the hold would continue to exist normally.
> 
> > An undo seems like it is out of the scope of this project anyway.
> 
> Perhaps. But I don't like a disabled button in OPAC saying "Cancellation
> requested" and the fact that if a patron wanted to revert the action there
> is nothing they can do other than giving the library a phone call and cause
> a lot of annoying manual work for the librarians, or an angry patron if
> nothing could be done.

There's no cancellation acceptance workflow (there was, originally, and I stripped it out). The main intention here is to let the staff know that hold has been cancelled even if waiting. I guess we could just remove it from the OPAC view. As it is really cancelled. Just having this intermediate status to be able to alert staff in different places.
Comment 76 Christopher Brannon 2022-07-06 22:16:20 UTC
I think removing the button is a better option.

Even if an undo was an option, it would have been for a limited time.  Yes, maybe we would have averted a phone call or annoying work, or an angry patron, but we get those anyway, and if they wait long enough, they will miss that window and still call.  I'm not saying I'm 100% against it, but a confirmation is their 2nd chance to fix a mistake.  We don't really offer undo's anywhere else.  Why should this be the exception?

I do like the process Tomás has laid out in the patch, regardless of what he does with the button.
Comment 77 Lari Taskula 2022-07-06 23:20:57 UTC
(In reply to Christopher Brannon from comment #76)
> Even if an undo was an option, it would have been for a limited time.  Yes,
> maybe we would have averted a phone call or annoying work, or an angry
> patron, but we get those anyway, and if they wait long enough, they will
> miss that window and still call.  I'm not saying I'm 100% against it, but a
> confirmation is their 2nd chance to fix a mistake.  We don't really offer
> undo's anywhere else.  Why should this be the exception?

I think anything that reduces manual work from librarians is worth considering.

> I think removing the button is a better option.

I'm fine with having the button stay as it is or get removed, or moving this into a new Bug. As you said it seems out of scope. I appreciate that and it's okay.

My intention was to share a possible scenario and an idea for enhancement but not stop the Bug from moving forward because of it. It's not that huge deal for me. Tomás has done great work with this Bug!
Comment 78 Tomás Cohen Arazi 2022-07-11 20:40:00 UTC
Created attachment 137580 [details] [review]
Bug 22456: Add cancellation request table

This patch adds a new table, for storing holds cancellation requests,
and managing them afterwards.

The *hold_id* attribute is not made a FK on the reserves table because
of the old_reserves thing we still do (see bug 25260 for a discussion
about it).

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: David Nind <david@davidnind.com>
Comment 79 Tomás Cohen Arazi 2022-07-11 20:40:10 UTC
Created attachment 137581 [details] [review]
Bug 22456: Add waiting_hold_cancellation circulation rule

This patch adds handling for the waiting_hold_cancellation circulation
rule. It is set no 'No' by default in the atomic update, if not
previously set.

Handling in the rules editor is added, in its own section.

To test:
1. Apply this patch
2. Run:
   $ updatedatabase
=> SUCCESS: All good
3. Verify that the syspref is set:
   $ koha-mysql kohadev
   > SELECT * FROM circulation_rules WHERE rule_name='waiting_hold_cancellation';
=> SUCCESS: Set to 0
4. Play with the rules editor, changing things back and forth, things
   should work, including library-specific and global/defualt settings.
=> SUCCESS: It works
=> SUCCESS: Texts are idiomatic
5. Sign off :-D

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: David Nind <david@davidnind.com>
Comment 80 Tomás Cohen Arazi 2022-07-11 20:40:17 UTC
Created attachment 137582 [details] [review]
[DO NOT PUSH] Bug 22456: DBIC update

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: David Nind <david@davidnind.com>
Comment 81 Tomás Cohen Arazi 2022-07-11 20:40:24 UTC
Created attachment 137583 [details] [review]
Bug 22456: Add Koha::Hold::CancellationRequest(s) class(es)

This patch adds the Koha::Object-derived classes for cancellation
requests.

Tests are added for the overloaded ->store method.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold/CancellationRequest.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: David Nind <david@davidnind.com>
Comment 82 Tomás Cohen Arazi 2022-07-11 20:40:32 UTC
Created attachment 137584 [details] [review]
Bug 22456: Add cancellation request methods to Koha::Hold

This patch adds the following helper methods for dealing with hold
cancellation requests to the Koha::Hold class:

 * cancellation_requests
 * add_cancellation_request
 * cancellation_requestable_from_opac
 * cancellation_requested_by_owner

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: David Nind <david@davidnind.com>
Comment 83 Tomás Cohen Arazi 2022-07-11 20:40:41 UTC
Created attachment 137585 [details] [review]
Bug 22456: Add Koha::Holds->filter_by_has_cancellation_requests

This patch adds that method. Which is covered by new tests.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Holds.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Comment 84 Tomás Cohen Arazi 2022-07-11 20:40:48 UTC
Created attachment 137586 [details] [review]
Bug 22456: Allow requesting cancellation from the OPAC

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: David Nind <david@davidnind.com>
Comment 85 Tomás Cohen Arazi 2022-07-11 20:40:56 UTC
Created attachment 137587 [details] [review]
Bug 22456: Staff interface handling

This patch introduces a new tab to the 'Holds awaiting pickup' screen
for holds that have cancellation requests. This can be accepted (by
cancelling the hold) or rejected.

To test:
1. Set circulation rules for cancellation requests, allowing them for
   certain item type.
2. Have some waiting holds for a patron on an item type that allows
   requesting cancellation.
3. Visit 'Holds awaiting pickup'
=> SUCCESS: No holds have cancellation requests
4. In the OPAC, on the user details, notice some holds have the ability
   to be requested cancellation.
5. Choose to request cancellation
=> SUCCESS: Request is placed, the button is disabled
6. In 'Holds awaiting pickup' you can see them
7. Play with rejecting/accepting
=> SUCCESS: It works!
8. Things are reflected in the OPAC side

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: David Nind <david@davidnind.com>
Comment 86 Tomás Cohen Arazi 2022-07-11 20:41:04 UTC
Created attachment 137588 [details] [review]
Bug 22456: (follow-up) Cancel holds at checkin

This patch makes checkin process the cancellation requested holds
actually cancel them before moving forward.

To test:
1. Have a waiting hold with a patron generated cancellation request
2. At circulation, check the item in
=> SUCCESS: The workflow follows as if the cancellation requested hold
was already cancelled
=> SUCCESS: The hold is actually cancelled
=> SUCCESS: Not shown in the OPAC anymore

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 87 Tomás Cohen Arazi 2022-07-11 20:41:12 UTC Comment hidden (obsolete)
Comment 88 Tomás Cohen Arazi 2022-07-11 20:43:58 UTC
Hi everyone, I addressed Lari's concerns about leftovers (hopefully I didn't miss things!).

I also decided to consider this holds cancelled. This means:
- They are not displayed anymore in the current holds
- They show as cancelled in the holds history for OPAC users.

Thanks!
Comment 89 Tomás Cohen Arazi 2022-07-11 20:44:49 UTC
Created attachment 137591 [details] [review]
Bug 22456: (QA follow-up) Consider cancellation requested as cancelled in OPAC

This patch adds a helper method for filtering out cancellation
requested-holds from resultsets, and makes use of it in the OPAC to
filter out those in the current holds tab. Holds history now shows
'Waiting (cancellation requested)' on those.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 90 Kyle M Hall 2022-07-28 17:11:39 UTC
Created attachment 138209 [details] [review]
Bug 22456: Add cancellation request table

This patch adds a new table, for storing holds cancellation requests,
and managing them afterwards.

The *hold_id* attribute is not made a FK on the reserves table because
of the old_reserves thing we still do (see bug 25260 for a discussion
about it).

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 91 Kyle M Hall 2022-07-28 17:12:04 UTC
Created attachment 138210 [details] [review]
Bug 22456: Add waiting_hold_cancellation circulation rule

This patch adds handling for the waiting_hold_cancellation circulation
rule. It is set no 'No' by default in the atomic update, if not
previously set.

Handling in the rules editor is added, in its own section.

To test:
1. Apply this patch
2. Run:
   $ updatedatabase
=> SUCCESS: All good
3. Verify that the syspref is set:
   $ koha-mysql kohadev
   > SELECT * FROM circulation_rules WHERE rule_name='waiting_hold_cancellation';
=> SUCCESS: Set to 0
4. Play with the rules editor, changing things back and forth, things
   should work, including library-specific and global/defualt settings.
=> SUCCESS: It works
=> SUCCESS: Texts are idiomatic
5. Sign off :-D

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 92 Kyle M Hall 2022-07-28 17:12:09 UTC
Created attachment 138211 [details] [review]
Bug 22456: DBIC update

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 93 Kyle M Hall 2022-07-28 17:12:15 UTC
Created attachment 138212 [details] [review]
Bug 22456: Add Koha::Hold::CancellationRequest(s) class(es)

This patch adds the Koha::Object-derived classes for cancellation
requests.

Tests are added for the overloaded ->store method.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold/CancellationRequest.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 94 Kyle M Hall 2022-07-28 17:12:20 UTC
Created attachment 138213 [details] [review]
Bug 22456: Add cancellation request methods to Koha::Hold

This patch adds the following helper methods for dealing with hold
cancellation requests to the Koha::Hold class:

 * cancellation_requests
 * add_cancellation_request
 * cancellation_requestable_from_opac
 * cancellation_requested_by_owner

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 95 Kyle M Hall 2022-07-28 17:12:25 UTC
Created attachment 138214 [details] [review]
Bug 22456: Add Koha::Holds->filter_by_has_cancellation_requests

This patch adds that method. Which is covered by new tests.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Holds.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 96 Kyle M Hall 2022-07-28 17:12:30 UTC
Created attachment 138215 [details] [review]
Bug 22456: Allow requesting cancellation from the OPAC

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 97 Kyle M Hall 2022-07-28 17:12:36 UTC
Created attachment 138216 [details] [review]
Bug 22456: Staff interface handling

This patch introduces a new tab to the 'Holds awaiting pickup' screen
for holds that have cancellation requests. This can be accepted (by
cancelling the hold) or rejected.

To test:
1. Set circulation rules for cancellation requests, allowing them for
   certain item type.
2. Have some waiting holds for a patron on an item type that allows
   requesting cancellation.
3. Visit 'Holds awaiting pickup'
=> SUCCESS: No holds have cancellation requests
4. In the OPAC, on the user details, notice some holds have the ability
   to be requested cancellation.
5. Choose to request cancellation
=> SUCCESS: Request is placed, the button is disabled
6. In 'Holds awaiting pickup' you can see them
7. Play with rejecting/accepting
=> SUCCESS: It works!
8. Things are reflected in the OPAC side

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 98 Kyle M Hall 2022-07-28 17:12:40 UTC
Created attachment 138217 [details] [review]
Bug 22456: (follow-up) Cancel holds at checkin

This patch makes checkin process the cancellation requested holds
actually cancel them before moving forward.

To test:
1. Have a waiting hold with a patron generated cancellation request
2. At circulation, check the item in
=> SUCCESS: The workflow follows as if the cancellation requested hold
was already cancelled
=> SUCCESS: The hold is actually cancelled
=> SUCCESS: Not shown in the OPAC anymore

Sponsored-by: Montgomery County Public Libraries

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 99 Kyle M Hall 2022-07-28 17:12:45 UTC
Created attachment 138218 [details] [review]
Bug 22456: (QA follow-up) Consider cancellation requested as cancelled in OPAC

This patch adds a helper method for filtering out cancellation
requested-holds from resultsets, and makes use of it in the OPAC to
filter out those in the current holds tab. Holds history now shows
'Waiting (cancellation requested)' on those.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 100 Tomás Cohen Arazi 2022-07-29 18:51:10 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 101 Christofer Zorn 2022-07-29 18:56:06 UTC
Thank you everyone for your hard work on this!
Comment 102 Katrin Fischer 2022-07-30 20:31:07 UTC
I am sorry, but I get this error when trying to return an item with a hold on it:

Can't call method "id" on an undefined value at /kohadevbox/koha/circ/returns.pl line 351

  349:     # is there a waiting hold for the item, for which cancellation
  350:     # has been requested?
  351:     my $waiting_holds_to_be_cancelled =
  352:       Koha::Holds->waiting->search( { itemnumber => $item->id } )
  353:       ->filter_by_has_cancellation_requests;
  354:     while ( my $hold = $waiting_holds_to_be_cancelled->next ) {
Comment 103 Katrin Fischer 2022-07-30 20:32:58 UTC
The hold that should be triggered is a "next available", but I have another 2 item level holds on this record as well. I did not try to cancel, so I wonder, why this code would be run?
Comment 104 Tomás Cohen Arazi 2022-08-01 13:31:09 UTC
(In reply to Katrin Fischer from comment #102)
> I am sorry, but I get this error when trying to return an item with a hold
> on it:
> 
> Can't call method "id" on an undefined value at
> /kohadevbox/koha/circ/returns.pl line 351
> 
>   349:     # is there a waiting hold for the item, for which cancellation
>   350:     # has been requested?
>   351:     my $waiting_holds_to_be_cancelled =
>   352:       Koha::Holds->waiting->search( { itemnumber => $item->id } )
>   353:       ->filter_by_has_cancellation_requests;
>   354:     while ( my $hold = $waiting_holds_to_be_cancelled->next ) {

Sorry, I cannot reproduce. I tried with:

- 3 item-level holds, for different patrons, on same item
- 1 biblio-level hold
- Doing a check-in for a not having item-level hold item on the biblio

=> SUCCESS: biblio-level hold trapped, offered to confirm. tried both confirming and discarding.

The fact it breaks on $item->id is pointing me to something else: it means the item hasn't been found using the barcode. So... it must be something else on your branch that wasn't correctly rebased after this one got pushed.
Comment 105 Tomás Cohen Arazi 2022-08-01 13:48:57 UTC
I'm marking this one as resolved now. Any further stuff related to it should be filed on a separate follow-up bug from now on.

Thanks!
Comment 106 David Cook 2022-08-08 04:48:19 UTC
(In reply to Tomás Cohen Arazi from comment #105)
> I'm marking this one as resolved now. Any further stuff related to it should
> be filed on a separate follow-up bug from now on.
> 
> Thanks!

Bumped into bugs caused by bug 22456 while testing bug 15516 so I've raised bug 31303.

Critical severity since it causes the OPAC to crash in koha-testing-docker if the patron has waiting holds due to the "waiting_hold_cancellation" rule missing. Not sure how this passed QA, but I'll investigate and report back on bug 31303.
Comment 107 David Cook 2022-08-08 05:12:13 UTC
(In reply to David Cook from comment #106)
> Critical severity since it causes the OPAC to crash in koha-testing-docker
> if the patron has waiting holds due to the "waiting_hold_cancellation" rule
> missing. Not sure how this passed QA, but I'll investigate and report back
> on bug 31303.

Patch uploaded! I think it's a different issue from Katrin's, but certainly a very important one...