Created attachment 167376 [details] [review]
Bug 37000: [Bug 36120 follow-up] Improve reliability of database update

This patch adds a series of fallthroughs to ensure pickup_library_id is
always set prior to adding the NOT NULL constraint.

We initially only looked at items.homebranch but as that's a nullable
field itself, we now look at items.holdingbranch before finally
defaulting to the first available branch in the branches table in the
worst case.

Created attachment 167563 [details] [review]
Bug 37000: [Bug 36120 follow-up] Improve reliability of database update

This patch adds a series of fallthroughs to ensure pickup_library_id is
always set prior to adding the NOT NULL constraint.

We initially only looked at items.homebranch but as that's a nullable
field itself, we now look at items.holdingbranch before finally
defaulting to the first available branch in the branches table in the
worst case.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Created attachment 167573 [details] [review]
Bug 37000: (Bug 36120 follow-up) Improve reliability of database update

This patch adds a series of fallthroughs to ensure pickup_library_id is
always set prior to adding the NOT NULL constraint.

We initially only looked at items.homebranch but as that's a nullable
field itself, we now look at items.holdingbranch before finally
defaulting to the first available branch in the branches table in the
worst case.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Sorry, this doesn't completely fix the issue for production environments using MySQL. If strict_mode is off, MySQL won't allow you to change a nullable column to non-nullable if the column has a foreign key, even if there are no existing null values in the data. (see https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35687#c13)

I was able to reproduce the error, even with this patch applied, with the following steps:
1. Shut down KTD
2. Switch to 23.11.x
3. Start up KTD with MySQL-8 (I did this using the built-in alias ku-my8)
4. Inside a kshell, edit /etc/koha/sites/kohadev/koha-conf.xml. Set <strict-sql-modes> to 0.
5. Switch to main and apply patch
6. restart_all
7. updatedatabase
--> observe the original error

Can the foreign key be added at the end of the dbrev, after the NOT NULL constraint is set, instead of the beginning? items.homebranch and items.holdingbranch already have a FK referencing branches.branchcode themselves, so it should be safe to fill in those values and add the new FK after.

Incidentally, if I ran the database update a second time immediately after the first, the database appeared to successfully complete the update to 24.05; however, this dbrev silently did nothing because the pickup_library_id column already exists, and the foreign key never got added.

Will upload a follow-up patch in a bit that I believe will address both of these issues...

Created attachment 167589 [details] [review]
Bug 37000: (follow-up) Add foreign key last

Certain configurations of MySQL will not allow a column to be changed
from nullable to non-nullable if the column has a foreign key constraint.
Add the foreign key constraint last to avoid issues from this.

Hope I'm not overstepping here...I felt that submitting code was clearer than my comment was on its own, and I didn't want to stall the bug at FQA. Setting back to SO to give Martin and Julian a chance to review.

Looks great to me, thanks.

Thanks for all the hard work!

Pushed to main for the next 24.11.00 release as RM Assistant

Backported to 23.05.x for upcoming 24.05.02

Depends on Bug 36120 not in 23.11.x

No updates to the manual required - upgrade errors are not covered.