https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20447
--- Comment #181 from Josef Moravec <[email protected]> --- Comment on attachment 99235 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99235 Bug 20447: MARC Holdings support Review of attachment 99235: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=20447&attachment=99235) ----------------------------------------------------------------- Hi Ere, I read through the code and have some notes/questions Overall, I think it's great enhancement and I really like this to be part of Koha. ::: C4/Search.pm @@ +2242,5 @@ > } > > + # Fetch summary holdings > + if (C4::Context->preference('SummaryHoldings')) { > + $summary_holdings = Koha::Holdings->search({ biblionumber => > $oldbiblio->{biblionumber}, deleted_on => undef }); Maybe something like $biblio->holdings? ::: Koha/Biblio.pm @@ +520,5 @@ > + > +sub holdings { > + my ($self) = @_; > + > + $self->{_holdings} ||= Koha::Holdings->search( { biblionumber => > $self->biblionumber } ); use relation and new_from_dbic? ::: Koha/Holding.pm @@ +23,5 @@ > +use Carp; > + > +use C4::Charset; > +use C4::Log; > + are all imports needed? @@ +138,5 @@ > + > + return > + wantarray > + ? Koha::Items->_new_from_dbic($items_rs)->as_list > + : Koha::Items->_new_from_dbic($items_rs); do we really need this duality? @@ +166,5 @@ > + my $guard = C4::Context->dbh->{AutoCommit} ? $schema->txn_scope_guard() > : undef; > + > + my $result = $self->SUPER::store(); > + > + return $result unless $result; Should it be just 'return unless $result'? ::: Koha/Holdings.pm @@ +25,5 @@ > +use C4::Biblio; > +use C4::Charset; > +use C4::Context; > +use Koha::Database; > +use Koha::Holding; Are all imports needed? ::: Koha/Holdings/Metadata.pm @@ +34,5 @@ > +=cut > + > +=head3 record > + > +my @record = $metadata->record($params); there are no params handled in method 'record' ::: catalogue/detail.pl @@ +432,3 @@ > C4::Search::enabled_staff_search_views, > + materials => $materials_flag, > + show_summary_holdings => C4::Context->preference('SummaryHoldings') ? 1 > : 0, You could use template plugin for getting value of a syspref and then you don't need to pass this variable to template ::: cataloguing/addholding.pl @@ +151,5 @@ > +=cut > + > +sub CreateKey { > + return int(rand(1000000)); > +} Maybe Koha::Token would be better to use instead of this sub, but could be leaved to other bug report, as this pattern is in more files now. ::: cataloguing/value_builder/marc21_field_008_holdings.pl @@ +22,5 @@ > +use C4::Auth; > +use CGI qw ( -utf8 ); > +use C4::Context; > + > +use C4::Search; Do you need C4::Search ? @@ +39,5 @@ > + my $function_name = $params->{id}; > + my $dateentered = date_entered(); > + my $res = " > +<script type=\"text/javascript\"> > +//<![CDATA[ type parameter and CDATA should not be used ::: cataloguing/value_builder/marc21_leader_holdings.pl @@ +29,5 @@ > + my ( $params ) = @_; > + my $function_name = $params->{id}; > + my $res = " > +<script type=\"text/javascript\"> > +//<![CDATA[ Type parameter and CDATA should not be used ::: koha-tmpl/intranet-tmpl/prog/css/addholding.css @@ +139,5 @@ > + > +.yui-gf .yui-u { > + width: 79.2%; > +} > + Are there still yui classes? ::: koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addholding.tt @@ +6,5 @@ > +[% INCLUDE 'doc-head-close.inc' %] > +[% Asset.js("lib/hc-sticky.js") | $raw %] > +[% Asset.js("js/cataloging.js") | $raw %] > +[% INCLUDE 'strings.inc' %] > +[% Asset.js("js/browser.js") | $raw %] Even javascript included by Asset plugin should be at end of template And you need to set variable footerjs, like: [% SET footerjs = 1 %] (And macro block - see bellow) @@ +287,5 @@ > +</div> > +</div> > +</div> > + > +<script> Scripts should be in enclosed by [% MACRO jsinclude BLOCK %] ::: koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/marc21_field_008_holdings.tt @@ +3,5 @@ > +<title>Koha › Holdings › 008 builder</title> > +[% INCLUDE 'doc-head-close.inc' %] > +</head> > +<body id="cat_marc21_field_008_holdings" class="cat" style="padding:1em;"> > +<h3> 008 Fixed-length data elements</h3> There is white space just after <h3> -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list [email protected] https://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/
