I'm wondering if this is normal. The system preference AutoApprovePatronProfileSettings is in updatedatabase.pl and in patrons.pref, but not in sysprefs.sql (unless I'm not looking in the right place).
Also missing - EmailSMSSendDriverFromAddress - HidePersonalPatronDetailOnCirculation - ManaToken - OPACDetailQRCode - OverDriveAuthname - OverDriveWebsiteID - RecordStaffUserOnCheckout - SelfCheckInModuleUserID And not removed from the db after being moved out of sysprefs, also not sure if it's supposed to be like that or not. - OpacMoreSearches - OPACMySummaryNote This one is in sysprefs.sql but not in updatedatabase.pl - UpdateNotForLoanStatusOnCheckout
Feel free to close if this is completely irrelevant :)
For OpacMoreSearches and OPACMySummaryNote, I see they are removed in dbrevs, but I ran updatedatabase.pl on an old db and they are still there. This might be a 'me' problem, (i.e. I might not be doing something correctly) I don't know...
We should definitely add these to sysprefs.sql. I feel like we used to have an automated test or something in the QA script to catch these, but couldn't find it. Joubu, do you know maybe?
(In reply to Katrin Fischer from comment #5) > We should definitely add these to sysprefs.sql. Agreed. > I feel like we used to have an automated test or something in the QA script > to catch these, but couldn't find it. Joubu, do you know maybe? t/db_dependent/check_sysprefs.t It is only making sure sysprefs in sysprefs.sql are in the DB. Now that we are testing the upgrade DB process (from 19.11) we could have the other way around and catch those missing ones (ie. make sure that the sysprefs in the DB are also in sysprefs.sql)
Created attachment 160614 [details] [review] Bug 34979: Add unit test for comparing YAMl files with sysprefs.sql The unit tests will help to detect any discrepancies and missing entries in the YAML and sysprefs.sql files. To test: * Apply only this patch * prove -v t/db_dependent/check_sysprefs.t * It will fail. * Apply other patches * Run database update * Re-run test. It should pass now. * Recreate your database or reset_all with patches applied. * System preferences should be installed correctly. * Test should still pass.
Created attachment 160615 [details] [review] Bug 34979: Sort sysprefs.sql alphabetically This doesn't add or remove any entries, but only changes the sequence of entries to be alphabetic again, compared to: SELECT variable FROM systempreferences ORDER BY variable;
Created attachment 160616 [details] [review] Bug 34979: Fix mistakes in system preference files highlighted by unit test This fixes the mistakes the unit test helped to find: * Add missing system preferences to sysprefs.sql * Correct spelling for AutoUpdateSetsEmbedItemData * Correct capitalization, so that code and pref files match * Fix OverDrive sytem preferences, where explanation/options columns where switched, resulting in strange database entries
The mistake in OAI-PMH:AutoUpdateSetsEmbedItemData (Set vs. Sets) made the system preference non-functional, so updating this to a bug. Be gentle in review, tried my best with the unit test :)
Created attachment 160617 [details] [review] Bug 34979: Add unit test for comparing YAMl files with sysprefs.sql The unit tests will help to detect any discrepancies and missing entries in the YAML and sysprefs.sql files. To test: * Apply only this patch * prove -v t/db_dependent/check_sysprefs.t * It will fail. * Apply other patches * Run database update * Re-run test. It should pass now. * Recreate your database or reset_all with patches applied. * System preferences should be installed correctly. * Test should still pass. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 160618 [details] [review] Bug 34979: Sort sysprefs.sql alphabetically This doesn't add or remove any entries, but only changes the sequence of entries to be alphabetic again, compared to: SELECT variable FROM systempreferences ORDER BY variable; Signed-off-by: David Nind <david@davidnind.com>
Created attachment 160619 [details] [review] Bug 34979: Fix mistakes in system preference files highlighted by unit test This fixes the mistakes the unit test helped to find: * Add missing system preferences to sysprefs.sql * Correct spelling for AutoUpdateSetsEmbedItemData * Correct capitalization, so that code and pref files match * Fix OverDrive sytem preferences, where explanation/options columns where switched, resulting in strange database entries Signed-off-by: David Nind <david@davidnind.com>
I would suggest to only display the "Added" message if the syspref has been added. Can be achieved with: $dbh->do(q{INSERT IGNORE statement}) == 1 && say "Added system preference 'foo'";
Created attachment 160978 [details] [review] Bug 34979: Add unit test for comparing YAMl files with sysprefs.sql The unit tests will help to detect any discrepancies and missing entries in the YAML and sysprefs.sql files. To test: * Apply only this patch * prove -v t/db_dependent/check_sysprefs.t * It will fail. * Apply other patches * Run database update * Re-run test. It should pass now. * Recreate your database or reset_all with patches applied. * System preferences should be installed correctly. * Test should still pass. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 160979 [details] [review] Bug 34979: Sort sysprefs.sql alphabetically This doesn't add or remove any entries, but only changes the sequence of entries to be alphabetic again, compared to: SELECT variable FROM systempreferences ORDER BY variable; Signed-off-by: David Nind <david@davidnind.com>
Created attachment 160980 [details] [review] Bug 34979: Fix mistakes in system preference files highlighted by unit test This fixes the mistakes the unit test helped to find: * Add missing system preferences to sysprefs.sql * Correct spelling for AutoUpdateSetsEmbedItemData * Correct capitalization, so that code and pref files match * Fix OverDrive sytem preferences, where explanation/options columns where switched, resulting in strange database entries Signed-off-by: David Nind <david@davidnind.com>
Created attachment 160981 [details] [review] Bug 34979: (follow-up) Only print message when preference was added Implements Joubu's suggestion from comment#14 Test plan remains unchanged. You can run the database update a second time to see the difference.
I've tried to further improve the database update for the "UPDATE" statements, but I didn't manage to only create the output when something actually was changed. It's always returning 1, I guess because the database isn't case-sensive and the WHERE statement will always return something to update.
Created attachment 160997 [details] [review] Bug 34979: Add unit test for comparing YAMl files with sysprefs.sql The unit tests will help to detect any discrepancies and missing entries in the YAML and sysprefs.sql files. To test: * Apply only this patch * prove -v t/db_dependent/check_sysprefs.t * It will fail. * Apply other patches * Run database update * Re-run test. It should pass now. * Recreate your database or reset_all with patches applied. * System preferences should be installed correctly. * Test should still pass. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 160998 [details] [review] Bug 34979: Sort sysprefs.sql alphabetically This doesn't add or remove any entries, but only changes the sequence of entries to be alphabetic again, compared to: SELECT variable FROM systempreferences ORDER BY variable; Signed-off-by: David Nind <david@davidnind.com>
Created attachment 160999 [details] [review] Bug 34979: Fix mistakes in system preference files highlighted by unit test This fixes the mistakes the unit test helped to find: * Add missing system preferences to sysprefs.sql * Correct spelling for AutoUpdateSetsEmbedItemData * Correct capitalization, so that code and pref files match * Fix OverDrive sytem preferences, where explanation/options columns where switched, resulting in strange database entries Signed-off-by: David Nind <david@davidnind.com>
Created attachment 161000 [details] [review] Bug 34979: (follow-up) Only print message when preference was added Implements Joubu's suggestion from comment#14 and improves on it for the cases where we fix capitalization. Test plan remains unchanged. You can run the database update a second time to see the difference.
Figured something out :)
Created attachment 161227 [details] [review] Bug 34979: Add unit test for comparing YAMl files with sysprefs.sql The unit tests will help to detect any discrepancies and missing entries in the YAML and sysprefs.sql files. To test: * Apply only this patch * prove -v t/db_dependent/check_sysprefs.t * It will fail. * Apply other patches * Run database update * Re-run test. It should pass now. * Recreate your database or reset_all with patches applied. * System preferences should be installed correctly. * Test should still pass. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 161228 [details] [review] Bug 34979: Sort sysprefs.sql alphabetically This doesn't add or remove any entries, but only changes the sequence of entries to be alphabetic again, compared to: SELECT variable FROM systempreferences ORDER BY variable; Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 161229 [details] [review] Bug 34979: Fix mistakes in system preference files highlighted by unit test This fixes the mistakes the unit test helped to find: * Add missing system preferences to sysprefs.sql * Correct spelling for AutoUpdateSetsEmbedItemData * Correct capitalization, so that code and pref files match * Fix OverDrive sytem preferences, where explanation/options columns where switched, resulting in strange database entries Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 161230 [details] [review] Bug 34979: (follow-up) Only print message when preference was added Implements Joubu's suggestion from comment#14 and improves on it for the cases where we fix capitalization. Test plan remains unchanged. You can run the database update a second time to see the difference. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Here is another signoff :) (checked also last commit function) Also tested with an upgrade from 19.11.x: this fix still catches all (test doesn't complain) I couldn't spin a KTD older than 19.11, for some reason MySQL takes all RAM at startup and I have to kill it.
(In reply to Victor Grousset/tuxayo from comment #29) > I couldn't spin a KTD older than 19.11, for some reason MySQL takes all RAM > at startup and I have to kill it. oh, mysql doesn't work but mariadb does! Then it fail when trying to update package cpanoutdated Err:1 http://httpredir.debian.org/debian stretch/main amd64 cpanoutdated all 0.31-1 404 Not Found ok, at least that makes more sense
Created attachment 161418 [details] [review] Bug 34979: Add unit test for comparing YAMl files with sysprefs.sql The unit tests will help to detect any discrepancies and missing entries in the YAML and sysprefs.sql files. To test: * Apply only this patch * prove -v t/db_dependent/check_sysprefs.t * It will fail. * Apply other patches * Run database update * Re-run test. It should pass now. * Recreate your database or reset_all with patches applied. * System preferences should be installed correctly. * Test should still pass. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 161419 [details] [review] Bug 34979: Sort sysprefs.sql alphabetically This doesn't add or remove any entries, but only changes the sequence of entries to be alphabetic again, compared to: SELECT variable FROM systempreferences ORDER BY variable; Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 161420 [details] [review] Bug 34979: Fix mistakes in system preference files highlighted by unit test This fixes the mistakes the unit test helped to find: * Add missing system preferences to sysprefs.sql * Correct spelling for AutoUpdateSetsEmbedItemData * Correct capitalization, so that code and pref files match * Fix OverDrive sytem preferences, where explanation/options columns where switched, resulting in strange database entries Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 161421 [details] [review] Bug 34979: (follow-up) Only print message when preference was added Implements Joubu's suggestion from comment#14 and improves on it for the cases where we fix capitalization. Test plan remains unchanged. You can run the database update a second time to see the difference. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Nice fix, Passing QA
Pushed for 24.05! Well done everyone, thank you!
Pushed to 23.11.x for 23.11.03
Doesn't apply cleanly to 23.05.x, no backport. If needed please rebase.
Error from this upgrade Upgrade to 23.11.02.002 [19:05:56]: Bug 34979 - Fix system preference discrepancies Fix mistakes in system preferences, if necessary: ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Duplicate entry 'OAI-PMH:AutoUpdateSetsEmbedItemData' for key 'PRIMARY' at /usr/share/koha/lib/C4/Installer.pm line 741
Dito, upgrade from 23.11.02 fails.
(In reply to Hans Pålsson from comment #40) > Dito, upgrade from 23.11.02 fails. I am sorry for that. If you remove the pref from the database and then re-run the script, that should fix things. We are also working on a fix here: bug 36232