Bug 41682 - Syspref discrepancies between new and upgraded installs
Summary: Syspref discrepancies between new and upgraded installs
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi (tcohen)
URL:
Keywords:
: 41663 (view as bug list)
Depends on:
Blocks:
 
Reported: 2026-01-22 09:45 UTC by Jonathan Druart
Modified: 2026-01-27 20:57 UTC (History)
6 users (show)

See Also:
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 (10.37 KB, patch)
2026-01-22 12:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 41682: Sync DB and sysprefs.sql for new and upgraded installs (184.45 KB, patch)
2026-01-22 12:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 41682: Sync DB and sysprefs.sql for new and upgraded installs (184.87 KB, patch)
2026-01-23 09:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 41682: Add tests (10.41 KB, patch)
2026-01-23 13:41 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 41682: Sync DB and sysprefs.sql for new and upgraded installs (184.92 KB, patch)
2026-01-23 13:41 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 41682: Add tests (10.60 KB, patch)
2026-01-23 14:56 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 41682: Sync DB and sysprefs.sql for new and upgraded installs (185.08 KB, patch)
2026-01-23 14:56 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 41682: Add ApiKeyLog description (1.55 KB, patch)
2026-01-23 14:56 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 41682: Fix 'integer' vs. 'Integer' inconsistency (9.92 KB, patch)
2026-01-23 14:56 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 41682: Fix 'cancelation' typo (2.92 KB, patch)
2026-01-23 14:56 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 41682: Ignore 'FrameworksLoaded' (993 bytes, patch)
2026-01-26 10:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 41682: Ensure that sysprefs.sql is kept ordered by syspref's name (3.58 KB, patch)
2026-01-26 12:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 41682: More fixes (73.28 KB, patch)
2026-01-26 14:00 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2026-01-22 09:45:23 UTC

    
Comment 1 Jonathan Druart 2026-01-22 12:50:03 UTC
Created attachment 191858 [details] [review]
Bug 41682: Add tests

Patch from commit e4023c2
Comment 2 Jonathan Druart 2026-01-22 12:50:04 UTC
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
Comment 3 David Nind 2026-01-22 18:35:50 UTC
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
Comment 4 Jonathan Druart 2026-01-23 09:45:58 UTC
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
Comment 5 Jonathan Druart 2026-01-23 09:46:25 UTC
(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.
Comment 6 Owen Leonard 2026-01-23 13:41:11 UTC
Created attachment 191931 [details] [review]
Bug 41682: Add tests

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 7 Owen Leonard 2026-01-23 13:41:13 UTC
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>
Comment 8 Tomás Cohen Arazi (tcohen) 2026-01-23 14:56:29 UTC
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.
Comment 9 Tomás Cohen Arazi (tcohen) 2026-01-23 14:56:34 UTC
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>
Comment 10 Tomás Cohen Arazi (tcohen) 2026-01-23 14:56:36 UTC
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>
Comment 11 Tomás Cohen Arazi (tcohen) 2026-01-23 14:56:38 UTC
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>
Comment 12 Tomás Cohen Arazi (tcohen) 2026-01-23 14:56:40 UTC
Created attachment 191953 [details] [review]
Bug 41682: Fix 'cancelation' typo

Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 13 Lucas 2026-01-23 15:01:30 UTC
*** Bug 41663 has been marked as a duplicate of this bug. ***
Comment 14 Lucas Gass (lukeg) 2026-01-23 18:08:38 UTC
Nice work all! 

Pushed to main for 26.05.00
Comment 15 Jonathan Druart 2026-01-26 09:13:14 UTC
Excellent, thanks a lot!
Comment 16 Jonathan Druart 2026-01-26 09:16:57 UTC
The tests are failing on Jenkins:

# Too many sysprefs in DB: Version, marcflavour, FrameworksLoaded, ElasticsearchIndexStatus_mydb, language
Comment 17 Jonathan Druart 2026-01-26 10:14:38 UTC
See bug 41710 for ElasticsearchIndexStatus_mydb
Comment 18 Jonathan Druart 2026-01-26 10:21:07 UTC
See bug 39745 for language
Comment 19 Jonathan Druart 2026-01-26 10:42:06 UTC
Created attachment 192017 [details] [review]
Bug 41682: Ignore 'FrameworksLoaded'

Patch from commit f3c4dcb
Comment 20 Jonathan Druart 2026-01-26 12:44:02 UTC
Created attachment 192027 [details] [review]
Bug 41682: Ensure that sysprefs.sql is kept ordered by syspref's name

Patch from commit d4b143c
Comment 21 Jonathan Druart 2026-01-26 14:00:20 UTC
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
Comment 22 Katrin Fischer 2026-01-27 07:24:16 UTC
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.
Comment 23 Jonathan Druart 2026-01-27 08:34:15 UTC
(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.
Comment 24 Lucas Gass (lukeg) 2026-01-27 20:57:39 UTC
follow-ups pushed to main