This patch makes the tests reflect the design decision made on the bug report.
The author for the original 'No CGISESSID then error' patch signed off on this
so I'm confident this is the right thing to do.
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Add tests for the OAuth/OIDC login endpoint covering:
- IdP-initiated flow (no CGISESSID cookie on initial request)
- Callback without CGISESSID cookie (session lost)
- Normal SP-initiated flow (cookie present)
Tests create a real identity provider in the database and assert:
- Initial request without cookie redirects to the IdP authorize URL
- A CGISESSID cookie is set for the new session
- Callback without cookie returns wrong_csrf_token error (not 500)
- Normal flow with cookie redirects to IdP as expected
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
1. add missing <tr> wrapper element
2. amend casing in cypress test
3. change method name so it is codespell-friendly
4. remove deprecated test from TestBuilder.t
(used Overduerules which do not exist anymore.
rely on 'Tests with composite FK in userpermission'
for FK test coverage instead -> no replacement
introduced.)
5. 'Bug 10190: Fix patron_categories permission format'
changed the permission for GET requests to patron_categories
to catalogue = 1, matching the Koha precedent for such
endpoints. However, patron_categories still tested the
endpoint with the old permission assigned. Amend this to
complete the work started in that commit.
6. remove redundant delete statement in permission test:
if the test fails, this means the db update has not
run, and should. It would be incorrect for the db
to still include the permission at the point where the
test runs.
Test plan:
1. run `npm run js:build` (or watch the js:watch output)
2. run t/cypress/integration/Admin/CirculationTriggers_spec.ts
3. run prove xt/author/codespell.t
4. run prove t/db_dependent/TestBuilder.t
5. run prove t/db_dependent/api/v1/patron_categories.t
6. run prove t/Koha/Auth/Permissions.t
All should run with no warnings or errors.
Assisted-by: Claude Opus 5 (Anthropic)
Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
1. apply html filter to letters so notice names are escaped (must
be done in attribute so the JSON format of the letters can
be parsed).
2. fix a correctness issue where the frontend was pinning the
logged in library instead of the logged in user's home library,
which would have led to 403s (correct) when saving rules. Since
the library dropdown is deliberately disabled unless a user has
the manage_circ_rules_from_any_libraries permission, the user
would then have needed to use 'Set library' to match their home
library to fix this. Prevent the issue by setting the library
to the user's home library.
Also rename the variable to user_library_id.
3. list_rules correctly references /api/v1/circulation_rules/kinds
4. make untranslatable strings translatable
5. swap svg for font-awesome icon.
6. avoid css duplication:
- remove dead / unused scoped css (leftover from an early component
extraction)
- import vue.css and only leave the minimal override required.
7. improve set_rules test coverage: add tests for manage_circ_rules
/ manage_circ_triggers separation
Test plan:
1) Administration > Circulation triggers: notice the 'Letter' dropdowns
list circulation notices as "name (code)" and offer 'No letter'.
(no regression)
2) Give a staff user 'catalogue' + 'manage_circ_triggers' only, with
home library A. Log in as them, 'Set library' to B, and open
Administration > Circulation triggers. Notice the view is pinned to
A, and that adding and editing a trigger saves without a 403.
(behaviour is now correct)
3) GET /api/v1/circulation_rules?rules=nosuchrule - notice the error
points at /api/v1/circulation_rules/kinds.
(behaviour is now correct)
4) Run 'gulp po:update --lang fr-FR'. Notice 'Add new trigger
{triggerNumber}', 'No letter' and 'any' now appear in
misc/translator/po/fr-FR-messages-js.po
(behaviour is now correct)
5) Edit a trigger. Notice the delay's reset button is an 'x' inside the
input, left of the chevrons, and clears the delay; the letter's
reset button does the same for the notice.
(no regression)
6) Notice no visual regression across the list, add, edit, reset and
delete screens - select widths, chevrons and modals unchanged.
Also run t/db_dependent/api/v1/circulation_rules.t
Assisted-by: Claude Opus 5 (Anthropic)
Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Koha::Calendar is, by construction, a per-library object — it croaks
without a branchcode argument and every method is branch-scoped. The
top-level namespace hides that. Move the whole tree under
Koha::Library::Calendar to match the actual semantics, the existing
/libraries/{library_id}/calendar REST routes, and the established
Koha::Library::* convention (Group, Hour, FloatLimit, OverDriveInfo).
This commit:
- renames the Koha::Calendar(::*) tree to Koha::Library::Calendar(::*)
- updates package declarations, POD and internal 'use' statements
within the moved tree
- updates the four DBIC Schema Result overrides (koha_object_class /
koha_objects_class) to the new namespace
- updates callers inside C4::* modules (Calendar, Circulation,
Reserves, Overdues, HoldsQueue), Koha::{Patron,Hold,Checkouts,
CurbsidePickup}, circ/returns.pl, the installer and cronjobs
- updates the 42310-owned tests (Calendar tests, HoldsQueue.t,
Holds/WaitingReserves.t, Holidays.t, Koha/CurbsidePickups.t,
Reserves/CancelExpiredReserves.t and the libraries.t CRUD block)
Koha::Library->calendar, the REST controllers and the 42311 tests
are updated in separate follow-up commits under their respective
bugs.
Test plan:
- prove t/db_dependent/Koha/Library/Calendar.t
- prove t/db_dependent/Calendar.t
- prove t/db_dependent/Holidays.t t/db_dependent/HoldsQueue.t
- prove t/db_dependent/Holds/WaitingReserves.t
- prove t/db_dependent/Reserves/CancelExpiredReserves.t
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Add to Koha::Calendar:
add_weekly_closure, add_repeating_closure, add_single_closure,
add_exception, delete_weekly_closure, delete_repeating_closure,
delete_single_closure, delete_exception, copy_to, _clear_cache
C4::Calendar write methods now delegate to Koha::Calendar instead
of using raw DBI. No old table references remain in C4::Calendar.
Tests in t/db_dependent/Koha/Calendar.t cover all CRUD operations:
create, verify via is_holiday, delete, copy_to (including
idempotency).
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
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>
UseRecalls was converted from a YesNo syspref to a Choice preference
("off"/"opac"/"staff"/"opac_and_staff") earlier in this bug, and every
call site was updated from a truthy check to `ne "off"` - except this
one. Since even the string "off" is truthy in Perl, the guard in
Koha::REST::V1::Transfers::cancel was silently defeated: cancelling a
transfer via the REST API always attempted to revert an in-transit
recall, even with recalls disabled.
Test plan:
1) prove t/db_dependent/api/v1/transfers.t - new subtest
'cancel() - UseRecalls disabled' fails against the old code
(the in-transit recall gets reverted even though UseRecalls is off)
2) Apply this patch
3) Same test now passes
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This fixes a test to use 'opac' in transfers.t
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The existing tests pass even if the endpoint counts all checkouts
instead of overdues only. This patch adds a checkout due in the future
and tests the count is unchanged. It also tests that a patron without
the borrowers flag gets a 403.
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Returns the count of overdue checkouts for a patron as a plain integer.
Computed live from the database (date_due < NOW()) to ensure accuracy,
as overdue status is time-based with no discrete event to trigger updates.
This endpoint is intended for async loading in the patron search table,
allowing the main search results to render immediately while overdues
are fetched separately per visible page.
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/api/v1/patrons_overdues_count.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
- Add OR-type x-koha-authorization to /circulation_rules, /circulation_rules/kinds,
and /circulation_rules/config endpoints so users with either
circulate:circulate_remaining_permissions OR parameters:manage_circ_rules
can access them
- Enforce manage_circ_rules_from_any_libraries in the set_rules controller:
users without this permission may only set rules for their own library and
cannot set default (*) rules
- Add explicit use C4::Auth qw(haspermission) import to CirculationRules.pm
- Lock the library selector in the Vue UI for users who lack
manage_circ_rules_from_any_libraries, defaulting them to their own library
- Update set_rules() tests to cover the new permission restrictions
Signed-off-by: Michaela <michaela.sieber@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk>
Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Signed-off-by: Michaela <michaela.sieber@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Test GET/POST/DELETE /api/v1/patrons/{id}/account_links.
Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Signed-off-by: Kellie Stephens <kellie.stephens@bywatersolutions.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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 OpenAPI spec documents from_library_id as a supported filter
parameter but the test suite only exercised to_library_id. Add a test
to verify filtering by source library returns the correct transfer.
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() controller reverts any in-transit recall tied to the item
when UseRecalls is enabled. Add a subtest to verify the recall status
is set back to 'requested' after the transfer is cancelled.
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>
The + character in RFC3339 timezone offsets (e.g. +00:00) is decoded
as a space when interpolated directly into a URL query string. Use
Test::Mojo form => { q => ... } which properly URL-encodes the
parameter value.
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>
- Add 'library' to the allowed x-koha-embed values for the
GET /patrons/{patron_id} endpoint
- Add unit test verifying the library embed returns library_id
and name
Test plan:
1. Apply patch
2. yarn api:bundle
3. prove t/db_dependent/api/v1/patrons.t
4. Verify all tests pass, including the new 'library embed'
subtest under get() which verifies that requesting
x-koha-embed: library returns the patron's library object
Sponsored-by: Büchereizentrale Schleswig-Holstein <https://www.bz-sh.de/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
1- Corrected tests according to the comments
2- Removed unused import
3- Added syspref check of 'patronimages'
4- Tweaked swagger path schema
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>
Api changes adding GET route for patronimage
Test plan:
1- Apply the patch
2- Run `npm run api:bundle`
2- Go to administration -> sys.pref. -> set patronimages to allow
3- Log in koha using a user with "list_borrowers" or "edit_borrowers" privileges
4- Have a patron with a profile image (add an image if necessary by
searching for a patron and then clicking on the default profile image on
the top left)
5- Open a an API testing tool (such as postman), write
{intranet_url}/api/v1/patrons/{borrowernumber}/default_image
where {intranet_url} is the base intranet url of koha and {borrowernumber}
is the borrowernumber of a borrower that has an image
6- Add OAuth 2.0 credentials coming from koha (patron -> more -> manage
api keys) and generate a token
7- Send the request and notice the image of the patron is returned
8- Do step 5-7 with a borrowernumber of a patron without an image and a
borrowernumber of a patron that doesn't exist (and other edge cases
where there should be an error message displayed such as "Access
forbidden", "Authentication required", etc.)
9- Notice an appropriate error message is displayed
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Bug 35797: Rename test file from 'image.t' to 'patron_images.t'
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>
To test:
1. prove t/db_dependent/api/v1/idp.t
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The default `_match=contains` matching builds a LIKE query even on
integer columns, so a plain `?object=42` also matches object values
like 420 or 142. The test's dynamically-assigned patron borrowernumber
(auto_increment, never reset by transaction rollback) could drift into
a value containing "42" as a substring, intermittently inflating the
match count in CI. Add `_match=exact` since the test's intent is an
exact object id match.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The ILL Vue app currently reads ILLModule and ILLPartnerCode through the system preferences service. That service requires parameters/manage_sysprefs, so a staff user with ILL permissions but without system preference administration permissions cannot load the ILL module.
Add an ILL-specific /ill/config REST endpoint protected by the ill permission, and switch the ILL Vue app to load its configuration from that endpoint. This follows the module-specific configuration endpoint pattern introduced by bug 33606 for the same manage_sysprefs problem in the ERM Vue app.
Security/compatibility considerations: the endpoint returns only allow-listed, non-secret ILL configuration values needed by the ILL Vue app. It does not expose the generic system preferences API, and /api/v1/sysprefs remains restricted to parameters/manage_sysprefs.
Regression test coverage: the new API test verifies that a staff user with the ill permission can read /ill/config, that a staff user without the ill permission cannot read it, and that the same ILL-only user still cannot access /api/v1/sysprefs.
Test plan:
1. prove -I. -It/lib t/db_dependent/api/v1/ill_config.t
2. prove -I. -It/lib t/db_dependent/api/v1/ill_users.t
Signed-off-by: David Nind <david@davidnind.com>
Amended-by: Jonathan Druart
chmod +x t/db_dependent/api/v1/ill_config.t
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
t/db_dependent/Holds.t, t/db_dependent/ILSDI_Services.t, and
t/db_dependent/api/v1/holds.t all fail if a site has a global
'Holds Allowed (Daily)' circulation rule configured, because
C4::Reserves::CanItemBeReserved checks holds_per_day before
holds_per_record/reservesallowed, so an ambient daily-cap rule
returns tooManyReservesToday and masks the status these tests
actually exercise (tooManyReserves, noReservesAllowed, OK, or the
API's hold_limit override). The affected subtests never set
holds_per_day themselves, so they unintentionally depend on the
database having no such rule configured.
Test plan:
1. On a fresh db, set a global 'Holds Allowed (Daily)' circulation
rule (Administration > Circulation and fine rules) to 1.
2. prove t/db_dependent/Holds.t t/db_dependent/ILSDI_Services.t
t/db_dependent/api/v1/holds.t
=> fails before this patch, passes after.
3. Remove the circulation rule and re-run the same tests.
=> still passes (no regression for the default/unconfigured case).
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>
INSERT INTO action_logs (timestamp, user, module, action, object, info, interface) VALUES
(NOW(), 5, 'CIRCULATION', 'ISSUE', 5,
'{"issue":1,"branchcode":"CPL","itemnumber":1,"confirmations":["TOO_MANY"],"forced":["DEBT"]}', 'intranet'),
(NOW(), 5, 'CIRCULATION', 'RETURN', 5,
'1', 'intranet'),
(NOW(), 5, 'HOLDS', 'CREATE', 7,
'{"hold":7,"biblionumber":1,"itemnumber":1,"confirmations":["HIGHHOLDS"],"forced":["HOLD_POLICY_OVERRIDE"]}', 'intranet');
Test plan:
1) Without the patch applied, run the populate SQL above against the kohadev database.
2) Go to Tools > Log viewer
3) Under Modules, select only Circulation - leave the date range empty and click Submit.
4) Observe the ISSUE row's Info column shows the raw JSON payload as plain text, and the RETURN row shows the bare itemnumber "1" as plain text - neither is a clickable link to the item.
5) Go back, select only the Holds module, click Submit, and observe the CREATE row's Info column likewise shows raw JSON with no hold/item links.
6) Apply the patch.
7) Run yarn build && restart_all, then hard refresh the browser.
8) Repeat step 3.
9) Confirm the ISSUE row's Info column now shows a link reading "Item <barcode> CPL" (e.g. Item 3999900000001 CPL) - the branchcode comes from the JSON payload - and clicking it opens the item page. Override information is also displayed correctly
10) Confirm the RETURN row's Info column now shows a link reading "Item <barcode>" (no branchcode) opening the same item's detail page - verifying the bare-itemnumber payload path.
11) Repeat step 5. Confirm the CREATE row now shows "Hold 7 on biblio 1" linking to reserve/request.pl?biblionumber=1, followed by " - Item 1" linking to the item detail page. Override information is also displayed correctly
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch adds Koha::REST::Plugin::CSRF which enforces CSRF token
validation on state-changing requests (POST/PUT/DELETE/PATCH) when
a CGISESSID cookie is present.
Requests using OAuth2 or HTTP Basic Auth are not affected since they
don't use session cookies and are inherently immune to CSRF.
The client-side already sends the CSRF-TOKEN header via http-client.js
(added in Bug 34478). This patch adds the server-side enforcement.
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/REST/Plugin/CSRF.t
=> SUCCESS: Tests pass!
3. Run:
k$ prove t/db_dependent/api/v1/csrf.t
=> SUCCESS: Tests pass!
4. Tests cover:
- GET/HEAD/OPTIONS requests bypass CSRF check
- POST/PUT/DELETE without CGISESSID bypass CSRF check
- POST with CGISESSID but no CSRF-TOKEN returns 403
- POST with CGISESSID and invalid CSRF-TOKEN returns 403
- POST with CGISESSID and valid CSRF-TOKEN passes through
- PUT and DELETE enforcement
5. Sign off :-D
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch adds configuration options for cashup reconciliation and
enhances the user interface with preview functionality and improved
interaction patterns.
Configuration Options:
1. CashupReconciliationNoteRequired (System Preference):
- Optional validation requiring reconciliation notes when discrepancies exist
- Ensures documentation of surplus/deficit causes when needed
- Can be disabled for less stringent workflows
- Applied at cashup completion time
2. CashupReconciliationNoteAuthorisedValue (System Preference):
- Allows use of authorized value dropdown for reconciliation notes
- Provides standardized note options (e.g., "Till count error", "Cash removed")
- Falls back to free text textarea when not configured
- Improves consistency and reporting capabilities
UI Enhancements:
1. Cashup Summary Preview:
- Preview cashup summary for in-progress cashups
- Shows current transaction totals before completion
- Clearly indicates preview vs. final summary status
- Helps staff verify amounts before committing
2. Improved User Experience:
- Conditional note fields based on configuration
- Authorized value dropdowns when configured
- Clear validation messages
- Responsive UI updates
Backend changes:
- Enhanced Koha::REST::V1::CashRegisters::Cashups with preview support
- Database atomicupdate script for system preferences
- Validation logic for required notes
Frontend changes:
- Preview modal functionality in cashup_modal.js
- Conditional rendering of note input fields
- Authorized value dropdown integration
- Enhanced summary display logic
Test plan:
1. Apply patches and run database update
2. Configure CashupReconciliationNoteRequired:
- Enable preference
- Perform cashup with discrepancy without note - should fail
- Add note and retry - should succeed
- Disable preference - verify note optional
3. Configure CashupReconciliationNoteAuthorisedValue:
- Create AV category (e.g., CASHUP_NOTE) with values
- Set preference to category name
- Verify dropdown appears in cashup modal
- Clear preference - verify textarea shown
4. Test cashup preview:
- Start cashup on register
- Click "Preview cashup summary"
- Verify current totals displayed
- Add transaction and re-preview - verify updated
- Complete cashup - verify final summary
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: Jackie Usher <jackie.usher@westsussex.gov.uk>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The previous follow-up restored warn in Object::store but missed
several test files that trigger DB exceptions via ->store(). These
tests now fail Test::NoWarnings because the warning is emitted but
not expected.
This patch adds warning_like wrappers to 15 additional test files,
asserting that the expected DB exception warning is emitted.
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/api/v1/biblios.t \
t/db_dependent/Koha/Holds.t \
t/db_dependent/api/v1/return_claims.t \
t/db_dependent/api/v1/libraries.t \
t/db_dependent/Koha/Booking.t \
t/db_dependent/api/v1/patrons.t \
t/db_dependent/SIP/Message.t \
t/db_dependent/Koha/Patron.t \
t/db_dependent/Koha/Acquisition/Fund.t \
t/db_dependent/api/v1/acquisitions_orders.t \
t/db_dependent/Koha/ILL/Requests.t \
t/db_dependent/Koha/Patron/Relationship.t \
t/db_dependent/Koha/Hold.t \
t/db_dependent/Circulation/Returns.t \
t/db_dependent/api/v1/bookings.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The bug this fixed only manifests when the library's configured
timezone (C4::Context->tz) is not UTC: Koha::Object::_recursive_fixup
converts an incoming RFC3339 instant to that timezone before storing
it in the timezone-naive 'bookings.start_date'/'end_date' columns, so
a UTC-anchored boundary shifts by the timezone offset on write.
None of the existing Cypress specs catch this, because they never run
with the browser/test-runner timezone diverging from the configured
library timezone - both always come from the same container clock.
Add a Perl-level API test that mocks C4::Context->tz to a non-UTC zone
(America/New_York) and asserts the boundaries sent by place_booking.js
round-trip to the correct library-local date, pinning this regression
independently of any particular environment's timezone.
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch adds regression tests for the 500 Internal Server Error
triggered when GET /api/v1/acquisitions/orders is called with nested
+count embeds (e.g. biblio.items+count) combined with a has_many
prefetch (e.g. biblio.suggestions.suggester) and pagination.
The root cause is that DBIC's limiting subquery optimization places
+select scalar refs in the inner subquery where the parent join alias
(biblio) is not yet available, resulting in:
DBD::mysql: Unknown column 'biblio.biblionumber' in 'WHERE'
The regression was introduced in:
commit e51334604f
Bug 41950: Make +count embeds sortable via SQL COUNT subqueries
Test plan:
1. Apply this patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/api/v1/acquisitions_orders.t
=> FAIL: The new subtest fails with 500 errors instead of 200
3. Sign off :-D
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
If a patron is deleted and there's a checkout associated with them, an api call to {url}/api/v1/checkouts?checked_in=true
will result in an error. This patch updates the api to allow for a null value to be returned for patron_id
To test:
Before applying the patch:
1. In Administration->System preference enable the RESTBasicAuth system preference.
2. Checkout an item to a patron (I used item barcode: 39999000000252 and patron barcode 23529000121682)
3. Check the item back in
4. In your brower go to {sandbox/ktdurl}/api/v1/checkouts?checked_in=1
5. Observe there is a patron ID in the JSON response
6. Delete the patron
7. Repeat step 4
8. Observe an error because a "null" value was returned
9. Apply patch
Sandbox:
In sandbox management Actions->Apply patches
Once complete Actions->Build CSS and JS
Actions->Restart services
KTD:
ktd --shell
git bz apply 41889
yarn build
restart_all
10. Repeat step 4
11. Observe the checkout loads, but now has a null patron id.
12. Sign off
On KTD can also run the script: qa
to run tests.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
These highlight why we may have made this choice. With only 'edit_borrowers' you can make
changes, but you cannot view your changes as you aren't allowed to see other borrowers.
Test now pass, but not sure if this answers the question of why we did it this way, or if there is more
work here. Should you be able to get back the fields that you updated at the least, to confirm your changes? Or
should we send a success with a message that you can't see the data?
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
An exception is not implemented as progress_request is called in
different circulation contexts (hold is placed, checkout happens,
etc) and those actions should not be dependent on the outcome of
progress_request, i.e. if progress_request fails due to schema
validation failure, the circulation action should happen as before.
prove t/db_dependent/Koha/ILL/ISO18626/Request.t
prove t/db_dependent/api/v1/iso18626/request.t
cypress run --spec t/cypress/integration/ILL/SupplyingRequests.ts
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This commit applies changes to V1.pm to be able to utilize the same XML methods.
We're also updating tests to reflect this new functionality.
Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Build on the new ActionLog DBIC relations to expose them as opt-in
embeds, and shore up gaps in the test coverage that QA flagged.
Swagger
* paths/action_logs.yaml declares an x-koha-embed header parameter
constrained to the supported relations (librarian, patron).
* definitions/action_log.yaml adds the corresponding optional
`librarian` and `patron` object fields.
Tests
* Convert the single 24-test subtest into focused subtests:
- authorization (403 vs 200)
- simple typed filters
- q-DSL: multi-value module, info LIKE, combined filter
- embeds: librarian and patron, with and without x-koha-embed
- pagination + X-Total-Count and order_by
* Mock AuthSuccessLog/AuthFailureLog/BorrowersLog to 0 to remove the
flakiness where the API GET itself appended an AUTH log row between
delete and assertion.
* Build the test librarian with both `tools` and `borrowers` flags so
embedded patron fields are not redacted by Koha::Patron->is_accessible.
* Use a small build_fixtures() helper to avoid copy-pasting setup
across subtests.
Test plan:
1. yarn build && restart_all (refresh swagger bundle)
2. prove t/db_dependent/api/v1/action_logs.t (all subtests green)
3. curl -H 'x-koha-embed: librarian,patron' \
/api/v1/action_logs?module=MEMBERS -- patron and librarian
objects appear in the response.
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
The cash register transaction history page (pos/register.pl) allows
access with either cashup OR anonymous_refund permission, but the
API endpoints for fetching cashups only checked for cashup permission.
This caused a 403 error when users with only anonymous_refund permission
tried to view the transaction history page, as the cashups table failed
to load.
This patch updates the API permissions for:
- GET /cash_registers/{id}/cashups
- GET /cashups/{id}
to accept either cashup or anonymous_refund permission, matching the
page access logic.
Test plan:
1. Create a staff user with only cash_management > anonymous_refund
permission (not cashup)
2. Navigate to Point of Sale > Transaction history for any cash register
3. Verify the cashups table loads successfully (previously returned 403)
4. Run: prove t/db_dependent/api/v1/cashups.t
Signed-off-by: Jackie Usher <jackie.usher@westsussex.gov.uk>
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Rename /libraries/{id}/holidays to /libraries/{id}/closed_dates
for consistency with existing sub-resource naming (desks,
cash_registers).
Efficiency: instead of calling $calendar->is_holiday() per day
(which re-checks all three data sources each time), access the
pre-loaded data structures directly: weekly_closed_days array,
day_month_closed_days hash, and _holidays hash. The logic properly
handles exceptions (dates marked open despite a weekly/annual
closure).
Additional fixes:
- Add 365-day max range guard
- Add missing 401/403 responses in swagger spec
- Add request_id_header parameter
- Test exception handling (open on normally-closed day)
- Remove fragile 'empty defaults' assertion
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
- Add list_holidays controller to Koha::REST::V1::Libraries
- Returns closed days for a library within a configurable date range
- Uses Koha::Calendar to determine holidays
- Defaults to today + 3 months if no range specified
- Add OpenAPI spec for the new endpoint
- Register route in swagger.yaml
- Add unit tests in t/db_dependent/api/v1/libraries.t
To test:
1. Apply patch
2. prove t/db_dependent/api/v1/libraries.t
3. Verify all tests pass, including the new list_holidays subtest
which covers:
- Holidays returned within a specified date range
- Empty list when no holidays exist in range
- 400 response when 'to' date precedes 'from' date
- 404 response for non-existent library
- Default range (no params) returns successfully
Sponsored-by: Büchereizentrale Schleswig-Holstein <https://www.bz-sh.de/>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
The OPAC REST self-checkout path (guarded by OpacTrustedCheckout) is
functionally SCO-over-API, so it should honour the same hold policy
as the SCO web interface. Previously _check_availability unconditionally
promoted RESERVED, RESERVE_WAITING, TRANSFERRED and PROCESSING from
the confirmation hash to the blockers hash for any public caller. This
meant the OPAC REST endpoint always refused held items regardless of
the librarian's AllowItemsOnHoldCheckoutSCO configuration.
With this patch the promotion is gated on AllowItemsOnHoldCheckoutSCO:
* 0 - Promote all four hold confirmations to blockers (previous
behaviour for pending holds)
* 1 - Promote only waiting/transit/processing; silently allow
checkout of items with a pending hold
* 2 - Promote none; silently allow checkout of items with any hold
Remaining hold confirmations are stripped from the response so the
caller does not have to resolve a confirmation token for a policy the
admin has already granted.
Test plan:
1. prove t/db_dependent/api/v1/checkouts.t
2. Enable OpacTrustedCheckout. POST an item with a pending hold for
another patron to /api/v1/public/patrons/X/checkouts:
- AllowItemsOnHoldCheckoutSCO=0: 403 Checkout not authorized
- AllowItemsOnHoldCheckoutSCO=1: 201 Created
3. Place a waiting hold for another patron on an item. Repeat:
- AllowItemsOnHoldCheckoutSCO=1: 403
- AllowItemsOnHoldCheckoutSCO=2: 201
Signed-off-by: Kristi Krueger <kkrueger@cuyahogalibrary.org>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Tomás Cohen Arazi
Chloé Zermatten
Martin Renvoize
Katrin Fischer
Kyle M Hall
Martin Renvoize
Jacob O'Mara
Paul Derscheid
Shi Yao Wang
Lari Taskula
Pedro Amorim
Andrii Nugged
Matt Blenkinsop
Lisette Scheer
Jonathan Druart
Lucas Gass