From a6f1622cf235a97aa8a056685e2a1c5c7bda7988 Mon Sep 17 00:00:00 2001 From: Alex Arnaud Date: Wed, 21 Feb 2018 13:10:50 +0000 Subject: [PATCH] Bug 10382 - Course reserves: handle empty values Test Plan: 1) Create an item, do not set a collection code 2) Add the item to a course, and choose to set a collection code 3) Ensure the course is enabled, and the collection code is now visible 4) Disable the course, ensure the collection code is no longer visible --- C4/CourseReserves.pm | 83 +++++------------------------ t/db_dependent/CourseReserves/CourseItems.t | 78 +++++++++++++++++++++++++-- 2 files changed, 85 insertions(+), 76 deletions(-) diff --git a/C4/CourseReserves.pm b/C4/CourseReserves.pm index 19ed685..dac6f04 100644 --- a/C4/CourseReserves.pm +++ b/C4/CourseReserves.pm @@ -459,11 +459,7 @@ sub ModCourseItem { my (%params) = @_; warn identify_myself(%params) if $DEBUG; - my $itemnumber = $params{'itemnumber'}; - my $itype = $params{'itype'}; - my $ccode = $params{'ccode'}; - my $holdingbranch = $params{'holdingbranch'}; - my $location = $params{'location'}; + my $itemnumber = $params{'itemnumber'}; return unless ($itemnumber); @@ -503,10 +499,8 @@ sub _AddCourseItem { push( @values, $params{'itemnumber'} ); foreach (@FIELDS) { - if ( $params{$_} ) { - push( @fields, "$_ = ?" ); - push( @values, $params{$_} ); - } + push( @fields, "$_ = ?" ); + push( @values, $params{$_} || undef ); } my $query = "INSERT INTO course_items SET " . join( ',', @fields ); @@ -530,10 +524,6 @@ sub _UpdateCourseItem { my $ci_id = $params{'ci_id'}; my $course_item = $params{'course_item'}; - my $itype = $params{'itype'}; - my $ccode = $params{'ccode'}; - my $holdingbranch = $params{'holdingbranch'}; - my $location = $params{'location'}; return unless ( $ci_id || $course_item ); @@ -541,49 +531,13 @@ sub _UpdateCourseItem { unless ($course_item); $ci_id = $course_item->{'ci_id'} unless ($ci_id); - ## Revert fields that had an 'original' value, but now don't - ## Update the item fields to the stored values from course_items - ## and then set those fields in course_items to NULL - my @fields_to_revert; - foreach (@FIELDS) { - if ( !$params{$_} && $course_item->{$_} ) { - push( @fields_to_revert, $_ ); - } - } - _RevertFields( - ci_id => $ci_id, - fields => \@fields_to_revert, - course_item => $course_item - ) if (@fields_to_revert); - - ## Update fields that still have an original value, but it has changed - ## This necessitates only changing the current item values, as we still - ## have the original values stored in course_items - my %mod_params; - foreach (@FIELDS) { - if ( $params{$_} - && $course_item->{$_} - && $params{$_} ne $course_item->{$_} ) { - $mod_params{$_} = $params{$_}; - } - } - ModItem( \%mod_params, undef, $course_item->{'itemnumber'} ) if %mod_params; - ## Update fields that didn't have an original value, but now do - ## We must save the original value in course_items, and also - ## update the item fields to the new value - my $item = GetItem( $course_item->{'itemnumber'} ); - my %mod_params_new; - my %mod_params_old; + my %mod_params; foreach (@FIELDS) { - if ( $params{$_} && !$course_item->{$_} ) { - $mod_params_new{$_} = $params{$_}; - $mod_params_old{$_} = $item->{$_}; - } + $mod_params{$_} = $params{$_}; } - _ModStoredFields( 'ci_id' => $params{'ci_id'}, %mod_params_old ); - ModItem( \%mod_params_new, undef, $course_item->{'itemnumber'} ) if %mod_params_new; + ModItem( \%mod_params, undef, $course_item->{'itemnumber'} ); } =head2 _ModStoredFields @@ -627,31 +581,18 @@ sub _RevertFields { my (%params) = @_; warn identify_myself(%params) if $DEBUG; - my $ci_id = $params{'ci_id'}; - my $course_item = $params{'course_item'}; - my $fields = $params{'fields'}; - my @fields = @$fields; + my $ci_id = $params{'ci_id'}; return unless ($ci_id); - $course_item = GetCourseItem( ci_id => $params{'ci_id'} ) - unless ($course_item); + my $course_item = GetCourseItem( ci_id => $params{'ci_id'} ); my $mod_item_params; - my @fields_to_null; - foreach my $field (@fields) { - foreach (@FIELDS) { - if ( $field eq $_ && $course_item->{$_} ) { - $mod_item_params->{$_} = $course_item->{$_}; - push( @fields_to_null, $_ ); - } - } + foreach my $field ( @FIELDS ) { + $mod_item_params->{$field} = $course_item->{$field}; } - ModItem( $mod_item_params, undef, $course_item->{'itemnumber'} ) if $mod_item_params && %$mod_item_params; - my $query = "UPDATE course_items SET " . join( ',', map { "$_=NULL" } @fields_to_null ) . " WHERE ci_id = ?"; - - C4::Context->dbh->do( $query, undef, $ci_id ) if (@fields_to_null); + ModItem( $mod_item_params, undef, $course_item->{'itemnumber'} ) if $mod_item_params && %$mod_item_params; } =head2 _SwapAllFields @@ -736,7 +677,7 @@ sub DelCourseItem { return unless ($ci_id); - _RevertFields( ci_id => $ci_id, fields => \@FIELDS ); + _RevertFields( ci_id => $ci_id ); my $query = " DELETE FROM course_items diff --git a/t/db_dependent/CourseReserves/CourseItems.t b/t/db_dependent/CourseReserves/CourseItems.t index c2bb237..45b4833 100644 --- a/t/db_dependent/CourseReserves/CourseItems.t +++ b/t/db_dependent/CourseReserves/CourseItems.t @@ -23,7 +23,7 @@ use C4::CourseReserves qw/ModCourseItem ModCourseReserve DelCourseReserve GetCou use C4::Context; use Koha::Items; -use Test::More tests => 17; +use Test::More tests => 27; BEGIN { require_ok('C4::CourseReserves'); @@ -37,10 +37,6 @@ my $builder = t::lib::TestBuilder->new(); create_dependent_objets(); my ($biblionumber, $itemnumber) = create_bib_and_item(); -my $course = $builder->build({ - source => 'CourseReserve', -}); - my $ci_id = ModCourseItem( itemnumber => $itemnumber, itype => 'BK_foo', @@ -49,6 +45,13 @@ my $ci_id = ModCourseItem( location => 'TH', ); +my $course = $builder->build({ + source => 'CourseReserve', + value => { + ci_id => $ci_id, + } +}); + my $cr_id = ModCourseReserve( course_id => $course->{course_id}, ci_id => $ci_id, @@ -68,6 +71,33 @@ is($item->ccode, 'BOOK', 'Item ccode in course should be BOOK'); is($item->holdingbranch, 'B2', 'Item holding branch in course should be B2'); is($item->location, 'TH', 'Item location in course should be TH'); +ModCourseItem( + itemnumber => $itemnumber, + itype => 'BK_foo', + ccode => 'DVD', + holdingbranch => 'B3', + location => 'TH', +); + +ModCourseReserve( + course_id => $course->{course_id}, + ci_id => $ci_id, + staff_note => '', + public_note => '', +); + +$course_item = GetCourseItem( ci_id => $ci_id ); +is($course_item->{itype}, 'CD_foo', 'Course item itype should be CD_foo'); +is($course_item->{ccode}, 'CD', 'Course item ccode should be CD'); +is($course_item->{holdingbranch}, 'B1', 'Course item holding branch should be B1'); +is($course_item->{location}, 'HR', 'Course item location should be HR'); + +$item = Koha::Items->find($itemnumber); +is($item->itype, 'BK_foo', 'Item type in course should be BK_foo'); +is($item->ccode, 'DVD', 'Item ccode in course should be DVD'); +is($item->holdingbranch, 'B3', 'Item holding branch in course should be B3'); +is($item->location, 'TH', 'Item location in course should be TH'); + DelCourseReserve( cr_id => $cr_id ); $item = Koha::Items->find($itemnumber); is($item->itype, 'CD_foo', 'Item type removed from course should be set back to CD_foo'); @@ -102,6 +132,27 @@ is($item->ccode, 'BOOK', 'Item ccode should be BOOK'); my $course_item2 = GetCourseItem( ci_id => $ci_id2 ); is($course_item2->{ccode}, '', 'Course item ccode should be empty'); +ModCourseItem( + itemnumber => $itemnumber, + itype => 'CD_foo', + ccode => 'DVD', + holdingbranch => 'B1', + location => 'HR', +); + +ModCourseReserve( + course_id => $course->{course_id}, + ci_id => $ci_id2, + staff_note => '', + public_note => '', +); + +$item = Koha::Items->find($itemnumber); +is($item->ccode, 'DVD', 'Item ccode should be BOOK'); + +$course_item2 = GetCourseItem( ci_id => $ci_id2 ); +is($course_item2->{ccode}, '', 'Course item ccode should be empty'); + DelCourseReserve( cr_id => $cr_id2 ); $item = Koha::Items->find($itemnumber); is($item->ccode, '', 'Item ccode should be set back to empty'); @@ -142,6 +193,14 @@ sub create_dependent_objets { }); $builder->build({ + source => 'Branch', + value => { + branchcode => 'B3', + branchname => 'Branch 3' + } + }); + + $builder->build({ source => 'AuthorisedValue', value => { category => 'CCODE', @@ -154,6 +213,15 @@ sub create_dependent_objets { source => 'AuthorisedValue', value => { category => 'CCODE', + authorised_value => 'DVD', + lib => 'Book' + } + }); + + $builder->build({ + source => 'AuthorisedValue', + value => { + category => 'CCODE', authorised_value => 'CD', lib => 'Compact Disk' } -- 2.7.4