Fix transactions posting a day early when booked around midnight.#2744
Fix transactions posting a day early when booked around midnight.#2744OtterBoops wants to merge 7 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughTimestamp-based transaction dates now use each account family’s timezone across multiple provider processors and shared date helpers. Date-only strings retain direct date parsing, and regression tests cover timezone-adjusted calendar dates. ChangesTransaction date timezone handling
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/models/akahu_entry/processor_test.rb (1)
101-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the UTC time in inline comments.
The Unix timestamp
1752537000evaluates exactly to23:50:00 UTCrather than23:30:00 UTC(which would be1752535800). While this 20-minute difference does not affect the correctness of the tests—since both times still fall on the targetDate.new(2025, 7, 15)in the specified timezones—updating the comments prevents potential confusion for future readers reviewing the math.
test/models/akahu_entry/processor_test.rb#L101-L107: update23:30:00to23:50:00and the local time to11:50:00 NZST.test/models/enable_banking_entry/processor_test.rb#L435-L441: update23:30:00to23:50:00and the local time to01:50:00 CEST.test/models/lunchflow_entry/processor_test.rb#L383-L391: update23:30:00to23:50:00and the local time to00:50:00 BST.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/models/akahu_entry/processor_test.rb` around lines 101 - 107, Correct the inline timestamp comments without changing test data: in test/models/akahu_entry/processor_test.rb lines 101-107, use 23:50:00 UTC and 11:50:00 NZST; in test/models/enable_banking_entry/processor_test.rb lines 435-441, use 23:50:00 UTC and 01:50:00 CEST; in test/models/lunchflow_entry/processor_test.rb lines 383-391, use 23:50:00 UTC and 00:50:00 BST.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/models/akahu_entry/processor_test.rb`:
- Around line 101-107: Correct the inline timestamp comments without changing
test data: in test/models/akahu_entry/processor_test.rb lines 101-107, use
23:50:00 UTC and 11:50:00 NZST; in
test/models/enable_banking_entry/processor_test.rb lines 435-441, use 23:50:00
UTC and 01:50:00 CEST; in test/models/lunchflow_entry/processor_test.rb lines
383-391, use 23:50:00 UTC and 00:50:00 BST.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9b23c24-15fd-4577-a698-0880c41911a1
📒 Files selected for processing (6)
app/models/akahu_entry/processor.rbapp/models/enable_banking_entry/processor.rbapp/models/lunchflow_entry/processor.rbtest/models/akahu_entry/processor_test.rbtest/models/enable_banking_entry/processor_test.rbtest/models/lunchflow_entry/processor_test.rb
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fcc43c97c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Reviewed the timezone conversion fix. The core approach (.in_time_zone(account&.family&.timezone).to_date) is sound, but a few things worth a look:
- Same bug likely still present elsewhere:
app/models/brex_entry/processor.rb,mercury_entry/processor.rb,sophtron_entry/processor.rb,up_entry/processor.rb,wise_entry/processor.rb, andcoinstats_entry/processor.rball have the identical unconvertedTime.at(...).to_date/Time.parse(...).to_datepattern in theirdatemethods that this PR fixes for Akahu/Enable Banking/Lunchflow. Same midnight/"day early" bug is reproducible in any of these. Might be worth a tracking issue even if out of scope here. - Inconsistent within the PR's own scope:
akahu_entry/processor.rb'sStringbranch now detects time-bearing ISO strings (include?("T") || include?(":")) and runs them throughin_time_zone, but theStringbranches inenable_banking_entry/processor.rbandlunchflow_entry/processor.rbstill do a bareDate.parse(value). If either provider ever returns a datetime string (vs. a pure date) forbooking_date/date, that path reproduces the exact bug being fixed here. - Nil-timezone fallback is silent:
families.timezonehas no DB default, soaccount&.family&.timezonecan benilfor many accounts.Time#in_time_zone(nil)is a documented ActiveSupport no-op — it just returns the original (system/UTC) time, silently reverting to the old buggy behavior. Probably fine as a fallback, but worth confirming that's intentional, or logging it viaDebugLogEntry.captureper repo convention so it's visible when a family lacks a timezone. - Test coverage is one-sided: all three new tests use positive-UTC-offset zones (Pacific/Auckland, Europe/Berlin, Europe/London). None cover a negative-offset zone (e.g.
America/New_York) or a DST-transition date, so the fix isn't verified in that direction even though thein_time_zoneapproach should logically handle it correctly.
No blocking issues — the fix itself looks correct for the three files it touches.
Generated by Claude Code
|
This fix looks right for The main place is I also found at least one already-implemented provider with the same issue still present: Separately, a few existing entry processors still appear vulnerable when the upstream payload is a real timestamp rather than a date-only string, especially |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models/indexa_capital_account/data_helpers.rb`:
- Around line 66-76: Reorder the date case branches so the combined Time,
DateTime, and ActiveSupport::TimeWithZone branch is evaluated before the Date
branch, allowing DateTime values to receive timezone conversion. Apply this
change in app/models/indexa_capital_account/data_helpers.rb (lines 66-76),
lib/generators/provider/family/templates/data_helpers.rb.tt (lines 47-57), and
app/models/wise_entry/processor.rb (lines 152-161); no other behavior needs
changing.
🪄 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: b8504b02-3d20-44bb-b1da-952ed880eaa2
📒 Files selected for processing (11)
app/models/brex_entry/processor.rbapp/models/coinstats_entry/processor.rbapp/models/enable_banking_entry/processor.rbapp/models/indexa_capital_account/data_helpers.rbapp/models/lunchflow_entry/processor.rbapp/models/mercury_entry/processor.rbapp/models/sophtron_entry/processor.rbapp/models/up_entry/processor.rbapp/models/wise_entry/processor.rblib/generators/provider/family/templates/data_helpers.rb.tttest/models/lunchflow_entry/processor_test.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- app/models/lunchflow_entry/processor.rb
- app/models/enable_banking_entry/processor.rb
|
Some pipeline tests fail with: The only thing I can think of is editing migrations to use # db/migrate/20260627101954_add_balance_reversal_to_enable_banking_accounts.rb
add_column :enable_banking_accounts, :treat_balance_as_available_credit, :boolean, default: false, null: false unless column_exists?(:enable_banking_accounts, :treat_balance_as_available_credit)Want me to try and apply a fix? Doesn't feel like a decision for me to make. @jjmata |
|
Following up on your question above: this looks like a real duplicate-migration issue rather than scope creep, so a defensive Generated by Claude Code |
Yeah I'll address it, I was just making sure that further changes I make won't be out of scope. |
Totally cool to add those to the scope here, @OtterBoops ... thanks for checking! |
Everything should be covered. Lemme know if there are any other remarks |
4 participants
This PR fixes #2668 and implements some tests surrounding the edge case.
Summary by CodeRabbit
Date, including Unix timestamps, ISO-like datetime strings, andTime/DateTimeinputs.