From 07eefd07b635cc50b5210bccf5c56e283db63680 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Fri, 14 Feb 2020 15:22:17 -0500 Subject: [PATCH] Bug 23727: Editing course reserve items is broken Adding an item to course reserves and trying to edit any values in a second step does not work. Values are not saved and the table shows all values as "Unchanged". This patch set adds two new sets of columns to the course_items table. The first set determines if the specified column should be swapped or not. The was previously 'implied' by the column being set to undef which has been the root problem with that way of knowing if a column should swap or not. The second set of new columns are for storing the item field values while the item is on course reserve. Previously, the column values were swapped between the items table and the course_items table, which leaves ambiguity as to what each value is. Now, the original columns *always* store the value when the item is on course reserve, and the new storage columns store the original item value while the item is on reserve, and are NULL when an item is *not* on reserve. Test Plan: 1) Apply this patch 2) Add and edit course items, not the new checkboxes for enabling fields 3) Everything should function as before Signed-off-by: Martin Renvoize Signed-off-by: Joy Nelson --- C4/CourseReserves.pm | 172 +++++++++--------- Koha/Course/Instructors.pm | 2 +- Koha/Course/Item.pm | 36 ++++ Koha/Course/Items.pm | 2 +- Koha/Course/Reserves.pm | 2 +- course_reserves/add_items.pl | 32 +++- course_reserves/batch_add_items.pl | 19 +- .../course_reserves/add_items-step2.tt | 91 +++++++-- .../course_reserves/batch_add_items.tt | 41 ++++- .../modules/course_reserves/course-details.tt | 24 +-- t/db_dependent/CourseReserves/CourseItems.t | 101 ++++++---- 11 files changed, 344 insertions(+), 178 deletions(-) diff --git a/C4/CourseReserves.pm b/C4/CourseReserves.pm index 2c130af192..9146e7e9be 100644 --- a/C4/CourseReserves.pm +++ b/C4/CourseReserves.pm @@ -23,6 +23,11 @@ use C4::Context; use C4::Items qw(ModItem); use C4::Circulation qw(GetOpenIssue); +use Koha::Courses; +use Koha::Course::Instructors; +use Koha::Course::Items; +use Koha::Course::Reserves; + use vars qw(@ISA @EXPORT @EXPORT_OK %EXPORT_TAGS $DEBUG @FIELDS); BEGIN { @@ -79,19 +84,17 @@ sub GetCourse { my ($course_id) = @_; warn whoami() . "( $course_id )" if $DEBUG; - my $query = "SELECT * FROM courses WHERE course_id = ?"; - my $dbh = C4::Context->dbh; - my $sth = $dbh->prepare($query); - $sth->execute($course_id); - - my $course = $sth->fetchrow_hashref(); + my $course = Koha::Courses->find( $course_id ); + return undef unless $course; + $course = $course->unblessed; - $query = " + my $dbh = C4::Context->dbh; + my $query = " SELECT b.* FROM course_instructors ci LEFT JOIN borrowers b ON ( ci.borrowernumber = b.borrowernumber ) WHERE course_id = ? "; - $sth = $dbh->prepare($query); + my $sth = $dbh->prepare($query); $sth->execute($course_id); $course->{'instructors'} = $sth->fetchall_arrayref( {} ); @@ -305,7 +308,7 @@ sub EnableOrDisableCourseItem { ## or disable and already disabled item, ## as that would cause the fields to swap if ( $course_item->{'enabled'} ne $enabled ) { - _SwapAllFields($ci_id); + _SwapAllFields($ci_id, $enabled ); my $query = " UPDATE course_items @@ -493,23 +496,20 @@ sub _AddCourseItem { my (%params) = @_; warn identify_myself(%params) if $DEBUG; - my ( @fields, @values ); + $params{holdingbranch} ||= undef; # Can't be empty string, FK constraint - push( @fields, 'itemnumber = ?' ); - push( @values, $params{'itemnumber'} ); + my %data = map { $_ => $params{$_} } @FIELDS; + my %enabled = map { $_ . "_enabled" => $params{ $_ . "_enabled" } } @FIELDS; - foreach (@FIELDS) { - push( @fields, "$_ = ?" ); - push( @values, $params{$_} || undef ); - } - - my $query = "INSERT INTO course_items SET " . join( ',', @fields ); - my $dbh = C4::Context->dbh; - $dbh->do( $query, undef, @values ); - - my $ci_id = $dbh->last_insert_id( undef, undef, 'course_items', 'ci_id' ); + my $ci = Koha::Course::Item->new( + { + itemnumber => $params{itemnumber}, + %data, + %enabled, + } + )->store(); - return $ci_id; + return $ci->id; } =head2 _UpdateCourseItem @@ -525,57 +525,34 @@ sub _UpdateCourseItem { my $ci_id = $params{'ci_id'}; my $course_item = $params{'course_item'}; - return unless ( $ci_id || $course_item ); - - $course_item = GetCourseItem( ci_id => $ci_id ) - unless ($course_item); - $ci_id = $course_item->{'ci_id'} unless ($ci_id); - - my %mod_params = - map { - defined $params{$_} && $params{$_} ne '' - ? ( $_ => $params{$_} ) - : () - } @FIELDS; + $params{holdingbranch} ||= undef; # Can't be empty string, FK constraint - ModItem( \%mod_params, undef, $course_item->{'itemnumber'} ); -} - -=head2 _ModStoredFields - - _ModStoredFields( %params ); - - Updates the values for the 'original' fields in course_items - for a given ci_id - -=cut + return unless ( $ci_id || $course_item ); -sub _ModStoredFields { - my (%params) = @_; - warn identify_myself(%params) if $DEBUG; + $course_item = Koha::Course::Items->find( $ci_id || $course_item->{ci_id} ); - return unless ( $params{'ci_id'} ); + my %data = map { $_ => $params{$_} } @FIELDS; + my %enabled = map { $_ . "_enabled" => $params{ $_ . "_enabled" } } @FIELDS; - my ( @fields_to_update, @values_to_update ); + $course_item->update( { %data, %enabled } ); + if ( $course_item->is_enabled ) { + my $item_fields = {}; + $item_fields->{itype} = $course_item->itype if $course_item->itype_enabled; + $item_fields->{ccode} = $course_item->ccode if $course_item->ccode_enabled; + $item_fields->{location} = $course_item->location if $course_item->location_enabled; + $item_fields->{holdingbranch} = $course_item->holdingbranch if $course_item->holdingbranch_enabled; - foreach (@FIELDS) { - if ( defined($params{$_}) ) { - push( @fields_to_update, $_ ); - push( @values_to_update, $params{$_} ); - } + ModItem( $item_fields, undef, $course_item->itemnumber ) if keys %$item_fields; } - my $query = "UPDATE course_items SET " . join( ',', map { "$_=?" } @fields_to_update ) . " WHERE ci_id = ?"; - - C4::Context->dbh->do( $query, undef, @values_to_update, $params{'ci_id'} ) - if (@values_to_update); - } =head2 _RevertFields _RevertFields( ci_id => $ci_id, fields => \@fields_to_revert ); + Copies fields from course item storage back to the actual item + =cut sub _RevertFields { @@ -584,17 +561,23 @@ sub _RevertFields { my $ci_id = $params{'ci_id'}; - return unless ($ci_id); + return unless $ci_id; - my $course_item = GetCourseItem( ci_id => $params{'ci_id'} ); + my $course_item = Koha::Course::Items->find( $ci_id ); - my $mod_item_params; - foreach my $field ( @FIELDS ) { - next unless defined $course_item->{$field}; - $mod_item_params->{$field} = $course_item->{$field}; - } + my $item_fields = {}; + $item_fields->{itype} = $course_item->itype_storage if $course_item->itype_enabled; + $item_fields->{ccode} = $course_item->ccode_storage if $course_item->ccode_enabled; + $item_fields->{location} = $course_item->location_storage if $course_item->location_enabled; + $item_fields->{holdingbranch} = $course_item->holdingbranch_storage if $course_item->holdingbranch_enabled; - ModItem( $mod_item_params, undef, $course_item->{'itemnumber'} ) if $mod_item_params && %$mod_item_params; + ModItem( $item_fields, undef, $course_item->itemnumber ) if keys %$item_fields; + + $course_item->itype_storage(undef); + $course_item->ccode_storage(undef); + $course_item->location_storage(undef); + $course_item->holdingbranch_storage(undef); + $course_item->store(); } =head2 _SwapAllFields @@ -604,23 +587,41 @@ sub _RevertFields { =cut sub _SwapAllFields { - my ($ci_id) = @_; + my ( $ci_id, $enabled ) = @_; warn "C4::CourseReserves::_SwapFields( $ci_id )" if $DEBUG; - my $course_item = GetCourseItem( ci_id => $ci_id ); - my $item = Koha::Items->find($course_item->{'itemnumber'}); - - my %course_item_fields; - my %item_fields; - foreach (@FIELDS) { - if ( defined( $course_item->{$_} ) ) { - $course_item_fields{$_} = $course_item->{$_}; - $item_fields{$_} = $item->$_ || q{}; - } + my $course_item = Koha::Course::Items->find( $ci_id ); + my $item = Koha::Items->find( $course_item->itemnumber ); + + if ( $enabled eq 'yes' ) { # Copy item fields to course item storage, course item fields to item + $course_item->itype_storage( $item->effective_itemtype ) if $course_item->itype_enabled; + $course_item->ccode_storage( $item->ccode ) if $course_item->ccode_enabled; + $course_item->location_storage( $item->location ) if $course_item->location_enabled; + $course_item->holdingbranch_storage( $item->holdingbranch ) if $course_item->holdingbranch_enabled; + $course_item->store(); + + my $item_fields = {}; + $item_fields->{itype} = $course_item->itype if $course_item->itype_enabled; + $item_fields->{ccode} = $course_item->ccode if $course_item->ccode_enabled; + $item_fields->{location} = $course_item->location if $course_item->location_enabled; + $item_fields->{holdingbranch} = $course_item->holdingbranch if $course_item->holdingbranch_enabled; + + ModItem( $item_fields, undef, $course_item->itemnumber ) if keys %$item_fields; + } else { # Copy course item storage to item + my $item_fields = {}; + $item_fields->{itype} = $course_item->itype_storage if $course_item->itype_enabled; + $item_fields->{ccode} = $course_item->ccode_storage if $course_item->ccode_enabled; + $item_fields->{location} = $course_item->location_storage if $course_item->location_enabled; + $item_fields->{holdingbranch} = $course_item->holdingbranch_storage if $course_item->holdingbranch_enabled; + + ModItem( $item_fields, undef, $course_item->itemnumber ) if keys %$item_fields; + + $course_item->itype_storage(undef); + $course_item->ccode_storage(undef); + $course_item->location_storage(undef); + $course_item->holdingbranch_storage(undef); + $course_item->store(); } - - ModItem( \%course_item_fields, undef, $course_item->{'itemnumber'} ) if %course_item_fields; - _ModStoredFields( %item_fields, ci_id => $ci_id ); } =head2 GetCourseItems { @@ -677,9 +678,10 @@ sub DelCourseItem { my $ci_id = $params{'ci_id'}; - return unless ($ci_id); + my $course_item = Koha::Course::Items->find( $ci_id ); + return unless $course_item; - _RevertFields( ci_id => $ci_id ); + _RevertFields( ci_id => $ci_id ) if $course_item->enabled eq 'yes'; my $query = " DELETE FROM course_items diff --git a/Koha/Course/Instructors.pm b/Koha/Course/Instructors.pm index 418ec4f717..68211eb793 100644 --- a/Koha/Course/Instructors.pm +++ b/Koha/Course/Instructors.pm @@ -19,7 +19,7 @@ use Modern::Perl; use Carp; -use Koha::Course; +use Koha::Course::Instructor; use base qw(Koha::Objects); diff --git a/Koha/Course/Item.pm b/Koha/Course/Item.pm index 7f9cd917d6..948a90c84b 100644 --- a/Koha/Course/Item.pm +++ b/Koha/Course/Item.pm @@ -21,12 +21,48 @@ use Carp; use base qw(Koha::Object); +use Koha::Course::Reserves; + =head1 NAME Koha::Course::Item - Koha Course Item Object class =head1 API +=head2 Methods + +=head3 is_enabled + +=cut + +sub is_enabled { + my ( $self ) = @_; + + return $self->enabled eq 'yes'; +} + +=head3 course_reserves + +=cut + +sub course_reserves { + my ($self) = @_; + + my $rs = $self->_result->course_reserves; + return Koha::Course::Reserves->_new_from_dbic( $rs ); +} + +=head3 item + +=cut + +sub item { + my ($self) = @_; + + my $rs = $self->_result->itemnumber; + return Koha::Item->_new_from_dbic( $rs ); +} + =head2 Internal methods =cut diff --git a/Koha/Course/Items.pm b/Koha/Course/Items.pm index efd06084b9..7fc713dee1 100644 --- a/Koha/Course/Items.pm +++ b/Koha/Course/Items.pm @@ -19,7 +19,7 @@ use Modern::Perl; use Carp; -use Koha::Course; +use Koha::Course::Item; use base qw(Koha::Objects); diff --git a/Koha/Course/Reserves.pm b/Koha/Course/Reserves.pm index c23a7a95ee..631ba49a93 100644 --- a/Koha/Course/Reserves.pm +++ b/Koha/Course/Reserves.pm @@ -19,7 +19,7 @@ use Modern::Perl; use Carp; -use Koha::Course; +use Koha::Course::Reserve; use base qw(Koha::Objects); diff --git a/course_reserves/add_items.pl b/course_reserves/add_items.pl index 0819e4787f..2a7a1e85d5 100755 --- a/course_reserves/add_items.pl +++ b/course_reserves/add_items.pl @@ -39,12 +39,14 @@ my $action = $cgi->param('action') || ''; my $course_id = $cgi->param('course_id') || ''; my $barcode = $cgi->param('barcode') || ''; my $return = $cgi->param('return') || ''; -my $itemnumber = ($cgi->param('itemnumber') && $action eq 'lookup') ? $cgi->param('itemnumber') : ''; +my $itemnumber = $cgi->param('itemnumber') || ''; my $is_edit = $cgi->param('is_edit') || ''; $barcode =~ s/^\s*|\s*$//g; #remove leading/trailing whitespace my $item = Koha::Items->find( { ( $itemnumber ? ( itemnumber => $itemnumber ) : ( barcode => $barcode ) ) } ); +$itemnumber = $item->id if $item; + my $title = ($item) ? $item->biblio->title : undef; my $step = ( $action eq 'lookup' && $item ) ? '2' : '1'; @@ -67,12 +69,12 @@ if ( !$item && $action eq 'lookup' ){ $template->param( course => GetCourse($course_id) ); if ( $action eq 'lookup' and $item ) { - my $course_item = GetCourseItem( itemnumber => $item->itemnumber ); + my $course_item = Koha::Course::Items->find({ itemnumber => $item->id }); my $course_reserve = ($course_item) ? GetCourseReserve( course_id => $course_id, - ci_id => $course_item->{'ci_id'} + ci_id => $course_item->ci_id, ) : undef; @@ -91,12 +93,26 @@ if ( $action eq 'lookup' and $item ) { ); } elsif ( $action eq 'add' ) { + my $itype = scalar $cgi->param('itype'); + my $ccode = scalar $cgi->param('ccode'); + my $holdingbranch = scalar $cgi->param('holdingbranch'); + my $location = scalar $cgi->param('location'); + + my $itype_enabled = scalar $cgi->param('itype_enabled') ? 1 : 0; + my $ccode_enabled = scalar $cgi->param('ccode_enabled') ? 1 : 0; + my $holdingbranch_enabled = scalar $cgi->param('holdingbranch_enabled') ? 1 : 0; + my $location_enabled = scalar $cgi->param('location_enabled') ? 1 : 0; + my $ci_id = ModCourseItem( - itemnumber => scalar $cgi->param('itemnumber'), - itype => scalar $cgi->param('itype'), - ccode => scalar $cgi->param('ccode'), - holdingbranch => scalar $cgi->param('holdingbranch'), - location => scalar $cgi->param('location'), + itemnumber => $itemnumber, + itype => $itype, + ccode => $ccode, + holdingbranch => $holdingbranch, + location => $location, + itype_enabled => $itype_enabled, + ccode_enabled => $ccode_enabled, + holdingbranch_enabled => $holdingbranch_enabled, + location_enabled => $location_enabled, ); my $cr_id = ModCourseReserve( diff --git a/course_reserves/batch_add_items.pl b/course_reserves/batch_add_items.pl index 76d0caa0b5..2a924b6f35 100755 --- a/course_reserves/batch_add_items.pl +++ b/course_reserves/batch_add_items.pl @@ -42,6 +42,11 @@ my $location = $cgi->param('location'); my $staff_note = $cgi->param('staff_note'); my $public_note = $cgi->param('public_note'); +my $itype_enabled = scalar $cgi->param('itype_enabled') ? 1 : 0; +my $ccode_enabled = scalar $cgi->param('ccode_enabled') ? 1 : 0; +my $holdingbranch_enabled = scalar $cgi->param('holdingbranch_enabled') ? 1 : 0; +my $location_enabled = scalar $cgi->param('location_enabled') ? 1 : 0; + my ( $template, $loggedinuser, $cookie ) = get_template_and_user( { template_name => "course_reserves/batch_add_items.tt", @@ -78,11 +83,15 @@ if ( $course_id && $course ) { foreach my $item (@items) { my $ci_id = ModCourseItem( - itemnumber => $item->id, - itype => $itype, - ccode => $ccode, - holdingbranch => $holdingbranch, - location => $location, + itemnumber => $item->id, + itype => $itype, + ccode => $ccode, + holdingbranch => $holdingbranch, + location => $location, + itype_enabled => $itype_enabled, + ccode_enabled => $ccode_enabled, + holdingbranch_enabled => $holdingbranch_enabled, + location_enabled => $location_enabled, ); my $cr_id = ModCourseReserve( diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/add_items-step2.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/add_items-step2.tt index 061821d708..2e3fb93c86 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/add_items-step2.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/add_items-step2.tt @@ -1,4 +1,5 @@ [% USE Branches %] +[% SET footerjs = 1 %] [% INCLUDE 'doc-head-open.inc' %] Koha › Course reserves ›[% IF is_edit || course_reserve %] Edit item[% ELSE %] Add items[% END %] [% INCLUDE 'doc-head-close.inc' %] @@ -16,7 +17,7 @@
[% IF course_reserve && !is_edit%]
This course already has this item on reserve.
[% END %] - [% IF course_item %]
Number of courses reserving this item: [% course_item.course_reserves.size | html %]
[% END %] + [% IF course_item %]
Number of courses reserving this item: [% course_item.course_reserves.count | html %]
[% END %]
@@ -39,11 +40,22 @@ [% IF item_level_itypes %]
  • - + [% ELSE %] + + [% END %] + + [% IF course_item.itype_enabled %] + + [% END %] + + [% FOREACH it IN itypes %] - [% IF course_item.itype.defined && ( ( course.enabled == 'yes' && it.itemtype == item.itype ) || ( course.enabled == 'no' && it.itemtype == course_item.itype ) ) %] + [% IF it.itemtype == course_item.itype %] [% ELSE %] @@ -54,12 +66,23 @@ [% END %]
  • - - + [% ELSE %] + + [% END %] + [% IF course_item.ccode_enabled %] + + [% END %] + + [% FOREACH c IN ccodes %] - [% IF course_item.ccode.defined && ( ( course.enabled == 'yes' && c.authorised_value == item.ccode ) || ( course.enabled == 'no' && c.authorised_value == course_item.ccode ) ) %] + [% IF c.authorised_value == course_item.ccode %] [% ELSE %] @@ -70,11 +93,22 @@
  • - + [% ELSE %] + + [% END %] + + [% IF course_item.location_enabled %] + + [% END %] + + [% FOREACH s IN locations %] - [% IF course_item.location.defined && ( ( course.enabled == 'yes' && s.authorised_value == item.location ) || ( course.enabled == 'no' && s.authorised_value == course_item.location ) ) %] + [% IF s.authorised_value == course_item.location %] [% ELSE %] @@ -85,10 +119,22 @@
  • - + [% ELSE %] + + [% END %] + + [% IF course_item.holdingbranch_enabled %] + + [% END %] + + [% FOREACH b IN Branches.all() %] - [% IF course_item.holdingbranch.defined && ( ( course.enabled == 'yes' && b.branchcode == item.holdingbranch ) || ( course.enabled == 'no' && b.branchcode == course_item.holdingbranch ) ) %] + [% IF b.branchcode == course_item.holdingbranch %] [% ELSE %] @@ -119,4 +165,21 @@
    • +[% MACRO jsinclude BLOCK %] + +[% END %] + [% INCLUDE 'intranet-bottom.inc' %] diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/batch_add_items.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/batch_add_items.tt index 068c6bafcb..c4adebce66 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/batch_add_items.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/batch_add_items.tt @@ -2,6 +2,8 @@ [% USE Branches %] [% USE ItemTypes %] +[% SET footerjs = 1 %] + [% INCLUDE 'doc-head-open.inc' %] Koha › Course reserves › Add items [% INCLUDE 'doc-head-close.inc' %] @@ -35,8 +37,9 @@ [% IF item_level_itypes %]
  • - + - + + + - + + - + +