https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20447 --- Comment #181 from Josef Moravec <josef.moravec@gmail.com> --- 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.