Bug 31086 - Do not allow hold requests with no branchcode
Summary: Do not allow hold requests with no branchcode
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 31673
  Show dependency treegraph
 
Reported: 2022-07-01 14:38 UTC by Nick Clemens
Modified: 2023-12-28 20:42 UTC (History)
8 users (show)

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


Attachments
Bug 31086: DB update (2.74 KB, patch)
2022-07-01 14:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31086: Throw an exception when a hold is stored with no branchcode (1.68 KB, patch)
2022-07-01 14:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31086: Prevent placing hold with no pickup location (1.44 KB, patch)
2022-07-01 14:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31086: DO NOT PUSH: Schema updates (1.98 KB, patch)
2022-07-01 14:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31086: Update existing unit tests (2.82 KB, patch)
2022-07-01 15:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31086: DB update (2.78 KB, patch)
2022-07-07 10:36 UTC, David Nind
Details | Diff | Splinter Review
Bug 31086: Throw an exception when a hold is stored with no branchcode (1.73 KB, patch)
2022-07-07 10:36 UTC, David Nind
Details | Diff | Splinter Review
Bug 31086: Prevent placing hold with no pickup location (1.48 KB, patch)
2022-07-07 10:36 UTC, David Nind
Details | Diff | Splinter Review
Bug 31086: DO NOT PUSH: Schema updates (2.03 KB, patch)
2022-07-07 10:36 UTC, David Nind
Details | Diff | Splinter Review
Bug 31086: Update existing unit tests (2.87 KB, patch)
2022-07-07 10:36 UTC, David Nind
Details | Diff | Splinter Review
Bug 31086: DB update (2.85 KB, patch)
2022-07-12 15:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31086: Throw an exception when a hold is stored with no branchcode (1.79 KB, patch)
2022-07-12 15:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31086: Prevent placing hold with no pickup location (1.55 KB, patch)
2022-07-12 15:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31086: DO NOT PUSH: Schema updates (2.09 KB, patch)
2022-07-12 15:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31086: Update existing unit tests (2.93 KB, patch)
2022-07-12 15:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31086: (QA follow-up) Remove DEFAULT from column (2.82 KB, patch)
2022-07-12 15:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31086: (QA follow-up) Fix failing tests (2.22 KB, patch)
2022-07-12 15:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31086: (QA follow-up) Add unit tests (1.97 KB, patch)
2022-07-12 15:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31086: (QA follow-up) Improve atomicupdate to be more resilient (1.64 KB, patch)
2022-07-22 09:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31086: Notify of problematic holds during upgrade (1.80 KB, patch)
2022-07-22 12:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31086: (QA follow-up) Use plain SQL in db_rev (2.00 KB, patch)
2022-07-22 17:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 31086: (follow-up) There's no ->fill in 21.11 (8.21 KB, patch)
2022-12-23 14:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2022-07-01 14:38:35 UTC
Holds require a branchcode, various code expects this, however, we do not enforce this requirement when saving holds
Comment 1 Nick Clemens 2022-07-01 14:53:21 UTC
Created attachment 136983 [details] [review]
Bug 31086: DB update

This update sets branchcode as NOT NULL
default set to 0 as borrowernumber is
Comment 2 Nick Clemens 2022-07-01 14:53:24 UTC
Created attachment 136984 [details] [review]
Bug 31086: Throw an exception when a hold is stored with no branchcode

To test:
1 - Apply first 2 patches
2 - On staff client, find a biblio, place hold, select a patron
3 - Before creating hold, right click on 'Pickup location'
4 - Inspect the element
5 - In the console find the top <li> for pickup, and right click
6 - Select 'Delete node'
7 - Save hold
8 - You get an exception/error
Comment 3 Nick Clemens 2022-07-01 14:53:28 UTC
Created attachment 136986 [details] [review]
Bug 31086: Prevent placing hold with no pickup location

To test:
1 - Apply patch
2 - Repeat test on previous patch
3 - You should get a warning popup when attempting to save hold
4 - Reload page
5 - Inspect the HTML and manualy set #pickup value of selected option to ""
6 - Attempt to save hold
7 - You get the popup
8 - Place a hold normally (with location) and verify it works
Comment 4 Nick Clemens 2022-07-01 14:53:31 UTC
Created attachment 136987 [details] [review]
Bug 31086: DO NOT PUSH: Schema updates
Comment 5 Nick Clemens 2022-07-01 15:31:51 UTC
Created attachment 137026 [details] [review]
Bug 31086: Update existing unit tests
Comment 6 David Nind 2022-07-07 10:36:33 UTC
Created attachment 137254 [details] [review]
Bug 31086: DB update

This update sets branchcode as NOT NULL
default set to 0 as borrowernumber is

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2022-07-07 10:36:38 UTC
Created attachment 137255 [details] [review]
Bug 31086: Throw an exception when a hold is stored with no branchcode

To test:
1 - Apply first 2 patches
2 - On staff client, find a biblio, place hold, select a patron
3 - Before creating hold, right click on 'Pickup location'
4 - Inspect the element
5 - In the console find the top <li> for pickup, and right click
6 - Select 'Delete node'
7 - Save hold
8 - You get an exception/error

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2022-07-07 10:36:42 UTC
Created attachment 137256 [details] [review]
Bug 31086: Prevent placing hold with no pickup location

To test:
1 - Apply patch
2 - Repeat test on previous patch
3 - You should get a warning popup when attempting to save hold
4 - Reload page
5 - Inspect the HTML and manualy set #pickup value of selected option to ""
6 - Attempt to save hold
7 - You get the popup
8 - Place a hold normally (with location) and verify it works

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2022-07-07 10:36:48 UTC
Created attachment 137257 [details] [review]
Bug 31086: DO NOT PUSH: Schema updates

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2022-07-07 10:36:52 UTC
Created attachment 137258 [details] [review]
Bug 31086: Update existing unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 11 Martin Renvoize 2022-07-12 14:32:08 UTC
Comment on attachment 137254 [details] [review]
Bug 31086: DB update

Review of attachment 137254 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/kohastructure.sql
@@ +4408,4 @@
>    `borrowernumber` int(11) NOT NULL DEFAULT 0 COMMENT 'foreign key from the borrowers table defining which patron this hold is for',
>    `reservedate` date DEFAULT NULL COMMENT 'the date the hold was placed',
>    `biblionumber` int(11) NOT NULL DEFAULT 0 COMMENT 'foreign key from the biblio table defining which bib record this hold is on',
> +  `branchcode` varchar(10) COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT 0 COMMENT 'foreign key from the branches table defining which branch the patron wishes to pick this hold up at',

Why DEFAULT 0?  We're throwing an exception if we don't pass a branchcode in the app... why default to a probably incorrect branch id in SQL.. to me I'd just drop the DEFAULT entirely and force anyone writing SQL to get thrown an error when they don't include it.
Comment 12 Martin Renvoize 2022-07-12 15:24:42 UTC
Created attachment 137635 [details] [review]
Bug 31086: DB update

This update sets branchcode as NOT NULL
default set to 0 as borrowernumber is

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2022-07-12 15:24:46 UTC
Created attachment 137636 [details] [review]
Bug 31086: Throw an exception when a hold is stored with no branchcode

To test:
1 - Apply first 2 patches
2 - On staff client, find a biblio, place hold, select a patron
3 - Before creating hold, right click on 'Pickup location'
4 - Inspect the element
5 - In the console find the top <li> for pickup, and right click
6 - Select 'Delete node'
7 - Save hold
8 - You get an exception/error

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2022-07-12 15:24:52 UTC
Created attachment 137637 [details] [review]
Bug 31086: Prevent placing hold with no pickup location

To test:
1 - Apply patch
2 - Repeat test on previous patch
3 - You should get a warning popup when attempting to save hold
4 - Reload page
5 - Inspect the HTML and manualy set #pickup value of selected option to ""
6 - Attempt to save hold
7 - You get the popup
8 - Place a hold normally (with location) and verify it works

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2022-07-12 15:24:56 UTC
Created attachment 137638 [details] [review]
Bug 31086: DO NOT PUSH: Schema updates

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2022-07-12 15:25:00 UTC
Created attachment 137639 [details] [review]
Bug 31086: Update existing unit tests

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2022-07-12 15:25:06 UTC
Created attachment 137640 [details] [review]
Bug 31086: (QA follow-up) Remove DEFAULT from column

There's no need to have a default here, and in fact it doesn't make
sense.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2022-07-12 15:25:11 UTC
Created attachment 137641 [details] [review]
Bug 31086: (QA follow-up) Fix failing tests

This test was missed first time around, branchcode needed adding to all
the calls to AddReserve.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2022-07-12 15:25:18 UTC
Created attachment 137642 [details] [review]
Bug 31086: (QA follow-up) Add unit tests

This patch adds the unit tests for the change to Koha::Hold::store.

We test for and catch the exception introduced for both the create and
update cases.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2022-07-12 15:26:19 UTC
All good now with the minor follow-ups.. Passing QA
Comment 21 Tomás Cohen Arazi 2022-07-18 22:08:13 UTC
- It feels weird that we add a DB-level constraint on the column, but we manually check it in ->store to raise the exception. I see how we got there by the bug history. Thoughts? Shouldn't we catch the DB-level exception and throw our own? Not a blocker, though.
- In cases like this, if the atomicupdate fails, it would be nice to tell how to fix things.
Comment 22 Martin Renvoize 2022-07-19 05:38:29 UTC
Good points raised there Tomas.. I was perhaps a little hasty in PQA.  We could do better; as you highlighted we could be catching the constraint failure rather than testing before hitting the database.. never sure which approach is actually cleaner/better for performance.. it would be interesting to benchmark that.

However, the DB update side is something I think we probably should try to improve by making it a little more resilient.. we could look for nulls prior to adding the constraint and arbitrarily pick the first branchcode in the branches table to fix the data should we find any nulls?

Our database updates are better now in terms of not silently ignoring issues, but I do think we as Devs aught to try and cater for more of these cases to keep the upgrades fairly smooth.

I'm happy to add that proposed follow-up
Comment 23 Tomás Cohen Arazi 2022-07-20 12:59:03 UTC
(In reply to Martin Renvoize from comment #22)
> Our database updates are better now in terms of not silently ignoring
> issues, but I do think we as Devs aught to try and cater for more of these
> cases to keep the upgrades fairly smooth.
> 
> I'm happy to add that proposed follow-up

+1
Comment 24 Martin Renvoize 2022-07-22 09:07:18 UTC
Created attachment 138006 [details] [review]
Bug 31086: (QA follow-up) Improve atomicupdate to be more resilient

We now populate the branchcode field with the first available branch on
the system if we find it to be NULL before we set the NOT NULL
Comment 25 Nick Clemens 2022-07-22 12:32:16 UTC
Created attachment 138018 [details] [review]
Bug 31086: Notify of problematic holds during upgrade
Comment 26 Martin Renvoize 2022-07-22 12:37:03 UTC
I like nicks addition to my follow-up.. best of all worlds
Comment 27 Tomás Cohen Arazi 2022-07-22 17:38:17 UTC
Created attachment 138051 [details] [review]
Bug 31086: (QA follow-up) Use plain SQL in db_rev

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Tomás Cohen Arazi 2022-07-22 17:52:26 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 29 Lucas Gass 2022-09-02 15:15:38 UTC
This patchset won't apply cleanly to 22.05.x. If needed, please rebase.
Comment 30 Lucas Gass 2022-09-02 17:30:33 UTC
(In reply to Lucas Gass from comment #29)
> This patchset won't apply cleanly to 22.05.x. If needed, please rebase.

Nevermind, this actually applied cleanly. Backported to 22.05.x for 22.05.05
Comment 31 Arthur Suzuki 2022-09-14 12:58:56 UTC
Thanks!

Pushed to 21.11 for 21.11.12
Comment 32 Victor Grousset/tuxayo 2022-09-19 21:59:36 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document it seems, marking resolved.
Comment 33 José Anjos 2022-10-03 09:28:10 UTC
I think this is related:

Updating database structure

Update errors :

    Upgrade to 22.05.04.002 [10:09:24]: Bug 31086 - Do not allow null values in branchcodes for reserves
    ERROR: {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Cannot change column 'branchcode': used in a foreign key constraint 'reserves_ibfk_4' at /usr/share/koha/lib/C4/Installer.pm line 739
Comment 34 José Anjos 2022-10-03 09:46:55 UTC
After manually doing this command:
ALTER TABLE reserves
MODIFY COLUMN `branchcode` varchar(10) COLLATE utf8mb4_unicode_ci NOT NULL COMMENT 'foreign key from the branches table defining which branch the patron wishes to pick this hold up at';
Query OK, 0 rows affected (0.06 sec)
Records: 0  Duplicates: 0  Warnings: 0

it was possible to continue with the upgrade...
Strange
Comment 35 Martin Renvoize 2022-10-05 07:30:47 UTC
I don't understand how running the exact same SQL as is in the update itself can resolve this problem.
Comment 36 Katrin Fischer 2022-10-24 22:01:49 UTC
(In reply to Martin Renvoize from comment #35)
> I don't understand how running the exact same SQL as is in the update itself
> can resolve this problem.

Fixed on bug 31673.
Comment 37 Tomás Cohen Arazi 2022-12-23 14:44:56 UTC
Created attachment 144824 [details] [review]
Bug 31086: (follow-up) There's no ->fill in 21.11

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 38 wainuiwitikapark 2023-03-15 01:23:14 UTC
Not backported to 21.05.x