Bug 7567

Summary: News by Library: refactor, enhance, and fix
Product: Koha Reporter: Savitra Sirohi <savitra.sirohi>
Component: ToolsAssignee: Mark Tompsett <mtompset>
Status: CLOSED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: enhancement    
Priority: P3 CC: abl, chris, gmcharlt, gwilliams, jonathan.druart, katrin.fischer, liz, nengard
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 12167    
Attachments: Bug 7567 - News by Branch
Bug 7567 - News by Branch
Bug 7567 - News by Branch
Bug 7567 - News by Branch
Bug 7567 - News by Branch
Bug 7567 - News by Branch
Bug 7567 - News by Branch
Bug 7567 - News by Branch
Bug 7567 - News by Branch
Bug 7567 - News by Branch
Bug 7567 - News by Branch
Bug 7567 - News by Branch
Bug 7567 - News by Branch
Bug 7567 - Testing for C4/NewsChannel.pm
Bug 7567 - Update License and use Modern::Perl
Bug 7567 - Update License tools/koha-news.pl
Bug 7567 - Clean up sample news files
Bug 7567 - Undefined $lang triggers warning
Bug 7567 - Correct Filtering and Default dropdown values
Bug 7567 - Removed finish calls
Bug 7567 - Too many parameters and tiny bugs
Bug 7567 - Added branchcode to opac_news
Bug 7567 - Use, Display, Filter News by Branch
Bug 7567 - Use, Display, Filter News by Branch
Bug 7567 - Testing for C4/NewsChannel.pm
Bug 7567 - Update License and use Modern::Perl
Bug 7567 - Update License tools/koha-news.pl
Bug 7567 - Clean up sample news files
Bug 7567 - Undefined $lang triggers warning
Bug 7567 - Correct Filtering and Default dropdown values
Bug 7567 - Removed finish calls
Bug 7567 - Too many parameters and tiny bugs
Bug 7567 - Added branchcode to opac_news
Bug 7567 - Use, Display, Filter News by Branch
Bug 7567 - Schema update for opac_news
Bug 7567 - Schema update for opac_news
Bug 7567 - Schema update for opac_news
Bug 7567 - Use, Display, Filter News by Branch
Bug 7567 - Schema update for opac_news
Bug 7567 - Testing for C4/NewsChannel.pm
Bug 7567 - Update License and use Modern::Perl
Bug 7567 - Update License tools/koha-news.pl
Bug 7567 - Clean up sample news files
Bug 7567 - Undefined $lang triggers warning
Bug 7567 - Correct Filtering and Default dropdown values
Bug 7567 - Removed finish calls
Bug 7567 - Too many parameters and tiny bugs
Bug 7567 - Added branchcode to opac_news
Bug 7567 - Use, Display, Filter News by Branch
Bug 7567 - Schema update for opac_news
Bug 7567 - Testing for C4/NewsChannel.pm
Bug 7567 - Update License and use Modern::Perl
Bug 7567 - Update License tools/koha-news.pl
Bug 7567 - Clean up sample news files
Bug 7567 - Undefined $lang triggers warning
[NEEDS SIGN OFF] Bug 7567 - Correct Filtering and Default dropdown values
Bug 7567 - Removed finish calls
[NEEDS SIGN OFF] Bug 7567 - Too many parameters and tiny bugs
Bug 7567 - Added branchcode to opac_news
[NEEDS SIGN OFF] Bug 7567 - Use, Display, Filter News by Branch
Bug 7567 - Schema update for opac_news
[NEED SIGN OFF] Bug 7567 - Remove excess line
Bug 7567 - Testing for C4/NewsChannel.pm
Bug 7567 - Update License and use Modern::Perl
Bug 7567 - Update License tools/koha-news.pl
Bug 7567 - Clean up sample news files
Bug 7567 - Remove excess line
Bug 7567 - Correct Filtering and Default dropdown values
Bug 7567 - Removed finish calls
Bug 7567 - Too many parameters and tiny bugs
Bug 7567 - Added branchcode to opac_news
Bug 7567 - Use, Display, Filter News by Branch
Bug 7567 - Schema update for opac_news
Bug 7567: QA followup: FIX SQL query
Bug 7567: QA followup: remove useless $parameters hashref
Bug 7567 - Modify DataTables configuration
Bug 7567 - Modify DataTables configuration

Description Savitra Sirohi 2012-02-21 06:13:20 UTC
Allow creation of News item by library. On both Staff and OPAC dispaly a library pull down menu so users can filter by library to see News specific to that library.
Comment 1 Mark Tompsett 2013-10-11 14:32:46 UTC
Since I recently heard that osslabs.biz is no longer in business, I am taking this bug. Though, I am probably going to take it a slightly different direction.

The tools->news will have branch fields added. This would allow for every branch to create a news item for their library hours or other specific news, which will be then become visible once a user logs in. Anonymous users will not be able to see branch specific news.
Comment 2 Mark Tompsett 2013-11-08 07:07:57 UTC
Refactored, Enhanced, and Fixed. I'm tempted to rename this to "Overhaul of News".
Comment 3 Mark Tompsett 2013-11-12 05:46:04 UTC Comment hidden (obsolete)
Comment 4 Mark Tompsett 2013-11-14 00:30:10 UTC Comment hidden (obsolete)
Comment 5 Mark Tompsett 2013-11-14 01:45:51 UTC Comment hidden (obsolete)
Comment 6 Liz Rea 2013-11-14 03:55:41 UTC
Hi Mark,

Do you have a test plan for this?

Cheers,
Liz
Comment 7 Mark Tompsett 2013-11-14 12:12:33 UTC
I'm still working on the test plan. I have to review if this is complete. This is why the status is "In Discussion" and not "Needs Signoff".

 1) Do a fresh install, to ensure that branchcode is created as a field
     in the opac_news table.
 2) Do an upgrade to confirm that the branchcode field is added
     to the opac_news table.
 3) prove -v t/db_dependent/NewsChannels.t
     This should test all the changes in C4::NewsChannels functions.
 4) Log into the staff client
    - Does the News display on the main page bust?
 5) Go to Tools->News
 6) Add a News item, for something other than 'All Branches'.
 7) Add a News item, for OPAC.
 8) Change the Location and Branch filters.
    - Do they show what you select after you click the Filter button?
          (Unpatched the Location does not!)
    - Are the items displayed matching what is selected?
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
 9) Click 'Edit' for one of the news items added.
    - Does what comes up match what was displayed?
          ( Unpatched the Location may be 'All' )
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
    - Does the submitted values match the changes made?
10) Check the news items added and click the delete button.
    - Did it delete them appropriately?
11) In a new tab, go to OPAC
    - Does the News display on the main page bust?
    - Does it display the news item for a specific branch?
          (It shouldn't when patched, but unpatched it would)
12) Log in as a user with the branch you added the news item for.
    - Does that piece of News display now too?
Comment 8 Liz Rea 2013-11-14 20:08:48 UTC
Cool, thanks. I'm actually quite interested in this patch - it's great functionality.

Cheers,
Liz
Comment 9 Mark Tompsett 2013-11-14 21:29:45 UTC
Okay, I've reviewed and I think this test plan should be good.

 1) Do a fresh install, to ensure that branchcode is created as a field
     in the opac_news table.
 2) Do an upgrade to confirm that the branchcode field is added
     to the opac_news table.
 3) prove -v t/db_dependent/NewsChannels.t
     This should test all the changes in C4::NewsChannels functions.
 4) Log into the staff client
    - Does the News display on the main page bust?
 5) Go to Tools->News
    - Are the locations properly display?
 6) Add a News item, for 'All' interfaces for 'All Branches'.
 7) Add a News item, for 'OPAC' interface at your user branch.
 8) Change the Location and Branch filters.
    - Do they show what you select after you click the Filter button?
          (Unpatched the Location does not!)
    - Are the items displayed matching what is selected?
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
 9) Click 'Edit' for one of the news items added.
    - Does what comes up match what was displayed?
          ( Unpatched the Location may be 'All' )
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
    - Does the submitted values match the changes made?
10) In a new tab, go to OPAC
    - Does the News display on the main page bust?
    - Does it display the news item for a specific branch?
          (It shouldn't when patched, but unpatched it would)
          (Only one of the new things added should be displayed)
11) Log in as a user with the branch you added the news item for.
    - Does that piece of News display now too?
          (Both things added should be displayed)
12) Check the news items added and click the delete button in the staff client.
    - Did it delete them appropriately?
Comment 10 Mark Tompsett 2013-12-07 04:35:21 UTC
I found another section affected by the GetNewsToDisplay change. This affects the slips, like Liz was asking about. Doing further testing first, and new patch up by next week.
Comment 11 Mark Tompsett 2013-12-09 02:43:56 UTC Comment hidden (obsolete)
Comment 12 Mark Tompsett 2013-12-09 02:57:10 UTC Comment hidden (obsolete)
Comment 13 Mark Tompsett 2013-12-09 05:53:23 UTC Comment hidden (obsolete)
Comment 14 Mark Tompsett 2013-12-09 05:54:41 UTC
This is the latest, but I haven't fully tested it yet, hence the status is not changed. Feel free to test it and then anxiously await my toggling the status if there are no problems. :)
Comment 15 Mark Tompsett 2013-12-09 18:37:28 UTC Comment hidden (obsolete)
Comment 16 Mark Tompsett 2013-12-10 04:58:49 UTC Comment hidden (obsolete)
Comment 17 Mark Tompsett 2013-12-10 07:16:30 UTC
Okay, average as compared to low. And, I just cleaned up the sample news issues for a fresh install. Now to actually test with a rolled system.
Comment 18 Katrin Fischer 2013-12-10 09:20:13 UTC
Hm, from the subject of this it looks a bit like it should better be several patches ('Ätell the RM a story' ;) ) than one big patch?
Comment 19 Mark Tompsett 2013-12-10 14:39:49 UTC
I would, except everything is so interdependent, I don't see where to break it up right now. I suppose I could try to break the "change from (parameter, parameter, parameter,etc.) into hash ref" portion out, but I didn't think to do that early on. I'll see if I can generate some sensible break lines.
Comment 20 Mark Tompsett 2013-12-10 14:45:54 UTC Comment hidden (obsolete)
Comment 21 Owen Leonard 2013-12-10 20:48:01 UTC
*** Bug 11370 has been marked as a duplicate of this bug. ***
Comment 22 Mark Tompsett 2013-12-10 22:17:00 UTC Comment hidden (obsolete)
Comment 23 Mark Tompsett 2013-12-11 05:14:38 UTC Comment hidden (obsolete)
Comment 24 Mark Tompsett 2013-12-11 05:43:38 UTC
I know it's big, and I didn't split it up, but I finally got through my test plan and it all works.
Comment 25 Katrin Fischer 2013-12-11 22:28:58 UTC
Comment on attachment 23386 [details] [review]
Bug 7567 - News by Branch

Review of attachment 23386 [details] [review]:
-----------------------------------------------------------------

Hi Mark,

I did a quick code review - noted some things using splinter review below. Could you please take a look?

::: C4/NewsChannels.pm
@@ +124,5 @@
>      $data->{$data->{'lang'}} = 1 if defined $data->{lang};
>      $data->{expirationdate} = format_date($data->{expirationdate});
>      $data->{timestamp}      = format_date($data->{timestamp});
> +    if (!defined($data->{branchcode}) || $data->{branchcode} eq '') {
> +        $data->{branchname}     = "All Branches";

Suspecting a translation problem here.

@@ +166,5 @@
>      while (my $row = $sth->fetchrow_hashref) {
>          if ((($limit) && ($count < $limit)) || (!$limit)) {
>              $row->{'newdate'} = format_date($row->{'newdate'});
>              $row->{'expirationdate'} = format_date($row->{'expirationdate'});
> +            $row->{'branchname'}     = "All Branches" if !$row->{'branchcode'};

And here.

::: installer/data/mysql/en/optional/sample_news.sql
@@ +1,1 @@
> +INSERT INTO `opac_news` ( title,             new,

Please also correct translated installers to avoid problems when installing Koha in other languages.

::: installer/data/mysql/updatedatabase.pl
@@ +7826,5 @@
>  
> +$DBversion = "3.15.00.XXX";
> +if ( CheckVersion($DBversion) ) {
> +    $dbh->do(q{
> +        ALTER TABLE opac_news ADD branchcode varchar(10) NULL DEFAULT NULL,

Please make sure that the new column is added at the same position than it is in kohastructure.sql (using AFTER should work).
Comment 26 Mark Tompsett 2013-12-12 02:40:04 UTC Comment hidden (obsolete)
Comment 27 Mark Tompsett 2013-12-12 06:07:23 UTC Comment hidden (obsolete)
Comment 28 Mark Tompsett 2013-12-12 06:09:44 UTC
As per Katrin Fischer's comments in comment #18, I am going to attempt to split this into smaller pieces. This is why the status is still Failed QA. I just wanted to make sure my patch was here for eyeball review, if someone wanted to peek. :)
Comment 29 Katrin Fischer 2013-12-12 07:55:05 UTC
Hm out of interest, why are those 2 lines needed?
+SET NAMES utf8;
+SET FOREIGN_KEY_CHECKS=0; ?
Comment 30 Liz Rea 2013-12-13 01:54:20 UTC
*** Bug 11372 has been marked as a duplicate of this bug. ***
Comment 31 Mark Tompsett 2013-12-13 04:23:48 UTC Comment hidden (obsolete)
Comment 32 Mark Tompsett 2013-12-13 04:29:46 UTC
http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23501&action=diff
is the beginning of the breaking the first patch into smaller pieces. It applies against master, and basically creates the basis for the tests that must continue to succeed at each step.

TEST PLAN
---------
1) git checkout -b bug_7567 origin/master
2) cat t/NewsChannels.t
3) cry that only a use was checked.
4) prove -v t/NewsChannels.t
5) git bz apply 7567
6) prove -v t/db_dependent/NewsChannels.t
7) cat t/NewsChannels.t
8) cheer that all the functions in C4/NewsChannels.pm are called at least once.
Comment 33 Mark Tompsett 2013-12-13 19:52:45 UTC Comment hidden (obsolete)
Comment 34 Mark Tompsett 2013-12-13 20:16:37 UTC Comment hidden (obsolete)
Comment 35 Mark Tompsett 2013-12-13 21:00:37 UTC Comment hidden (obsolete)
Comment 36 Mark Tompsett 2013-12-13 21:25:57 UTC Comment hidden (obsolete)
Comment 37 Mark Tompsett 2013-12-14 04:48:58 UTC Comment hidden (obsolete)
Comment 38 Mark Tompsett 2013-12-16 03:38:08 UTC Comment hidden (obsolete)
Comment 39 Mark Tompsett 2013-12-19 03:41:19 UTC Comment hidden (obsolete)
Comment 40 Mark Tompsett 2013-12-19 04:23:21 UTC
Only last patch to add tomorrow. All the "small" pieces (not the 85.93K patch) so far can apply to 3.14.x and master as "bug fixes". The patch tomorrow will add branch.
Comment 41 Mark Tompsett 2013-12-19 23:10:13 UTC Comment hidden (obsolete)
Comment 42 Mark Tompsett 2013-12-20 06:39:21 UTC Comment hidden (obsolete)
Comment 43 Mark Tompsett 2013-12-20 06:41:56 UTC Comment hidden (obsolete)
Comment 44 Mark Tompsett 2013-12-20 06:44:18 UTC
I've put up my split version now, but I will fully finish testing it and obsoleting the 85.93KB patch later.
Comment 45 Mark Tompsett 2013-12-21 04:16:06 UTC
Comment on attachment 23475 [details] [review]
Bug 7567 - News by Branch

This mega patch has been replaced by all the smaller patches. I think I even fixed up a couple things from it too.
Comment 46 Mark Tompsett 2013-12-21 04:27:23 UTC
I believe this test plan properly tests everything. However, each smaller one should have it's own test plan in the appropriate comment, for those who don't have time to test everything at once.

And really only the last two patches are enhancements, so I would hope that the other 8 patches could make it into 3.14.x and/or earlier too.

REVISED TEST PLAN
-----------------
 0) Test the existing functionality and notice some of the bugs.
    Backup the database if you care about your opac_news. :)
    Apply all the patches.
 1) Do a fresh install, to ensure that branchcode is created as a field
     in the opac_news table.
    - Did you try for something non-English?
    - This would require rolling your own distribution, or
      being lazy and just source'ing kohastructure.sql after dropping
      and recreating the database.
 2) Do an upgrade to confirm that the branchcode field is added
     to the opac_news table.
    - useful MySQL commands: show create table opac_news;
                             describe opac_news;
 3) prove -v t/db_dependent/NewsChannels.t
     This should test all the changes in C4::NewsChannels functions.
 4) Log into the staff client
    - Does the News display on the main page bust?
 5) Add a new dummy branch (so it has no items or borrowers)
 6) Go to Tools->News
    - Are the locations properly display?
 7) Add a News item, for 'All' interfaces for 'All Branches'.
 8) Add a News item, for 'Librarian interface' interfaces for
     the dummy branch.
 9) Add a News item, for 'Slip' interfaces at your user branch.
10) Add a News item, for 'OPAC' interface at your user branch.
11) Change the Location and Branch filters.
    - Do they show what you select after you click the Filter button?
          (Unpatched the Location does not!)
    - Are the items displayed matching what is selected?
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
12) Click 'Edit' for one of the news items added.
    - Does what comes up match what was displayed?
          ( Unpatched the Location may be 'All' )
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
    - Does the submitted values match the changes made?
13) Find a patron to print a slip for. Print Slip.
    - Does both the 'All' and 'Slip' news show up on the print slip?
14) In a new tab, go to OPAC
    - Does the News display on the main page bust?
    - Does it display the news item for a specific branch?
          (It shouldn't when patched, but unpatched it would)
          (Only one of the new things added should be displayed)
15) Log in as yourself on OPAC.
    - Does that piece of News display now too?
          (Both things added should be displayed)
16) In the staff client, delete the dummy branch.
    - Does the 'Librarian interface' news item exist any more?
      It shouldn't.
17) Check the news items added and click the delete button in the
     staff client.
    - Did it delete them appropriately?
Comment 47 Chris Cormack 2014-02-16 20:55:21 UTC
For 3.16 we are making more and more use of DBIX::Class .. which means you need to update the schema files too.

I will sign off these patches, and when you add a follow up doing the Schema change I will sign that off too and set to signed off.
Comment 48 Chris Cormack 2014-02-16 21:01:22 UTC Comment hidden (obsolete)
Comment 49 Chris Cormack 2014-02-16 21:01:27 UTC Comment hidden (obsolete)
Comment 50 Chris Cormack 2014-02-16 21:01:32 UTC Comment hidden (obsolete)
Comment 51 Chris Cormack 2014-02-16 21:01:39 UTC Comment hidden (obsolete)
Comment 52 Chris Cormack 2014-02-16 21:01:44 UTC Comment hidden (obsolete)
Comment 53 Chris Cormack 2014-02-16 21:01:50 UTC Comment hidden (obsolete)
Comment 54 Chris Cormack 2014-02-16 21:01:55 UTC Comment hidden (obsolete)
Comment 55 Chris Cormack 2014-02-16 21:02:03 UTC Comment hidden (obsolete)
Comment 56 Chris Cormack 2014-02-16 21:02:09 UTC Comment hidden (obsolete)
Comment 57 Chris Cormack 2014-02-16 21:02:14 UTC Comment hidden (obsolete)
Comment 58 Mark Tompsett 2014-02-17 03:11:30 UTC
I don't know how to do Schema files. Sorry. Reading links?
Comment 60 Mark Tompsett 2014-02-17 03:46:43 UTC Comment hidden (obsolete)
Comment 61 Mark Tompsett 2014-02-17 04:26:37 UTC Comment hidden (obsolete)
Comment 62 Chris Cormack 2014-02-17 05:17:50 UTC Comment hidden (obsolete)
Comment 63 Jonathan Druart 2014-03-21 11:01:26 UTC
Mark, could you please rebase the last patch please?

fatal: sha1 information is lacking or useless (C4/NewsChannels.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 7567 - Use, Display, Filter News by Branch
Comment 64 Mark Tompsett 2014-03-21 12:37:51 UTC Comment hidden (obsolete)
Comment 65 Mark Tompsett 2014-03-21 12:38:00 UTC Comment hidden (obsolete)
Comment 66 Mark Tompsett 2014-03-21 12:43:49 UTC
Switching to 'signed off' since the last two are only a rebase.
Comment 67 Mark Tompsett 2014-03-22 03:56:31 UTC
For the record, it was bug 11736 that triggered this required rebase.
Comment 68 Jonathan Druart 2014-03-26 12:54:44 UTC
QA comment:

1)
+    $template->param( $lang => 1 ) if $lang;
Why ? Seems not used in template. But maybe I am wrong.

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

3)
+    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;
+    }

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

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.

6)

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)

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

Marked as Failed QA.
Comment 69 Mark Tompsett 2014-03-26 15:13:39 UTC
(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.
Comment 70 Jonathan Druart 2014-03-26 15:54:30 UTC
(In reply to M. Tompsett from comment #69)
> > 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.

In my opinion, the trigger is when we have a call with undef:
+    my ( $opac_news_count, $opac_news ) = &get_opac_news( undef, $lang, $branchcode );

It is not blocker for my QA point of view. Maybe for the RM point of view.
Comment 71 Mark Tompsett 2014-03-26 15:56:14 UTC Comment hidden (obsolete)
Comment 72 Mark Tompsett 2014-03-26 15:56:25 UTC Comment hidden (obsolete)
Comment 73 Mark Tompsett 2014-03-26 15:56:33 UTC Comment hidden (obsolete)
Comment 74 Mark Tompsett 2014-03-26 15:56:48 UTC Comment hidden (obsolete)
Comment 75 Mark Tompsett 2014-03-26 15:57:05 UTC Comment hidden (obsolete)
Comment 76 Mark Tompsett 2014-03-26 15:57:20 UTC Comment hidden (obsolete)
Comment 77 Mark Tompsett 2014-03-26 15:57:32 UTC Comment hidden (obsolete)
Comment 78 Mark Tompsett 2014-03-26 15:57:54 UTC Comment hidden (obsolete)
Comment 79 Mark Tompsett 2014-03-26 15:58:04 UTC Comment hidden (obsolete)
Comment 80 Mark Tompsett 2014-03-26 15:58:17 UTC Comment hidden (obsolete)
Comment 81 Mark Tompsett 2014-03-26 15:58:27 UTC Comment hidden (obsolete)
Comment 82 Mark Tompsett 2014-03-26 16:01:16 UTC
(In reply to Jonathan Druart from comment #70)
> (In reply to M. Tompsett from comment #69)
> > > 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.
> 
> In my opinion, the trigger is when we have a call with undef:
> +    my ( $opac_news_count, $opac_news ) = &get_opac_news( undef, $lang,
> $branchcode );
> 
> It is not blocker for my QA point of view. Maybe for the RM point of view.

Perhaps the problem isn't undef, but rather it probably should be 0, even though it isn't used.
Comment 83 Mark Tompsett 2014-03-26 17:19:29 UTC Comment hidden (obsolete)
Comment 84 Mark Tompsett 2014-03-26 17:19:37 UTC Comment hidden (obsolete)
Comment 85 Mark Tompsett 2014-03-26 17:19:44 UTC Comment hidden (obsolete)
Comment 86 Mark Tompsett 2014-03-26 17:19:55 UTC Comment hidden (obsolete)
Comment 87 Mark Tompsett 2014-03-26 17:20:16 UTC Comment hidden (obsolete)
Comment 88 Mark Tompsett 2014-03-26 17:20:34 UTC Comment hidden (obsolete)
Comment 89 Mark Tompsett 2014-03-26 17:20:52 UTC Comment hidden (obsolete)
Comment 90 Mark Tompsett 2014-03-26 17:21:17 UTC Comment hidden (obsolete)
Comment 91 Mark Tompsett 2014-03-26 17:21:35 UTC Comment hidden (obsolete)
Comment 92 Mark Tompsett 2014-03-26 17:21:53 UTC Comment hidden (obsolete)
Comment 93 Mark Tompsett 2014-03-26 17:22:07 UTC Comment hidden (obsolete)
Comment 94 Mark Tompsett 2014-03-26 17:27:55 UTC Comment hidden (obsolete)
Comment 95 Mark Tompsett 2014-03-26 17:42:43 UTC
The difficulty of a multi-patch bug fix is changing just parts after a code review. Only the ones with [NEEDS SIGN OFF] are changed compared to earlier versions which Chris Cormack signed off.

These changes include:
- chopping an unused template parameter (comment #68 - point 1)
- simplifying the lang/branchcode query building logic (comment #68 - point 3)
- using Library or Libraries instead of Branch or Braches in the template (comment #68 - point 4)
- white space fixes (comment #68 - point 6)
- renaming a variable from hashref to parameters (comment #55)

To recap the other comments by Jonathan Druart:
comment #68 - point 2: parameter is still there, and left for historical and future expansion purposes.
comment #68 - point 5: empty div's can be styled without breaking javascript.
comment #68 - point 7: it probably should be 0, but left undef, so obvious change was only the addition of branch code. I figure 4+ parameters deserve a hashref as a parameter.
Comment 96 Mark Tompsett 2014-03-26 17:43:47 UTC
SIDE COMMENT: Bug 7731 was likely the motivation for comment #68 point 4. :)
Comment 97 Jonathan Druart 2014-03-27 12:24:31 UTC
Created attachment 26631 [details] [review]
Bug 7567 - Testing for C4/NewsChannel.pm

Testing was lacking, this tests every function call. As a
side effect, this moved the stub of a test file from
t/NewsChannels.t to t/db_dependent/NewsChannels.t, since the
table opac_news must exist.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Passes all 8 tests

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 98 Jonathan Druart 2014-03-27 12:24:42 UTC
Created attachment 26632 [details] [review]
Bug 7567 - Update License and use Modern::Perl

Grabbed the current license from
  http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence
and changed the use strict; use warnings; into a
use Modern::Perl instead.

TEST PLAN
---------
1) Log into staff client.
    - Does news look okay?
2) Apply patch
3) Refresh staff client.
    - Does news look the same?

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Safe no op action

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 99 Jonathan Druart 2014-03-27 12:24:51 UTC
Created attachment 26633 [details] [review]
Bug 7567 - Update License tools/koha-news.pl

Updated license text using
    http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence
for the base text.

TEST PLAN
---------
1) less tools/koha-news.pl
2) apply patch
3) less tools/koha-news.pl
    - was any contributor information lost?

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Safe no op action

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 100 Jonathan Druart 2014-03-27 12:25:00 UTC
Created attachment 26634 [details] [review]
Bug 7567 - Clean up sample news files

Some files had truncate. Some files had foreign key checks.
Other files had foreign key checks disabled. Some files gave
the column names expressly, and others assumed the fields in
a particular order. Everything was modified to:
- not truncate
- not say utf8 (because it should already be by default)
- not disable foreign key checks (because it should work)
- expressly name the fields (adding fields shouldn't break it)

TEST PLAN
---------
0) Backup opac_news if you care about your news.
1) open mysql client
2) use the koha database
3) Run the following script
TRUNCATE opac_news;
source ~/kohaclone/installer/data/mysql/uk-UA/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/it-IT/necessari/sample_news.sql
source ~/kohaclone/installer/data/mysql/de-DE/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/nb-NO/2-Valgfritt/sample_news.sql
source ~/kohaclone/installer/data/mysql/en/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/pl-PL/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/es-ES/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/ru-RU/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/fr-FR/2-Optionel/sample_news.sql

- Were there errors?
- Did everything add nicely?
    - Not likely, by expressly stating the idnew field, the news
      items in some files would not be added.

4) apply patch
5) try running the script again

- Were there errors?
- Did everything add nicely?
6) Log into the staff client
7) Tools->News
- Do you see the news items added?

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Much needed clean up, making for much more robust and less error prone
sql

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 101 Jonathan Druart 2014-03-27 12:25:11 UTC
Created attachment 26635 [details] [review]
Bug 7567 - Remove excess line

The template parameter { $lang => 1 } does not seem used in
master. It is unconfirmed whether this is used in earlier
versions of Koha.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 102 Jonathan Druart 2014-03-27 12:25:19 UTC
Created attachment 26636 [details] [review]
Bug 7567 - Correct Filtering and Default dropdown values

In the staff client, when you went to Home -> Tools -> News
the default 'Display location' is 'All'. Everything is correctly
displayed. However, if you change the filter it filters the
table correctly, but does not set the dropdown value to match.
This patch corrects this.

TEST PLAN
---------
 1) Home -> Tools -> News
 2) Change 'Display location' value and click 'Filter'.
    - Is the 'Display location' value the same now?
      (no)
    - Are the correct values displayed?
 3) Click 'New entry'
    - Is it anything other than 'All'?
      (no, and is this correct?)
 4) Click 'Cancel'
 5) Click 'Edit' for various news items with different locations.
    - Does the 'Display location' get proper set?
      (no)
 6) Apply patch
 7) Change the 'Display location' value and click 'Filter'.
    - Does the 'Display location' value stay the same?
      (this should be correct now)
    - Are the correct values displayed?
 8) Change the 'Display location' value to 'All' and click 'Filter'.
    - Does everything display?
 9) Change the 'Display location' value and click 'Filter'.
10) Click 'New entry'
    - Does it match what the filter was?
      (should match now)
11) Click 'Cancel'
12) Click 'Edit' for various news items with different locations.
    - Does the 'Display location' get proper set?
      (this should be correct now)

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 103 Jonathan Druart 2014-03-27 12:25:29 UTC
Created attachment 26637 [details] [review]
Bug 7567 - Removed finish calls

"When all the data has been fetched from a SELECT statement,
the driver will automatically call finish for you. So you should
not call it explicitly except when you know that you've not
fetched all the data from a statement handle and the handle
won't be destroyed soon."
(http://search.cpan.org/~timb/DBI-1.627/DBI.pm#finish)

All the $sth variables were scoped within the functions,
and would be destroyed immediately. Additionally, there was
one after a SELECT, for only a single idnew, and so it was
not necessary.

TEST PLAN
---------
1) prove -v t/db_dependent/NewsChannels.t
2) apply patch
3) prove -v t/db_dependent/NewsChannels.t

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

ok 1 - use C4::NewsChannels;
ok 2 - Successfully added the first dummy news item!
ok 3 - Successfully added the second dummy news item!
ok 4 - Successfully updated second dummy news item!
ok 5 - Successfully tested get_opac_new id1!
ok 6 - Successfully tested get_opac_new id2!
ok 7 - Successfully tested get_opac_news!
ok 8 - Successfully tested GetNewsToDisplay!

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 104 Jonathan Druart 2014-03-27 12:25:38 UTC
Created attachment 26638 [details] [review]
Bug 7567 - Too many parameters and tiny bugs

Changed the add and update functions to use a hash reference
for the parameter, so that adding or subtracting parameters
should be easier. Added some POD for the add_opac_news and
upd_opac_news functions, so that developers would know how to
call it.

The hashref changes resulted in being able to return 0 for
failure and 1 for success. This meant adding a couple tests
to the test file.

And while testing, there was some sort of logic problem with
the matter of '' being all, but selecting all only showed
things set for all, and excluded particular languages, or other
interfaces.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 105 Jonathan Druart 2014-03-27 12:25:47 UTC
Created attachment 26639 [details] [review]
Bug 7567 - Added branchcode to opac_news

This patch merely adds branchcode varchar(10) DEFAULT NULL to
the opac_news table.

TEST PLAN
---------
 1) backup your kohadata base if you care about the data.
 2) use the koha database
 3) describe opac_news;
 4) show create table opac_news;
    -- No branchcode constraint will exist.
 5) apply the patch
 6) upgrade the database (either staff client or script)
 7) use the koha database
 8) describe opac_news;
 9) show create table opac_news;
    -- The branchcode constraint should be listed.
10) drop that koha database
11) create the koha database
12) use the koha database
13) source ~/kohaclone/installer/data/mysql/kohastructure.sql
    -- there should be no errors in creating the database.
14) describe opac_news;
15) show create table opac_news;
    -- The branchcode constraint should be listed.
16) restore your koha database if you backed it up.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 106 Jonathan Druart 2014-03-27 12:25:55 UTC
Created attachment 26640 [details] [review]
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: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 107 Jonathan Druart 2014-03-27 12:26:06 UTC
Created attachment 26641 [details] [review]
Bug 7567 - Schema update for opac_news

This was generated by:
Reset hard to master.
Restore old DB
Apply the patches for 7567
./installer/data/mysql/updatedatabase.pl
./misc/devel/update_dbix_class_files.pl -db_name=... -db_user=... -db_passwd=...
git add Koha/Schema/Results/OpacNews.pm

This should provide the necessary changes to the schema files for
Koha 3.16 and forward.

Had to generate under Debian, because Ubuntu's was broken by comparison.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 108 Jonathan Druart 2014-03-27 12:26:14 UTC
Created attachment 26642 [details] [review]
Bug 7567: QA followup: FIX SQL query

The WHERE clause should not erase $query.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 109 Jonathan Druart 2014-03-27 12:26:23 UTC
Created attachment 26643 [details] [review]
Bug 7567: QA followup: remove useless $parameters hashref

This hashref is useless, it is only used once.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 110 Jonathan Druart 2014-03-27 12:27:53 UTC
(In reply to M. Tompsett from comment #95)
> The difficulty of a multi-patch bug fix is changing just parts after a code
> review. Only the ones with [NEEDS SIGN OFF] are changed compared to earlier
> versions which Chris Cormack signed off.

It is why it is preferable not to modify signed off patches.
It is really hard for QAers to know what changes you introduced. Next time, prefer a followup, QAer can squash patches.

I squashed 2 patches
  Bug 7567 - Undefined $lang triggers warning
 [NEED SIGN OFF] Bug 7567 - Remove excess line
into 
    Bug 7567 - Remove excess line

And I provided 2 followups:
1/ Fix the sql query on getting rows from DB
2/ remove an unnecessary variable.

Marked as Passed QA.
Comment 111 Owen Leonard 2014-03-27 13:35:31 UTC Comment hidden (obsolete)
Comment 112 Jonathan Druart 2014-03-27 13:50:14 UTC
Created attachment 26647 [details] [review]
Bug 7567 - Modify DataTables configuration

This follow-up corrects a JavaScript error by converting the sorting
configuration to be class-based
(http://wiki.koha-community.org/wiki/DataTables_HowTo#Sorting_dates_regardless_of_date_format_preference).

I have also changed the DataTables configuration to allow for pagination
and JS-based filtering.

To test, apply the patch and view the list of News items. Your browser
should report no JavaScript errors. Table sorting, pagination, and
filtering should work correctly.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 113 Jonathan Druart 2014-03-27 13:50:49 UTC
(In reply to Owen Leonard from comment #111)
> Created attachment 26644 [details] [review] [review]

Good catch Owen, thanks!
Comment 114 Galen Charlton 2014-04-07 18:18:15 UTC
Pushed to master.  Thanks, Mark!
Comment 115 Marc Véron 2014-08-09 08:57:07 UTC
*** Bug 6745 has been marked as a duplicate of this bug. ***