| Summary: | Syspref discrepancies between new and upgraded installs | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Jonathan Druart <jonathan.druart> |
| Component: | Test Suite | Assignee: | Jonathan Druart <jonathan.druart> |
| Status: | Pushed to main --- | QA Contact: | Tomás Cohen Arazi (tcohen) <tomascohen> |
| Severity: | major | ||
| Priority: | P5 - low | CC: | david, lucas, lucasmontoya, martin.renvoize, nick, tomascohen |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| See Also: |
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41663 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41460 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41710 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39745 |
||
| GIT URL: | Initiative type: | --- | |
| Sponsorship status: | --- | Comma delimited list of Sponsors: | |
| Crowdfunding goal: | 0 | Patch complexity: | Small patch |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: |
This fixes several system preferences discrepancies and adds tests, including:
* Setting options to NULL when options=""
* Fixing the explanation when different
* Fixing the wrong order (some rows had options=explanation)
* Fixing the wrong type "Yes/No" => "YesNo"
* Removed StaffLoginBranchBasedOnIP: both StaffLoginLibraryBasedOnIP and StaffLoginBranchBasedOnIP are in the database for upgraded installs
* Adding a description for ApiKeyLog
* Fixing 'integer' vs. 'Integer' inconsistency
* Fixing 'cancelation' typo
* Improving the tests:
* Compare sysprefs.sql and the database content for options, explanation and type
* Catch type not defined
* Catch incorrect YesNo values (must be 0 or 1)
An example where discrepancies have crept in during upgrades includes warnings in the About Koha > System information - the system preferences had no value (either 1 or 0) in the 'value' field:
* Warning System preference 'ILLHistoryCheck' must be '0' or '1', but is ''.
* Warning System preference 'ILLOpacUnauthenticatedRequest' must be '0' or '1', but is ''.
* Warning System preference 'SeparateHoldingsByGroup' must be '0' or '1', but is ''.
|
Version(s) released in: |
26.05.00
|
| Circulation function: | |||
| Attachments: |
Bug 41682: Add tests
Bug 41682: Sync DB and sysprefs.sql for new and upgraded installs Bug 41682: Sync DB and sysprefs.sql for new and upgraded installs Bug 41682: Add tests Bug 41682: Sync DB and sysprefs.sql for new and upgraded installs Bug 41682: Add tests Bug 41682: Sync DB and sysprefs.sql for new and upgraded installs Bug 41682: Add ApiKeyLog description Bug 41682: Fix 'integer' vs. 'Integer' inconsistency Bug 41682: Fix 'cancelation' typo Bug 41682: Ignore 'FrameworksLoaded' Bug 41682: Ensure that sysprefs.sql is kept ordered by syspref's name Bug 41682: More fixes |
||
|
Description
Jonathan Druart
2026-01-22 09:45:23 UTC
Created attachment 191858 [details] [review] Bug 41682: Add tests Patch from commit e4023c2 Created attachment 191859 [details] [review] Bug 41682: Sync DB and sysprefs.sql for new and upgraded installs This patch removes the discrepancies and put in sync the systempreferences table for new and upgraded installations. Main changes: * Set options to NULL when options="" * Fix explanation when different * Fix wrong order (some rows had options=explanation) * Fix wrong type "Yes/No" => "YesNo" * Remove StaffLoginBranchBasedOnIP: both StaffLoginLibraryBasedOnIP and StaffLoginBranchBasedOnIP are in the DB for upgraded installs * Remove the 'showsql' from the test. If we need it back then it must be a maintenance script. * Improve the tests: * Compare sysprefs.sql and the DB content for options, explanation and type * Catch type not defined * Catch incorrect YesNo values (must be 0 or 1) Later: * Add a C4::Installer sub to add new syspref: add_pref( { variable => $variable, value => $value, options => $options, explanation => $explanation, type => $type } ); * Clean 'type' and make it an ENUM * Make it easy to run the test for devs (should be in xt but we need it in t/db_dependent for packages - FIXME?) * Run the tests on upgraded installs on CI Test plan: 0. Apply the "tests" patch only 1. prove it => It fails loudly 2. Apply the DB change and run updatedatabase, prove the tests => they pass 3. Test upgraded installs: * checkout v24.05.00 (the version we use for Jenkins' Koha_Main_MariaDB_update job) * reset_all * checkout the branch with this patchset * updatabase * prove t/db_dependent/check_sysprefs.t => Should be green The second patch doesn't apply for me: git bz apply 41682 Bug 41682 - Syspref discrepancies between new and upgraded installs 191858 - Bug 41682: Add tests 191859 - Bug 41682: Sync DB and sysprefs.sql for new and upgraded installs Apply? [(y)es, (n)o, (i)nteractive] i Applying: Bug 41682: Sync DB and sysprefs.sql for new and upgraded installs Using index info to reconstruct a base tree... M installer/data/mysql/mandatory/sysprefs.sql Falling back to patching base and 3-way merge... Auto-merging installer/data/mysql/mandatory/sysprefs.sql CONFLICT (content): Merge conflict in installer/data/mysql/mandatory/sysprefs.sql error: Failed to merge in the changes. Patch failed at 0001 Bug 41682: Sync DB and sysprefs.sql for new and upgraded installs Created attachment 191906 [details] [review] Bug 41682: Sync DB and sysprefs.sql for new and upgraded installs This patch removes the discrepancies and put in sync the systempreferences table for new and upgraded installations. Main changes: * Set options to NULL when options="" * Fix explanation when different * Fix wrong order (some rows had options=explanation) * Fix wrong type "Yes/No" => "YesNo" * Remove StaffLoginBranchBasedOnIP: both StaffLoginLibraryBasedOnIP and StaffLoginBranchBasedOnIP are in the DB for upgraded installs * Remove the 'showsql' from the test. If we need it back then it must be a maintenance script. * Improve the tests: * Compare sysprefs.sql and the DB content for options, explanation and type * Catch type not defined * Catch incorrect YesNo values (must be 0 or 1) Later: * Add a C4::Installer sub to add new syspref: add_pref( { variable => $variable, value => $value, options => $options, explanation => $explanation, type => $type } ); * Clean 'type' and make it an ENUM * Make it easy to run the test for devs (should be in xt but we need it in t/db_dependent for packages - FIXME?) * Run the tests on upgraded installs on CI Test plan: 0. Apply the "tests" patch only 1. prove it => It fails loudly 2. Apply the DB change and run updatedatabase, prove the tests => they pass 3. Test upgraded installs: * checkout v24.05.00 (the version we use for Jenkins' Koha_Main_MariaDB_update job) * reset_all * checkout the branch with this patchset * updatabase * prove t/db_dependent/check_sysprefs.t => Should be green (In reply to David Nind from comment #3) > The second patch doesn't apply for me: Yes, that will be a nightmare to rebase, we should push ASAP. Created attachment 191931 [details] [review] Bug 41682: Add tests Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 191932 [details] [review] Bug 41682: Sync DB and sysprefs.sql for new and upgraded installs This patch removes the discrepancies and put in sync the systempreferences table for new and upgraded installations. Main changes: * Set options to NULL when options="" * Fix explanation when different * Fix wrong order (some rows had options=explanation) * Fix wrong type "Yes/No" => "YesNo" * Remove StaffLoginBranchBasedOnIP: both StaffLoginLibraryBasedOnIP and StaffLoginBranchBasedOnIP are in the DB for upgraded installs * Remove the 'showsql' from the test. If we need it back then it must be a maintenance script. * Improve the tests: * Compare sysprefs.sql and the DB content for options, explanation and type * Catch type not defined * Catch incorrect YesNo values (must be 0 or 1) Later: * Add a C4::Installer sub to add new syspref: add_pref( { variable => $variable, value => $value, options => $options, explanation => $explanation, type => $type } ); * Clean 'type' and make it an ENUM * Make it easy to run the test for devs (should be in xt but we need it in t/db_dependent for packages - FIXME?) * Run the tests on upgraded installs on CI Test plan: 0. Apply the "tests" patch only 1. prove it => It fails loudly 2. Apply the DB change and run updatedatabase, prove the tests => they pass 3. Test upgraded installs: * checkout v24.05.00 (the version we use for Jenkins' Koha_Main_MariaDB_update job) * reset_all * checkout the branch with this patchset * updatabase * prove t/db_dependent/check_sysprefs.t => Should be green Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 191949 [details] [review] Bug 41682: Add tests Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Edit: uncommented the `Test::NoWarnings` line, probably commented by mistake. Created attachment 191950 [details] [review] Bug 41682: Sync DB and sysprefs.sql for new and upgraded installs This patch removes the discrepancies and put in sync the systempreferences table for new and upgraded installations. Main changes: * Set options to NULL when options="" * Fix explanation when different * Fix wrong order (some rows had options=explanation) * Fix wrong type "Yes/No" => "YesNo" * Remove StaffLoginBranchBasedOnIP: both StaffLoginLibraryBasedOnIP and StaffLoginBranchBasedOnIP are in the DB for upgraded installs * Remove the 'showsql' from the test. If we need it back then it must be a maintenance script. * Improve the tests: * Compare sysprefs.sql and the DB content for options, explanation and type * Catch type not defined * Catch incorrect YesNo values (must be 0 or 1) Later: * Add a C4::Installer sub to add new syspref: add_pref( { variable => $variable, value => $value, options => $options, explanation => $explanation, type => $type } ); * Clean 'type' and make it an ENUM * Make it easy to run the test for devs (should be in xt but we need it in t/db_dependent for packages - FIXME?) * Run the tests on upgraded installs on CI Test plan: 0. Apply the "tests" patch only 1. prove it => It fails loudly 2. Apply the DB change and run updatedatabase, prove the tests => they pass 3. Test upgraded installs: * checkout v24.05.00 (the version we use for Jenkins' Koha_Main_MariaDB_update job) * reset_all * checkout the branch with this patchset * updatabase * prove t/db_dependent/check_sysprefs.t => Should be green Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 191951 [details] [review] Bug 41682: Add ApiKeyLog description This particular case already introduced discrepancies. Fixing. Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 191952 [details] [review] Bug 41682: Fix 'integer' vs. 'Integer' inconsistency There are 72 occurences for the 'Integer' type, and 8 for 'integer'. Tests fail because of `StoreLastBorrower` already, so fixing them all for good. Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 191953 [details] [review] Bug 41682: Fix 'cancelation' typo Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> *** Bug 41663 has been marked as a duplicate of this bug. *** Nice work all! Pushed to main for 26.05.00 Excellent, thanks a lot! The tests are failing on Jenkins: # Too many sysprefs in DB: Version, marcflavour, FrameworksLoaded, ElasticsearchIndexStatus_mydb, language See bug 41710 for ElasticsearchIndexStatus_mydb See bug 39745 for language Created attachment 192017 [details] [review] Bug 41682: Ignore 'FrameworksLoaded' Patch from commit f3c4dcb Created attachment 192027 [details] [review] Bug 41682: Ensure that sysprefs.sql is kept ordered by syspref's name Patch from commit d4b143c Created attachment 192031 [details] [review] Bug 41682: More fixes * free => Free * options=NULL where type=Free * fix options and type for ArticleRequestsSupportedFormats * fix more explanation and options Looking at recent atomicupdates for examples I noticed that the database update here has no success or other output after the SQL has been run. (In reply to Katrin Fischer from comment #22) > Looking at recent atomicupdates for examples I noticed that the database > update here has no success or other output after the SQL has been run. I didn't manage to produce easily a consistent output (because of the UPDATE returning always 1) so I decided to not produce any. follow-ups pushed to main |