Bug 41438 - Batch hold tool: Suspended holds are unsuspended when making other changes to holds
Summary: Batch hold tool: Suspended holds are unsuspended when making other changes to...
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Emmi Takkinen
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-12-12 10:56 UTC by Emmi Takkinen
Modified: 2025-12-18 07:26 UTC (History)
2 users (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Comma delimited list of Sponsors:
Crowdfunding goal: 0
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 41438: Do not always unsuspend holds when using batch modify holds tool (2.55 KB, patch)
2025-12-12 11:03 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 41438: Do not always unsuspend holds when using batch modify holds tool (2.60 KB, patch)
2025-12-12 19:47 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 41438: Do not always unsuspend holds when using batch modify holds tool (2.67 KB, patch)
2025-12-17 13:40 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41438: (follow-up) Fix JavaScript validation issues (2.34 KB, patch)
2025-12-17 13:40 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41438: (follow-up) Fix JavaScript validation issues (2.40 KB, patch)
2025-12-18 07:23 UTC, Emmi Takkinen
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emmi Takkinen 2025-12-12 10:56:15 UTC
If one modifies suspended holds e.g. changes their pickup library, holds are unsuspended. This happens due error in logic of checking if "Suspend holds" selection is empty.

To reproduce:
1. Navigate to Tools->Batch modify holds.
2. Search suspended holds.
3. Select a hold to modify and change it's pickup library, but leave "Suspend holds" selection empty.
4. Press "Modify holds".
=> Note that hold is now not suspended.
Comment 1 Emmi Takkinen 2025-12-12 11:03:12 UTC
Created attachment 190454 [details] [review]
Bug 41438: Do not always unsuspend holds when using batch modify holds tool

If one modifies suspended holds e.g. changes their pickup library, holds
are unsuspended. This happens due error in logic of checking if "Suspend
holds" selection is empty. This patch fixes this by moving the if statement
for checking new suspended status before doing anything else for the value.

To reproduce:
1. Navigate to Tools->Batch modify holds.
2. Search suspended holds.
3. Select a hold to modify and change it's pickup library, but leave
"Suspend holds" selection empty.
4. Press "Modify holds".
=> Note that hold is now unsuspended.
5. Apply this patch, start services again if needed.
6. Repeat steps from 2. to 4.
=> Note that hold is not suspended.
=> Confirm that suspending and unsuspending with and without suspend date
still works as expected.

Sponsored-by: Koha-Suomi Oy
Comment 2 Roman Dolny 2025-12-12 19:47:30 UTC
Created attachment 190468 [details] [review]
Bug 41438: Do not always unsuspend holds when using batch modify holds tool

If one modifies suspended holds e.g. changes their pickup library, holds
are unsuspended. This happens due error in logic of checking if "Suspend
holds" selection is empty. This patch fixes this by moving the if statement
for checking new suspended status before doing anything else for the value.

To reproduce:
1. Navigate to Tools->Batch modify holds.
2. Search suspended holds.
3. Select a hold to modify and change it's pickup library, but leave
"Suspend holds" selection empty.
4. Press "Modify holds".
=> Note that hold is now unsuspended.
5. Apply this patch, start services again if needed.
6. Repeat steps from 2. to 4.
=> Note that hold is not suspended.
=> Confirm that suspending and unsuspending with and without suspend date
still works as expected.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 3 Martin Renvoize (ashimema) 2025-12-17 13:35:42 UTC
I think this may have an unintended side effect.. If a user sets new_suspend_date but leaves new_suspend_status empty (default), the suspend date will be completely ignored now when it wasn't before.
Comment 4 Martin Renvoize (ashimema) 2025-12-17 13:40:34 UTC
Created attachment 190575 [details] [review]
Bug 41438: Do not always unsuspend holds when using batch modify holds tool

If one modifies suspended holds e.g. changes their pickup library, holds
are unsuspended. This happens due error in logic of checking if "Suspend
holds" selection is empty. This patch fixes this by moving the if statement
for checking new suspended status before doing anything else for the value.

To reproduce:
1. Navigate to Tools->Batch modify holds.
2. Search suspended holds.
3. Select a hold to modify and change it's pickup library, but leave
"Suspend holds" selection empty.
4. Press "Modify holds".
=> Note that hold is now unsuspended.
5. Apply this patch, start services again if needed.
6. Repeat steps from 2. to 4.
=> Note that hold is not suspended.
=> Confirm that suspending and unsuspending with and without suspend date
still works as expected.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 5 Martin Renvoize (ashimema) 2025-12-17 13:40:35 UTC
Created attachment 190576 [details] [review]
Bug 41438: (follow-up) Fix JavaScript validation issues

This follow-up patch corrects two JavaScript issues in the batch
modify holds form:

1. Fixes typo: 'suspened' -> 'suspended' on line 637
   This typo prevented the suspended status check from working
   correctly when determining if any selected holds are suspended.

2. Fixes ineffective validation logic on line 643
   When a suspend date is provided without explicitly setting the
   suspend status, the code now properly updates the form field
   value instead of just setting a local variable. Changes:
   - Old: new_suspend_status = 1;
   - New: $("#new_suspend_status").val("1");

   This ensures that setting a suspend date automatically sets
   the suspend status to "Suspend" as intended, allowing the
   behavior to match the server-side logic.

Test plan:
1. Navigate to Tools->Batch modify holds
2. Search for and select some holds
3. Set only a "Suspend until" date without changing "Suspend holds"
4. Click "Modify holds"
5. Verify the holds are suspended with the specified date
6. Confirm the form properly auto-selects "Suspend" when a date
   is provided
Comment 6 Martin Renvoize (ashimema) 2025-12-17 13:41:44 UTC
Can you take a look at my follow-up Emmi and let me know if you agree with it?

I think it resolves the case where one could attempt to set a suspend date without setting the suspend state correspondingly.

If you're happy I think we can PQA
Comment 7 Emmi Takkinen 2025-12-18 07:23:25 UTC
Created attachment 190611 [details] [review]
Bug 41438: (follow-up) Fix JavaScript validation issues

This follow-up patch corrects two JavaScript issues in the batch
modify holds form:

1. Fixes typo: 'suspened' -> 'suspended' on line 637
   This typo prevented the suspended status check from working
   correctly when determining if any selected holds are suspended.

2. Fixes ineffective validation logic on line 643
   When a suspend date is provided without explicitly setting the
   suspend status, the code now properly updates the form field
   value instead of just setting a local variable. Changes:
   - Old: new_suspend_status = 1;
   - New: $("#new_suspend_status").val("1");

   This ensures that setting a suspend date automatically sets
   the suspend status to "Suspend" as intended, allowing the
   behavior to match the server-side logic.

Test plan:
1. Navigate to Tools->Batch modify holds
2. Search for and select some holds
3. Set only a "Suspend until" date without changing "Suspend holds"
4. Click "Modify holds"
5. Verify the holds are suspended with the specified date
6. Confirm the form properly auto-selects "Suspend" when a date
   is provided

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 8 Emmi Takkinen 2025-12-18 07:26:36 UTC
(In reply to Martin Renvoize (ashimema) from comment #6)
> Can you take a look at my follow-up Emmi and let me know if you agree with
> it?
> 
> I think it resolves the case where one could attempt to set a suspend date
> without setting the suspend state correspondingly.
> 
> If you're happy I think we can PQA

Thank you Martin for the quick follow-up! Tested this and different suspend scenarios (suspending with and without suspend date, unsuspend) seem to be working now as intended.