This patch adds an option -exact_days, to filter objects by an exact
number of days.
To test:
1 - Apply test patch
2 - Run tests -> they fail
3 - Apply patch
4 - Run tests -> they don't fail
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The INIT/END blocks in Koha::Script fire for every consumer of the
module, including non-cron admin and migration tools. Logging those as
CRONJOBS when CronjobLog is enabled would pollute the action log with
entries for tools that are not scheduled jobs.
Introduce a package-level $_cron flag that is set by import() when the
-cron argument is passed (import() runs at compile time, before INIT
blocks execute, so the flag is in the correct state when INIT fires).
Guard both the INIT and END blocks behind this flag so only scripts that
explicitly declare themselves as cron scripts receive automatic logging.
Also update the import() POD to document the auto-logging behaviour.
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Add INIT and END blocks to Koha::Script that automatically call
cronlogaction on script start (with command line options) and
completion. This removes the need for scripts to manually handle
cron log entries.
This will mean that any script importing Koha::Script will be logged,
but I think the addition is beneficial.
To test:
1 - Ensure CronjobLog is enabled
2 - perl misc/cronjobs/fines.pl --test
3 - Confirm start and end are logged in action logs:
http://localhost:8081/cgi-bin/koha/tools/viewlog.pl?src=&user=&modules=CRONJOBS&actions=&object=&info=&interfaces=&from=&to=&output=screen&basename=Export&report_name=&do_it=1
4 - Apply patch
5 - Repeat 2
6 - Reload the logs, confirm the new logs look the same
7 - Sign off
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: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch implements the library_limit concept for Report/Reports. Staff with the correct permission and superlibrarians can set libaries to which the report should be visible, if the system preference is enabled.
Test plan:
Confirm Initial Settings
1. Log in to Koha staff with username: koha and password: koha and set your library as Centerville if it's not already.
2. Visit the Saved reports page, and create and save three reports: Report 1, Report 2, and Report 3. The SQL query for each report doesn't matter, it should just be something valid.
3. Visit the Patrons page and create two staff users: StaffReportLimitUser and StaffNoLimitPermission. Set their home libraries to Centerville and Fairfield, respectively.
4. Under Allow access to the reports module, ensure both users have report management permissions for Create, Delete, Execute.
Verify Patch Settings
5. Apply the patch and restart services. If using Koha Testing Docker (KTD): from the Koha repo directory, apply the patch (e.g. `git bz apply 16631`, or check out the patched branch with `git fetch && git checkout <branch>`), then run `ktd --shell` to enter the KTD shell and run `restart_all`. If the patched branch is already checked out, just enter the KTD shell and run `restart_all`.
6. Click Patrons in the top navbar to go to the Patrons page.
7. Search for "StaffReportLimitUser" in the patron search field.
8. Click StaffReportLimitUser to edit the patron record for this user.
9. Click More -> Set permissions to access the permissions screens for the StaffReportLimitUser.
10. In the row for Allow access to the reports module (reports), click Show details to view relevant permissions.
11. Confirm Manage Report Limits (manage_report_limits) is an option under Allow access to the reports module (reports).
12. Click the checkbox for Manage Report Limits to give this user this permission, and click Save.
13. Go to the System Preferences page (Click the More dropdown from the top nav bar, click Administration, click System Preferences).
14. Verify Reports is an option in the left side navigation bar, and that when clicked, you see the LimitReportsByBranch option.
15. Enable LimitReportsByBranch as described in step 14.
16. Go back to Saved reports as in Step 2.
17. Click Edit in the row menu for Report 1.
18. At the bottom of the Report 1 edit page, you should see a Library limitation select box.
Verify the Library Limitation Settings are Persisted
19. Choose Centerville from the select box and click Update SQL.
20. Reload the page, or navigate away from the page and go back to the edit screen for Report 1.
21. Verify that Centerville is still in the Library limitation select box.
22. Add Fairfield in the Library limitation select box and click Update and run SQL.
23. Go back to editing Report 1.
24. Confirm the Library limitation select box has both Centerville and Fairfield selected.
25. Duplicate Report 1.
26. Confirm the Library limitation select box has both Centerville and Fairfield selected.
27. Click Save Report.
28. Confirm the Library limitation select box has both Centerville and Fairfield selected.
Verify Reports are Limited Based on Library Limitations
29. Edit Report 1 to add Centerville in the Library limitation select box as the only choice and click Update SQL.
30. Edit Report 2 to add Fairfield in the Library limitation select box as the only choice and click Update SQL.
31. Report 3 should have no library limitations, so it should be visible to all libraries.
32. Go to Saved reports as in Step 2.
33. Confirm you do not see Report 2, and only Report 1 and Report 3.
34. In the upper right corner, click the drop down that says koha Centerville and click Set library to change your library.
35. Set your library to Fairfield.
36. Go to Saved reports as in Step 2.
37. Confirm you do not see Report 1, and only Report 2 and Report 3.
Verify Disabling LimitReportsByBranch Preference Shows All Reports
38. Disable LimitReportsByBranch preference as in steps 13 to 15.
39. Go to Saved reports as in Step 2.
40. Confirm Report 1, Report 2, Report 3 are visible.
41. Click to edit Report 1 and verify there is no Library limitation select box.
Verify Library limitations are Persisted When EnableFilteringReports is Toggled On or Off
42. Enable LimitReportsByBranch preference as in steps 13 to 15.
43. Ensure your library is set to Fairfield as in step 35.
44. Go to Saved reports as in Step 2.
45. Confirm Report 1 is not visible, and Report 2 and Report 3 are visible.
46. Click to edit Report 2 and verify the Library limitation select box is at the bottom of the page and that it has Fairfield selected.
Verify the Show all reports option shows all reports for users with the Manage Report Limits permission and superlibrarians
47. Go to Saved reports as in Step 2 and in the Show reports dropdown, select Show all reports, and click Apply filter.
48. Confirm the page reloads with all reports visible: Report 1, Report 2, and Report 3 should be visible.
49. Select Show my branch reports from the dropdown and click Apply filter.
50. Confirm Report 1 is not visible, and Report 2 and Report 3 are visible (you should only see reports for your branch, Fairfield).
Verify Report filters return the correct results
51. Go to Saved reports as in Step 2.
52. Verify you're viewing only your branch's reports in the filter.
53. Under Filter in the left sidebar, type "2" in the Keyword field and click Apply filter.
54. Confirm only Report 2 is visible.
55. Under Filter in the left sidebar, type "1" in the Keyword field and click Apply filter.
56. Confirm there are no reports visible, and the message "No saved reports match your criteria" is displayed.
Verify Library Limits column is available to superlibrarians and users with the Manage Report Limits permission
57. Go to Saved reports as in Step 2.
58. Click the Columns dropdown and ensure that the Library limits column is available to select for display. Select it.
59. Confirm that the Library limits column is now visible in the list of reports and that it shows the correct library limitations for each report (e.g., Report 1 should show 1 library limitation and Centerville (CPL) on hover, Report 2 should show 1 library limitation and Fairfield (FFL) on hover, and Report 3 should show no limitation).
Verify the Show all reports and Limit reports options are available to users that are not superlibrarian but have the Manage Report Limits permission
60. For the StaffReportLimitUser, add the permission for Manage Report Limits as described in steps 9 to 12. This will allow this user to see the Show all reports and Limit reports options.
61. Log out of the koha admin account. In the upper right corner, click the drop down that says koha Fairfield and click Log out.
62. Log in as StaffReportLimitUser, the previously created staff user with report permissions.
63. Confirm the staff user's library is Centerville in the upper right corner of the screen.
64. Go to Saved reports as in Step 2.
65. Confirm the filter option for Show reports is visible.
66. Click to edit Report 1.
67. Verify there is a Library limitation select box and that it can be interacted with.
Verify the Show all reports and Limit reports options are not available to users that are not superlibrarian and do not have the Manage Report Limits permission
68. Log out of the StaffReportLimitUser account.
69. Log in as StaffNoLimitPermission, the previously created staff user without the Manage Report Limits permission.
70. Confirm the staff user's library is Fairfield in the upper right corner of the screen.
71. Go to Saved reports as in Step 2.
72. Click the Columns dropdown and ensure that the Library limits column is not in the list of available columns to select for display.
73. Confirm the filter option for Show reports is not available.
74. Click to edit Report 2.
75. Verify there is no Library limitation select box at the bottom of the page.
[NEW] Verify library limits are auto-assigned and preserved correctly for non-permissioned users
76. Still logged in as StaffNoLimitPermission (Fairfield) from the previous section. Go to Saved reports and click New report. Enter "Limit Auto-Assign Test" as the report name and SELECT 1 as the SQL. Confirm there is no Library limitations select box visible on the form. Click Save.
77. Go back to Saved reports. Click Edit for Report 2 (limited to Fairfield). Add `-- test` as a trailing SQL comment and click Update SQL. This simulates a non-permissioned user saving an existing report that already has limits.
78. Go back to Saved reports. Click Edit for Report 3 (no library limits). Add `-- test` as a trailing SQL comment and click Update SQL. This simulates a non-permissioned user saving an existing report that has no limits.
79. Log out and log back in as StaffReportLimitUser (Centerville, has manage_report_limits).
80. Go to Saved reports. Use Show all reports if needed. Find "Limit Auto-Assign Test" and click Edit. Confirm the Library limitations select box shows Fairfield as the only selected library — the report was automatically limited to the creator's home library on create.
81. Find Report 2 and click Edit. Confirm the Library limitations select box still shows only Fairfield — the existing limit was preserved when the non-permissioned user saved the report.
82. Find Report 3 and click Edit. Confirm the Library limitations select box is empty — no limits were introduced when the non-permissioned user saved a report that had no existing limits.
Verify limited reports are not visible to users without permissions
83. While still logged in as StaffReportLimitUser from step 79, copy the URL for the edit page of Report 1.
84. Log out of the StaffReportLimitUser account.
85. Log in as StaffNoLimitPermission, the user without the Manage Report Limits permission.
86. Paste the copied URL for the edit page of Report 1 into the browser and navigate to it.
87. Confirm that you see an error message that says "This report is restricted and cannot be accessed by the current user." and that the page does not load the report details.
Run Unit Tests
88. Using Koha Testing Docker, in the Koha home directory run the command: ktd --shell to start the ktd shell.
89. Run command: reset_all to drop and recreate the koha database.
90. Run command prove t/db_dependent/Koha/Reports.t to run the test.
91. This file contains the unit tests for the reports_branches table and the new subtest `library limits enforcement on save and update`, which covers auto-assignment of branch limits on create and preservation of limits on update for non-permissioned users.
92. All tests should pass.
93. Run command prove t/Koha/Auth/Permissions.t
94. Permissions unit tests in this file were updated.
95. All tests should pass.
96. Run command prove t/db_dependent/Reports/Guided.t
97. The Guided report tests were updated to account for the new library limitations and permissions.
98. All tests should pass.
Signoff
99. Sign off if the steps above check out.
Sponsored-by: Ramapo Catskill Library System
Co-authored-by: Casey Conlin <casey.conlin@gmail.com>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
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>
Transitioning a booking to 'issued' no longer skipped clash detection,
and Item::check_booking treats any live checkout on the item as a
clash. Switching an on-site checkout of a booked item to a regular
checkout (SwitchOnSiteCheckouts) therefore crashed AddIssue with an
unhandled Koha::Exceptions::Booking::Clash; collection could fail the
same way whenever the biblio's bookable item pool had shrunk since the
booking was placed.
- short-circuit no-op stores before validation; circulation issues
redundant status writes that must not re-trigger the checks
- rename _is_final_status_transition to _skip_clash_detection and also
skip when transitioning to 'issued' from an active status, as long
as the booked period and item are unchanged
- keep validating period/item changes and reactivations of terminal
bookings
- cover the on-site switch in Circulation.t and the store() semantics
in Booking.t
Test plan:
1. Enable OnSiteCheckouts and SwitchOnSiteCheckouts
2. Place a booking starting today for a patron
3. Check the booked item out to that patron from Circulation > Check
out with 'On-site checkout' ticked. Do NOT use the bookings page's
'Transform to checkout' button: it submits a fixed field set that
does not carry the on-site flag (Bug 36789), so the checkout would
silently be a regular one and step 4 could not reproduce the switch
4. The booking shows 'Issued'. Scan the same barcode for the same
patron again to switch the on-site checkout to a regular one; this
crashed with Koha::Exceptions::Booking::Clash before the patch and
now succeeds with the booking still 'Issued'
5. prove t/db_dependent/Koha/Booking.t t/db_dependent/Circulation.t
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Implements a 4-state booking lifecycle: new → issued (on checkout) →
completed (on return), plus cancelled at any point.
Backend changes:
- AddIssue sets booking status to 'issued' (was incorrectly 'completed')
- AddReturn transitions 'issued' → 'completed' via booking_id on the issue
- filter_by_active updated to status-based: status IN ('new', 'issued');
previously it filtered by date range — active/inactive now reflect booking
lifecycle rather than the current date relative to start/end dates
- _is_final_status_transition reverted to only skip clash detection for
terminal statuses 'cancelled' and 'completed'; 'issued' is active and does
not bypass clash detection
- GET /api/v1/bookings no longer pre-filters to active bookings server-side;
callers must supply an explicit status filter if they need active-only
Display changes:
- Patron bookings tab: replaces the date-based expired/active toggle with
per-status filters (completed, cancelled) using the AdditionalFilters
framework; active bookings (new/issued) are shown by default
- Biblio bookings list: same status-based filters; removes the date-based
expired filter that was still present
- Status column is always visible on both views (was previously hidden on
the patron tab unless expired bookings were shown)
- Status badges: new=bg-success, issued=bg-info, completed/cancelled=bg-secondary
- Timeline isActive check corrected to ['new','issued']
- patron-detail-tabs.inc: rename expired_bookings_count → closed_bookings_count
to reflect status-based (not date-based) counting
- Filter button colour fixed to use $link-color so both views are consistent
Test plan:
1. Enable the EnableBookings system preference.
2. Create a biblio with at least one bookable item.
3. Place a booking for a patron via the biblio's Bookings tab.
- The booking should appear with status badge "New" (green).
4. Check out the booked item to that patron (Circulation → Check out).
- Reload the Bookings tab; the booking should now show "Issued" (blue).
5. Return the item (Circulation → Check in).
- Reload the Bookings tab; the booking should now show "Completed" (grey).
6. Place a second booking and cancel it from the Bookings tab Actions menu.
- The booking should show "Cancelled" (grey), with the reason if provided.
7. On the biblio Bookings tab:
- Completed and cancelled bookings are hidden by default.
- Clicking "Include completed" shows completed bookings.
- Clicking "Include cancelled" shows cancelled bookings.
- Both can be toggled independently.
- The Status column is always visible.
8. Navigate to the patron's detail page → Bookings tab.
- Active bookings (new/issued) are shown by default.
- Completed and cancelled can be toggled independently using the same
filter buttons.
- The Status column is always visible.
9. Confirm via the REST API that GET /api/v1/bookings (no filters) returns
bookings of all statuses, not just active ones.
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>
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>
To test:
1. Set ReservesControlBranch is set to 'item's home library'
2. Set the deafult 'Holds and bookings policies by item type' for the itype BK to allow holds from patrons from any library, but pickup at CPL only
3. Have a bib owned by another library ( I used MPL ), and bib owned by CPL. Both are the item type BK, and each has one item on the bib.
4. Place a hold for a patron on bib A for pickup at MPL
5. Move the hold to bib B
6. Note that the hold moves successfully, and is still set for pickup at MPL
7. Login and try to trigger the hold, nothing happens
8. APPLY PATCH, restart_all
9. Try spets 3 - 6 again. This time on step 6 you should see a message that the hold cannot be moved.
Signed-off-by: Eric Phetteplace <phette23@gmail.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Always include the plural version, not the singular one (the plural
imports the singular)
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Laura_Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch replaces all calls to C4::Stats::UpdateStats with the wrapped
function from the Koha namespace. As this is the last function in C4::Stats, the
module is also deleted.
Several tests depended on transitive includes to Koha::Statistics through
C4::Stats. I've identified uses of that module by searching, and added the
necessary use in each case. I've run `prove -r t xt` with this patch applied and
didn't hit any relevant errors.
Test plan:
1. Inspect the patch, and verify each changed call maintains the same parameters.
2. Apply patch.
3. Prove the following files:
- t/db_dependent/Accounts.t
- t/db_dependent/Circulation.t
- t/db_dependent/Koha/Account.t
- t/db_dependent/Koha/ILL/Request.t
- t/db_dependent/Koha/Item.t
- t/db_dependent/Koha/Pseudonymization.t
- t/db_dependent/Koha/Recalls.t
Sponsored-by: Open Fifth
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Laura_Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
To test:
1. Set both system preferences NotifyBorrowerDeparture and
MembershipExpiryDaysNotice to 10 days
2. Create or edit a patron account with the following settings:
- expiry date is exactly 10 days in the future
- add an email address on the patron account
- set messaging preferences to receive email notices for Patron
account expiry
3. Run the membership expiry cron job to generate a notice:
ktd --shell
perl misc/cronjobs/membership_expiry.pl -c
4. Check notices tab on patron account
--> A membership expiry notice has been queued for the patron
5. Click back to the patron details
--> Note that there is no alert that the patron account will be expiring
soon
6. Apply patch and restart_all
7. Refresh the patron details page
--> You should now see an alert that the account will be expiring soon
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>
Queries that use OR instead of IN are less efficient and can lead to
situations where the query takes a very long time to complete due to
row level scanning. We found one instance where they query had been
running for over 10 days!
This patch makes ids() return a subquery instead of a list of results,
and switches the callers to using IN. The database now resolves the set
itself and the queries stay a constant size no matter how large the
catalog is. This is super efficient because the subquery doesn't even
execute as a separate query since biblionumber is the primary key!
Test Plan:
1) Apply the unit tests patch
2) prove t/db_dependent/Koha/Database/DataInconsistency.t
3) Note the ids() subtest fails
4) Apply this patch
5) prove t/db_dependent/Koha/Database/DataInconsistency.t
6) Note all tests pass!
7) Run misc/maintenance/search_for_data_inconsistencies.pl
8) Note the output is the same as before the patch
9) Bonus: Re-run it with DBIC_TRACE=1 and note the queries now use
IN (SELECT ...) instead of one OR clause per bib!
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>
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 checks for waiting or in_transit items allocated to recalls, and excludes them from the Holds to Pull report.
To test:
1. Enable UseRecalls and set the recalls-related circulation rules
2. Check out an item to Patron A
3. Place an item-level hold on the item for Patron B
4. Log into the OPAC as Patron C and search for the item. Place an item-level recall on the item.
5. Go back to the staff interface and check in the item. This should prompt a pop-up to confirm the recall as waiting. Confirm the recall as waiting.
6. Go to Circulation -> Holds to pull.
7. Notice that even though the item is allocated for a recall, the hold is showing. It should not be showing in this page.
8. Apply the patch, restart services and refresh the page
9. The item allocated to a recall should no longer be showing in the Holds to pull report
10. Go to the record detail page and go to the Recalls tab
11. Revert the recall from its waiting status
12. Go back to the Holds to pull page. The record will now show for the hold.
13. Go to the Circulation -> Check in page
14. Go to the menu in the top-right of the page and click Set library
15. Change your library to a different one
16. Check in the item to confirm the recall as waiting and trigger a transfer
17. Go back to the Holds to pull page
19. The item should no longer show on the Holds to pull report
20. Confirm tests pass t/db_dependent/Koha/Holds.t
Sponsored-by: Catalyst IT
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
When RealTimeHoldsQueueUnallocated is enabled, the background job
passes unallocated => 1 and delete => 0 to update_queue_for_biblio.
This skips deleting existing queue entries and only processes holds
not already allocated, matching the --unallocated behavior of
build_holds_queue.pl.
Test plan:
1. Apply patches
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/BackgroundJob/BatchUpdateBiblioHoldsQueue.t
=> SUCCESS: Tests pass!
3. Tests verify:
- delete => 1 and no unallocated when pref is off
- delete => 0 and unallocated => 1 when pref is on
4. Sign off :-D
Signed-off-by: Alex Carver [Acerock7] <alex@rcls.org>
Sponsored-by: The Main Library Alliance <https://www.mainlib.org/>
Signed-off-by: Alex Carver [Acerock7] <alex@rcls.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
A cashup is the process of removing cash from the till to send it
to the bank, so labelling the headline cash line "Cash added to
register" was confusing. Re-word it to "Cash removed from register"
in the typical case and "Cash added to register" in the edge case
where the cashier has had to top up the till to cover refunds.
While doing this, align the sign convention of total_grouped with
that of the overall total field on the summary: positive means cash
collected (i.e. removed from the register at cashup time), negative
means cash had to be added back into the register. Previously
total_grouped used the raw accounting sign (credits negative, debits
positive), so a typical $2 cash session would display "Total: 2.00"
and "Cash added to register: -2.00", which was internally
inconsistent. The conditions on the cash and other-payment-type rows
in cashup_modal.js are swapped to match the new convention.
Test plan:
1. Apply the patch and rebuild JS (yarn js:build) inside KTD.
2. As a staff user with permission to manage cash registers, take
payments via several payment types (some cash, some not).
3. Trigger a cashup and view the cashup summary modal.
4. Confirm the "Total" line and the per-payment-type lines all
display as positive amounts in the typical "money was collected"
scenario, with the cash row labelled "Cash removed from
register".
5. Issue some refunds large enough to exceed cash collected, then
cash up again.
6. Confirm the cash row is labelled "Cash added to register" with a
negative amount and the per-non-cash-type rows are labelled
"{type} to add" where applicable.
7. Run prove t/db_dependent/Koha/Cash/Register/Cashup.t and prove
t/db_dependent/Koha/Cash/Register.t inside KTD; both should
pass.
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>
This patch resolves three QA findings on the cashup reconciliation
work:
* Add a CSP nonce to the inline <style> block in cashup_summary.inc
so it is not flagged by the forbidden_patterns check.
* Document the private _get_session_end method on
Koha::Cash::Register::Cashup so pod_coverage passes.
* Mock CashupReconciliationNoteRequired to 0 in
t/db_dependent/Koha/Cash/Register/Cashup.t so the test does not
depend on the site preference value (it previously failed when
the syspref was set to "Require" because surpluses and deficits
are created without a reconciliation note in this test).
Test plan:
1. Apply the patch.
2. Set CashupReconciliationNoteRequired to "Require".
3. Run prove t/db_dependent/Koha/Cash/Register/Cashup.t -- it should
pass.
4. Set CashupReconciliationNoteRequired to "Don't require" and
re-run -- it should still pass.
5. Run koha-qa.pl on the bug branch -- pod_coverage on
Koha/Cash/Register/Cashup.pm and forbidden_patterns on
cashup_summary.inc should now pass.
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>
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>
This patch implements an optional two-phase cashup workflow for point
of sale operations, along with improved error handling and validation.
Two-Phase Cashup Workflow:
- Staff can initiate cashup (CASHUP_START) and complete later
- Prevents new transactions on registers with cashups in progress
- Tracks cashup sessions with start/end timestamps
- Supports both quick cashup (immediate) and staged cashup workflows
- Updates batch cashup workflow operations across multiple registers
Error Handling & Validation:
- Centralized exception handling for cashup operations
- Clear validation messages for missing parameters
- Prevents cashup when no cash transactions exist
- Improved handling of zero/negative amounts
- Informative error messages for user guidance
Negative Amount Support:
- Supports negative cashup amounts for cashup deficits
- Automatic detection when actual cash is less than expected amount
- Creates appropriate CASHUP_DEFICIT records
- UI handles both positive and negative reconciliation amounts
- Updated calculations and display logic throughout
Modal Refactoring:
- Eliminates code duplication between register.tt and registers.tt
- Centralized cashup modal functionality in cashup_modals.js
- Shared confirm_cashup.inc and trigger_cashup.inc templates
- Consistent user experience across all cashup workflows
- Improved maintainability and code organization
Backend changes (Koha::Cash::Register):
- start_cashup(): Creates CASHUP_START action to begin session
- cashup_in_progress(): Detects active cashup sessions
- add_cashup(): Enhanced with two-phase completion support
- outstanding_accountlines(): Respects cashup session boundaries
Frontend changes:
- Interactive modals for starting and completing cashups
- Real-time calculation displays
- Support for reconciliation with actual amounts
- Batch operations UI for multiple registers
- Responsive error messaging and validation feedback
Test plan:
1. Apply patches and restart services
2. Run prove t/db_dependent/Koha/Cash/Register.t
3. Run prove t/db_dependent/Koha/Cash/Register/Cashup.t
4. Test single register workflows:
- Start cashup, add transactions, verify blocked
- Complete cashup with reconciliation amounts
- Test with positive, negative, and zero amounts
5. Test multi-register workflows:
- Select multiple registers on registers page
- Perform batch cashup operations
- Verify each register processes correctly
6. Test error conditions:
- Attempt cashup with no transactions
- Attempt cashup with invalid amounts
- Verify clear error messages displayed
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>
Bug 42391 removed the warn from Koha::Object::store when it moved
exception translation to the exception_action hook. This lost useful
debugging information from the Plack error logs.
This patch restores the warning by adding a try/catch in store() that
stringifies and warns the exception before rethrowing. The warning now
contains the translated Koha exception message (e.g. 'Broken FK
constraint') rather than the raw DBI error, since translation happens
before the exception reaches store().
Note: delete() does not warn. Ideally the logging would live in the
ExceptionMapper itself (via an injected logger), covering all code paths
uniformly. That is a larger refactor tracked separately.
Tests that assert on these warnings are updated to match the new
message format.
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/Object.t \
t/db_dependent/Koha/Objects.t \
t/db_dependent/Koha/Schema/ExceptionMapper.t \
t/db_dependent/Koha/Item.t \
t/db_dependent/Koha/Suggestions.t \
t/db_dependent/Koha/Account.t \
t/db_dependent/Koha/ApiKey.t \
t/db_dependent/Koha/Checkouts/ReturnClaim.t \
t/db_dependent/Circulation/MarkIssueReturned.t \
t/db_dependent/Suggestions.t \
t/db_dependent/Biblio.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>
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 Koha::Schema::Util:: namespace adds unnecessary nesting for a
single-purpose class. This follow-up moves it to
Koha::Schema::ExceptionMapper, which sits alongside Koha::Schema
itself — a better fit given it's tightly coupled to the Schema's
exception_action hook.
Changes:
- Koha::Schema::Util::ExceptionTranslator → Koha::Schema::ExceptionMapper
- Update Koha::Schema to use the new module
- Move test file accordingly
- Remove now-empty Koha/Schema/Util/ directory
Signed-off-by: David Nind <david@davidnind.com>
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>
Extend the ExceptionTranslator to handle two additional MySQL error
patterns:
- NOT NULL violations ('Column X cannot be null' and 'X doesn't have
a default value') → Koha::Exceptions::Object::NotNull
- Not in database errors → Koha::Exceptions::Object::NotInStorage
Add Koha::Exceptions::Object::NotNull exception class with a property
field identifying the offending column.
Add Koha::Exceptions::Object::UnhandledDBError as a catch-all for DBI
errors that don't match any known pattern. This closes the gap where
unmatched MySQL errors could leak as DBIx::Class::Exception to callers
(e.g. templates that no longer catch that type).
The exception_action handler now distinguishes three cases:
- Koha exception objects (from txn_do re-throws): passed through via
'die $msg' to preserve the original exception type. Without this,
txn_do's rollback-and-rethrow would stringify the Koha exception
and wrap it in UnhandledDBError, losing the specific type.
- DBI errors (prefixed 'DBI Exception:'): translated if possible,
otherwise wrapped in UnhandledDBError.
- DBIC internal errors (e.g. 'Not in database'): translated if
possible, otherwise thrown as DBIx::Class::Exception.
Note: NotInStorage translation works for update operations but not for
delete-on-detached-Row where DBIC bypasses the Schema's exception_action.
Those cases remain DBIx::Class::Exception.
Update tests:
- OAIServers.t, Z3950Servers.t: assert NotNull with property check
- Hold.t: assert NotNull, remove warn suppression hack
- Objects.t: update-not-in-storage cases assert NotInStorage
- Upload.t: delete-not-in-storage stays DBIx::Class::Exception
Signed-off-by: David Nind <david@davidnind.com>
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>
DBIx::Class provides an exception_action hook that intercepts every
throw_exception call before it becomes a DBIx::Class::Exception. By
installing this hook on Koha::Schema via a connection() override, all
database exceptions are automatically translated into Koha exceptions
at the source.
This eliminates the need for translate_exception catch blocks in every
caller:
- Remove try/catch from Koha::Object::store
- Remove try/catch from Koha::Object::delete
- Remove try/catch from Koha::Item::add_to_bundle
- Remove translate_exception convenience method from Koha::Schema
- Remove safe_do (already removed in previous follow-up)
ExceptionTranslator is refactored to work on plain error message strings
instead of DBIx::Class::Exception objects, since exception_action
receives the message before wrapping.
Unmatched errors fall through to DBIx::Class::Exception->throw() as
the default, preserving backward compatibility.
The warn $_->{msg} that was in Object::store is no longer needed since
exceptions are translated before they propagate. Tests that asserted on
that warning (via warning_like) are updated to use throws_ok directly.
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/Schema/Util/ExceptionTranslator.t
=> SUCCESS: Tests pass!
k$ prove t/db_dependent/Koha/Object.t
=> SUCCESS: Tests pass!
k$ prove t/db_dependent/Koha/Item.t
=> SUCCESS: Tests pass!
k$ prove t/db_dependent/Koha/Patron/Modifications.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: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
In MySQL8 the order on the PK is not preserved. We need to explicitely
pass it.
Test plan:
export DB_IMAGE=mysql:8.0;
then startup ktd
% prove t/db_dependent/api/v1/biblios.t
should pass with this patch
Without this patch it failed with:
13:00:29 # Failed test 'exact match for JSON Pointer "/4/item_id"'
13:00:29 # at t/db_dependent/api/v1/biblios.t line 276.
13:00:34 # got: '3420'
13:00:36 # expected: '3417'
13:00:37
13:00:38 # Failed test 'Withdrawn comes after lost'
13:00:57 # at t/db_dependent/api/v1/biblios.t line 276.
13:00:59 # got: '3417'
13:01:04 # expected: '3420'
13:01:05 # Looks like you failed 2 tests of 8.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Jan Kissig <bibliothek@th-wildau.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
check_for_existing_matches() called Koha::Patrons->search(undef) when
PatronDuplicateMatchingAddFields left no usable field to match on,
which matches every patron instead of none.
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>
On the holdings table the filter by status=recalls is not working correctly.
Test plan:
Add a recall on an item of a bibliographic record that has several
items.
Filter the items table by status=recall
=> Without this patch all items are displayed
=> With this patch applied only the item with a recall is displayed
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Aleisha Amohia <aleisha@catalyst.net.nz>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
When there is a filter with the "status" param, we add a validation step
to secure the use of such a filter.
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 allows _status to be in a nested structure.
Cannot be combined in all the possible ways, but at least it should fix
the UI.
Test plan:
Use the filters on the holdings table and confirm that you can combine
the status with other filters.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch updates offline circulation and ILL checkouts to properly
record their nature in circulation logs as forced transactions.
Offline circulation (process_koc.pl):
- Offline transactions have already occurred without access to normal
circulation checks
- Now marks these as forced with 'OFFLINE_CIRCULATION' override code
- This helps identify historical transactions that bypassed checks
ILL checkouts (Koha/ILL/Request.pm):
- ILL checkouts are system-initiated and may bypass certain restrictions
by design
- Now marks these as forced with 'ILL_CHECKOUT' override code
- This helps track and report on ILL circulation activity separately
Display updates (action-logs.inc):
- Added translation strings for both new override codes:
- OFFLINE_CIRCULATION: "Offline circulation checkout"
- ILL_CHECKOUT: "ILL system checkout"
- These will now display properly in Tools > Log viewer
This ensures these special checkout types are properly identified in
reporting and analysis, making it easier to understand the context
of historical transactions.
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
InstallPlugins forces every discovered plugin class to be deleted and
re-required from disk on every call, so plugin_methods reflects the
.pm file as it currently exists rather than whatever was already
loaded into a long-lived Plack worker. That deletion used
Symbol::delete_package, which clears the target package's whole
symbol table -- including any already-loaded packages nested under it
(e.g. a REST API controller a plugin bundles alongside itself, such as
Koha/Plugin/Foo/Controller.pm). Those nested classes keep their own
%INC entry, so they are never re-required afterwards and are left
permanently blanked out for the rest of the worker's life.
This was caught when running
`prove -r t/db_dependent/Koha/Plugins/ t/db_dependent/Koha/REST/Plugin/`
on top of the original patch. Tests fail several files that have nothing
to do with the bug being fixed (Biblio_and_Items_plugin_hooks.t,
Holds_hooks.t, TemplateIncludePathHook.t, REST/Plugin/PluginRoutes.t),
because Koha::Plugin::Test::Controller (a bundled test-fixture
controller living in t/lib/plugins/Koha/Plugin/Test/Controller.pm)
gets its symbol table wiped as collateral damage every time
Koha::Plugin::Test is reloaded.
_forget_plugin_class() replaces the delete_package call with the same
technique used by the CPAN module Class::Unload (not added as a
dependency here, just the technique): only clear the target class's
own symbols and reset its @ISA, explicitly leaving any nested package
entries in its symbol table alone. Module::Pluggable discovers bundled
nested classes as their own entries in the plugin list regardless, so
they still get reloaded correctly on their own turn.
I also annotate the no strict 'refs' block added by the previous commit
with a '## no critic' justification, as flagged by koha-qa.pl.
Test plan:
1. Confirm the bug exists without this patch:
a) git checkout <previous commit> (drop this commit, keeping the
first 3 commits of the bug applied)
b) Inside KTD:
prove -r t/db_dependent/Koha/Plugins/ t/db_dependent/Koha/REST/Plugin/
c) Observe failing files unrelated to this bug's own tests:
Biblio_and_Items_plugin_hooks.t, Holds_hooks.t,
TemplateIncludePathHook.t and REST/Plugin/PluginRoutes.t.
2. Confirm the fix:
a) Re-apply this commit
b) Re-run the same command from 1(b) -- all tests now pass
3. Confirm the original bug this branch set out to fix is still fixed,
both via the manual UI steps from the first commit's test plan
(install Batch Permission Modifier Plugin 1.3.1, upload 1.4.1 over
it via Tools > Plugins, confirm the renamed method takes effect
without restarting Plack) and via:
prove t/db_dependent/Koha/Plugins/Plugins.t
(see the 'InstallPlugins() reloads stale symbol tables tests'
subtest)
4. Confirm QA tooling is clean:
koha-qa.pl -c 4
=> PASS for Koha/Plugins.pm and all touched test files, no critic
or tidiness failures.
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
When a plugin is upgraded in place (by uploading a new version over
the old one), the list of methods Koha knows about for that plugin
can get out of date. For example, if a plugin used to have a "tool"
method and the new version renames it to "admin", the staff interface
can still show the old "Run tool" link, and clicking it produces an
error saying the plugin has no "tool" method.
This happens because the plugin upload runs inside a long-lived
Plack worker that has already loaded the old version of the plugin
in memory. When Koha scans the plugin to record its methods, Perl
has no reason to re-read the file from disk, so the scan sees the
old method list and stores that in the plugin_methods table. The
plugin code on disk is new, but the record of what methods it has
is stale.
This patch forces Koha to throw away the in-memory copy of each
plugin and reload it from disk before scanning, so the recorded
list of methods matches the plugin that is actually installed.
Test Plan:
1) Install the Batch Permission Modifier Plugin version 1.3.1
(this version exposes a "tool" method)
2) Go to Tools > Plugins and confirm the plugin row shows
"Run tool" in its actions menu
3) Upload Batch Permission Modifier Plugin version 1.4.1 over
the top of 1.3.1 (this version exposes "admin" instead of
"tool")
4) Reload the Plugins page
5) Note the plugin still shows "Run tool" in its actions menu
6) Click "Run tool"
7) Note the error: the plugin has no "tool" method!
8) Apply this patch
9) Restart all the things!
10) Uninstall the plugin
11) Repeat steps 1-4
12) Note that "Run tool" is gone and "Run admin tool" appears
in its place
13) Click "Run admin tool" and confirm it loads without error!
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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>
The FK-chain resolution added in the previous patch only handles ONE
level of nesting: it requires $parent_alias to be a direct relationship
of $main_source. This holds for every +count embed currently declared
in api/v1/swagger/ (all exactly one level deep, e.g.
biblio.items+count), but if that assumption is ever violated - a
deeper nested +count, a parent relation with a coderef (custom join)
condition, or no matching FK column name - the previous code silently
fell back to the pre-fix expression ($parent_alias.$fk_self), which can
reference a join alias that isn't available inside DBIC's limiting
subquery, reproducing the original 500 with no diagnostic.
This patch makes _build_count_subquery decline to build a subquery
(return undef) when the correlation can't be safely resolved, instead
of emitting SQL that may be broken. The caller already has an existing,
tested degradation path for "no DBIC relationship info" (prefetch the
association directly; to_api falls back to the Perl-level ->count call;
sorting on it is rejected with a clean 400) - unresolved nested
correlations now go through that same path rather than risking a 500.
Also:
- Extract the repeated "_resultset vs _result" result-source lookup
into a shared _result_source() helper (was duplicated in
dbic_merge_prefetch and _build_count_subquery).
- Document the one-level-deep limitation in _build_count_subquery's POD.
- Add a regression test pinning the 2-level-nested-+count case: it must
fall back to a plain prefetch (no COUNT subquery) rather than error
or emit unsafe SQL.
Test plan:
1. ktd --shell
2. prove t/Koha/REST/Plugin/Query.t t/db_dependent/Koha/Object.t \
t/db_dependent/Koha/REST/Plugin/Objects.t \
t/db_dependent/api/v1/acquisitions_orders.t \
t/db_dependent/api/v1/acquisitions_vendors.t \
t/db_dependent/api/v1/ill_requests.t \
t/db_dependent/api/v1/tickets.t \
t/db_dependent/api/v1/patrons.t
=> All pass, no regressions.
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
When nested +count embeds (e.g. biblio.items+count) are used alongside a
has_many prefetch (e.g. biblio.suggestions or the implicitly added
biblio.biblioitem) and pagination, DBIC's limiting subquery optimization
places +select scalar refs in the inner subquery where the parent join
alias (e.g. 'biblio') is not yet available.
This patch fixes _build_count_subquery to resolve the correlation back to
the main table (me) via the FK chain. Instead of generating:
(SELECT COUNT(*) FROM items
WHERE items.biblionumber = biblio.biblionumber)
It now generates:
(SELECT COUNT(*) FROM items
WHERE items.biblionumber = me.biblionumber)
This works because me.biblionumber IS the FK to biblio, holding the same
value. The main table columns are always available regardless of DBIC's
query structure.
Changes:
- Add $main_source parameter to _build_count_subquery for FK resolution
- Thread main_source through _parse_prefetch and dbic_merge_prefetch
- When parent_alias != 'me', resolve through parent relationship condition
- Update Query.t assertion to match the new correlation expression
Test plan:
1. Apply the regression tests from the previous commit
2. Run:
$ ktd --shell
k$ prove t/db_dependent/api/v1/acquisitions_orders.t
=> FAIL: The Bug 42995 subtest fails with 500 errors
3. Apply this patch, repeat from step 2
k$ prove t/Koha/REST/Plugin/Query.t \
t/db_dependent/Koha/Object.t \
t/db_dependent/Koha/REST/Plugin/Objects.t \
t/db_dependent/api/v1/acquisitions_orders.t \
t/db_dependent/api/v1/acquisitions_vendors.t \
t/db_dependent/api/v1/ill_requests.t \
t/db_dependent/api/v1/tickets.t \
t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests pass!
4. 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>
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>
Adds DBIC relationship aliases so that +count embeds advertised in
the API specs are backed by actual database relationships, making
them sortable via the SQL COUNT subquery infrastructure.
New aliases:
- Borrower: checkouts (issues)
- Aqbookseller: baskets (aqbaskets), invoices (aqinvoices)
- Ticket: updates (ticket_updates)
- ErmEholdingsPackage: resources (erm_eholdings_resources)
- Biblio: holds (reserves)
Koha methods are updated to use the new relationship names directly,
following the one-liner style: Koha::Foo->_new_from_dbic( scalar ... )
Note: checkouts on Borrower and subscriptions on Aqbookseller already
had matching DBIC relationships. comments on Illrequest, requests on
IllBatch, and items on Biblio also already existed.
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/Koha/REST/Plugin/Query.t \
t/db_dependent/api/v1/patrons.t \
t/db_dependent/api/v1/acquisitions_vendors.t \
t/db_dependent/Koha/REST/Plugin/Objects.t \
t/db_dependent/Koha/Biblio.t
=> SUCCESS: All tests pass
3. Verify sorting works:
GET /api/v1/patrons
x-koha-embed: checkouts+count
_order_by: -me.checkouts_count
=> SUCCESS: Patrons sorted by checkout count
4. Sign off :-D
Assisted-by: Sonnet 4.6 (Anthropic)
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Currently, +count embeds (e.g. claims+count) are computed in Perl via
$object->$relation->count during to_api serialization. This means they
cannot be used in _order_by and each count triggers a separate SQL query
per row (N+1).
This patch makes dbic_merge_prefetch detect +count embeds and, when a
matching DBIC relationship exists, inject a correlated COUNT subquery
via +select/+as instead of adding a prefetch.
The order_by entries referencing count aliases are fixed up to strip the
me. prefix, since +as aliases are virtual columns not bound to a table.
In to_api, the is_count handler now checks for the pre-computed column
via get_column before falling back to the Perl-level ->count call. This
preserves backward compatibility for Koha-level methods that don't have
a corresponding DBIC relationship.
Test plan:
1. Apply patches
2. Run:
$ ktd --shell
k$ prove t/Koha/REST/Plugin/Query.t \
t/db_dependent/Koha/Object.t \
t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Apply encode_text function to all text fields in EDIfact Order messages
to properly escape special characters (?, ', :, +) that could break
message parsing. Previously only bibliographic description fields were
escaped, but GIR segments containing collection codes, branch codes,
item types, call numbers, and other data were vulnerable.
Test plan:
- Verify all existing tests pass
- Added comprehensive tests for special character escaping in GIR segments
- Confirmed that fields containing ?, ', :, + are properly escaped
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: Laura_Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Baptiste Wojtkowski
Martin Renvoize
Nick Clemens
Pedro Amorim
Cameron E. Tidd
Andrii Nugged
Paul Derscheid
Brendan Lawlor
Lucas Gass
Jonathan Druart
Olivia Reynolds
Emily Lamancusa
Kyle M Hall
Matt Blenkinsop
Tomás Cohen Arazi
Aleisha Amohia