| Summary: | Batch hold tool: Suspended holds are unsuspended when making other changes to holds | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Emmi Takkinen <emmi.takkinen> |
| Component: | Tools | Assignee: | Emmi Takkinen <emmi.takkinen> |
| Status: | Signed Off --- | QA Contact: | Martin Renvoize (ashimema) <martin.renvoize> |
| Severity: | major | ||
| Priority: | P5 - low | CC: | lucas, martin.renvoize |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| 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
Bug 41438: Do not always unsuspend holds when using batch modify holds tool Bug 41438: Do not always unsuspend holds when using batch modify holds tool Bug 41438: (follow-up) Fix JavaScript validation issues Bug 41438: (follow-up) Fix JavaScript validation issues |
||
|
Description
Emmi Takkinen
2025-12-12 10:56:15 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 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. |