[Koha-bugs] [Bug 7567] News by Library: refactor, enhance, and fix

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

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7567

--- Comment #69 from M. Tompsett <mtompset at hotmail.com> ---
(In reply to Jonathan Druart from comment #68)
> +    $template->param( $lang => 1 ) if $lang;
> Why ? Seems not used in template. But maybe I am wrong.

Because I was thinking it may exist in an earlier version of the template, and
was hoping that this could get backported, not just into master. I didn't
search earlier versions, though I think you are correct about it not being
necessary in master. I'll add a patch for that.


> 2)
> -    #    $query.= "LIMIT 0, " . $limit;
> +    #    $query.= 'LIMIT 0, ' . $limit;
> Could be removed

However, the parameter $limit was not removed, and so I'd like to leave it to
show how it was used. It may come in handy if some libraries go news-happy, and
we need to add a system preference to specify the limit on the amount of news
displayed.


> 3)
> Maybe the following is more readable:
> 
>   $query .= " WHERE 1 ";
>   if ( $lang ) {
>     $query .= "AND (opac_news.lang='' OR opac_news.lang=?)";
>     push @values, $lang
>   }
> 
> and same for $branchcode

Agreed.


> 4) s/branch/library in templates
> 
> 5) in opac-main.tt
> -       [% IF ( koha_news_count ) %]
>  <div id="news" class="container">
> +       [% IF ( koha_news_count ) %]
> 
> Why? It will produce an empty div.

Empty divs can still be styled without generating errors in javascript. And
empty divs should not cause massive screen variation over the existing output.


> Some QA issues (minor):
>  FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/tools/koha-news.tt
>    FAIL	  forbidden patterns
> 		forbidden pattern: tab char (line 233)
> 		forbidden pattern: tab char (line 169)
> 		forbidden pattern: tab char (line 227)
> 		forbidden pattern: tab char (line 229)
> 		forbidden pattern: tab char (line 228)
> 		forbidden pattern: tab char (line 225)
> 		forbidden pattern: tab char (line 167)
> 		forbidden pattern: tab char (line 231)
> 
> 
>  FAIL	koha-tmpl/opac-tmpl/prog/en/modules/opac-main.tt
>    FAIL	  forbidden patterns
> 		forbidden pattern: tab char (line 25)

Oh fiddlesticks! Patching.


> 7) It would be great to pass a hashref to get_opac_news

I don't see the number of parameters on get_opac_news growing past 3. Once you
hit that fourth parameter, that seems like the point in time to consider making
the parameter a hashref.

Hope this addresses your concerns. I'll have a revision up shortly.

-- 
You are receiving this mail because:
You are watching all bug changes.