Bug 13967 - Add package for System preferences
Summary: Add package for System preferences
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
: 13340 (view as bug list)
Depends on:
Blocks: 15341 14339 15446
  Show dependency treegraph
 
Reported: 2015-04-08 10:38 UTC by Kyle M Hall
Modified: 2017-06-14 22:10 UTC (History)
6 users (show)

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


Attachments
Bug 13967 - System preferences need a package (5.27 KB, patch)
2015-04-08 10:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13967 - System preferences need a package (7.32 KB, patch)
2015-04-08 10:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13967 - System preferences need a package (7.30 KB, patch)
2015-04-08 10:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13967 - System preferences need a package (7.31 KB, patch)
2015-04-08 14:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13967 - System preferences need a package (7.44 KB, patch)
2015-04-08 14:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED OFF] Bug 13967 - System preferences need a package (7.51 KB, patch)
2015-04-08 14:59 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 13967 - System preferences need a package (7.59 KB, patch)
2015-06-04 10:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13967: Add a couple of tests for SysPref and Object (2.76 KB, patch)
2015-06-04 10:07 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 Kyle M Hall 2015-04-08 10:38:20 UTC
System preferences should have a package based on Koha::Object to remove the need for direct manipulation via SQL.
Comment 1 Kyle M Hall 2015-04-08 10:39:39 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2015-04-08 10:52:45 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2015-04-08 10:54:05 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2015-04-08 12:53:33 UTC
Kyle, what do u think of Koha::Config::SysPrefs instead?
Comment 5 Mark Tompsett 2015-04-08 13:35:32 UTC
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?

::: 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.
Comment 6 Mark Tompsett 2015-04-08 13:39:40 UTC
(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?
Comment 7 Tomás Cohen Arazi 2015-04-08 13:40:27 UTC
(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
Comment 8 Mark Tompsett 2015-04-08 13:48:40 UTC
(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.
Comment 9 Chris Nighswonger 2015-04-08 13:54:12 UTC
(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?
Comment 10 Kyle M Hall 2015-04-08 14:17:22 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2015-04-08 14:24:37 UTC
(In reply to M. Tompsett from comment #5)
> Comment on attachment 37576 [details] [review] [review]
> Bug 13967 - System preferences need a package
> 
> Review of attachment 37576 [details] [review] [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.
Comment 12 Kyle M Hall 2015-04-08 14:29:14 UTC Comment hidden (obsolete)
Comment 13 Chris Nighswonger 2015-04-08 14:59:34 UTC Comment hidden (obsolete)
Comment 14 Chris Nighswonger 2015-04-08 15:00:01 UTC
Works as advertised.
Comment 15 Mark Tompsett 2015-04-08 18:11:13 UTC
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.
Comment 16 Mark Tompsett 2015-04-08 18:45:10 UTC
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.
Comment 17 Mark Tompsett 2015-04-08 18:46:58 UTC
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.
Comment 18 Kyle M Hall 2015-04-08 19:59:18 UTC
(In reply to M. Tompsett from comment #16)
> Comment on attachment 37583 [details] [review] [review]
> [SIGNED OFF] Bug 13967 - System preferences need a package
> 
> Review of attachment 37583 [details] [review] [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 ; )
Comment 19 Robin Sheat 2015-04-08 23:26:03 UTC
(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.
Comment 20 Kyle M Hall 2015-04-10 13:11:52 UTC
(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!
Comment 21 Kyle M Hall 2015-04-10 13:12:37 UTC
(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?
Comment 22 Tomás Cohen Arazi 2015-04-10 13:14:24 UTC
(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.
Comment 23 Mark Tompsett 2015-04-10 13:29:33 UTC
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?
Comment 24 Robin Sheat 2015-04-12 22:17:13 UTC
(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.
Comment 25 Jonathan Druart 2015-04-13 07:24:23 UTC
(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.
Comment 26 Jonathan Druart 2015-06-04 10:07:13 UTC
Created attachment 39839 [details] [review]
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>
Comment 27 Jonathan Druart 2015-06-04 10:07:22 UTC
Created attachment 39840 [details] [review]
Bug 13967: Add a couple of tests for SysPref and Object

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>
Comment 28 Tomás Cohen Arazi 2015-06-04 13:59:52 UTC
Enhancement pushed to master.

Thanks Kyle and Jonathan!
Comment 29 Tomás Cohen Arazi 2015-07-08 17:27:43 UTC
*** Bug 13340 has been marked as a duplicate of this bug. ***