|
Description
Donna
2021-11-17 15:22:23 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? 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. (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. 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. 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)' There are no string changes here, just template logic fixes. 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. Martin, did you see my comment? I did, but I haven't had a moment to come back to work on this again. The same issue is present with HIGHHOLDS then right.. 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)' Created attachment 128550 [details] [review] Bug 29504: (follow-up) Account for other blockers My attempt at a fix My attempt at a fix 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> Created attachment 130188 [details] [review] Bug 29504: (follow-up) Account for other blockers Signed-off-by: David Nind <david@davidnind.com> 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? Resetting status, looks like something is wrong with master or my dev setup. What about batch checkouts? *** Bug 30448 has been marked as a duplicate of this bug. *** 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) 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> 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> Thanks Katrin, much appreciated. I'm not sure how to handle the batch option honestly.. hopefully something will dawn on me later. Pushed to master for 22.11. Nice work everyone, thanks! Backported to 22.05.x for 22.05.03 pushed to 21.11.x for 21.11.11 Backported: Pushed to 21.05.x branch for 21.05.17 Nothing to document, marking resolved. |