From 68d248c6e76c6c65473d3cf3f2849b288482b629 Mon Sep 17 00:00:00 2001
From: Kyle M Hall <kyle@bywatersolutions.com>
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
---
 C4/CourseReserves.pm                          | 167 +++++++++---------
 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        |  89 ++++++++--
 .../course_reserves/batch_add_items.tt        |  39 +++-
 .../modules/course_reserves/course-details.tt |  24 ++-
 t/db_dependent/CourseReserves/CourseItems.t   | 101 ++++++-----
 11 files changed, 339 insertions(+), 174 deletions(-)

diff --git a/C4/CourseReserves.pm b/C4/CourseReserves.pm
index 2c130af192..fe8f5660e9 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);
+    $params{holdingbranch} ||= undef; # Can't be empty string, FK constraint
 
-    my %mod_params =
-      map {
-        defined $params{$_} && $params{$_} ne ''
-          ? ( $_ => $params{$_} )
-          : ()
-      } @FIELDS;
-
-    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 {
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 42416b3a63..edae03b891 100755
--- a/course_reserves/add_items.pl
+++ b/course_reserves/add_items.pl
@@ -39,10 +39,12 @@ 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')      || '';
 
 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';
@@ -65,12 +67,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;
 
@@ -89,12 +91,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 919c7d57c6..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' %]
 <title>Koha &rsaquo; Course reserves &rsaquo;[% IF is_edit || course_reserve %] Edit item[% ELSE %] Add items[% END %]</title>
 [% INCLUDE 'doc-head-close.inc' %]
@@ -16,7 +17,7 @@
         <div class="col-md-8 col-md-offset-2">
 
         [% IF course_reserve && !is_edit%]<div class="dialog message" id="already_on_reserve_this">This course already has this item on reserve.</div>[% END %]
-        [% IF course_item %]<div class="dialog message" id="already_on_reserve">Number of courses reserving this item: [% course_item.course_reserves.size | html %]</div>[% END %]
+        [% IF course_item %]<div class="dialog message" id="already_on_reserve">Number of courses reserving this item: [% course_item.course_reserves.count | html %]</div>[% END %]
 
         <form method="post" action="/cgi-bin/koha/course_reserves/add_items.pl">
             <input type="hidden" name="course_id" value="[% course.course_id | html %]" />
@@ -39,11 +40,22 @@
                     [% IF item_level_itypes %]
                     <li>
                         <label class="required" for="itype">Item type:</label>
-                        <select id="itype" name="itype">
-                            <option value="">LEAVE UNCHANGED</option>
 
+                        [% IF course_item.itype_enabled %]
+                            <input type="checkbox" class="field-toggle" data-pulldown="itype" value="1" name="itype_enabled" id="itype_enabled" checked="checked" />
+                        [% ELSE %]
+                            <input type="checkbox" class="field-toggle" data-pulldown="itype" value="1" name="itype_enabled" id="itype_enabled" />
+                        [% END %]
+
+                        [% IF course_item.itype_enabled %]
+                            <select id="itype" name="itype">
+                        [% ELSE %]
+                            <select id="itype" name="itype" disabled="disabled">
+                        [% END %]
+
+                            <option value=""></option>
                             [% 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 %]
                                     <option value="[% it.itemtype | html %]" selected="selected">[% it.description | html %]</option>
                                 [% ELSE %]
                                     <option value="[% it.itemtype | html %]">[% it.description | html %]</option>
@@ -55,11 +67,22 @@
 
                     <li>
                         <label class="required" for="ccode">Collection code:</label>
-                        <select id="ccode" name="ccode">
-                            <option value="">LEAVE UNCHANGED</option>
 
+                        [% IF course_item.ccode_enabled %]
+                            <input type="checkbox" class="field-toggle" data-pulldown="ccode" value="1" name="ccode_enabled" id="ccode_enabled" checked="checked" />
+                        [% ELSE %]
+                            <input type="checkbox" class="field-toggle" data-pulldown="ccode" value="1" name="ccode_enabled" id="ccode_enabled" />
+                        [% END %]
+
+                        [% IF course_item.ccode_enabled %]
+                            <select id="ccode" name="ccode">
+                        [% ELSE %]
+                            <select id="ccode" name="ccode" disabled="disabled">
+                        [% END %]
+
+                            <option value=""></option>
                             [% 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 %]
                                     <option value="[% c.authorised_value | html %]" selected="selected">[% c.lib | html %]</option>
                                 [% ELSE %]
                                     <option value="[% c.authorised_value | html %]">[% c.lib | html %]</option>
@@ -70,11 +93,22 @@
 
                     <li>
                         <label class="required" for="location">Shelving location:</label>
-                        <select id="location" name="location">
-                            <option value="">LEAVE UNCHANGED</option>
 
+                        [% IF course_item.location_enabled %]
+                            <input type="checkbox" class="field-toggle" data-pulldown="location" value="1" name="location_enabled" id="location_enabled" checked="checked" />
+                        [% ELSE %]
+                            <input type="checkbox" class="field-toggle" data-pulldown="location"  value="1" name="location_enabled" id="location_enabled" />
+                        [% END %]
+
+                        [% IF course_item.location_enabled %]
+                            <select id="location" name="location">
+                        [% ELSE %]
+                            <select id="location" name="location" disabled="disabled">
+                        [% END %]
+
+                            <option value=""></option>
                             [% 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 %]
                                     <option value="[% s.authorised_value | html %]" selected="selected">[% s.lib | html %]</option>
                                 [% ELSE %]
                                     <option value="[% s.authorised_value | html %]">[% s.lib | html %]</option>
@@ -85,10 +119,22 @@
 
                     <li>
                         <label class="required" for="holdingbranch">Holding library:</label>
-                        <select id="holdingbranch" name="holdingbranch">
-                            <option value="">LEAVE UNCHANGED</option>
+
+                        [% IF course_item.holdingbranch_enabled %]
+                            <input type="checkbox" value="1" class="field-toggle" data-pulldown="holdingbranch" name="holdingbranch_enabled" id="holdingbranch_enabled" checked="checked" />
+                        [% ELSE %]
+                            <input type="checkbox" value="1" class="field-toggle" data-pulldown="holdingbranch" name="holdingbranch_enabled" id="holdingbranch_enabled" />
+                        [% END %]
+
+                        [% IF course_item.holdingbranch_enabled %]
+                            <select id="holdingbranch" name="holdingbranch">
+                        [% ELSE %]
+                            <select id="holdingbranch" name="holdingbranch" disabled="disabled">
+                        [% END %]
+
+                            <option value=""></option>
                             [% 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 %]
                                     <option value="[% b.branchcode | html %]" selected="selected">[% b.branchname | html %]</option>
                                 [% ELSE %]
                                     <option value="[% b.branchcode | html %]">[% b.branchname | html %]</option>
@@ -119,4 +165,21 @@
     </div>
 </div>
 
+[% MACRO jsinclude BLOCK %]
+    <script type="text/javascript">
+    //<![CDATA[
+        $(document).ready(function(){
+            $('.field-toggle').change(function() {
+                if( this.checked ) {
+                    $('#' + $(this).data('pulldown') ).removeAttr('disabled');
+                } else {
+                    $('#' + $(this).data('pulldown') ).val('');
+                    $('#' + $(this).data('pulldown') ).attr('disabled', 'disabled');
+                }
+            });
+        });
+    //]]>
+    </script>
+[% 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 ce67aab3ff..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' %]
 <title>Koha &rsaquo; Course reserves &rsaquo; Add items</title>
 [% INCLUDE 'doc-head-close.inc' %]
@@ -35,8 +37,9 @@
                         [% IF item_level_itypes %]
                         <li>
                             <label class="required" for="itype">Item type:</label>
-                            <select id="itype" name="itype">
-                                <option value="">LEAVE UNCHANGED</option>
+                            <input type="checkbox" class="field-toggle" data-pulldown="itype" value="1" name="itype_enabled" id="itype_enabled" />
+                            <select id="itype" name="itype" disabled="disabled">
+                                <option value=""></option>
 
                                 [% FOREACH it IN ItemTypes.Get() %]
                                     <option value="[% it.itemtype | html %]">[% it.description | html %]</option>
@@ -47,8 +50,9 @@
 
                         <li>
                             <label class="required" for="ccode">Collection code:</label>
-                            <select id="ccode" name="ccode">
-                                <option value="">LEAVE UNCHANGED</option>
+                            <input type="checkbox" class="field-toggle" data-pulldown="ccode" value="1" name="ccode_enabled" id="ccode_enabled" />
+                            <select id="ccode" name="ccode" disabled="disabled">
+                                <option value=""></option>
                                 [% FOREACH c IN AuthorisedValues.Get('CCODE') %]
                                     <option value="[% c.authorised_value | html %]">[% c.lib | html %]</option>
                                 [% END %]
@@ -57,8 +61,9 @@
 
                         <li>
                             <label class="required" for="location">Shelving location:</label>
-                            <select id="location" name="location">
-                                <option value="">LEAVE UNCHANGED</option>
+                            <input type="checkbox" class="field-toggle" data-pulldown="location"  value="1" name="location_enabled" id="location_enabled" />
+                            <select id="location" name="location" disabled="disabled">
+                                <option value=""></option>
                                 [% FOREACH s IN AuthorisedValues.Get('LOC') %]
                                     <option value="[% s.authorised_value | html %]">[% s.lib | html %]</option>
                                 [% END %]
@@ -67,8 +72,9 @@
 
                         <li>
                             <label class="required" for="holdingbranch">Holding library:</label>
-                            <select id="holdingbranch" name="holdingbranch">
-                                <option value="">LEAVE UNCHANGED</option>
+                            <input type="checkbox" value="1" class="field-toggle" data-pulldown="holdingbranch" name="holdingbranch_enabled" id="holdingbranch_enabled" />
+                            <select id="holdingbranch" name="holdingbranch" disabled="disabled">
+                                <option value=""></option>
                                 [% FOREACH b IN Branches.all() %]
                                     <option value="[% b.branchcode | html %]">[% b.branchname | html %]</option>
                                 [% END %]
@@ -132,4 +138,21 @@
     </div>
 </div>
 
+[% MACRO jsinclude BLOCK %]
+    <script type="text/javascript">
+    //<![CDATA[
+        $(document).ready(function(){
+            $('.field-toggle').change(function() {
+                if( this.checked ) {
+                    $('#' + $(this).data('pulldown') ).removeAttr('disabled');
+                } else {
+                    $('#' + $(this).data('pulldown') ).val('');
+                    $('#' + $(this).data('pulldown') ).attr('disabled', 'disabled');
+                }
+            });
+        });
+    //]]>
+    </script>
+[% END %]
+
 [% INCLUDE 'intranet-bottom.inc' %]
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/course-details.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/course-details.tt
index ab08e01654..7542892803 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/course-details.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/course-details.tt
@@ -101,17 +101,13 @@
                             <td>[% cr.item.itemcallnumber | html %]</td>
                             [% IF item_level_itypes %]
                             <td>
-                                [% IF cr.course_item.itype.defined %]
+                                [% IF cr.course_item.itype_enabled %]
                                     [% IF cr.course_item.enabled == 'yes' %]
-                                        <strong>[% ItemTypes.GetDescription( cr.item.itype) | html %]</strong>
-                                        [% IF cr.item.itype %]
-                                            ([% ItemTypes.GetDescription( cr.course_item.itype ) | html %])
-                                        [% END %]
+                                        <strong>[% ItemTypes.GetDescription( cr.item.effective_itemtype ) | html %]</strong>
+                                        ([% ItemTypes.GetDescription( cr.course_item.itype_storage ) | html %])
                                     [% ELSE %]
                                         [% ItemTypes.GetDescription( cr.course_item.itype ) | html %]
-                                        [% IF cr.item.itype %]
-                                            (<strong>[% ItemTypes.GetDescription( cr.item.itype) | html %]</strong>)
-                                        [% END %]
+                                        (<strong>[% ItemTypes.GetDescription( cr.item.effective_itemtype) | html %]</strong>)
                                     [% END %]
                                 [% ELSE %]
                                      <i>Unchanged</i>
@@ -122,11 +118,11 @@
                             </td>
                             [% END %]
                             <td>
-                                 [% IF cr.course_item.ccode.defined %]
+                                 [% IF cr.course_item.ccode_enabled %]
                                      [% IF cr.course_item.enabled == 'yes' %]
                                         <strong>[% AuthorisedValues.GetByCode( 'CCODE', cr.item.ccode ) | html %]</strong>
                                         [% IF cr.item.ccode %]
-                                            ([% AuthorisedValues.GetByCode( 'CCODE', cr.course_item.ccode ) | html %])
+                                            ([% AuthorisedValues.GetByCode( 'CCODE', cr.course_item.ccode_storage ) | html %])
                                         [% END %]
                                      [% ELSE %]
                                         [% AuthorisedValues.GetByCode( 'CCODE', cr.course_item.ccode ) | html %]
@@ -142,11 +138,11 @@
                                  [% END %]
                             </td>
                             <td>
-                                [% IF cr.course_item.location.defined %]
+                                [% IF cr.course_item.location_enabled %]
                                     [% IF cr.course_item.enabled == 'yes' %]
                                         <strong>[% AuthorisedValues.GetByCode( 'LOC', cr.item.permanent_location ) | html %]</strong>
                                         [% IF cr.item.permanent_location %]
-                                            ([% AuthorisedValues.GetByCode( 'LOC', cr.course_item.location ) | html %])
+                                            ([% AuthorisedValues.GetByCode( 'LOC', cr.course_item.location_storage ) | html %])
                                         [% END %]
                                     [% ELSE %]
                                         [% AuthorisedValues.GetByCode( 'LOC', cr.course_item.location ) | html %]
@@ -162,11 +158,11 @@
                                 [% END %]
                             </td>
                             <td>
-                                [% IF cr.course_item.holdingbranch.defined %]
+                                [% IF cr.course_item.holdingbranch_enabled %]
                                     [% IF cr.course_item.enabled == 'yes' %]
                                         <strong>[% Branches.GetName( cr.item.holdingbranch ) | html %]</strong>
                                         [% IF cr.item.holdingbranch %]
-                                            ([% Branches.GetName( cr.course_item.holdingbranch ) | html %])
+                                            ([% Branches.GetName( cr.course_item.holdingbranch_storage ) | html %])
                                         [% END %]
                                     [% ELSE %]
                                         [% Branches.GetName( cr.course_item.holdingbranch ) | html %]
diff --git a/t/db_dependent/CourseReserves/CourseItems.t b/t/db_dependent/CourseReserves/CourseItems.t
index c5ce9652ff..fe375b424f 100644
--- a/t/db_dependent/CourseReserves/CourseItems.t
+++ b/t/db_dependent/CourseReserves/CourseItems.t
@@ -38,17 +38,22 @@ create_dependent_objects();
 my ($biblionumber, $itemnumber) = create_bib_and_item();
 
 my $ci_id = ModCourseItem(
-    itemnumber    => $itemnumber,
-    itype         => 'BK_foo',
-    ccode         => 'BOOK',
-    holdingbranch => 'B2',
-    location      => 'TH',
+    itemnumber            => $itemnumber,
+    itype_enabled         => 1,
+    ccode_enabled         => 1,
+    holdingbranch_enabled => 1,
+    location_enabled      => 1,
+    itype                 => 'BK_foo',
+    ccode                 => 'BOOK',
+    holdingbranch         => 'B2',
+    location              => 'TH',
 );
 
 my $course = $builder->build({
     source => 'CourseReserve',
     value => {
         ci_id => $ci_id,
+        enabled => 'no',
     }
 });
 
@@ -60,23 +65,27 @@ my $cr_id = ModCourseReserve(
 );
 
 my $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');
+is($course_item->{itype_storage}, 'CD_foo', 'Course item itype storage should be CD_foo');
+is($course_item->{ccode_storage}, 'CD', 'Course item ccode storage should be CD');
+is($course_item->{holdingbranch_storage}, 'B1', 'Course item holding branch storage should be B1');
+is($course_item->{location_storage}, 'HR', 'Course item location storage should be HR');
 
 my $item = Koha::Items->find($itemnumber);
-is($item->itype, 'BK_foo', 'Item type in course should be BK_foo');
+is($item->effective_itemtype, 'BK_foo', 'Item type in course should be BK_foo');
 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',
+    itemnumber            => $itemnumber,
+    itype_enabled         => 1,
+    ccode_enabled         => 1,
+    holdingbranch_enabled => 1,
+    location_enabled      => 1,
+    itype                 => 'BK_foo',
+    ccode                 => 'DVD',
+    holdingbranch         => 'B3',
+    location              => 'TH',
 );
 
 ModCourseReserve(
@@ -87,20 +96,20 @@ ModCourseReserve(
 );
 
 $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');
+is($course_item->{itype_storage}, 'CD_foo', 'Course item itype storage should be CD_foo');
+is($course_item->{ccode_storage}, 'CD', 'Course item ccode storage should be CD');
+is($course_item->{holdingbranch_storage}, 'B1', 'Course item holding branch storage should be B1');
+is($course_item->{location_storage}, 'HR', 'Course item location storage should be HR');
 
 $item = Koha::Items->find($itemnumber);
-is($item->itype, 'BK_foo', 'Item type in course should be BK_foo');
+is($item->effective_itemtype, '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');
+is($item->effective_itemtype, 'CD_foo', 'Item type removed from course should be set back to CD_foo');
 is($item->ccode, 'CD', 'Item ccode removed from course should be set back to CD');
 is($item->holdingbranch, 'B1', 'Item holding branch removed from course should be set back B1');
 is($item->location, 'HR', 'Item location removed from course should be TH');
@@ -112,11 +121,15 @@ $item = Koha::Items->find($itemnumber);
 is($item->ccode, '', 'Item ccode should be empty');
 
 my $ci_id2 = ModCourseItem(
-    itemnumber    => $itemnumber,
-    itype         => 'CD_foo',
-    ccode         => 'BOOK',
-    holdingbranch => 'B1',
-    location      => 'HR',
+    itemnumber            => $itemnumber,
+    itype_enabled         => 1,
+    ccode_enabled         => 1,
+    holdingbranch_enabled => 1,
+    location_enabled      => 1,
+    itype                 => 'CD_foo',
+    ccode                 => 'BOOK',
+    holdingbranch         => 'B1',
+    location              => 'HR',
 );
 
 my $cr_id2 = ModCourseReserve(
@@ -130,14 +143,18 @@ $item = Koha::Items->find($itemnumber);
 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');
+is($course_item2->{ccode_storage}, '', 'Course item ccode storage should be empty');
 
 ModCourseItem(
-    itemnumber    => $itemnumber,
-    itype         => 'CD_foo',
-    ccode         => 'DVD',
-    holdingbranch => 'B1',
-    location      => 'HR',
+    itemnumber            => $itemnumber,
+    itype_enabled         => 1,
+    ccode_enabled         => 1,
+    holdingbranch_enabled => 1,
+    location_enabled      => 1,
+    itype                 => 'CD_foo',
+    ccode                 => 'DVD',
+    holdingbranch         => 'B1',
+    location              => 'HR',
 );
 
 ModCourseReserve(
@@ -151,17 +168,21 @@ $item = Koha::Items->find($itemnumber);
 is($item->ccode, 'DVD', 'Item ccode should be DVD');
 
 ModCourseItem(
-    itemnumber    => $itemnumber,
-    itype         => 'BK',
-    ccode         => 'BOOK',
-    holdingbranch => '', # LEAVE UNCHANGED
-    location      => 'TH',
+    itemnumber            => $itemnumber,
+    itype_enabled         => 1,
+    ccode_enabled         => 1,
+    holdingbranch_enabled => 0,             # LEAVE UNCHANGED
+    location_enabled      => 1,
+    itype                 => 'BK',
+    ccode                 => 'BOOK',
+    holdingbranch         => undef,         # LEAVE UNCHANGED
+    location              => 'TH',
 );
 $item = Koha::Items->find($itemnumber);
 is($item->ccode, 'BOOK', 'Item ccode should be BOOK');
 
 $course_item2 = GetCourseItem( ci_id => $ci_id2 );
-is($course_item2->{ccode}, '', 'Course item ccode should be empty');
+is($course_item2->{ccode_storage}, '', 'Course item ccode storage should be empty');
 
 DelCourseReserve( cr_id => $cr_id2 );
 $item = Koha::Items->find($itemnumber);
@@ -195,7 +216,7 @@ subtest 'Ensure item info is preserved' => sub {
     #Remove course reservei
     DelCourseReserve( cr_id => $course_reserve_id );
     my $item_after = Koha::Items->find( $item->itemnumber );
-    is( $item->itype, $item_after->itype, "Itemtype is unchanged after adding to and removing from course reserves for inactive course");
+    is( $item->effective_itemtype, $item_after->itype, "Itemtype is unchanged after adding to and removing from course reserves for inactive course");
     is( $item->location, $item_after->location, "Location is unchanged after adding to and removing from course reserves for inactive course");
     is( $item->holdingbranch, $item_after->holdingbranch, "Holdingbranch is unchanged after adding to and removing from course reserves for inactive course");
     is( $item->ccode, $item_after->ccode, "Collection is unchanged after adding to and removing from course reserves for inactive course");
@@ -225,7 +246,7 @@ subtest 'Ensure item info is preserved' => sub {
     #Remove course reserve
     DelCourseReserve( cr_id => $course_reserve_id );
     $item_after = Koha::Items->find( $item->itemnumber );
-    is( $item->itype, $item_after->itype, "Itemtype is unchanged after adding to and removing from course reserves for inactive course");
+    is( $item->effective_itemtype, $item_after->itype, "Itemtype is unchanged after adding to and removing from course reserves for inactive course");
     is( $item->location, $item_after->location, "Location is unchanged after adding to and removing from course reserves for inactive course");
     is( $item->holdingbranch, $item_after->holdingbranch, "Holdingbranch is unchanged after adding to and removing from course reserves for inactive course");
     is( $item->ccode, $item_after->ccode, "Collection is unchanged after adding to and removing from course reserves for inactive course");
-- 
2.21.1 (Apple Git-122.3)