Bug 7567: convert news add/update routines to take hashref; fix bugs
authorMark Tompsett <mtompset@hotmail.com>
Mon, 16 Dec 2013 05:41:47 +0000 (00:41 -0500)
committerGalen Charlton <gmc@esilibrary.com>
Mon, 7 Apr 2014 18:13:56 +0000 (18:13 +0000)
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>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>
C4/NewsChannels.pm
t/db_dependent/NewsChannels.t [changed mode: 0755->0644]
tools/koha-news.pl

index cc931d2..b9aa8ad 100644 (file)
@@ -3,8 +3,7 @@ package C4::NewsChannels;
 # This file is part of Koha.
 #
 # Copyright (C) 2000-2002  Katipo Communications
-#
-# This file is part of Koha.
+# Copyright (C) 2013       Mark Tompsett
 #
 # Koha is free software; you can redistribute it and/or modify it
 # under the terms of the GNU General Public License as published by
@@ -26,12 +25,12 @@ use C4::Dates qw(format_date);
 use vars qw($VERSION @ISA @EXPORT);
 
 BEGIN { 
-    $VERSION = 3.07.00.049;    # set the version for version checking
-       @ISA = qw(Exporter);
-       @EXPORT = qw(
-               &GetNewsToDisplay
-               &add_opac_new &upd_opac_new &del_opac_new &get_opac_new &get_opac_news
-       );
+    $VERSION = 3.07.00.049;    # set the version for version checking
+    @ISA = qw(Exporter);
+    @EXPORT = qw(
+        &GetNewsToDisplay
+        &add_opac_new &upd_opac_new &del_opac_new &get_opac_new &get_opac_news
+    );
 }
 
 =head1 NAME
@@ -46,29 +45,69 @@ This module provides the functions needed to mange OPAC and intranet news.
 
 =cut
 
+=head2 add_opac_new
+
+    $retval = add_opac_new($hashref);
+
+    $hashref should contains all the fields found in opac_news,
+    except idnew. The idnew field is auto-generated.
+
+=cut
+
 sub add_opac_new {
-    my ($title, $new, $lang, $expirationdate, $timestamp, $number) = @_;
-    my $dbh = C4::Context->dbh;
-    my $sth = $dbh->prepare("INSERT INTO opac_news (title, new, lang, expirationdate, timestamp, number) VALUES (?,?,?,?,?,?)");
-    $sth->execute($title, $new, $lang, $expirationdate, $timestamp, $number);
-    return 1;
+    my ($href_entry) = @_;
+    my $retval = 0;
+
+    if ($href_entry) {
+        my @fields = keys %{$href_entry};
+        my @values = values %{$href_entry};
+        my $field_string = join ',',@fields;
+        $field_string = $field_string // q{};
+        my $values_string = '?,' x ($#fields) . '?';
+        my $dbh = C4::Context->dbh;
+        my $sth = $dbh->prepare("INSERT INTO opac_news ( $field_string ) VALUES ( $values_string )");
+        $sth->execute(@values);
+        $retval = 1;
+    }
+    return $retval;
 }
 
+=head2 upd_opac_new
+
+    $retval = upd_opac_new($hashref);
+
+    $hashref should contains all the fields found in opac_news,
+    including idnew, since it is the key for the SQL UPDATE.
+
+=cut
+
 sub upd_opac_new {
-    my ($idnew, $title, $new, $lang, $expirationdate, $timestamp,$number) = @_;
-    my $dbh = C4::Context->dbh;
-    my $sth = $dbh->prepare("
-        UPDATE opac_news SET 
-            title = ?,
-            new = ?,
-            lang = ?,
-            expirationdate = ?,
-            timestamp = ?,
-            number = ?
-        WHERE idnew = ?
-    ");
-    $sth->execute($title, $new, $lang, $expirationdate, $timestamp,$number,$idnew);
-    return 1;
+    my ($href_entry) = @_;
+    my $retval = 0;
+
+    if ($href_entry) {
+        # take the keys of hash entry and make a list, but...
+        my @fields = keys %{$href_entry};
+        my @values;
+        $#values = -1;
+        my $field_string = q{};
+        foreach my $field_name (@fields) {
+            # exclude idnew
+            if ( $field_name ne 'idnew' ) {
+                $field_string = $field_string . "$field_name = ?,";
+                push @values,$href_entry->{$field_name};
+            }
+        }
+        # put idnew at the end, so we know which record to update
+        push @values,$href_entry->{'idnew'};
+        chop $field_string; # remove that excess ,
+
+        my $dbh = C4::Context->dbh;
+        my $sth = $dbh->prepare("UPDATE opac_news SET $field_string WHERE idnew = ?;");
+        $sth->execute(@values);
+        $retval = 1;
+    }
+    return $retval;
 }
 
 sub del_opac_new {
@@ -86,7 +125,8 @@ sub del_opac_new {
 sub get_opac_new {
     my ($idnew) = @_;
     my $dbh = C4::Context->dbh;
-    my $sth = $dbh->prepare("SELECT * FROM opac_news WHERE idnew = ?");
+    my $query = q{ SELECT * FROM opac_news WHERE idnew = ? };
+    my $sth = $dbh->prepare($query);
     $sth->execute($idnew);
     my $data = $sth->fetchrow_hashref;
     $data->{$data->{'lang'}} = 1 if defined $data->{lang};
@@ -97,17 +137,19 @@ sub get_opac_new {
 
 sub get_opac_news {
     my ($limit, $lang) = @_;
+    my @values;
     my $dbh = C4::Context->dbh;
-    my $query = "SELECT *, timestamp AS newdate FROM opac_news";
+    my $query = q{ SELECT *, timestamp AS newdate FROM opac_news };
     if ($lang) {
-        $query.= " WHERE lang = '" .$lang ."' ";
+        $query.= " WHERE (lang='' OR lang=?)";
+        push @values,$lang;
     }
-    $query.= " ORDER BY timestamp DESC ";
+    $query.= ' ORDER BY timestamp DESC ';
     #if ($limit) {
-    #    $query.= "LIMIT 0, " . $limit;
+    #    $query.= 'LIMIT 0, ' . $limit;
     #}
     my $sth = $dbh->prepare($query);
-    $sth->execute();
+    $sth->execute(@values);
     my @opac_news;
     my $count = 0;
     while (my $row = $sth->fetchrow_hashref) {
@@ -128,26 +170,29 @@ sub get_opac_news {
 =cut
 
 sub GetNewsToDisplay {
-    my $lang = shift;
+    my ($lang) = @_;
     my $dbh = C4::Context->dbh;
     # SELECT *,DATE_FORMAT(timestamp, '%d/%m/%Y') AS newdate
-    my $query = "
+    my $query = q{
      SELECT *,timestamp AS newdate
      FROM   opac_news
      WHERE   (
         expirationdate >= CURRENT_DATE()
         OR    expirationdate IS NULL
         OR    expirationdate = '00-00-0000'
-      )
-      AND   `timestamp` <= CURRENT_DATE()
-      AND   lang = ?
-      ORDER BY number
-    ";                         # expirationdate field is NOT in ISO format?
+     )
+     AND   `timestamp` < CURRENT_DATE()+1
+     AND   (lang = '' OR lang = ?)
+     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);
     my @results;
     while ( my $row = $sth->fetchrow_hashref ){
-               $row->{newdate} = format_date($row->{newdate});
+        $row->{newdate} = format_date($row->{newdate});
         push @results, $row;
     }
     return \@results;
old mode 100755 (executable)
new mode 100644 (file)
index 261abd5..0e34cb0
@@ -3,10 +3,18 @@
 use Modern::Perl;
 use C4::Dates qw(format_date);
 use C4::Branch qw(GetBranchName);
-use Test::More tests => 8;
+use Test::More tests => 10;
+use Readonly;
+
+Readonly my $F1 => 1;
+Readonly my $F2 => 2;
+Readonly my $F3 => 3;
+Readonly my $F4 => 4;
+Readonly my $F5 => 5;
+Readonly my $F6 => 6;
 
 BEGIN {
-        use_ok('C4::NewsChannels');
+    use_ok('C4::NewsChannels');
 }
 
 my $dbh = C4::Context->dbh;
@@ -16,77 +24,94 @@ $dbh->{AutoCommit} = 0;
 $dbh->{RaiseError} = 1;
 
 # Test add_opac_new
+my $rv = add_opac_new();    # intentionally bad
+ok( $rv == 0, 'Correctly failed on no parameter!' );
+
 my $timestamp = '2000-01-01';
 my ( $timestamp1, $timestamp2 ) = ( $timestamp, $timestamp );
-my ($title1,         $new1,
-    $lang1, $expirationdate1, $number1) =
-   ( 'News Title',  '<p>We have some exciting news!</p>',
-    '',    '2999-12-30',    1 );
-my $rv = add_opac_new( $title1, $new1, $lang1, $expirationdate1, $timestamp1, $number1 );
-ok($rv==1,"Successfully added the first dummy news item!");
-
-my ($title2,         $new2,
-    $lang2, $expirationdate2, $number2) =
-   ( 'News Title2', '<p>We have some exciting news!</p>',
-    '',    '2999-12-31',    1 );
-$rv = add_opac_new( $title2, $new2, $lang2, $expirationdate2, $timestamp2, $number2 );
-ok($rv==1,"Successfully added the second dummy news item!");
+my ( $title1, $new1, $lang1, $expirationdate1, $number1 ) =
+  ( 'News Title', '<p>We have some exciting news!</p>', q{}, '2999-12-30', 1 );
+my $href_entry1 = {
+    title          => $title1,
+    new            => $new1,
+    lang           => $lang1,
+    expirationdate => $expirationdate1,
+    timestamp      => $timestamp1,
+    number         => $number1,
+};
+
+$rv = add_opac_new($href_entry1);
+ok( $rv == 1, 'Successfully added the first dummy news item!' );
+
+my ( $title2, $new2, $lang2, $expirationdate2, $number2 ) =
+  ( 'News Title2', '<p>We have some exciting news!</p>', q{}, '2999-12-31', 1 );
+my $href_entry2 = {
+    title          => $title2,
+    new            => $new2,
+    lang           => $lang2,
+    expirationdate => $expirationdate2,
+    timestamp      => $timestamp2,
+    number         => $number2,
+};
+$rv = add_opac_new($href_entry2);
+ok( $rv == 1, 'Successfully added the second dummy news item!' );
 
 # We need to determine the idnew in a non-MySQLism way.
 # This should be good enough.
-my $sth = $dbh->prepare(q{
-  SELECT idnew from opac_news
-  WHERE timestamp='2000-01-01' AND
-        expirationdate='2999-12-30';
-                        });
+my $query =
+q{ SELECT idnew from opac_news WHERE timestamp='2000-01-01' AND expirationdate='2999-12-30'; };
+my $sth = $dbh->prepare($query);
 $sth->execute();
 my $idnew1 = $sth->fetchrow;
-$sth = $dbh->prepare(q{
-  SELECT idnew from opac_news
-  WHERE timestamp='2000-01-01' AND
-        expirationdate='2999-12-31';
-                      });
+$query =
+q{ SELECT idnew from opac_news WHERE timestamp='2000-01-01' AND expirationdate='2999-12-31'; };
+$sth = $dbh->prepare($query);
 $sth->execute();
 my $idnew2 = $sth->fetchrow;
 
 # Test upd_opac_new
-$new2 = '<p>Update! There is no news!</p>';
-$rv = upd_opac_new( $idnew2, $title2, $new2, $lang2, $expirationdate2, $timestamp2, $number2 );
-ok($rv==1,"Successfully updated second dummy news item!");
+$rv = upd_opac_new();    # intentionally bad parmeters
+ok( $rv == 0, 'Correctly failed on no parameter!' );
+
+$new2                 = '<p>Update! There is no news!</p>';
+$href_entry2->{new}   = $new2;
+$href_entry2->{idnew} = $idnew2;
+$rv                   = upd_opac_new($href_entry2);
+ok( $rv == 1, 'Successfully updated second dummy news item!' );
 
 # Test get_opac_new (single news item)
-$timestamp1 = format_date( $timestamp1 );
-$expirationdate1 = format_date( $expirationdate1 );
-$timestamp2 = format_date( $timestamp2 );
-$expirationdate2 = format_date( $expirationdate2 );
+$timestamp1      = format_date($timestamp1);
+$expirationdate1 = format_date($expirationdate1);
+$timestamp2      = format_date($timestamp2);
+$expirationdate2 = format_date($expirationdate2);
 
 my $hashref_check = get_opac_new($idnew1);
-my $failure = 0;
-if ($hashref_check->{title}          ne $title1)          { $failure = 1; }
-if ($hashref_check->{new}            ne $new1)            { $failure = 1; }
-if ($hashref_check->{lang}           ne $lang1)           { $failure = 1; }
-if ($hashref_check->{expirationdate} ne $expirationdate1) { $failure = 1; }
-if ($hashref_check->{timestamp}      ne $timestamp1)      { $failure = 1; }
-if ($hashref_check->{number}         ne $number1)         { $failure = 1; }
-ok($failure==0,"Successfully tested get_opac_new id1!");
+my $failure       = 0;
+if ( $hashref_check->{title}          ne $title1 )          { $failure = $F1; }
+if ( $hashref_check->{new}            ne $new1 )            { $failure = $F2; }
+if ( $hashref_check->{lang}           ne $lang1 )           { $failure = $F3; }
+if ( $hashref_check->{expirationdate} ne $expirationdate1 ) { $failure = $F4; }
+if ( $hashref_check->{timestamp}      ne $timestamp1 )      { $failure = $F5; }
+if ( $hashref_check->{number}         ne $number1 )         { $failure = $F6; }
+ok( $failure == 0, "Successfully tested get_opac_new id1 ($failure)!" );
 
 # Test get_opac_new (single news item)
 $hashref_check = get_opac_new($idnew2);
-$failure = 0;
-if ($hashref_check->{title}          ne $title2)          { $failure = 1; }
-if ($hashref_check->{new}            ne $new2)            { $failure = 1; }
-if ($hashref_check->{lang}           ne $lang2)           { $failure = 1; }
-if ($hashref_check->{expirationdate} ne $expirationdate2) { $failure = 1; }
-if ($hashref_check->{timestamp}      ne $timestamp2)      { $failure = 1; }
-if ($hashref_check->{number}         ne $number2)         { $failure = 1; }
-ok($failure==0,"Successfully tested get_opac_new id2!");
+$failure       = 0;
+if ( $hashref_check->{title}          ne $title2 )          { $failure = $F1; }
+if ( $hashref_check->{new}            ne $new2 )            { $failure = $F2; }
+if ( $hashref_check->{lang}           ne $lang2 )           { $failure = $F3; }
+if ( $hashref_check->{expirationdate} ne $expirationdate2 ) { $failure = $F4; }
+if ( $hashref_check->{timestamp}      ne $timestamp2 )      { $failure = $F5; }
+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,'');
-ok($opac_news_count>=2,"Successfully tested get_opac_news!");
+my ( $opac_news_count, $arrayref_opac_news ) = get_opac_news( 0, q{} );
+ok( $opac_news_count >= 2, 'Successfully tested get_opac_news!' );
 
 # Test GetNewsToDisplay
-($opac_news_count, $arrayref_opac_news) = GetNewsToDisplay('');
-ok($opac_news_count>=2,"Successfully tested GetNewsToDisplay!");
+( $opac_news_count, $arrayref_opac_news ) = GetNewsToDisplay(q{});
+ok( $opac_news_count >= 2, 'Successfully tested GetNewsToDisplay!' );
 
 $dbh->rollback;
index 5578b31..057a601 100755 (executable)
@@ -87,11 +87,28 @@ if ( $op eq 'add_form' ) {
     }
 }
 elsif ( $op eq 'add' ) {
-    add_opac_new( $title, $new, $lang, $expirationdate, $timestamp, $number );
+    my $parameters = {
+                      title          => $title,
+                      new            => $new,
+                      lang           => $lang,
+                      expirationdate => $expirationdate,
+                      timestamp      => $timestamp,
+                      number         => $number,
+                  };
+    add_opac_new( $parameters );
     print $cgi->redirect("/cgi-bin/koha/tools/koha-news.pl");
 }
 elsif ( $op eq 'edit' ) {
-    upd_opac_new( $id, $title, $new, $lang, $expirationdate, $timestamp ,$number );
+    my $parameters = {
+                      idnew          => $id,
+                      title          => $title,
+                      new            => $new,
+                      lang           => $lang,
+                      expirationdate => $expirationdate,
+                      timestamp      => $timestamp,
+                      number         => $number,
+                  };
+    upd_opac_new( $parameters );
     print $cgi->redirect("/cgi-bin/koha/tools/koha-news.pl");
 }
 elsif ( $op eq 'del' ) {