Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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>
Note that t/Makefile.t and xt/check_makefile.t were doing the same test.
Merging them.
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>
$Test::Strict::TEST_STRICT = 0 is only set for all_perl_files_ok
It's no longer used since bug 42126
Test plan:
prove t/db_dependent/00-strict.t
should still return green
Remove a use Modern::Perl for mainpage.pl
=> Should still return green
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>
Bug 40866 introduced JSON logging for HOLDS CREATE but stored null for
'confirmations' and 'forced' when no overrides were present. This patch
mirrors the earlier change to CIRCULATION ISSUE by defaulting them to
empty arrays, keeping the two log streams structurally identical and
simplifying reporting queries.
Changes:
C4/Reserves.pm:
- Default confirmations/forced to [] in the AddReserve log payload.
t/db_dependent/Reserves.t:
- Update the Bug 40866 subtest to assert the empty-array defaults on
normal holds, and fix a 'my $log_data' redeclaration.
tools/viewlog.pl:
- Extend the CSV export JSON-decode block to also cover HOLDS logs,
guarding the item lookup on itemnumber being present (HOLDS payloads
may lack one) and filling the CSV item columns from the decoded
payload. The legacy bare-itemnumber fallback is restricted to
CIRCULATION so a legacy HOLDS hold_id is not mistaken for an
itemnumber.
koha-tmpl/intranet-tmpl/prog/js/viewlog.js:
- Extend the info column override rendering (renderCircInfo) to HOLDS
entries so forced/confirmations arrays display translated, exactly as
they do for CIRCULATION. The log viewer moved to an ajax datatable in
bug 29016, so this replaces the earlier json_found change to
viewlog.pl/viewlog.tt.
misc/maintenance/migrate_action_logs_to_json.pl:
- Extended to handle legacy pre-Bug 40866 HOLDS CREATE logs where info
is a bare hold_id, recovering branchcode / biblionumber / itemnumber
from old_reserves / reserves.
- Uses the same keyset-pagination pattern as the circulation loop so
rows are not skipped under --commit.
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>
Previously, CIRCULATION ISSUE logs would store just the itemnumber when
there were no confirmations or forced overrides, but would store a full
JSON structure when overrides were present. This inconsistency makes
reporting difficult as queries need to handle both formats.
This patch changes AddIssue to always store logs in JSON format,
matching the pattern already used in AddRenewal. The JSON structure
now consistently includes:
- issue: issue_id
- branchcode: branchcode
- itemnumber: itemnumber
- confirmations: array of confirmation codes (empty array if none)
- forced: array of forced override codes (empty array if none)
Benefits:
- Consistent data format simplifies reporting and analysis
- Matches existing RENEWAL logging pattern
- Display code (tools/viewlog.pl) already handles both formats,
so backward compatibility is maintained
Test plan:
1. prove t/db_dependent/Circulation.t
2. Perform a normal checkout - verify action log contains JSON
3. Perform a checkout with overrides - verify confirmations/forced
are populated in the JSON
4. Check Tools > Log viewer - both types should display correctly
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>
Example: supplier_report on the basket page
Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
- Modify column-visibility.dt event handler to store the visibility
preference of the user
- Improve _dt_force_visibility() to correctly pick the correctly visibility value depending
on visibility_condition, is_hidden, and the user preference
- Add _dt_get_expected_column_visibility() to guess what's the expected visibility
- Force boolean true/false (vs 1/0) using !!
- Rely on a global variable kohaTableUserChoices to store the user
choice. It will be part of the state and store in localStorage.
Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
So we protect other calls
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The previous two commits on this bug fixed two ordering assumptions in
the RETRY loop of _allocateWithTransportCostMatrix(): resetting $inf at
the start of each attempt, and splicing unallocated task indices out of
@requests in descending order (removing a lower index first would shift
the position of any higher index still waiting to be removed).
This is a third instance of the same underlying problem, a few lines
earlier in the loop that compacts the working cost matrix $m before
that splice runs. @unallocated is built by iterating over agents (item
rows), pushing the column index Munkres assigned to each row that
turned out infeasible - and Munkres' row-to-column assignment is a
permutation, not a monotonic function of the row index, so
@unallocated is no more reliably sorted here than it was for the
splice.
That compaction loop scans matrix columns in ascending order and
advances a single pointer into @unallocated, expecting to meet each
entry in that same ascending order. When @unallocated arrives out of
order, the pointer never catches up: columns that should have been
dropped are left in place, and the columns after them are never
shifted left to fill the gap. From that point on, $m no longer lines
up with @requests, which *is* correctly reduced by the splice - so on
the next retry pass a hold's transport cost gets evaluated using a
different hold's matrix row. Depending on the exact misalignment, this
can silently drop a fillable hold from the queue while a genuinely
available item sits unused, or fill a hold via the wrong-cost item.
It didn't show up in the "many holds" regression test added earlier on
this bug, because that test's transport cost matrix is uniform (every
disabled pair costs the same); this defect only surfaces once the
columns competing for a scarce item have distinct costs.
Sort @unallocated once, ascending, and use that copy for the
compaction pass; reverse it for the splice, which needs descending
order.
Test plan:
1. Run the new t/db_dependent/HoldsQueue/RetryAllocationOrdering.t -
it calls _allocateWithTransportCostMatrix() directly with a small
hand-built scarce transport cost matrix and a pinned srand() (the
specific item-to-branch permutation needed to trigger the bug
depends on the shuffle() inside the routine). Reverting just this
patch makes 3 of its 6 subtests fail: the L2 pickup hold is dropped
even though an available item could have filled it.
2. Run t/db_dependent/HoldsQueue/TransportCostOptimizations.t to
confirm no regression in the existing retry-loop coverage.
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
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>
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>
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>
[FAIL] t/db_dependent/Koha/Plugins/C3.t
FAIL file_permissions
File must have the exec flag
FAIL test_no_warnings
'use Test::NoWarnings' must be present in this test file.
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
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>
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>
The tests were lacking a 'real' scenario proving the
sorting on a _counts works at the DB level
prove t/Koha/REST/Plugin/Query.t
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>
Add tests to verify that +count embeds produce +select/+as attributes
instead of a prefetch, enabling SQL-level sorting by count columns.
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>
As with guided reports through the admin interface, this patch switches
EmailReport to use the same function for processing templates. This
gives patron emails sent via cronjobs access to the same variables as
other report locations.
Test plan:
1. Under Tools > Notices and Slips, create a new notice in the Reports
module. Choose a code and a name, and under Email assign a subject
and the following body:
Date: [% today %]
2. Under Reports, create a new report from SQL with a name and the
following query:
SELECT 1 AS borrowernumber, 'to@example.com' AS email
3. Run the following command from the base of Koha, replacing CODE with
the notice code created earlier, and ID with the report ID created
earlier:
misc/cronjobs/patron_emailer.pl -module report -from from@example.com -notice CODE -report ID
4. Verify that the report runs successfully, and the console output
includes "Date:" on a line with nothing following it.
5. Apply patch.
6. Run the following command from the base of Koha, replacing CODE with
the notice code created earlier, and ID with the report ID created
earlier:
misc/cronjobs/patron_emailer.pl -module report -from from@example.com -notice CODE -report ID
7. Verify that the report runs successfully, and the console output
includes "Date:" on a line with the current time following it.
8. Run `prove t/db_dependent/Reports/Guided.t`, and verify it passes.
Sponsored-by: Open Fifth
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>
Verify that the SMS fallback in ModSuggestion() respects EmailFieldPrimary
and EmailFieldPrecedence: a patron with smsalertnumber and an emailpro
address (but no borrowers.email) must receive the notice by email, not SMS.
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>
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>
The booking date picker tests fake "today" with cy.clock and create bookings on
hardcoded June 2026 dates. The modal fetches existing bookings from from
the api and that query uses the real current time, not the fake one in the browser.
Once the real date passes the hardcoded dates, the server stops returning those bookings (
they are now in the past ), so the modal sees no conflicts and assertions such as "the date
should have the leadDisable class" fail.
This patch anchors each test's "today" to the start of next month based on the current
date so the bookings always stay in the future and are returned no
matter when the test runs.
The 'any item' test also sets cy.clock before its second page reload
because the freeze does not survive the reload.
Test Plan:
1) On main run:
yarn cypress run --spec t/cypress/integration/Circulation/bookingsModalDatePicker_spec.ts
2) Note the test fails
3) Apply this patch
4) Run the command again
5) Note all tests pass!
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>
SIP2 accounts/institutions are stored in the database and the SIP server is supposed to reload its configuration when they change.
In practice, after adding or editing a SIP2 account, most SIP worker processes keep serving the old configuration (e.g. new accounts get "SIP access not authorized") until the SIP server is restarted ( I tested multiple times over 30 minutes ).
There seem to be two issues:
* L1 cache flushed too late. process_request() calls _config_up_to_date() before Koha::Caches->flush_L1_caches(). The SIP parent reads sip2_resource_last_modified into its own L1 cache at startup and never flushes it. Every worker inherits that stale L1, so a worker's first connection compares against the startup-era value instead of the current shared value.
* sip2_config_read_timestamp is stored in the shared (memcached) cache, but the configuration it tracks is per-process. When one worker reloads the config from the database, it bumps the shared timestamp, so every other worker then evaluates as "up to date" and never reloads its own stale in-memory config.
Test Plan:
1) Configure the SIP server with several worker processes (e.g. set
min_servers and max_servers to 5) and confirm an existing SIP2
account works
2) Add a new SIP2 account in the staff interface
3) Without restarting the SIP server, test the new account with misc/sip_cli_emulator.pl
4) Note the login fails
5) Apply this patch
6) Restart all the things!
7) Add another new SIP2 account
8) Without restarting the SIP server test this new account with sip_cli_emulator.pl
9) Note that the new account works!
10) prove t/db_dependent/SIP/SIPServer.t
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Some item statuses can correspond to multiple authorised values: Not
for loan, Withdrawn, Lost and Damaged. Until now, only a single entry
could be selected, and filtering applied to all values at once.
This change introduces finer-grained filtering by exposing each
authorised value individually in the filter.
Test plan:
Assign multiple authorised values to items on a bibliographic record
for the four affected attributes: Not for loan, Withdrawn, Lost and
Damaged. Use the "Status" filter and verify that each value behaves as
expected.
Display note:
The current dropdown presentation is not ideal.
Suggestions for improvement are welcome, although avoiding optgroup
would be preferable to keep the scope small.
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
lib/fontawesome/css/fontawesome.min.css was used as an example for this
test, let's use datatables.min.css
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Unit tests
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
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>
To test:
Test the cronjob purge_suggestions.pl
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>
ConnectDuplicate does not appear to be used anywhere.
No test plan to provide here.
git grep connectDuplicate should return nothing
Since there is no cud-, it would at least have been broken for a year
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>
MARC21 4xx/5xx $w is a control subfield with up to four positions;
only position 0 encodes the hierarchical relationship. The previous
regex required the whole subfield to equal g or h, so values such as
"g " or "gnna" would fall into the middle bucket and lose the
broader/narrower distinction.
This case is hypothetical and likely rare in practice (most catalogs
store a single character in $w), but the correctness improvement still
makes sense and aligns with how BuildSummary reads the same subfield
elsewhere via substr($w, 0, 1).
To test:
- prove -v t/db_dependent/AuthoritiesMarc.t
- Verify BuildSummary/_marc21_sort_hierarchy_alpha subtest passes,
including the new multi-position $w case.
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Resolve critic warn:
"return" statement followed by "sort" at line 1238, column 5. Behavior is undefined if called in scalar context. (Severity: 5)
Tidied with lastest perltidy version.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Test plan:
Run t/db_dependent/AuthoritiesMarc.t
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch adds autoMemberNumValue to the Patrons system preferences
page immediately below autoMemberNum, so superlibrarians can view and
correct a runaway counter without needing vendor support.
The entry carries a caution note explaining:
- the value is managed automatically and should not normally be changed
- Koha generates the next cardnumber from max(counter, live_db_max),
so lowering the counter only has effect once the patron responsible
for the inflated number has been deleted or had their cardnumber
corrected first
- the risk of setting it too low: potential re-issuance of a previously
used cardnumber, which may grant a new patron access to third-party
services linked to the original holder
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>
Add new tests to assert the usage of the new ILL_BATCHES action_logs modulename.
Update existing tests to utilize real log_something instead of mocking 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>
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>
Test plan:
1. Navigate to Reports > Create from SQL.
2. Create a new report with a name and the following SQL query:
SELECT 'test', 1, '2'
3. Run the report.
4. Under the Download dropdown, select "Open Document Spreadsheet".
5. Verify a ".ods" file is downloaded.
6. Open the downloaded spreadsheet.
7. Verify the two number-like columns are handled as such. This depends
on your choice of spreadsheet software:
- In Excel, the numbers should not have a leading single quote in the
edit-bar when selected.
- In Google Sheets, the numbers should be right-aligned, and
selecting both of them should display their sum in the bottom
toolbar.
8. Run `prove t/Koha/Util/OpenDocument.t` and verify all tests were
successful.
Sponsored-by: Open Fifth
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>
To test:
1. prove t/db_dependent/MarcModificationTemplates.t
Observe test failing with the following error
t/db_dependent/MarcModificationTemplates.t .. 1/135 Control fields (generally, just tags below 010) do not have subfields, use data() at /home/koha/Koha/Koha/SimpleMARC.pm line 490.
Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Before this patch, control fields were not modifiable using this subroutine.
Control fields use character positions instead of subfields. When modifying
control fields, this patch makes the subroutine's parameter "subfield" to indicate
a character position. An example
Koha::SimpleMARC::update_field({
record => $record,
field => '008',
subfield => '2',
values => [09],
});
changes field 008 starting from character position 2 to contain values "09".
This replaces char pos 2 with "0", and char pos 3 with "9".
To test:
1. prove t/SimpleMARC.t
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Add explicit `use JSON::PP ()` to Elasticsearch.pm and the test file
to make the dependency on JSON::PP::false visible rather than relying
on YAML::XS loading it as a side effect.
Extend the get_elasticsearch_mappings() test subtest to assert that
date_detection and numeric_detection are disabled at the mapping root.
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Jonathan Druart
Baptiste Wojtkowski
Martin Renvoize
Nick Clemens
Kyle M Hall
Tomás Cohen Arazi
Lisette Scheer
Pedro Amorim
Olivia Reynolds
Janusz Kaczmarek
Paul Derscheid
Marcel de Rooy
Lari Taskula