From 28740124fea2b6c202e3ed83fc27f6f2571ac5bf Mon Sep 17 00:00:00 2001 From: Paul Derscheid Date: Fri, 20 Feb 2026 14:24:21 +0100 Subject: [PATCH] Bug 41129: Fix booking modal runtime and test regressions Runtime: - Add missing use C4::Context in Libraries.pm - Use safe DOM construction instead of innerHTML in external-dependents.mjs (booking count update, success message) - Remove reference to undefined BookingDateRangeConstraint syspref - Stub OPAC fetchHolidays (public endpoint does not exist) - Pass UI visibility flags to useBookingValidation as reactive parameter instead of reading from store Type/test corrections: - Remove duplicate ItemType type declaration in bookings.d.ts - Remove overly broad uncaught:exception handler from e2e.js - Stabilise DatePicker spec against flatpickr navigation races - Correct fencepost in max booking period calculation --- Koha/REST/V1/Libraries.pm | 1 + .../en/includes/modals/booking/island.inc | 2 +- .../vue/components/Bookings/BookingModal.vue | 8 +- .../composables/useBookingValidation.mjs | 49 +----- .../Bookings/lib/adapters/api/opac.js | 38 ++--- .../lib/adapters/external-dependents.mjs | 23 +-- .../components/Bookings/types/bookings.d.ts | 6 - .../bookingsModalDatePicker_spec.ts | 142 +++++++++--------- t/cypress/support/e2e.js | 13 -- t/cypress/support/flatpickr.js | 45 +++--- 10 files changed, 135 insertions(+), 192 deletions(-) diff --git a/Koha/REST/V1/Libraries.pm b/Koha/REST/V1/Libraries.pm index 5c940fdaf40..2975bc9a893 100644 --- a/Koha/REST/V1/Libraries.pm +++ b/Koha/REST/V1/Libraries.pm @@ -18,6 +18,7 @@ package Koha::REST::V1::Libraries; use Modern::Perl; use Mojo::Base 'Mojolicious::Controller'; +use C4::Context; use Koha::Libraries; use Koha::Calendar; use Koha::DateUtils qw( dt_from_string ); diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/modals/booking/island.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/modals/booking/island.inc index 2403f811259..df436d8830f 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/modals/booking/island.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/modals/booking/island.inc @@ -6,7 +6,7 @@ show-patron-select show-item-details-selects show-pickup-location-select - date-range-constraint="[% (Koha.Preference('BookingDateRangeConstraint') || 'issuelength_with_renewals') | html %]" [%# FIXME: issuelength_with_renewals needs to be the db default, don't constrain needs a value %] + date-range-constraint="issuelength_with_renewals" > [% SET islands = Asset.js("js/vue/dist/islands.esm.js").match('(src="([^"]+)")').1 %] diff --git a/koha-tmpl/intranet-tmpl/prog/js/vue/components/Bookings/BookingModal.vue b/koha-tmpl/intranet-tmpl/prog/js/vue/components/Bookings/BookingModal.vue index d9b3d9ba4f7..3158577372e 100644 --- a/koha-tmpl/intranet-tmpl/prog/js/vue/components/Bookings/BookingModal.vue +++ b/koha-tmpl/intranet-tmpl/prog/js/vue/components/Bookings/BookingModal.vue @@ -272,8 +272,6 @@ const { loading, } = storeToRefs(store); -const { canSubmit: canSubmitReactive } = useBookingValidation(store); - const modalState = reactive({ isOpen: props.open, step: 1, @@ -297,6 +295,12 @@ const showPickupLocationSelect = computed(() => { return props.showPickupLocationSelect; }); +const { canSubmit: canSubmitReactive } = useBookingValidation(store, { + showPatronSelect: computed(() => props.showPatronSelect), + showItemDetailsSelects: computed(() => props.showItemDetailsSelects), + showPickupLocationSelect, +}); + const stepNumber = computed(() => { return calculateStepNumbers( props.showPatronSelect, diff --git a/koha-tmpl/intranet-tmpl/prog/js/vue/components/Bookings/composables/useBookingValidation.mjs b/koha-tmpl/intranet-tmpl/prog/js/vue/components/Bookings/composables/useBookingValidation.mjs index 4d24c68cebb..b7c88d51d5e 100644 --- a/koha-tmpl/intranet-tmpl/prog/js/vue/components/Bookings/composables/useBookingValidation.mjs +++ b/koha-tmpl/intranet-tmpl/prog/js/vue/components/Bookings/composables/useBookingValidation.mjs @@ -5,18 +5,15 @@ import { computed } from "vue"; import { storeToRefs } from "pinia"; -import { - canProceedToStep3, - canSubmitBooking, -} from "../lib/booking/validation.mjs"; -import { handleBookingDateChange } from "../lib/booking/availability.mjs"; +import { canSubmitBooking } from "../lib/booking/validation.mjs"; /** * Composable for booking validation with reactive state * @param {Object} store - Pinia booking store instance - * @returns {Object} Reactive validation properties and methods + * @param {{ showPatronSelect: import('vue').Ref|boolean, showItemDetailsSelects: import('vue').Ref|boolean, showPickupLocationSelect: import('vue').Ref|boolean }} uiFlags - Reactive UI visibility flags (props or refs) + * @returns {{ canSubmit: import('vue').ComputedRef }} */ -export function useBookingValidation(store) { +export function useBookingValidation(store, uiFlags = {}) { const { bookingPatron, pickupLibraryId, @@ -25,32 +22,14 @@ export function useBookingValidation(store) { bookingItemId, bookableItems, selectedDateRange, - bookings, - checkouts, - circulationRules, - bookingId, } = storeToRefs(store); - const canProceedToStep3Computed = computed(() => { - return canProceedToStep3({ - showPatronSelect: store.showPatronSelect, - bookingPatron: bookingPatron.value, - showItemDetailsSelects: store.showItemDetailsSelects, - showPickupLocationSelect: store.showPickupLocationSelect, - pickupLibraryId: pickupLibraryId.value, - bookingItemtypeId: bookingItemtypeId.value, - itemtypeOptions: itemTypes.value, - bookingItemId: bookingItemId.value, - bookableItems: bookableItems.value, - }); - }); - const canSubmitComputed = computed(() => { const validationData = { - showPatronSelect: store.showPatronSelect, + showPatronSelect: uiFlags.showPatronSelect?.value ?? uiFlags.showPatronSelect ?? false, bookingPatron: bookingPatron.value, - showItemDetailsSelects: store.showItemDetailsSelects, - showPickupLocationSelect: store.showPickupLocationSelect, + showItemDetailsSelects: uiFlags.showItemDetailsSelects?.value ?? uiFlags.showItemDetailsSelects ?? false, + showPickupLocationSelect: uiFlags.showPickupLocationSelect?.value ?? uiFlags.showPickupLocationSelect ?? false, pickupLibraryId: pickupLibraryId.value, bookingItemtypeId: bookingItemtypeId.value, itemtypeOptions: itemTypes.value, @@ -60,21 +39,7 @@ export function useBookingValidation(store) { return canSubmitBooking(validationData, selectedDateRange.value); }); - const validateDates = selectedDates => { - return handleBookingDateChange( - selectedDates, - circulationRules.value, - bookings.value, - checkouts.value, - bookableItems.value, - bookingItemId.value, - bookingId.value - ); - }; - return { - canProceedToStep3: canProceedToStep3Computed, canSubmit: canSubmitComputed, - validateDates, }; } diff --git a/koha-tmpl/intranet-tmpl/prog/js/vue/components/Bookings/lib/adapters/api/opac.js b/koha-tmpl/intranet-tmpl/prog/js/vue/components/Bookings/lib/adapters/api/opac.js index 2d26f5a967c..ae707541ac7 100644 --- a/koha-tmpl/intranet-tmpl/prog/js/vue/components/Bookings/lib/adapters/api/opac.js +++ b/koha-tmpl/intranet-tmpl/prog/js/vue/components/Bookings/lib/adapters/api/opac.js @@ -231,36 +231,16 @@ export async function fetchCirculationRules(params = {}) { } /** - * Fetches holidays (closed days) for a library - * @param {string} libraryId - The library branchcode - * @param {string} [from] - Start date (ISO format), defaults to today - * @param {string} [to] - End date (ISO format), defaults to 3 months from start - * @returns {Promise} Array of holiday dates in YYYY-MM-DD format - * @throws {Error} If the request fails or returns a non-OK status + * Fetches holidays (closed days) for a library. + * Stub — no public holidays endpoint exists yet; returns empty array + * so the calendar renders without holiday highlighting in OPAC context. + * @param {string} _libraryId - The library branchcode (unused) + * @param {string} [_from] - Start date (unused) + * @param {string} [_to] - End date (unused) + * @returns {Promise} Always returns empty array */ -export async function fetchHolidays(libraryId, from, to) { - if (!libraryId) { - return []; - } - - const params = new URLSearchParams(); - if (from) params.set("from", from); - if (to) params.set("to", to); - - const url = `/api/v1/public/libraries/${encodeURIComponent( - libraryId - )}/holidays${params.toString() ? `?${params.toString()}` : ""}`; - - const response = await fetch(url); - - if (!response.ok) { - throw bookingValidation.validationError("fetch_holidays_failed", { - status: response.status, - statusText: response.statusText, - }); - } - - return await response.json(); +export async function fetchHolidays(_libraryId, _from, _to) { + return []; } export async function createBooking() { diff --git a/koha-tmpl/intranet-tmpl/prog/js/vue/components/Bookings/lib/adapters/external-dependents.mjs b/koha-tmpl/intranet-tmpl/prog/js/vue/components/Bookings/lib/adapters/external-dependents.mjs index be235a949f3..4fd27d8a547 100644 --- a/koha-tmpl/intranet-tmpl/prog/js/vue/components/Bookings/lib/adapters/external-dependents.mjs +++ b/koha-tmpl/intranet-tmpl/prog/js/vue/components/Bookings/lib/adapters/external-dependents.mjs @@ -152,11 +152,9 @@ function updateBookingCounts(isUpdate, dependencies) { let updatedCount = 0; countEls.forEach(el => { - const html = el.innerHTML; - const match = html.match(/(\d+)/); - if (match) { - const newCount = parseInt(match[1], 10) + 1; - el.innerHTML = html.replace(/(\d+)/, String(newCount)); + const current = el.textContent.match(/(\d+)/); + if (current) { + el.textContent = el.textContent.replace(/(\d+)/, String(parseInt(current[1], 10) + 1)); updatedCount++; } }); @@ -191,10 +189,17 @@ function showTransientSuccess(isUpdate, dependencies) { : $__("Booking successfully placed"); const el = container[0] || container; - el.innerHTML = ``; + const alert = document.createElement("div"); + alert.className = "alert alert-success alert-dismissible fade show"; + alert.setAttribute("role", "alert"); + alert.textContent = msg; + const closeBtn = document.createElement("button"); + closeBtn.type = "button"; + closeBtn.className = "btn-close"; + closeBtn.setAttribute("data-bs-dismiss", "alert"); + closeBtn.setAttribute("aria-label", "Close"); + alert.appendChild(closeBtn); + el.replaceChildren(alert); return { success: true }; } catch (error) { diff --git a/koha-tmpl/intranet-tmpl/prog/js/vue/components/Bookings/types/bookings.d.ts b/koha-tmpl/intranet-tmpl/prog/js/vue/components/Bookings/types/bookings.d.ts index 1e63dc70e63..51aa1e22650 100644 --- a/koha-tmpl/intranet-tmpl/prog/js/vue/components/Bookings/types/bookings.d.ts +++ b/koha-tmpl/intranet-tmpl/prog/js/vue/components/Bookings/types/bookings.d.ts @@ -97,12 +97,6 @@ export type CalendarMarker = { barcode: string | null; }; -/** Minimal item type shape used in constraints */ -export type ItemType = { - item_type_id: string; - name?: string; -}; - /** * Result of availability calculation: Flatpickr disable function + daily map. */ diff --git a/t/cypress/integration/Circulation/bookingsModalDatePicker_spec.ts b/t/cypress/integration/Circulation/bookingsModalDatePicker_spec.ts index b649f60d192..c1eb23a0999 100644 --- a/t/cypress/integration/Circulation/bookingsModalDatePicker_spec.ts +++ b/t/cypress/integration/Circulation/bookingsModalDatePicker_spec.ts @@ -453,23 +453,26 @@ describe("Booking Modal Date Picker Tests", () => { // Test in clear zone starting at Day 50 to avoid conflicts const clearZoneStart = today.add(50, "day"); - const calculatedMaxDate = clearZoneStart.add( + // Start date counts as day 1 (Koha convention), so max end = start + (maxPeriod - 1) + const maxPeriod = dateTestCirculationRules.issuelength + - dateTestCirculationRules.renewalsallowed * - dateTestCirculationRules.renewalperiod, + dateTestCirculationRules.renewalsallowed * + dateTestCirculationRules.renewalperiod; // 10 + 3*5 = 25 + const calculatedMaxDate = clearZoneStart.add( + maxPeriod - 1, "day" - ); // Day 50 + 25 = Day 75 + ); // Day 50 + 24 = Day 74 - const beyondMaxDate = calculatedMaxDate.add(1, "day"); // Day 76 + const beyondMaxDate = calculatedMaxDate.add(1, "day"); // Day 75 cy.log( `Clear zone start: ${clearZoneStart.format("YYYY-MM-DD")} (Day 50)` ); cy.log( - `Calculated max date: ${calculatedMaxDate.format("YYYY-MM-DD")} (Day 75)` + `Calculated max date: ${calculatedMaxDate.format("YYYY-MM-DD")} (Day 74)` ); cy.log( - `Beyond max date: ${beyondMaxDate.format("YYYY-MM-DD")} (Day 76 - should be disabled)` + `Beyond max date: ${beyondMaxDate.format("YYYY-MM-DD")} (Day 75 - should be disabled)` ); // Select the start date to establish context for bold date calculation @@ -477,21 +480,24 @@ describe("Booking Modal Date Picker Tests", () => { clearZoneStart.toDate() ); - // Verify max date is selectable - cy.get("@dateTestFlatpickr") - .getFlatpickrDate(calculatedMaxDate.toDate()) - .should("not.have.class", "flatpickr-disabled") - .and("be.visible"); - - // Verify beyond max date is disabled (if in visible month range) - if ( - beyondMaxDate.month() === clearZoneStart.month() || - beyondMaxDate.month() === clearZoneStart.add(1, "month").month() - ) { - cy.get("@dateTestFlatpickr") - .getFlatpickrDate(beyondMaxDate.toDate()) - .should("have.class", "flatpickr-disabled"); - } + // Verify max date enforcement via the flatpickr disable function. + // We query the disable function directly rather than navigating the + // calendar DOM, because the booking modal's reactive system + // (onMonthChange → visibleRangeRef → syncInstanceDatesFromStore → + // fp.jumpToDate) races with test navigation and pulls the calendar + // back to the start date's month. + cy.get("@dateTestFlatpickr").should($el => { + const fp = $el[0]._flatpickr; + const disableFn = fp.config.disable[0]; + expect(disableFn(calculatedMaxDate.toDate())).to.eq( + false, + `Max date ${calculatedMaxDate.format("YYYY-MM-DD")} should be selectable` + ); + expect(disableFn(beyondMaxDate.toDate())).to.eq( + true, + `Beyond max date ${beyondMaxDate.format("YYYY-MM-DD")} should be disabled` + ); + }); cy.log("✓ Maximum date calculation enforced correctly"); @@ -527,22 +533,40 @@ describe("Booking Modal Date Picker Tests", () => { `Expected bold dates: ${expectedBoldDates.map(d => d.format("YYYY-MM-DD")).join(", ")}` ); - // Test each expected bold date has the "booking-loan-boundary" class - expectedBoldDates.forEach(boldDate => { - if ( - boldDate.month() === clearZoneStart.month() || - boldDate.month() === clearZoneStart.add(1, "month").month() - ) { + // Verify bold dates are registered in the instance's loan boundary + // cache. We check the cache directly to avoid cross-month navigation + // races (the booking modal's onMonthChange handler asynchronously + // pulls the calendar back to the start date's month). + cy.get("@dateTestFlatpickr").should($el => { + const fp = $el[0]._flatpickr; + const boundaryTimes = fp._loanBoundaryTimes; + // Cross-frame: instanceof Set fails across iframe boundaries, + // so check for Set-like interface instead + expect(boundaryTimes).to.exist; + expect(typeof boundaryTimes.has).to.eq("function"); + + expectedBoldDates.forEach(boldDate => { + const ts = boldDate.toDate().getTime(); + expect( + boundaryTimes.has(ts), + `Expected ${boldDate.format("YYYY-MM-DD")} to be a loan boundary` + ).to.be.true; + }); + }); + + // For dates in the currently visible month, also verify the DOM class + expectedBoldDates + .filter(d => d.month() === clearZoneStart.month()) + .forEach(boldDate => { cy.get("@dateTestFlatpickr") .getFlatpickrDate(boldDate.toDate()) .should("have.class", "booking-loan-boundary"); cy.log( `✓ Day ${boldDate.format("YYYY-MM-DD")}: Has 'booking-loan-boundary' class (bold)` ); - } - }); + }); - // Verify that only expected dates are bold (have "booking-loan-boundary" class) + // Verify that only expected dates are bold in the current view cy.get(".flatpickr-day.booking-loan-boundary").each($el => { const ariaLabel = $el.attr("aria-label"); const date = dayjs(ariaLabel, "MMMM D, YYYY"); @@ -745,11 +769,11 @@ describe("Booking Modal Date Picker Tests", () => { // ======================================================================== cy.log("=== PHASE 3: Lead period conflicts ==="); - // Hover June 11 - Lead period (June 9-10), June 9 is past - // Vue version only disables when lead period has BOOKING conflicts, - // not when lead dates are in the past. No bookings on June 9-10. - hoverDateByISO("2026-06-11"); - getDateByISO("2026-06-11").should( + // Hover June 14 - Lead period (June 12-13), both after today, no booking conflict + // Note: June 11 would be disabled by minimum advance booking (today+leadDays=June 12), + // so we test with June 14 which is past the minimum advance period. + hoverDateByISO("2026-06-14"); + getDateByISO("2026-06-14").should( "not.have.class", "flatpickr-disabled" ); @@ -1523,55 +1547,33 @@ describe("Booking Modal Date Picker Tests", () => { .should("have.class", "flatpickr-disabled"); // ================================================================ - // SCENARIO 3: Visual feedback - booking marker dots for trail period + // SCENARIO 3: Visual feedback - trail period hover classes // ================================================================ cy.log( - "=== Scenario 3: Visual feedback - Trail period marker dots ===" + "=== Scenario 3: Visual feedback - Trail period hover classes ===" ); - // Days 13-14 should have trail marker dots + // Hover a date whose trail overlaps with bookings + // Day 13 trail should get hover-trail class on hover cy.get("@flatpickrInput2") - .getFlatpickrDate(today.add(13, "day").toDate()) - .within(() => { - cy.get( - ".booking-marker-grid .booking-marker-dot" - ).should("exist"); - }); - + .hoverFlatpickrDate(today.add(13, "day").toDate()); cy.get("@flatpickrInput2") - .getFlatpickrDate(today.add(14, "day").toDate()) - .within(() => { - cy.get( - ".booking-marker-grid .booking-marker-dot" - ).should("exist"); - }); + .getFlatpickrDate(today.add(13, "day").toDate()) + .should("have.class", "booking-day--hover-trail"); // ================================================================ - // SCENARIO 4: Visual feedback - booking marker dots for lead period + // SCENARIO 4: Visual feedback - lead period hover classes // ================================================================ cy.log( - "=== Scenario 4: Visual feedback - Lead period marker dots ===" + "=== Scenario 4: Visual feedback - Lead period hover classes ===" ); + // Hover a date whose lead period overlaps with bookings cy.get("@flatpickrInput2") - .hoverFlatpickrDate(today.add(5, "day").toDate()); - - // Days 8-9 should have lead marker dots + .hoverFlatpickrDate(today.add(8, "day").toDate()); cy.get("@flatpickrInput2") .getFlatpickrDate(today.add(8, "day").toDate()) - .within(() => { - cy.get( - ".booking-marker-grid .booking-marker-dot" - ).should("exist"); - }); - - cy.get("@flatpickrInput2") - .getFlatpickrDate(today.add(9, "day").toDate()) - .within(() => { - cy.get( - ".booking-marker-grid .booking-marker-dot" - ).should("exist"); - }); + .should("have.class", "booking-day--hover-lead"); }); }); diff --git a/t/cypress/support/e2e.js b/t/cypress/support/e2e.js index 7265d99c709..6851de190f5 100644 --- a/t/cypress/support/e2e.js +++ b/t/cypress/support/e2e.js @@ -55,19 +55,6 @@ Cypress.on("window:before:load", win => { }; }); -// Handle common application errors gracefully in booking modal tests -// This prevents test failures from known JS errors that don't affect functionality -Cypress.on("uncaught:exception", (err, runnable) => { - // Return false to prevent the error from failing the test - // These errors can occur when the booking modal JS has timing issues - if ( - err.message.includes("Cannot read properties of undefined") || - err.message.includes("Cannot convert undefined or null to object") - ) { - return false; - } - return true; -}); function get_fallback_login_value(param) { var env_var = param == "username" ? "KOHA_USER" : "KOHA_PASS"; diff --git a/t/cypress/support/flatpickr.js b/t/cypress/support/flatpickr.js index cb21ab81da2..54654e04c66 100644 --- a/t/cypress/support/flatpickr.js +++ b/t/cypress/support/flatpickr.js @@ -322,7 +322,7 @@ Cypress.Commands.add( ); } - // Select start date - use native click to avoid DOM detachment from Vue re-renders + // Click start date return ensureDateIsVisible( startDayjsDate, $input, @@ -332,21 +332,24 @@ Cypress.Commands.add( .should("be.visible") .then($el => $el[0].click()); - // Wait for complex date recalculations (e.g., booking availability) to complete - cy.get( - _getFlatpickrDateSelector(startDayjsDate) - ).should( - $el => { - expect($el).to.have.class("selected"); - expect($el).to.have.class("startRange"); - }, - { timeout: 5000 } - ); + // Validate start date registered via instance state + cy.wrap($input).should($el => { + const fp = $el[0]._flatpickr; + expect(fp.selectedDates).to.have.length(1); + expect( + dayjs(fp.selectedDates[0]).format("YYYY-MM-DD") + ).to.eq(startDayjsDate.format("YYYY-MM-DD")); + }); - // Ensure calendar stays open - cy.get(".flatpickr-calendar.open").should("be.visible"); + // Wait for async side effects to settle — the booking + // modal's onChange handler schedules a delayed calendar + // navigation (navigateCalendarIfNeeded, 100ms) that can + // jump the view to a different month. + // eslint-disable-next-line cypress/no-unnecessary-waiting + cy.wait(150); - // Navigate to end date and select it + // Navigate to end date (may need to switch months if + // the calendar navigated away) and click it return ensureDateIsVisible( endDayjsDate, $input, @@ -356,12 +359,7 @@ Cypress.Commands.add( .should("be.visible") .then($el => $el[0].click()); - cy.get(".flatpickr-calendar.open").should( - "not.exist", - { timeout: 5000 } - ); - - // Validate via flatpickr instance (format-agnostic) + // Validate range completed via instance state cy.wrap($input).should($el => { const fp = $el[0]._flatpickr; expect(fp.selectedDates.length).to.eq(2); @@ -369,6 +367,13 @@ Cypress.Commands.add( expect(dayjs(fp.selectedDates[1]).format("YYYY-MM-DD")).to.eq(endDayjsDate.format("YYYY-MM-DD")); }); + // Close calendar if it stayed open + cy.get("body").then($body => { + if ($body.find(".flatpickr-calendar.open").length > 0) { + $input[0]._flatpickr.close(); + } + }); + return cy.wrap($input); }); }); -- 2.53.0