http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13967
--- Comment #11 from Kyle M Hall <[email protected]> --- (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. _______________________________________________ Koha-bugs mailing list [email protected] http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
