[Koha-bugs] [Bug 20447] Add support for MARC holdings records

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20447

--- Comment #181 from Josef Moravec <josef.moravec at 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.