This follow-up fixes the formatting issue reported by xt/perltidy.t.
Test plan:
1. Run prove xt/perltidy.t
2. Confirm it passes.
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Add two test cases covering reports with no borrowernumber (NULL owner):
a patron without edit_all_reports should be denied, one with the
permission should succeed. The POD documents this case but it was
not previously exercised.
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
- Add null check around Koha::Patrons->find() in guided_reports.pl
before calling ->has_permission, matching the defensive pattern
already used in Koha::Report::check_edit_permission
- Add missing description to Koha::Exceptions::Report base class
for consistency with its sub-exceptions
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Prevents possible template bugs from allowing editing
of reports by others without the proper permissions.
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Test plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Restart all the things!
4) Create 2 users with create_reports and not edit_all_reports
5) Create reports with each
6) Note that each cannot edit the others reports
7) Create a user with create_reports and edit_all_reports
8) Note the this user can edit reports from the previous two users
Edit button placements to look for:
* Reports table row
* Actions menu
* Toolbar
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Something very messy happened during the last rebase before push.
Sorry!
I've restored the original version:
git rebase -i HEAD~258
Edit commit b02ca1270d and remove the next one (Tidy)
perl misc/devel/tidy.pl $(git diff --name-only HEAD~1)
git commit -a
git rebase --continue
Luckily there was no conflict!
I then picked the version of those files and here is the commit.
Use this to confirm:
`git show -w ad694a3411e677fcea96f315758793fd1fe0f203`
You should see changes that are not related to tidiness
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The original _GetCircControlBranch tests covered the case where CircControl
is PickupLibrary but no userenv branch is available, in which case the
resolution falls through to the ItemHomeLibrary behaviour. That coverage was
lost when the tests moved to t/db_dependent/Koha/Policy/Circulation.t, since
the PickupLibrary subtest always mocks a userenv.
This restores coverage for the fall-through path: with CircControl set to
PickupLibrary, no userenv, and no pickup_library_id, the controlling library
is resolved from the item's home library.
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/Policy/Circulation.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
(cherry picked from commit 6a13c6e5066384e59e52f0d48f5a430958671ef2)
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Removes the deprecated _GetCircControlBranch function and its export
from C4::Circulation. Removes the corresponding tests from
Circulation.t (now covered by t/db_dependent/Koha/Policy/Circulation.t).
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Circulation.t \
t/db_dependent/Koha/Policy/Circulation.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Signed-off-by: Alex Carver [Acerock7] <alex@rcls.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
(cherry picked from commit 8e784622815a2db2ce6f558987d7c0b0951ceec0)
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
# Conflicts:
# C4/Circulation.pm
# t/db_dependent/Circulation.t
Adds Koha::Policy::Circulation->circ_control_library($item, $patron)
class method that resolves which library controls circulation rules,
based on the CircControl and HomeOrHoldingBranch system preferences.
This follows the same pattern as Koha::Policy::Holds->holds_control_library.
An optional pickup_library_id parameter allows callers in API/daemon
contexts to pass the library explicitly instead of relying on userenv.
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/Policy/Circulation.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Signed-off-by: Alex Carver [Acerock7] <alex@rcls.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
_get_data_and_patron() falls back to Mojo::Parameters form-data
parsing whenever $tx->res->json fails to decode the userinfo
response body. That branch was untested: all existing userinfo_url
subtests mock json() to return a hashref, so the fallback path was
never actually exercised.
This adds a subtest that mocks a genuine url-encoded response body
(json() returning undef) and confirms the fallback still maps data
and finds the matching patron correctly.
Test plan:
1. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/Auth/Client/OAuth.t
=> SUCCESS: All 6 subtests pass, including the new fallback-path test
2. Sign off
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch simplifies the userinfo response parsing by relying on
Mojo::Message::Response->json which decodes JSON regardless of
Content-Type header parameters. If JSON decoding fails (returns undef),
it falls back to URL-encoded form data parsing.
This fixes integration with APIs that return extra Content-Type
parameters (e.g. Microsoft Graph returns odata.metadata,
odata.streaming, and IEEE754Compatible alongside charset) which
previously caused the strict regex to fail and silently misparse
the response as form data.
Test plan:
1. Apply the regression test (previous patch)
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/Auth/Client/OAuth.t
=> FAIL: Subtest fails - Content-Type with extra params not recognized
3. Apply this patch
4. Repeat step 2
=> SUCCESS: All tests pass
5. 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 a regression test demonstrating that the OAuth
userinfo_url response parsing fails when the Content-Type header
contains extra parameters beyond charset (e.g. Microsoft Graph API
returns application/json;odata.metadata=minimal;odata.streaming=true).
The strict regex in _get_data_and_patron only matches Content-Type
values with an optional ;charset=... suffix, causing valid JSON
responses to be silently misinterpreted as URL-encoded form data.
Test plan:
1. Apply this patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/Auth/Client/OAuth.t
=> FAIL: New subtest fails - email is undef, patron not found
3. Sign off :-D
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Previously, the system preference
PatronSelfRegistrationBorrowerUnwantedField was used to remove hidden
fields regardless of whether the operation was self-registration or
self-modification. This patch uses the appropriate set of hidden fields
depending on the current operation (create/update).
Test plan:
1. Set OPACPatronDetails to "Allow".
2. Ensure PatronSelfModificationBorrowerUnwantedField is empty.
3. Set PatronSelfRegistrationBorrowerUnwantedField to exclude
Title (towards the bottom right of the modal).
4. Log into the OPAC, and navigate to Your account > Personal details.
5. Change your title and submit the request.
6. Verify you see the notice "No changes were made." at the top of the
page.
7. Apply patch.
8. Change your title again and submit the request.
9. Verify the request is submitted successfully.
10. Return to the personal details screen, but don't submit.
11. Set PatronSelfModificationBorrowerUnwantedField to exclude Title.
12. On the preloaded personal details screen, update the title and
submit.
13. Verify you see the notice "No changes were made." at the top of the
page.
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>
Before the refactoring in this bug, redirect_if_opac_suppressed was
intended to be called after checking OpacSuppression was enabled. Now
it performs this check internally, so to adapt these unit tests
correspondingly, we mock OpacSuppression to be enabled, as was the
implied state before.
Test plan:
1. Prove t/db_dependent/Output.t and verify all tests pass.
Sponsored-by: Open Fifth
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
- cleanup_database.pl: rewrite DeleteSpecialHolidays to purge the new
library_single_closures and library_closure_exceptions tables via their
Koha::Objects classes (was querying the dropped special_holidays table).
- kohastructure.sql: fix the comment header above library_repeating_closures
that still referred to repeatable_holidays.
- C4::Overdues: drop the dead GetSpecialHolidays / GetRepeatableHolidays /
GetWdayFromItemnumber routines (unreferenced, exported nowhere) that still
queried the dropped special_holidays / repeatable_holidays tables, along
with the now-unused Date::Calc and Date::Manip imports.
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
t/db_dependent/Holidays.t was a legacy file whose coverage is now
provided by the two dedicated calendar test files. Its calendar
behaviour assertions (is_holiday for weekly/day-month/single holidays)
are already exercised by t/db_dependent/Calendar.t and
t/db_dependent/Koha/Library/Calendar.t. The remaining unique cases
queried the special_holidays / repeatable_holidays tables directly,
which this bug drops during the migration to the
Koha::Library::Calendar::*Closure schema, so the file could no longer
run.
This commit folds the unique coverage into
t/db_dependent/Koha/Library/Calendar.t, rewritten against the new
closure objects/API (no raw legacy-table SQL):
- is_holiday DST edge case (America/Santiago days_between)
- next_open_days / prev_open_days throwing NoOpenDays when the library
is never open, and finding an open day created via an exception
- copy_to completeness: repeating-closure copy, exception copy, and
exclusion of past-dated single closures and exceptions, plus the
no-duplication guarantee for all four closure types
and then deletes t/db_dependent/Holidays.t.
Test plan:
- prove t/db_dependent/Koha/Library/Calendar.t
- prove t/db_dependent/Calendar.t
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The historical 16.12.00.032 upgrade step (Bug 12063) instantiated
Koha::Calendar to push reserves.expirationdate forward by the max
pickup delay in open days. Depending on a live application class from a
frozen upgrade step is fragile: when this bug renamed and removed
Koha::Calendar, the migration had to be edited to chase the new
namespace.
Frozen updatedatabase.pl steps should be self-contained and prefer
direct DB access. Reimplement the holiday-aware date advance inline,
reading the legacy repeatable_holidays / special_holidays tables (which
still exist at this point in the upgrade path) directly, with no
dependency on any Koha::* calendar class. This keeps the step working
regardless of later refactors and removes the calendar-class reference
from updatedatabase.pl entirely.
The weekly / day-month / single-closure logic and exception (open
override) handling mirror the historical C4::Calendar semantics.
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The koha-qa pod_coverage check flags 'sub _type has no pod coverage'
in Koha/Checkouts.pm. The POD block above the sub was headed '=head3
type' while the sub is named '_type', so the documentation was not
associated with the sub. Correct the heading to '=head3 _type' to
match, mirroring the adjacent object_class POD.
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Coverage gaps identified during QA review:
- add_single_closure/add_exception were only ever tested against a cold
cache; nothing proved they invalidate a cache already warmed with the
stale answer before the add.
- No test exercised a library with no closures at all.
- No test exercised a repeating (day/month) closure on 29 February across
a leap and a non-leap year.
Atomicupdate migration coverage is intentionally left out of scope.
Test plan:
1) prove t/db_dependent/Koha/Library/Calendar.t t/db_dependent/Calendar.t
t/db_dependent/Holidays.t t/db_dependent/HoldsQueue.t
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
add_weekly_closure/delete_weekly_closure and add_repeating_closure/
delete_repeating_closure returned $self without refreshing
$self->{weekly_closed_days}/{day_month_closed_days}, so a calendar object
would keep reporting the pre-change state to is_holiday (and everything
built on it - days_forward, next_open_days, etc.) until it was
re-instantiated. This is distinct from the _holidays Koha::Caches entry,
which single/exception closures already flush correctly on store/delete.
Call _init to rebuild both structures from the database after each
add/delete, so the same object reflects the change immediately.
Test plan:
1) prove t/db_dependent/Koha/Library/Calendar.t t/db_dependent/Calendar.t
t/db_dependent/Holidays.t t/db_dependent/HoldsQueue.t
t/db_dependent/Holds/WaitingReserves.t t/db_dependent/Koha/CurbsidePickups.t
t/db_dependent/Reserves/CancelExpiredReserves.t
t/db_dependent/api/v1/libraries.t
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
store() only flushes the _holidays cache on insert, on the assumption that
updates only ever touch title/description. That assumption held for every
current caller (ModSingleholiday/ModExceptionholiday only ever change those
two fields), but nothing enforced it - a future caller updating date in
place would silently desync the cache for up to its TTL, with no error.
Make the assumption explicit: reject an update that changes date, rather
than silently allowing it. Callers that need to move a closure/exception to
a different date must delete and re-add.
Test plan:
1) prove t/db_dependent/Koha/Library/Calendar.t t/db_dependent/Calendar.t
t/db_dependent/Holidays.t t/db_dependent/HoldsQueue.t
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
C4::Calendar::insert_week_day_holiday used to croak "Invalid weekday
$weekday" unless $weekday =~ m/^[0-6]$/. That guard was dropped when the
insert delegated to Koha::Library::Calendar::WeeklyClosure, and nothing
replaced it - an out-of-range or undef weekday now persists silently to
library_weekly_closures instead of failing at the API boundary.
Add the check to WeeklyClosure->store() rather than the legacy wrapper, so
every caller (C4::Calendar, Koha::Library::Calendar->add_weekly_closure,
and any future REST/admin caller) gets it, not just the old entry point.
Test plan:
1) prove t/db_dependent/Koha/Library/Calendar.t t/db_dependent/Calendar.t
t/db_dependent/Holidays.t t/db_dependent/HoldsQueue.t
t/db_dependent/Holds/WaitingReserves.t
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
insert_single_holiday and insert_exception_holiday delegate to
Koha::Library::Calendar::SingleClosure/Exception, but stopped updating
$self->{single_holidays}/{exception_holidays} on the C4::Calendar object
itself, unlike the sibling insert_week_day_holiday/insert_day_month_holiday
methods which still do. isHoliday() reads exactly those hashes, so a date
just inserted via insert_single_holiday/insert_exception_holiday would read
back as "not a holiday" for the rest of that object's lifetime - reachable
from tools/newHolidays.pl and tools/exceptionHolidays.pl, which loop over
several dates on one $calendar object, checking isHoliday() before each
insert to skip duplicates.
While adding a regression test, also found that _init() has the same class
of bug on the DB-load path: it builds the single/exception lookup keys from
`split(/-/, $row->{date})`, which keeps the zero-padding from the DATE
string (e.g. "06"), while isHoliday() numifies its lookup key (e.g. "6").
Any single/exception holiday with a single-digit month or day was therefore
invisible to isHoliday() even on a freshly reloaded C4::Calendar object.
Both call sites now numify year/month/day before building the hash key.
Test plan:
1) prove t/db_dependent/Holidays.t t/db_dependent/Calendar.t
t/db_dependent/Koha/Library/Calendar.t t/db_dependent/HoldsQueue.t
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The normalization commit dropped `repeatable_holidays` and
`special_holidays` but t/db_dependent/Holidays.t still referenced them
directly via raw SQL, so the test blew up on the first
`DELETE FROM special_holidays` with "Table doesn't exist". The earlier
namespace follow-up touched this file but only renamed Koha::Calendar
references, missing the SQL.
Replace the legacy table references with the four new normalized tables:
repeatable_holidays (weekday set) -> library_weekly_closures
repeatable_holidays (weekday NULL) -> library_repeating_closures
special_holidays (isexception=0) -> library_single_closures
special_holidays (isexception=1) -> library_closure_exceptions
- Two fixture-cleanup DELETEs now wipe all four new tables.
- The copy_to_branch 'no duplicates' check, previously two raw-SQL
queries against the old two tables, becomes four queries against the
new tables, keyed on each table's natural unique key
(library_id+weekday / library_id+day+month / library_id+date).
- Plan for the copy_to_branch subtest bumps from 8 to 10 to cover the
two extra duplicate-check assertions.
Test plan:
1. prove t/db_dependent/Holidays.t
2. All 15 top-level tests pass, including the 10 subtest assertions in
copy_to_branch.
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Koha::Calendar is, by construction, a per-library object — it croaks
without a branchcode argument and every method is branch-scoped. The
top-level namespace hides that. Move the whole tree under
Koha::Library::Calendar to match the actual semantics, the existing
/libraries/{library_id}/calendar REST routes, and the established
Koha::Library::* convention (Group, Hour, FloatLimit, OverDriveInfo).
This commit:
- renames the Koha::Calendar(::*) tree to Koha::Library::Calendar(::*)
- updates package declarations, POD and internal 'use' statements
within the moved tree
- updates the four DBIC Schema Result overrides (koha_object_class /
koha_objects_class) to the new namespace
- updates callers inside C4::* modules (Calendar, Circulation,
Reserves, Overdues, HoldsQueue), Koha::{Patron,Hold,Checkouts,
CurbsidePickup}, circ/returns.pl, the installer and cronjobs
- updates the 42310-owned tests (Calendar tests, HoldsQueue.t,
Holds/WaitingReserves.t, Holidays.t, Koha/CurbsidePickups.t,
Reserves/CancelExpiredReserves.t and the libraries.t CRUD block)
Koha::Library->calendar, the REST controllers and the 42311 tests
are updated in separate follow-up commits under their respective
bugs.
Test plan:
- prove t/db_dependent/Koha/Library/Calendar.t
- prove t/db_dependent/Calendar.t
- prove t/db_dependent/Holidays.t t/db_dependent/HoldsQueue.t
- prove t/db_dependent/Holds/WaitingReserves.t
- prove t/db_dependent/Reserves/CancelExpiredReserves.t
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Introduces a public Koha::Calendar->closed_dates_in_range($from_dt,
$to_dt) method that returns an arrayref of ISO date strings representing
every closed day in the inclusive range, honouring weekly, annual
repeating, single and exception (open-override) closures.
This replaces callers that were reaching into Koha::Calendar private
state (_holidays, weekly_closed_days, day_month_closed_days) to
reconstruct the same logic. A follow-up under Bug 42206 converts the
REST controller; further callers can be migrated incrementally.
Test plan:
1. prove t/db_dependent/Koha/Calendar.t
2. Verify the new 'closed_dates_in_range' subtest passes
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The new calendar tables add UNIQUE(library_id, weekday),
UNIQUE(library_id, day, month) and UNIQUE(library_id, date) constraints
that did not exist on repeatable_holidays / special_holidays, so the
INSERT IGNORE migration can silently drop legacy duplicate rows.
This patch compares source vs destination row counts for each of the
four migrations and emits a say_warning naming the constraint and
pointing the operator at their pre-upgrade backup so titles or
descriptions can be recovered if needed. Success lines now include
the migrated row count. DROP statements for the legacy tables are
guarded with TableExists and reported via say_success for
consistency with the rest of the atomicupdate.
Test plan:
1) Apply the update on a database containing legacy duplicates and
verify the warning is emitted with the correct count.
2) Apply on a database with no duplicates and verify no warning
appears.
3) Re-run the update and verify it is idempotent (no warnings, no
re-migration).
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Wrap the body in schema->txn_do so a failure partway through does not
leave the target library partially populated. Flush the target's
_holidays cache after the copy, and document the behaviour in POD:
weekly and repeating closures are always copied, but single closures
and exceptions are filtered to today-or-later dates by design.
Test plan:
1) prove t/db_dependent/Koha/Calendar.t
2) Confirm existing copy_to tests still pass.
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The four delete_*_closure / delete_exception methods in Koha::Calendar
were calling ResultSet->delete, which issues a single bulk SQL DELETE
and never calls the row-level delete() override. The cache-invalidation
hooks on Koha::Calendar::SingleClosure->delete and
Koha::Calendar::Exception->delete were therefore skipped when callers
went through $library->calendar->delete_single_closure(...) or
delete_exception(...), and the <library>_holidays cache kept reporting
deleted dates as closed until its TTL expired.
The underlying tables enforce UNIQUE constraints on (library_id, weekday),
(library_id, day, month) and (library_id, date), so each search can
match at most one row. Switch the four methods to ->search({...},
{ rows => 1 })->single followed by ->delete on the row, which both
makes the single-row expectation explicit and ensures the per-row
delete override runs.
A regression test is added to t/db_dependent/Koha/Calendar.t asserting
that the _holidays cache is cleared after delete_single_closure and
delete_exception.
Test plan:
1) prove t/db_dependent/Koha/Calendar.t
2) Confirm the new 'delete_*_closure clears _holidays cache' subtest
passes.
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
When using the new Koha::Calendar::* classes for writing the CRUD API
endpoints I noticed that in my attempt to keep C4::Calendar similar to what it
was, I ended up with cache invalidation not always done in the right place.
The holidays cache (keyed by {branchcode}_holidays) stores dates and their
open/closed status, not titles or descriptions. This means only inserts and
deletes need to invalidate it — metadata updates (title/description) do not.
Following the pattern established by Koha::AuthorisedValue and Koha::ItemType,
I added store() and delete() overrides to Koha::Calendar::SingleClosure and
Koha::Calendar::Exception:
- store() flushes the cache only on insert (!in_storage), not on updates,
since the cache doesn't store titles or descriptions.
- delete() always flushes, since removing a closure changes the cached set.
This allowed removing the now-redundant _clear_cache calls from
Koha::Calendar::add_single_closure, add_exception, delete_single_closure,
delete_exception, and from C4::Calendar::ModSingleholiday and
ModExceptionholiday.
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Add to Koha::Calendar:
add_weekly_closure, add_repeating_closure, add_single_closure,
add_exception, delete_weekly_closure, delete_repeating_closure,
delete_single_closure, delete_exception, copy_to, _clear_cache
C4::Calendar write methods now delegate to Koha::Calendar instead
of using raw DBI. No old table references remain in C4::Calendar.
Tests in t/db_dependent/Koha/Calendar.t cover all CRUD operations:
create, verify via is_holiday, delete, copy_to (including
idempotency).
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Replace all references to the dropped tables (RepeatableHoliday,
SpecialHoliday resultsets and raw DBI inserts) with Koha::Object
classes: Koha::Calendar::WeeklyClosure, RepeatingClosure,
SingleClosure, and Exception.
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Add koha_object_class/koha_objects_class overrides for the four new
Result classes backing the normalized calendar tables, below the
auto-generated marker.
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Regenerated via dbic --force after the library_weekly_closures,
library_repeating_closures, library_single_closures, and
library_closure_exceptions tables replaced repeatable_holidays and
special_holidays.
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Replace raw DBI queries in _init() and _holidays() with
Koha::Calendar::WeeklyClosures, RepeatingClosures, SingleClosures,
and Exceptions searches.
The internal data structures remain identical (weekly_closed_days
array, day_month_closed_days hash, _holidays hash with YYYYMMDD
keys) so all consumers (is_holiday, addDuration, next_open_days)
continue to work unchanged.
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Four Koha::Object + Koha::Objects pairs following the Koha::City(ies)
pattern:
- Koha::Calendar::WeeklyClosure(s) -> library_weekly_closures
- Koha::Calendar::RepeatingClosure(s) -> library_repeating_closures
- Koha::Calendar::SingleClosure(s) -> library_single_closures
- Koha::Calendar::Exception(s) -> library_closure_exceptions
DBIC schema files to be generated via the 'dbic' alias in KTD
after the database migration runs.
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Replace the two overloaded tables (repeatable_holidays, special_holidays)
with four purpose-specific tables:
- library_weekly_closures: weekly closed days (e.g. every Sunday)
- library_repeating_closures: annual repeating closures (e.g. Dec 25)
- library_single_closures: one-off closures with a DATE column
- library_closure_exceptions: open overrides with a DATE column
All use library_id FK and follow current community standards. It points to API
usage in a short term so it follows the API naming conventions.
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
$due_interval and $due_date (and $recall itself) returned by
Koha::Recalls->add_recall were captured but never used in either
branch of the new rank => "recall" handling. Drop the unused
assignment - no behaviour change.
Test plan:
1) prove t/db_dependent/Holds.t t/db_dependent/Reserves.t - all pass
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Two code paths introduced by this bug had no direct test coverage:
- C4::Reserves::ModReserve's title-level branch of the new
`rank => "recall"` conversion (only the item-level branch was
tested).
- The `hold_convert` bypass in Koha::Item::can_be_recalled, which
skips the "patron already has a hold on this item" check so an
existing hold can be converted to a recall for the same patron.
No production code changes here, coverage only.
Test plan:
1) prove t/db_dependent/Holds.t t/db_dependent/Koha/Item.t - all pass
2) Confirm the new tests actually exercise the code they claim to:
reverting the `unless ( $params->{hold_convert} )` guard in
Koha/Item.pm locally makes the new Item.t subtest fail, as
expected.
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
UseRecalls was converted from a YesNo syspref to a Choice preference
("off"/"opac"/"staff"/"opac_and_staff") earlier in this bug, and every
call site was updated from a truthy check to `ne "off"` - except this
one. Since even the string "off" is truthy in Perl, the guard in
Koha::REST::V1::Transfers::cancel was silently defeated: cancelling a
transfer via the REST API always attempted to revert an in-transit
recall, even with recalls disabled.
Test plan:
1) prove t/db_dependent/api/v1/transfers.t - new subtest
'cancel() - UseRecalls disabled' fails against the old code
(the in-transit recall gets reverted even though UseRecalls is off)
2) Apply this patch
3) Same test now passes
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Slava Shishkin
Pedro Amorim
Lucas Gass
Martin Renvoize
Kyle M Hall
Jonathan Druart
Tomás Cohen Arazi
Olivia Reynolds
Baptiste Wojtkowski