[Bug 42432] New: "On hold due date" input on patron checkouts table is ignored when renewing
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42432 Bug ID: 42432 Summary: "On hold due date" input on patron checkouts table is ignored when renewing Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: minor Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: martin.renvoize@openfifth.co.uk QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle@bywatersolutions.com ## Description Bug 7088 introduced an **"On hold due date"** input in the patron checkouts table footer (file: `koha-tmpl/intranet-tmpl/prog/en/includes/checkouts-table-footer.inc`). The input is shown when `AllowRenewalOnHoldOverride` is enabled and the librarian ticks the renew checkbox on any row whose item is on hold for another patron. The intent is that renewals of on-hold items use the value from this input, while non-on-hold renewals use the ordinary "Renewal due date" input. Since Bug 36084 ("svc - renew", Feb 2024, commit `99d776f58e0`), which replaced the `svc/renew` endpoint with a REST API client call, the code that conditionally read the on-hold input was lost. The replaced snippet was: ```js // pre-Bug 36084 var dueDate = isOnReserve ? $("#newonholdduedate input").val() : $("#newduedate").val(); // post-Bug 36084 (current) var dueDate = $("#newduedate").val(); ``` at what is now `koha-tmpl/intranet-tmpl/prog/js/checkouts.js:1041`. The `#newonholdduedate` input is still shown, still mirrored from `#newduedate`, and still flips the `onHoldDueDateSet` flag when edited â but its value is never attached to the `params` object posted to `POST /api/v1/circulation/checkouts/{id}/renewals`. A grep across `*.pl`, `*.pm`, `*.tt`, `*.inc`, `*.js`, `*.yaml` confirms that the form field name `newonholdduedate` (distinct from `renewonholdduedate` used by `circ/renew.pl`) has no consumer. The **Renew** page (`circ/renew.pl`, field `renewonholdduedate`) is unaffected and continues to work. ## Steps to reproduce 1. In a KTD environment, set `AllowRenewalOnHoldOverride = Allow`. 2. Check out item A to patron P1. 3. Place a hold on item A for patron P2. 4. Open P1's Patron details page and scroll to the **Checkouts** table. 5. Tick the **Renew** checkbox on the row for item A. Observe that the **"On hold due date"** input appears in the table footer. 6. In **"Renewal due date"** enter a date two weeks from now (say `2026-05-05`). 7. In **"On hold due date"** enter a clearly-distinct date one month from now (say `2026-05-20`). 8. Click **"Renew or check in selected items"**. 9. Inspect the resulting `issues.date_due` for item A, or observe the value now shown in the refreshed table. ## Expected behaviour `issues.date_due` equals `2026-05-20` (the "On hold due date" value), because item A is on hold. ## Actual behaviour `issues.date_due` equals `2026-05-05` (the "Renewal due date" value). The "On hold due date" value is silently discarded. ## Suggested fix In `koha-tmpl/intranet-tmpl/prog/js/checkouts.js`: - The `renew(item_id)` function (around line 1010) currently takes only the `item_id`. Inside it, look up whether the row is on hold by testing `$("#renew_" + item_id).is("[data-on-reserve]")` (the `data-on-reserve` attribute is already set on the checkbox by the table builder at around line 557). - Around line 1041, restore the conditional: ```js var isOnReserve = $("#renew_" + item_id).is("[data-on-reserve]"); var dueDate = isOnReserve ? $("#newonholdduedate input").val() : $("#newduedate").val(); ``` Keep the existing `if (dueDate && dueDate.length > 0) { params.date_due = dueDate; }` guard so blank values still fall through to the standard loan rule. ## Tests This bug should also introduce regression coverage. There is currently: - No Perl test that exercises `newonholdduedate` form submission. - No Cypress spec covering the on-hold branch of the checkouts-table renewal flow (which is what would have caught the regression in 2024). A Cypress spec under `t/cypress/integration/Circulation/` should: 1. Seed a patron with an on-hold checkout (use existing factories). 2. Visit the patron's checkouts page. 3. Tick the renew checkbox on the on-hold row. 4. Assert the "On hold due date" input is visible. 5. Set distinct dates in "Renewal due date" and "On hold due date". 6. Click "Renew or check in selected items". 7. Assert the refreshed row's displayed due date matches the "On hold due date" value, not the "Renewal due date" value. ## References - Bug 7088 â original feature (commit `c48af49ded4`, 2018) - Bug 36084 â regression point (commit `99d776f58e0`, 2024) -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42432 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |martin.renvoize@openfifth.c |ity.org |o.uk -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42432 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42432 Aude Charillon <aude.charillon@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |aude.charillon@openfifth.co | |.uk -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42432 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42432 --- Comment #1 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 197927 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197927&action=edit Bug 42432: Restore 'On hold due date' on patron checkouts table The 'On hold due date' input in the patron checkouts table footer (shown when AllowRenewalOnHoldOverride is enabled and a renewal row is on reserve) is currently ignored: editing it has no effect, and on-hold items are renewed using the 'Renewal due date' value instead. This is a regression from Bug 36084 (commit 99d776f58e0, Feb 2024). That patch ported the renewal AJAX call to use CirculationAPIClient.checkouts.renew() and, in the process, dropped the pre-existing conditional that picked between '#newonholdduedate' (for on-reserve rows) and '#newduedate' (for regular rows). The template input and its show/hide + mirror JS were left in place, so the field still appears and accepts input -- its value is just no longer sent. This patch restores the original behaviour introduced by Bug 7088: for each renewed row, if the row's checkbox has the 'data-on-reserve' attribute, the value from '#newonholdduedate' is used as the renewal due date; otherwise '#newduedate' is used, matching the rest of the table. We identify the row by its hidden value attribute (input.renew[value='<item_id>']) rather than by the checkbox id '#renew_<item_id>', because renew_all() injects a spinner <img> that shares the same id while the request is in flight. Test plan: 1. Set 'AllowRenewalOnHoldOverride' to 'Allow'. 2. Check item A out to patron P1. 3. Place a hold on item A for a different patron P2. 4. Open P1's patron details page and tick the Renew checkbox on the row for item A. Confirm the 'On hold due date' input appears below the 'Renewal due date' input. 5. Set 'Renewal due date' to a date two weeks out, and set 'On hold due date' to a clearly distinct date one month out. 6. Click 'Renew or check in selected items'. 7. Without this patch: the new issues.date_due for item A matches the two-week 'Renewal due date' (the override is ignored). With this patch: issues.date_due matches the one-month 'On hold due date' value. 8. Remove the hold (or pick a different item without a hold), repeat, and confirm the 'Renewal due date' value is used -- non-on-hold renewals are unchanged. References: Bug 7088 (original feature), Bug 36084 (regression). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42432 --- Comment #2 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 197928 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197928&action=edit Bug 42432: Add Cypress regression tests for 'On hold due date' Add integration tests covering the two branches of the renewal due-date picker in the patron checkouts table: 1. When a row's item has an outstanding hold, ticking the Renew checkbox exposes the 'On hold due date' input; the value entered there must be the date_due sent to /cgi-bin/koha/svc/renew when 'Renew or check in selected items' is clicked. This is the branch that was silently broken by Bug 36084. 2. When a row's item has no hold, the 'On hold due date' input is not relevant; the value in 'Renewal due date' must be used instead. This guards against regressions that would flip the branches. The specs intercept POST /cgi-bin/koha/svc/renew (the form-urlencoded endpoint invoked by CirculationAPIClient.checkouts.renew) and assert directly on the 'date_due' form parameter in the request body, which is the narrowest stable observable for this behaviour. Test plan: 1. Apply the previous patch in this series. 2. Run: npx cypress run --spec \\ t/cypress/integration/Circulation/OnHoldDueDate_spec.ts 3. Both tests pass. 4. Temporarily revert the previous patch so the JS fix is undone. 5. Re-run the spec; the first test ('sends the On hold due date value to svc/renew for on-hold items') fails and the second test still passes -- demonstrating that the spec captures the regression. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42432 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA CC| |david@davidnind.com --- Comment #3 from David Nind <david@davidnind.com> --- I've failed QA as the new tests don't pass (things work as per the first patch, once I got my head around it). This occurs for me when running the tests with both patches, or with just the test patch. Test failures: 0 passing (50s) 2 failing 1) Circulation - AllowRenewalOnHoldOverride 'On hold due date' sends the 'On hold due date' value to svc/renew for on-hold items: AssertionError: Timed out retrying after 20000ms: expected '<table#issues-table>' to be 'visible' This element `<table#issues-table>` is not visible because it has CSS property: `display: none` at Context.eval (webpack://koha/./t/cypress/integration/Circulation/OnHoldDueDate_spec.ts:38:52) 2) Circulation - AllowRenewalOnHoldOverride 'On hold due date' falls back to 'Renewal due date' for non-on-hold items: AssertionError: Timed out retrying after 20000ms: expected '<table#issues-table>' to be 'visible' This element `<table#issues-table>` is not visible because it has CSS property: `display: none` at Context.eval (webpack://koha/./t/cypress/integration/Circulation/OnHoldDueDate_spec.ts:75:60) (Results) ┌────────────────────────────────────────┐ │ Tests: 2 │ │ Passing: 0 │ │ Failing: 2 │ │ Pending: 0 │ │ Skipped: 0 │ │ Screenshots: 2 │ │ Video: true │ │ Duration: 50 seconds │ │ Spec Ran: OnHoldDueDate_spec.ts │ └────────────────────────────────────────┘ ..... Spec Tests Passing Failing Pending Skipped ┌──────────────────────────────────────────────────────────────────────────────────┐ │ ✖ OnHoldDueDate_spec.ts 00:50 2 - 2 - - │ └──────────────────────────────────────────────────────────────────────────────────┘ ✖ 1 of 1 failed (100%) 00:50 2 - 2 - - -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42432 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42432 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197927|0 |1 is obsolete| | Attachment #197928|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42432 --- Comment #4 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 200261 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200261&action=edit Bug 42432: Restore 'On hold due date' on patron checkouts table The 'On hold due date' input in the patron checkouts table footer (shown when AllowRenewalOnHoldOverride is enabled and a renewal row is on reserve) is currently ignored: editing it has no effect, and on-hold items are renewed using the 'Renewal due date' value instead. This is a regression from Bug 36084 (commit 99d776f58e0, Feb 2024). That patch ported the renewal AJAX call to use CirculationAPIClient.checkouts.renew() and, in the process, dropped the pre-existing conditional that picked between '#newonholdduedate' (for on-reserve rows) and '#newduedate' (for regular rows). The template input and its show/hide + mirror JS were left in place, so the field still appears and accepts input -- its value is just no longer sent. This patch restores the original behaviour introduced by Bug 7088: for each renewed row, if the row's checkbox has the 'data-on-reserve' attribute, the value from '#newonholdduedate' is used as the renewal due date; otherwise '#newduedate' is used, matching the rest of the table. We identify the row by its hidden value attribute (input.renew[value='<item_id>']) rather than by the checkbox id '#renew_<item_id>', because renew_all() injects a spinner <img> that shares the same id while the request is in flight. Test plan: 1. Set 'AllowRenewalOnHoldOverride' to 'Allow'. 2. Check item A out to patron P1. 3. Place a hold on item A for a different patron P2. 4. Open P1's patron details page and tick the Renew checkbox on the row for item A. Confirm the 'On hold due date' input appears below the 'Renewal due date' input. 5. Set 'Renewal due date' to a date two weeks out, and set 'On hold due date' to a clearly distinct date one month out. 6. Click 'Renew or check in selected items'. 7. Without this patch: the new issues.date_due for item A matches the two-week 'Renewal due date' (the override is ignored). With this patch: issues.date_due matches the one-month 'On hold due date' value. 8. Remove the hold (or pick a different item without a hold), repeat, and confirm the 'Renewal due date' value is used -- non-on-hold renewals are unchanged. References: Bug 7088 (original feature), Bug 36084 (regression). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42432 --- Comment #5 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 200262 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200262&action=edit Bug 42432: Add Cypress regression tests for 'On hold due date' Add integration tests covering the two branches of the renewal due-date picker in the patron checkouts table: 1. When a row's item has an outstanding hold, ticking the Renew checkbox exposes the 'On hold due date' input; the value entered there must be the date_due sent to /cgi-bin/koha/svc/renew when 'Renew or check in selected items' is clicked. This is the branch that was silently broken by Bug 36084. 2. When a row's item has no hold, the 'On hold due date' input is not relevant; the value in 'Renewal due date' must be used instead. This guards against regressions that would flip the branches. The specs intercept POST /cgi-bin/koha/svc/renew (the form-urlencoded endpoint invoked by CirculationAPIClient.checkouts.renew) and assert directly on the 'date_due' form parameter in the request body, which is the narrowest stable observable for this behaviour. Test plan: 1. Apply the previous patch in this series. 2. Run: npx cypress run --spec \\ t/cypress/integration/Circulation/OnHoldDueDate_spec.ts 3. Both tests pass. 4. Temporarily revert the previous patch so the JS fix is undone. 5. Re-run the spec; the first test ('sends the On hold due date value to svc/renew for on-hold items') fails and the second test still passes -- demonstrating that the spec captures the regression. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42432 --- Comment #6 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Thanks for testing David.. seems a few new preferences crept in since first submission that meant the tests failed.. setting them in the test helps so they should pass now. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42432 --- Comment #7 from David Nind <david@davidnind.com> --- The tests are still failing for me (everything else in the first patch works as expected). Testing notes (using KTD): 1. Apply the second patch (cypress tests) 2. yarn build (don't think this is required) 3. restart_all 4. Tests fail (doesn't matter if AllowRenewalOnHoldOverride is set to allow or not) cypress run --spec t/cypress/integration/Circulation/OnHoldDueDate_spec.ts 5. Reset everything as normal: - git checkout main - git branch -D bz42432 - git reset --hard oriin/main - reset_all 6. Apply both patches 7. Tests fail (doesn't matter if AllowRenewalOnHoldOverride is set to allow or not, although different results) Cypress regression patch only ============================= With AllowRenewalOnHoldOverride set to don't allow -------------------------------------------------- Circulation - AllowRenewalOnHoldOverride 'On hold due date' 1) sends the 'On hold due date' value to svc/renew for on-hold items ✓ falls back to 'Renewal due date' for non-on-hold items (5303ms) 1 passing (22s) 1 failing 1) Circulation - AllowRenewalOnHoldOverride 'On hold due date' sends the 'On hold due date' value to svc/renew for on-hold items: CypressError: Timed out retrying after 10050ms: `cy.check()` failed because this element is not visible: `<input type="checkbox" data-on-reserve="" class="renew" id="renew_973" name="renew" value="973">` This element `<input#renew_973.renew>` is not visible because its parent `<span.renewals-allowed-on_reserve>` has CSS property: `display: none` Fix this problem, or use `{force: true}` to disable error checking. https://on.cypress.io/element-cannot-be-interacted-with .... With AllowRenewalOnHoldOverride set to allow -------------------------------------------- Circulation - AllowRenewalOnHoldOverride 'On hold due date' 1) sends the 'On hold due date' value to svc/renew for on-hold items ✓ falls back to 'Renewal due date' for non-on-hold items (5313ms) 1 passing (12s) 1 failing 1) Circulation - AllowRenewalOnHoldOverride 'On hold due date' sends the 'On hold due date' value to svc/renew for on-hold items: AssertionError: expected '2099-05-05 23:59' to equal '2099-06-15 23:59' + expected - actual -'2099-05-05 23:59' +'2099-06-15 23:59' Both patches applied ==================== With AllowRenewalOnHoldOverride set to don't allow -------------------------------------------------- Running: OnHoldDueDate_spec.ts (1 of 1) Circulation - AllowRenewalOnHoldOverride 'On hold due date' ✓ sends the 'On hold due date' value to svc/renew for on-hold items (6546ms) 1) falls back to 'Renewal due date' for non-on-hold items 1 passing (23s) 1 failing 1) Circulation - AllowRenewalOnHoldOverride 'On hold due date' falls back to 'Renewal due date' for non-on-hold items: CypressError: Timed out retrying after 10050ms: `cy.check()` failed because the center of this element is hidden from view: `<input type="checkbox" class="renew" id="renew_981" name="renew" value="981">` Fix this problem, or use `{force: true}` to disable error checking. https://on.cypress.io/element-cannot-be-interacted-with ... With AllowRenewalOnHoldOverride set to allow -------------------------------------------- Running: OnHoldDueDate_spec.ts (1 of 1) Circulation - AllowRenewalOnHoldOverride 'On hold due date' 1) sends the 'On hold due date' value to svc/renew for on-hold items ✓ falls back to 'Renewal due date' for non-on-hold items (5619ms) 1 passing (22s) 1 failing 1) Circulation - AllowRenewalOnHoldOverride 'On hold due date' sends the 'On hold due date' value to svc/renew for on-hold items: CypressError: Timed out retrying after 10050ms: `cy.check()` failed because this element is not visible: `<input type="checkbox" data-on-reserve="" class="renew" id="renew_973" name="renew" value="973">` This element `<input#renew_973.renew>` is not visible because its parent `<span.renewals-allowed-on_reserve>` has CSS property: `display: none` Fix this problem, or use `{force: true}` to disable error checking. https://on.cypress.io/element-cannot-be-interacted-with -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42432 Sally <sally.healey@cheshiresharedservices.gov.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |sally.healey@cheshireshared | |services.gov.uk -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org