From f9c53e00ac1d63c093583455f1a3697f14bd43df Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Fri, 23 Jan 2026 07:21:36 +0000 Subject: [PATCH] Bug 37707: (follow-up) Support lead/trail periods for 'any item' bookings When booking "any item of type X", lead/trail period handling must consider that multiple items may be available: - Track per-item conflicts instead of global conflicts - Only disable dates when ALL items of the type have conflicts - Find closest "all items booked" dates for visual feedback - Use mathematical search (180 days) to detect conflicts across month boundaries Visual feedback for 'any item' mode: - Show lead/trail periods around "all items booked" dates - These represent dates where no item of the type is available Skip redundant class-based conflict checks since mathematical detection already handles cross-month boundary cases. Signed-off-by: Paul Derscheid Signed-off-by: Nick Clemens --- .../prog/js/modals/place_booking.js | 338 ++++++++++++++++-- .../Circulation/bookingsModalBasic_spec.ts | 315 ++++++++++++++++ 2 files changed, 614 insertions(+), 39 deletions(-) diff --git a/koha-tmpl/intranet-tmpl/prog/js/modals/place_booking.js b/koha-tmpl/intranet-tmpl/prog/js/modals/place_booking.js index ed0fe2aebca..e3c04692ff8 100644 --- a/koha-tmpl/intranet-tmpl/prog/js/modals/place_booking.js +++ b/koha-tmpl/intranet-tmpl/prog/js/modals/place_booking.js @@ -1135,6 +1135,47 @@ $("#placeBookingModal").on("show.bs.modal", function (e) { withBooking: false, }; + // For "any item" mode, we need to check if at least one item + // of the selected itemtype is free from lead/trail conflicts. + // For specific item mode, we use the original single-item logic. + const isAnyItemMode = + !booking_item_id && booking_itemtype_id; + + // Get items of the selected itemtype for "any item" mode + let itemsOfSelectedType = []; + if (isAnyItemMode) { + itemsOfSelectedType = bookable_items.filter( + item => + item.effective_item_type_id === + booking_itemtype_id + ); + } + + // Track per-item conflicts for "any item" mode + // Maps item_id -> { leadConflict: bool, trailConflict: bool, leadReason: {...}, trailReason: {...} } + const itemConflicts = new Map(); + if (isAnyItemMode) { + itemsOfSelectedType.forEach(item => { + itemConflicts.set( + parseInt(item.item_id, 10), + { + leadConflict: false, + trailConflict: false, + leadReason: { + withTrail: false, + withLead: false, + withBooking: false, + }, + trailReason: { + withTrail: false, + withLead: false, + withBooking: false, + }, + } + ); + }); + } + bookings.forEach(booking => { // Skip if we're editing this booking if ( @@ -1144,13 +1185,26 @@ $("#placeBookingModal").on("show.bs.modal", function (e) { return; } - // Skip if not same item (for item-specific bookings) - if ( - booking.item_id && - booking_item_id && - booking.item_id != booking_item_id - ) { - return; + const bookingItemId = parseInt( + booking.item_id, + 10 + ); + + // For specific item mode: skip bookings for different items + if (!isAnyItemMode) { + if ( + booking.item_id && + booking_item_id && + bookingItemId !== + parseInt(booking_item_id, 10) + ) { + return; + } + } else { + // For "any item" mode: skip bookings for items not of the selected itemtype + if (!itemConflicts.has(bookingItemId)) { + return; + } } const bookingStart = dayjs( @@ -1181,6 +1235,13 @@ $("#placeBookingModal").on("show.bs.modal", function (e) { // Check if new booking's LEAD period overlaps with existing booking if (!periodPicker.selectedDates[0]) { + let hasLeadConflict = false; + let reason = { + withTrail: false, + withLead: false, + withBooking: false, + }; + // Check overlap with existing booking's trail period if ( leadStart.isSameOrBefore( @@ -1190,8 +1251,8 @@ $("#placeBookingModal").on("show.bs.modal", function (e) { existingTrailStart ) ) { - leadDisable = true; - leadConflictReason.withTrail = true; + hasLeadConflict = true; + reason.withTrail = true; } // Check overlap with existing booking's lead period else if ( @@ -1200,21 +1261,52 @@ $("#placeBookingModal").on("show.bs.modal", function (e) { ) && leadEnd.isSameOrAfter(existingLeadStart) ) { - leadDisable = true; - leadConflictReason.withLead = true; + hasLeadConflict = true; + reason.withLead = true; } // Check overlap with existing booking itself else if ( leadStart.isSameOrBefore(bookingEnd) && leadEnd.isSameOrAfter(bookingStart) ) { - leadDisable = true; - leadConflictReason.withBooking = true; + hasLeadConflict = true; + reason.withBooking = true; + } + + if (hasLeadConflict) { + if (isAnyItemMode) { + // Track conflict for this specific item + const itemState = + itemConflicts.get( + bookingItemId + ); + if (itemState) { + itemState.leadConflict = true; + Object.assign( + itemState.leadReason, + reason + ); + } + } else { + // Specific item mode: set global flags + leadDisable = true; + Object.assign( + leadConflictReason, + reason + ); + } } } // Check if new booking's TRAIL period overlaps with existing booking if (periodPicker.selectedDates[0]) { + let hasTrailConflict = false; + let reason = { + withTrail: false, + withLead: false, + withBooking: false, + }; + // Check overlap with existing booking's lead period if ( trailStart.isSameOrBefore( @@ -1224,8 +1316,8 @@ $("#placeBookingModal").on("show.bs.modal", function (e) { existingLeadStart ) ) { - trailDisable = true; - trailConflictReason.withLead = true; + hasTrailConflict = true; + reason.withLead = true; } // Check overlap with existing booking's trail period else if ( @@ -1236,20 +1328,45 @@ $("#placeBookingModal").on("show.bs.modal", function (e) { existingTrailStart ) ) { - trailDisable = true; - trailConflictReason.withTrail = true; + hasTrailConflict = true; + reason.withTrail = true; } // Check overlap with existing booking itself else if ( trailStart.isSameOrBefore(bookingEnd) && trailEnd.isSameOrAfter(bookingStart) ) { - trailDisable = true; - trailConflictReason.withBooking = true; + hasTrailConflict = true; + reason.withBooking = true; + } + + if (hasTrailConflict) { + if (isAnyItemMode) { + // Track conflict for this specific item + const itemState = + itemConflicts.get( + bookingItemId + ); + if (itemState) { + itemState.trailConflict = true; + Object.assign( + itemState.trailReason, + reason + ); + } + } else { + // Specific item mode: set global flags + trailDisable = true; + Object.assign( + trailConflictReason, + reason + ); + } } } // Find closest bookings for visual feedback (when dates are in view) + // For "any item" mode, only track closest bookings for items of the selected type if (bookingEnd.isBefore(hoverDate)) { const distance = hoverDate.diff( bookingEnd, @@ -1279,6 +1396,103 @@ $("#placeBookingModal").on("show.bs.modal", function (e) { } }); + // For "any item" mode: only disable if ALL items have conflicts + if (isAnyItemMode && itemConflicts.size > 0) { + // Check if all items have lead conflicts + let allHaveLeadConflict = true; + let allHaveTrailConflict = true; + + for (const [ + itemId, + state, + ] of itemConflicts.entries()) { + if (!state.leadConflict) { + allHaveLeadConflict = false; + } + if (!state.trailConflict) { + allHaveTrailConflict = false; + } + } + + if (allHaveLeadConflict) { + leadDisable = true; + // Use the reason from the first item with a conflict for messaging + for (const [ + itemId, + state, + ] of itemConflicts.entries()) { + if (state.leadConflict) { + Object.assign( + leadConflictReason, + state.leadReason + ); + break; + } + } + } + + if (allHaveTrailConflict) { + trailDisable = true; + // Use the reason from the first item with a conflict for messaging + for (const [ + itemId, + state, + ] of itemConflicts.entries()) { + if (state.trailConflict) { + Object.assign( + trailConflictReason, + state.trailReason + ); + break; + } + } + } + } + + // For "any item" mode, find closest "all items booked" dates mathematically + // These are dates where ALL items of the itemtype have bookings + // Using mathematical search allows detection across month boundaries + let closestFullyBookedBefore = null; + let closestFullyBookedAfter = null; + + if ( + isAnyItemMode && + itemsOfSelectedType.length > 0 + ) { + const searchLimit = 180; // Days to search in each direction + + // Search backwards for closest fully-booked date + for (let i = 1; i <= searchLimit; i++) { + const checkDate = hoverDate.subtract( + i, + "day" + ); + const availableItems = + getAvailableItemsOnDate( + checkDate.toDate(), + itemsOfSelectedType + ); + if (availableItems.length === 0) { + closestFullyBookedBefore = checkDate; + break; + } + } + + // Search forwards for closest fully-booked date + for (let i = 1; i <= searchLimit; i++) { + const checkDate = hoverDate.add(i, "day"); + const availableItems = + getAvailableItemsOnDate( + checkDate.toDate(), + itemsOfSelectedType + ); + if (availableItems.length === 0) { + closestFullyBookedAfter = checkDate; + break; + } + } + } + // Work through all days in view and add classes appropraitely based on hovered date periodPicker.calendarContainer .querySelectorAll(".flatpickr-day") @@ -1346,18 +1560,39 @@ $("#placeBookingModal").on("show.bs.modal", function (e) { ); } - // Show closest preceding existing booking's trail period - if (closestBeforeBooking && trailDays > 0) { + // Show closest preceding booking's trail period + // For "any item" mode, use closest fully-booked date; for specific item, use closest booking + const useClosestFullyBookedForTrail = + isAnyItemMode && + closestFullyBookedBefore; + const useClosestBookingForTrail = + !isAnyItemMode && closestBeforeBooking; + + if ( + trailDays > 0 && + (useClosestFullyBookedForTrail || + useClosestBookingForTrail) + ) { const existingTrailStart = - closestBeforeBooking.end.add( - 1, - "day" - ); + useClosestFullyBookedForTrail + ? closestFullyBookedBefore.add( + 1, + "day" + ) + : closestBeforeBooking.end.add( + 1, + "day" + ); const existingTrailEnd = - closestBeforeBooking.end.add( - trailDays, - "day" - ); + useClosestFullyBookedForTrail + ? closestFullyBookedBefore.add( + trailDays, + "day" + ) + : closestBeforeBooking.end.add( + trailDays, + "day" + ); if ( elemDate.isSameOrAfter( @@ -1392,18 +1627,39 @@ $("#placeBookingModal").on("show.bs.modal", function (e) { } } - // Show closest following existing booking's lead period - if (closestAfterBooking && leadDays > 0) { + // Show closest following booking's lead period + // For "any item" mode, use closest fully-booked date; for specific item, use closest booking + const useClosestFullyBookedForLead = + isAnyItemMode && + closestFullyBookedAfter; + const useClosestBookingForLead = + !isAnyItemMode && closestAfterBooking; + + if ( + leadDays > 0 && + (useClosestFullyBookedForLead || + useClosestBookingForLead) + ) { const existingLeadStart = - closestAfterBooking.start.subtract( - leadDays, - "day" - ); + useClosestFullyBookedForLead + ? closestFullyBookedAfter.subtract( + leadDays, + "day" + ) + : closestAfterBooking.start.subtract( + leadDays, + "day" + ); const existingLeadEnd = - closestAfterBooking.start.subtract( - 1, - "day" - ); + useClosestFullyBookedForLead + ? closestFullyBookedAfter.subtract( + 1, + "day" + ) + : closestAfterBooking.start.subtract( + 1, + "day" + ); if ( elemDate.isSameOrAfter( @@ -1475,6 +1731,7 @@ $("#placeBookingModal").on("show.bs.modal", function (e) { } // Check for conflicts with existing booking's trail period + // In "any item" mode, these classes now represent "all items booked" periods if ( !periodPicker.selectedDates[0] && dayElem.classList.contains( @@ -1491,6 +1748,7 @@ $("#placeBookingModal").on("show.bs.modal", function (e) { } // Check for conflicts with existing booking's lead period + // In "any item" mode, these classes now represent "all items booked" periods if ( periodPicker.selectedDates[0] && dayElem.classList.contains( @@ -1518,6 +1776,7 @@ $("#placeBookingModal").on("show.bs.modal", function (e) { }); // Additional check for hovering directly on existing booking's lead/trail periods + // In "any item" mode, these classes now represent "all items booked" periods // If hovering on an existing booking's lead period when selecting start date, block selection if ( !periodPicker.selectedDates[0] && @@ -1527,6 +1786,7 @@ $("#placeBookingModal").on("show.bs.modal", function (e) { } // If hovering on an existing booking's trail period when selecting end date, block selection + // In "any item" mode, these classes now represent "all items booked" periods if ( periodPicker.selectedDates[0] && target.classList.contains( diff --git a/t/cypress/integration/Circulation/bookingsModalBasic_spec.ts b/t/cypress/integration/Circulation/bookingsModalBasic_spec.ts index 6342961cd39..82dab696bc9 100644 --- a/t/cypress/integration/Circulation/bookingsModalBasic_spec.ts +++ b/t/cypress/integration/Circulation/bookingsModalBasic_spec.ts @@ -1485,4 +1485,319 @@ describe("Booking Modal Basic Tests", () => { } }); }); + + it("should correctly handle lead/trail period conflicts for 'any item' bookings", () => { + /** + * Bug 37707: Lead/Trail Period Conflict Detection for "Any Item" Bookings + * ======================================================================== + * + * This test validates that lead/trail period conflict detection works correctly + * when "any item of itemtype X" is selected. The key principle is: + * + * - Only block date selection when ALL items of the itemtype have conflicts + * - Allow selection when at least one item is free from lead/trail conflicts + * + * The bug occurred because the mouseover handler was checking conflicts against + * ALL bookings regardless of itemtype, rather than tracking per-item conflicts. + * + * Test Setup: + * =========== + * - 3 items of itemtype BK + * - Lead period: 2 days, Trail period: 2 days + * - ITEM 0: Booking on days 10-12 (trail period: 13-14) + * - ITEM 1: Booking on days 10-12 (same as item 0) + * - ITEM 2: No bookings (always available) + * + * Test Scenarios: + * ============== + * 1. Hover day 15: ITEM 0 and ITEM 1 have trail period conflict (lead period + * June 13-14 overlaps their trail June 13-14), but ITEM 2 is free + * → Should NOT be blocked (at least one item available) + * + * 2. Create booking on ITEM 2 for days 10-12, then hover day 15 again: + * → ALL items now have trail period conflicts + * → Should BE blocked + */ + + const today = dayjs(); + let testItems = []; + let testBiblio = null; + let testPatron = null; + let testLibraries = null; + + // Circulation rules with non-zero lead/trail periods + const circulationRules = { + bookings_lead_period: 2, + bookings_trail_period: 2, + issuelength: 14, + renewalsallowed: 2, + renewalperiod: 7, + }; + + // Setup: Create biblio with 3 items of the same itemtype + cy.task("insertSampleBiblio", { item_count: 3 }) + .then(objects => { + testBiblio = objects.biblio; + testItems = objects.items; + testLibraries = objects.libraries; + + // Make all items the same itemtype (BK) + const itemUpdates = testItems.map((item, index) => { + const enumchron = String.fromCharCode(65 + index); + return cy.task("query", { + sql: "UPDATE items SET bookable = 1, itype = 'BK', homebranch = 'CPL', enumchron = ?, dateaccessioned = ? WHERE itemnumber = ?", + values: [ + enumchron, + `2024-12-0${4 - index}`, + item.item_id, + ], + }); + }); + return Promise.all(itemUpdates); + }) + .then(() => { + return cy.task("buildSampleObject", { + object: "patron", + values: { + firstname: "LeadTrail", + surname: "Tester", + cardnumber: `LT${Date.now()}`, + category_id: "PT", + library_id: "CPL", + }, + }); + }) + .then(mockPatron => { + testPatron = mockPatron; + return cy.task("query", { + sql: `INSERT INTO borrowers (borrowernumber, firstname, surname, cardnumber, categorycode, branchcode, dateofbirth) + VALUES (?, ?, ?, ?, ?, ?, ?)`, + values: [ + mockPatron.patron_id, + mockPatron.firstname, + mockPatron.surname, + mockPatron.cardnumber, + mockPatron.category_id, + mockPatron.library_id, + "1990-01-01", + ], + }); + }) + .then(() => { + // Create bookings on ITEM 0 and ITEM 1 for days 10-12 + // ITEM 2 remains free + const bookingInserts = [ + // ITEM 0: Booked days 10-12 + cy.task("query", { + sql: `INSERT INTO bookings (biblio_id, patron_id, item_id, pickup_library_id, start_date, end_date, status) + VALUES (?, ?, ?, ?, ?, ?, ?)`, + values: [ + testBiblio.biblio_id, + testPatron.patron_id, + testItems[0].item_id, + "CPL", + today.add(10, "day").format("YYYY-MM-DD"), + today.add(12, "day").format("YYYY-MM-DD"), + "new", + ], + }), + // ITEM 1: Booked days 10-12 (same period) + cy.task("query", { + sql: `INSERT INTO bookings (biblio_id, patron_id, item_id, pickup_library_id, start_date, end_date, status) + VALUES (?, ?, ?, ?, ?, ?, ?)`, + values: [ + testBiblio.biblio_id, + testPatron.patron_id, + testItems[1].item_id, + "CPL", + today.add(10, "day").format("YYYY-MM-DD"), + today.add(12, "day").format("YYYY-MM-DD"), + "new", + ], + }), + // ITEM 2: No booking - remains free + ]; + return Promise.all(bookingInserts); + }) + .then(() => { + cy.intercept( + "GET", + `/api/v1/biblios/${testBiblio.biblio_id}/pickup_locations*` + ).as("getPickupLocations"); + cy.intercept("GET", "/api/v1/circulation_rules*", { + body: [circulationRules], + }).as("getCirculationRules"); + + cy.visit( + `/cgi-bin/koha/catalogue/detail.pl?biblionumber=${testBiblio.biblio_id}` + ); + + cy.get('[data-bs-target="#placeBookingModal"]').first().click(); + cy.get("#placeBookingModal").should("be.visible"); + + cy.selectFromSelect2( + "#booking_patron_id", + `${testPatron.surname}, ${testPatron.firstname}`, + testPatron.cardnumber + ); + cy.wait("@getPickupLocations"); + + cy.get("#pickup_library_id").should("not.be.disabled"); + cy.selectFromSelect2ByIndex("#pickup_library_id", 0); + + // Select itemtype BK + cy.get("#booking_itemtype").should("not.be.disabled"); + cy.selectFromSelect2("#booking_itemtype", "Books"); + cy.wait("@getCirculationRules"); + + // Select "Any item" (index 0) + cy.selectFromSelect2ByIndex("#booking_item_id", 0); + cy.get("#booking_item_id").should("have.value", "0"); + + cy.get("#period").should("not.be.disabled"); + cy.get("#period").as("flatpickrInput"); + + // ================================================================ + // SCENARIO 1: Hover day 15 - ITEM 2 is free, should NOT be blocked + // ================================================================ + cy.log( + "=== Scenario 1: Day 15 should be selectable (ITEM 2 is free) ===" + ); + + /** + * Day 15 as start date: + * - Lead period: days 13-14 + * - ITEM 0's trail period: days 13-14 (booking ended day 12, trail = 2 days) + * - ITEM 1's trail period: days 13-14 (same) + * - ITEM 2: No booking, no trail period conflict + * + * The new booking's lead period (13-14) overlaps with ITEM 0 and ITEM 1's + * trail period, but ITEM 2 has no conflict. + * + * With the bug, this would be blocked because ANY booking conflicted. + * With the fix, this should be allowed because ITEM 2 is available. + */ + + cy.get("@flatpickrInput").openFlatpickr(); + cy.get("@flatpickrInput") + .getFlatpickrDate(today.add(15, "day").toDate()) + .trigger("mouseover"); + + // Day 15 should NOT have leadDisable class (at least one item is free) + cy.get("@flatpickrInput") + .getFlatpickrDate(today.add(15, "day").toDate()) + .should("not.have.class", "leadDisable"); + + cy.log( + "✓ Day 15 is selectable - lead period conflict detection correctly allows selection when one item is free" + ); + + // Actually click day 15 to verify it's selectable + cy.get("@flatpickrInput") + .getFlatpickrDate(today.add(15, "day").toDate()) + .should("not.have.class", "flatpickr-disabled") + .click(); + + // Verify day 15 was selected as start date + cy.get("@flatpickrInput") + .getFlatpickrDate(today.add(15, "day").toDate()) + .should("have.class", "selected"); + + cy.log( + "✓ CONFIRMED: Day 15 successfully selected as start date" + ); + + // Reset for next scenario + cy.get("@flatpickrInput").clearFlatpickr(); + + // ================================================================ + // SCENARIO 2: Add booking on ITEM 2 - ALL items now have conflicts + // ================================================================ + cy.log( + "=== Scenario 2: Day 15 should be BLOCKED when all items have conflicts ===" + ); + + // Add booking on ITEM 2 for same period (days 10-12) + cy.task("query", { + sql: `INSERT INTO bookings (biblio_id, patron_id, item_id, pickup_library_id, start_date, end_date, status) + VALUES (?, ?, ?, ?, ?, ?, ?)`, + values: [ + testBiblio.biblio_id, + testPatron.patron_id, + testItems[2].item_id, + "CPL", + today.add(10, "day").format("YYYY-MM-DD"), + today.add(12, "day").format("YYYY-MM-DD"), + "new", + ], + }).then(() => { + // Reload page to get updated booking data + cy.visit( + `/cgi-bin/koha/catalogue/detail.pl?biblionumber=${testBiblio.biblio_id}` + ); + + cy.get('[data-bs-target="#placeBookingModal"]') + .first() + .click(); + cy.get("#placeBookingModal").should("be.visible"); + + cy.selectFromSelect2( + "#booking_patron_id", + `${testPatron.surname}, ${testPatron.firstname}`, + testPatron.cardnumber + ); + cy.wait("@getPickupLocations"); + + cy.get("#pickup_library_id").should("not.be.disabled"); + cy.selectFromSelect2ByIndex("#pickup_library_id", 0); + + // Select itemtype BK + cy.get("#booking_itemtype").should("not.be.disabled"); + cy.selectFromSelect2("#booking_itemtype", "Books"); + cy.wait("@getCirculationRules"); + + // Select "Any item" (index 0) + cy.selectFromSelect2ByIndex("#booking_item_id", 0); + cy.get("#booking_item_id").should("have.value", "0"); + + cy.get("#period").should("not.be.disabled"); + cy.get("#period").as("flatpickrInput2"); + + cy.get("@flatpickrInput2").openFlatpickr(); + cy.get("@flatpickrInput2") + .getFlatpickrDate(today.add(15, "day").toDate()) + .trigger("mouseover"); + + // Day 15 should NOW have leadDisable class (all items have conflicts) + cy.get("@flatpickrInput2") + .getFlatpickrDate(today.add(15, "day").toDate()) + .should("have.class", "leadDisable"); + + cy.log( + "✓ Day 15 is BLOCKED - all items have lead period conflicts" + ); + }); + }); + + // Cleanup + cy.then(() => { + if (testBiblio) { + cy.task("query", { + sql: "DELETE FROM bookings WHERE biblio_id = ?", + values: [testBiblio.biblio_id], + }); + cy.task("deleteSampleObjects", { + biblio: testBiblio, + items: testItems, + libraries: testLibraries, + }); + } + if (testPatron) { + cy.task("query", { + sql: "DELETE FROM borrowers WHERE borrowernumber = ?", + values: [testPatron.patron_id], + }); + } + }); + }); }); -- 2.39.5