Attachment 25323 Details for Bug 7567 – Bug 7567 - Too many parameters and tiny bugs

[patch] Bug 7567 - Too many parameters and tiny bugs

Bug-7567---Too-many-parameters-and-tiny-bugs.patch (text/plain), 16.37 KB, created by Chris Cormack on 2014-02-16 21:02:03 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Cormack

Created: 2014-02-16 21:02:03 UTC

Size: 16.37 KB

patch

obsolete

>From 9406691b568306187264ea343c48781fe84478d3 Mon Sep 17 00:00:00 2001
>From: Mark Tompsett <mtompset@hotmail.com>
>Date: Mon, 16 Dec 2013 00:41:47 -0500
>Subject: [PATCH] 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: 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!
>ok 10 - Successfully tested GetNewsToDisplay!
>
>My only nitpick here is the variable naming ... hashref is a really
>bad name for a variable. But I will sign off, because apart from that
>it is a good clean up
>---
> C4/NewsChannels.pm | 127 +++++++++++++++++++++++++++--------------
> t/db_dependent/NewsChannels.t | 129 +++++++++++++++++++++++++-----------------
> tools/koha-news.pl | 21 ++++++-
> 3 files changed, 182 insertions(+), 95 deletions(-)
> mode change 100755 => 100644 t/db_dependent/NewsChannels.t
>
>diff --git a/C4/NewsChannels.pm b/C4/NewsChannels.pm
>index b7e9855..1e060ff 100644
>--- a/C4/NewsChannels.pm
>+++ b/C4/NewsChannels.pm
>@@ -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) {
>@@ -130,26 +172,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;
>diff --git a/t/db_dependent/NewsChannels.t b/t/db_dependent/NewsChannels.t
>old mode 100755
>new mode 100644
>index 261abd5..0e34cb0
>--- a/t/db_dependent/NewsChannels.t
>+++ b/t/db_dependent/NewsChannels.t
>@@ -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', 'We have some exciting news!',
>- '', '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', 'We have some exciting news!',
>- '', '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', 'We have some exciting news!', 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', 'We have some exciting news!', 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 = 'Update! There is no news!';
>-$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 = 'Update! There is no news!';
>+$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;
>diff --git a/tools/koha-news.pl b/tools/koha-news.pl
>index 58ee573..5170433 100755
>--- a/tools/koha-news.pl
>+++ b/tools/koha-news.pl
>@@ -87,11 +87,28 @@ if ( $op eq 'add_form' ) {
> }
> }
> elsif ( $op eq 'add' ) {
>- add_opac_new( $title, $new, $lang, $expirationdate, $timestamp, $number );
>+ my $hashref = {
>+ title => $title,
>+ new => $new,
>+ lang => $lang,
>+ expirationdate => $expirationdate,
>+ timestamp => $timestamp,
>+ number => $number,
>+ };
>+ add_opac_new( $hashref );
> 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 $hashref = {
>+ idnew => $id,
>+ title => $title,
>+ new => $new,
>+ lang => $lang,
>+ expirationdate => $expirationdate,
>+ timestamp => $timestamp,
>+ number => $number,
>+ };
>+ upd_opac_new( $hashref );
> print $cgi->redirect("/cgi-bin/koha/tools/koha-news.pl");
> }
> elsif ( $op eq 'del' ) {
>-- 
>1.8.5.3

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