[Bug 41834] New: Remove systempreferences's options, explanation and type DB values
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Bug ID: 41834 Summary: Remove systempreferences's options, explanation and type DB values Initiative type: --- Sponsorship --- status: Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: jonathan.druart@gmail.com Reporter: jonathan.druart@gmail.com QA Contact: testopia@bugs.koha-community.org Depends on: 41833 We currently have to set options, explanation and type in sysprefs.sql and the atomic/dbrev files. However we (almost) never use them, we rely on the values in the yml files. We could remove them and don't require them when when add new sysprefs. It will avoid the mess we currently have (mismatch between new and updated installations, see bug 41682 and bug 41800). Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41833 [Bug 41833] Use YML description for syspref's search -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=41800 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=41745 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=41746 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Bug 41834 depends on bug 41833, which changed state. Bug 41833 Summary: Use YML description for syspref's search https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41833 What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |RESOLVED Resolution|--- |INVALID -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |41837 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41837 [Bug 41837] Are we ready for rc1? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #1 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 194343 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194343&action=edit Bug 41834: Set systempreferences's options, explanation and type to NULL Patch from commit 637b6a5 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #2 from David Cook <dcook@prosentient.com.au> --- This sounds really good. It drives me a bit crazy that we duplicate things between the DB and the YAML files. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194343|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #3 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 194393 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194393&action=edit Bug 41834: Set systempreferences's options, explanation and type to NULL We currently have to set options, explanation and type in sysprefs.sql and the atomic/dbrev files. However we (almost) never use them, we rely on the values in the yml files. We could remove them and don't require them when when add new sysprefs. It will avoid the mess we currently have (mismatch between new and updated installations, see bug 41682 and bug 41800). Instead of fixing the discrepancies (again, see bug 41800) we should NULL the 3 columns in sysprefs.sql and so at the DB level to rely only on the yaml files. This patch does several things: 1. Remove the 3 columns from sysprefs.sql to use the default NULL 2. Atomic update: * Set to NULL for existing installations as well * Rename UseICUStyleQUotes => UseICUStyleQuotes * Delete OpacMoreSearches and OPACMySummaryNote, old prefs that have been moved to additional contents 3. Fix some errors in yaml files (mostly empty entries) 4. Move parsing of yaml files to a dedicated Koha::Devel::Syspref module 5. Retrieve all yaml content using new methods added to Koha::Config::SysPrefs (that will be reused in follow-up bugs) Test plan: 1. Run updatedatabase and confirm that now all systempreferences rows in DB have options, explanation and type set to NULL (except local-use prefs) 2. `prove t/db_dependent/check_sysprefs.t` should return green 3. Confirm that the UI is still working correctly -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194393|0 |1 is obsolete| | --- Comment #4 from David Nind <david@davidnind.com> --- Created attachment 194409 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194409&action=edit Bug 41834: Set systempreferences's options, explanation and type to NULL We currently have to set options, explanation and type in sysprefs.sql and the atomic/dbrev files. However we (almost) never use them, we rely on the values in the yml files. We could remove them and don't require them when when add new sysprefs. It will avoid the mess we currently have (mismatch between new and updated installations, see bug 41682 and bug 41800). Instead of fixing the discrepancies (again, see bug 41800) we should NULL the 3 columns in sysprefs.sql and so at the DB level to rely only on the yaml files. This patch does several things: 1. Remove the 3 columns from sysprefs.sql to use the default NULL 2. Atomic update: * Set to NULL for existing installations as well * Rename UseICUStyleQUotes => UseICUStyleQuotes * Delete OpacMoreSearches and OPACMySummaryNote, old prefs that have been moved to additional contents 3. Fix some errors in yaml files (mostly empty entries) 4. Move parsing of yaml files to a dedicated Koha::Devel::Syspref module 5. Retrieve all yaml content using new methods added to Koha::Config::SysPrefs (that will be reused in follow-up bugs) Test plan: 1. Run updatedatabase and confirm that now all systempreferences rows in DB have options, explanation and type set to NULL (except local-use prefs) 2. `prove t/db_dependent/check_sysprefs.t` should return green 3. Confirm that the UI is still working correctly Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |41994 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41994 [Bug 41994] REST API route to list system preferences -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #5 from David Cook <dcook@prosentient.com.au> --- Comment on attachment 194409 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194409 Bug 41834: Set systempreferences's options, explanation and type to NULL Review of attachment 194409: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=41834&attachment=194409) ----------------------------------------------------------------- Unfortunately, I've run out of time today, but I'll take another look at this one next week, as I'm pretty excited about it. ::: C4/Installer.pm @@ +460,5 @@
# marc_cleaned finds the marcflavour, without the variant. my $marc_cleaned = 'MARC21'; $marc_cleaned = 'UNIMARC' if $marcflavour =~ /unimarc/i; + my $request = $self->{'dbh'} + ->prepare("INSERT IGNORE INTO `systempreferences` (variable, value) VALUES('marcflavour', '$marc_cleaned')");
At some point, someone should make this use a prepared statement, but all good for now. ::: installer/data/mysql/atomicupdate/bug_41834.pl @@ +8,5 @@
+ up => sub { + my ($args) = @_; + my ( $dbh, $out ) = @$args{qw(dbh out)}; + + # First fix some discrepancies
Hmm I don't know that we should be fixing discrepancies here? @@ +36,5 @@
+ DELETE FROM systempreferences WHERE variable='OPACMySummaryNote' + } + ); + + # Then remove NULL the 3 columns for sysprefs listed in sysprefs.sql
Hmm that's an interesting point. I suppose local sysprefs will still need to retain this data in the database... and there's no clear separation of local and built-in sysprefs... ::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref @@ +696,3 @@
multiple: intransit: In-transit checkedout: Checked out
Are all the circulation.pref changes just tidy-ups unrelated to the overall bug here? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #6 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to David Cook from comment #5)
Comment on attachment 194409 [details] [review] Bug 41834: Set systempreferences's options, explanation and type to NULL
Review of attachment 194409 [details] [review]: -----------------------------------------------------------------
Unfortunately, I've run out of time today, but I'll take another look at this one next week, as I'm pretty excited about it.
::: C4/Installer.pm @@ +460,5 @@
# marc_cleaned finds the marcflavour, without the variant. my $marc_cleaned = 'MARC21'; $marc_cleaned = 'UNIMARC' if $marcflavour =~ /unimarc/i; + my $request = $self->{'dbh'} + ->prepare("INSERT IGNORE INTO `systempreferences` (variable, value) VALUES('marcflavour', '$marc_cleaned')");
At some point, someone should make this use a prepared statement, but all good for now.
::: installer/data/mysql/atomicupdate/bug_41834.pl @@ +8,5 @@
+ up => sub { + my ($args) = @_; + my ( $dbh, $out ) = @$args{qw(dbh out)}; + + # First fix some discrepancies
Hmm I don't know that we should be fixing discrepancies here?
I've embedded what I've done on https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192826 and that was still valid after the deletion of the 3 rows. We can move them to a dedicate bug report if you insist. IIRC the tests (check_sysprefs.t) fail if we don't.
@@ +36,5 @@
+ DELETE FROM systempreferences WHERE variable='OPACMySummaryNote' + } + ); + + # Then remove NULL the 3 columns for sysprefs listed in sysprefs.sql
Hmm that's an interesting point. I suppose local sysprefs will still need to retain this data in the database... and there's no clear separation of local and built-in sysprefs...
Exactly. We should add a "is_system" flag to this table but that's for another bug: it's not mandatory as we can guess it reading the yml files.
::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref @@ +696,3 @@
multiple: intransit: In-transit checkedout: Checked out
Are all the circulation.pref changes just tidy-ups unrelated to the overall bug here?
It's related, the code in get_all_from_yml does not build the resulting structure if the yml contains incorrect/empty entries -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Patch doesn't apply --- Comment #7 from David Cook <dcook@prosentient.com.au> --- Ok I think that answers my questions. Unfortunately, it looks like there's a merge conflict on installer/data/mysql/mandatory/sysprefs.sql It looks to me like a few missing sysprefs although it's tough to see exactly which ones. I think it's the following: AnonymizeLastBorrower AnonymizeLastBorrowerDays OPACTableColExpandedByDefault But tough to tell as the order is different before/after the patch. Do you have a way of generating the sysprefs.sql? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194409|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #8 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 195042 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195042&action=edit Bug 41834: Set systempreferences's options, explanation and type to NULL We currently have to set options, explanation and type in sysprefs.sql and the atomic/dbrev files. However we (almost) never use them, we rely on the values in the yml files. We could remove them and don't require them when when add new sysprefs. It will avoid the mess we currently have (mismatch between new and updated installations, see bug 41682 and bug 41800). Instead of fixing the discrepancies (again, see bug 41800) we should NULL the 3 columns in sysprefs.sql and so at the DB level to rely only on the yaml files. This patch does several things: 1. Remove the 3 columns from sysprefs.sql to use the default NULL 2. Atomic update: * Set to NULL for existing installations as well * Rename UseICUStyleQUotes => UseICUStyleQuotes * Delete OpacMoreSearches and OPACMySummaryNote, old prefs that have been moved to additional contents 3. Fix some errors in yaml files (mostly empty entries) 4. Move parsing of yaml files to a dedicated Koha::Devel::Syspref module 5. Retrieve all yaml content using new methods added to Koha::Config::SysPrefs (that will be reused in follow-up bugs) Test plan: 1. Run updatedatabase and confirm that now all systempreferences rows in DB have options, explanation and type set to NULL (except local-use prefs) 2. `prove t/db_dependent/check_sysprefs.t` should return green 3. Confirm that the UI is still working correctly Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #9 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 195043 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195043&action=edit Bug 41834: Remove columns from sysprefs.sql In vim `:%s/^\(('[^']*', \?'[^']*'\).*/\1),/` -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #10 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 195044 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195044&action=edit Bug 41834: Fix order of underscore In the previous tests we had: $b =~ s/_/ZZZ/g; # mysql sorts underscore last, if you modify this qa-test-tools will need adjustments But this is wrong, try: select variable from systempreferences; and confirm that hide_marc is before hidelostitems and that intranet_includes is before IntranetAddMastheadLibraryPulldown We will deal with qa-test-tools once this is pushed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #11 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to David Cook from comment #7)
Do you have a way of generating the sysprefs.sql?
I've split the patches to ease rebases. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #12 from David Cook <dcook@prosentient.com.au> --- Thanks for that. It looks like marcflavour isn't in ./installer/data/mysql/mandatory/sysprefs.sql so it doesn't get its options, explanation, and type nullified. We should probably add that to the atomic update. Although if we do that we'll want to update /kohadevbox/misc4dev/populate_db.pl as well I think since it looks like it might due a manual SQL insert with options, explanations, and type populated... I dropped and re-created the database and using the web installer it all looks pretty good. Everything NULL | NULL | NULL except Version and FrameworksLoaded which is NULL | Frameworks loaded through webinstaller | choice. I suppose we could include a change so that FrameworksLoaded doesn't have a type but... I'm not too fussed at this point since it's a local syspref. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #195042|0 |1 is obsolete| | --- Comment #13 from David Cook <dcook@prosentient.com.au> --- Created attachment 195109 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195109&action=edit Bug 41834: Set systempreferences's options, explanation and type to NULL We currently have to set options, explanation and type in sysprefs.sql and the atomic/dbrev files. However we (almost) never use them, we rely on the values in the yml files. We could remove them and don't require them when when add new sysprefs. It will avoid the mess we currently have (mismatch between new and updated installations, see bug 41682 and bug 41800). Instead of fixing the discrepancies (again, see bug 41800) we should NULL the 3 columns in sysprefs.sql and so at the DB level to rely only on the yaml files. This patch does several things: 1. Remove the 3 columns from sysprefs.sql to use the default NULL 2. Atomic update: * Set to NULL for existing installations as well * Rename UseICUStyleQUotes => UseICUStyleQuotes * Delete OpacMoreSearches and OPACMySummaryNote, old prefs that have been moved to additional contents 3. Fix some errors in yaml files (mostly empty entries) 4. Move parsing of yaml files to a dedicated Koha::Devel::Syspref module 5. Retrieve all yaml content using new methods added to Koha::Config::SysPrefs (that will be reused in follow-up bugs) Test plan: 1. Run updatedatabase and confirm that now all systempreferences rows in DB have options, explanation and type set to NULL (except local-use prefs) 2. `prove t/db_dependent/check_sysprefs.t` should return green 3. Confirm that the UI is still working correctly Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Cook <dcook@prosentient.com.au> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #195043|0 |1 is obsolete| | --- Comment #14 from David Cook <dcook@prosentient.com.au> --- Created attachment 195110 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195110&action=edit Bug 41834: Remove columns from sysprefs.sql In vim `:%s/^\(('[^']*', \?'[^']*'\).*/\1),/` Signed-off-by: David Cook <dcook@prosentient.com.au> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #195044|0 |1 is obsolete| | --- Comment #15 from David Cook <dcook@prosentient.com.au> --- Created attachment 195111 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195111&action=edit Bug 41834: Fix order of underscore In the previous tests we had: $b =~ s/_/ZZZ/g; # mysql sorts underscore last, if you modify this qa-test-tools will need adjustments But this is wrong, try: select variable from systempreferences; and confirm that hide_marc is before hidelostitems and that intranet_includes is before IntranetAddMastheadLibraryPulldown We will deal with qa-test-tools once this is pushed. Signed-off-by: David Cook <dcook@prosentient.com.au> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #16 from David Cook <dcook@prosentient.com.au> --- Created attachment 195112 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195112&action=edit Bug 41834: (follow-up) nullify marcflavour in atomicupdate Signed-off-by: David Cook <dcook@prosentient.com.au> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |dcook@prosentient.com.au |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #17 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to David Cook from comment #16)
Created attachment 195112 [details] [review] Bug 41834: (follow-up) nullify marcflavour in atomicupdate
Signed-off-by: David Cook <dcook@prosentient.com.au>
Thanks for this patch, and the code review, David! There is a tiny problem with this patch, $updated_last will always be 1, but not very important. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #18 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #17)
(In reply to David Cook from comment #16)
Created attachment 195112 [details] [review] [review] Bug 41834: (follow-up) nullify marcflavour in atomicupdate
Signed-off-by: David Cook <dcook@prosentient.com.au>
Thanks for this patch, and the code review, David!
There is a tiny problem with this patch, $updated_last will always be 1, but not very important.
I noticed the same thing with $updated but figured it wasn't very important either. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #19 from Jonathan Druart <jonathan.druart@gmail.com> --- *** Bug 41800 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #20 from Jonathan Druart <jonathan.druart@gmail.com> --- *** Bug 41745 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |26.05.00 released in| | Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #21 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Thanks for all the hard work! Pushed to main for the next 26.05.00 release as RM Assistant -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #22 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 195688 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195688&action=edit Bug 41834: Set pref to 0 if '' passed for YesNo This is a bit ugly, we do that already in t/db_dependent/check_sysprefs.t We could have this moved to a "is_boolean" method of Koha::Config::SyspPref (LATER) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #23 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to Jonathan Druart from comment #22)
Created attachment 195688 [details] [review] Bug 41834: Set pref to 0 if '' passed for YesNo
This is a bit ugly, we do that already in t/db_dependent/check_sysprefs.t We could have this moved to a "is_boolean" method of Koha::Config::SyspPref (LATER)
This patch fixes: prove t/db_dependent/Context.t t/db_dependent/Context.t .. 1/6 # Failed test 'set_preference should have set the value to 0, instead of an empty string' # at t/db_dependent/Context.t line 142. # got: '' # expected: '0' # Looks like you failed 1 test of 22. # Failed test 'Tests with preferences' # at t/db_dependent/Context.t line 145. # Looks like you failed 1 test of 6. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #24 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 195690 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195690&action=edit Bug 41834: Fix GetDelimiterChoices after options column removed from sysprefs.sql We removed the `options` column from all sysprefs.sql INSERT statements, leaving CSVDelimiter.options as NULL in the DB. This broke GetDelimiterChoices() which relied on that column for the list of valid delimiter choices, resulting in empty dropdowns across all report pages and an uninitialized value warning in the test suite. Fix by hardcoding the choices (matching admin.pref) and reading the current value via C4::Context->preference() instead of a raw DB query. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |adolfo.rodriguez@xercode.es --- Comment #25 from Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> --- I just updated a local instance of Koha with main branch and it's stuck in the installer screen. Instead of the file installer/data/mysql/db_revs/251200029.pl the update is in the file installer/data/mysql/db_revs/211200029.pl so Koha has nothing to execute and version in the database stays as 25.1200028 https://github.com/Koha-Community/Koha/commit/9818b3bf8d08873328b64b72f27d79... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #26 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to Adolfo Rodríguez Taboada from comment #25)
I just updated a local instance of Koha with main branch and it's stuck in the installer screen.
Instead of the file installer/data/mysql/db_revs/251200029.pl the update is in the file installer/data/mysql/db_revs/211200029.pl so Koha has nothing to execute and version in the database stays as 25.1200028
https://github.com/Koha-Community/Koha/commit/ 9818b3bf8d08873328b64b72f27d799b428affd7#diff- ea84bfc19fbe0dd7679e479dfacc5eb043d26582965ed3c5c867adcc78703ccb
Do you see that? ISBN_RANGE_MESSAGE is set to [] but that file does not exist! Trying to use the default locations at /usr/share/perl5/Business/ISBN.pm line 87. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #27 from Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> --- (In reply to Jonathan Druart from comment #26)
(In reply to Adolfo Rodríguez Taboada from comment #25)
I just updated a local instance of Koha with main branch and it's stuck in the installer screen.
Instead of the file installer/data/mysql/db_revs/251200029.pl the update is in the file installer/data/mysql/db_revs/211200029.pl so Koha has nothing to execute and version in the database stays as 25.1200028
https://github.com/Koha-Community/Koha/commit/ 9818b3bf8d08873328b64b72f27d799b428affd7#diff- ea84bfc19fbe0dd7679e479dfacc5eb043d26582965ed3c5c867adcc78703ccb
Do you see that?
ISBN_RANGE_MESSAGE is set to [] but that file does not exist! Trying to use the default locations at /usr/share/perl5/Business/ISBN.pm line 87.
I don't see how your message is related to mine. I have managed to update the local instance doing this: cp installer/data/mysql/db_revs/211200029.pl installer/data/mysql/db_revs/251200029.pl and then running installer/data/mysql/updatedatabase.pl I think the commit should be fixed so it creates the file installer/data/mysql/db_revs/251200029.pl and restores installer/data/mysql/db_revs/211200029.pl to the original version -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #28 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to Adolfo Rodríguez Taboada from comment #27)
(In reply to Jonathan Druart from comment #26)
(In reply to Adolfo Rodríguez Taboada from comment #25)
I just updated a local instance of Koha with main branch and it's stuck in the installer screen.
Instead of the file installer/data/mysql/db_revs/251200029.pl the update is in the file installer/data/mysql/db_revs/211200029.pl so Koha has nothing to execute and version in the database stays as 25.1200028
https://github.com/Koha-Community/Koha/commit/ 9818b3bf8d08873328b64b72f27d799b428affd7#diff- ea84bfc19fbe0dd7679e479dfacc5eb043d26582965ed3c5c867adcc78703ccb
Do you see that?
ISBN_RANGE_MESSAGE is set to [] but that file does not exist! Trying to use the default locations at /usr/share/perl5/Business/ISBN.pm line 87.
I don't see how your message is related to mine.
Me neither but it's what I see when I try the update from a previous version! It's fixed upstream. "Bug 41834: (RM follow-up) Undo RM mistake on db update" -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #29 from Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> --- (In reply to Jonathan Druart from comment #28)
(In reply to Adolfo Rodríguez Taboada from comment #27)
(In reply to Jonathan Druart from comment #26)
(In reply to Adolfo Rodríguez Taboada from comment #25)
I just updated a local instance of Koha with main branch and it's stuck in the installer screen.
Instead of the file installer/data/mysql/db_revs/251200029.pl the update is in the file installer/data/mysql/db_revs/211200029.pl so Koha has nothing to execute and version in the database stays as 25.1200028
https://github.com/Koha-Community/Koha/commit/ 9818b3bf8d08873328b64b72f27d799b428affd7#diff- ea84bfc19fbe0dd7679e479dfacc5eb043d26582965ed3c5c867adcc78703ccb
Do you see that?
ISBN_RANGE_MESSAGE is set to [] but that file does not exist! Trying to use the default locations at /usr/share/perl5/Business/ISBN.pm line 87.
I don't see how your message is related to mine.
Me neither but it's what I see when I try the update from a previous version!
It's fixed upstream.
"Bug 41834: (RM follow-up) Undo RM mistake on db update"
Ok, when I tried the update before it just did nothing. Koha was in version 25.1200028 and there was no update to execute. Thanks for the quick fix. :-) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |victor@tuxayo.net --- Comment #30 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Updating this is also needed: https://wiki.koha-community.org/wiki/Database_updates (additional_work_needed was already set) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #31 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to Victor Grousset/tuxayo from comment #30)
Updating this is also needed: https://wiki.koha-community.org/wiki/Database_updates (additional_work_needed was already set)
Done. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |release-notes-needed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Chloé Zermatten <chloe.zermatten@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |chloe.zermatten@openfifth.c | |o.uk Status|Pushed to main |Needs documenting --- Comment #32 from Chloé Zermatten <chloe.zermatten@openfifth.co.uk> --- Went with not backporting this to 25.11.x (as it's an enhancement) - do let me know if it needs to be added in! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Jan Kissig <bibliothek@th-wildau.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |bibliothek@th-wildau.de --- Comment #33 from Jan Kissig <bibliothek@th-wildau.de> --- I still can see this error on web installer updates in ktd: ISBN_RANGE_MESSAGE is set to [] but that file does not exist! Trying to use the default locations at /usr/share/perl5/Business/ISBN.pm line 87. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #34 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #3)
Bug 41834: Set systempreferences's options, explanation and type to NULL
We currently have to set options, explanation and type in sysprefs.sql and the atomic/dbrev files. However we (almost) never use them, we rely on the values in the yml files.
We could remove them and don't require them when when add new sysprefs.
Are we still planning to remove them now? That feels better than keeping those fields that we obsoleted.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #35 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to Marcel de Rooy from comment #34)
(In reply to Jonathan Druart from comment #3)
Bug 41834: Set systempreferences's options, explanation and type to NULL
We currently have to set options, explanation and type in sysprefs.sql and the atomic/dbrev files. However we (almost) never use them, we rely on the values in the yml files.
We could remove them and don't require them when when add new sysprefs.
Are we still planning to remove them now? That feels better than keeping those fields that we obsoleted..
The DB columns? No we have to keep them for "local use" prefs. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #36 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #35)
(In reply to Marcel de Rooy from comment #34)
(In reply to Jonathan Druart from comment #3)
Bug 41834: Set systempreferences's options, explanation and type to NULL
We currently have to set options, explanation and type in sysprefs.sql and the atomic/dbrev files. However we (almost) never use them, we rely on the values in the yml files.
We could remove them and don't require them when when add new sysprefs.
Are we still planning to remove them now? That feels better than keeping those fields that we obsoleted..
The DB columns? No we have to keep them for "local use" prefs.
Ah. Sure. Exceptions.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #37 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Thanks Jonathan, this is a nice enhancement, now adding a syspref is cleaner :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #38 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (In reply to Jan Kissig from comment #33)
I still can see this error on web installer updates in ktd:
ISBN_RANGE_MESSAGE is set to [] but that file does not exist! Trying to use the default locations at /usr/share/perl5/Business/ISBN.pm line 87.
I'm not sure it's related to this. If you can still reproduce, do you have the error upgrading to a commit just before this was pushed? How far/close are you upgrading from? What you have seems rather to be an occurrence of Bug 17438 @Jonathan
Me neither but it's what I see when I try the update from a previous version!
Do you still remember how far from/close where you doing your upgrade that got you that ISBN_RANGE_MESSAGE error? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs documenting |RESOLVED Resolution|--- |FIXED --- Comment #39 from David Nind <david@davidnind.com> --- (In reply to Jonathan Druart from comment #31)
(In reply to Victor Grousset/tuxayo from comment #30)
Updating this is also needed: https://wiki.koha-community.org/wiki/Database_updates (additional_work_needed was already set)
Done.
I think an update in the history section would be useful as well 8-) I've changed the status, as no changes to the manual are required. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 --- Comment #40 from Jan Kissig <bibliothek@th-wildau.de> --- (In reply to Victor Grousset/tuxayo from comment #38)
(In reply to Jan Kissig from comment #33)
I still can see this error on web installer updates in ktd:
ISBN_RANGE_MESSAGE is set to [] but that file does not exist! Trying to use the default locations at /usr/share/perl5/Business/ISBN.pm line 87.
I'm not sure it's related to this. If you can still reproduce, do you have the error upgrading to a commit just before this was pushed? How far/close are you upgrading from? What you have seems rather to be an occurrence of Bug 17438
@Jonathan
Me neither but it's what I see when I try the update from a previous version!
Do you still remember how far from/close where you doing your upgrade that got you that ISBN_RANGE_MESSAGE error?
You are right, this is not related to this patch but it was mentioned in some comments and I referred to it here. A simple search in BZ for ISBN_RANGE_MESSAGE did not bring any results. Moving to 17438 with further info -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41834 Bug 41834 depends on bug 41837, which changed state. Bug 41837 Summary: Are we ready for rc1? https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41837 What |Removed |Added ---------------------------------------------------------------------------- Status|Needs documenting |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org