Bug 25690 - SIP should not allow to check out an item in transfer because of a hold to another patron
Summary: SIP should not allow to check out an item in transfer because of a hold to an...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Joonas Kylmälä
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on:
Blocks: 28136 28139
  Show dependency treegraph
 
Reported: 2020-06-08 04:31 UTC by Minna Kivinen
Modified: 2021-04-13 08:31 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
- Proper warning messages are added in staff interface when trying to initiate transfer to an attached hold. - Checking out someone else's hold that is in transit is prevented
Version(s) released in:
21.05.00,20.11.04


Attachments
Bug 25690: Remove double usage of 'Reserved' return value (14.96 KB, patch)
2021-02-17 16:51 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25690: SIP2: Don't allow checking out attached hold that is being transferred (1.88 KB, patch)
2021-02-17 16:51 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25690: Remove duplicated logic in SIP2 checkouts (4.08 KB, patch)
2021-02-17 16:51 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25690: Make CanBookBeIssued return In Processing state as needing confirmation (11.48 KB, patch)
2021-02-17 16:51 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25690: Remove double usage of 'Reserved' return value (16.20 KB, patch)
2021-02-18 16:28 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25690: SIP2: Don't allow checking out attached hold that is being transferred (2.02 KB, patch)
2021-02-18 16:28 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25690: Remove duplicated logic in SIP2 checkouts (4.35 KB, patch)
2021-02-18 16:29 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25690: Make CanBookBeIssued return In Processing state as needing confirmation (11.99 KB, patch)
2021-02-18 16:29 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25690: Add SIP2 tests for checking out with holds (3.63 KB, patch)
2021-02-18 16:29 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25690: Remove double usage of 'Reserved' return value (16.21 KB, patch)
2021-02-19 12:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25690: SIP2: Don't allow checking out attached hold that is being transferred (2.08 KB, patch)
2021-02-19 12:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25690: Remove duplicated logic in SIP2 checkouts (4.41 KB, patch)
2021-02-19 12:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25690: Make CanBookBeIssued return In Processing state as needing confirmation (12.00 KB, patch)
2021-02-19 12:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25690: Add SIP2 tests for checking out with holds (3.69 KB, patch)
2021-02-19 12:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25690: (QA follow-up) PROCESSED -> PROCESSING for consistency (7.56 KB, patch)
2021-02-19 12:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25690: (follow-up) Correct grammar in warning messages (2.75 KB, patch)
2021-02-26 11:36 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25690: Remove double usage of 'Reserved' return value (16.26 KB, patch)
2021-03-02 13:00 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25690: SIP2: Don't allow checking out attached hold that is being transferred (2.08 KB, patch)
2021-03-02 13:00 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25690: Remove duplicated logic in SIP2 checkouts (4.41 KB, patch)
2021-03-02 13:00 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25690: Make CanBookBeIssued return In Processing state as needing confirmation (12.06 KB, patch)
2021-03-02 13:00 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25690: Add SIP2 tests for checking out with holds (3.70 KB, patch)
2021-03-02 13:00 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25690: (QA follow-up) PROCESSED -> PROCESSING for consistency (7.61 KB, patch)
2021-03-02 13:00 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25690: (follow-up) Correct grammar in warning messages (2.77 KB, patch)
2021-03-02 13:00 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25690: Remove double usage of 'Reserved' return value (16.28 KB, patch)
2021-03-02 14:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25690: SIP2: Don't allow checking out attached hold that is being transferred (2.15 KB, patch)
2021-03-02 14:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25690: Remove duplicated logic in SIP2 checkouts (4.48 KB, patch)
2021-03-02 14:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25690: Make CanBookBeIssued return In Processing state as needing confirmation (12.07 KB, patch)
2021-03-02 14:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25690: Add SIP2 tests for checking out with holds (3.76 KB, patch)
2021-03-02 14:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25690: (QA follow-up) PROCESSED -> PROCESSING for consistency (7.63 KB, patch)
2021-03-02 14:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25690: (follow-up) Correct grammar in warning messages (2.82 KB, patch)
2021-03-02 14:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25690: (follow-up) Supress warning about unitialized string (1.52 KB, patch)
2021-03-04 10:34 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Minna Kivinen 2020-06-08 04:31:37 UTC
When system preference AllowItemsOnHoldCheckout is enabled, SIP allows another patron to check out an item with hold in transfer. After that the item is attached both to hold (reserves-table) and issue (issues table).

The system should allow to check out an item to another patron than the one with hold, when item is attached to hold and is in transfer.

This bug leads to unfortunate situations where patron with hold never gets the item. Borrower can't renew item since there is the hold, but the borrower can check in the item by self service station and check out again despite the hold in item.
Comment 1 Minna Kivinen 2020-06-10 06:17:54 UTC Comment hidden (obsolete)
Comment 2 Minna Kivinen 2020-06-10 06:21:20 UTC
Sorry, there is a typo in description.

The system should NOT allow to check out an item to another patron than the one with hold, when item is attached to hold and is in transfer.
Comment 3 Joonas Kylmälä 2021-02-17 16:51:36 UTC Comment hidden (obsolete)
Comment 4 Joonas Kylmälä 2021-02-17 16:51:40 UTC Comment hidden (obsolete)
Comment 5 Joonas Kylmälä 2021-02-17 16:51:43 UTC Comment hidden (obsolete)
Comment 6 Joonas Kylmälä 2021-02-17 16:51:46 UTC Comment hidden (obsolete)
Comment 7 Joonas Kylmälä 2021-02-17 16:57:00 UTC Comment hidden (obsolete)
Comment 8 Joonas Kylmälä 2021-02-18 16:28:53 UTC
Created attachment 117000 [details] [review]
Bug 25690: Remove double usage of 'Reserved' return value

The patch "Bug 19116: Hold not set to waiting after transfer" added a
new meaning to 'Reserved' return value of C4::Reserves::CheckReserves
function. Let's remove double usage and have separate Transferred
return value so we can differentiate between attached and non-attached
holds. This will come useful in future refactorings.

This patch does no changes to the logic except in the
/cgi-bin/koha/circ/branchtransfers.pl and circulation.pl we now give
similarly to waiting state notice about hold being transferred.

To test:
   1) Apply this patch
   2) Create a new item level hold so that pickup library is different
   than where the item is currently. Then return the item so that hold
   is being attached and transferred.
   3) Go to branchtransfers.pl and try to create a new transfer: it
   should prompt you with message "Item is attached to a hold and
   being transferred for XXX" and provide you with option to cancel
   the hold or to ignore the transfer.
Comment 9 Joonas Kylmälä 2021-02-18 16:28:57 UTC
Created attachment 117001 [details] [review]
Bug 25690: SIP2: Don't allow checking out attached hold that is being transferred

Items that are attached to a hold and being transferred (found = T) is
final decision and you cannot checkout those to other patrons.

To test (if you have applied all patches from bug 25690):
 1) prove t/db_dependent/SIP/Transaction.t => passes

To test (if you don't have all patches):
 1) Create hold and set pickup library to something else where the
    item is at
 2) Return the item and it should confirm the hold and start transfer
 3) Apply this patch
 4) Try to checkout this item now to some other patron via SIP2 and
    notice it doesn't allow it
 5) Revert this patch
 6) Notice you can now checkout somebody else's hold that is being transferred!
Comment 10 Joonas Kylmälä 2021-02-18 16:29:01 UTC
Created attachment 117002 [details] [review]
Bug 25690: Remove duplicated logic in SIP2 checkouts

Because AllowItemsOnHoldCheckoutSIP only affects the checkoutability
of non-attached, i.e. RESERVED holds in SIP2 we can therefore use the
common code from CanBookBeIssued and ignore only the RESERVED confirmation
message case in SIP2 checkout code.

This slightly changes the checkout error message given for "In
processing" holds that someone other than the holdee tries to
checkout. Otherwise there is no logic changes. The message that this
changes is "Item is on hold for another patron." vs. now "Item cannot
be issued: $confirmation". It is easier to create follow-up patch to
properly add INPROCESSING confirmation to CanBookBeIssued and then
show correct message based on the CanBookBeIssued return value.

To test:
 1) Apply all patches from bug 25690 to get latest Transaction.t version
 2) prove t/db_dependent/SIP/Transaction.t => passes
Comment 11 Joonas Kylmälä 2021-02-18 16:29:04 UTC
Created attachment 117003 [details] [review]
Bug 25690: Make CanBookBeIssued return In Processing state as needing confirmation

This prevents checking out to a patron an item with hold to someone
else in the In Processing state via staff interface.

Also the checkout error message via SIP is now a more clearer one: "Item is
on hold for another patron." Before it was "Item cannot be issued:
$confirmation".

Also the branch transfer and batch checkout pages are adapted to this new
confirmation message as well.

To test:
 1) Create bib level hold to an item for patron A
 2) Check-in that item via SIP2, now the hold state should be "In processing"
 3) Apply patch
 4) Try to checkout the item to patron B via staff interface and
    notice we get now confirmation prompt do we really want to do it
    because it is in processing.

 In order to not have to setup SIP2 server, alternatively steps 1) and
 2) can be done so that you check-in the item in staff interface and
 make it Waiting, and then with SQL change it to "In processing":

 UPDATE reserves SET found = "P" WHERE reserve_id = XXX;
 UPDATE reserves SET waitingdate = NULL WHERE reserve_id = XX
 UPDATE reserves SET expirationdate = NULL WHERE reserve_id = XXX;
Comment 12 Joonas Kylmälä 2021-02-18 16:29:08 UTC
Created attachment 117004 [details] [review]
Bug 25690: Add SIP2 tests for checking out with holds

This should cover whether checking out is allowed for all different
hold states:

Attached:
 - Waiting
 - In processing
 - Transfer

Unattached:
 - Reserved

To test:
 1) prove t/db_dependent/SIP/Transaction.t => passes
Comment 13 Joonas Kylmälä 2021-02-18 16:34:06 UTC
The patch series is now ready for Sign-off. The actual bug reported here is fixed in the patch "Bug 25690: SIP2: Don't allow checking out attached hold that is being transferred" but the series cleans up also a lot of the duplicated logic and in addition fixes the same issue as here reported for SIP in staff interface for Transfer and In processing states – now the librarian gets a pop-up saying the items is in transit or in processing when before it didn't, thus preventing accidental checkouts also there. Please see the commit messages and test plans for more info.
Comment 14 Martin Renvoize 2021-02-19 11:49:36 UTC
Comment on attachment 117000 [details] [review]
Bug 25690: Remove double usage of 'Reserved' return value

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

::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/branchtransfers.tt
@@ +31,5 @@
>              [% IF ( waiting ) %]
>                  Item is marked waiting at [% branchname | html %] for [% name | html %] (<a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% borrowernumber | uri %]">[% borrowernumber | html %]</a>).
>              [% END %]
> +            [% IF ( transferred ) %]
> +                Item is attached to a hold and being transferred for [% name | html %] (<a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% borrowernumber | uri %]">[% borrowernumber | html %]</a>).

I wonder if this reads better:

`Item is in transit to [% branchname | HTML %] for [% name | HTML %] (<a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% borrowernumber | uri %]">[% borrowernumber | html %]</a>)`

But.. do we have access to the transit to location here?

::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation_batch_checkouts.tt
@@ +147,5 @@
>                    [% IF checkout_info.RESERVE_WAITING %]
>                        <li><i class="fa fa-li fa-warning"></i>This item is waiting for another patron.</li>
>                    [% END %]
> +                  [% IF checkout_info.TRANSFERRED %]
> +                      <li><i class="fa fa-li fa-warning"></i>This item is on hold and being transferred to another patron.</li>

to => for
Comment 15 Martin Renvoize 2021-02-19 12:09:05 UTC
Created attachment 117062 [details] [review]
Bug 25690: Remove double usage of 'Reserved' return value

The patch "Bug 19116: Hold not set to waiting after transfer" added a
new meaning to 'Reserved' return value of C4::Reserves::CheckReserves
function. Let's remove double usage and have separate Transferred
return value so we can differentiate between attached and non-attached
holds. This will come useful in future refactorings.

This patch does no changes to the logic except in the
/cgi-bin/koha/circ/branchtransfers.pl and circulation.pl we now give
similarly to waiting state notice about hold being transferred.

To test:
   1) Apply this patch
   2) Create a new item level hold so that pickup library is different
   than where the item is currently. Then return the item so that hold
   is being attached and transferred.
   3) Go to branchtransfers.pl and try to create a new transfer: it
   should prompt you with message "Item is attached to a hold and
   being transferred for XXX" and provide you with option to cancel
   the hold or to ignore the transfer.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2021-02-19 12:09:09 UTC
Created attachment 117063 [details] [review]
Bug 25690: SIP2: Don't allow checking out attached hold that is being transferred

Items that are attached to a hold and being transferred (found = T) is
final decision and you cannot checkout those to other patrons.

To test (if you have applied all patches from bug 25690):
 1) prove t/db_dependent/SIP/Transaction.t => passes

To test (if you don't have all patches):
 1) Create hold and set pickup library to something else where the
    item is at
 2) Return the item and it should confirm the hold and start transfer
 3) Apply this patch
 4) Try to checkout this item now to some other patron via SIP2 and
    notice it doesn't allow it
 5) Revert this patch
 6) Notice you can now checkout somebody else's hold that is being transferred!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2021-02-19 12:09:13 UTC
Created attachment 117064 [details] [review]
Bug 25690: Remove duplicated logic in SIP2 checkouts

Because AllowItemsOnHoldCheckoutSIP only affects the checkoutability
of non-attached, i.e. RESERVED holds in SIP2 we can therefore use the
common code from CanBookBeIssued and ignore only the RESERVED confirmation
message case in SIP2 checkout code.

This slightly changes the checkout error message given for "In
processing" holds that someone other than the holdee tries to
checkout. Otherwise there is no logic changes. The message that this
changes is "Item is on hold for another patron." vs. now "Item cannot
be issued: $confirmation". It is easier to create follow-up patch to
properly add INPROCESSING confirmation to CanBookBeIssued and then
show correct message based on the CanBookBeIssued return value.

To test:
 1) Apply all patches from bug 25690 to get latest Transaction.t version
 2) prove t/db_dependent/SIP/Transaction.t => passes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2021-02-19 12:09:17 UTC
Created attachment 117065 [details] [review]
Bug 25690: Make CanBookBeIssued return In Processing state as needing confirmation

This prevents checking out to a patron an item with hold to someone
else in the In Processing state via staff interface.

Also the checkout error message via SIP is now a more clearer one: "Item is
on hold for another patron." Before it was "Item cannot be issued:
$confirmation".

Also the branch transfer and batch checkout pages are adapted to this new
confirmation message as well.

To test:
 1) Create bib level hold to an item for patron A
 2) Check-in that item via SIP2, now the hold state should be "In processing"
 3) Apply patch
 4) Try to checkout the item to patron B via staff interface and
    notice we get now confirmation prompt do we really want to do it
    because it is in processing.

 In order to not have to setup SIP2 server, alternatively steps 1) and
 2) can be done so that you check-in the item in staff interface and
 make it Waiting, and then with SQL change it to "In processing":

 UPDATE reserves SET found = "P" WHERE reserve_id = XXX;
 UPDATE reserves SET waitingdate = NULL WHERE reserve_id = XX
 UPDATE reserves SET expirationdate = NULL WHERE reserve_id = XXX;

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2021-02-19 12:09:21 UTC
Created attachment 117066 [details] [review]
Bug 25690: Add SIP2 tests for checking out with holds

This should cover whether checking out is allowed for all different
hold states:

Attached:
 - Waiting
 - In processing
 - Transfer

Unattached:
 - Reserved

To test:
 1) prove t/db_dependent/SIP/Transaction.t => passes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2021-02-19 12:09:24 UTC
Created attachment 117067 [details] [review]
Bug 25690: (QA follow-up) PROCESSED -> PROCESSING for consistency

PROCESSED gave the apearance that the item processing had been completed
whereas in reading the code it appears to actually signify that the item
is awaiting/in proessing state.

This patch updates the variable to be PROCESSING consistently throughout
the codebase.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2021-02-19 12:10:29 UTC
This all seems to work well in testing.. with the minor points raised above and my followup to correct the PROCESSED -> PROCESSING confusion.. I'm happy to sign off.
Comment 22 Martin Renvoize 2021-02-19 12:11:18 UTC
Above =  comment 14
Comment 23 Joonas Kylmälä 2021-02-26 11:36:15 UTC
Created attachment 117374 [details] [review]
Bug 25690: (follow-up) Correct grammar in warning messages

This makes the meaning of the warnings a bit more clear.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 24 Joonas Kylmälä 2021-02-26 11:40:16 UTC
Attached trivial follow-up to address the grammar issues raised in comment 14. To me it looks like we cannot get the destination branch info to the transfer warning without refactoring the code so that we return a Koha::Hold object instead of selected information in the confirmation message hash. Therefore I didn't address that and IMHO it is outside the scope of this because giving a descriptive message about a transfer happening is already improvement.
Comment 25 Kyle M Hall 2021-03-02 12:39:44 UTC
Comment on attachment 117064 [details] [review]
Bug 25690: Remove duplicated logic in SIP2 checkouts

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

::: C4/SIP/ILS/Transaction/Checkout.pm
@@ +53,4 @@
>      my $patron         = Koha::Patrons->find($self->{patron}->{borrowernumber});
>      my $overridden_duedate; # usually passed as undef to AddIssue
>      $debug and warn "do_checkout borrower: . " . $patron->borrowernumber;
> +    my ($issuingimpossible, $needsconfirmation) = _can_we_issue($patron, $barcode, 0);

Can you explain why this is hard coded to 0 now?
Comment 26 Kyle M Hall 2021-03-02 12:41:17 UTC
Comment on attachment 117374 [details] [review]
Bug 25690: (follow-up) Correct grammar in warning messages

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

::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/branchtransfers.tt
@@ +32,4 @@
>                  Item is marked waiting at [% branchname | html %] for [% name | html %] (<a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% borrowernumber | uri %]">[% borrowernumber | html %]</a>).
>              [% END %]
>              [% IF ( transferred ) %]
> +                Item has an hold and is in transit for [% name | html %] (<a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% borrowernumber | uri %]">[% borrowernumber | html %]</a>).

I would change "Item has an hold and is in transit" to "Items has been trapped to fill a hold and is in transit", or at least "Item is filling a hold and is in transit".
Comment 27 Joonas Kylmälä 2021-03-02 12:52:59 UTC
(In reply to Kyle M Hall from comment #25)
> Comment on attachment 117064 [details] [review] [review]
> Bug 25690: Remove duplicated logic in SIP2 checkouts
> 
> Review of attachment 117064 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/SIP/ILS/Transaction/Checkout.pm
> @@ +53,4 @@
> >      my $patron         = Koha::Patrons->find($self->{patron}->{borrowernumber});
> >      my $overridden_duedate; # usually passed as undef to AddIssue
> >      $debug and warn "do_checkout borrower: . " . $patron->borrowernumber;
> > +    my ($issuingimpossible, $needsconfirmation) = _can_we_issue($patron, $barcode, 0);
> 
> Can you explain why this is hard coded to 0 now?

The logic is moved now to

>  $confirmation eq 'RESERVED' and C4::Context->preference("AllowItemsOnHoldCheckoutSIP")

and 

> $confirmation eq 'RESERVED' and !C4::Context->preference("AllowItemsOnHoldCheckoutSIP")

checks. There was never need to do the skipping in the core module. The 0 can be refactored out but I didn't do so here because the patch series got already so long.
Comment 28 Joonas Kylmälä 2021-03-02 13:00:21 UTC
Created attachment 117507 [details] [review]
Bug 25690: Remove double usage of 'Reserved' return value

The patch "Bug 19116: Hold not set to waiting after transfer" added a
new meaning to 'Reserved' return value of C4::Reserves::CheckReserves
function. Let's remove double usage and have separate Transferred
return value so we can differentiate between attached and non-attached
holds. This will come useful in future refactorings.

This patch does no changes to the logic except in the
/cgi-bin/koha/circ/branchtransfers.pl and circulation.pl we now give
similarly to waiting state notice about hold being transferred.

To test:
   1) Apply this patch
   2) Create a new item level hold so that pickup library is different
   than where the item is currently. Then return the item so that hold
   is being attached and transferred.
   3) Go to branchtransfers.pl and try to create a new transfer: it
   should prompt you with message "Item is attached to a hold and
   being transferred for XXX" and provide you with option to cancel
   the hold or to ignore the transfer.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Joonas Kylmälä 2021-03-02 13:00:25 UTC
Created attachment 117508 [details] [review]
Bug 25690: SIP2: Don't allow checking out attached hold that is being transferred

Items that are attached to a hold and being transferred (found = T) is
final decision and you cannot checkout those to other patrons.

To test (if you have applied all patches from bug 25690):
 1) prove t/db_dependent/SIP/Transaction.t => passes

To test (if you don't have all patches):
 1) Create hold and set pickup library to something else where the
    item is at
 2) Return the item and it should confirm the hold and start transfer
 3) Apply this patch
 4) Try to checkout this item now to some other patron via SIP2 and
    notice it doesn't allow it
 5) Revert this patch
 6) Notice you can now checkout somebody else's hold that is being transferred!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Joonas Kylmälä 2021-03-02 13:00:28 UTC
Created attachment 117509 [details] [review]
Bug 25690: Remove duplicated logic in SIP2 checkouts

Because AllowItemsOnHoldCheckoutSIP only affects the checkoutability
of non-attached, i.e. RESERVED holds in SIP2 we can therefore use the
common code from CanBookBeIssued and ignore only the RESERVED confirmation
message case in SIP2 checkout code.

This slightly changes the checkout error message given for "In
processing" holds that someone other than the holdee tries to
checkout. Otherwise there is no logic changes. The message that this
changes is "Item is on hold for another patron." vs. now "Item cannot
be issued: $confirmation". It is easier to create follow-up patch to
properly add INPROCESSING confirmation to CanBookBeIssued and then
show correct message based on the CanBookBeIssued return value.

To test:
 1) Apply all patches from bug 25690 to get latest Transaction.t version
 2) prove t/db_dependent/SIP/Transaction.t => passes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Joonas Kylmälä 2021-03-02 13:00:32 UTC
Created attachment 117510 [details] [review]
Bug 25690: Make CanBookBeIssued return In Processing state as needing confirmation

This prevents checking out to a patron an item with hold to someone
else in the In Processing state via staff interface.

Also the checkout error message via SIP is now a more clearer one: "Item is
on hold for another patron." Before it was "Item cannot be issued:
$confirmation".

Also the branch transfer and batch checkout pages are adapted to this new
confirmation message as well.

To test:
 1) Create bib level hold to an item for patron A
 2) Check-in that item via SIP2, now the hold state should be "In processing"
 3) Apply patch
 4) Try to checkout the item to patron B via staff interface and
    notice we get now confirmation prompt do we really want to do it
    because it is in processing.

 In order to not have to setup SIP2 server, alternatively steps 1) and
 2) can be done so that you check-in the item in staff interface and
 make it Waiting, and then with SQL change it to "In processing":

 UPDATE reserves SET found = "P" WHERE reserve_id = XXX;
 UPDATE reserves SET waitingdate = NULL WHERE reserve_id = XX
 UPDATE reserves SET expirationdate = NULL WHERE reserve_id = XXX;

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Joonas Kylmälä 2021-03-02 13:00:36 UTC
Created attachment 117511 [details] [review]
Bug 25690: Add SIP2 tests for checking out with holds

This should cover whether checking out is allowed for all different
hold states:

Attached:
 - Waiting
 - In processing
 - Transfer

Unattached:
 - Reserved

To test:
 1) prove t/db_dependent/SIP/Transaction.t => passes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Joonas Kylmälä 2021-03-02 13:00:39 UTC
Created attachment 117512 [details] [review]
Bug 25690: (QA follow-up) PROCESSED -> PROCESSING for consistency

PROCESSED gave the apearance that the item processing had been completed
whereas in reading the code it appears to actually signify that the item
is awaiting/in proessing state.

This patch updates the variable to be PROCESSING consistently throughout
the codebase.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Joonas Kylmälä 2021-03-02 13:00:43 UTC
Created attachment 117513 [details] [review]
Bug 25690: (follow-up) Correct grammar in warning messages

This makes the meaning of the warnings a bit more clear.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 35 Joonas Kylmälä 2021-03-02 13:01:47 UTC
(In reply to Kyle M Hall from comment #26)
> I would change "Item has an hold and is in transit" to "Items has been
> trapped to fill a hold and is in transit", or at least "Item is filling a
> hold and is in transit".

Thanks for the feedback, corrected the message now in the "Bug 25690: (follow-up) Correct grammar in warning messages" patch.
Comment 36 Joonas Kylmälä 2021-03-02 13:03:45 UTC
Kyle, btw, you had set the bug to "Doesn't apply status". For me it applied but I noticed:

> Using index info to reconstruct a base tree...
> M	C4/SIP/ILS/Transaction/Checkout.pm
> Falling back to patching base and 3-way merge...
> Auto-merging C4/SIP/ILS/Transaction/Checkout.pm

in the git messages, so maybe it depends on git settings. I attached all the patches now again, so hopefully it works for you now as well.
Comment 37 Kyle M Hall 2021-03-02 14:34:00 UTC
Created attachment 117519 [details] [review]
Bug 25690: Remove double usage of 'Reserved' return value

The patch "Bug 19116: Hold not set to waiting after transfer" added a
new meaning to 'Reserved' return value of C4::Reserves::CheckReserves
function. Let's remove double usage and have separate Transferred
return value so we can differentiate between attached and non-attached
holds. This will come useful in future refactorings.

This patch does no changes to the logic except in the
/cgi-bin/koha/circ/branchtransfers.pl and circulation.pl we now give
similarly to waiting state notice about hold being transferred.

To test:
   1) Apply this patch
   2) Create a new item level hold so that pickup library is different
   than where the item is currently. Then return the item so that hold
   is being attached and transferred.
   3) Go to branchtransfers.pl and try to create a new transfer: it
   should prompt you with message "Item is attached to a hold and
   being transferred for XXX" and provide you with option to cancel
   the hold or to ignore the transfer.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 38 Kyle M Hall 2021-03-02 14:34:14 UTC
Created attachment 117520 [details] [review]
Bug 25690: SIP2: Don't allow checking out attached hold that is being transferred

Items that are attached to a hold and being transferred (found = T) is
final decision and you cannot checkout those to other patrons.

To test (if you have applied all patches from bug 25690):
 1) prove t/db_dependent/SIP/Transaction.t => passes

To test (if you don't have all patches):
 1) Create hold and set pickup library to something else where the
    item is at
 2) Return the item and it should confirm the hold and start transfer
 3) Apply this patch
 4) Try to checkout this item now to some other patron via SIP2 and
    notice it doesn't allow it
 5) Revert this patch
 6) Notice you can now checkout somebody else's hold that is being transferred!

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 39 Kyle M Hall 2021-03-02 14:34:17 UTC
Created attachment 117521 [details] [review]
Bug 25690: Remove duplicated logic in SIP2 checkouts

Because AllowItemsOnHoldCheckoutSIP only affects the checkoutability
of non-attached, i.e. RESERVED holds in SIP2 we can therefore use the
common code from CanBookBeIssued and ignore only the RESERVED confirmation
message case in SIP2 checkout code.

This slightly changes the checkout error message given for "In
processing" holds that someone other than the holdee tries to
checkout. Otherwise there is no logic changes. The message that this
changes is "Item is on hold for another patron." vs. now "Item cannot
be issued: $confirmation". It is easier to create follow-up patch to
properly add INPROCESSING confirmation to CanBookBeIssued and then
show correct message based on the CanBookBeIssued return value.

To test:
 1) Apply all patches from bug 25690 to get latest Transaction.t version
 2) prove t/db_dependent/SIP/Transaction.t => passes

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 40 Kyle M Hall 2021-03-02 14:34:21 UTC
Created attachment 117522 [details] [review]
Bug 25690: Make CanBookBeIssued return In Processing state as needing confirmation

This prevents checking out to a patron an item with hold to someone
else in the In Processing state via staff interface.

Also the checkout error message via SIP is now a more clearer one: "Item is
on hold for another patron." Before it was "Item cannot be issued:
$confirmation".

Also the branch transfer and batch checkout pages are adapted to this new
confirmation message as well.

To test:
 1) Create bib level hold to an item for patron A
 2) Check-in that item via SIP2, now the hold state should be "In processing"
 3) Apply patch
 4) Try to checkout the item to patron B via staff interface and
    notice we get now confirmation prompt do we really want to do it
    because it is in processing.

 In order to not have to setup SIP2 server, alternatively steps 1) and
 2) can be done so that you check-in the item in staff interface and
 make it Waiting, and then with SQL change it to "In processing":

 UPDATE reserves SET found = "P" WHERE reserve_id = XXX;
 UPDATE reserves SET waitingdate = NULL WHERE reserve_id = XX
 UPDATE reserves SET expirationdate = NULL WHERE reserve_id = XXX;

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 41 Kyle M Hall 2021-03-02 14:34:25 UTC
Created attachment 117523 [details] [review]
Bug 25690: Add SIP2 tests for checking out with holds

This should cover whether checking out is allowed for all different
hold states:

Attached:
 - Waiting
 - In processing
 - Transfer

Unattached:
 - Reserved

To test:
 1) prove t/db_dependent/SIP/Transaction.t => passes

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 42 Kyle M Hall 2021-03-02 14:34:28 UTC
Created attachment 117524 [details] [review]
Bug 25690: (QA follow-up) PROCESSED -> PROCESSING for consistency

PROCESSED gave the apearance that the item processing had been completed
whereas in reading the code it appears to actually signify that the item
is awaiting/in proessing state.

This patch updates the variable to be PROCESSING consistently throughout
the codebase.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 43 Kyle M Hall 2021-03-02 14:34:32 UTC
Created attachment 117525 [details] [review]
Bug 25690: (follow-up) Correct grammar in warning messages

This makes the meaning of the warnings a bit more clear.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 44 Jonathan Druart 2021-03-03 15:52:06 UTC
-            if ( $res->{'itemnumber'} && $res->{'itemnumber'} == $itemnumber && $res->{'priority'} == 0) {

Why did you remove that test?
Comment 45 Jonathan Druart 2021-03-03 15:57:01 UTC
I am a bit tired right now to deep into the whole changes, but basically the line you removed was making sure we returned there for item-level holds, but @reserves can contain biblio-level holds (line 1766 AND (reserves.itemnumber IS NULL OR reserves.itemnumber = ?)).
Comment 46 Joonas Kylmälä 2021-03-04 05:39:49 UTC
(In reply to Jonathan Druart from comment #44)
> -            if ( $res->{'itemnumber'} && $res->{'itemnumber'} ==
> $itemnumber && $res->{'priority'} == 0) {
> 
> Why did you remove that test?

Thanks for asking. The reason I removed the line is because it equivalent of a reserve being found / attached and now we finally handle all the different cases so doing the check twice is not necessary. If you look at the patch https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=67411 (bug 19116) then it should be really clear. Previously the "Reserved" return value had double usage/meaning, firstly it could mean an unattached hold (biblio or item level) or secondly it could have meant an attached hold in transfer. Please see the explanation also in the commit message.
Comment 47 Joonas Kylmälä 2021-03-04 05:42:01 UTC
(In reply to Joonas Kylmälä from comment #46)
> (In reply to Jonathan Druart from comment #44)
> > -            if ( $res->{'itemnumber'} && $res->{'itemnumber'} ==
> > $itemnumber && $res->{'priority'} == 0) {
> > 
> > Why did you remove that test?
> 
> Thanks for asking. The reason I removed the line is because it equivalent of
> a reserve being found / attached and now we finally handle all the different
> cases so doing the check twice is not necessary. If you look at the patch
> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=67411 (bug
> 19116) then it should be really clear. Previously the "Reserved" return
> value had double usage/meaning, firstly it could mean an unattached hold
> (biblio or item level) or secondly it could have meant an attached hold in
> transfer. Please see the explanation also in the commit message.

And to explain even a bit more verbosely: The "$res->{'priority'} == 0" makes the hold found/attached (when combined together with itemnumber).
Comment 48 Jonathan Druart 2021-03-04 09:32:42 UTC
Thanks for you answer, Joonas.

Can you have a look at the output of t/db_dependent/Reserves.t please?
There are tons of warnings:
  Use of uninitialized value in string eq at /kohadevbox/koha/C4/Reserves.pm line 860.
Comment 49 Joonas Kylmälä 2021-03-04 10:34:47 UTC
Created attachment 117674 [details] [review]
Bug 25690: (follow-up) Supress warning about unitialized string

This fixes the warning:
Use of uninitialized value in string eq at /kohadevbox/koha/C4/Reserves.pm line 860.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 50 Joonas Kylmälä 2021-03-04 10:35:22 UTC
(In reply to Jonathan Druart from comment #48)
> Can you have a look at the output of t/db_dependent/Reserves.t please?
> There are tons of warnings:
>   Use of uninitialized value in string eq at /kohadevbox/koha/C4/Reserves.pm
> line 860.

Attached follow-up to fix that, thanks for spotting.
Comment 51 Joonas Kylmälä 2021-03-04 10:37:39 UTC
Btw, there quite nice refactoring opportunity around that if-else block that had the warnings, thinking of doing a follow-up patch series to tidy that up even more and also the now unnecessary SIP code that Kyle also spotted.
Comment 52 Joonas Kylmälä 2021-03-04 10:38:36 UTC
(In reply to Joonas Kylmälä from comment #51)
> Btw, there quite nice refactoring opportunity around that if-else block that
> had the warnings, thinking of doing a follow-up patch series to tidy that up
> even more and also the now unnecessary SIP code that Kyle also spotted.

To clarify, in another bug report totally, this one I consider big enough already.
Comment 53 Jonathan Druart 2021-03-04 16:25:14 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 54 Fridolin Somers 2021-03-08 15:36:38 UTC
Pushed to 20.11.x for 20.11.04
Comment 55 Andrew Fuerste-Henry 2021-03-14 19:21:46 UTC
Doesn't apply cleanly to 20.05, please rebase for backport. Thanks!