The AV added as part of bug 28854 can be duplicated if the update is run more than once.
Created attachment 156047 [details] [review] Bug 34881: Improve idempotency of DBRev
I think this part + $dbh->do(qq{ + INSERT IGNORE INTO systempreferences( `variable`, `value`, `options`, `explanation`, `type` ) + VALUES + ( 'BundleLostValue', $lost_val, '', 'Sets the LOST AV value that represents "Missing from bundle" as a lost value', 'Free' ), + ( 'BundleNotLoanValue', $nfl_val, '', 'Sets the NOT_LOAN AV value that represents "Added to bundle" as a not for loan value', 'Free') + }); + + say $out "Added new system preference 'BundleLostValue'"; + say $out "Added new system preference 'BundleNotLoanValue'"; should not be in the if you added.
It can't not be.. it depends on a variable added within the IF
Just checking on what is the best way to test this. I tried these steps, but not sure if this is the best way: 1. Check the authorized values list for LOST and note that there is this value: 5 Missing from bundle 2. Run the database update (found this on another idempotent bug): perl -e 'use C4::Installer; warn Data::Dumper::Dumper(C4::Installer::run_db_rev("installer/data/mysql/db_revs/220600015.pl"));' 3. Note that for the LOST authorized value lists, the value is duplicated: 6 Missing from bundle 4. Reset everything: reset_all 5. Run the database update again (see step 2). This time I got this error when running the update (at least the value isn't duplicated!): perl -e 'use C4::Installer; warn Data::Dumper::Dumper(C4::Installer::run_db_rev("installer/data/mysql/db_revs/220600015.pl"));' $VAR1 = { 'error' => bless( { 'msg' => '{UNKNOWN}: DBI Exception: DBD::mysql::db selectrow_array failed: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near \'\' at line 1 at /kohadevbox/koha/C4/Installer.pm line 741 ' }, 'DBIx::Class::Exception' ), 'time' => '18:19:10', 'exec_output' => 'Added new table \'item_bundles\' ', 'bug_number' => '28854', 'output' => [ 'Upgrade to 22.06.00.015 [18:19:10]: Bug 28854 - Item bundles support', ' Added new table \'item_bundles\'' ], 'description' => 'Item bundles support', 'version' => '22.06.00.015', 'filepath' => 'installer/data/mysql/db_revs/220600015.pl'
The subquery in line 24 of the patch is missing its close parenthesis at the end - that's probably what's causing the error. I agree that it feels off to put the system preference revisions (and the not-for-loan AV revision for that matter) all inside of the IF statement that depends only on the Lost AV. Can we safely assume that either the whole dbrev has been run, or none of it has been run? If so, why not put everything inside the if block that's already there: 10 if( !TableExists( 'item_bundles' ) ) { If not, then shouldn't we check each thing separately? Uploading an alternative patch for how this could work...
Created attachment 156823 [details] [review] Bug 34881: [alternative patch] Improve idempotency of DBRev
Created attachment 156878 [details] [review] Bug 34881: [alternative patch] Improve idempotency of DBRev Signed-off-by: David Nind <david@davidnind.com>
Thanks for the alternative patch Emily! I've signed off as I don't get an error when running the database update. If it shouldn't be signed off yet, please change the status back to needs sign-off. Running the database update - first time ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Following the test plan in #c4 I no longer get an error message when I run the database update for the first time: perl -e 'use C4::Installer; warn Data::Dumper::Dumper(C4::Installer::run_db_rev("installer/data/mysql/db_revs/220600015.pl"));' $VAR1 = { 'output' => [ 'Upgrade to 22.06.00.015 [20:11:52]: Bug 28854 - Item bundles support', ' Added new table \'item_bundles\'', ' Added new system preference \'BundleLostValue\'', ' Added new system preference \'BundleNotLoanValue\'' ], 'filepath' => 'installer/data/mysql/db_revs/220600015.pl', 'version' => '22.06.00.015', 'exec_output' => 'Added new table \'item_bundles\' Added new system preference \'BundleLostValue\' Added new system preference \'BundleNotLoanValue\' ', 'description' => 'Item bundles support', 'bug_number' => '28854', 'error' => undef, 'time' => '20:11:52' Running the database update - second time ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ perl -e 'use C4::Installer; warn Data::Dumper::Dumper(C4::Installer::run_db_rev("installer/data/mysql/db_revs/220600015.pl"));' $VAR1 = { 'output' => [ 'Upgrade to 22.06.00.015 [20:11:52]: Bug 28854 - Item bundles support', ' Added new table \'item_bundles\'', ' Added new system preference \'BundleLostValue\'', ' Added new system preference \'BundleNotLoanValue\'' ], 'filepath' => 'installer/data/mysql/db_revs/220600015.pl', 'version' => '22.06.00.015', 'exec_output' => 'Added new table \'item_bundles\' Added new system preference \'BundleLostValue\' Added new system preference \'BundleNotLoanValue\' ', 'description' => 'Item bundles support', 'bug_number' => '28854', 'error' => undef, 'time' => '20:11:52' }; Running the database update - third and subsequent times ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ perl -e 'use C4::Installer; warn Data::Dumper::Dumper(C4::Installer::run_db_rev("installer/data/mysql/db_revs/220600015.pl"));' $VAR1 = { 'bug_number' => '28854', 'exec_output' => 'Added new table \'item_bundles\' Added new system preference \'BundleLostValue\' Added new system preference \'BundleNotLoanValue\' ', 'filepath' => 'installer/data/mysql/db_revs/220600015.pl', 'error' => undef, 'version' => '22.06.00.015', 'time' => '20:15:11', 'output' => [ 'Upgrade to 22.06.00.015 [20:15:11]: Bug 28854 - Item bundles support', ' Added new table \'item_bundles\'', ' Added new system preference \'BundleLostValue\'', ' Added new system preference \'BundleNotLoanValue\'' ], 'description' => 'Item bundles support' };
Created attachment 156978 [details] [review] Bug 34881: Improve idempotency of DBRev Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 156979 [details] [review] Bug 34881: (QA follow-up) Remove stray paren Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Pushed to master for 23.11. Nice work everyone, thanks!
Pushed to 23.05.x for 23.05.05
Nice work everyone! Pushed to oldstable for 22.11.x