Bug 18856 - Cancel Waiting Hold in OPAC does not give useful message
Summary: Cancel Waiting Hold in OPAC does not give useful message
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-26 11:45 UTC by Fiona Borthwick
Modified: 2019-10-14 19:56 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 18856 - Cancel Waiting Hold in OPAC does not give useful message (2.34 KB, patch)
2017-09-29 17:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18856 - Cancel Waiting Hold in OPAC does not give useful message (2.46 KB, patch)
2017-10-02 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18856 - Cancel Waiting Hold in OPAC does not give useful message (2.56 KB, patch)
2017-11-10 11:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18856 [QA Followup] - Catch missed instance of is_cancelable (2.16 KB, patch)
2018-01-05 15:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18856: Don't show cancel option for waiting holds in OPAC (2.57 KB, patch)
2018-01-10 07:24 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 18856: (follow-up) Catch missed instance of is_cancelable (2.21 KB, patch)
2018-01-10 07:24 UTC, Katrin Fischer
Details | Diff | Splinter Review
[ALTERNATIVE-PATCH] Bug 18856: Don't show cancel option for waiting holds in OPAC (2.82 KB, patch)
2018-01-10 19:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18856: Don't show cancel option for waiting holds in OPAC (2.86 KB, patch)
2018-01-11 14:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18856 [QA Followup] - Rename method to avoid confusion (4.38 KB, patch)
2018-01-11 14:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18856: Don't show cancel option for waiting holds in OPAC (2.95 KB, patch)
2018-05-17 12:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18856: (QA follow-up) Rename method to avoid confusion (4.37 KB, patch)
2018-05-17 12:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fiona Borthwick 2017-06-26 11:45:46 UTC
If a hold is 'waiting' for the patron to collect then the patron should be prevented from cancelling the hold via their account in the opac.

If a patron tries to cancel the hold, Koha will give an 'are you sure' alert and when you click Yes the page just refreshes and the hold remains.

Staff can cancel the hold from the staff interface but they can then action the waiting hold.  

I think therefore that it is correct behaviour that a patron cannot cancel a hold when it reaches waiting state via the opac but it would be useful to either have a warning to prevent the cancellation or a useful message when they attempt to do so.
Comment 1 Barton Chittenden 2017-08-14 22:24:09 UTC
I think that this is an actual bug -- it's confusing to patrons who think that their hold was actually cancelled, and it doesn't give them any indication that they need to talk to the library who would actually have to cancel the hold.

--Barton
Comment 2 Katrin Fischer 2017-08-15 11:00:27 UTC
I did some digging and it looks like there should be no button when the hold is waiting or in transfer already. (see bug 12876)
Comment 3 Kyle M Hall 2017-09-29 17:45:32 UTC
Created attachment 67480 [details] [review]
Bug 18856 - Cancel Waiting Hold in OPAC does not give useful message

If a hold is 'waiting' for the patron to collect then the patron should be prevented from cancelling the hold via their account in the opac.
If a patron tries to cancel the hold, Koha will give an 'are you sure' alert and when you click Yes the page just refreshes and the hold remains.
Staff can cancel the hold from the staff interface but they can then action the waiting hold.
I think therefore that it is correct behaviour that a patron cannot cancel a hold when it reaches waiting state via the opac but it would be useful to either have a warning to prevent the cancellation or a useful message when they attempt to do so.

The template was using a method that tells Koha if *staff* can cancel a hold,
instead of patron.

Test Plan:
1) Set up a waiting hold
2) Try to cancel it from the opac
3) Note you cannot
4) Apply this patch
5) Reload the page
6) Note the cancel button has disappeared for found holds
Comment 4 Caroline Cyr La Rose 2017-09-29 19:15:21 UTC
Works as described! :)
Comment 5 Kyle M Hall 2017-10-02 12:58:19 UTC
Created attachment 67520 [details] [review]
Bug 18856 - Cancel Waiting Hold in OPAC does not give useful message

If a hold is 'waiting' for the patron to collect then the patron should be prevented from cancelling the hold via their account in the opac.
If a patron tries to cancel the hold, Koha will give an 'are you sure' alert and when you click Yes the page just refreshes and the hold remains.
Staff can cancel the hold from the staff interface but they can then action the waiting hold.
I think therefore that it is correct behaviour that a patron cannot cancel a hold when it reaches waiting state via the opac but it would be useful to either have a warning to prevent the cancellation or a useful message when they attempt to do so.

The template was using a method that tells Koha if *staff* can cancel a hold,
instead of patron.

Test Plan:
1) Set up a waiting hold
2) Try to cancel it from the opac
3) Note you cannot
4) Apply this patch
5) Reload the page
6) Note the cancel button has disappeared for found holds

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Comment 6 Marcel de Rooy 2017-11-10 11:21:52 UTC
Just confirming the above by doublechecking the code:
Cancel button calls opac-modrequest.pl (once called only in opac-user)
opac-modrequest.pl checks with CanReserveBeCanceledFromOpac. This sub returns false for waiting or transit.
Comment 7 Marcel de Rooy 2017-11-10 11:22:46 UTC
Created attachment 69077 [details] [review]
Bug 18856 - Cancel Waiting Hold in OPAC does not give useful message

If a hold is 'waiting' for the patron to collect then the patron should be prevented from cancelling the hold via their account in the opac.
If a patron tries to cancel the hold, Koha will give an 'are you sure' alert and when you click Yes the page just refreshes and the hold remains.
Staff can cancel the hold from the staff interface but they can then action the waiting hold.
I think therefore that it is correct behaviour that a patron cannot cancel a hold when it reaches waiting state via the opac but it would be useful to either have a warning to prevent the cancellation or a useful message when they attempt to do so.

The template was using a method that tells Koha if *staff* can cancel a hold,
instead of patron.

Test Plan:
1) Set up a waiting hold
2) Try to cancel it from the opac
3) Note you cannot
4) Apply this patch
5) Reload the page
6) Note the cancel button has disappeared for found holds

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Jonathan Druart 2017-11-29 17:21:58 UTC
What about the other occurrence of is_cancelable in the same template file?

 695 [% IF ( RESERVE.is_cancelable ) %]
Comment 9 Kyle M Hall 2018-01-05 15:14:52 UTC
Created attachment 70294 [details] [review]
Bug 18856 [QA Followup] - Catch missed instance of is_cancelable
Comment 10 Katrin Fischer 2018-01-10 07:23:47 UTC
Fixed the patch subjects according to guidelines (XXXX: (follow-up) state what was fixed)

Should is_cancelable be removed after this change? It seems the only remaining occurrences are in tests:

Koha/Hold.pm:=head3 is_cancelable
Koha/Hold.pm:sub is_cancelable {
t/db_dependent/Hold.t:# Test method is_cancelable
t/db_dependent/Hold.t:ok( $hold->is_cancelable(), "Unfound hold is cancelable" );
t/db_dependent/Hold.t:ok( $hold->is_cancelable, "Waiting hold is cancelable" );
t/db_dependent/Hold.t:ok( !$hold->is_cancelable, "In transit hold is not cancelable" );


Sorry Marcel, forgot to check the QA contact first!
Comment 11 Katrin Fischer 2018-01-10 07:24:17 UTC
Created attachment 70398 [details] [review]
Bug 18856: Don't show cancel option for waiting holds in OPAC

If a hold is 'waiting' for the patron to collect then the patron should be prevented from cancelling the hold via their account in the opac.
If a patron tries to cancel the hold, Koha will give an 'are you sure' alert and when you click Yes the page just refreshes and the hold remains.
Staff can cancel the hold from the staff interface but they can then action the waiting hold.
I think therefore that it is correct behaviour that a patron cannot cancel a hold when it reaches waiting state via the opac but it would be useful to either have a warning to prevent the cancellation or a useful message when they attempt to do so.

The template was using a method that tells Koha if *staff* can cancel a hold,
instead of patron.

Test Plan:
1) Set up a waiting hold
2) Try to cancel it from the opac
3) Note you cannot
4) Apply this patch
5) Reload the page
6) Note the cancel button has disappeared for found holds

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2018-01-10 07:24:21 UTC
Created attachment 70399 [details] [review]
Bug 18856: (follow-up) Catch missed instance of is_cancelable

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Jonathan Druart 2018-01-10 19:50:55 UTC
Created attachment 70419 [details] [review]
[ALTERNATIVE-PATCH] Bug 18856: Don't show cancel option for waiting holds in OPAC

This is the alternative patch of Kyle's
"""
If a hold is 'waiting' for the patron to collect then the patron should
be prevented from cancelling the hold via their account in the opac.
If a patron tries to cancel the hold, Koha will give an 'are you sure'
alert and when you click Yes the page just refreshes and the hold
remains.
Staff can cancel the hold from the staff interface but they can then
action the waiting hold.
I think therefore that it is correct behaviour that a patron cannot
cancel a hold when it reaches waiting state via the opac but it would be
useful to either have a warning to prevent the cancellation or a useful
message when they attempt to do so.

The template was using a method that tells Koha if *staff* can cancel a
hold,
instead of patron.

Test Plan:
1) Set up a waiting hold
2) Try to cancel it from the opac
3) Note you cannot
4) Apply this patch
5) Reload the page
6) Note the cancel button has disappeared for found holds
"""

It sounds better to keep the ->is_cancelable method, for readability
Comment 14 Jonathan Druart 2018-01-10 19:53:06 UTC
Kyle, Katrin and Marcel, could you take a look at this l
ast patch?
It sounds better to me to keep the method and fix its behaviour instead of testing is_found that can be confusing. Feel free to obsolete it if you disagree and I will pushed Kyle's patches with a follow-up to remove the method and the tests.
Comment 15 Barton Chittenden 2018-01-10 20:29:03 UTC
(In reply to Jonathan Druart from comment #14)
> Kyle, Katrin and Marcel, could you take a look at this l
> ast patch?
> It sounds better to me to keep the method and fix its behaviour instead of
> testing is_found that can be confusing. Feel free to obsolete it if you
> disagree and I will pushed Kyle's patches with a follow-up to remove the
> method and the tests.

The problem with 'is_cancelable' is that staff and opac have different views of whether the item is cancelable or not, whereas is_found is un-ambiguous.

is_cancelable *sounds* like it's clearer because it signals its intent, but under the circumstances, I think 'is_found' is better.

Just my humble opinion; I'm also probably missing context because I haven't taken the time to read the code, so take my words with a large grain of salt.
Comment 16 Jonathan Druart 2018-01-10 20:55:05 UTC
Yes Barton you are right, that's why I added a comment to the POD of the method to tell it only applies for the OPAC.
Maybe rename the method to something more verbose (is_cancelable_from_opac) to remove the ambiguity?
Comment 17 Katrin Fischer 2018-01-10 21:05:36 UTC
is_found is also a bit confusing, because this also includes transit items. found = F in the reserves table is for waiting. So you might not think about the transit items as well immediately. It confused me on testing this.
Comment 18 Kyle M Hall 2018-01-11 14:44:45 UTC
Created attachment 70424 [details] [review]
Bug 18856: Don't show cancel option for waiting holds in OPAC

This is the alternative patch of Kyle's
"""
If a hold is 'waiting' for the patron to collect then the patron should
be prevented from cancelling the hold via their account in the opac.
If a patron tries to cancel the hold, Koha will give an 'are you sure'
alert and when you click Yes the page just refreshes and the hold
remains.
Staff can cancel the hold from the staff interface but they can then
action the waiting hold.
I think therefore that it is correct behaviour that a patron cannot
cancel a hold when it reaches waiting state via the opac but it would be
useful to either have a warning to prevent the cancellation or a useful
message when they attempt to do so.

The template was using a method that tells Koha if *staff* can cancel a
hold,
instead of patron.

Test Plan:
1) Set up a waiting hold
2) Try to cancel it from the opac
3) Note you cannot
4) Apply this patch
5) Reload the page
6) Note the cancel button has disappeared for found holds
"""

It sounds better to keep the ->is_cancelable method, for readability

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Kyle M Hall 2018-01-11 14:44:53 UTC
Created attachment 70425 [details] [review]
Bug 18856 [QA Followup] - Rename method to avoid confusion

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Marcel de Rooy 2018-01-11 16:56:42 UTC
(In reply to Katrin Fischer from comment #17)
> is_found is also a bit confusing, because this also includes transit items.
> found = F in the reserves table is for waiting. So you might not think about
> the transit items as well immediately. It confused me on testing this.

found=W is waiting
found=F is finished (only in deleted reserves)
Comment 21 Marcel de Rooy 2018-01-11 17:00:52 UTC
(In reply to Jonathan Druart from comment #16)
> Yes Barton you are right, that's why I added a comment to the POD of the
> method to tell it only applies for the OPAC.
> Maybe rename the method to something more verbose (is_cancelable_from_opac)
> to remove the ambiguity?

Maybe include a parameter for opac/intranet/api ?
$hold->is_cancelable{( from => 'opac' or opac=>1 }) ?
Comment 22 Katrin Fischer 2018-01-11 17:01:07 UTC
You are absolutely right, Marcel. Tired brain.
Comment 23 Barton Chittenden 2018-01-11 17:04:52 UTC
(In reply to Marcel de Rooy from comment #21)
> (In reply to Jonathan Druart from comment #16)
> > Yes Barton you are right, that's why I added a comment to the POD of the
> > method to tell it only applies for the OPAC.
> > Maybe rename the method to something more verbose (is_cancelable_from_opac)
> > to remove the ambiguity?
> 
> Maybe include a parameter for opac/intranet/api ?
> $hold->is_cancelable{( from => 'opac' or opac=>1 }) ?

This strikes the right balance for me.
Comment 24 Jonathan Druart 2018-01-15 15:12:18 UTC
Looking at bug 4319: We also have C4::Reserves::IsItemOnHoldAndFound that does exactly the same thing. So maybe "is_cancelable" is too specific?

What could we do to keep this code simple, understandable and consistent?
Comment 25 Juan Romay Sieira 2018-02-16 10:04:55 UTC
I think de better option is to add a parameter in is_cancelable, what Marcel explains
Comment 26 Hugh Rundle 2018-02-22 22:19:43 UTC
I don't want to derail a quick bugfix given that the current behaviour is clearly misleading, but is there a reason this initial assumption has gone unchallenged?

>If a hold is 'waiting' for the patron to collect then the patron should be
>prevented from cancelling the hold via their account in the opac.

Why? Surely we should be making it easy for patrons to self-manage their loans and holds. As a library patron I would expect to be able to cancel a hold at any time. If the problem is that staff need to be notified somehow about cancelled allocated holds then it would be better to tackle that. Would that be a new bug or have i missed something?
Comment 27 Liz Rea 2018-02-22 22:48:37 UTC
I'm with Hugh.

What should probably happen here, is that if a user cancels the waiting hold from the OPAC, it should be immediately expired, and put on the "expired holds" ("holds waiting over x days) list in the staff interface. Items on this list are necessarily actioned by librarians, i.e. pulled off the hold shelf and returned to circulation or moved on to the next borrower. 

If we need a message for the borrower, it should be something like "This hold is waiting for you at the library, canceling this hold will remove you from the list and immediately make the item available to other library users. Cancel this hold anyway? / Do not cancel this hold"

We could even do a separate tab for these on that waitingreserves page, if that makes the decision easier.
Comment 28 Hugh Rundle 2018-02-22 23:12:50 UTC
I like Liz's suggested way of dealing with it.

On the topic of alerts, there is a related problem in that the holds cancellation alert is quite confusing anyway, because "ok" cancels the hold and "cancel" doesn't. I've listed this as a separate bug since it affects unallocated holds as well: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20278
Comment 29 Katrin Fischer 2018-02-23 08:36:41 UTC
I agree with Hugh and Liz that we might want to rethink the policy of not allowing patrons to cancel waiting holds, but think the fix might need a bit more thought. Would we like to know that the user cancelled it (for statistics)?

My suggestion would be to fix the current behaviour (not allow cancelling but give a good message/not show the button) for the stable releases and do a better fix for the next release that allows to change the behaviour.

Given we have a feature in Koha to 'punish' people who don't pick up their waiting items (ExpireReservesMaxPickUpDelayCharge), libraries might have different ideas about this?
Comment 30 Katrin Fischer 2018-04-16 14:28:38 UTC
We have another library complaining about this :( 
I think at the moment, we just need a quick fix that doesn't appear broken to the patron.
Comment 31 Hugh Rundle 2018-04-16 22:56:08 UTC
+1 on a quick fix, current behaviour is baffling to patrons.
Comment 32 Jonathan Druart 2018-04-19 14:58:53 UTC
What's wrong with the patches, could they be considered ok as a quick fix?
Comment 33 Hugh Rundle 2018-04-19 22:28:43 UTC
If I am understanding correctly what the patch does (removes the cancel button from allocated holds), then yes, it qualifies as a quick fix.
Comment 34 Marcel de Rooy 2018-05-17 12:29:55 UTC
Created attachment 75394 [details] [review]
Bug 18856: Don't show cancel option for waiting holds in OPAC

This is the alternative patch of Kyle's
"""
If a hold is 'waiting' for the patron to collect then the patron should
be prevented from cancelling the hold via their account in the opac.
If a patron tries to cancel the hold, Koha will give an 'are you sure'
alert and when you click Yes the page just refreshes and the hold
remains.
Staff can cancel the hold from the staff interface but they can then
action the waiting hold.
I think therefore that it is correct behaviour that a patron cannot
cancel a hold when it reaches waiting state via the opac but it would be
useful to either have a warning to prevent the cancellation or a useful
message when they attempt to do so.

The template was using a method that tells Koha if *staff* can cancel a
hold,
instead of patron.

Test Plan:
1) Set up a waiting hold
2) Try to cancel it from the opac
3) Note you cannot
4) Apply this patch
5) Reload the page
6) Note the cancel button has disappeared for found holds
"""

It sounds better to keep the ->is_cancelable method, for readability

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Marcel de Rooy 2018-05-17 12:30:01 UTC
Created attachment 75395 [details] [review]
Bug 18856: (QA follow-up) Rename method to avoid confusion

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Hmm. I would have liked the opac parameter even better ;)
Comment 36 Marcel de Rooy 2018-05-17 12:31:48 UTC
This is a quick fix, the other point mentioned in earlier comments should go on a new enhancement.
Why not add it to 18.05 still, Joubu ?
Comment 37 Marcel de Rooy 2018-05-17 12:35:33 UTC
(In reply to Katrin Fischer from comment #17)
> is_found is also a bit confusing, because this also includes transit items.

Yes, confirming it again.
is_found_or_transit ?
New report btw
Comment 38 Marcel de Rooy 2018-05-17 12:35:57 UTC
is_waiting_or_transit ?
Comment 39 Jonathan Druart 2018-05-17 15:16:59 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 40 Fridolin Somers 2018-07-16 09:59:34 UTC
Pushed to 17.11.x for 17.11.08