https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #104488|0 |1 is obsolete| | --- Comment #2 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 104496 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104496&action=edit Bug 25408: Add "opacitemholds" checks in CanBookBeReserved test plan: 1) set issuingrule with opacitemholds set to "Force" 2) try to add a new hold using multi-hold 3) hold is placed on the record where it shouldn't 4) apply patch 5) retry to place same hold 6) success: hold cannot be placed since record-level hold is not available) -- 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=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=21563 -- 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=25408 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Please add tests to cover this change. -- 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=25408 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |victor@tuxayo.net Status|Needs Signoff |Failed QA --- Comment #4 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Aside from the QA issue:

1) set issuingrule with opacitemholds set to "Force" 2) try to add a new hold using multi-hold 3) hold is placed on the record where it shouldn't

Can't reproduce. To detail: - went to circ rules - there is only one general rule, for all sites, all item types, all categories - set "OPAC item level holds" for "Force" - went to the OPAC - used the search - ticked two result - "Place hold" - placed a hold without touching anything. (on record had 1 item, the other 2) - went back to the staff interface to each record's hold page - both hold are listen as "Only item" That's not expected right? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|CanBookBeReserved should |CanBookBeReserved & |check "opacitemholds" |CanItemBeReserved should |policy |check "opacitemholds" | |policy --- Comment #6 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Well, seems this bug also impact holds placed with WebServices. CanItemBeReserved also miss this check... patch definitely need rework. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #8 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 105989 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=105989&action=edit Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved test plan: 1) apply tests patch 2) run tests 3) verify not green 4) apply patch 5) run tests (again) 6) success: green :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #104496|0 |1 is obsolete| | Attachment #105989|0 |1 is obsolete| | --- Comment #9 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 105990 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=105990&action=edit Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved test plan: 1) apply tests patch 2) run tests 3) verify not green 4) apply patch 5) run tests (again) 6) success: green :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #105988|0 |1 is obsolete| | --- Comment #10 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 106413 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=106413&action=edit Bug 25408: Added unit tests for opacitemholds policy Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #12 from Victor Grousset/tuxayo <victor@tuxayo.net> --- It works! Are the additional warnings caused by the patch? # Only test patch applied kohadev-koha@ea2bce8f4ac7:/kohadevbox/koha$ prove t/db_dependent/Reserves.t t/db_dependent/Reserves.t .. 14/62 Use of uninitialized value in string eq at /kohadevbox/koha/C4/Reserves.pm line 558. Use of uninitialized value in string eq at /kohadevbox/koha/C4/Reserves.pm line 558. t/db_dependent/Reserves.t .. 59/62 # Failed test 'record-level holds should not be possible with opacitemholds set to "Force"' # at t/db_dependent/Reserves.t line 1013. # got: 'OK' # expected: 'RecordHoldNotAllowed' # Failed test 'item-level holds should not be possible with opacitemholds set to "No"' # at t/db_dependent/Reserves.t line 1049. # got: 'OK' # expected: 'notReservable' # Looks like you failed 2 tests of 6. # Failed test 'test opacitemholds rules' # at t/db_dependent/Reserves.t line 1079. # Looks like you failed 1 test of 3. t/db_dependent/Reserves.t .. 61/62 # Failed test 'reserves.item_level_hold' # at t/db_dependent/Reserves.t line 1080. t/db_dependent/Reserves.t .. 62/62 # Looks like you failed 1 test of 62. t/db_dependent/Reserves.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/62 subtests Test Summary Report ------------------- t/db_dependent/Reserves.t (Wstat: 256 Tests: 62 Failed: 1) Failed test: 61 Non-zero exit status: 1 Files=1, Tests=62, 7 wallclock secs ( 0.04 usr 0.01 sys + 5.90 cusr 0.83 csys = 6.78 CPU) Result: FAIL # Applied the implementation kohadev-koha@ea2bce8f4ac7:/kohadevbox/koha$ prove t/db_dependent/Reserves.t t/db_dependent/Reserves.t .. 20/62 Use of uninitialized value in string eq at /kohadevbox/koha/C4/Reserves.pm line 577. Use of uninitialized value in string eq at /kohadevbox/koha/C4/Reserves.pm line 577. Use of uninitialized value in string eq at /kohadevbox/koha/C4/Reserves.pm line 329. Use of uninitialized value $opacitemholds in string eq at /kohadevbox/koha/C4/Reserves.pm line 425. Use of uninitialized value in string eq at /kohadevbox/koha/C4/Reserves.pm line 329. Use of uninitialized value in string eq at /kohadevbox/koha/C4/Reserves.pm line 329. Use of uninitialized value $opacitemholds in string eq at /kohadevbox/koha/C4/Reserves.pm line 425. t/db_dependent/Reserves.t .. ok All tests successful. Files=1, Tests=62, 8 wallclock secs ( 0.05 usr 0.01 sys + 5.97 cusr 0.71 csys = 6.74 CPU) Result: PASS kohadev-koha@ea2bce8f4ac7:/kohadevbox/koha$ -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #14 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Hi Tuxayo, Hope you are doing well, you seem to be doing fine! I've made a small qa patch to solve the warn :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #106414|0 |1 is obsolete| | --- Comment #16 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 106536 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=106536&action=edit Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved test plan: 1) apply tests patch 2) run tests 3) verify not green 4) apply patch 5) run tests (again) 6) success: green :) Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #18 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (In reply to Arthur Suzuki from comment #14)

I've made a small qa patch to solve the warn :)

It works and no additional warnings :) (In reply to Arthur Suzuki from comment #14)

Hi Tuxayo, Hope you are doing well, you seem to be doing fine!

Doing okay, best wishes for you :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com Status|Signed Off |Failed QA --- Comment #20 from Nick Clemens <nick@bywatersolutions.com> --- Needs a minor rebase I am not sure this is the best way to go. With these patches we fetch every item from the db and check the hold rule - but this doubles work we do in CanItemBeReserved and feels unnecessary Currently the loop in CanBookBeReserved returns as soon as it finds one 'OK' - you loop over all items to check the holds policy first - I think if we return the holds policy too from CanItemBeReserved we can return if one item is OK and item level holds not forced. I believe this would allow removal of the caller check too The code on the patches returns 'RecordHoldNotAllowed' if any items have item level holds forced, but shouldn't it only return that if all items on the biblio are forced? You add an extra fetch for the item which has already been fetched in the code: 429 $item = Koha::Items->find( $itemnumber ); -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #106535|0 |1 is obsolete| | Attachment #106536|0 |1 is obsolete| | Attachment #106537|0 |1 is obsolete| | --- Comment #21 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 107777 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=107777&action=edit Bug 25408: Added unit tests for opacitemholds policy Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #23 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 107779 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=107779&action=edit Bug 25408: (QA follow-up) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #25 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- (In reply to Nick Clemens from comment #20)

Needs a minor rebase

done \o/

I am not sure this is the best way to go. With these patches we fetch every item from the db and check the hold rule - but this doubles work we do in CanItemBeReserved and feels unnecessary

Currently the loop in CanBookBeReserved returns as soon as it finds one 'OK' - you loop over all items to check the holds policy first - I think if we return the holds policy too from CanItemBeReserved we can return if one item is OK and item level holds not forced. I believe this would allow removal of the caller check too

Good advice, applied :)

The code on the patches returns 'RecordHoldNotAllowed' if any items have item level holds forced, but shouldn't it only return that if all items on the biblio are forced?

Erfff... you mean different items belonging to a single biblio could have different rules? daaaaaamn...

You add an extra fetch for the item which has already been fetched in the code: 429 $item = Koha::Items->find( $itemnumber );

This has been corrected in the qa patch :) -- You are receiving this mail because: You are watching all bug changes.

afaik the most strict is enforced in the GUI (which makes sense to me) I talked this over with Andrew, there doesn't seem to be a perfect way to do

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #27 from Nick Clemens <nick@bywatersolutions.com> --- This seems to be affecting the staff side as well To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place hold on staff side 3 - Hold is not allowed 4 - Change rule to 'force' 5 - Hold can be placed I worry about this line: return { status => 'OK' } if ( $canReserve->{status} eq 'notReservable' && $opacitemholds eq 'N' ); as it short circuits some other checks, but I think the scripts end up double checking and it is not a problem. Other that that all the login in the scripts should be in the modules ;-) this, but as long as this patch isn't changing current rules I think we are good Can we get a more detailed test plan covering the cases on opac and staff side? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #29 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- (In reply to Nick Clemens from comment #27)

I worry about this line: return { status => 'OK' } if ( $canReserve->{status} eq 'notReservable' && $opacitemholds eq 'N' ); as it short circuits some other checks, but I think the scripts end up double checking and it is not a problem. Other that that all the login in the scripts should be in the modules ;-)

Checks not made in ILS-DI, at least when I first looked at the bug ;) Moving checks in this modules will helps maintaining coherence between interfaces (so i think...) but any opinion is welcome :) @Katrin : I see that there is now a "params" which might help passing data to these function, i'll try to get previous behaviour using this argument. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #32 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 111682 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111682&action=edit Bug 25408: More test (with staff context) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #34 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hi Arthur, your commit messages are a little sparse, the first commit should have a description of the problem and the test plan. Can you please add some more inofrmation? (see also comment#27?) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #111693|0 |1 is obsolete| | --- Comment #36 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 111696 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111696&action=edit Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place hold on staff side 3 - Hold is still allowed 4 - Attempt to place hold from opac or api 5 - Hold is not allowed 6 - Change rule to 'force' 7 - Hold can be placed in all interfaces Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #107777|0 |1 is obsolete| | Attachment #107780|0 |1 is obsolete| | Attachment #111681|0 |1 is obsolete| | Attachment #111682|0 |1 is obsolete| | Attachment #111716|0 |1 is obsolete| | --- Comment #38 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 111717 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111717&action=edit Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved The rule "opacitemholds" seemed not to be controlled in some cases. This patch adds a control for this issuingrule in such a way it is checked across all interfaces (staff, opac, api) To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place hold on staff side 3 - Hold is still allowed 4 - Attempt to place hold from opac or api 5 - Hold is not allowed 6 - Change rule to 'force' 7 - Hold can be placed in all interfaces Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #40 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 111719 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111719&action=edit Bug 25408: (QA follow-up) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #42 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 111721 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111721&action=edit Bug 25408: More test (with staff context) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #44 from Victor Grousset/tuxayo <victor@tuxayo.net> ---

5 - Hold is not allowed

«There are no items that can be placed on hold.» Looks ok I guess.

7 - Hold can be placed in all interfaces

Hold in staff interface silently fails. I go back to the hold list page but no hold listed. No logged error. Hold in OPAC fails with «This title cannot be requested.» -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #111717|0 |1 is obsolete| | Attachment #111718|0 |1 is obsolete| | Attachment #111719|0 |1 is obsolete| | Attachment #111720|0 |1 is obsolete| | Attachment #111721|0 |1 is obsolete| | --- Comment #45 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 115125 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=115125&action=edit Bug 25408: Added unit tests for opacitemholds policy Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #115125|0 |1 is obsolete| | --- Comment #47 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 115126 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=115126&action=edit Bug 25408: Added unit tests for opacitemholds policy Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #49 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 115128 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=115128&action=edit Bug 25408: (QA follow-up) Added error message when trying to hold on the record and opacitemhold is set to "Force" to item-level hold -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #51 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 115130 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=115130&action=edit Bug 25408: (QA follow-up) Added handling of calling context (staff or else) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #52 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 1. Not sure I understand, CanItemBeReserved is called from other C4::Reserves subroutines where the context is not passed. Why only passing it from direct calls from controller scripts? 2. The following change is not needed: sub CanBookBeReserved{ my ($borrowernumber, $biblionumber, $pickup_branchcode, $params) = @_; + my $patron = Koha::Patrons->find( $borrowernumber ); + my $borrower = $patron->unblessed; 3. Commit messages "(QA follow-up)" are not meaningful. Add more info or squash. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|20985 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20985 [Bug 20985] CanBookBeReserved and CanItemBeReserved should check 'On shelf holds allowed' policy -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #115126|0 |1 is obsolete| | Attachment #115127|0 |1 is obsolete| | Attachment #115128|0 |1 is obsolete| | Attachment #115129|0 |1 is obsolete| | Attachment #115130|0 |1 is obsolete| | --- Comment #54 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 121524 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121524&action=edit Bug 25408: Added unit tests for opacitemholds policy Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #56 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Hi Joubu, points 2 and 3 from last comment has been addressed. "Context" variable name is missused, I'm uploading a new version of the patch with use of C4::Context->interface. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #121526|0 |1 is obsolete| | --- Comment #58 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 121527 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121527&action=edit Bug 25408: Added unit tests for opacitemholds policy Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #121528|0 |1 is obsolete| | --- Comment #60 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 121530 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121530&action=edit Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved The rule "opacitemholds" seemed not to be controlled in some cases. This patch adds a control for this issuingrule in such a way it is checked across all interfaces (staff, opac, api) Added error message when trying to hold on the record and opacitemhold is set to "Force" to item-level hold To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place hold on staff side 3 - Hold is still allowed 4 - Attempt to place hold from opac or api 5 - Hold is not allowed 6 - Change rule to 'force' 7 - Item-level hold can be placed in all interfaces 8 - Record-level hold cannot be placed in all interfaces Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #62 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 128505 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128505&action=edit Bug 25408: (QA follow-up) This rule should not apply when placing a hold from staff interface -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #64 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Oh damn it, I see what you meant by "I can't place a hold at all from Opac". forget the "re-test please", I will re-work things (opac interface) and re-upload a patch. Things are working from staff and ils-di though. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #67 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 130181 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=130181&action=edit Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved The rule "opacitemholds" seemed not to be controlled in some cases. This patch adds a control for this issuingrule in such a way it is checked across all interfaces (staff, opac, api) Added error message when trying to hold on the record and opacitemhold is set to "Force" to item-level hold To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place hold on staff side 3 - Hold is still allowed 4 - Attempt to place hold from opac or api 5 - Hold is not allowed 6 - Change rule to 'force' 7 - Item-level hold can be placed in all interfaces 8 - Record-level hold cannot be placed in all interfaces Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply --- Comment #69 from Fridolin Somers <fridolin.somers@biblibre.com> --- Patch does not apply :

git bz apply 25408

Bug 25408 - CanBookBeReserved & CanItemBeReserved should check "opacitemholds" policy 130180 - Bug 25408: Added unit tests for opacitemholds policy 130181 - Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 25408: Added unit tests for opacitemholds policy Applying: Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved error: sha1 information is lacking or useless (t/db_dependent/Reserves.t). error: could not build fake ancestor Patch failed at 0001 Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-25408-Add-opacitemholds-checks-in-CanBookBeRes-2ODFm7.patch -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #130180|0 |1 is obsolete| | Attachment #130181|0 |1 is obsolete| | --- Comment #70 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 132658 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=132658&action=edit Bug 25408: Added unit tests for opacitemholds policy Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #72 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 132661 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=132661&action=edit Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved The rule "opacitemholds" seemed not to be controlled in some cases. This patch adds a control for this issuingrule in such a way it is checked across all interfaces (staff, opac, api) Added error message when trying to hold on the record and opacitemhold is set to "Force" to item-level hold To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place hold on staff side 3 - Hold is still allowed 4 - Attempt to place hold from opac or api 5 - Hold is not allowed 6 - Change rule to 'force' 7 - Item-level hold can be placed in all interfaces 8 - Record-level hold cannot be placed in all interfaces Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff --- Comment #73 from Victor Grousset/tuxayo <victor@tuxayo.net> --- There has been a confusion. My last testing was from before the last changes to fix issues found afterwards. So my signoff isn't valid anymore and this patch is still waiting for me or someone else to test it again. Switching back to need SO -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |30851 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30851 [Bug 30851] Improve performance of request.pl -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=15404 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #76 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- @David, I rebased the patch on latest master but reproduce what you've seen. I cannot place holds from OPAC with opacitemholds rule set to "Don't Allow" : should be able to place a record-level hold. Same with value "Force", although I should be able to place item-level holds. This patch needs rework... working on it -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #132661|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #78 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 164681 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164681&action=edit Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #164680|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #164682|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #81 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 164685 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164685&action=edit Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved The rule "opacitemholds" seemed not to be controlled in some cases. This patch adds a control for this issuingrule in such a way it is checked across all interfaces (staff, opac, api) Added error message when trying to hold on the record and opacitemhold is set to "Force" to item-level hold To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place item-level hold on staff interface 3 - Hold is still allowed (rule do not apply to staff) 4 - Attempt to place item-level hold from opac or ilsdi 5 - Hold is not allowed (rule apply from opac or ilsdi) 6 - Change rule to 'force' 7 - Item-level hold can still be placed from the staff interface 8 - Record-level hold cannot be placed from opac or ilsdi -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #83 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Hello Guys, I've made a rework of that one so I thought it would be better to have it back to Need-Signoff status. So, it's been rebased, tests provided. I made a separate patch for perltidy because it introduce a lot of cosmetic changes (but no logic changes, which is in the previous patch). Hopefully it will pass the signoff and qa this time :) Best, Arthur -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #164685|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #84 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 164693 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164693&action=edit Bug 25408: Added unit tests for opacitemholds policy -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |pedro.amorim@ptfs-europe.co | |m -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #164694|0 |1 is obsolete| | --- Comment #88 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 164879 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164879&action=edit Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved The rule "opacitemholds" seemed not to be controlled in some cases. This patch adds a control for this issuingrule in such a way it is checked across all interfaces (staff, opac, api) Added error message when trying to hold on the record and opacitemhold is set to "Force" to item-level hold To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place item-level hold on staff interface 3 - Hold is still allowed (rule do not apply to staff) 4 - Attempt to place item-level hold from opac or ilsdi 5 - Hold is not allowed (rule apply from opac or ilsdi) 6 - Change rule to 'force' 7 - Item-level hold can still be placed from the staff interface 8 - Record-level hold cannot be placed from opac or ilsdi Signed-off-by: Pedro Amorim <pedro.amorim@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=25408 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #90 from Victor Grousset/tuxayo <victor@tuxayo.net> ---

- Unnecessary changes are made e.g. $patron->borrowernumber to $patron->id (correct me if I'm wrong here, please!)

Indeed, in bunch of lines that are already changed, it's a good opportunity to enforce terminology without adding more conflict possibilities. But here it's in lines that are not changed, so it's adding new conflicts possibilities with other submitted patches and when backporting.

- Some lines that have not been changed have been tidied and included

Yes, also that.

- Personally I'd prefer this to be unfolded in more commits to ease the review process, with each having a short explanation of why the changes are required:

At least having the full commit message list these points. Not sure about splitting the commit after the work is done. Maybe I missed the main point, but in this case the 3 changes look 95% likely to have been done together and not as iteration with a different logic on each step. That's a nice thing to keep in mind for other reviews or my own patches, thanks :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #164879|0 |1 is obsolete| | --- Comment #92 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 165112 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=165112&action=edit Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved The rule "opacitemholds" seemed not to be controlled in some cases. This patch adds a control for this issuingrule in such a way it is checked across all interfaces (staff, opac, api). Added error message when trying to hold on the record and opacitemhold is set to "Force" to item-level hold What this patch does : -- Move get_opacitemholds_policy check into Reserves.pm::CanBookBeReserved -- Update CanItemBeReserved to return recordHoldsOnly status if interface is OPAC -- Update warning labels in opac-reserve.tt accordingly. To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place item-level hold on staff interface 3 - Hold is still allowed (rule do not apply to staff) 4 - Attempt to place item-level hold from opac or ilsdi 5 - Hold is not allowed (rule apply from opac or ilsdi) 6 - Change rule to 'force' 7 - Item-level hold can still be placed from the staff interface 8 - Record-level hold cannot be placed from opac or ilsdi -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |victor@tuxayo.net |y.org | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #96 from Victor Grousset/tuxayo <victor@tuxayo.net> ---

it should be set back to Signed-off

Indeed, there are no changes to the code that would invalidate the signoff. (minor ones wouldn't invalidate it, but here, it's even zero actual changes :) ) Thanks Pedro for giving a look and thanks Arthur for the fixes :) Will look back asap see if I understand enough the code to QA. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #165112|0 |1 is obsolete| | --- Comment #98 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 165443 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=165443&action=edit Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved The rule "opacitemholds" seemed not to be controlled in some cases. This patch adds a control for this issuingrule in such a way it is checked across all interfaces (staff, opac, api). Added error message when trying to hold on the record and opacitemhold is set to "Force" to item-level hold What this patch does : -- Move get_opacitemholds_policy check into Reserves.pm::CanBookBeReserved -- Update CanItemBeReserved to return recordHoldsOnly status if interface is OPAC -- Update warning labels in opac-reserve.tt accordingly. To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place item-level hold on staff interface 3 - Hold is still allowed (rule do not apply to staff) 4 - Attempt to place item-level hold from opac or ilsdi 5 - Hold is not allowed (rule apply from opac or ilsdi) 6 - Change rule to 'force' 7 - Item-level hold can still be placed from the staff interface 8 - Record-level hold cannot be placed from opac or ilsdi Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #165443|0 |1 is obsolete| | --- Comment #100 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 165445 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=165445&action=edit Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved The rule "opacitemholds" seemed not to be controlled in some cases. This patch adds a control for this issuingrule in such a way it is checked across all interfaces (staff, opac, api). Added error message when trying to hold on the record and opacitemhold is set to "Force" to item-level hold What this patch does : -- Move get_opacitemholds_policy check into Reserves.pm::CanBookBeReserved -- Update CanItemBeReserved to return recordHoldsOnly status if interface is OPAC -- Update warning labels in opac-reserve.tt accordingly. To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place item-level hold on staff interface 3 - Hold is still allowed (rule do not apply to staff) 4 - Attempt to place item-level hold from opac or ilsdi 5 - Hold is not allowed (rule apply from opac or ilsdi) 6 - Change rule to 'force' 7 - Item-level hold can still be placed from the staff interface 8 - Record-level hold cannot be placed from opac or ilsdi Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #102 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- I am sure everyone here has tested well, but I'd feel a little bit more confident if we could maybe get Kyle or another hold expert to look at this. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #104 from Nick Clemens <nick@bywatersolutions.com> --- I am still unsure about one of the lines here: In CanBookBeReserved, during the item loop: return { status => 'recordHoldNotAllowed' } if $opacitemholds eq 'F'; If one of the items is type 'new' and doesn't allow record level holds, but another is 'book' and does allow it - we will return 'recordHoldNotAllowed' if we hit the 'new' item first. You also call 'get_opacitemholds_policy' in CanBookBeReserved and also add a call to get_effective_rules in CanItemBeReserved - these should be getting the same rule - why do we need to check at both levels? The check in CanItemBeReseerved should handle returning the necessary status -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #105 from Nick Clemens <nick@bywatersolutions.com> --- see comment 20 and comment 25 - I think the problem with multiple item types on a single bib is still the issue -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |baptiste.wojtkowski@biblibr | |e.com --- Comment #107 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Hi ! I rebased and tried to go through the test plan but I could not manage to reproduce the first issue, is it normal ? My tests on "OPAC item level holds" on the opac BEFORE applying patch: 1 - Force -> I cannot place hold on record, only on item 2 - Don't allow -> I cannot place hold on item 3 - Allow -> I can place on both I always can place an hold on opac. (Applying the patch breaks this but I might have improperly rebased) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #165444|0 |1 is obsolete| | --- Comment #109 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 176115 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176115&action=edit Bug 25408: Added unit tests for opacitemholds policy Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #111 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Rebased on main -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|32702 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 [Bug 32702] Item statuses that block holds should be checked in CanItemBeReserved -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|victor@tuxayo.net | Status|Failed QA |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=25408 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #176115|0 |1 is obsolete| | Attachment #176116|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #114 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 206968 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206968&action=edit Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved The rule "opacitemholds" seemed not to be controlled in some cases. This patch adds a control for this issuingrule in such a way it is checked across all interfaces (staff, opac, api). Added error message when trying to hold on the record and opacitemhold is set to "Force" to item-level hold What this patch does : -- Move get_opacitemholds_policy check into Reserves.pm::CanBookBeReserved -- Update CanItemBeReserved to return recordHoldsOnly status if interface is OPAC -- Update warning labels in opac-reserve.tt accordingly. To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place item-level hold on staff interface 3 - Hold is still allowed (rule do not apply to staff) 4 - Attempt to place item-level hold from opac or ilsdi 5 - Hold is not allowed (rule apply from opac or ilsdi) 6 - Change rule to 'force' 7 - Item-level hold can still be placed from the staff interface 8 - Record-level hold cannot be placed from opac or ilsdi Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 Brendan Lawlor <blawlor@clamsnet.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #115 from Brendan Lawlor <blawlor@clamsnet.org> --- This patch removed/changed these lines in opac-reserve.pl: - my $status = CanBookBeReserved( $borrowernumber, $biblioNum )->{status}; - $biblioLoopIter{holdable} &&= $status eq 'OK'; - $biblioLoopIter{$status} = 1; Without $biblioLoopIter{$status} = 1; the statuses returned by CanBookBeReserved are never set as keys in $biblioLooperIter, so in the template the error messages can never be shown. E.g. in opac-reserves.tt:221-247 [% ELSIF bibitemloo.itemAlreadyOnHold %] <div class="alert alert-warning">This title cannot be requested because you already have hold for this item.</div> Also I think the new recordHoldsOnly and recordHoldNotAllowed should have messages added to opac-user.tt:79-115 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #116 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 209287 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209287&action=edit Bug 25408: (QA follow-up) Fix missing hold error messages in the OPAC The previous patch dropped the line recording the CanBookBeReserved status as a key of biblioLoopIter, so the specific reasons a hold cannot be placed (already in your possession, too many holds, ...) were never passed to the template and only a generic message was displayed. Restore $biblioLoopIter{$record_status} = 1; in opac-reserve.pl. Also add messages for the new recordHoldsOnly and recordHoldNotAllowed statuses to the failed_holds list in opac-user.tt, so patrons get a meaningful notice when a hold fails because of the "OPAC item level holds" rule. To test: 0 - Apply the patch 1 - Check out an item to a patron, then try to place a hold on its record from the OPAC 2 - The page should say the title cannot be requested because it is already in your possession 3 - Set the "OPAC item level holds" rule to "Force" and try to place a record-level hold from the OPAC (e.g. via multi-hold) 4 - The notice on opac-user.pl should say record-level holds are not allowed on this title -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #117 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 209288 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209288&action=edit Bug 25408: Added unit tests for opacitemholds policy Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #118 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 209289 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209289&action=edit Bug 25408: Added unit tests for opacitemholds policy Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #120 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 209291 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209291&action=edit Bug 25408: (QA follow-up) Fix missing hold error messages in the OPAC The previous patch dropped the line recording the CanBookBeReserved status as a key of biblioLoopIter, so the specific reasons a hold cannot be placed (already in your possession, too many holds, ...) were never passed to the template and only a generic message was displayed. Restore $biblioLoopIter{$record_status} = 1; in opac-reserve.pl. Also add messages for the new recordHoldsOnly and recordHoldNotAllowed statuses to the failed_holds list in opac-user.tt, so patrons get a meaningful notice when a hold fails because of the "OPAC item level holds" rule. To test: 0 - Apply the patch 1 - Check out an item to a patron, then try to place a hold on its record from the OPAC 2 - The page should say the title cannot be requested because it is already in your possession 3 - Set the "OPAC item level holds" rule to "Force" and try to place a record-level hold from the OPAC (e.g. via multi-hold) 4 - The notice on opac-user.pl should say record-level holds are not allowed on this title -- You are receiving this mail because: You are watching all bug changes.