There is no circular dependency, so the runtime require can be
replaced with a compile-time use statement.
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Introduce a dedicated Koha::Item::Availability::* namespace for checkers
that return a Koha::Result::Availability. Keeping these under the Item
subject prefix keeps them co-located with the Item they describe, while
the Availability sub-namespace gives future parallel checkers
(Checkout, Hold, Book, ...) an obvious home rather than scattering them
as single-file directories alongside Koha/Item/Transfer/ etc.
The $item->checkin_availability convenience method on Koha::Item is
preserved; only its internal delegation target changes.
Renames:
Koha::Item::Checkin::Availability -> Koha::Item::Availability::Checkin
Koha/Item/Checkin/Availability.pm -> Koha/Item/Availability/Checkin.pm
t/db_dependent/Koha/Item/Checkin/Availability.t
-> t/db_dependent/Koha/Item/Availability/Checkin.t
Test plan:
prove t/db_dependent/Koha/Item/Availability/Checkin.t
prove t/db_dependent/Circulation/Returns.t
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Consolidate result base classes under the Koha::Result::* namespace
alongside the existing Koha::Result::Boolean. This keeps return-value
wrappers discoverable in one place and avoids a lone Koha::Availability::
namespace that would otherwise host a single base class.
Renames:
Koha::Availability::Result -> Koha::Result::Availability
Koha/Availability/Result.pm -> Koha/Result/Availability.pm
t/Koha/Availability/Result.t -> t/Koha/Result/Availability.t
Call sites in Koha::Item, Koha::Item::Checkin::Availability, and the
test files are updated accordingly.
Test plan:
prove t/Koha/Result/Availability.t
prove t/db_dependent/Koha/Item/Checkin/Availability.t
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This follow-up replaces the unconditional removal of short-circuit logic
with a caller-controlled no_short_circuit parameter, following the same
pattern established by Koha::Patron->can_place_holds.
By default, check() short-circuits on the first blocker. This is the
correct behavior for AddReturn, which processes blockers sequentially
and only acts on the first one it finds (BlockedWithdrawn => early return,
Wrongbranch => early return, BlockedLost => sets doreturn=0). Running all
policy checks only to discard the results is wasteful, specially when
some of them (e.g. can_be_returned_at) hit the database.
When no_short_circuit => 1 is passed, all checks run and all blockers
are collected. This is the behavior API consumers will need: present the
full set of issues in a single response instead of requiring
fix-one-recheck cycles.
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/Item/Checkin/Availability.t
=> SUCCESS: Tests pass!
k$ prove t/db_dependent/Circul*
=> SUCCESS: Tests pass!
3. Sign off :-D
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Remove early returns from Koha::Item::Checkin::Availability::check() so
that all blockers, confirmations, and warnings are collected in a single
pass. Previously, the first blocker triggered an early return, meaning
callers could only discover one problem at a time.
This is particularly useful for API consumers who want to present the
full set of issues to the user in a single response rather than
requiring fix-one-recheck cycles.
The change is safe for AddReturn because it already handles blocker
priority through its sequential if/return structure:
1. BlockedWithdrawn → early return before status updates
2. Wrongbranch → early return after status updates
3. BlockedLost → sets doreturn=0
Even when multiple blockers are present, AddReturn processes them in
the correct priority order.
Also makes existing tests deterministic by explicitly setting withdrawn
and itemlost values on test items, preventing random TestBuilder values
from triggering unrelated blockers now that all checks always run.
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Restore the withdrawn warning in Koha::Item::Checkin::Availability that
was incorrectly removed. This warning is set when an item is withdrawn
but BlockReturnOfWithdrawnItems is off, making it useful for API
consumers in subsequent work.
Update AddReturn to read the withdrawn state from the availability
result's warnings rather than re-checking $item->withdrawn directly,
avoiding duplication of logic.
Add test for the withdrawn warning (non-blocked) path.
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Move the AllowReturnToBranch and branch transfer limit checks from both
C4::Circulation::CanBookBeReturned and
Koha::Item::Checkin::Availability::_check_return_policy into a single
authoritative method on Koha::Item: can_be_returned_at().
This eliminates the duplicate implementations and follows the Koha
convention of placing item-related logic on the Koha::Item object.
Changes:
- Add Koha::Item->can_be_returned_at() with AllowReturnToBranch policy
and transfer limit checks
- Make CanBookBeReturned a thin wrapper delegating to the new method
- Update Checkin::Availability to call $item->can_be_returned_at()
- Remove _check_return_policy and unused withdrawn warning from
the Availability class
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Reorder blocker checks to match the original AddReturn priority:
Withdrawn > Wrongbranch > Lost. Previously Lost was checked before
Wrongbranch, which changed behavior for items that were both lost
and at the wrong branch.
QA note: There are other checks in-between these, but I don't think
their order betwixt these blockers matters for retaining existing
behavior.
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The availability check is returning early on blockers (BlockedWithdrawn,
BlockedLost, Wrongbranch) before checking if the item had an active
checkout. This causes AddReturn to lose checkout/patron context for
checked-out items that hit a blocker, resulting in incorrect NotIssued
messages, unintended onloan clearing, and incorrect local use recording.
Move the checkout check to run first so context is always populated
regardless of blockers.
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
- Rename params: branch => library, add to_library for destination
- Move $returnbranch computation before the availability check in
AddReturn so it can be passed as to_library
- Add transfer limit check (bug 7376) to the Availability class
- Rename internal _can_book_be_returned to _check_return_policy
- Remove duplicate $returnbranch block and standalone transfer
limit check from AddReturn
- Update tests for new param names
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch introduces a standardized result class for availability checks,
providing a clean API with proper methods instead of raw hashrefs.
The Koha::Availability::Result class provides:
- add_blocker() - Add conditions that prevent the action
- add_confirmation() - Add conditions requiring user confirmation
- add_warning() - Add informational messages
- set_context() - Store related objects (item, checkout, patron, etc.)
- available() - Check if action can proceed (no blockers)
- needs_confirmation() - Check if confirmations are required
- Accessor methods for all categories
- to_hashref() for backward compatibility
This design:
- Provides a consistent interface across all availability checks
- Makes the API more discoverable and self-documenting
- Enables future enhancements without breaking changes
- Follows OO principles with proper encapsulation
- Can be used as base for checkout, hold, renewal availability
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/Koha/Availability/Result.t
=> SUCCESS: Tests pass!
3. Verify all methods work correctly
4. Sign off :-D
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Cancelling a transfer is a state change on the resource, not a deletion of
it, so it should be modelled as an action sub-resource rather than a DELETE
on the transfer itself. This aligns the endpoint with the API coding
guidelines (REST2: non-CRUD actions use action sub-resources; REST3.2:
conflicts return 409) and with the design used in bug 43454.
Changes:
- Replace DELETE /transfers/{transfer_id} with
POST /transfers/{transfer_id}/cancellation
- Rename the controller method delete() to cancel(), look the transfer up
with objects->find_rs, and return 200 with the updated resource
representation instead of 204 with an empty body
- Make cancellation_reason optional, defaulting to 'Manual', matching the
legacy returns.pl behaviour
- Return 409 with a structured error_code ('already_arrived',
'already_cancelled') when the transfer cannot be cancelled, instead of a
bare 400
- Update the transfers to receive page to POST to the new endpoint and to
treat 404/409 as "already gone, refresh the table"
- Adapt the tests accordingly, including coverage for the default reason
- The *_date API attribute names introduced by this bug are kept, as they
are the guideline-compliant form (REST1.3.4.1: *_date, not date_*)
Test plan:
1. Apply patch
2. Run the tests:
$ ktd --shell
k$ prove t/db_dependent/api/v1/transfers.t
=> SUCCESS: Tests pass!
3. Rebuild and validate the API spec:
k$ yarn api:bundle
k$ prove xt/api.t
=> SUCCESS: Tests pass!
4. In the staff interface, go to Circulation -> Transfers to receive,
click "Cancel transfer" on a row
=> SUCCESS: The transfer is cancelled and the row drops off the table
5. Cancel the same transfer again from a second tab
=> SUCCESS: The API returns 409 (already_cancelled) and the row is
treated as already gone
6. Sign off :-D
Assisted-by: Sonnet 4.6 (Anthropic)
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The REST framework resolves _order_by parameters and embed prefetches by
walking prefetch_whitelist, which is built from DBIC relationship names.
The auto-generated relationships use the FK column names (itemnumber,
frombranch, tobranch), but the API exposes them as item, from_library,
and to_library. Sorting by item.biblio.title etc. threw a 500 because
those names were not in the whitelist.
Adding belongs_to aliases below the DO NOT MODIFY line bridges the API
names to the DBIC names, matching the convention used by other Koha
schema result classes (e.g. Koha::Schema::Result::Item).
Also updates the relation methods in Koha::Item::Transfer to use the new
aliases consistently, so the accessor names line up across both layers.
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The delete endpoint was passing force => 1 to cancel() with no state
guards, allowing already-received and already-cancelled transfers to
be re-cancelled, corrupting datearrived/datecancelled audit fields.
Add explicit 400 responses for both cases and tests to cover them.
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch adds a DELETE /transfers/{transfer_id} route that cancels the
given transfer by id and reverts an in transit recall for the item the same
way as returns.pl does. The cancellation reason is passed in the request body
so this api endpoint could be used in other places we can cancel transfers.
Signed-off-by: Alex Carver [Acerock7] <alex@rcls.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Alex Carver [Acerock7] <alex@rcls.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Koha::Biblio->check_booking counts current checkouts on bookable items
against the pool of available items. Like Item::check_booking before
this patchset, it made no exception for the checkout that belongs to
the very booking being validated, so updating an issued booking's
period could clash against its own loan whenever the new period no
longer overlaps the stored one (the item_id exemption for booked items
only catches bookings that overlap the window under test).
This is masked in the end_date syncs earlier in this patchset because
the synced window always contains the stored start_date; it surfaces
as soon as a sync moves the whole period, and would equally bite the
extension pathway on bug 42790.
- skip checkouts whose booking_id matches the booking being validated
when counting occupied bookable items, mirroring the
Item::check_booking exemption
- cover the blocking and non-blocking checkout cases in Biblio.t
Test plan:
1. prove t/db_dependent/Koha/Biblio.t
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
When checking out a booked item, staff may override the suggested due
date. If that override runs into the next booking for the item, the
previous patch would silently keep the booking's original end date -
staff had no way of knowing their override crossed another booking.
This patch adds an exclude_booking_id option to Koha::Item->find_booking
so a lookup can skip a specific booking (here: the patron's own booking
being collected), and uses it in CanBookBeIssued to detect when an
explicitly requested due date would run into the next booking for the
item. When it would, a new BOOKED_DUE_DATE_CLASH confirmation is raised
and displayed on the checkout screen, telling staff the linked booking
will keep its original end date if they continue.
Only explicitly requested due dates trigger the confirmation -
calculated due dates are clamped to the booking end date later in the
staff checkout flow, so checking them would produce false positives.
SIP is unaffected as it never passes a requested due date.
Test plan:
1) Run t/db_dependent/Circulation.t and t/db_dependent/Koha/Item.t
2) Create two consecutive bookings on the same item for two patrons
3) Check the item out to the first patron specifying a due date beyond
the second booking's start date; confirm the new confirmation
message appears and checkout only proceeds once confirmed
Sponsored-by: OpenFifth <https://openfifth.co.uk/\>
Sponsored-by: Westminster City Council <https://www.westminster.gov.uk/\>
Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Koha::Item->check_booking refuses any booking period that starts before
the item's current checkout is due back. That is correct when assessing
a new booking against someone else's loan, but it also fired when the
checkout in question is the booking's own linked loan - making it
impossible to ever move an issued booking's end_date, as any update
would 'clash' with its own checkout.
This patch skips the checkout guard when the checkout's booking_id
matches the booking_id passed for exclusion, i.e. when the checkout
belongs to the very booking being updated. This is a prerequisite for
keeping booking end dates in sync with checkout due dates in this
patchset, and for the booking extension pathway on bug 42790.
Test plan:
1) Run t/db_dependent/Koha/Item.t and confirm the check_booking
subtest passes, including the two new checkout-related tests
Sponsored-by: OpenFifth <https://openfifth.co.uk/\>
Sponsored-by: Westminster City Council <https://www.westminster.gov.uk/\>
Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The plugin cache used a single slot, so the first backend looked up
poisoned it for all subsequent calls. This caused 500 errors when
viewing ILL requests that had been migrated between backends.
Test plan:
1) Download and install 2 additional plugin backends that implement
their own templates:
https://github.com/openfifth/koha-ill-libkey-lending-tool/releases/download/v2.6.1/koha-ill-libkey-lending-tool-v2.6.1.kpzhttps://github.com/openfifth/koha-ill-reprintsdesk/releases/download/v4.2.0/koha-ill-reprintsdesk-v4.2.0.kpz
2) Enable ILLModule and run the following SQL:
INSERT INTO illrequests (borrowernumber, branchcode, status, backend, placed)
SELECT b.borrowernumber, br.branchcode, 'COMP', 'ReprintsDesk', NOW()
FROM borrowers b, branches br
LIMIT 1;
INSERT INTO action_logs (timestamp, user, module, action, object, info, interface)
VALUES
(NOW() - INTERVAL 2 HOUR, 1, 'ILL', 'INCDOCS_REQUEST_PLACED', 1,
'{"log_origin":"IncDocs","response":1}', 'intranet'),
(NOW() - INTERVAL 1 HOUR, 1, 'ILL', 'REPRINTS_DESK_REQUEST_ORDER_UPDATED', 1,
'{"log_origin":"ReprintsDesk","response":null}', 'commandline');
3) Attempt to open the 'Manage request' page for this newly created
request:
http://localhost:8081/cgi-bin/koha/ill/ill-requests.pl?op=illview&illrequest_id=1
4) Notice you get a 500 error:
Template process failed: file error - : not found
5) Apply all patches, restart plack, refresh. Notice the 'Manage
request' page now renders correctly.
6) Click the 'ILL request log' toolbar button. Confirm that both the
IncDocs and ReprintsDesk entries are shown.
Signed-off-by: Hannah Prince <helm.consortium.nhs.net>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Koha::Edifact::Transport reported every transport failure (no transport
configured, connect failure, directory-change failure, file-listing
failure and per-file download/upload failure) with carp. Under
edi_cron.pl these go to STDERR, so on a normal cron run the messages are
lost and EDI simply goes quiet with nothing in editrace.log to explain
why.
Route these failures through Koha::Logger on the 'edi' interface (the
same interface edi_cron.pl already uses) so they are recorded in the EDI
log. The two ingest()-level data warnings (duplicate filename, unreadable
file) are intentionally left as carp: one is asserted by the existing
unit test and neither is a transport failure.
No change to the transfer logic.
Test plan:
1) prove t/db_dependent/Koha/Edifact/Transport.t
2) Point an EDI account at an unreachable transport and run edi_cron.pl;
confirm the failure is now written to the edi log rather than only to
STDERR.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch changes the default MaxAge from 8 hours to
168 hours or 7 days due to usability issues encountered from
using a short-lived token.
See OWASP for more information on why timestamps with expiry
are not necessary for CSRF tokens. The MaxAge is a requirement
of the WWW::CSRF token used by Koha, so let's just use a longer
expiry in lieu of no expiry.
Test plan:
0. apply patch
1. prove t/Token.t
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
_parse_dbic_query maps field names but did not convert values. When
datetime or boolean filters are nested inside DBIC operators (-and,
-or) in the q= parameter, attributes_from_api (which normally handles
the conversion via _recursive_fixup) cannot reach them because it only
iterates top-level keys.
This patch fixes the issue in two places:
1. _parse_dbic_query (Query.pm): When a key maps to a known column
(on the primary table or a related object), call _recursive_fixup
to convert RFC3339 timestamps to MySQL format and booleans to 0/1.
Related object instances are cached on the result_set to avoid
repeated prefetch_whitelist lookups during recursive parsing.
2. Objects.pm: Move attributes_from_api to process named query params
before merging with q= params. This avoids double-processing since
q= values are already fixed up by _parse_dbic_query.
Test plan:
1. Apply the regression tests from the previous commit
2. Run:
$ prove t/Koha/REST/Plugin/Query.t
$ prove t/db_dependent/Koha/REST/Plugin/Objects.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat step 2
=> SUCCESS: Tests pass!
5. Sign off :-D
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The servicing_instruction column was not mapped in
Koha::Acquisition::Order->to_api_mapping, so it was exposed verbatim in
the REST API order representation. Since api/v1/swagger/definitions/order.yaml
does not declare this property and sets additionalProperties: false, any
request touching the acquisitions/orders endpoint (e.g. the "Receive
shipments" page) failed with a 500 error: "Properties not allowed:
servicing_instruction".
Exclude it from the API representation, consistent with the other
EDIFACT-only columns (line_item_id, suppliers_reference_number, etc.)
that are already excluded the same way.
Test plan:
1) Go to a vendor page, create a basket, add an order line, and close
the basket.
2) From the vendor page, click "Receive shipments".
3) Without this patch, the orders table fails to load with a 500 error
mentioning "Properties not allowed: servicing_instruction".
4) With this patch applied, the orders table loads correctly.
5) prove t/db_dependent/api/v1/acquisitions_orders.t
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This commit adds comprehensive tests for servicing instruction
processing in both incoming quotes and outgoing orders:
1. Integration tests for incoming quote processing (Koha/EDI.pm)
- Tests LVC (coded) and LVT (freetext) extraction
- Tests multiple GIR occurrences with different servicing instructions
- Tests all combinations: LVC only, LVT only, and LVC+LVT together
2. Test EDIFACT quote file (QUOTES_SERVICING.CEQ)
- Contains 3 line items with 4 total orders
- Demonstrates different servicing instruction patterns
3. Fix for multi-occurrence servicing instruction extraction
- Modified quote_item() to extract servicing instructions
per GIR occurrence instead of across all occurrences
- Ensures each order gets only its own servicing instructions
- Aligns with existing multi-item processing for funds and stock rotation
Test coverage:
- t/db_dependent/Koha/EDI.t: 29 tests for incoming quote processing
- t/db_dependent/Koha/Edifact/Order.t: 18 tests for outgoing order generation
(previously added)
All tests pass successfully.
Sponsored-by: Martin Renvoize <martin.renvoize@gmail.com>
Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch updates the EDIFACT processing to handle multiple servicing
instruction groups in both incoming quotes and outgoing orders.
Changes to Koha/EDI.pm (quote_item):
- Reads both LVC (coded) and LVT (freetext) instructions from incoming quotes
- Validates LVC codes against EDIFACT_SI authorized values
- Stores instructions as JSON array structure
- Groups instructions that work together
Changes to Koha/Edifact/Order.pm (gir_segments):
- Parses JSON servicing instruction data from order
- Generates multiple GIR segments for outgoing orders
- Each instruction becomes a separate GIR element
- Logs JSON parse errors to EDI log using Koha::Logger
- Fixed element counter reset bug when segments exceed 5 elements
Structure allows vendors to specify:
- Single coded instruction: [[{type:"LVC",value:"BB"}]]
- Single text instruction: [[{type:"LVT",value:"Handle with care"}]]
- Combined instructions: [[{type:"LVC",value:"BB"},{type:"LVT",value:"Apply on spine"}]]
- Multiple services: [[{type:"LVC",value:"BB"}],[{type:"LVC",value:"BI"}]]
Test plan:
1. Receive EDIFACT quote with servicing instructions
2. Verify instructions are stored correctly in aqorders
3. Create EDI order from basket
4. Verify GIR segments are generated correctly in outgoing message
5. Check EDI log for any JSON parse errors
Sponsored-by: Martin Renvoize <martin.renvoize@gmail.com>
Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Sponsored-by: Martin Renvoize <martin.renvoize@gmail.com>
Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
In the case when a biblio is deleted, we should not suppress its
orphaned records. Therefore, invert the conditions in the WHERE clause
to not suppress the record if no biblio could be found.
Test plan:
1. Enable the system preferences "OAI-PMH" and "OpacSuppression".
Ensure "OpacSuppressionByIPRange" is blank.
2. Navigate to
/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=oai_dc
3. Select a displayed record, and delete its corresponding biblio.
4. Refresh the OAI page, verify the record has vanished.
5. Apply patch.
6. Refresh the OAI page, verify the record now appears and is marked as
deleted. It may have moved to the top of the list.
Sponsored-by: Open Fifth
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
As we're aliasing the primary tables as `main`, we need to use that name
to refer to their columns.
Test plan:
1. Apply patch.
2. Prove t/db_dependent/OAI/Server.t and verify no tests fail.
Sponsored-by: Open Fifth
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Rather than fetching each biblio before choosing to suppress it, this
patch performs this filtering in the initial SQL query that fetches the
biblionumbers.
Note: for queries including items, this additional filtering is
performed in each of the three subqueries, necessitating three
quasi-joins against biblio. If we were to hoist this filtering to the
outer layer, we would also need to remove the LIMIT from each subquery
so as to allow each of them to be fairly weighted. As the preceding
comment implies keeping the subqueries separate has been beneficial for
performance, I've elected not to hoist the WHERE.
Test plan (specific URLs given for KTD, with thanks to David Cook):
1. Apply patch.
2. Navigate to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=2
3. Set 942$n to "Yes" and save the record.
4. Disable the system preference "OpacSuppression", and ensure
"OpacSuppressionByIPRange" is blank.
5a. Verify you can successfully access
http://localhost:8080/cgi-bin/koha/oai.pl?verb=GetRecord&metadataPrefix=oai_dc&identifier=KOHA-OAI-TEST:2
5b. Verify KOHA-OAI-TEST-2 appears on
http://localhost:8080/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=oai_dc
6. Enable the system preference "OpacSuppression", and ensure
"OpacSuppressionByIPRange" is blank.
7. Repeat steps 5a-5b, verifying that:
- the OAI GetRecord page returns "There is no biblio record with
this identifier".
- the OAI ListRecords page does not show KOHA-OAI-TEST-2.
Sponsored-by: Open Fifth
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
When OpacSuppression is enabled, records marked as suppressed
(biblio.opac_suppressed = 1) still appear in public lists viewed
through the OPAC. This is confusing for patrons.
This patch adds a filter_by_not_suppressed method to
Koha::Virtualshelfcontents and uses it in opac-shelves.pl to
exclude suppressed records from list display when OpacSuppression
is enabled. OpacSuppressionByIPRange is respected: users within
the allowed IP range still see all records.
Test plan:
1. Apply patch
2. Enable OpacSuppression system preference
3. Create a public list and add some titles to it
4. Suppress one of the titles (set 942$n = 1)
5. View the list in the OPAC
=> SUCCESS: Suppressed record is no longer shown in the list
6. Set OpacSuppressionByIPRange to match your IP (e.g. 127.)
7. Repeat step 5
=> SUCCESS: All records (including suppressed) are shown
8. Run:
$ ktd --shell
k$ prove t/db_dependent/Virtualshelves.t
=> SUCCESS: Tests pass!
9. Sign off :-D
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Laura_Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
If a plugin reports a date_authored or date_updated we can't parse,
remove the invalid property from the metadata hashref and emit a
warning.
To test this, introduce a separate test plugin directory and test file.
Having a plugin that produces warnings in the main test directory would
require accounting for this warning in every existing test that loads
plugins. Correspondingly, this patch also fixes an invalid date in an
existing test plugin.
Test plan:
1. Apply patch.
2. Run `prove -r t/db_dependent/Koha/Plugins/` and verify there are no
test failures.
Sponsored-by: Open Fifth
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Laura_Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
If dt_from_string throws an exception whilst trying to parse the input,
catch it and return an empty result.
Test plan:
1. Apply patch.
2. Prove t/db_dependent/Template/Plugin/KohaDates.t
Sponsored-by: Open Fifth
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Replaces all calls to GetNormalizedOCLCNumber() and
Koha::Util::MARC::oclc_number() with the extractor pattern.
OPAC scripts pass their already ViewPolicy-filtered $record directly
to the extractor. This preserves the existing filtering behavior
without introducing new abstraction layers:
Koha::Biblio::Metadata::Extractor->new( { metadata => $record } )->get_normalized_oclc
Intranet scripts and the Koha::Biblio convenience method use the
raw cached extractor (035 visibility is not affected by ViewPolicy
in practice, and intranet doesn't filter):
$biblio->normalized_oclc
Test plan:
1. Verify OCLC numbers display on OPAC detail, ISBD, MARC detail
2. Verify staff catalogue detail and search results
3. prove t/db_dependent/Koha/Biblio.t
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Assisted-by: Sonnet 4.6 (Anthropic)
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Moves get_normalized_oclc from the MARC21-specific extractor to the
base ::MARC class (035$a is flavor-independent) with improved matching:
- Case-insensitive OCoLC detection (matches ocolc, OCOLC, etc.)
- Generic orgcode prefix stripping (any parenthesized prefix, not
just literal "(OCoLC)") for resilience against cataloging typos
Test plan:
$ prove t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC/MARC21.t
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Assisted-by: Sonnet 4.6 (Anthropic)
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch adds a new option to allow defining overlay rules for 'Add to basket' - such that they
will be used when adding orders in acquisition. Existing 'Staged MARC Import' rules will be copied to
the new fitler in order to preserve behavior
To test:
1 - Paste the following into MarcFieldsToOrder and save
price: 975$p
quantity: 975$q
budget_code: 975$h
2 - Paste the following into MarcItemFieldsToOrder and save
homebranch: 949$a
holdingbranch: 949$b
itype: 949$y
nonpublic_note: 949$x
public_note: 949$z
loc: 949$c
ccode: 949$8
notforloan: 949$7
uri: 949$u
copyno: 949$t
price: 949$g
replacementprice: 949$v
itemcallnumber: 949$o
quantity: 949$k
budget_code: 949$l
3 - Go to Admin -> Record overlay rules
4 - Define 2 rules:
Source - Staged Marc Import - 949 - Add and append (the rest of the options are set when add and append is selected
Source - Staged Marc Import - * - Protect (the rest of the options are set when add and append is selected
5 - Grab test file from bug 39518
6 - Import the records, ignoring items
7 - Edit the test file and change some values for the 949 rows
8 - Go to Acquisitions, find a basket for a vendor, add to basket from a new file
9 - Stage the file to find matches (already imported above) and to replace
10 - Complete the orders
11 - View the record and confirm the new 949 are added
12 - Edit the 949 in the file again
13 - Apply patch, update database, restart all
14 - Edit overlay rules and remove the 'Staged Marc Import' rules, leaving only the 'Add to basket rules'
15 - Add to basket again using the updated file and confirm new 949 are added
16 - Success!
Signed-off-by: Benjamin Daeuber <BDaeuber@fargolibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This page is intended for redirect/return from external payment vendors - they are not going to have/get a CSRF token from Koha for this.
The payments are handled in a POST to the API, so this should not be a sensitive page, it just provides user confirmation.
Some vendors use only a POST, we should not require CSRF on this page.
Test Plan:
1) Apply the first patch
2) prove t/Koha/Middleware/CSRF.t
3) Note the tests fail!
4) Apply this patch
5) prove t/Koha/Middleware/CSRF.t
6) All tests pass!
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
All errors generated during process_invoice and receipt_items now
store the invoicenumber of the specific invoice being processed. This
allows errors to be displayed only on the relevant invoice page rather
than appearing on every invoice generated from the same EDI file.
Also fixes two pre-existing spacing bugs in error message strings
("itemnumberis missing" and "barcode"for invoice line").
Signed-off-by: Ray Delahunty <lib-systems@arts.ac.uk>
Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Ray Delahunty <lib-systems@arts.ac.uk>
Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
When EDI invoice processing fails to match an item to a branch, the
error message now includes:
- The ordernumber referenced in the invoice (RFF+LI segment)
- The branches that order actually has items at
- A hint that the ordernumber in the invoice may be incorrect
Also adds a warning when an invoice line references an order that is
already marked as complete, which can indicate the supplier sent the
wrong ordernumber.
These improvements help acquisitions staff and vendors quickly diagnose
whether the problem is a supplier error (wrong RFF+LI value) rather
than a Koha data issue.
Signed-off-by: Ray Delahunty <lib-systems@arts.ac.uk>
Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch logs any object messages returned by cancel() to the EDI log,
including the itemnumber if available.
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
These private subs were added without pod_coverage documentation,
causing a QA failure. Add minimal =head3 blocks to resolve it.
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: Sharon Dugdale <sharon.dugdale@cumberland.gov.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The process_ordrsp() function was calling C4::Acquisition::ModOrder()
directly when processing a supplier cancellation, which only updated
the order status fields and left associated items and purchase
suggestions untouched.
Replace with Koha::Acquisition::Order->cancel(), which mirrors the
behaviour of manual cancellation via the acquisitions UI:
- Deletes on-order items via safe_delete() with error capture
- Reverts any linked purchase suggestion from ORDERED back to ACCEPTED
- Updates orderstatus, cancellationreason and datecancellationprinted
The delete_biblio option is intentionally omitted (defaults to false)
as silent biblio deletion is not appropriate for an automated EDI flow.
ModOrder is retained for the non-cancellation branch which only needs
to update suppliers_report.
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: Sharon Dugdale <sharon.dugdale@cumberland.gov.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Tomás Cohen Arazi
Martin Renvoize
Kyle M Hall
Paul Derscheid
Pedro Amorim
David Cook
Olivia Reynolds
Nick Clemens