Feature: implement fiscal year configuration with FYTD period#1540
Draft
BurningDog wants to merge 13 commits intowe-promise:mainfrom
Draft
Feature: implement fiscal year configuration with FYTD period#1540BurningDog wants to merge 13 commits intowe-promise:mainfrom
BurningDog wants to merge 13 commits intowe-promise:mainfrom
Conversation
brin-security-scanner
Bot
added
contributor:verified
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds family-level fiscal year start (month/day): DB columns, validations, model helpers, preferences UI, translations, tests, and a new Period "fiscal_year_to_date" gated by family usage that computes FYTD ranges using the configured fiscal year start. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as "User (browser)"
participant Controller as "UsersController"
participant Family as "Family model (Current.family)"
participant Period as "Period"
participant DB as "Database"
Browser->>Controller: submit preferences (fiscal_month/day)
Controller->>Family: assign `family_attributes` and save
Family->>DB: persist `fiscal_year_start_month` / `fiscal_year_start_day` (with checks)
Note right of Family: uses_fiscal_year? / current_fiscal_year_start available
Browser->>Period: request "fiscal_year_to_date"
Period->>Family: read `Current.family` (if present)
Period->>Period: compute date_range using `current_fiscal_year_start` or fallback
Period-->>Browser: return FYTD date range
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/models/period.rb (1)
61-70: Consider filtering FYTD when family has default fiscal year.When a family uses the default Jan 1 fiscal start,
fiscal_year_to_dateproduces the exact same range ascurrent_year("YTD"), so users see two indistinguishable options in selectors built fromPeriod.as_options. Consider conditionally excluding this period (or makingas_options/allfilter it) whenCurrent.family&.uses_fiscal_year?is false.Also: the
Current.familynil fallback toDate.current.beginning_of_yearwill silently mislabel a YTD range as "Financial Year" — acceptable given the caller context inPeriodable, but worth a brief comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/period.rb` around lines 61 - 70, The fiscal_year_to_date period produces the same range as current_year when a family uses the default Jan 1 start; update the logic so fiscal_year_to_date is excluded unless Current.family&.uses_fiscal_year? is true—either by changing the fiscal_year_to_date entry to return nil/skip when uses_fiscal_year? is false or by filtering it out in Period.as_options / Period.all (whichever builds selector options); also add a brief comment next to the Current.family fallback that Date.current.beginning_of_year may silently produce a YTD-equivalent range when Current.family is nil so callers like Periodable are aware.app/views/settings/preferences/show.html.erb (1)
36-43: Optional: hide FYTD/these controls when fiscal year isn't used, or at least clarify the day field.Minor UX note: when users keep the default (Jan 1), the new "fiscal_year_to_date" period will render a "FYTD" option that is identical to "YTD" in the period dropdown. Consider either (a) only registering/exposing the FYTD period when
family.uses_fiscal_year?is true, or (b) leaving as-is and accepting the duplicate. Also, the hint is attached only to the month select; users may miss it when scanning the day select — not a blocker.No correctness issues with
Date::MONTHNAMES.compact.each_with_index.map { ... i + 1 }(first element isnil, so compact yields January..December mapped to 1..12 correctly).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/settings/preferences/show.html.erb` around lines 36 - 43, Wrap the two selects (family_form.select :fiscal_year_start_month and :fiscal_year_start_day) in a conditional that checks family.uses_fiscal_year? so the month/day controls (and the resulting "fiscal_year_to_date" behavior) are only exposed when the family actually uses a fiscal year; additionally move or duplicate the hint (t(".fiscal_year_start_hint")) so it is applied to both the month and the day selects (or render a shared hint element adjacent to them) so users see the guidance when scanning either control; ensure you update any code that registers the FYTD period to only do so when family.uses_fiscal_year? to avoid creating a duplicate FYTD identical to YTD for Jan 1 defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/models/period.rb`:
- Around line 61-70: The fiscal_year_to_date period produces the same range as
current_year when a family uses the default Jan 1 start; update the logic so
fiscal_year_to_date is excluded unless Current.family&.uses_fiscal_year? is
true—either by changing the fiscal_year_to_date entry to return nil/skip when
uses_fiscal_year? is false or by filtering it out in Period.as_options /
Period.all (whichever builds selector options); also add a brief comment next to
the Current.family fallback that Date.current.beginning_of_year may silently
produce a YTD-equivalent range when Current.family is nil so callers like
Periodable are aware.
In `@app/views/settings/preferences/show.html.erb`:
- Around line 36-43: Wrap the two selects (family_form.select
:fiscal_year_start_month and :fiscal_year_start_day) in a conditional that
checks family.uses_fiscal_year? so the month/day controls (and the resulting
"fiscal_year_to_date" behavior) are only exposed when the family actually uses a
fiscal year; additionally move or duplicate the hint
(t(".fiscal_year_start_hint")) so it is applied to both the month and the day
selects (or render a shared hint element adjacent to them) so users see the
guidance when scanning either control; ensure you update any code that registers
the FYTD period to only do so when family.uses_fiscal_year? to avoid creating a
duplicate FYTD identical to YTD for Jan 1 defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1155aa49-0964-422c-ae4d-c7a806349cc3
📒 Files selected for processing (7)
app/controllers/users_controller.rbapp/models/family.rbapp/models/period.rbapp/views/settings/preferences/show.html.erbconfig/locales/views/settings/en.ymldb/migrate/20260421000000_add_fiscal_year_start_to_families.rbdb/schema.rb
BurningDog
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/models/family/fiscal_year_test.rb (1)
51-58: Add the exact fiscal-start-day boundary.This test name covers “on or after”, but it only tests “after”. An implementation using
>instead of>=would still pass while rolling back incorrectly on the fiscal-year start date.Suggested boundary assertion
test "current_fiscal_year_start returns this year's start when today is on or after it" do `@family.fiscal_year_start_month` = 3 `@family.fiscal_year_start_day` = 1 + travel_to Date.new(2026, 3, 1) do + assert_equal Date.new(2026, 3, 1), `@family.current_fiscal_year_start` + end + travel_to Date.new(2026, 4, 21) do assert_equal Date.new(2026, 3, 1), `@family.current_fiscal_year_start` end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/family/fiscal_year_test.rb` around lines 51 - 58, The test only verifies the "after" case and misses the exact boundary; add a case that travels to the exact fiscal start date and asserts current_fiscal_year_start returns that same date. Specifically, add a travel_to Date.new(2026, 3, 1) (matching `@family.fiscal_year_start_month` and _day) and assert_equal Date.new(2026, 3, 1), `@family.current_fiscal_year_start` to ensure the implementation uses >= rather than >.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/period.rb`:
- Around line 61-73: Period.from_key is being called for "fiscal_year_to_date"
without verifying availability, so FYTD can be constructed for families that
don't use fiscal years; add an availability check and use it wherever user input
is accepted. Either implement Period.available_key?(key, family =
Current.family) (mirroring Period.valid_key? but also checking the same filter
logic used by Period.all/as_options) and make Period.from_key raise or return
nil when passed an unavailable key, or add an explicit guard in
periodable.rb#set_period to call that availability check (e.g. replace the
Period.valid_key? check with an availability check or additionally verify
Current.family&.uses_fiscal_year? before passing "fiscal_year_to_date" to
Period.from_key) so user-supplied params[:period] cannot produce FYTD for
calendar-year families.
In `@app/views/settings/preferences/show.html.erb`:
- Around line 36-46: The fiscal year month/day selects are hidden by the
conditional that checks `@user.family.uses_fiscal_year`?, which is false for the
default Jan 1 and prevents users from ever switching; remove or change that
guard so the selects (family_form.select :fiscal_year_start_month and
:fiscal_year_start_day) are rendered for all families, and optionally add a UI
toggle (e.g., "use_fiscal_year" checkbox) that client-side shows/hides those
selects or change the predicate to a mutable attribute (e.g.,
family.use_fiscal_year) that the user can flip; ensure the hint paragraph
t(".fiscal_year_start_hint") remains with the selects and keep the data
attribute auto_submit_form_target on both selects.
---
Nitpick comments:
In `@test/models/family/fiscal_year_test.rb`:
- Around line 51-58: The test only verifies the "after" case and misses the
exact boundary; add a case that travels to the exact fiscal start date and
asserts current_fiscal_year_start returns that same date. Specifically, add a
travel_to Date.new(2026, 3, 1) (matching `@family.fiscal_year_start_month` and
_day) and assert_equal Date.new(2026, 3, 1), `@family.current_fiscal_year_start`
to ensure the implementation uses >= rather than >.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 948a8302-9510-4903-9ee0-530dc31686f8
📒 Files selected for processing (4)
app/models/period.rbapp/views/settings/preferences/show.html.erbtest/models/family/fiscal_year_test.rbtest/models/period_test.rb
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/models/period.rb (2)
61-73: Fallback branch and comment are now stale.With
from_keygated byavailable_key?(which requiresfamily&.uses_fiscal_year?to be truthy for this key), this lambda is never invoked whenCurrent.familyis nil. That makes thefamily ? … : Date.current.beginning_of_yearternary and the guidance comment on lines 64–66 effectively dead code. Not a bug, but worth simplifying to avoid drift.♻️ Suggested simplification
"fiscal_year_to_date" => { - date_range: -> { - family = Current.family - # When family is nil the fallback silently produces a YTD-equivalent range - # (beginning_of_year..today) identical to current_year; callers such as - # Periodable should guard against this with uses_fiscal_year?. - start_date = family ? family.current_fiscal_year_start : Date.current.beginning_of_year - [ start_date, Date.current ] - }, + # Availability is gated by Period.available_key?, so Current.family is guaranteed + # to be present and uses_fiscal_year? when this lambda is evaluated via from_key. + date_range: -> { [ Current.family.current_fiscal_year_start, Date.current ] }, label_short: "FYTD",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/period.rb` around lines 61 - 73, The lambda for "fiscal_year_to_date" contains a stale fallback and comment because from_key is guarded by available_key? (which requires Current.family&.uses_fiscal_year?), so remove the ternary and explanatory comment and simplify the date_range lambda to directly use Current.family.current_fiscal_year_start as the start date (e.g., return [Current.family.current_fiscal_year_start, Date.current]); keep labels unchanged and ensure references to the selector name "fiscal_year_to_date", Current.family, available_key? and uses_fiscal_year? are used to locate the code.
137-141: Minor redundancy inPeriod.all.
from_keyalready raisesInvalidKeyErrorviaavailable_key?, so the guard here is redundant. You can either drop it or usePERIODS[key]directly to avoid the double lookup — either form is fine; just flagging for hygiene.♻️ Option A — rely on from_key
def all - PERIODS.filter_map do |key, _period| - from_key(key) if available_key?(key) - end + PERIODS.keys.select { |key| available_key?(key) }.map { |key| from_key(key) } end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/period.rb` around lines 137 - 141, Replace the redundant double-lookup in Period.all: instead of calling from_key(key) guarded by available_key?(key), return the stored period directly from PERIODS to avoid re-resolving the key; e.g. implement Period.all using PERIODS.filter_map { |key, period| period if available_key?(key) } (references: Period.all, from_key, available_key?, PERIODS).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/models/period.rb`:
- Around line 61-73: The lambda for "fiscal_year_to_date" contains a stale
fallback and comment because from_key is guarded by available_key? (which
requires Current.family&.uses_fiscal_year?), so remove the ternary and
explanatory comment and simplify the date_range lambda to directly use
Current.family.current_fiscal_year_start as the start date (e.g., return
[Current.family.current_fiscal_year_start, Date.current]); keep labels unchanged
and ensure references to the selector name "fiscal_year_to_date",
Current.family, available_key? and uses_fiscal_year? are used to locate the
code.
- Around line 137-141: Replace the redundant double-lookup in Period.all:
instead of calling from_key(key) guarded by available_key?(key), return the
stored period directly from PERIODS to avoid re-resolving the key; e.g.
implement Period.all using PERIODS.filter_map { |key, period| period if
available_key?(key) } (references: Period.all, from_key, available_key?,
PERIODS).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8f095ea-999a-4138-b494-e027243b3395
📒 Files selected for processing (3)
app/controllers/concerns/periodable.rbapp/models/period.rbapp/views/settings/preferences/show.html.erb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/settings/preferences/show.html.erb
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/models/period_test.rb (1)
85-115: Stub query collaborators and cover the calendar-year rejection path.These tests assert calls to query collaborators (
Current.family,current_fiscal_year_start) instead of only thePeriodoutput. Also add coverage for a present family whereuses_fiscal_year?is false, since that is the main FYTD guard.🧪 Proposed test adjustment
test "fiscal_year_to_date has correct labels" do mock_family = mock("family") mock_family.stubs(:uses_fiscal_year?).returns(true) - mock_family.expects(:current_fiscal_year_start).returns(Date.current.beginning_of_year) - Current.expects(:family).at_least_once.returns(mock_family) + mock_family.stubs(:current_fiscal_year_start).returns(Date.current.beginning_of_year) + Current.stubs(:family).returns(mock_family) period = Period.from_key("fiscal_year_to_date") assert_equal "Financial Year", period.label assert_equal "FYTD", period.label_short @@ test "fiscal_year_to_date uses family's fiscal year start" do mock_family = mock("family") mock_family.stubs(:uses_fiscal_year?).returns(true) - mock_family.expects(:current_fiscal_year_start).returns(Date.new(2026, 3, 1)) - Current.expects(:family).at_least_once.returns(mock_family) + mock_family.stubs(:current_fiscal_year_start).returns(Date.new(2026, 3, 1)) + Current.stubs(:family).returns(mock_family) travel_to Date.new(2026, 4, 21) do period = Period.from_key("fiscal_year_to_date") assert_equal Date.new(2026, 3, 1), period.start_date @@ end test "fiscal_year_to_date raises for nil family" do - Current.expects(:family).at_least_once.returns(nil) + Current.stubs(:family).returns(nil) assert_raises(Period::InvalidKeyError) do Period.from_key("fiscal_year_to_date") end end + + test "fiscal_year_to_date raises for calendar-year family" do + mock_family = mock("family") + mock_family.stubs(:uses_fiscal_year?).returns(false) + Current.stubs(:family).returns(mock_family) + + assert_raises(Period::InvalidKeyError) do + Period.from_key("fiscal_year_to_date") + end + endAs per coding guidelines, “Distinguish between commands and query methods. Test output of query methods; test that commands were called with the correct params” and “Never test the implementation details of one class in another class's test suite.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/period_test.rb` around lines 85 - 115, Remove assertions that expect calls to collaborators and instead stub their return values; for tests referencing Period.from_key, stop using mock.expects and mock.stubs with call-count expectations. Specifically, in tests using Current.family and the family double, replace mock_family.expects(...) with a double that responds to uses_fiscal_year? and current_fiscal_year_start but do not assert those methods were called; use Current.stubs(:family).returns(family_double) or Current.expects(:family).at_least_once -> change to Current.stubs(:family).returns(...). Also add a new test that sets Current.family to a family double where uses_fiscal_year? returns false and assert Period.from_key("fiscal_year_to_date") raises Period::InvalidKeyError to cover the calendar-year rejection path (use symbols: Period.from_key, Current.family, uses_fiscal_year?, current_fiscal_year_start, Period::InvalidKeyError).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/models/period_test.rb`:
- Around line 85-115: Remove assertions that expect calls to collaborators and
instead stub their return values; for tests referencing Period.from_key, stop
using mock.expects and mock.stubs with call-count expectations. Specifically, in
tests using Current.family and the family double, replace
mock_family.expects(...) with a double that responds to uses_fiscal_year? and
current_fiscal_year_start but do not assert those methods were called; use
Current.stubs(:family).returns(family_double) or
Current.expects(:family).at_least_once -> change to
Current.stubs(:family).returns(...). Also add a new test that sets
Current.family to a family double where uses_fiscal_year? returns false and
assert Period.from_key("fiscal_year_to_date") raises Period::InvalidKeyError to
cover the calendar-year rejection path (use symbols: Period.from_key,
Current.family, uses_fiscal_year?, current_fiscal_year_start,
Period::InvalidKeyError).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89254552-b57f-43e4-86f2-6db417978eb6
📒 Files selected for processing (3)
app/models/period.rbtest/models/family/fiscal_year_test.rbtest/models/period_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- test/models/family/fiscal_year_test.rb
Collaborator
|
Can you include a screenshot of new settings before you mark this as ready @BurningDog? |
Contributor
Author
|
Here is a screenshot of the new settings: 
The values of the "Financial year starts in" are the months of the year (e.g. January, February, etc). The values of "Financial year starts on" are from 1 - 28...but I'm wondering if we just remove this select list entirely as financial years only begin on the first of the month. |
Collaborator
|
Can we get all three "starts on" values on one single line? OR some other |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#1411 refers.
fiscal_year_start_monthandfiscal_year_start_daycolumns with check constraints and non-null defaults.What changed
Database —
db/migrate/20260421000000_add_fiscal_year_start_to_families.rb/db/schema.rbTwo integer columns are added to the
familiestable:fiscal_year_start_month— integer, default1, not null, constrained to1..12fiscal_year_start_day— integer, default1, not null, constrained to1..28Both columns have database-level check constraints (
fiscal_year_start_month_range,fiscal_year_start_day_range) mirroring the ActiveRecord validations. The day ceiling is28— the same conservative limit used bymonth_start_day— to avoid invalid dates in short months.Model —
app/models/family.rbTwo new instance methods expose fiscal-year logic to the rest of the app:
uses_fiscal_year?— returnstruewhen the family's fiscal year does not start on January 1, useful for conditional UI display.current_fiscal_year_start— returns the most recent past (or today's) fiscal year start date, handling the year-boundary roll-over correctly (if the configured start falls after today, it steps back one year).Period model —
app/models/period.rbA new
"fiscal_year_to_date"entry is registered in the period definitions hash. Itsdate_rangelambda readsCurrent.familyand callscurrent_fiscal_year_startto compute the start; it gracefully falls back toDate.current.beginning_of_yearwhen no family context is available. Labels: short"FYTD", long"Financial Year", comparison"vs. start of financial year".Preferences UI —
app/views/settings/preferences/show.html.erbTwo
selectfields are added to the preferences form immediately after the existingmonth_start_dayblock:Date::MONTHNAMES, auto-submits on change.1st,2nd, …,28th), auto-submits on change.Both fields include a hint string directing users to an example (
March 1 for a March – February fiscal year).Summary by CodeRabbit
New Features
Validation & Data
UI
Tests