Bug 29504 - Confirm item parts requires force_checkout permission (checkouts tab)
Summary: Confirm item parts requires force_checkout permission (checkouts tab)
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low major with 10 votes (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
: 30448 (view as bug list)
Depends on: 25261
Blocks: 30995
  Show dependency treegraph
 
Reported: 2021-11-17 15:22 UTC by Donna
Modified: 2023-06-08 22:26 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.03, 21.11.11, 21.05.17


Attachments
Bug 29504: Allow 'ADDITIONAL_MATERIALS' check to be confirmed (2.55 KB, patch)
2021-11-22 11:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29504: Allow 'ADDITIONAL_MATERIALS' check to be confirmed (2.55 KB, patch)
2021-12-15 09:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29504: (follow-up) Account for other blockers (3.42 KB, patch)
2021-12-15 09:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29504: Allow 'ADDITIONAL_MATERIALS' check to be confirmed (2.60 KB, patch)
2022-02-05 13:36 UTC, David Nind
Details | Diff | Splinter Review
Bug 29504: (follow-up) Account for other blockers (3.46 KB, patch)
2022-02-05 13:36 UTC, David Nind
Details | Diff | Splinter Review
Bug 29504: Allow 'ADDITIONAL_MATERIALS' check to be confirmed (2.62 KB, patch)
2022-06-19 12:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29504: (follow-up) Account for other blockers (3.48 KB, patch)
2022-06-19 12:51 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Donna 2021-11-17 15:22:23 UTC
With bug 25261, CircConfirmItemParts system preference was added, which is highly popular with libraries.  However, it requires staff to have the permission "Force checkout if a limitation exists (force_checkout)".  This is a permission that many staff do not have.

When a staff member without the Force permission attempts to checkout an item with a 952$3, they get a yellow box stating: 
Please confirm checkout
Please confirm that the accompanying materials are present: check for 12 CDs

The only option given is the Continue button (with a red X) and when clicked, the item does not get checked out.

To replicate:
1.  Set a staff member with circ permissions, but not FORCE_CHECKOUT
2.  Turn on CircConfirmItemParts
3.  Log in as staff member in step 1
4.  Attempt to checkout an item with a 952$3
Comment 1 Katrin Fischer 2021-11-18 09:42:37 UTC
Hm, I was under the impression force_checkout was only for restrictions on the account. 
Maybe this should be made more similar to how "Override blocked renewals"?
Or can we assume that this should never block a checkout but only force staff to check for completeness and confirm?
Comment 2 Donna 2021-11-18 13:44:56 UTC
From talking with libraries, the thought is that it should never block a checkout, but make staff pause to verify parts and then either confirm so the item is checked out, or decline, and the item does not get checked out.
Comment 3 Katrin Fischer 2021-11-18 13:54:01 UTC
(In reply to Donna from comment #2)
> From talking with libraries, the thought is that it should never block a
> checkout, but make staff pause to verify parts and then either confirm so
> the item is checked out, or decline, and the item does not get checked out.

Thx Donna, that's what I was thinking too. So maybe it's just about removing the check? Adding Martin.
Comment 4 Christopher Brannon 2021-11-19 23:17:50 UTC
I agree this should not have been a blocker.  It is only meant to grab attention to details.  Having it block is a blocker itself.
Comment 5 Martin Renvoize 2021-11-22 11:38:11 UTC
Created attachment 127926 [details] [review]
Bug 29504: Allow 'ADDITIONAL_MATERIALS' check to be confirmed

The additional materials check when CircConfirmParts is enabled was
errantly relying upon the 'FORCE_CHECKOUT' permission.  This patch
updates the template to allow confirmation as well as cancellation of
chekout.

Test plan
1. Set a staff member with circ permissions, but not FORCE_CHECKOUT
2. Turn on CircConfirmItemParts
3. Log in as staff member in step 1
4. Attempt to checkout an item with a 952$3
5. The only option given is the Continue button (with a red X) and when
   clicked, the item does not get checked out.
6. Apply patch
7. Repeat step 4.
8. You should now have the option to 'Yes, check out (Y)' or
   'No, don't check out (N)'
Comment 6 Martin Renvoize 2021-11-22 11:39:16 UTC
There are no string changes here, just template logic fixes.
Comment 7 Jonathan Druart 2021-11-22 13:06:34 UTC
Batch checkout is broken as well for 'ADDITIONAL_MATERIALS'. I've tried to fix it but it's not trivial, and I think this patch is impacted by the same problem.
You can have ADDITIONAL_MATERIALS AND another flags, in that case you shouldn't be allowed to bypass.
We need to test that ADDITIONAL_MATERIALS is the only flag that needs a confirmation.
Comment 8 Jonathan Druart 2021-12-15 08:07:12 UTC
Martin, did you see my comment?
Comment 9 Martin Renvoize 2021-12-15 08:52:14 UTC
I did, but I haven't had a moment to come back to work on this again.
Comment 10 Martin Renvoize 2021-12-15 09:13:55 UTC
The same issue is present with HIGHHOLDS then right..
Comment 11 Martin Renvoize 2021-12-15 09:21:53 UTC
Created attachment 128549 [details] [review]
Bug 29504: Allow 'ADDITIONAL_MATERIALS' check to be confirmed

The additional materials check when CircConfirmParts is enabled was
errantly relying upon the 'FORCE_CHECKOUT' permission.  This patch
updates the template to allow confirmation as well as cancellation of
chekout.

Test plan
1. Set a staff member with circ permissions, but not FORCE_CHECKOUT
2. Turn on CircConfirmItemParts
3. Log in as staff member in step 1
4. Attempt to checkout an item with a 952$3
5. The only option given is the Continue button (with a red X) and when
   clicked, the item does not get checked out.
6. Apply patch
7. Repeat step 4.
8. You should now have the option to 'Yes, check out (Y)' or
   'No, don't check out (N)'
Comment 12 Martin Renvoize 2021-12-15 09:21:57 UTC
Created attachment 128550 [details] [review]
Bug 29504: (follow-up) Account for other blockers
Comment 13 Martin Renvoize 2021-12-15 09:22:35 UTC
My attempt at a fix
Comment 14 Martin Renvoize 2021-12-15 09:22:40 UTC
My attempt at a fix
Comment 15 David Nind 2022-02-05 13:36:52 UTC
Created attachment 130187 [details] [review]
Bug 29504: Allow 'ADDITIONAL_MATERIALS' check to be confirmed

The additional materials check when CircConfirmParts is enabled was
errantly relying upon the 'FORCE_CHECKOUT' permission.  This patch
updates the template to allow confirmation as well as cancellation of
chekout.

Test plan
1. Set a staff member with circ permissions, but not FORCE_CHECKOUT
2. Turn on CircConfirmItemParts
3. Log in as staff member in step 1
4. Attempt to checkout an item with a 952$3
5. The only option given is the Continue button (with a red X) and when
   clicked, the item does not get checked out.
6. Apply patch
7. Repeat step 4.
8. You should now have the option to 'Yes, check out (Y)' or
   'No, don't check out (N)'

Signed-off-by: David Nind <david@davidnind.com>
Comment 16 David Nind 2022-02-05 13:36:57 UTC
Created attachment 130188 [details] [review]
Bug 29504: (follow-up) Account for other blockers

Signed-off-by: David Nind <david@davidnind.com>
Comment 17 Katrin Fischer 2022-02-07 21:59:39 UTC
QA test tools fail:
 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
   FAIL	  valid_template
		Attempt to reload Koha/Template/Plugin/Branches.pm aborted.
Compilation failed in require at /usr/lib/x86_64-linux-gnu/perl5/5.32/Template/Plugins.pm line 206.


Can you also check Jonathan's comment on batch checkouts please? Should we also be testing batch checkouts here?
Comment 18 Katrin Fischer 2022-02-07 22:06:27 UTC
Resetting status, looks like something is wrong with master or my dev setup.
Comment 19 Jonathan Druart 2022-02-08 16:37:17 UTC
What about batch checkouts?
Comment 20 Kelly McElligott 2022-04-04 13:07:58 UTC
*** Bug 30448 has been marked as a duplicate of this bug. ***
Comment 21 Katrin Fischer 2022-06-19 12:50:05 UTC
I've tested the batch checkout and Joubu is right, it doesn't work. The button to perform the checkouts is missing, but it's there when testing with a superlibrarian.

My suggestion would be to still have this fix and report/fix the batch checkout feature separately to get this moving again:

Bug 30995 - Bug 29504 - Confirm item parts requires force_checkout permission (batch checkout)
Comment 22 Katrin Fischer 2022-06-19 12:51:38 UTC
Created attachment 136330 [details] [review]
Bug 29504: Allow 'ADDITIONAL_MATERIALS' check to be confirmed

The additional materials check when CircConfirmParts is enabled was
errantly relying upon the 'FORCE_CHECKOUT' permission.  This patch
updates the template to allow confirmation as well as cancellation of
chekout.

Test plan
1. Set a staff member with circ permissions, but not FORCE_CHECKOUT
2. Turn on CircConfirmItemParts
3. Log in as staff member in step 1
4. Attempt to checkout an item with a 952$3
5. The only option given is the Continue button (with a red X) and when
   clicked, the item does not get checked out.
6. Apply patch
7. Repeat step 4.
8. You should now have the option to 'Yes, check out (Y)' or
   'No, don't check out (N)'

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Katrin Fischer 2022-06-19 12:51:44 UTC
Created attachment 136331 [details] [review]
Bug 29504: (follow-up) Account for other blockers

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 24 Martin Renvoize 2022-06-20 11:30:45 UTC
Thanks Katrin, much appreciated.

I'm not sure how to handle the batch option honestly.. hopefully something will dawn on me later.
Comment 25 Tomás Cohen Arazi 2022-06-21 18:29:24 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 26 Lucas Gass 2022-07-15 17:49:12 UTC
Backported to 22.05.x for 22.05.03
Comment 27 Arthur Suzuki 2022-07-21 14:28:40 UTC
pushed to 21.11.x for 21.11.11
Comment 28 Victor Grousset/tuxayo 2022-07-24 03:09:28 UTC
Backported: Pushed to 21.05.x branch for 21.05.17
Nothing to document, marking resolved.