Bug 19974: Make MarkLostItemsAsReturned multiple

Given the confusion regarding this behaviour it sounds better to make it
configurable.
This pref will take 4 different values, 1 per place an item can be
marked as lost.

Test plan:
Mark items as lost and confirm the item is returned or not, depending on
the value of the system preference.

- from the longoverdue cronjob (--mark-returned takes precedence if set)
- from the batch item modification tool
- when cataloguing an item
- from the items tab of the catalog module

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index 1f1a431..c6dc6af 100644 (file)
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -3613,9 +3613,20 @@ sub ReturnLostItem{
 
 
 sub LostItem{
-    my ($itemnumber, $mark_returned) = @_;
+    my ($itemnumber, $mark_lost_from, $force_mark_returned) = @_;
 
-    $mark_returned //= C4::Context->preference('MarkLostItemsAsReturned');
+    unless ( $mark_lost_from ) {
+        # Temporary check to avoid regressions
+        die q|LostItem called without $mark_lost_from, check the API.|;
+    }
+
+    my $mark_returned;
+    if ( $force_mark_returned ) {
+        $mark_returned = 1;
+    } else {
+        my $pref = C4::Context->preference('MarkLostItemsAsReturned') // q{};
+        $mark_returned = ( $pref =~ m|$mark_lost_from| );
+    }
 
     my $dbh = C4::Context->dbh();
     my $sth=$dbh->prepare("SELECT issues.*,items.*,biblio.title 

diff --git a/catalogue/updateitem.pl b/catalogue/updateitem.pl
index b7cbc94..f06dfed 100755 (executable)
--- a/catalogue/updateitem.pl
+++ b/catalogue/updateitem.pl
@@ -80,6 +80,6 @@ elsif (defined $itemnotes) { # i.e., itemnotes parameter passed from form
 
 ModItem($item_changes, $biblionumber, $itemnumber);
 
-LostItem($itemnumber) if $itemlost;
+LostItem($itemnumber, 'moredetail') if $itemlost;
 
 print $cgi->redirect("moredetail.pl?biblionumber=$biblionumber&itemnumber=$itemnumber#item$itemnumber");

diff --git a/cataloguing/additem.pl b/cataloguing/additem.pl
index 802e908..1b746f7 100755 (executable)
--- a/cataloguing/additem.pl
+++ b/cataloguing/additem.pl
@@ -695,7 +695,7 @@ if ($op eq "additem") {
         $itemnumber = q{};
         my $olditemlost = $item->{itemlost};
         my $newitemlost = $newitem->{itemlost};
-        LostItem( $item->{itemnumber} )
+        LostItem( $item->{itemnumber}, 'additem' )
             if $newitemlost && $newitemlost ge '1' && !$olditemlost;
     }
     $nextop="additem";

diff --git a/installer/data/mysql/atomicupdate/bug_19974.perl b/installer/data/mysql/atomicupdate/bug_19974.perl
new file mode 100644 (file)
index 0000000..78ff577
--- /dev/null
+++ b/installer/data/mysql/atomicupdate/bug_19974.perl
@@ -0,0 +1,26 @@
+$DBversion = 'XXX';  # will be replaced by the RM
+if( CheckVersion( $DBversion ) ) {
+    my ( $original_value ) = $dbh->selectrow_array(q|
+        SELECT value FROM systempreferences WHERE variable="MarkLostItemsAsReturned"
+    |);
+    if ( $original_value and $original_value eq '1' ) {
+        $dbh->do(q{
+            UPDATE systempreferences
+            SET type="multiple",
+                options="batchmod|moredetail|cronjob|additem",
+                value="batchmod|moredetail|cronjob|additem"
+            WHERE variable="MarkLostItemsAsReturned"
+        });
+    } else {
+        $dbh->do(q{
+            UPDATE systempreferences
+            SET type="multiple",
+                options="batchmod|moredetail|cronjob|additem",
+                value=""
+            WHERE variable="MarkLostItemsAsReturned"
+        });
+    }
+
+    SetVersion( $DBversion );
+    print "Upgrade to $DBversion done (Bug 19974 - Make MarkLostItemsAsReturned multiple)\n";
+}

diff --git a/installer/data/mysql/sysprefs.sql b/installer/data/mysql/sysprefs.sql
index ad8f41d..f01f572 100644 (file)
--- a/installer/data/mysql/sysprefs.sql
+++ b/installer/data/mysql/sysprefs.sql
@@ -261,7 +261,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `
 ('MarcFieldDocURL', NULL, NULL, 'URL used for MARC field documentation. Following substitutions are available: {MARC} = marc flavour, eg. "MARC21" or "UNIMARC". {FIELD} = field number, eg. "000" or "048". {LANG} = user language, eg. "en" or "fi-FI"', 'free'),
 ('MarcFieldsToOrder','',NULL,'Set the mapping values for a new order line created from a MARC record in a staged file. In a YAML format.','textarea'),
 ('MarcItemFieldsToOrder','',NULL,'Set the mapping values for new item records created from a MARC record in a staged file. In a YAML format.','textarea'),
-('MarkLostItemsAsReturned','1','','Mark items as returned when flagged as lost','YesNo'),
+('MarkLostItemsAsReturned','batchmod|moredetail|cronjob|additem','batchmod|moredetail|cronjob|additem','Mark items as returned when flagged as lost','multiple'),
 ('MARCOrgCode','OSt','','Define MARC Organization Code for MARC21 records - http://www.loc.gov/marc/organizations/orgshome.html','free'),
 ('MaxFine',NULL,'','Maximum fine a patron can have for all late returns at one moment. Single item caps are specified in the circulation rules matrix.','Integer'),
 ('MaxItemsToDisplayForBatchDel','1000',NULL,'Display up to a given number of items in a single item deletionbatch.','Integer'),

diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
index 8ec6a6b..7aa79ba 100644 (file)
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
@@ -413,11 +413,14 @@ Circulation:
                   nothing : "do nothing"
             - .
         -
+            - "Mark items as returned when flagged as lost "
             - pref: MarkLostItemsAsReturned
-              choices:
-                  yes: "Mark"
-                  no: "Do not mark"
-            - "items as returned when flagged as lost"
+              multiple:
+                cronjob: "from the longoverdue cronjob"
+                batchmod: "from the batch item modification tool"
+                additem: "when cataloguing an item"
+                moredetail: "from the items tab of the catalog module"
+            - .
         -
             - pref: AllowMultipleIssuesOnABiblio
               choices:

diff --git a/misc/cronjobs/longoverdue.pl b/misc/cronjobs/longoverdue.pl
index 394000b..f8fd871 100755 (executable)
--- a/misc/cronjobs/longoverdue.pl
+++ b/misc/cronjobs/longoverdue.pl
@@ -310,7 +310,7 @@ foreach my $startrange (sort keys %$lost) {
             if($confirm) {
                 ModItem({ itemlost => $lostvalue }, $row->{'biblionumber'}, $row->{'itemnumber'});
                 if ( $charge && $charge eq $lostvalue ) {
-                    LostItem( $row->{'itemnumber'}, $mark_returned );
+                    LostItem( $row->{'itemnumber'}, 'cronjob', $mark_returned );
                 } elsif ( $mark_returned ) {
                     my $patron = Koha::Patrons->find( $row->{borrowernumber} );
                     MarkIssueReturned($row->{borrowernumber},$row->{itemnumber},undef,undef,$patron->privacy)

diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t
index bc14b53..468fdfa 100755 (executable)
--- a/t/db_dependent/Circulation.t
+++ b/t/db_dependent/Circulation.t
@@ -17,7 +17,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 114;
+use Test::More tests => 116;
 
 use DateTime;
 use POSIX qw( floor );
@@ -846,10 +846,12 @@ C4::Context->dbh->do("DELETE FROM accountlines");
     t::lib::Mocks::mock_preference('WhenLostForgiveFine','0');
     t::lib::Mocks::mock_preference('WhenLostChargeReplacementFee','0');
 
-    LostItem( $itemnumber, 1 );
+    LostItem( $itemnumber, 'test', 1 );
 
     my $item = Koha::Database->new()->schema()->resultset('Item')->find($itemnumber);
     ok( !$item->onloan(), "Lost item marked as returned has false onloan value" );
+    my $checkout = Koha::Checkouts->find({ itemnumber => $itemnumber });
+    is( $checkout, undef, 'LostItem called with forced return has checked in the item' );
 
     my $total_due = $dbh->selectrow_array(
         'SELECT SUM( amountoutstanding ) FROM accountlines WHERE borrowernumber = ?',
@@ -871,10 +873,11 @@ C4::Context->dbh->do("DELETE FROM accountlines");
         }
     );
 
-    LostItem( $itemnumber2, 0 );
+    LostItem( $itemnumber2, 'test', 0 );
 
     my $item2 = Koha::Database->new()->schema()->resultset('Item')->find($itemnumber2);
     ok( $item2->onloan(), "Lost item *not* marked as returned has true onloan value" );
+    ok( Koha::Checkouts->find({ itemnumber => $itemnumber2 }), 'LostItem called without forced return has checked in the item' );
 
     $total_due = $dbh->selectrow_array(
         'SELECT SUM( amountoutstanding ) FROM accountlines WHERE borrowernumber = ?',

diff --git a/tools/batchMod.pl b/tools/batchMod.pl
index 94062f7..192deb9 100755 (executable)
--- a/tools/batchMod.pl
+++ b/tools/batchMod.pl
@@ -207,7 +207,7 @@ if ($op eq "action") {
                 if ( $modified ) {
                     eval {
                         if ( my $item = ModItemFromMarc( $localmarcitem, $itemdata->{biblionumber}, $itemnumber ) ) {
-                            LostItem($itemnumber) if $item->{itemlost} and not $itemdata->{itemlost};
+                            LostItem($itemnumber, 'batchmod') if $item->{itemlost} and not $itemdata->{itemlost};
                         }
                     };
                 }