Bugzilla – Attachment 25325 Details for
Bug 7567
News by Library: refactor, enhance, and fix
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 7567 - Use, Display, Filter News by Branch
Bug-7567---Use-Display-Filter-News-by-Branch.patch (text/plain), 21.15 KB, created by
Chris Cormack
on 2014-02-16 21:02:14 UTC
(
hide
)
Description:
Bug 7567 - Use, Display, Filter News by Branch
Filename:
MIME Type:
Creator:
Chris Cormack
Created:
2014-02-16 21:02:14 UTC
Size:
21.15 KB
patch
obsolete
>From fbad503a93c65bb6944b15c94ec05bb2f8bb9da0 Mon Sep 17 00:00:00 2001 >From: Mark Tompsett <mtompset@hotmail.com> >Date: Thu, 19 Dec 2013 22:59:28 -0500 >Subject: [PATCH] Bug 7567 - Use, Display, Filter News by Branch > >With the branch parameter added to key functions in >C4/NewsChannels.pm, function calls in C4/Members.pm, >mainpage.pl, opac/opac-main.pl, tools/koha-news.pl, and >t/db_dependent/NewsChannels.t were needed. > >Some license texts were updated. > >Templates were modified to display, allow for entry and editing >of the branches selected. > >TEST PLAN >--------- >1) Having logged into the staff client, is the news displaying > correctly? Have you entered a news item which should not > display for this branch of logged in user? >2) Find a patron (with some items checked out?) >3) Print a slip > - News which is labelled 'All Branches' or for the same branch > as the one printing the slip should display on the slip. > - THIS DOES NOT AFFECT QUICK SLIPS >4) Home -> Tools -> News > - Can you edit a news item? > - Does the change save correctly? > - Can you filter based on location and branch correctly? > - Can you add a new entry correctly? > - Can you delete an entry correctly? >5) Open an OPAC client. > - Does only the news for all branches display? >6) Log into the OPAC client. > - Does the news for all branches and the specific branch display? >7) prove -v t/db_dependent/NewsChannels.t > - Does it run and all succeed? > - Does the code seem to catch the required cases? >8) Comparing the patched and unpatched versions of files affected, > are the license changes missing anything? > >Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> > >1..10 >ok 1 - use C4::NewsChannels; >ok 2 - Correctly failed on no parameter! >ok 3 - Successfully added the first dummy news item! >ok 4 - Successfully added the second dummy news item! >ok 5 - Correctly failed on no parameter! >ok 6 - Successfully updated second dummy news item! >ok 7 - Successfully tested get_opac_new id1 (0)! >ok 8 - Successfully tested get_opac_new id2 (0)! >ok 9 - Successfully tested get_opac_news for LIB1! >ok 10 - Successfully tested GetNewsToDisplay for LIB1! >--- > C4/Members.pm | 2 +- > C4/NewsChannels.pm | 43 ++++++++++++++---- > .../prog/en/modules/tools/koha-news.tt | 52 +++++++++++++++++++--- > koha-tmpl/opac-tmpl/prog/en/modules/opac-main.tt | 4 +- > mainpage.pl | 36 ++++++++------- > opac/opac-main.pl | 33 ++++++++------ > t/db_dependent/NewsChannels.t | 19 ++++++-- > tools/koha-news.pl | 17 +++++-- > 8 files changed, 151 insertions(+), 55 deletions(-) > >diff --git a/C4/Members.pm b/C4/Members.pm >index 29eda68..3f661bb 100644 >--- a/C4/Members.pm >+++ b/C4/Members.pm >@@ -2425,7 +2425,7 @@ sub IssueSlip { > 'news' => [ map { > $_->{'timestamp'} = $_->{'newdate'}; > { opac_news => $_ } >- } @{ GetNewsToDisplay("slip") } ], >+ } @{ GetNewsToDisplay("slip",$branch) } ], > ); > } > >diff --git a/C4/NewsChannels.pm b/C4/NewsChannels.pm >index 1e060ff..3e21285 100644 >--- a/C4/NewsChannels.pm >+++ b/C4/NewsChannels.pm >@@ -125,7 +125,12 @@ sub del_opac_new { > sub get_opac_new { > my ($idnew) = @_; > my $dbh = C4::Context->dbh; >- my $query = q{ SELECT * FROM opac_news WHERE idnew = ? }; >+ my $query = q{ >+ SELECT opac_news.*,branches.branchname >+ FROM opac_news LEFT JOIN branches >+ ON opac_news.branchcode=branches.branchcode >+ WHERE opac_news.idnew = ?; >+ }; > my $sth = $dbh->prepare($query); > $sth->execute($idnew); > my $data = $sth->fetchrow_hashref; >@@ -136,14 +141,32 @@ sub get_opac_new { > } > > sub get_opac_news { >- my ($limit, $lang) = @_; >+ my ($limit, $lang, $branchcode) = @_; > my @values; > my $dbh = C4::Context->dbh; >- my $query = q{ SELECT *, timestamp AS newdate FROM opac_news }; >- if ($lang) { >- $query.= " WHERE (lang='' OR lang=?)"; >+ my $query = q{ >+ SELECT opac_news.*, branches.branchname, >+ timestamp AS newdate >+ FROM opac_news LEFT JOIN branches >+ ON opac_news.branchcode=branches.branchcode >+ }; >+ if ($lang && $branchcode) { >+ $query .= " WHERE (opac_news.lang='' OR opac_news.lang=?)"; >+ $query .= " AND (opac_news.branchcode IS NULL OR opac_news.branchcode=?)"; >+ push @values,$lang; >+ push @values,$branchcode; >+ } >+ elsif ($lang) { >+ $query .= " WHERE (opac_news.lang='' OR opac_news.lang=?)"; > push @values,$lang; > } >+ elsif ($branchcode) { >+ $query .= " WHERE (opac_news.branchcode IS NULL OR opac_news.branchcode=?)"; >+ push @values,$branchcode; >+ } >+ else { >+ # nothing >+ } > $query.= ' ORDER BY timestamp DESC '; > #if ($limit) { > # $query.= 'LIMIT 0, ' . $limit; >@@ -165,14 +188,15 @@ sub get_opac_news { > > =head2 GetNewsToDisplay > >- $news = &GetNewsToDisplay($lang); >+ $news = &GetNewsToDisplay($lang,$branch); > C<$news> is a ref to an array which containts >- all news with expirationdate > today or expirationdate is null. >+ all news with expirationdate > today or expirationdate is null >+ that is applicable for a given branch. > > =cut > > sub GetNewsToDisplay { >- my ($lang) = @_; >+ my ($lang,$branch) = @_; > my $dbh = C4::Context->dbh; > # SELECT *,DATE_FORMAT(timestamp, '%d/%m/%Y') AS newdate > my $query = q{ >@@ -185,13 +209,14 @@ sub GetNewsToDisplay { > ) > AND `timestamp` < CURRENT_DATE()+1 > AND (lang = '' OR lang = ?) >+ AND (branchcode IS NULL OR branchcode = ?) > ORDER BY number > }; # expirationdate field is NOT in ISO format? > # timestamp has HH:mm:ss, CURRENT_DATE generates 00:00:00 > # by adding 1, that captures today correctly. > my $sth = $dbh->prepare($query); > $lang = $lang // q{}; >- $sth->execute($lang); >+ $sth->execute($lang,$branch); > my @results; > while ( my $row = $sth->fetchrow_hashref ){ > $row->{newdate} = format_date($row->{newdate}); >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/koha-news.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/koha-news.tt >index 48f6025..834a2cc 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/koha-news.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/koha-news.tt >@@ -75,7 +75,7 @@ Edit News Item[% ELSE %]Add News Item[% END %][% ELSE %]News[% END %]</div> > <fieldset class="rows"> > <legend>OPAC and Koha news</legend> > <ol> <li> >- <label for="lang">([% new_detail.lang %])([% lang %])Display location</label> >+ <label for="lang">Display location</label> > <select id="lang" name="lang"> > [% IF ( default_lang == "" ) %] > <option value="" selected>All</option> >@@ -102,6 +102,23 @@ Edit News Item[% ELSE %]Add News Item[% END %][% ELSE %]News[% END %]</div> > </select> > </li> > <li> >+ <label for="branch">Branch: </label> >+ <select id="branch" name="branch"> >+ [% IF ( new_detail.branchcode == '' ) %] >+ <option value="" selected>All Branches</option> >+ [% ELSE %] >+ <option value="" >All Branches</option> >+ [% END %] >+ [% FOREACH branch_item IN branch_list %] >+ [% IF ( branch_item.value.branchcode == new_detail.branchcode ) %] >+ <option value="[% branch_item.value.branchcode %]" selected>[% branch_item.value.branchname %]</option> >+ [% ELSE %] >+ <option value="[% branch_item.value.branchcode %]">[% branch_item.value.branchname %]</option> >+ [% END %] >+ [% END %] >+ </select> >+ </li> >+ <li> > <label for="title">Title: </label> > <input id="title" size="30" type="text" name="title" value="[% new_detail.title %]" /> > </li> >@@ -154,7 +171,26 @@ Edit News Item[% ELSE %]Add News Item[% END %][% ELSE %]News[% END %]</div> > [% IF ( lang_lis.language == lang ) %] > <option value="[% lang_lis.language %]" selected>OPAC ([% lang_lis.language %])</option> > [% ELSE %] >- <option value="[% lang_lis.language %]">OPAC ([% lang_lis.language %])</option> >+ <option value="[% lang_lis.language %]" >OPAC ([% lang_lis.language %])</option> >+ [% END %] >+ [% END %] >+ </select> >+ <label for="branch">Branch: </label> >+ <select id="branch" name="branch"> >+ [% IF ( branchcode == "" ) %] >+ <option value="" selected>All Branches</option> >+ [% ELSE %] >+ <option value="" >All Branches</option> >+ [% END %] >+ [% FOREACH branch_item IN branch_list %] >+ [% IF ( branch_item.value.branchcode == branchcode ) %] >+ <option value="[% branch_item.value.branchcode %]" >+ selected>[% branch_item.value.branchname %] >+ </option> >+ [% ELSE %] >+ <option value="[% branch_item.value.branchcode %]" >+ >[% branch_item.value.branchname %] >+ </option> > [% END %] > [% END %] > </select> >@@ -167,6 +203,7 @@ Edit News Item[% ELSE %]Add News Item[% END %][% ELSE %]News[% END %]</div> > <thead> <tr> > <th> </th> > <th>Location</th> >+ <th>Branch</th> > <th>Number</th> > <th>Creation date</th> > <th>Expiration date</th> >@@ -184,17 +221,20 @@ Edit News Item[% ELSE %]Add News Item[% END %][% ELSE %]News[% END %]</div> > <input type="checkbox" name="ids" value="[% opac_new.idnew %]" /> > </td> > <td>[% SWITCH opac_new.lang %] >- [% CASE "koha" %] >+ [% CASE 'koha' %] > Librarian interface >- [% CASE "slip" %] >+ [% CASE 'slip' %] > Slip >- [% CASE "" %] >+ [% CASE '' %] > All > [% CASE %] > OPAC ([% opac_new.lang %]) > [% END %] > </td> >- >+ <td>[% IF ( opac_new.branchcode == "" ) -%] >+ All Branches >+ [% ELSE %][% opac_new.branchname %] >+ [% END %]</td> > <td>[% opac_new.number %]</td> > <td>[% opac_new.newdate %]</td> > <td>[% opac_new.expirationdate %] [% IF ( opac_new.expired ) %](<span class="expired">expired</span>)[% END %]</td> >diff --git a/koha-tmpl/opac-tmpl/prog/en/modules/opac-main.tt b/koha-tmpl/opac-tmpl/prog/en/modules/opac-main.tt >index eb1e600..df368ba 100644 >--- a/koha-tmpl/opac-tmpl/prog/en/modules/opac-main.tt >+++ b/koha-tmpl/opac-tmpl/prog/en/modules/opac-main.tt >@@ -21,8 +21,8 @@ > <div id="notloggedin" class="yui-ge"> > [% END %] > <div class="yui-u first"> >- [% IF ( koha_news_count ) %] > <div id="news" class="container"> >+ [% IF ( koha_news_count ) %] > <table> > [% FOREACH koha_new IN koha_news %] > <tr><th>[% koha_new.title %]</th></tr> >@@ -30,8 +30,8 @@ > <p class="newsfooter"><i>(published on [% koha_new.newdate %])</i></p></td></tr> > [% END %] > </table> >-</div> > [% END %] >+</div> > > [% IF ( display_daily_quote && daily_quote ) %] > <div id="daily-quote" class="container"><h1>Quote of the Day</h1><div><span id="daily-quote-text">[% daily_quote.text %]</span><span id="daily-quote-sep"> ~ </span><span id="daily-quote-source">[% daily_quote.source %]</span></div></div> >diff --git a/mainpage.pl b/mainpage.pl >index 4159618..a0de153 100755 >--- a/mainpage.pl >+++ b/mainpage.pl >@@ -1,30 +1,30 @@ > #!/usr/bin/perl > >+# This file is part of Koha. >+# > # Copyright Paul Poulain 2002 > # Parts Copyright Liblime 2007 >+# Copyright (C) 2013 Mark Tompsett > # >-# This file is part of Koha. >+# Koha is free software; you can redistribute it and/or modify it >+# under the terms of the GNU General Public License as published by >+# the Free Software Foundation; either version 3 of the License, or >+# (at your option) any later version. > # >-# Koha is free software; you can redistribute it and/or modify it under the >-# terms of the GNU General Public License as published by the Free Software >-# Foundation; either version 2 of the License, or (at your option) any later >-# version. >+# Koha is distributed in the hope that it will be useful, but >+# WITHOUT ANY WARRANTY; without even the implied warranty of >+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >+# GNU General Public License for more details. > # >-# Koha is distributed in the hope that it will be useful, but WITHOUT ANY >-# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR >-# A PARTICULAR PURPOSE. See the GNU General Public License for more details. >-# >-# You should have received a copy of the GNU General Public License along >-# with Koha; if not, write to the Free Software Foundation, Inc., >-# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. >+# You should have received a copy of the GNU General Public License >+# along with Koha; if not, see <http://www.gnu.org/licenses>. > >-use strict; >-use warnings; >+use Modern::Perl; > use CGI; > use C4::Output; > use C4::Auth; > use C4::Koha; >-use C4::NewsChannels; >+use C4::NewsChannels; # GetNewsToDisplay > use C4::Review qw/numberofreviews/; > use C4::Suggestions qw/CountSuggestion/; > use C4::Tags qw/get_count_by_tag_status/; >@@ -42,7 +42,11 @@ my ( $template, $loggedinuser, $cookie, $flags ) = get_template_and_user( > } > ); > >-my $all_koha_news = &GetNewsToDisplay("koha"); >+my $homebranch; >+if (C4::Context->userenv) { >+ $homebranch = C4::Context->userenv->{'branch'}; >+} >+my $all_koha_news = &GetNewsToDisplay("koha",$homebranch); > my $koha_news_count = scalar @$all_koha_news; > > $template->param( >diff --git a/opac/opac-main.pl b/opac/opac-main.pl >index 7c6ee36..a58659c 100755 >--- a/opac/opac-main.pl >+++ b/opac/opac-main.pl >@@ -2,26 +2,27 @@ > > # This file is part of Koha. > # >-# Koha is free software; you can redistribute it and/or modify it under the >-# terms of the GNU General Public License as published by the Free Software >-# Foundation; either version 2 of the License, or (at your option) any later >-# version. >+# Parts Copyright (C) 2013 Mark Tompsett > # >-# Koha is distributed in the hope that it will be useful, but WITHOUT ANY >-# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR >-# A PARTICULAR PURPOSE. See the GNU General Public License for more details. >+# Koha is free software; you can redistribute it and/or modify it >+# under the terms of the GNU General Public License as published by >+# the Free Software Foundation; either version 3 of the License, or >+# (at your option) any later version. > # >-# You should have received a copy of the GNU General Public License along >-# with Koha; if not, write to the Free Software Foundation, Inc., >-# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. >+# Koha is distributed in the hope that it will be useful, but >+# WITHOUT ANY WARRANTY; without even the implied warranty of >+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >+# GNU General Public License for more details. >+# >+# You should have received a copy of the GNU General Public License >+# along with Koha; if not, see <http://www.gnu.org/licenses>. > > >-use strict; >-use warnings; >+use Modern::Perl; > use CGI; > use C4::Auth; # get_template_and_user > use C4::Output; >-use C4::NewsChannels; # get_opac_news >+use C4::NewsChannels; # GetNewsToDisplay > use C4::Languages qw(getTranslatedLanguages accept_language); > use C4::Koha qw( GetDailyQuote ); > >@@ -48,7 +49,11 @@ $template->param( > # use cookie setting for language, bug default to syspref if it's not set > my ($theme, $news_lang, $availablethemes) = C4::Templates::themelanguage(C4::Context->config('opachtdocs'),'opac-main.tt','opac',$input); > >-my $all_koha_news = &GetNewsToDisplay($news_lang); >+my $homebranch; >+if (C4::Context->userenv) { >+ $homebranch = C4::Context->userenv->{'branch'}; >+} >+my $all_koha_news = &GetNewsToDisplay($news_lang,$homebranch); > my $koha_news_count = scalar @$all_koha_news; > > my $quote = GetDailyQuote(); # other options are to pass in an exact quote id or select a random quote each pass... see perldoc C4::Koha >diff --git a/t/db_dependent/NewsChannels.t b/t/db_dependent/NewsChannels.t >index 0e34cb0..f2a29e4 100644 >--- a/t/db_dependent/NewsChannels.t >+++ b/t/db_dependent/NewsChannels.t >@@ -23,6 +23,13 @@ my $dbh = C4::Context->dbh; > $dbh->{AutoCommit} = 0; > $dbh->{RaiseError} = 1; > >+# Add LIB1, if it doesn't exist. >+my $addbra = 'LIB1'; >+if ( !GetBranchName($addbra) ) { >+ $dbh->do( q{ INSERT INTO branches (branchcode,branchname) VALUES (?,?) }, >+ undef, ( $addbra, "$addbra branch" ) ); >+} >+ > # Test add_opac_new > my $rv = add_opac_new(); # intentionally bad > ok( $rv == 0, 'Correctly failed on no parameter!' ); >@@ -38,6 +45,7 @@ my $href_entry1 = { > expirationdate => $expirationdate1, > timestamp => $timestamp1, > number => $number1, >+ branchcode => 'LIB1', > }; > > $rv = add_opac_new($href_entry1); >@@ -52,6 +60,7 @@ my $href_entry2 = { > expirationdate => $expirationdate2, > timestamp => $timestamp2, > number => $number2, >+ branchcode => 'LIB1', > }; > $rv = add_opac_new($href_entry2); > ok( $rv == 1, 'Successfully added the second dummy news item!' ); >@@ -107,11 +116,13 @@ if ( $hashref_check->{number} ne $number2 ) { $failure = $F6; } > ok( $failure == 0, "Successfully tested get_opac_new id2 ($failure)!" ); > > # Test get_opac_news (multiple news items) >-my ( $opac_news_count, $arrayref_opac_news ) = get_opac_news( 0, q{} ); >-ok( $opac_news_count >= 2, 'Successfully tested get_opac_news!' ); >+my ( $opac_news_count, $arrayref_opac_news ) = get_opac_news( 0, q{}, 'LIB1' ); >+ >+# using >= 2, because someone may have LIB1 news already. >+ok( $opac_news_count >= 2, 'Successfully tested get_opac_news for LIB1!' ); > > # Test GetNewsToDisplay >-( $opac_news_count, $arrayref_opac_news ) = GetNewsToDisplay(q{}); >-ok( $opac_news_count >= 2, 'Successfully tested GetNewsToDisplay!' ); >+( $opac_news_count, $arrayref_opac_news ) = GetNewsToDisplay( q{}, 'LIB1' ); >+ok( $opac_news_count >= 2, 'Successfully tested GetNewsToDisplay for LIB1!' ); > > $dbh->rollback; >diff --git a/tools/koha-news.pl b/tools/koha-news.pl >index 5170433..f5ef7be 100755 >--- a/tools/koha-news.pl >+++ b/tools/koha-news.pl >@@ -33,6 +33,7 @@ use C4::Output; > use C4::NewsChannels; > use C4::Languages qw(getTranslatedLanguages); > use Date::Calc qw/Date_to_Days Today/; >+use C4::Branch qw/GetBranches/; > > my $cgi = new CGI; > >@@ -43,6 +44,10 @@ my $expirationdate = format_date_in_iso($cgi->param('expirationdate')); > my $timestamp = format_date_in_iso($cgi->param('timestamp')); > my $number = $cgi->param('number'); > my $lang = $cgi->param('lang'); >+my $branchcode = $cgi->param('branch'); >+# Foreign Key constraints work with NULL, not '' >+# NULL = All branches. >+$branchcode = undef if (defined($branchcode) && $branchcode eq ''); > > my $new_detail = get_opac_new($id); > >@@ -68,9 +73,13 @@ foreach my $language ( @$tlangs ) { > }; > } > >-$template->param( lang_list => \@lang_list ); >+my $branches = GetBranches; > >-my $op = $cgi->param('op'); >+$template->param( lang_list => \@lang_list, >+ branch_list => $branches, >+ branchcode => $branchcode ); >+ >+my $op = $cgi->param('op') // ''; > > if ( $op eq 'add_form' ) { > $template->param( add_form => 1 ); >@@ -94,6 +103,7 @@ elsif ( $op eq 'add' ) { > expirationdate => $expirationdate, > timestamp => $timestamp, > number => $number, >+ branchcode => $branchcode, > }; > add_opac_new( $hashref ); > print $cgi->redirect("/cgi-bin/koha/tools/koha-news.pl"); >@@ -107,6 +117,7 @@ elsif ( $op eq 'edit' ) { > expirationdate => $expirationdate, > timestamp => $timestamp, > number => $number, >+ branchcode => $branchcode, > }; > upd_opac_new( $hashref ); > print $cgi->redirect("/cgi-bin/koha/tools/koha-news.pl"); >@@ -119,7 +130,7 @@ elsif ( $op eq 'del' ) { > > else { > >- my ( $opac_news_count, $opac_news ) = &get_opac_news( undef, $lang ); >+ my ( $opac_news_count, $opac_news ) = &get_opac_news( undef, $lang, $branchcode ); > > foreach my $new ( @$opac_news ) { > next unless $new->{'expirationdate'}; >-- >1.8.5.3
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=25325">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 7567
:
22863
|
22917
|
22920
|
23350
|
23351
|
23352
|
23362
|
23368
|
23378
|
23383
|
23386
|
23474
|
23475
|
23501
|
23544
|
23547
|
23552
|
23553
|
23554
|
23559
|
23664
|
23703
|
23704
|
23705
|
25316
|
25317
|
25318
|
25319
|
25320
|
25321
|
25322
|
25323
|
25324
|
25325
|
25331
|
25332
|
25333
|
26510
|
26511
|
26590
|
26591
|
26592
|
26593
|
26594
|
26595
|
26596
|
26597
|
26598
|
26599
|
26600
|
26601
|
26602
|
26603
|
26604
|
26605
|
26606
|
26607
|
26608
|
26609
|
26610
|
26611
|
26612
|
26631
|
26632
|
26633
|
26634
|
26635
|
26636
|
26637
|
26638
|
26639
|
26640
|
26641
|
26642
|
26643
|
26644
|
26647