Bug 34643 - Split CircConfirmItemParts for self-checkout and self-checkin
Summary: Split CircConfirmItemParts for self-checkout and self-checkin
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Self checkout (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Victor Grousset/tuxayo
URL:
Keywords:
: 33700 (view as bug list)
Depends on:
Blocks:
 
Reported: 2023-08-29 16:07 UTC by Kyle M Hall
Modified: 2024-03-23 03:54 UTC (History)
3 users (show)

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


Attachments
Bug 34643: Split CircConfirmItemParts for self-checkout and self-checkin (5.27 KB, patch)
2023-08-29 16:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 34643: Split CircConfirmItemParts for self-checkout and self-checkin (5.38 KB, patch)
2023-11-17 21:33 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 34643: Split CircConfirmItemParts for self-checkout and self-checkin (5.41 KB, patch)
2024-03-14 19:16 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 34643: (QA follow-up) Add new sysprefs to sysprefs.sql (1.81 KB, patch)
2024-03-18 12:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 34643: (QA follow-up): fix order of new sysprefs and quotation (2.14 KB, patch)
2024-03-22 00:45 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 34643: (QA follow-up) Fix default syspref values (1.95 KB, patch)
2024-03-22 10:24 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2023-08-29 16:07:05 UTC
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.
Comment 1 Kyle M Hall 2023-08-29 16:52:24 UTC
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!
Comment 2 Kyle M Hall 2023-08-29 16:52:37 UTC
*** Bug 33700 has been marked as a duplicate of this bug. ***
Comment 3 Andrew Fuerste-Henry 2023-11-17 16:31:25 UTC
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.
Comment 4 Andrew Fuerste-Henry 2023-11-17 21:33:49 UTC
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>
Comment 5 Andrew Fuerste-Henry 2023-11-17 21:34:42 UTC
I made those wording changes as part of signing off.
Comment 6 Victor Grousset/tuxayo 2024-03-14 19:16:06 UTC
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>
Comment 7 Victor Grousset/tuxayo 2024-03-14 19:18:46 UTC
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.
Comment 8 Victor Grousset/tuxayo 2024-03-14 19:23:29 UTC
(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
Comment 9 Kyle M Hall 2024-03-18 12:20:06 UTC
Created attachment 163318 [details] [review]
Bug 34643: (QA follow-up) Add new sysprefs to sysprefs.sql
Comment 10 Victor Grousset/tuxayo 2024-03-22 00:45:38 UTC
Created attachment 163657 [details] [review]
Bug 34643: (QA follow-up): fix order of new sysprefs and quotation
Comment 11 Victor Grousset/tuxayo 2024-03-22 04:05:54 UTC
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.
Comment 12 Kyle M Hall 2024-03-22 10:24:34 UTC
Created attachment 163675 [details] [review]
Bug 34643: (QA follow-up) Fix default syspref values
Comment 13 Kyle M Hall 2024-03-22 10:25:46 UTC
(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!
Comment 14 Victor Grousset/tuxayo 2024-03-23 03:54:46 UTC
Ain't no way... SCI and SCO are broken due to not complying with CSRF. Reviewing this patch is endless 😭

Great, it's a known bug and it's active: Bug 36349 (OMG it's a mess 😭)