[Bug 42334] New: Introduce Koha::Calendar::DateCalculator for date arithmetic
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42334 Bug ID: 42334 Summary: Introduce Koha::Calendar::DateCalculator for date arithmetic Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org Koha::Calendar currently mixes two concerns: calendar data (which days are closed) and date arithmetic (calculating due dates, counting open days, finding next open day). The arithmetic methods all depend on a days_mode parameter that gets passed into the Calendar constructor even though it has nothing to do with the calendar itself, it's a circulation policy. The goal is to extract the date arithmetic methods into a new Koha::Calendar::DateCalculator class that takes a calendar and a days_mode as constructor parameters: ``` my $calc = Koha::Calendar::DateCalculator->new( calendar => $library->calendar, days_mode => C4::Context->preference('useDaysMode'), ); my $due_date = $calc->add_duration( $start, $dur, 'days' ); ``` Methods to move: addDuration, addHours, addDays, get_push_amt, next_open_days, prev_open_days, days_forward, days_between, hours_between, has_business_days_between. This separates the calendar (what's closed) from the policy (how to handle closed days when calculating dates), and removes the days_mode parameter from Koha::Calendar->new where it never belonged. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42334 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |42310 CC| |tomascohen@gmail.com Status|NEW |ASSIGNED Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42310 [Bug 42310] Move calendar CRUD to Koha::Calendar::* with normalized schema -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42334 Bug 42334 depends on bug 42310, which changed state. Bug 42310 Summary: Move calendar CRUD to Koha::Calendar::* with normalized schema https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42310 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42334 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |43838 Summary|Introduce |Introduce |Koha::Calendar::DateCalcula |Koha::Library::Calendar::Da |tor for date arithmetic |teCalculator for date | |arithmetic Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43838 [Bug 43838] Dayweek pushes dates by a whole week when it should move to the next open day -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42334 --- Comment #1 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- The implementation differs from the original description: 1. The class is Koha::Library::Calendar::DateCalculator (Koha::Calendar was moved to Koha::Library::Calendar by bug 42310). 2. Only the methods that depend on days_mode move to the calculator: - addDuration, addDays, addHours => add_duration( $start, $duration, $unit ) - next_open_days, prev_open_days - get_push_amt (now internal) days_between, hours_between, has_business_days_between and days_forward stay in Koha::Library::Calendar: they only count open days and never use days_mode. 3. Callers get a calculator from the calendar, without loading the class: my $calculator = $library->calendar->date_calculator( { days_mode => $days_mode } ); my $due_date = $calculator->add_duration( $start, $duration, 'days' ); days_mode is mandatory. There is no fallback to the useDaysMode system preference: callers pass the effective value (the daysmode circulation rule or the system preference). 4. days_mode is validated when the calculator is created. Valid values are Days, Calendar, Datedue and Dayweek; anything else throws Koha::Exceptions::WrongParameter. Before, unknown values silently behaved like 'Days'. An atomic update replaces invalid values stored in useDaysMode or in daysmode circulation rules with 'Days', which keeps the previous behavior, and warns about each one so it can be reviewed. Koha::Library::Calendar also loses days_mode as a constructor parameter, the unused set_daysmode and clear_weekly_closed_days methods, and the {test} flag they relied on. It gains library_id and is_weekly_closure accessors, which the calculator uses instead of reading the calendar internals. This depends on bug 43838, which fixes how Dayweek moves dates over closed days, so the fix can be backported separately before this refactoring. Follow-ups, to be filed separately: - Validate daysmode values in admin/smart-rules.pl and in the circulation rules API, so invalid values cannot be stored again. - A single implementation for "is this day closed" (closed_dates_in_range, staticfines.pl using C4::Calendar::isHoliday). - Remove C4::Calendar once the calendar administration is moved to Koha::Library::Calendar. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42334 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Medium patch Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42334 --- Comment #2 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 208481 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208481&action=edit Bug 42334: Add Koha::Library::Calendar::DateCalculator Koha::Library::Calendar mixes two concerns: calendar data (which days are closed) and date arithmetic that applies a useDaysMode policy (calculating due dates, suspension end dates, etc). The policy is not a property of the calendar, but it is currently passed to its constructor as days_mode. This patch introduces Koha::Library::Calendar::DateCalculator, which takes a calendar and a days_mode and implements the policy-dependent arithmetic: - add_duration (replaces addDuration, addDays and addHours) - next_open_days / prev_open_days days_mode is mandatory and validated once in the constructor (Days, Calendar, Datedue or Dayweek). Koha::Library::Calendar gets: - date_calculator( { days_mode => $days_mode } ): accessor returning a calculator for the calendar, so callers don't need to load the class - library_id: accessor for the library the calendar belongs to - is_weekly_closure( $dt ): tells if the weekday of $dt is a weekly closure, needed by the Dayweek policy without reaching into the calendar internals The logic is moved from Koha::Library::Calendar, including the bug 43838 fix (next_open_days and prev_open_days step one day at a time, and only weekly Dayweek periods move to the next open matching weekday). The old methods are removed in the next patch. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Library/Calendar/DateCalculator.t \ t/db_dependent/Koha/Library/Calendar.t => SUCCESS: Tests pass! 3. Sign off :-D Assisted-by: Opus 5.5 (Anthropic) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42334 --- Comment #3 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 208482 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208482&action=edit Bug 42334: Use DateCalculator and remove days_mode from Koha::Library::Calendar This patch moves all callers of the date arithmetic methods to Koha::Library::Calendar::DateCalculator, obtained through the new date_calculator accessor, and removes the moved methods and the days_mode parameter from Koha::Library::Calendar. Changes: - Remove addDuration, addDays, addHours, get_push_amt, next_open_days and prev_open_days from Koha::Library::Calendar - Remove the unused set_daysmode and clear_weekly_closed_days methods and the {test} flag they relied on - days_forward no longer requires days_mode, which it never used - Koha::Hold->set_waiting no longer looks up the effective daysmode, it was only computed to satisfy the days_forward check - Adapt CalcDateDue, checkHighHolds, _calculate_new_debar_dt, Koha::Checkouts->calculate_dropbox_date, holds_reminder.pl and TalkingTech_itiva_outbound.pl - Adapt tests The counting methods (days_between, hours_between, has_business_days_between, days_forward) stay in Koha::Library::Calendar: they don't depend on any useDaysMode policy. Only behavior change: an invalid days_mode (useDaysMode syspref or daysmode circulation rule) now throws Koha::Exceptions::WrongParameter instead of silently behaving like 'Days'. Neither smart-rules.pl nor the circulation rules API validate the daysmode value, so a misconfigured rule now makes due date calculations fail loudly. CalcDateDue.t used such an invalid value (noDays) and is adapted to use Datedue, which matches its intent. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Calendar.t \ t/db_dependent/Koha/Library/Calendar.t \ t/db_dependent/Koha/Library/Calendar/DateCalculator.t \ t/db_dependent/Circulation/CalcDateDue.t \ t/db_dependent/Circulation.t \ t/db_dependent/Hold.t \ t/db_dependent/Holds.t \ t/db_dependent/Koha/Checkouts.t \ t/db_dependent/Koha/Hold.t => SUCCESS: Tests pass! 3. Verify no references to the removed methods remain: k$ git grep -nE '\->(addDuration|addDays|addHours|get_push_amt|set_daysmode|clear_weekly_closed_days)\b' -- ':!misc/release_notes' => SUCCESS: No results 4. Sign off :-D Assisted-by: Opus 5.5 (Anthropic) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42334 --- Comment #4 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 208483 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208483&action=edit Bug 42334: Replace invalid useDaysMode and daysmode values Koha::Library::Calendar::DateCalculator rejects invalid days_mode values. Before, any unknown value (in the useDaysMode system preference or the daysmode circulation rule) silently behaved like 'Days', and neither the circulation rules editor nor the API validate them, so existing installations could have such values stored. This patch adds an atomic update that replaces invalid values with 'Days', which keeps the previous behavior, and warns about each one so it can be reviewed. Empty daysmode rule values are left alone, as they mean the system preference applies. Test plan: 1. Set invalid values: $ ktd --shell k$ koha-mysql kohadev -e "UPDATE systempreferences SET value='noDays' WHERE variable='useDaysMode'; INSERT INTO circulation_rules (branchcode,rule_name,rule_value) VALUES ('CPL','daysmode','Calender'),('MPL','daysmode','Dayweek');" 2. Apply patch, run: k$ updatedatabase => SUCCESS: Warnings about useDaysMode ('noDays') and the CPL daysmode rule ('Calender') are displayed 3. Check the values: k$ koha-mysql kohadev -e "SELECT value FROM systempreferences WHERE variable='useDaysMode'; SELECT branchcode,rule_value FROM circulation_rules WHERE rule_name='daysmode';" => SUCCESS: useDaysMode and the CPL rule are 'Days', the MPL rule is still 'Dayweek' 4. Run updatedatabase again => SUCCESS: No warnings, nothing changes 5. Sign off :-D Assisted-by: Opus 5.5 (Anthropic) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42334 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@openfifth.c | |o.uk -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42334 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42334 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #208481|0 |1 is obsolete| | Attachment #208482|0 |1 is obsolete| | Attachment #208483|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42334 --- Comment #5 from David Nind <david@davidnind.com> --- Created attachment 208515 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208515&action=edit Bug 42334: Add Koha::Library::Calendar::DateCalculator Koha::Library::Calendar mixes two concerns: calendar data (which days are closed) and date arithmetic that applies a useDaysMode policy (calculating due dates, suspension end dates, etc). The policy is not a property of the calendar, but it is currently passed to its constructor as days_mode. This patch introduces Koha::Library::Calendar::DateCalculator, which takes a calendar and a days_mode and implements the policy-dependent arithmetic: - add_duration (replaces addDuration, addDays and addHours) - next_open_days / prev_open_days days_mode is mandatory and validated once in the constructor (Days, Calendar, Datedue or Dayweek). Koha::Library::Calendar gets: - date_calculator( { days_mode => $days_mode } ): accessor returning a calculator for the calendar, so callers don't need to load the class - library_id: accessor for the library the calendar belongs to - is_weekly_closure( $dt ): tells if the weekday of $dt is a weekly closure, needed by the Dayweek policy without reaching into the calendar internals The logic is moved from Koha::Library::Calendar, including the bug 43838 fix (next_open_days and prev_open_days step one day at a time, and only weekly Dayweek periods move to the next open matching weekday). The old methods are removed in the next patch. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Library/Calendar/DateCalculator.t \ t/db_dependent/Koha/Library/Calendar.t => SUCCESS: Tests pass! 3. Sign off :-D Assisted-by: Opus 5.5 (Anthropic) Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42334 --- Comment #6 from David Nind <david@davidnind.com> --- Created attachment 208516 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208516&action=edit Bug 42334: Use DateCalculator and remove days_mode from Koha::Library::Calendar This patch moves all callers of the date arithmetic methods to Koha::Library::Calendar::DateCalculator, obtained through the new date_calculator accessor, and removes the moved methods and the days_mode parameter from Koha::Library::Calendar. Changes: - Remove addDuration, addDays, addHours, get_push_amt, next_open_days and prev_open_days from Koha::Library::Calendar - Remove the unused set_daysmode and clear_weekly_closed_days methods and the {test} flag they relied on - days_forward no longer requires days_mode, which it never used - Koha::Hold->set_waiting no longer looks up the effective daysmode, it was only computed to satisfy the days_forward check - Adapt CalcDateDue, checkHighHolds, _calculate_new_debar_dt, Koha::Checkouts->calculate_dropbox_date, holds_reminder.pl and TalkingTech_itiva_outbound.pl - Adapt tests The counting methods (days_between, hours_between, has_business_days_between, days_forward) stay in Koha::Library::Calendar: they don't depend on any useDaysMode policy. Only behavior change: an invalid days_mode (useDaysMode syspref or daysmode circulation rule) now throws Koha::Exceptions::WrongParameter instead of silently behaving like 'Days'. Neither smart-rules.pl nor the circulation rules API validate the daysmode value, so a misconfigured rule now makes due date calculations fail loudly. CalcDateDue.t used such an invalid value (noDays) and is adapted to use Datedue, which matches its intent. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Calendar.t \ t/db_dependent/Koha/Library/Calendar.t \ t/db_dependent/Koha/Library/Calendar/DateCalculator.t \ t/db_dependent/Circulation/CalcDateDue.t \ t/db_dependent/Circulation.t \ t/db_dependent/Hold.t \ t/db_dependent/Holds.t \ t/db_dependent/Koha/Checkouts.t \ t/db_dependent/Koha/Hold.t => SUCCESS: Tests pass! 3. Verify no references to the removed methods remain: k$ git grep -nE '\->(addDuration|addDays|addHours|get_push_amt|set_daysmode|clear_weekly_closed_days)\b' -- ':!misc/release_notes' => SUCCESS: No results 4. Sign off :-D Assisted-by: Opus 5.5 (Anthropic) Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42334 --- Comment #7 from David Nind <david@davidnind.com> --- Created attachment 208517 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208517&action=edit Bug 42334: Replace invalid useDaysMode and daysmode values Koha::Library::Calendar::DateCalculator rejects invalid days_mode values. Before, any unknown value (in the useDaysMode system preference or the daysmode circulation rule) silently behaved like 'Days', and neither the circulation rules editor nor the API validate them, so existing installations could have such values stored. This patch adds an atomic update that replaces invalid values with 'Days', which keeps the previous behavior, and warns about each one so it can be reviewed. Empty daysmode rule values are left alone, as they mean the system preference applies. Test plan: 1. Set invalid values: $ ktd --shell k$ koha-mysql kohadev -e "UPDATE systempreferences SET value='noDays' WHERE variable='useDaysMode'; INSERT INTO circulation_rules (branchcode,rule_name,rule_value) VALUES ('CPL','daysmode','Calender'),('MPL','daysmode','Dayweek');" 2. Apply patch, run: k$ updatedatabase => SUCCESS: Warnings about useDaysMode ('noDays') and the CPL daysmode rule ('Calender') are displayed 3. Check the values: k$ koha-mysql kohadev -e "SELECT value FROM systempreferences WHERE variable='useDaysMode'; SELECT branchcode,rule_value FROM circulation_rules WHERE rule_name='daysmode';" => SUCCESS: useDaysMode and the CPL rule are 'Days', the MPL rule is still 'Dayweek' 4. Run updatedatabase again => SUCCESS: No warnings, nothing changes 5. Sign off :-D Assisted-by: Opus 5.5 (Anthropic) Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42334 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #8 from David Nind <david@davidnind.com> --- Testing notes (using KTD): 1. The dependent bug 43838 has patches for Koha versions other than main. When applying the patches use interactive mode, and only apply the first two patches and not the =<26.05 ones. 2. Step 3, second patch - there is one occurrence, but it is a comment - not sure if this section needs review or not - will leave it up to you: installer/data/mysql/updatedatabase.pl:17886: # Self-contained equivalent of Koha::Library::Calendar->get_push_amt: the -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org