Bug 34979 - System preferences missing from sysprefs.sql
Summary: System preferences missing from sysprefs.sql
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (command-line installer) (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Katrin Fischer
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 35985 36232
  Show dependency treegraph
 
Reported: 2023-10-03 18:23 UTC by Caroline Cyr La Rose
Modified: 2024-03-11 20:04 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.03


Attachments
Bug 34979: Add unit test for comparing YAMl files with sysprefs.sql (5.19 KB, patch)
2024-01-07 22:20 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34979: Sort sysprefs.sql alphabetically (76.09 KB, patch)
2024-01-07 22:20 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34979: Fix mistakes in system preference files highlighted by unit test (25.09 KB, patch)
2024-01-07 22:20 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34979: Add unit test for comparing YAMl files with sysprefs.sql (5.24 KB, patch)
2024-01-07 23:50 UTC, David Nind
Details | Diff | Splinter Review
Bug 34979: Sort sysprefs.sql alphabetically (76.14 KB, patch)
2024-01-07 23:50 UTC, David Nind
Details | Diff | Splinter Review
Bug 34979: Fix mistakes in system preference files highlighted by unit test (25.14 KB, patch)
2024-01-07 23:50 UTC, David Nind
Details | Diff | Splinter Review
Bug 34979: Add unit test for comparing YAMl files with sysprefs.sql (5.24 KB, patch)
2024-01-12 15:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34979: Sort sysprefs.sql alphabetically (76.14 KB, patch)
2024-01-12 15:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34979: Fix mistakes in system preference files highlighted by unit test (25.14 KB, patch)
2024-01-12 15:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34979: (follow-up) Only print message when preference was added (6.95 KB, patch)
2024-01-12 15:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34979: Add unit test for comparing YAMl files with sysprefs.sql (5.24 KB, patch)
2024-01-12 22:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34979: Sort sysprefs.sql alphabetically (76.14 KB, patch)
2024-01-12 22:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34979: Fix mistakes in system preference files highlighted by unit test (25.14 KB, patch)
2024-01-12 22:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34979: (follow-up) Only print message when preference was added (9.13 KB, patch)
2024-01-12 22:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34979: Add unit test for comparing YAMl files with sysprefs.sql (5.29 KB, patch)
2024-01-21 22:24 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 34979: Sort sysprefs.sql alphabetically (76.20 KB, patch)
2024-01-21 22:24 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 34979: Fix mistakes in system preference files highlighted by unit test (25.20 KB, patch)
2024-01-21 22:24 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 34979: (follow-up) Only print message when preference was added (9.19 KB, patch)
2024-01-21 22:24 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 34979: Add unit test for comparing YAMl files with sysprefs.sql (5.36 KB, patch)
2024-01-25 14:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34979: Sort sysprefs.sql alphabetically (76.26 KB, patch)
2024-01-25 14:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34979: Fix mistakes in system preference files highlighted by unit test (25.26 KB, patch)
2024-01-25 14:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34979: (follow-up) Only print message when preference was added (9.25 KB, patch)
2024-01-25 14:30 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Caroline Cyr La Rose 2023-10-03 18:23:34 UTC
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).
Comment 1 Caroline Cyr La Rose 2023-10-03 18:39:15 UTC
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
Comment 2 Caroline Cyr La Rose 2023-10-03 18:39:39 UTC
Feel free to close if this is completely irrelevant :)
Comment 3 Caroline Cyr La Rose 2023-10-04 13:38:25 UTC
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...
Comment 4 Caroline Cyr La Rose 2023-10-04 13:38:35 UTC
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...
Comment 5 Katrin Fischer 2023-10-29 23:05:15 UTC
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?
Comment 6 Jonathan Druart 2023-10-31 10:47:57 UTC
(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)
Comment 7 Katrin Fischer 2024-01-07 22:20:46 UTC
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.
Comment 8 Katrin Fischer 2024-01-07 22:20:49 UTC
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;
Comment 9 Katrin Fischer 2024-01-07 22:20:52 UTC
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
Comment 10 Katrin Fischer 2024-01-07 22:22:52 UTC
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 :)
Comment 11 David Nind 2024-01-07 23:50:02 UTC
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>
Comment 12 David Nind 2024-01-07 23:50:05 UTC
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>
Comment 13 David Nind 2024-01-07 23:50:08 UTC
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>
Comment 14 Jonathan Druart 2024-01-12 09:10:53 UTC
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'";
Comment 15 Katrin Fischer 2024-01-12 15:09:57 UTC
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>
Comment 16 Katrin Fischer 2024-01-12 15:10:01 UTC
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>
Comment 17 Katrin Fischer 2024-01-12 15:10:04 UTC
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>
Comment 18 Katrin Fischer 2024-01-12 15:10:06 UTC
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.
Comment 19 Katrin Fischer 2024-01-12 16:21:52 UTC
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.
Comment 20 Katrin Fischer 2024-01-12 22:09:34 UTC
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>
Comment 21 Katrin Fischer 2024-01-12 22:09:38 UTC
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>
Comment 22 Katrin Fischer 2024-01-12 22:09:41 UTC
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>
Comment 23 Katrin Fischer 2024-01-12 22:09:43 UTC
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.
Comment 24 Katrin Fischer 2024-01-12 22:10:47 UTC
Figured something out :)
Comment 25 Victor Grousset/tuxayo 2024-01-21 22:24:13 UTC
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>
Comment 26 Victor Grousset/tuxayo 2024-01-21 22:24:16 UTC
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>
Comment 27 Victor Grousset/tuxayo 2024-01-21 22:24:19 UTC
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>
Comment 28 Victor Grousset/tuxayo 2024-01-21 22:24:22 UTC
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>
Comment 29 Victor Grousset/tuxayo 2024-01-21 22:31:44 UTC
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.
Comment 30 Victor Grousset/tuxayo 2024-01-22 00:24:26 UTC
(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
Comment 31 Martin Renvoize 2024-01-25 14:30:37 UTC
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>
Comment 32 Martin Renvoize 2024-01-25 14:30:40 UTC
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>
Comment 33 Martin Renvoize 2024-01-25 14:30:43 UTC
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>
Comment 34 Martin Renvoize 2024-01-25 14:30:46 UTC
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>
Comment 35 Martin Renvoize 2024-01-25 14:31:02 UTC
Nice fix, Passing QA
Comment 36 Katrin Fischer 2024-01-26 14:17:33 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 37 Fridolin Somers 2024-01-31 13:18:49 UTC
Pushed to 23.11.x for 23.11.03
Comment 38 Lucas Gass 2024-02-09 20:03:57 UTC
Doesn't apply cleanly to 23.05.x, no backport. If needed please rebase.
Comment 39 Mohd Hafiz Yusoff 2024-03-06 11:10:34 UTC
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
Comment 40 Hans Pålsson 2024-03-11 12:55:46 UTC
Dito, upgrade from 23.11.02 fails.
Comment 41 Katrin Fischer 2024-03-11 20:04:10 UTC
(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