[Koha-bugs] [Bug 13967] System preferences need a package
bugzilla-daemon at bugs.koha-community.org
bugzilla-daemon at bugs.koha-community.org
Wed Apr 8 16:24:37 CEST 2015
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967
--- Comment #11 from Kyle M Hall <kyle at 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.
More information about the Koha-bugs
mailing list