|
Description
Martin Renvoize (ashimema)
2018-10-26 10:50:19 UTC
Created attachment 81280 [details] [review] Bug 21682: Corrections for strict sql in stockrotationrotas Test Plan: 1) Ensure you're database is prior to version 18.06.00.040 2) Ensure 'strict_sql_modes' is enabled 3) Run updatedatabase 4) See no errors during upgrade 5) Run a fresh install 6) Note no errors during database installation As astutely pointed out by Jonathan... the fix appears to be simple, patch submitted. You should update existing [dev] installs as well. As the devs refresh the dbs quite frequently, shoudl this be blocker? (In reply to Katrin Fischer from comment #4) > As the devs refresh the dbs quite frequently, shoudl this be blocker? Either we need it or we write that somewhere in the guidelines :) Created attachment 81347 [details] [review] Bug 21683: (follow-up) - DBUpdate for Dev installs I'm not 100% sure on the idempotency of the atomicupdate.. I believe 'DROP DEFAULT' is idempotent in itself, but couldn't find any documentation either way and it's not trivial to check that a DEFAULT constraint exists before attempting to drop it... open to advise here. Created attachment 81348 [details] [review] Bug 21683: (follow-up) - DBUpdate for Dev installs Actually.. I do need help here.. When running in SQL strict mode I get the following for the atomic update. DBD::mysql::db do failed: BLOB/TEXT column 'description' can't have a default value [for Statement "ALTER TABLE `stockrotationrotas` ALTER COLUMN `description` DROP DEFAULT"] at (eval 1403) line 3. I think we need to check for sql strict mode before attempting to apply the update or not.. :( This was meant to be a simple fix :( Actually.. I get that error whether I enable strict modes or not.. I have no idea. This should be fixed on the DBRev, no point spending this much time for a simple fix. Specially if the engine rejects 'fixing' the DB structure. Created attachment 81525 [details] [review] Bug 21682: Prequel addition of default_exists prove t/db_dependent/Installer.t perldoc C4::Installer -- There should be something for default_exists Created attachment 81526 [details] [review] Bug 21682: Prequel addition of default_exists prove t/db_dependent/Installer.t perldoc C4::Installer -- There should be something for default_exists Created attachment 81527 [details] [review] Bug 21682: Corrections for strict sql in stockrotationrotas Test Plan: 1) Ensure you're database is prior to version 18.06.00.040 2) Ensure 'strict_sql_modes' is enabled 3) Run updatedatabase 4) See no errors during upgrade 5) Run a fresh install 6) Note no errors during database installation Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 81528 [details] [review] Bug 21682: (follow-up) - DBUpdate for Dev installs https://bugs.koha-community.org/show_bug.cgi?id=21682 Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 81529 [details] [review] Bug 21682: Follow up to use the default_exists check This will make sure it is idempotent, and avoid issues if the DB has already been corrected either through upgrade or manually before. Wow.. thanks M.Tompsett Why do not you use the same statement we always use? :) ALTER TABLE stockrotationrotas CHANGE COLUMN description description text; I wasn't aware that was a statement 'we always use' Created attachment 81769 [details] [review] Bug 21682: Corrections for strict sql in stockrotationrotas Test Plan: 1) Ensure you're database is prior to version 18.06.00.040 2) Ensure 'strict_sql_modes' is enabled 3) Run updatedatabase 4) See no errors during upgrade 5) Run a fresh install 6) Note no errors during database installation Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 81770 [details] [review] Bug 21682: Add update DB entry for existing installs I removed the need of the new subroutine, could you please test this version? Created attachment 81787 [details] [review] Bug 21682: Corrections for strict sql in stockrotationrotas Test Plan: 1) Ensure you're database is prior to version 18.06.00.040 2) Ensure 'strict_sql_modes' is enabled 3) Run updatedatabase 4) See no errors during upgrade 5) Run a fresh install 6) Note no errors during database installation Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 81788 [details] [review] Bug 21682: Add update DB entry for existing installs Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Works for me Created attachment 81815 [details] [review] Bug 21682: Add update DB entry for existing installs Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> I amended the last patch to move atomicupdate/bug_21620.perl to atomicupdate/bug_21682.perl Awesome work all! Pushed to master for 18.11 Bug 11897 not in 18.05.x series. (In reply to Jonathan Druart from comment #26) > Created attachment 81815 [details] [review] [review] > Bug 21682: Add update DB entry for existing installs > > Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> This patch removed the NOT NULL from the title field at upgrade time. WIll open a report for it. (In reply to Marcel de Rooy from comment #30) > (In reply to Jonathan Druart from comment #26) > > Created attachment 81815 [details] [review] [review] [review] > > Bug 21682: Add update DB entry for existing installs > > > > Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> > > This patch removed the NOT NULL from the title field at upgrade time. > WIll open a report for it. Description field |