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.
(In reply to Minna Kivinen from comment #0) > 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 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. > > 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.
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.
Created attachment 116948 [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.
Created attachment 116949 [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: 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!
Created attachment 116950 [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.
Created attachment 116951 [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. 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". 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.
I have done some rudimentary testing to those attached patches that fix this issue and a bit more. I will move the status to Needs Sign-Off when I'm as certain as I possibly can that there is no bugs from my changes.
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.
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!
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
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;
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
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 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
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>
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>
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>
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>
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>
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>
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.
Above = comment 14
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>
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 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 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".
(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.
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>
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>
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>
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>
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>
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>
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>
(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.
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.
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>
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>
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>
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>
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>
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>
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>
- if ( $res->{'itemnumber'} && $res->{'itemnumber'} == $itemnumber && $res->{'priority'} == 0) { Why did you remove that test?
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 = ?)).
(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.
(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).
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.
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>
(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.
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.
(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.
Pushed to master for 21.05, thanks to everybody involved!
Pushed to 20.11.x for 20.11.04
Doesn't apply cleanly to 20.05, please rebase for backport. Thanks!