Many libraries wish to be able to allow self-check for multi-part items while requesting librarians to verify the parts when checking out/in at the desk. It makes sense to split CircConfirmItemParts into three prefs such that the feature can be enabled/disabled for SCO and SCI independently.
Created attachment 154910 [details] [review] Bug 34643: Split CircConfirmItemParts for self-checkout and self-checkin Many libraries wish to be able to allow self-check for multi-part items while requesting librarians to verify the parts when checking out/in at the desk. It makes sense to split CircConfirmItemParts into three prefs such that the feature can be enabled/disabled for SCO and SCI independently. Test Plan: 1) Apply this patch 2) Enable SCO and SCI 3) Set CircConfirmItemPartsSCO to "Don't allow" 4) Attempt to checkout out an item with materials via SCO, it should fail 5) Set CircConfirmItemPartsSCO to "Allow" 6) Repeat step 4, it should succeed 7) Set CircConfirmItemPartsSCI to "Don't allow" 8) Attempt to check in the item via SCI, it should fail 9) Attempt to check in the item via SCO, it should fail 9) Set CircConfirmItemPartsSCI to "Allow" 10) Attempt to check in the item via SCO and SCI, it should succeed!
*** Bug 33700 has been marked as a duplicate of this bug. ***
This works, but could be presented more clearly. Your syspref names strongly imply that one preference controls the behavior of the web-based self checkout while the other controls the behavior of the web-based self check-in. However that is not quite the case. I suggest rewording the sysprefs thus: CircConfirmItemPartsSCI: (Allow / Don't allow) multi-part items to be checked in via the web-based self check-in and self checkout modules. CircConfirmItemPartsSCO: (Allow / Don't allow) multi-part items to be checked out via the web-based self checkout module. This clarifies the difference between the two sysprefs, clarifies that they're not about SIP or any other interface, and makes the wording consistent with SelfCheckInModule and WebBasedSelfCheck.
Created attachment 159113 [details] [review] Bug 34643: Split CircConfirmItemParts for self-checkout and self-checkin Many libraries wish to be able to allow self-check for multi-part items while requesting librarians to verify the parts when checking out/in at the desk. It makes sense to split CircConfirmItemParts into three prefs such that the feature can be enabled/disabled for SCO and SCI independently. Test Plan: 1) Apply this patch 2) Enable SCO and SCI 3) Set CircConfirmItemPartsSCO to "Don't allow" 4) Attempt to checkout out an item with materials via SCO, it should fail 5) Set CircConfirmItemPartsSCO to "Allow" 6) Repeat step 4, it should succeed 7) Set CircConfirmItemPartsSCI to "Don't allow" 8) Attempt to check in the item via SCI, it should fail 9) Attempt to check in the item via SCO, it should fail 9) Set CircConfirmItemPartsSCI to "Allow" 10) Attempt to check in the item via SCO and SCI, it should succeed! Signed-off-by: AFHDubCoLib <andrewfh@dubcolib.org>
I made those wording changes as part of signing off.
Created attachment 163145 [details] [review] Bug 34643: Split CircConfirmItemParts for self-checkout and self-checkin Many libraries wish to be able to allow self-check for multi-part items while requesting librarians to verify the parts when checking out/in at the desk. It makes sense to split CircConfirmItemParts into three prefs such that the feature can be enabled/disabled for SCO and SCI independently. Test Plan: 1) Apply this patch 2) Enable SCO and SCI 3) Set CircConfirmItemPartsSCO to "Don't allow" 4) Attempt to checkout out an item with materials via SCO, it should fail 5) Set CircConfirmItemPartsSCO to "Allow" 6) Repeat step 4, it should succeed 7) Set CircConfirmItemPartsSCI to "Don't allow" 8) Attempt to check in the item via SCI, it should fail 9) Attempt to check in the item via SCO, it should fail 9) Set CircConfirmItemPartsSCI to "Allow" 10) Attempt to check in the item via SCO and SCI, it should succeed! Signed-off-by: AFHDubCoLib <andrewfh@dubcolib.org>
Is that something we usually do to not have some sysprefs in installer/data/mysql/mandatory/sysprefs.sql ? When their default value is 0. It's not mentioned here: https://wiki.koha-community.org/wiki/System_Preferences#Adding_a_new_system_preference --- Amended the patch for minor tidying. --- Otherwise, code looks good.
(In reply to Victor Grousset/tuxayo from comment #7) > Is that something we usually do to not have some sysprefs in > installer/data/mysql/mandatory/sysprefs.sql ? > When their default value is 0. Confirmed that is an issue: Bug 34979
Created attachment 163318 [details] [review] Bug 34643: (QA follow-up) Add new sysprefs to sysprefs.sql
Created attachment 163657 [details] [review] Bug 34643: (QA follow-up): fix order of new sysprefs and quotation
Thanks for the follow-up. > ("CircConfirmItemPartsSCI", ?, NULL, "Deny checkout of multipart items for self-checkin", "Yes/No"), ↑↑↑↑ Hmm, that question mark pretty sus. Somehow it doesn't exploded when populating the DB and can go unnoticed :o So, it should be 0, right? Because we copy CircConfirmItemParts in the dbrev and here it defaults to 0.
Created attachment 163675 [details] [review] Bug 34643: (QA follow-up) Fix default syspref values
(In reply to Victor Grousset/tuxayo from comment #11) > Thanks for the follow-up. > > > ("CircConfirmItemPartsSCI", ?, NULL, "Deny checkout of multipart items for self-checkin", "Yes/No"), > ↑↑↑↑ > Hmm, that question mark pretty sus. > Somehow it doesn't exploded when populating the DB and can go unnoticed :o Very sus ;) Also fixed now!
Ain't no way... SCI and SCO are broken due to not complying with CSRF. Reviewing this patch is endless [U+1F62D] Great, it's a known bug and it's active: Bug 36349 (OMG it's a mess [U+1F62D])
Kyle, this one no longer applies to main. Some merge conflicts in opac/sco/sco-main.pl
Created attachment 170829 [details] [review] Bug 34643: Split CircConfirmItemParts for self-checkout and self-checkin Many libraries wish to be able to allow self-check for multi-part items while requesting librarians to verify the parts when checking out/in at the desk. It makes sense to split CircConfirmItemParts into three prefs such that the feature can be enabled/disabled for SCO and SCI independently. Test Plan: 1) Apply this patch 2) Enable SCO and SCI 3) Set CircConfirmItemPartsSCO to "Don't allow" 4) Attempt to checkout out an item with materials via SCO, it should fail 5) Set CircConfirmItemPartsSCO to "Allow" 6) Repeat step 4, it should succeed 7) Set CircConfirmItemPartsSCI to "Don't allow" 8) Attempt to check in the item via SCI, it should fail 9) Attempt to check in the item via SCO, it should fail 9) Set CircConfirmItemPartsSCI to "Allow" 10) Attempt to check in the item via SCO and SCI, it should succeed! Signed-off-by: AFHDubCoLib <andrewfh@dubcolib.org>
Created attachment 170830 [details] [review] Bug 34643: (QA follow-up) Add new sysprefs to sysprefs.sql
Created attachment 170831 [details] [review] Bug 34643: (QA follow-up): fix order of new sysprefs and quotation
Created attachment 170832 [details] [review] Bug 34643: (QA follow-up) Fix default syspref values
> 4) Attempt to checkout out an item with materials via SCO, it should > fail Turns out it worked >_< Another try: 1. CircConfirmItemParts: don't require CircConfirmItemPartsSCI: allow CircConfirmItemPartsSCO: Don't allow 2. have an item with "3 - Materials specified (bound volume or other part)" filled 3. go to http://localhost:8080/cgi-bin/koha/sco/sco-main.pl 4. try to check out the item 5. it works! :o After more testing, it needs only CircConfirmItemParts: "Require" to finally get "Item must be checked out at a circulation desk" Looked at the code, no idea why delete $needconfirm->{ADDITIONAL_MATERIALS} unless C4::Context->preference("CircConfirmItemPartsSCO"); wouldn't do it's work or would be circumvented by something else. After it's fixed, I think it should be back to need signoff. We can see that there were enough changes between the iterations so that basic workflow was compromised. Otherwise it would make QA be the only up to date test. --- Also, any sponsor for this change?