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.
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
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>
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.
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>
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
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
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>
(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.