Bug 24802 - Updating holds can cause suspensions to apply to wrong hold
Summary: Updating holds can cause suspensions to apply to wrong hold
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-03-04 17:07 UTC by Kyle M Hall
Modified: 2020-04-13 11:27 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.05


Attachments
Bug 24802: Updating holds can cause suspensions to apply to wrong hold (4.23 KB, patch)
2020-03-04 17:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24802: Updating holds can cause suspensions to apply to wrong hold (4.28 KB, patch)
2020-03-04 20:55 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 24802: Updating holds can cause suspensions to apply to wrong hold (4.38 KB, patch)
2020-03-06 08:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24802 [19.05]: Updating holds can cause suspensions to apply to wrong hold (4.18 KB, patch)
2020-04-13 11:27 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2020-03-04 17:07:09 UTC
On request.pl, the table of holds shows a suspend_until date picker for each hold, *unless* that hold is waiting or in transit. The script reserve/modrequest.pl assumes that there will be a suspend_until input for each hold, but that is incorrect. Assume there are 20 holds on a record, and 10 of them are waiting or in transit. If you were to then set the suspend until date on the 10 open holds, and use the "Update hold(s)" button, those 10 suspensions would apply to the 10 found holds and not the holds they should apply to!
Comment 1 Kyle M Hall 2020-03-04 17:11:06 UTC
Created attachment 100141 [details] [review]
Bug 24802: Updating holds can cause suspensions to apply to wrong hold

On request.pl, the table of holds shows a suspend_until date picker for each hold, *unless* that hold is waiting or in transit. The script reserve/modrequest.pl assumes that there will be a suspend_until input for each hold, but that is incorrect. Assume there are 20 holds on a record, and 10 of them are waiting or in transit. If you were to then set the suspend until date on the 10 open holds, and use the "Update hold(s)" button, those 10 suspensions would apply to the 10 found holds and not the holds they should apply to!

Test Plan:
1) Place two holds on a record
2) Check in an item and trap it for the first hold
3) Now that one hold is waiting and the other is not, attempt to set
   a suspension date using the "Update hold(s)" button
4) Note your hold does not get suspended!
5) Apply this patch
6) Restart all the things!
7) Repeat steps 1-3
8) Your hold should now be suspended!
Comment 2 Andrew Fuerste-Henry 2020-03-04 20:55:59 UTC
Created attachment 100155 [details] [review]
Bug 24802: Updating holds can cause suspensions to apply to wrong hold

On request.pl, the table of holds shows a suspend_until date picker for each hold, *unless* that hold is waiting or in transit. The script reserve/modrequest.pl assumes that there will be a suspend_until input for each hold, but that is incorrect. Assume there are 20 holds on a record, and 10 of them are waiting or in transit. If you were to then set the suspend until date on the 10 open holds, and use the "Update hold(s)" button, those 10 suspensions would apply to the 10 found holds and not the holds they should apply to!

Test Plan:
1) Place two holds on a record
2) Check in an item and trap it for the first hold
3) Now that one hold is waiting and the other is not, attempt to set
   a suspension date using the "Update hold(s)" button
4) Note your hold does not get suspended!
5) Apply this patch
6) Restart all the things!
7) Repeat steps 1-3
8) Your hold should now be suspended!

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 3 Marcel de Rooy 2020-03-06 08:22:31 UTC
QAing
Comment 4 Marcel de Rooy 2020-03-06 08:34:24 UTC
Created attachment 100216 [details] [review]
Bug 24802: Updating holds can cause suspensions to apply to wrong hold

On request.pl, the table of holds shows a suspend_until date picker for each hold, *unless* that hold is waiting or in transit. The script reserve/modrequest.pl assumes that there will be a suspend_until input for each hold, but that is incorrect. Assume there are 20 holds on a record, and 10 of them are waiting or in transit. If you were to then set the suspend until date on the 10 open holds, and use the "Update hold(s)" button, those 10 suspensions would apply to the 10 found holds and not the holds they should apply to!

Test Plan:
1) Place two holds on a record
2) Check in an item and trap it for the first hold
3) Now that one hold is waiting and the other is not, attempt to set
   a suspension date using the "Update hold(s)" button
4) Note your hold does not get suspended!
5) Apply this patch
6) Restart all the things!
7) Repeat steps 1-3
8) Your hold should now be suspended!

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Martin Renvoize 2020-03-06 10:05:10 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 6 Joy Nelson 2020-04-01 22:44:26 UTC
Pushed to 19.11.x for 19.11.05
Comment 7 Lucas Gass 2020-04-09 20:36:10 UTC
attempted to rebase and all seems good, test plan works but I fail the qa tool:

 FAIL	reserve/modrequest.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   FAIL	  valid
		Global symbol "@reservedates" requires explicit package name (did you forget to declare "my @reservedates"?)
		Global symbol "@reservedates" requires explicit package name (did you forget to declare "my @reservedates"?)
		reserve/modrequest.pl had compilation errors.
Comment 8 Lucas Gass 2020-04-09 20:36:44 UTC
My rebase:

else {
    for (my $i=0;$i<$count;$i++){
        undef $itemnumber[$i] if !$itemnumber[$i];
-        ModReserve({
+        my $suspend_until = $query->param( "suspend_until_" . $reserve_id[$i] );
+        my $params = {
            rank => $rank[$i],
            reserve_id => $reserve_id[$i],
            branchcode => $branch[$i],
            itemnumber => $itemnumber[$i],
-            suspend_until => $suspend_until[$i]
-        });
+            defined $suspend_until ? ( suspend_until => $suspend_until ) : (),
+        };
+        if (C4::Context->preference('AllowHoldDateInFuture')) {
+            $params->{reservedate} = $reservedates[$i] ? dt_from_string($reservedates[$i]) : undef;
+        }
+
+        ModReserve($params);
    }
}
Comment 9 Kyle M Hall 2020-04-13 11:27:38 UTC
Created attachment 102811 [details] [review]
Bug 24802 [19.05]: Updating holds can cause suspensions to apply to wrong hold

On request.pl, the table of holds shows a suspend_until date picker for each hold, *unless* that hold is waiting or in transit. The script reserve/modrequest.pl assumes that there will be a suspend_until input for each hold, but that is incorrect. Assume there are 20 holds on a record, and 10 of them are waiting or in transit. If you were to then set the suspend until date on the 10 open holds, and use the "Update hold(s)" button, those 10 suspensions would apply to the 10 found holds and not the holds they should apply to!

Test Plan:
1) Place two holds on a record
2) Check in an item and trap it for the first hold
3) Now that one hold is waiting and the other is not, attempt to set
   a suspension date using the "Update hold(s)" button
4) Note your hold does not get suspended!
5) Apply this patch
6) Restart all the things!
7) Repeat steps 1-3
8) Your hold should now be suspended!

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Joy Nelson <joy@bywatersolutions.com>