[Bug 25690] New: SIP allows to check out an item with hold in transfer to another patron
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Bug ID: 25690 Summary: SIP allows to check out an item with hold in transfer to another patron Change sponsored?: --- Product: Koha Version: 20.05 Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: minna.kivinen@hamk.fi QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #1 from Minna Kivinen <minna.kivinen@hamk.fi> --- (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.
-- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #2 from Minna Kivinen <minna.kivinen@hamk.fi> --- 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|SIP allows to check out an |SIP should not allow to |item with hold in transfer |check out an item in |to another patron |transfer because of a hold | |to another patron -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |nugged@gmail.com |ity.org | CC| |nugged@gmail.com, | |slavashishkin@gmail.com, | |stalkernoid@gmail.com -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|nugged@gmail.com |joonas.kylmala@helsinki.fi CC| |joonas.kylmala@helsinki.fi -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|20.05 |master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #3 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 116948 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=116948&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #4 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 116949 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=116949&action=edit 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #5 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 116950 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=116950&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #6 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 116951 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=116951&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #7 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |emmi.takkinen@koha-suomi.fi -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #116948|0 |1 is obsolete| | Attachment #116949|0 |1 is obsolete| | Attachment #116950|0 |1 is obsolete| | Attachment #116951|0 |1 is obsolete| | --- Comment #8 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 117000 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117000&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #9 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 117001 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117001&action=edit 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #10 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 117002 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117002&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #11 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 117003 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117003&action=edit 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; -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #12 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 117004 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117004&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #13 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |- Proper warning messages release notes| |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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #14 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Comment on attachment 117000 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117000 Bug 25690: Remove double usage of 'Reserved' return value Review of attachment 117000: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=25690&attachment=117000) ----------------------------------------------------------------- ::: 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117000|0 |1 is obsolete| | --- Comment #15 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 117062 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117062&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117001|0 |1 is obsolete| | --- Comment #16 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 117063 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117063&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117002|0 |1 is obsolete| | --- Comment #17 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 117064 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117064&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117003|0 |1 is obsolete| | --- Comment #18 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 117065 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117065&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117004|0 |1 is obsolete| | --- Comment #19 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 117066 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117066&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #20 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 117067 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117067&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com Status|Needs Signoff |Signed Off --- Comment #21 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #22 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Above = comment 14 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #23 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 117374 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117374&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #24 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #25 from Kyle M Hall <kyle@bywatersolutions.com> --- Comment on attachment 117064 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117064 Bug 25690: Remove duplicated logic in SIP2 checkouts Review of attachment 117064: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=25690&attachment=117064) ----------------------------------------------------------------- ::: 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #26 from Kyle M Hall <kyle@bywatersolutions.com> --- Comment on attachment 117374 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117374 Bug 25690: (follow-up) Correct grammar in warning messages Review of attachment 117374: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=25690&attachment=117374) ----------------------------------------------------------------- ::: 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". -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |kyle@bywatersolutions.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Patch doesn't apply -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #27 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (In reply to Kyle M Hall from comment #25)
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?
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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117062|0 |1 is obsolete| | Attachment #117063|0 |1 is obsolete| | Attachment #117064|0 |1 is obsolete| | Attachment #117065|0 |1 is obsolete| | Attachment #117066|0 |1 is obsolete| | Attachment #117067|0 |1 is obsolete| | Attachment #117374|0 |1 is obsolete| | --- Comment #28 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 117507 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117507&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #29 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 117508 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117508&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #30 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 117509 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117509&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #31 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 117510 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117510&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #32 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 117511 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117511&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #33 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 117512 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117512&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #34 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 117513 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117513&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Signed Off --- Comment #35 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #36 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117507|0 |1 is obsolete| | Attachment #117508|0 |1 is obsolete| | Attachment #117509|0 |1 is obsolete| | Attachment #117510|0 |1 is obsolete| | Attachment #117511|0 |1 is obsolete| | Attachment #117512|0 |1 is obsolete| | Attachment #117513|0 |1 is obsolete| | --- Comment #37 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 117519 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117519&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #38 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 117520 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117520&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #39 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 117521 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117521&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #40 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 117522 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117522&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #41 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 117523 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117523&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #42 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 117524 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117524&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #43 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 117525 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117525&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #44 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- - if ( $res->{'itemnumber'} && $res->{'itemnumber'} == $itemnumber && $res->{'priority'} == 0) { Why did you remove that test? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #45 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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 = ?)). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #46 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #47 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #48 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #49 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 117674 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117674&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #50 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #51 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #52 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |21.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #53 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 21.05, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|21.05.00 |21.05.00,20.11.04 released in| | Status|Pushed to master |Pushed to stable CC| |fridolin.somers@biblibre.co | |m --- Comment #54 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 20.11.x for 20.11.04 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 --- Comment #55 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Doesn't apply cleanly to 20.05, please rebase for backport. Thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |27936 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27936 [Bug 27936] AllowItemsOnHoldCheckoutSIP does not allow checkout of items currently waiting for a hold -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|27936 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27936 [Bug 27936] AllowItemsOnHoldCheckoutSIP does not allow checkout of items currently waiting for a hold -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |28136 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28136 [Bug 28136] Transferred holds are not triggering -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |28139 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28139 [Bug 28139] Processing holds are not filled automatically -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25690 Daniel Gaghan <daniel.gaghan@pueblolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |daniel.gaghan@pueblolibrary | |.org -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org