Bug 22833 - Block suspend and cancel on holds
Summary: Block suspend and cancel on holds
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Magnus Enger
QA Contact:
URL:
Keywords:
: 29851 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-05-02 16:51 UTC by Magnus Enger
Modified: 2023-06-25 10:42 UTC (History)
5 users (show)

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


Attachments
Bug 22833 - Schema update, DO NOT PUSH (1.62 KB, patch)
2019-12-11 11:39 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 22833 - Flag booleans in Itemtype.pm (970 bytes, patch)
2019-12-11 11:40 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 22833 - Block suspend and cancel on holds (10.86 KB, patch)
2019-12-11 11:40 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 22833 - Block suspend and cancel on holds (10.90 KB, patch)
2019-12-11 13:32 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 22833 - Block suspend and cancel on holds (10.23 KB, patch)
2020-02-10 13:28 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 22833 - Block suspend and cancel on holds (10.23 KB, patch)
2020-02-10 13:29 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 22833: Schema update, DO NOT PUSH (1.68 KB, patch)
2020-02-10 15:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22833: Flag booleans in Itemtype.pm (1.01 KB, patch)
2020-02-10 15:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22833: Block suspend and cancel on holds (10.25 KB, patch)
2020-02-10 15:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22833 - QA followup (2.31 KB, patch)
2020-02-14 14:24 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 22833: Schema update, DO NOT PUSH (1.73 KB, patch)
2020-02-14 14:43 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 22833: Flag booleans in Itemtype.pm (1.06 KB, patch)
2020-02-14 14:43 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 22833: Block suspend and cancel on holds (10.34 KB, patch)
2020-02-14 14:43 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 22833 - QA followup (2.37 KB, patch)
2020-02-14 14:43 UTC, ByWater Sandboxes
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2019-05-02 16:51:38 UTC
In the Swedish (Libris) ILL module, when a new ILL request is created, a record and an item (with a specific ILL itemtype) are also created. And a hold is created between the local patron that is waiting for the ILL and the local ILL item. 

This means that the ILL shows up as a regular hold when the patron logs into the OPAC. That in it self is OK, but the problematic thing is that patrons can cancel the holds connected with the ILL (maybe thinking that they are then also canceling the ILL request). Libraries would like to stop patrons from doing this. 

A couple of options come to mind:

1. Add a syspref that can take a list of itemtypes that should not be cancel'able by the patron in the OPAC

2. Add a boolean field like itetypes.let_patrons_cancel_holds_in_opac which defaults to 1 for existing itemtypes, but can be set to 0 to achieve the desired effect. 

1 is more unobtrusive, 2 is probably cleaner in the long run?
Comment 1 Magnus Enger 2019-12-10 08:50:48 UTC
Third option:

3. Detect when a hold is related to an ILL request and don't display Suspend/Cancel for these holds.
Comment 2 Magnus Enger 2019-12-11 11:39:55 UTC
Created attachment 96196 [details] [review]
Bug 22833 - Schema update, DO NOT PUSH
Comment 3 Magnus Enger 2019-12-11 11:40:05 UTC
Created attachment 96197 [details] [review]
Bug 22833 - Flag booleans in Itemtype.pm
Comment 4 Magnus Enger 2019-12-11 11:40:09 UTC Comment hidden (obsolete)
Comment 5 Magnus Enger 2019-12-11 13:32:42 UTC Comment hidden (obsolete)
Comment 6 Stina Hallin 2020-01-30 12:22:44 UTC
Hello Magnus, I tried the patch, but when testing, Koha gave an error message when I unchecked the Cancel and Suspend hold check boxes on the Item types administration side (the error message came after saving). It said "Error when saving..." The new settings where not saved.
Comment 7 Magnus Enger 2020-02-10 13:28:03 UTC Comment hidden (obsolete)
Comment 8 Magnus Enger 2020-02-10 13:29:06 UTC Comment hidden (obsolete)
Comment 9 Magnus Enger 2020-02-10 13:30:37 UTC
(In reply to Stina Hallin from comment #6)
> Hello Magnus, I tried the patch, but when testing, Koha gave an error
> message when I unchecked the Cancel and Suspend hold check boxes on the Item
> types administration side (the error message came after saving). It said
> "Error when saving..." The new settings where not saved.

Hm, I can't reproduce that problem. But I did have to rebase/update the patch a bit to work with current master. Would you mind trying again now?
Comment 10 Martin Renvoize 2020-02-10 15:52:21 UTC
This is more of a general enhancement than purely an ILL feature.. as such I've set the status's accordingly to try and expose it to a few more interested parties and get it moving.

Looks like a reasonable implementation to me so far :)
Comment 11 Martin Renvoize 2020-02-10 15:55:40 UTC
Created attachment 98669 [details] [review]
Bug 22833: Schema update, DO NOT PUSH

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2020-02-10 15:55:43 UTC
Created attachment 98670 [details] [review]
Bug 22833: Flag booleans in Itemtype.pm

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2020-02-10 15:55:46 UTC
Created attachment 98671 [details] [review]
Bug 22833: Block suspend and cancel on holds

Swedish ILL uses a hold as part of the ILL workflow. It is problematic if patrons
suspend or cancel those holds before the ILL request is received. This patch
makes it possible to configure, on the itemtype level, if it should be possible
to suspend or cancel holds.

Since a record can have items with different itemtypes, the record level itemtype
is used to determine if holds connected to a record can be suspended and/or
cancelled.

If suspension is blocked for an itemtype, holds connected to records with that
itemtype can not be
- individually suspended by patrons in the OPAC (The "Suspend" button is not
  shown in the table of active holds in the "Holds" tab in "My summary".)
- suspended as part of a "suspend all" operation in the OPAC
- suspended as part of a "suspend all" operation in the staff client (but
  individual holds can still be suspended by staff)

If cancellation is blocked for an itemtype, holds connected to records with that
itemtype can not be
- individually cancelled by patrons in the OPAC (The "Cancel" button is not
  shown in the table of active holds in the "Holds" tab in "My summary".)

To test:
- Apply the patch and make sure the atomic database update is run
- Make sure you have two different itemtypes and records with those itemtypes at
  the record level (field 942$c in MARC 21).
- Toggle the two new settings on the itemtypes, and make sure the behaviour is
  in line with the behaviour described above.

Update 2020-02-10: Minor rebase

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2020-02-10 15:56:34 UTC
Works well for me, Signing off
Comment 15 Stina Hallin 2020-02-11 07:53:29 UTC
Tried the patsch again and got this plack error message when I unchecked the box:

DBD::mysql::st execute failed: Column 'can_cancel_hold' cannot be null [for Statement "UPDATE `itemtypes` SET `can_cancel_hold` = ?, `can_suspend_hold` = ?, `defaultreplacecost` = ?, `processfee` = ? WHERE ( `itemtype` = ? )" with ParamValues: 0=undef, 1=undef, 2=undef, 3=undef, 4='BK'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.

When I tried to uncheck the boxes Koha gave me this message:

An error occurred when updating this item type. Perhaps the value already exists.
Comment 16 Magnus Enger 2020-02-11 08:12:56 UTC
(In reply to Stina Hallin from comment #15)
> Tried the patsch again and got this plack error message when I unchecked the
> box:
> 
> DBD::mysql::st execute failed: Column 'can_cancel_hold' cannot be null [for
> Statement "UPDATE `itemtypes` SET `can_cancel_hold` = ?, `can_suspend_hold`
> = ?, `defaultreplacecost` = ?, `processfee` = ? WHERE ( `itemtype` = ? )"
> with ParamValues: 0=undef, 1=undef, 2=undef, 3=undef, 4='BK'] at
> /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.
> 
> When I tried to uncheck the boxes Koha gave me this message:
> 
> An error occurred when updating this item type. Perhaps the value already
> exists.

Weird. It still works for me. And I double checked that the database is getting updated as it should. 

Some sources of error I can think of:

- Testing with Swedish as the active language in the staff client, without updating the templates (but then you should not see the checkboxes for "Hold can be suspended/cancelled in OPAC", I guess

- The db update did not succeed completely. Could you paste the output of "describe itemtypes;" here?
Comment 17 Stina Hallin 2020-02-12 08:44:44 UTC
Here's the description of itemtypes:
+---------------------+---------------+------+-----+---------+-------+
| Field               | Type          | Null | Key | Default | Extra |
+---------------------+---------------+------+-----+---------+-------+
| itemtype            | varchar(10)   | NO   | PRI |         |       |
| description         | longtext      | YES  |     | NULL    |       |
| rentalcharge        | decimal(28,6) | YES  |     | NULL    |       |
| rentalcharge_daily  | decimal(28,6) | YES  |     | NULL    |       |
| rentalcharge_hourly | decimal(28,6) | YES  |     | NULL    |       |
| defaultreplacecost  | decimal(28,6) | YES  |     | NULL    |       |
| processfee          | decimal(28,6) | YES  |     | NULL    |       |
| notforloan          | smallint(6)   | YES  |     | NULL    |       |
| imageurl            | varchar(200)  | YES  |     | NULL    |       |
| summary             | mediumtext    | YES  |     | NULL    |       |
| checkinmsg          | varchar(255)  | YES  |     | NULL    |       |
| checkinmsgtype      | char(16)      | NO   |     | message |       |
| sip_media_type      | varchar(3)    | YES  |     | NULL    |       |
| hideinopac          | tinyint(1)    | NO   |     | 0       |       |
| searchcategory      | varchar(80)   | YES  |     | NULL    |       |
| can_suspend_hold    | tinyint(1)    | NO   |     | 1       |       |
| can_cancel_hold     | tinyint(1)    | NO   |     | 1       |       |
+---------------------+---------------+------+-----+---------+-------+

We discussed it here, and have a suggestion for solution. 
Replace:
my $can_suspend_hold = $input->param('can_suspend_hold');
my $can_cancel_hold  = $input->param('can_cancel_hold');

with:
my $can_suspend_hold = $input->param('can_suspend_hold') // 0;
my $can_cancel_hold  = $input->param('can_cancel_hold') // 0;

And replace:
[% IF ( HOLD.is_cancelable_from_opac ) %]

with:
[% IF ( HOLD.is_cancelable_from_opac && ItemTypes.CanCancelHold( HOLD.biblio.biblioitem.itemtype )) %]
Comment 18 Magnus Enger 2020-02-14 12:49:33 UTC
(In reply to Stina Hallin from comment #17)
> And replace:
> [% IF ( HOLD.is_cancelable_from_opac ) %]
> 
> with:
> [% IF ( HOLD.is_cancelable_from_opac && ItemTypes.CanCancelHold(
> HOLD.biblio.biblioitem.itemtype )) %]

Hm, I think I'm already doing that in the last part of the diff here? 
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=98671&action=diff

Did I miss some occurrence of this, or did your template not get updated?
Comment 19 Stina Hallin 2020-02-14 13:56:43 UTC
(In reply to Magnus Enger from comment #18)
> (In reply to Stina Hallin from comment #17)
> > And replace:
> > [% IF ( HOLD.is_cancelable_from_opac ) %]
> > 
> > with:
> > [% IF ( HOLD.is_cancelable_from_opac && ItemTypes.CanCancelHold(
> > HOLD.biblio.biblioitem.itemtype )) %]
> 
> Hm, I think I'm already doing that in the last part of the diff here? 
> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=98671&action=diff
> 
> Did I miss some occurrence of this, or did your template not get updated?

That was line 140. We think that you also need to add it to line 186.
Comment 20 Magnus Enger 2020-02-14 14:24:16 UTC
Created attachment 98947 [details] [review]
Bug 22833 - QA followup

This patch incorporates the suggestions made by Stina Hallin.

To test:
- Apply with the other patches and make sure there are no errors
  when following the previous test plan.
Comment 21 Magnus Enger 2020-02-14 14:25:01 UTC
(In reply to Stina Hallin from comment #19)
> That was line 140. We think that you also need to add it to line 186.

Ah, gotach! Could you please test again now, with all the patches?
Comment 22 ByWater Sandboxes 2020-02-14 14:43:10 UTC
Created attachment 98951 [details] [review]
Bug 22833: Schema update, DO NOT PUSH

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se>
Comment 23 ByWater Sandboxes 2020-02-14 14:43:13 UTC
Created attachment 98952 [details] [review]
Bug 22833: Flag booleans in Itemtype.pm

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se>
Comment 24 ByWater Sandboxes 2020-02-14 14:43:15 UTC
Created attachment 98953 [details] [review]
Bug 22833: Block suspend and cancel on holds

Swedish ILL uses a hold as part of the ILL workflow. It is problematic if patrons
suspend or cancel those holds before the ILL request is received. This patch
makes it possible to configure, on the itemtype level, if it should be possible
to suspend or cancel holds.

Since a record can have items with different itemtypes, the record level itemtype
is used to determine if holds connected to a record can be suspended and/or
cancelled.

If suspension is blocked for an itemtype, holds connected to records with that
itemtype can not be
- individually suspended by patrons in the OPAC (The "Suspend" button is not
  shown in the table of active holds in the "Holds" tab in "My summary".)
- suspended as part of a "suspend all" operation in the OPAC
- suspended as part of a "suspend all" operation in the staff client (but
  individual holds can still be suspended by staff)

If cancellation is blocked for an itemtype, holds connected to records with that
itemtype can not be
- individually cancelled by patrons in the OPAC (The "Cancel" button is not
  shown in the table of active holds in the "Holds" tab in "My summary".)

To test:
- Apply the patch and make sure the atomic database update is run
- Make sure you have two different itemtypes and records with those itemtypes at
  the record level (field 942$c in MARC 21).
- Toggle the two new settings on the itemtypes, and make sure the behaviour is
  in line with the behaviour described above.

Update 2020-02-10: Minor rebase

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se>
Comment 25 ByWater Sandboxes 2020-02-14 14:43:18 UTC
Created attachment 98954 [details] [review]
Bug 22833 - QA followup

This patch incorporates the suggestions made by Stina Hallin.

To test:
- Apply with the other patches and make sure there are no errors
  when following the previous test plan.

Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se>
Comment 26 Magnus Enger 2020-02-14 14:47:27 UTC
Thanks, Stina! :-)
Comment 27 Jonathan Druart 2020-02-26 14:45:19 UTC
Hi Magnus,
* I am not sure I understand why you did not use these new flags/conditions for item-level hold?
* I do not understand why you need the 2 subroutines in the TT plugin (CanCancelHold and CanSuspendHold), you should not need that, but call the Koha::ItemType method instead). It seems that you will need a new method in Koha::Biblio to retrieve the Koha::ItemType object.
* Tests no provided for new subroutines anyway.
Comment 28 Jonathan Druart 2020-02-26 14:47:39 UTC
* Does that need to block staff users as well? At first I understood that is was for OPAC only, but C4::Reserves::SuspendAll is blocking it.
Comment 29 Katrin Fischer 2023-06-25 10:42:27 UTC
*** Bug 29851 has been marked as a duplicate of this bug. ***