From 1e4954e043a5394bff87514801bcaf1742d24bb5 Mon Sep 17 00:00:00 2001 From: Yohann Dufour Date: Thu, 7 Aug 2014 16:40:13 +0200 Subject: [PATCH] Bug 10363: Add a package for authorised values Currently, the authorised values are anarchically managed :) We have to have a better way to manage them. I propose a package in order to execute the sql queries into a package instead of directly into the pl script. This development adds: - A new unique key (category and authorised values) for the authorised_values table. - 1 new package Koha::AuthorisedValue - 1 new unit test file in order to test these 2 packages. It is a code source evolution, but there is no change for the final user. Test plan: 1/ Apply the patches 2/ Check that existing authorised values is displayed on your admin page (admin/authorised_values.pl). 3/ Update an existing one 4/ Create a new category 5/ Create a new AV for this category 6/ Create another one with the same value, an error should be occur. 7/ Delete one AV. 8/ Click everywhere on the authorised values admin page and try to find a bug. 9/ Launch t/db_dependent/AuthorisedValues.t and check that all tests pass (1 sql error appears, it is normal). --- Koha/AuthorisedValue.pm | 208 +++++++++++++ admin/authorised_values.pl | 321 ++++++++------------- .../prog/en/modules/admin/authorised_values.tt | 101 ++++--- 3 files changed, 382 insertions(+), 248 deletions(-) create mode 100644 Koha/AuthorisedValue.pm diff --git a/Koha/AuthorisedValue.pm b/Koha/AuthorisedValue.pm new file mode 100644 index 0000000..9ce051f --- /dev/null +++ b/Koha/AuthorisedValue.pm @@ -0,0 +1,208 @@ +package Koha::AuthorisedValue; + +use Modern::Perl; +use List::MoreUtils qw( any uniq ); + +use base qw(Class::Accessor); + +use C4::Context; +use Koha::Database; + +my $rs_av = Koha::Database->new()->schema->resultset('AuthorisedValue'); +my $rs_avb = Koha::Database->new()->schema->resultset('AuthorisedValuesBranch'); + +sub fetch { + my ( $self ) = @_; + + my $av; + if( $self->{id} ) { + $av = $rs_av->find($self->{id}); + } + else { + $av = $rs_av->find({ + category => $self->{category}, + authorised_value => $self->{authorised_value}, + }); + } + die unless $av; + + my %iv = $av->get_columns; + my $data = \%iv; + + $self->{id} = $data->{id}; + $self->{category} = $data->{category}; + $self->{authorised_value} = $data->{authorised_value}; + $self->{lib} = $data->{lib}; + $self->{lib_opac} = $data->{lib_opac}; + $self->{imageurl} = $data->{imageurl}; + return $self; +} + +sub update { + my ( $self ) = @_; + die "There is no id defined for this authorised value. I cannot update it" unless $self->{id}; + + $rs_av->find($self->{id})->update({ + category => $self->{category}, + authorised_value => $self->{authorised_value}, + lib => $self->{lib}, + lib_opac => $self->{lib_opac}, + imageurl => $self->{imageurl}, + }); + + $rs_avb->search({ av_id => $self->{id} })->delete; + for my $branch ( @{ $self->{branches_limitations} } ) { + next unless $branch->{branchcode}; + $rs_avb->create({ + av_id => $self->{id}, + branchcode => $branch->{branchcode}, + }); + } +} + +sub delete { + my ( $self ) = @_; + $rs_av->find( $self->{id} )->delete; + return 1; +} + +sub insert { + my ( $self ) = @_; + $self->{id} = $rs_av->create({ + category => $self->{category}, + authorised_value => $self->{authorised_value}, + lib => $self->{lib}, + lib_opac => $self->{lib_opac}, + imageurl => $self->{imageurl}, + })->id; + + for my $branch ( @{ $self->{branches_limitations} } ) { + next unless $branch->{branchcode}; + $rs_avb->create({ + av_id => $self->{id}, + branchcode => $branch->{branchcode}, + }); + } +} + +sub all { + my ( $self ) = @_; + return $self->search(); +} + +sub search { + my ( $self, $filters ) = @_; + + my $rs = $rs_av->search( $filters ); + $rs->result_class('DBIx::Class::ResultClass::HashRefInflator'); + return wantarray ? ( $rs->all ) : [ $rs->all ]; +} + +sub categories { + my ( $self ) = @_; + my @avs = $self->all; + my @categories = uniq map { + $_->{category} + } @avs; + + return wantarray ? @categories : \@categories; +} + +sub branches_limitations { + my ( $self, $limitations ) = @_; + my $dbh = C4::Context->dbh; + unless ( defined $limitations ) { + my $rs = $rs_avb->search({ + av_id => $self->{id}, + },{ + join => 'branchcode', + }); + + my @limitations; + while( my $avb = $rs->next ) { + push @limitations, { + branchcode => $avb->branchcode->branchcode, + branchname => $avb->branchcode->branchname, + } + } + $self->{branches_limitations} = \@limitations; + return $self->{branches_limitations}; + } else { + die "setter not implemented yet"; + } +} + +1; + +__END__ + +=head1 NAME + +Koha::AuthorisedValue + +=head1 SYNOPSIS + + use Koha::AuthorisedValue; + my $av1 = Koha::AuthorisedValue->new({id => $id}); + my $av2 = Koha::AuthorisedValue->new({av => $av}); + my $av3 = Koha::AuthorisedValue->new({category => $category, authorised_value => $authorised_value}); + $av1->fetch; + $av2->delete; + +=head1 DESCRIPTION + +Class for managing an authorised value into Koha. + +=head1 METHODS + +=head2 new + +Create a new Koha::AuthorisedValue object. No sql query is done in this method. + +=head2 fetch + +The information will be retrieved from the database. + +=head2 update + +If the AuthorisedValue object has been modified and the values have to be modified into the database, call this method. + +=head2 delete + +Remove a the record in the database using the id the object. + +=head2 insert + +Insert a new AuthorisedValue object into the database. + +=head2 branches_limitations + +In the interest of being pleasant with the performance, the branches limitations is not retrieve with the fetch method. +If they are required, you should call this method. + +=head1 AUTHOR + +Jonathan Druart + +=head1 COPYRIGHT + +Copyright 2013 BibLibre + +=head1 LICENSE + +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 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. + +=cut diff --git a/admin/authorised_values.pl b/admin/authorised_values.pl index 8a44256..2708868 100755 --- a/admin/authorised_values.pl +++ b/admin/authorised_values.pl @@ -1,6 +1,7 @@ #!/usr/bin/perl # Copyright 2000-2002 Katipo Communications +# Copyright 2013 BibLibre # # This file is part of Koha. # @@ -17,8 +18,7 @@ # with Koha; if not, write to the Free Software Foundation, Inc., # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. -use strict; -use warnings; +use Modern::Perl; use CGI; use C4::Auth; @@ -26,32 +26,16 @@ use C4::Branch; use C4::Context; use C4::Koha; use C4::Output; +use Koha::AuthorisedValue; -sub AuthorizedValuesForCategory { - my ($searchstring) = shift or return; - my $dbh = C4::Context->dbh; - $searchstring=~ s/\'/\\\'/g; - my @data=split(' ',$searchstring); - my $sth=$dbh->prepare(' - SELECT id, category, authorised_value, lib, lib_opac, imageurl - FROM authorised_values - WHERE (category = ?) - ORDER BY category, authorised_value - '); - $sth->execute("$data[0]"); - return $sth->fetchall_arrayref({}); -} - my $input = new CGI; my $id = $input->param('id'); -my $op = $input->param('op') || ''; -our $offset = $input->param('offset') || 0; -our $searchfield = $input->param('searchfield'); +my $op = $input->param('op') || 'list'; +my $searchfield = $input->param('searchfield'); $searchfield = '' unless defined $searchfield; $searchfield =~ s/\,//g; -our $script_name = "/cgi-bin/koha/admin/authorised_values.pl"; -our $dbh = C4::Context->dbh; +my @messages; our ($template, $borrowernumber, $cookie)= get_template_and_user({ template_name => "admin/authorised_values.tt", @@ -62,31 +46,22 @@ our ($template, $borrowernumber, $cookie)= get_template_and_user({ debug => 1, }); -$template->param( script_name => $script_name, - ($op||'else') => 1 ); ################## ADD_FORM ################################## # called by default. Used to create form to add or modify a record if ($op eq 'add_form') { - my $data; - my @selected_branches; - if ($id) { - my $sth=$dbh->prepare("select id, category, authorised_value, lib, lib_opac, imageurl from authorised_values where id=?"); - $sth->execute($id); - $data=$sth->fetchrow_hashref; - $sth = $dbh->prepare("SELECT b.branchcode, b.branchname FROM authorised_values_branches AS avb, branches AS b WHERE avb.branchcode = b.branchcode AND avb.av_id = ?;"); - $sth->execute( $id ); - while ( my $branch = $sth->fetchrow_hashref ) { - push @selected_branches, $branch; - } - } else { - $data->{'category'} = $input->param('category'); - } + my ( $selected_branches, $category, $av ); + if ($id) { + $av = Koha::AuthorisedValue->new( { id => $id } )->fetch; + $selected_branches = $av->branches_limitations; + } else { + $category = $input->param('category'); + } my $branches = GetBranches; my @branches_loop; foreach my $branch (sort keys %$branches) { - my $selected = ( grep {$_->{branchcode} eq $branch} @selected_branches ) ? 1 : 0; + my $selected = ( grep {$_->{branchcode} eq $branch} @$selected_branches ) ? 1 : 0; push @branches_loop, { branchcode => $branches->{$branch}{branchcode}, branchname => $branches->{$branch}{branchname}, @@ -96,152 +71,108 @@ if ($op eq 'add_form') { if ($id) { $template->param(action_modify => 1); - $template->param('heading_modify_authorized_value_p' => 1); - } elsif ( ! $data->{'category'} ) { + } elsif ( ! $category ) { $template->param(action_add_category => 1); - $template->param('heading_add_new_category_p' => 1); } else { $template->param(action_add_value => 1); - $template->param('heading_add_authorized_value_p' => 1); } - $template->param('use_heading_flags_p' => 1); - $template->param( category => $data->{'category'}, - authorised_value => $data->{'authorised_value'}, - lib => $data->{'lib'}, - lib_opac => $data->{'lib_opac'}, - id => $data->{'id'}, - imagesets => C4::Koha::getImageSets( checked => $data->{'imageurl'} ), - offset => $offset, - branches_loop => \@branches_loop, - ); - -################## ADD_VALIDATE ################################## -# called by add_form, used to insert/modify data in DB -} elsif ($op eq 'add_validate') { + + $template->param( + category => ( $av ? $av->{category} : $category ), + authorised_value => $av->{authorised_value}, + lib => $av->{lib}, + lib_opac => $av->{lib_opac}, + id => $av->{id}, + imagesets => C4::Koha::getImageSets( checked => $av->{imageurl} ), + branches_loop => \@branches_loop, + ); + +} elsif ($op eq 'add') { my $new_authorised_value = $input->param('authorised_value'); my $new_category = $input->param('category'); my $imageurl = $input->param( 'imageurl' ) || ''; - $imageurl = '' if $imageurl =~ /removeImage/; + $imageurl = '' if $imageurl =~ /removeImage/; my $duplicate_entry = 0; my @branches = $input->param('branches'); if ( $id ) { # Update - my $sth = $dbh->prepare( "SELECT category, authorised_value FROM authorised_values WHERE id = ? "); - $sth->execute($id); - my ($category, $authorised_value) = $sth->fetchrow_array(); - if ( $authorised_value ne $new_authorised_value ) { - my $sth = $dbh->prepare_cached( "SELECT COUNT(*) FROM authorised_values " . - "WHERE category = ? AND authorised_value = ? and id <> ? "); - $sth->execute($new_category, $new_authorised_value, $id); - ($duplicate_entry) = $sth->fetchrow_array(); - } - unless ( $duplicate_entry ) { - my $sth=$dbh->prepare( 'UPDATE authorised_values - SET category = ?, - authorised_value = ?, - lib = ?, - lib_opac = ?, - imageurl = ? - WHERE id=?' ); - my $lib = $input->param('lib'); - my $lib_opac = $input->param('lib_opac'); - undef $lib if ($lib eq ""); # to insert NULL instead of a blank string - undef $lib_opac if ($lib_opac eq ""); # to insert NULL instead of a blank string - $sth->execute($new_category, $new_authorised_value, $lib, $lib_opac, $imageurl, $id); - if ( @branches ) { - $sth = $dbh->prepare("DELETE FROM authorised_values_branches WHERE av_id = ?"); - $sth->execute( $id ); - $sth = $dbh->prepare( - "INSERT INTO authorised_values_branches - ( av_id, branchcode ) - VALUES ( ?, ? )" - ); - for my $branchcode ( @branches ) { - next if not $branchcode; - $sth->execute($id, $branchcode); - } + my $av = Koha::AuthorisedValue->new( { id => $id } ); + + # TODO in updatedatabase and kohastructure + # Add a uniq key on auv.category and av.authorised_values + #if ( $av->{authorised_value} ne $new_authorised_value ) { + # my $sth = $dbh->prepare_cached( "SELECT COUNT(*) FROM authorised_values " . + # "WHERE category = ? AND authorised_value = ? and id <> ? "); + # $sth->execute($new_category, $new_authorised_value, $id); + # ($duplicate_entry) = $sth->fetchrow_array(); + #} + #unless ( $duplicate_entry ) { + $av->{lib} = $input->param('lib') || undef; + $av->{lib_opac} = $input->param('lib_opac') || undef; + $av->{category} = $new_category; + $av->{authorised_value} = $new_authorised_value; + $av->{imageurl} = $imageurl; + $av->{id} = $id; + $av->{branches_limitations} = [ + map { {branchcode => $_, branchname => undef} } @branches + ]; + if ( $@ ) { + push @messages, {type => 'error', code => 'error_on_update' }; + } else { + push @messages, { type => 'message', code => 'success_on_update' }; } - $sth->finish; - print "Content-Type: text/html\n\n"; - exit; - } + $av->update; + $template->param( update_success => 1 ); + #} } else { # Insert - my $sth = $dbh->prepare_cached( "SELECT COUNT(*) FROM authorised_values " . - "WHERE category = ? AND authorised_value = ? "); - $sth->execute($new_category, $new_authorised_value); - ($duplicate_entry) = $sth->fetchrow_array(); - unless ( $duplicate_entry ) { - my $sth=$dbh->prepare( 'INSERT INTO authorised_values - ( category, authorised_value, lib, lib_opac, imageurl ) - values (?, ?, ?, ?, ?)' ); - my $lib = $input->param('lib'); - my $lib_opac = $input->param('lib_opac'); - undef $lib if ($lib eq ""); # to insert NULL instead of a blank string - undef $lib_opac if ($lib_opac eq ""); # to insert NULL instead of a blank string - $sth->execute( $new_category, $new_authorised_value, $lib, $lib_opac, $imageurl ); - $id = $dbh->{'mysql_insertid'}; - if ( @branches ) { - $sth = $dbh->prepare( - "INSERT INTO authorised_values_branches - ( av_id, branchcode ) - VALUES ( ?, ? )" - ); - for my $branchcode ( @branches ) { - next if not $branchcode; - $sth->execute($id, $branchcode); - } - } - print "Content-Type: text/html\n\nparam('category')."&offset=$offset\">"; - exit; + my $av = Koha::AuthorisedValue->new( { + category => $new_category, + authorised_value => $new_authorised_value, + lib => $input->param('lib') || undef, + lib_opac => $input->param('lib_opac') || undef, + imageurl => $imageurl, + branches_limitations => [ map { {branchcode => $_, branchname => undef} } @branches ], + } ); + eval {$av->insert}; + if ( $@ ) { + push @messages, {type => 'error', code => 'error_on_insert' }; + } else { + push @messages, { type => 'message', code => 'success_on_insert' }; } } - if ( $duplicate_entry ) { + if ( $duplicate_entry ) { $template->param(duplicate_category => $new_category, - duplicate_value => $new_authorised_value, - else => 1); - default_form(); - } - -################## DELETE_CONFIRM ################################## -# called by default form, used to confirm deletion of data in DB -} elsif ($op eq 'delete_confirm') { - my $sth=$dbh->prepare("select category,authorised_value,lib,lib_opac from authorised_values where id=?"); - $sth->execute($id); - my $data=$sth->fetchrow_hashref; - $id = $input->param('id') unless $id; - $template->param(searchfield => $searchfield, - Tlib => $data->{'lib'}, - Tlib_opac => $data->{'lib_opac'}, - Tvalue => $data->{'authorised_value'}, - id =>$id, - ); + duplicate_value => $new_authorised_value); + } - # END $OP eq DELETE_CONFIRM -################## DELETE_CONFIRMED ################################## -# called by delete_confirm, used to effectively confirm deletion of data in DB -} elsif ($op eq 'delete_confirmed') { - my $id = $input->param('id'); - my $sth=$dbh->prepare("delete from authorised_values where id=?"); - $sth->execute($id); - print "Content-Type: text/html\n\n"; - exit; - # END $OP eq DELETE_CONFIRMED -################## DEFAULT ################################## -} else { # DEFAULT - default_form(); -} #---- END $OP eq DEFAULT -output_html_with_http_headers $input, $cookie, $template->output; + $op = 'list'; + $searchfield = $new_category; +} elsif ($op eq 'delete') { + my $av = Koha::AuthorisedValue->new( { id => $input->param('id') } ); + eval {$av->delete}; + if ( $@ ) { + push @messages, {type => 'error', code => 'error_on_delete' }; + } else { + push @messages, { type => 'message', code => 'success_on_delete' }; + } + + $op = 'list'; + $template->param( delete_success => 1 ); +} -exit 0; +$template->param( + op => $op, + searchfield => $searchfield, + messages => \@messages, +); -sub default_form { +if ( $op eq 'list' ) { # build categories list - my $sth = $dbh->prepare("select distinct category from authorised_values"); - $sth->execute; + my $avs = Koha::AuthorisedValue->new; my @category_list; my %categories; # a hash, to check that some hardcoded categories exist. - while ( my ($category) = $sth->fetchrow_array ) { + for my $category ( $avs->categories ) { push( @category_list, $category ); $categories{$category} = 1; } @@ -251,44 +182,32 @@ sub default_form { push @category_list, $_ unless $categories{$_}; } - #reorder the list - @category_list = sort {$a cmp $b} @category_list; - my $tab_list = CGI::scrolling_list(-name=>'searchfield', - -id=>'searchfield', - -values=> \@category_list, - -default=>"", - -size=>1, - -multiple=>0, - ); - if (!$searchfield) { - $searchfield=$category_list[0]; - } - my ($results) = AuthorizedValuesForCategory($searchfield); - my $count = scalar(@$results); - my @loop_data = (); - # builds value list - my $dbh = C4::Context->dbh; - $sth = $dbh->prepare("SELECT b.branchcode, b.branchname FROM authorised_values_branches AS avb, branches AS b WHERE avb.branchcode = b.branchcode AND avb.av_id = ?"); - for (my $i=0; $i < $count; $i++){ - $sth->execute( $results->[$i]{id} ); - my @selected_branches; - while ( my $branch = $sth->fetchrow_hashref ) { - push @selected_branches, $branch; - } - my %row_data; # get a fresh hash for the row data - $row_data{category} = $results->[$i]{'category'}; - $row_data{authorised_value} = $results->[$i]{'authorised_value'}; - $row_data{lib} = $results->[$i]{'lib'}; - $row_data{lib_opac} = $results->[$i]{'lib_opac'}; - $row_data{imageurl} = getitemtypeimagelocation( 'intranet', $results->[$i]{'imageurl'} ); - $row_data{edit} = "$script_name?op=add_form&id=".$results->[$i]{'id'}."&offset=$offset"; - $row_data{delete} = "$script_name?op=delete_confirm&searchfield=$searchfield&id=".$results->[$i]{'id'}."&offset=$offset"; - $row_data{branches} = \@selected_branches; - push(@loop_data, \%row_data); - } + #reorder the list + @category_list = sort {$a cmp $b} @category_list; + + $searchfield ||= $category_list[0]; + + my $avs_for_category = $avs->search( { category => $searchfield } ); + my @loop_data = (); + # builds value list + for my $av ( @$avs_for_category ) { + my $branches = Koha::AuthorisedValue->new($av)->branches_limitations; + my %row_data; # get a fresh hash for the row data + $row_data{category} = $av->{category}; + $row_data{authorised_value} = $av->{authorised_value}; + $row_data{lib} = $av->{lib}; + $row_data{lib_opac} = $av->{lib_opac}; + $row_data{imageurl} = getitemtypeimagelocation( 'intranet', $av->{imageurl} ); + $row_data{branches} = $branches; + $row_data{id} = $av->{id}; + push(@loop_data, \%row_data); + } - $template->param( loop => \@loop_data, - tab_list => $tab_list, - category => $searchfield ); -} + $template->param( + loop => \@loop_data, + category => $searchfield, + categories => \@category_list, + ); +} +output_html_with_http_headers $input, $cookie, $template->output; diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/authorised_values.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/authorised_values.tt index 9e96c34..b1c5e39 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/authorised_values.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/authorised_values.tt @@ -1,9 +1,11 @@ [% INCLUDE 'doc-head-open.inc' %] -Koha › Administration › Authorized values [% IF ( add_form ) %] › [% IF ( action_modify ) %]Modify authorized value[% END %] - [% IF ( action_add_value ) %] › New authorized value[% END %] - [% IF ( action_add_category ) %] › New category[% END %][% END %] -[% IF ( delete_confirm ) %] › Confirm deletion[% END %] -[% IF ( else ) %]Authorized values[% END %] +Koha › Administration › Authorized values +[% IF op == 'add_form' %] + [% IF ( action_modify ) %] › Modify authorized value[% END %] + [% IF ( action_add_value ) %] › New authorized value[% END %] + [% IF ( action_add_category ) %] › New category[% END %] +[% END %] + [% INCLUDE 'doc-head-close.inc' %] @@ -23,11 +25,15 @@ $("#branches option:first").attr("selected", "selected"); } $('#icons').tabs(); + + $("a.delete").click(function(){ + return confirm(_("Are you sure you want to delete this authorised value?")); + }); }); //]]> -[% IF ( else ) %] +[% IF op == 'list' %]