[Bug 13967] New: System preferences need a package
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 Bug ID: 13967 Summary: System preferences need a package Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: gmcharlt@gmail.com Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org System preferences should have a package based on Koha::Object to remove the need for direct manipulation via SQL. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 37572 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=37572&action=edit Bug 13967 - System preferences need a package System preferences should have a package based on Koha::Object to remove the need for direct manipulation via SQL. Test Plan: 1) Apply this patch 2) prove t/db_dependent/sysprefs.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #37572|0 |1 is obsolete| | --- Comment #2 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 37575 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=37575&action=edit Bug 13967 - System preferences need a package System preferences should have a package based on Koha::Object to remove the need for direct manipulation via SQL. Test Plan: 1) Apply this patch 2) prove t/db_dependent/sysprefs.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #37575|0 |1 is obsolete| | --- Comment #3 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 37576 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=37576&action=edit Bug 13967 - System preferences need a package System preferences should have a package based on Koha::Object to remove the need for direct manipulation via SQL. Test Plan: 1) Apply this patch 2) prove t/db_dependent/sysprefs.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@biblibre.co | |m Assignee|gmcharlt@gmail.com |kyle@bywatersolutions.com See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=11998, | |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=9327 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 Chris Nighswonger <cnighswonger@foundations.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |cnighswonger@foundations.ed | |u -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #4 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Kyle, what do u think of Koha::Config::SysPrefs instead? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 --- Comment #5 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 37576 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=37576 Bug 13967 - System preferences need a package Review of attachment 37576: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=13967&attachment=37576) ----------------------------------------------------------------- ::: C4/Context.pm @@ +106,5 @@
use Module::Load::Conditional qw(can_load); use Carp;
+use C4::Boolean; +use C4::Debug;
Why move these two lines? ::: Koha/Objects.pm @@ +84,4 @@
my $result = $self->_resultset()->find($id);
+ return unless $result;
This is a behaviour we want for backward compatibility with preference(), but is this a correction? If so, perhaps this is a separate bug fix, technically? More scope creep? ::: Koha/SysPrefs.pm @@ +28,5 @@
+use base qw(Koha::Objects); + +=head1 NAME + +Koha::SysPrefs - Koha Borrower Object set class
Cut and paste issues? What does Borrower got to do with anything? ::: t/db_dependent/sysprefs.t @@ +27,4 @@
$dbh->{RaiseError} = 1; $dbh->{AutoCommit} = 0;
+my $opacheader = C4::Context->preference('opacheader');
Good thing this is small. Perl tidying should be a separate patch. @@ +31,4 @@
my $newopacheader = "newopacheader";
+C4::Context->set_preference( 'OPACHEADER', $newopacheader ); +is( C4::Context->preference('opacheader'), $newopacheader );
This is the better way, in my opinion, to test. ok() with 'eq' type compatisons is asking for trouble. However, if the scope of this bug is to add Koha::SysPref(s), then this is scope creep. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtompset@hotmail.com --- Comment #6 from M. Tompsett <mtompset@hotmail.com> --- (In reply to Tomás Cohen Arazi from comment #4)
Kyle, what do u think of Koha::Config::SysPrefs instead?
Out of curiousity? If logic related to the koha-conf.xml was moved into its own object, would that make it Koha::Config::Config? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 --- Comment #7 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to M. Tompsett from comment #6)
(In reply to Tomás Cohen Arazi from comment #4)
Kyle, what do u think of Koha::Config::SysPrefs instead?
Out of curiousity? If logic related to the koha-conf.xml was moved into its own object, would that make it Koha::Config::Config?
Koha::Config sounds more reasonable :-D -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |In Discussion --- Comment #8 from M. Tompsett <mtompset@hotmail.com> --- (In reply to Tomás Cohen Arazi from comment #7)
(In reply to M. Tompsett from comment #6)
(In reply to Tomás Cohen Arazi from comment #4)
Kyle, what do u think of Koha::Config::SysPrefs instead?
Out of curiousity? If logic related to the koha-conf.xml was moved into its own object, would that make it Koha::Config::Config?
Koha::Config sounds more reasonable :-D
Since the invocation currently is C4::Context->preference() and C4::Context->config(), I don't see why we would move system preferences to a different level compared to koha-conf.xml values. P.S. Reminder that at least one of my comments in comment #5, needs addressing at least (the Borrower?! one). Changing status accordingly. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 --- Comment #9 from Chris Nighswonger <cnighswonger@foundations.edu> --- (In reply to M. Tompsett from comment #8)
Since the invocation currently is C4::Context->preference() and C4::Context->config(), I don't see why we would move system preferences to a different level compared to koha-conf.xml values.
Which really raises the questions: What was intended by "Context" and would it be better/desireable at this juncture to separate system preferences (which arguably are a form of configuration) and back-end configuration? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #37576|0 |1 is obsolete| | --- Comment #10 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 37579 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=37579&action=edit Bug 13967 - System preferences need a package System preferences should have a package based on Koha::Object to remove the need for direct manipulation via SQL. Test Plan: 1) Apply this patch 2) prove t/db_dependent/sysprefs.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 --- Comment #11 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to M. Tompsett from comment #5)
Comment on attachment 37576 [details] [review] Bug 13967 - System preferences need a package
Review of attachment 37576 [details] [review]: -----------------------------------------------------------------
::: C4/Context.pm @@ +106,5 @@
use Module::Load::Conditional qw(can_load); use Carp;
+use C4::Boolean; +use C4::Debug;
Why move these two lines?
Just to group our internal libraries separately from external ones. I'd say more perl files in Koha due this than not, and I figured I'd do it while I was in there.
::: Koha/Objects.pm @@ +84,4 @@
my $result = $self->_resultset()->find($id);
+ return unless $result;
This is a behaviour we want for backward compatibility with preference(), but is this a correction? If so, perhaps this is a separate bug fix, technically? More scope creep?
Yes, I suppose this is a bug fix. I can split it out if you wish.
::: Koha/SysPrefs.pm @@ +28,5 @@
+use base qw(Koha::Objects); + +=head1 NAME + +Koha::SysPrefs - Koha Borrower Object set class
Cut and paste issues? What does Borrower got to do with anything?
Indeed, I starting with the Borrower objects as templates. Fixed!
::: t/db_dependent/sysprefs.t @@ +27,4 @@
$dbh->{RaiseError} = 1; $dbh->{AutoCommit} = 0;
+my $opacheader = C4::Context->preference('opacheader');
Good thing this is small. Perl tidying should be a separate patch
@@ +31,4 @@
my $newopacheader = "newopacheader";
+C4::Context->set_preference( 'OPACHEADER', $newopacheader ); +is( C4::Context->preference('opacheader'), $newopacheader );
This is the better way, in my opinion, to test. ok() with 'eq' type compatisons is asking for trouble. However, if the scope of this bug is to add Koha::SysPref(s), then this is scope creep.
I can see what you mean, but I did add a unit test as well, and since the syspref unit tests are part and parcel with this, I hope it's not too big a deal. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #37579|0 |1 is obsolete| | --- Comment #12 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 37581 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=37581&action=edit Bug 13967 - System preferences need a package System preferences should have a package based on Koha::Object to remove the need for direct manipulation via SQL. Test Plan: 1) Apply this patch 2) prove t/db_dependent/sysprefs.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 Chris Nighswonger <cnighswonger@foundations.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #37581|0 |1 is obsolete| | --- Comment #13 from Chris Nighswonger <cnighswonger@foundations.edu> --- Created attachment 37583 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=37583&action=edit [SIGNED OFF] Bug 13967 - System preferences need a package System preferences should have a package based on Koha::Object to remove the need for direct manipulation via SQL. Test Plan: 1) Apply this patch 2) prove t/db_dependent/sysprefs.t Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 Chris Nighswonger <cnighswonger@foundations.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #14 from Chris Nighswonger <cnighswonger@foundations.edu> --- Works as advertised. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 --- Comment #15 from M. Tompsett <mtompset@hotmail.com> --- I was noticing that the test that was added also works in the unpatched master. And on a different note, the whole enable, disable, and clear system preferences cache needs to escape C4::Context as well. This provides a good step in the right direction, even if it is small. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 --- Comment #16 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 37583 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=37583 [SIGNED OFF] Bug 13967 - System preferences need a package Review of attachment 37583: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=13967&attachment=37583) ----------------------------------------------------------------- Not worth failing over, but after my testing, this is the only issue I can seem to find. ::: t/db_dependent/sysprefs.t @@ -44,3 @@
);
-$dbh->rollback;
I know it is automatically done when the script closes, but it would be nice to keep it explicit. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 --- Comment #17 from M. Tompsett <mtompset@hotmail.com> --- To clarify, for those who think my comments mean I forgot to change the status: no. I would still consider this signed off and worthy of QA checking. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 --- Comment #18 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to M. Tompsett from comment #16)
Comment on attachment 37583 [details] [review] [SIGNED OFF] Bug 13967 - System preferences need a package
Review of attachment 37583 [details] [review]: -----------------------------------------------------------------
Not worth failing over, but after my testing, this is the only issue I can seem to find.
::: t/db_dependent/sysprefs.t @@ -44,3 @@
);
-$dbh->rollback;
I know it is automatically done when the script closes, but it would be nice to keep it explicit.
At some point it was generally decided to remove the rollback calls. I don't recall when or where. That being said I have no strong opinion on the matter. We can leave that for QA ; ) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |robin@catalyst.net.nz --- Comment #19 from Robin Sheat <robin@catalyst.net.nz> --- (In reply to Kyle M Hall from comment #18)
At some point it was generally decided to remove the rollback calls. I don't recall when or where.
This sounds dangerous for plack. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 --- Comment #20 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to M. Tompsett from comment #15)
I was noticing that the test that was added also works in the unpatched master.
And on a different note, the whole enable, disable, and clear system preferences cache needs to escape C4::Context as well. This provides a good step in the right direction, even if it is small.
I completely agree! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 --- Comment #21 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Robin Sheat from comment #19)
(In reply to Kyle M Hall from comment #18)
At some point it was generally decided to remove the rollback calls. I don't recall when or where.
This sounds dangerous for plack.
I was not aware of that! Should we create a bug report to re-add the rollback to all unit tests? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 --- Comment #22 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Kyle M Hall from comment #21)
(In reply to Robin Sheat from comment #19)
(In reply to Kyle M Hall from comment #18)
At some point it was generally decided to remove the rollback calls. I don't recall when or where.
This sounds dangerous for plack.
I was not aware of that! Should we create a bug report to re-add the rollback to all unit tests?
We haven't been removing them, just not requesting them as mandatory. I've added rollback calls to all the tests I wrote so far :-D So what we need is have it written on the Coding guidelines, and have the QA team enforce it. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 --- Comment #23 from M. Tompsett <mtompset@hotmail.com> --- In light of Tomás' comments in comment #22, could we have the rollback left alone? Or am I just being too particular, but we will on a forward basis? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 --- Comment #24 from Robin Sheat <robin@catalyst.net.nz> --- (In reply to Kyle M Hall from comment #21)
(In reply to Robin Sheat from comment #19)
This sounds dangerous for plack.
I was not aware of that! Should we create a bug report to re-add the rollback to all unit tests?
I think I was confused, I didn't realise it was referencing test cases, which won't be run under plack at all, and was to do with regular koha scripts. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 --- Comment #25 from Jonathan Druart <jonathan.druart@biblibre.com> --- (In reply to Robin Sheat from comment #24)
(In reply to Kyle M Hall from comment #21)
(In reply to Robin Sheat from comment #19)
This sounds dangerous for plack.
I was not aware of that! Should we create a bug report to re-add the rollback to all unit tests?
I think I was confused, I didn't realise it was referencing test cases, which won't be run under plack at all, and was to do with regular koha scripts.
Yes, transactions are not used using the interface (except from tools/batch_delete_records.pl, this script explicitly rollbacks). We could add the rollback call at the end of each db dependent test file, but... it's just useless. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #37583|0 |1 is obsolete| | --- Comment #26 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 39839 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=39839&action=edit Bug 13967 - System preferences need a package System preferences should have a package based on Koha::Object to remove the need for direct manipulation via SQL. Test Plan: 1) Apply this patch 2) prove t/db_dependent/sysprefs.t Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu> Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 --- Comment #27 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 39840 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=39840&action=edit Bug 13967: Add a couple of tests for SysPref and Object Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #28 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Enhancement pushed to master. Thanks Kyle and Jonathan! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |14339 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 --- Comment #29 from Tomás Cohen Arazi <tomascohen@gmail.com> --- *** Bug 13340 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|System preferences need a |Add package for System |package |preferences CC| |katrin.fischer@bsz-bw.de -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15341 Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15341 [Bug 15341] Performance - Retrieve all sysprefs at once -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15446 Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15446 [Bug 15446] Koha::Object[s]->type should be renamed to _type to avoid conflict with column name -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org