Holds require a branchcode, various code expects this, however, we do not enforce this requirement when saving holds
Created attachment 136983 [details] [review] Bug 31086: DB update This update sets branchcode as NOT NULL default set to 0 as borrowernumber is
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
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
Created attachment 136987 [details] [review] Bug 31086: DO NOT PUSH: Schema updates
Created attachment 137026 [details] [review] Bug 31086: Update existing unit tests
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>
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>
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>
Created attachment 137257 [details] [review] Bug 31086: DO NOT PUSH: Schema updates Signed-off-by: David Nind <david@davidnind.com>
Created attachment 137258 [details] [review] Bug 31086: Update existing unit tests Signed-off-by: David Nind <david@davidnind.com>
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.
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>
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>
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>
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>
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>
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>
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>
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>
All good now with the minor follow-ups.. Passing QA
- 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.
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
(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
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
Created attachment 138018 [details] [review] Bug 31086: Notify of problematic holds during upgrade
I like nicks addition to my follow-up.. best of all worlds
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>
Pushed to master for 22.11. Nice work everyone, thanks!
This patchset won't apply cleanly to 22.05.x. If needed, please rebase.
(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
Thanks! Pushed to 21.11 for 21.11.12
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed. Nothing to document it seems, marking resolved.
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
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
I don't understand how running the exact same SQL as is in the update itself can resolve this problem.
(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.
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>
Not backported to 21.05.x