[Bug 41514] New: The API should fully respect lead and trail periods for bookings
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 Bug ID: 41514 Summary: The API should fully respect lead and trail periods for bookings Initiative type: --- Sponsorship --- status: Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement 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 Depends on: 37707 Currently, the lead/trail period handling for bookings is entirely handled in the UI. We should add handling to the API to ensure these settings are adhered to by third-party consumers too. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37707 [Bug 37707] Lead/Trail times should work in combination -- 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=41514 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Circulation| |Bookings function| | -- 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=41514 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=41514 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=41514 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |37477 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37477 [Bug 37477] Add option to override warm-up/cool-down periods on bookings at booking time -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- 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=41514 --- Comment #1 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194462 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194462&action=edit Bug 41514: Enforce lead/trail periods server-side in booking API Previously, lead and trail period validation for bookings was only enforced in the UI (place_booking.js), allowing third-party API consumers to create bookings that violated the configured buffer zones. This patch adds server-side enforcement in the two check_booking() methods and the Booking->store() call: - Koha::Item::check_booking(): accepts an optional branchcode param, fetches bookings_lead_period and bookings_trail_period circ rules for the item's effective itemtype, expands the overlap query so that effective periods (core ± lead/trail) are checked rather than just core date ranges. Also expands the checkout-due-date check so that items must be returned at least lead_days before the booking start. - Koha::Biblio::check_booking(): same expansion using the global (itemtype-agnostic) rule as an approximation for biblios that have items of mixed types. The item-level check provides the definitive per-item validation. - Koha::Booking::store(): passes pickup_library_id as branchcode to both check_booking() calls so that branch-specific rules are used. Tests added to t/db_dependent/Koha/Booking.t covering: - booking ending in existing booking's lead period → clash - booking ending before effective period → OK - booking starting in existing booking's trail period → clash - new booking's lead period overlapping existing trail period → clash - booking starting after full effective period ends → OK - checkout due within lead period → clash - checkout due before lead period → OK -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194462|0 |1 is obsolete| | --- Comment #2 from David Nind <david@davidnind.com> --- Created attachment 194476 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194476&action=edit Bug 41514: Enforce lead/trail periods server-side in booking API Previously, lead and trail period validation for bookings was only enforced in the UI (place_booking.js), allowing third-party API consumers to create bookings that violated the configured buffer zones. This patch adds server-side enforcement in the two check_booking() methods and the Booking->store() call: - Koha::Item::check_booking(): accepts an optional branchcode param, fetches bookings_lead_period and bookings_trail_period circ rules for the item's effective itemtype, expands the overlap query so that effective periods (core ± lead/trail) are checked rather than just core date ranges. Also expands the checkout-due-date check so that items must be returned at least lead_days before the booking start. - Koha::Biblio::check_booking(): same expansion using the global (itemtype-agnostic) rule as an approximation for biblios that have items of mixed types. The item-level check provides the definitive per-item validation. - Koha::Booking::store(): passes pickup_library_id as branchcode to both check_booking() calls so that branch-specific rules are used. Tests added to t/db_dependent/Koha/Booking.t covering: - booking ending in existing booking's lead period → clash - booking ending before effective period → OK - booking starting in existing booking's trail period → clash - new booking's lead period overlapping existing trail period → clash - booking starting after full effective period ends → OK - checkout due within lead period → clash - checkout due before lead period → OK Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #3 from David Nind <david@davidnind.com> --- Testing notes (using KTD): 1. Tests pass after the patch: t/db_dependent/Koha/Booking.t 2. I didn't use an API client (such as Bruno) to test - happy to if someone can provide some examples. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |me@paulderscheid.xyz |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194476|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=41514 --- Comment #4 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 204926 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204926&action=edit Bug 41514: Enforce lead/trail periods server-side in booking API Previously, lead and trail period validation for bookings was only enforced in the UI (place_booking.js), allowing third-party API consumers to create bookings that violated the configured buffer zones. This patch adds server-side enforcement in the two check_booking() methods and the Booking->store() call: - Koha::Item::check_booking(): accepts an optional branchcode param, fetches bookings_lead_period and bookings_trail_period circ rules for the item's effective itemtype, expands the overlap query so that effective periods (core ± lead/trail) are checked rather than just core date ranges. Also expands the checkout-due-date check so that items must be returned at least lead_days before the booking start. - Koha::Biblio::check_booking(): same expansion using the global (itemtype-agnostic) rule as an approximation for biblios that have items of mixed types. The item-level check provides the definitive per-item validation. - Koha::Booking::store(): passes pickup_library_id as branchcode to both check_booking() calls so that branch-specific rules are used. Tests added to t/db_dependent/Koha/Booking.t covering: - booking ending in existing booking's lead period → clash - booking ending before effective period → OK - booking starting in existing booking's trail period → clash - new booking's lead period overlapping existing trail period → clash - booking starting after full effective period ends → OK - checkout due within lead period → clash - checkout due before lead period → OK Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 --- Comment #5 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 204927 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204927&action=edit Bug 41514: (QA follow-up) Enforce lead/trail periods in item auto-assignment The "any item" path in Koha::Booking::store() excluded only items whose bookings overlapped the core date range, so a biblio-level booking could be stored on an item whose trail period it violates, while the same booking with an explicit item_id was rejected. _select_optimal_item() made this worse by preferring the just-vacated item. - Fetch bookings_trail_period alongside bookings_lead_period in _assign_item_for_booking() - Expand the existing bookings query by lead + trail on both sides, matching the check_booking() overlap condition This makes three copies of the rule lookup and expanded overlap query; the next follow-up consolidates them. Test plan: 1. Set bookings_lead_period=3 and bookings_trail_period=2 for a branch and itemtype 2. Create a biblio with two bookable items X and Y of that itemtype; book X for today+5..today+10 and Y for today+40..today+45 3. Via the REST API, POST a booking for that biblio without item_id for today+12..today+14 => Without this patch it is stored and assigned to X, inside X's trail period => With this patch it is assigned to Y; a second identical request is rejected with a clash 4. prove t/db_dependent/Koha/Booking.t Assisted-by: Fable 5.1 (Anthropic) Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 --- Comment #6 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 204928 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204928&action=edit Bug 41514: (QA follow-up) Add Koha::Bookings->filter_by_overlapping After the previous follow-up the lead/trail rule lookup and the expanded overlap query existed three times: in Koha::Item::check_booking, Koha::Biblio::check_booking and Koha::Booking::_assign_item_for_booking. - Add Koha::Bookings->filter_by_overlapping, taking the period and the lead/trail days; callers chain it with filter_by_active - Fetch both rules with a single get_effective_rules call at each site - Use the new method at the three call sites Test plan: 1. prove t/db_dependent/Koha/Bookings.t t/db_dependent/Koha/Booking.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Biblio.t t/db_dependent/api/v1/bookings.t Assisted-by: Fable 5.1 (Anthropic) Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 Bug 41514 depends on bug 37707, which changed state. Bug 37707 Summary: Lead/Trail times should work in combination https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37707 What |Removed |Added ---------------------------------------------------------------------------- Status|Needs documenting |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #204926|0 |1 is obsolete| | Attachment #204927|0 |1 is obsolete| | Attachment #204928|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=41514 --- Comment #7 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204952 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204952&action=edit Bug 41514: Enforce lead/trail periods server-side in booking API Previously, lead and trail period validation for bookings was only enforced in the UI (place_booking.js), allowing third-party API consumers to create bookings that violated the configured buffer zones. This patch adds server-side enforcement in the two check_booking() methods and the Booking->store() call: - Koha::Item::check_booking(): accepts an optional branchcode param, fetches bookings_lead_period and bookings_trail_period circ rules for the item's effective itemtype, expands the overlap query so that effective periods (core ± lead/trail) are checked rather than just core date ranges. Also expands the checkout-due-date check so that items must be returned at least lead_days before the booking start. - Koha::Biblio::check_booking(): same expansion using the global (itemtype-agnostic) rule as an approximation for biblios that have items of mixed types. The item-level check provides the definitive per-item validation. - Koha::Booking::store(): passes pickup_library_id as branchcode to both check_booking() calls so that branch-specific rules are used. Tests added to t/db_dependent/Koha/Booking.t covering: - booking ending in existing booking's lead period → clash - booking ending before effective period → OK - booking starting in existing booking's trail period → clash - new booking's lead period overlapping existing trail period → clash - booking starting after full effective period ends → OK - checkout due within lead period → clash - checkout due before lead period → OK Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 --- Comment #8 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204953 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204953&action=edit Bug 41514: (QA follow-up) Enforce lead/trail periods in item auto-assignment The "any item" path in Koha::Booking::store() excluded only items whose bookings overlapped the core date range, so a biblio-level booking could be stored on an item whose trail period it violates, while the same booking with an explicit item_id was rejected. _select_optimal_item() made this worse by preferring the just-vacated item. - Fetch bookings_trail_period alongside bookings_lead_period in _assign_item_for_booking() - Expand the existing bookings query by lead + trail on both sides, matching the check_booking() overlap condition This makes three copies of the rule lookup and expanded overlap query; the next follow-up consolidates them. Test plan: 1. Set bookings_lead_period=3 and bookings_trail_period=2 for a branch and itemtype 2. Create a biblio with two bookable items X and Y of that itemtype; book X for today+5..today+10 and Y for today+40..today+45 3. Via the REST API, POST a booking for that biblio without item_id for today+12..today+14 => Without this patch it is stored and assigned to X, inside X's trail period => With this patch it is assigned to Y; a second identical request is rejected with a clash 4. prove t/db_dependent/Koha/Booking.t Assisted-by: Fable 5.1 (Anthropic) Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Martin Renvoize <martin.renvoize@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=41514 --- Comment #9 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204954 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204954&action=edit Bug 41514: (QA follow-up) Add Koha::Bookings->filter_by_overlapping After the previous follow-up the lead/trail rule lookup and the expanded overlap query existed three times: in Koha::Item::check_booking, Koha::Biblio::check_booking and Koha::Booking::_assign_item_for_booking. - Add Koha::Bookings->filter_by_overlapping, taking the period and the lead/trail days; callers chain it with filter_by_active - Fetch both rules with a single get_effective_rules call at each site - Use the new method at the three call sites Test plan: 1. prove t/db_dependent/Koha/Bookings.t t/db_dependent/Koha/Booking.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Biblio.t t/db_dependent/api/v1/bookings.t Assisted-by: Fable 5.1 (Anthropic) Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Martin Renvoize <martin.renvoize@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=41514 --- Comment #10 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204955 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204955&action=edit Bug 41514: (QA follow-up) Use the itemtype_id filter for auto-assignment's lead/trail rule lookup _assign_item_for_booking() always looked up the bookings_lead_period / bookings_trail_period rule using the biblio's default itemtype (biblioitem.itemtype), even when the caller requested an "any item of itemtype X" booking via set_itemtype_filter() (wired up from the REST API's itemtype_id parameter). On a biblio with mixed item-level itemtypes that have different lead/ trail rules, this meant an item could be auto-assigned to a booking that actually violates its own itemtype's configured buffer period, because the exclusion query used the wrong itemtype's rule while the item search correctly used the filter for candidate selection. Use the itemtype filter for the rule lookup when it is set, falling back to the biblio's default itemtype otherwise. Test plan: 1. prove t/db_dependent/Koha/Booking.t Signed-off-by: Martin Renvoize <martin.renvoize@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=41514 --- Comment #11 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204956 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204956&action=edit Bug 41514: (QA follow-up) Add explicit use Koha::CirculationRules to Koha::Booking _assign_item_for_booking() calls Koha::CirculationRules->get_effective_rules without the module being directly loaded. It currently works only because use Koha::Items transitively loads Koha::Item, which loads Koha::CirculationRules - a fragile, load-order-dependent dependency. Load it explicitly. Signed-off-by: Martin Renvoize <martin.renvoize@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=41514 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Patch doesn't apply CC| |pedro.amorim@openfifth.co.u | |k -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #204952|0 |1 is obsolete| | Attachment #204953|0 |1 is obsolete| | Attachment #204954|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=41514 --- Comment #12 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 208326 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208326&action=edit Bug 41514: Enforce lead/trail periods server-side in booking API Previously, lead and trail period validation for bookings was only enforced in the UI (place_booking.js), allowing third-party API consumers to create bookings that violated the configured buffer zones. This patch adds server-side enforcement in the two check_booking() methods and the Booking->store() call: - Koha::Item::check_booking(): accepts an optional branchcode param, fetches bookings_lead_period and bookings_trail_period circ rules for the item's effective itemtype, expands the overlap query so that effective periods (core ± lead/trail) are checked rather than just core date ranges. Also expands the checkout-due-date check so that items must be returned at least lead_days before the booking start. - Koha::Biblio::check_booking(): same expansion using the global (itemtype-agnostic) rule as an approximation for biblios that have items of mixed types. The item-level check provides the definitive per-item validation. - Koha::Booking::store(): passes pickup_library_id as branchcode to both check_booking() calls so that branch-specific rules are used. Tests added to t/db_dependent/Koha/Booking.t covering: - booking ending in existing booking's lead period → clash - booking ending before effective period → OK - booking starting in existing booking's trail period → clash - new booking's lead period overlapping existing trail period → clash - booking starting after full effective period ends → OK - checkout due within lead period → clash - checkout due before lead period → OK Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 --- Comment #13 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 208327 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208327&action=edit Bug 41514: (QA follow-up) Enforce lead/trail periods in item auto-assignment The "any item" path in Koha::Booking::store() excluded only items whose bookings overlapped the core date range, so a biblio-level booking could be stored on an item whose trail period it violates, while the same booking with an explicit item_id was rejected. _select_optimal_item() made this worse by preferring the just-vacated item. - Fetch bookings_trail_period alongside bookings_lead_period in _assign_item_for_booking() - Expand the existing bookings query by lead + trail on both sides, matching the check_booking() overlap condition This makes three copies of the rule lookup and expanded overlap query; the next follow-up consolidates them. Test plan: 1. Set bookings_lead_period=3 and bookings_trail_period=2 for a branch and itemtype 2. Create a biblio with two bookable items X and Y of that itemtype; book X for today+5..today+10 and Y for today+40..today+45 3. Via the REST API, POST a booking for that biblio without item_id for today+12..today+14 => Without this patch it is stored and assigned to X, inside X's trail period => With this patch it is assigned to Y; a second identical request is rejected with a clash 4. prove t/db_dependent/Koha/Booking.t Assisted-by: Fable 5.1 (Anthropic) Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 --- Comment #14 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 208328 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208328&action=edit Bug 41514: (QA follow-up) Add Koha::Bookings->filter_by_overlapping After the previous follow-up the lead/trail rule lookup and the expanded overlap query existed three times: in Koha::Item::check_booking, Koha::Biblio::check_booking and Koha::Booking::_assign_item_for_booking. - Add Koha::Bookings->filter_by_overlapping, taking the period and the lead/trail days; callers chain it with filter_by_active - Fetch both rules with a single get_effective_rules call at each site - Use the new method at the three call sites Test plan: 1. prove t/db_dependent/Koha/Bookings.t t/db_dependent/Koha/Booking.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Biblio.t t/db_dependent/api/v1/bookings.t Assisted-by: Fable 5.1 (Anthropic) Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 --- Comment #15 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 208329 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208329&action=edit Bug 41514: (QA follow-up) Honour the trail period when extending a booked checkout Koha::Booking->store now enforces lead and trail periods, but Koha::Item->find_booking only checks the lead period at the global level. Renewing a booked checkout, or overriding its due date at checkout, into the next booking's trail window was allowed, and the booking end_date sync then silently failed on the clash. - find_booking with exclude_booking_id also adds the excluded booking's trail period and reads both rules for its pickup library - plain checkout lookups are unchanged Test plan: 1. prove /kohadevbox/koha/t/db_dependent/Circulation.t 2. prove /kohadevbox/koha/t/db_dependent/Koha/Item.t Assisted-by: Opus 5.5 (Anthropic) Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 --- Comment #16 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Sorry, missed the two new QA follow-ups. Fixing it. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #204955|0 |1 is obsolete| | Attachment #204956|0 |1 is obsolete| | Attachment #208326|0 |1 is obsolete| | Attachment #208327|0 |1 is obsolete| | Attachment #208328|0 |1 is obsolete| | Attachment #208329|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=41514 --- Comment #17 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 208333 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208333&action=edit Bug 41514: Enforce lead/trail periods server-side in booking API Previously, lead and trail period validation for bookings was only enforced in the UI (place_booking.js), allowing third-party API consumers to create bookings that violated the configured buffer zones. This patch adds server-side enforcement in the two check_booking() methods and the Booking->store() call: - Koha::Item::check_booking(): accepts an optional branchcode param, fetches bookings_lead_period and bookings_trail_period circ rules for the item's effective itemtype, expands the overlap query so that effective periods (core ± lead/trail) are checked rather than just core date ranges. Also expands the checkout-due-date check so that items must be returned at least lead_days before the booking start. - Koha::Biblio::check_booking(): same expansion using the global (itemtype-agnostic) rule as an approximation for biblios that have items of mixed types. The item-level check provides the definitive per-item validation. - Koha::Booking::store(): passes pickup_library_id as branchcode to both check_booking() calls so that branch-specific rules are used. Tests added to t/db_dependent/Koha/Booking.t covering: - booking ending in existing booking's lead period → clash - booking ending before effective period → OK - booking starting in existing booking's trail period → clash - new booking's lead period overlapping existing trail period → clash - booking starting after full effective period ends → OK - checkout due within lead period → clash - checkout due before lead period → OK Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 --- Comment #18 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 208334 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208334&action=edit Bug 41514: (QA follow-up) Enforce lead/trail periods in item auto-assignment The "any item" path in Koha::Booking::store() excluded only items whose bookings overlapped the core date range, so a biblio-level booking could be stored on an item whose trail period it violates, while the same booking with an explicit item_id was rejected. _select_optimal_item() made this worse by preferring the just-vacated item. - Fetch bookings_trail_period alongside bookings_lead_period in _assign_item_for_booking() - Expand the existing bookings query by lead + trail on both sides, matching the check_booking() overlap condition This makes three copies of the rule lookup and expanded overlap query; the next follow-up consolidates them. Test plan: 1. Set bookings_lead_period=3 and bookings_trail_period=2 for a branch and itemtype 2. Create a biblio with two bookable items X and Y of that itemtype; book X for today+5..today+10 and Y for today+40..today+45 3. Via the REST API, POST a booking for that biblio without item_id for today+12..today+14 => Without this patch it is stored and assigned to X, inside X's trail period => With this patch it is assigned to Y; a second identical request is rejected with a clash 4. prove t/db_dependent/Koha/Booking.t Assisted-by: Fable 5.1 (Anthropic) Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 --- Comment #19 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 208335 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208335&action=edit Bug 41514: (QA follow-up) Add Koha::Bookings->filter_by_overlapping After the previous follow-up the lead/trail rule lookup and the expanded overlap query existed three times: in Koha::Item::check_booking, Koha::Biblio::check_booking and Koha::Booking::_assign_item_for_booking. - Add Koha::Bookings->filter_by_overlapping, taking the period and the lead/trail days; callers chain it with filter_by_active - Fetch both rules with a single get_effective_rules call at each site - Use the new method at the three call sites Test plan: 1. prove t/db_dependent/Koha/Bookings.t t/db_dependent/Koha/Booking.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Biblio.t t/db_dependent/api/v1/bookings.t Assisted-by: Fable 5.1 (Anthropic) Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 --- Comment #20 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 208336 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208336&action=edit Bug 41514: (QA follow-up) Use the itemtype_id filter for auto-assignment's lead/trail rule lookup _assign_item_for_booking() always looked up the bookings_lead_period / bookings_trail_period rule using the biblio's default itemtype (biblioitem.itemtype), even when the caller requested an "any item of itemtype X" booking via set_itemtype_filter() (wired up from the REST API's itemtype_id parameter). On a biblio with mixed item-level itemtypes that have different lead/ trail rules, this meant an item could be auto-assigned to a booking that actually violates its own itemtype's configured buffer period, because the exclusion query used the wrong itemtype's rule while the item search correctly used the filter for candidate selection. Use the itemtype filter for the rule lookup when it is set, falling back to the biblio's default itemtype otherwise. Test plan: 1. prove t/db_dependent/Koha/Booking.t Signed-off-by: Martin Renvoize <martin.renvoize@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=41514 --- Comment #21 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 208337 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208337&action=edit Bug 41514: (QA follow-up) Add explicit use Koha::CirculationRules to Koha::Booking _assign_item_for_booking() calls Koha::CirculationRules->get_effective_rules without the module being directly loaded. It currently works only because use Koha::Items transitively loads Koha::Item, which loads Koha::CirculationRules - a fragile, load-order-dependent dependency. Load it explicitly. Signed-off-by: Martin Renvoize <martin.renvoize@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=41514 --- Comment #22 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 208338 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208338&action=edit Bug 41514: (QA follow-up) Honour the trail period when extending a booked checkout Koha::Booking->store now enforces lead and trail periods, but Koha::Item->find_booking only checks the lead period at the global level. Renewing a booked checkout, or overriding its due date at checkout, into the next booking's trail window was allowed, and the booking end_date sync then silently failed on the clash. - find_booking with exclude_booking_id also adds the excluded booking's trail period and reads both rules for its pickup library - plain checkout lookups are unchanged Test plan: 1. prove /kohadevbox/koha/t/db_dependent/Circulation.t 2. prove /kohadevbox/koha/t/db_dependent/Koha/Item.t Assisted-by: Opus 5.5 (Anthropic) Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 --- Comment #23 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Fixed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Keywords| |release-notes-needed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |26.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41514 --- Comment #24 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Thanks everyone! Pushed to main for 26.11! -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org