gavin/maybe
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 28 Wiki Activity

Add income and spending insight cards to dashboard #668

Merged
JoshAntBrown merged 10 commits from feature/transaction-insight-cards into main 2024-04-24 20:34:50 +08:00
Conversation 9 Commits 10 Files Changed 8 +207 -64
JoshAntBrown commented 2024-04-23 21:55:04 +08:00 (Migrated from github.com)
Copy Link

This PR implements the income and spending insight cards for the dashboard.

Screenshot 2024-04-23 at 14 50 25

Requirements

  • Should generate a time series of income and spending for a Family, scoped to Period.last_30_days
    • Each data point in the series represents a "Trailing 30 day sum" of income (negative transaction values)
    • The "headline" number shows the last value in the series
    • The "trend" value is the overall time series trend
  • Should find the top 3 accounts with the most income or spending within the last 30 days and display the sum
    • Each account is an anchor that goes to that account page
    • Bank icons are not in-scope (just use a generic letter as shown in designs)
    • A "more" counter should show if there are more than 3 accounts
  • Display a trendline of series. No tooltips, no axes.
  • These insights should be included in the dashboard

/claim #645
Resolves #645

This PR implements the income and spending insight cards for the dashboard. <img width="1624" alt="Screenshot 2024-04-23 at 14 50 25" src="https://github.com/maybe-finance/maybe/assets/1793797/f00e873e-54ef-4410-8982-9a19ae4c77ab"> ## Requirements * [x] Should generate a time series of income and spending for a `Family`, scoped to `Period.last_30_days` * [x] Each data point in the series represents a "Trailing 30 day sum" of income (negative transaction values) * [x] The "headline" number shows the last value in the series * [x] The "trend" value is the overall time series trend * [x] Should find the top 3 accounts with the _most_ income or spending within the last 30 days and display the sum * [x] Each account is an anchor that goes to that account page * [x] Bank icons are not in-scope (just use a generic letter as shown in designs) * [x] A "more" counter should show if there are more than 3 accounts * [x] Display a trendline of series. No tooltips, no axes. * [x] These insights should be included in the dashboard /claim #645 Resolves #645
JoshAntBrown commented 2024-04-23 22:46:09 +08:00 (Migrated from github.com)
Copy Link

/claim #645

/claim #645
zachgoll (Migrated from github.com) reviewed 2024-04-23 23:53:09 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

The structure of this looks good to me, just added some notes regarding tests / exchange rate conversions for handling multi-currency.

The structure of this looks good to me, just added some notes regarding tests / exchange rate conversions for handling multi-currency.
app/models/family.rb
@@ -28,6 +28,49 @@ class Family < ApplicationRecord
}
zachgoll (Migrated from github.com) commented 2024-04-23 22:32:56 +08:00
Copy Link

Small nitpick—this may be something to handle in Period, but I'm thinking days_rolling should always be a function of the period. If we changed period to be Period.last_365_days, I'd probably expect the calculation to switch to a "trailing 12 month" (365 day) period.

Would you agree with that? Might be worth expressing that here.

Small nitpick—this may be something to handle in `Period`, but I'm thinking `days_rolling` should _always_ be a function of the period. If we changed `period` to be `Period.last_365_days`, I'd probably expect the calculation to switch to a "trailing 12 month" (365 day) period. Would you agree with that? Might be worth expressing that here.
app/models/family.rb
@@ -31,0 +69,4 @@
income_series: TimeSeries.new(income, favorable_direction: "up"),
spending_series: TimeSeries.new(spending, favorable_direction: "down")
}
end
zachgoll (Migrated from github.com) commented 2024-04-23 23:52:23 +08:00
Copy Link

I like the overall direction of this query. Definitely a complex one (that will likely need to be modified as we start adding various transaction types), so I'm thinking we may need a few test cases for documentation purposes. As a quick summary of this comment:

  • Series length should always be 30 days, regardless of how many currencies a Family has
  • We'll need to handle exchange rate conversions for this rollup
  • Consider using a CTE for clarity of the query? - Rails 7 has the with method that may make this a little easier to scan top-to-bottom?

Looking at the output for the demo account (rake demo_data:reset):

CleanShot 2024-04-23 at 10 53 17

In the screenshot above, I would expect these insights to always show in the Family.currency (the demo account is USD, but one of the top moving accounts is EUR). Also, it appears that both currency series from the query are being included, causing the count to be 62 rather than the intended 31.

To handle multiple currencies, I'm thinking we'll need an additional step that will normalize everything into the Family.currency prior to summing it all up

  • Since we cannot depend on exchange rates being available, we'll either need to omit unconverted amounts from the aggregation or omit accounts that contain "foreign" currencies from the calculation entirely.
  • My guess is that it would be easiest to perform a join on exchange_rates table somewhere within this query and do the filtering there (rather than trying to call exchange_to on each money instance after-the-fact). In this strategy, we'd basically be saying, "we'll try to convert, but if the app doesn't already have the needed exchange rates cached to the DB, we can't calculate this and will omit it"
  • We could still technically show a "top mover" in a native foreign currency (since we have a series for all currencies), but series that cannot be converted via the query should never make it into that headline number

I've been using this spreadsheet to visualize the expected results and conversions a little bit better. Just throwing it out there in case it helps with this one.

As this is a tricky one, I'm open to ideas! All the comments above are just my best guess at how we could approach it.

Here's a sketch of what I was thinking for the exchange rates (haven't tested thoroughly, using some hardcoded placeholders):

WITH date_series AS (
    SELECT 
        gs.date, 
        c.currency
    FROM 
        generate_series('2024-02-23'::date, '2024-04-23'::date, '1 day') AS gs(date)
        CROSS JOIN (SELECT DISTINCT currency FROM transactions) AS c
),
normalized AS (
    SELECT
        ds.date,
        ds.currency,
        COALESCE(SUM(CASE WHEN t.amount > 0 THEN t.amount ELSE 0 END), 0) AS spending,
        COALESCE(SUM(CASE WHEN t.amount < 0 THEN -t.amount ELSE 0 END), 0) AS income
    FROM
        date_series ds
        LEFT JOIN transactions t ON t.date = ds.date AND t.currency = ds.currency
    GROUP BY
        ds.date, ds.currency
),
rolling AS (
    SELECT 
        n.date,
        n.currency,
        SUM(n.spending) OVER (PARTITION BY n.currency ORDER BY n.date RANGE BETWEEN '30 days' PRECEDING AND CURRENT ROW) AS rolling_spend,
        SUM(n.income) OVER (PARTITION BY n.currency ORDER BY n.date RANGE BETWEEN '30 days' PRECEDING AND CURRENT ROW) AS rolling_income
    FROM 
        normalized n
),
converted_rolling AS (
    SELECT
        r.date,
        SUM(COALESCE(r.rolling_spend * er.rate, 0)) AS converted_spend,
        SUM(COALESCE(r.rolling_income * er.rate, 0)) AS converted_income
    FROM
        rolling r
        LEFT JOIN exchange_rates er ON r.currency = er.base_currency AND er.converted_currency = 'USD' -- replace USD with whatever the Family's preferred currency is
    GROUP BY 
        r.date
)
SELECT * FROM converted_rolling ORDER BY date;

IMO, these CTEs read a little easier but I know it completely deviates from ActiveRecord convention. Not sure what the best approach is?

I like the overall direction of this query. Definitely a complex one (that will likely need to be modified as we start adding various transaction types), so I'm thinking we may need a few test cases for documentation purposes. As a quick summary of this comment: - Series length should always be 30 days, regardless of how many currencies a `Family` has - We'll need to handle exchange rate conversions for this rollup - Consider using a CTE for clarity of the query? - Rails 7 has the `with` method that _may_ make this a little easier to scan top-to-bottom? Looking at the output for the demo account (`rake demo_data:reset`): ![CleanShot 2024-04-23 at 10 53 17](https://github.com/maybe-finance/maybe/assets/16676157/e31017a9-b8f7-4a82-878a-93a86abdd95c) In the screenshot above, I would expect these insights to _always_ show in the `Family.currency` (the demo account is `USD`, but one of the top moving accounts is `EUR`). Also, it appears that both currency series from the query are being included, causing the count to be `62` rather than the intended `31`. To handle multiple currencies, I'm thinking we'll need an _additional_ step that will normalize everything into the `Family.currency` prior to summing it all up - Since we cannot _depend on_ exchange rates being available, we'll either need to _omit_ unconverted amounts from the aggregation or omit accounts that contain "foreign" currencies from the calculation entirely. - My guess is that it would be easiest to perform a join on `exchange_rates` table somewhere within this query and do the filtering there (rather than trying to call `exchange_to` on each money instance after-the-fact). In this strategy, we'd basically be saying, "we'll try to convert, but if the app doesn't already have the needed exchange rates cached to the DB, we can't calculate this and will omit it" - We could still technically show a "top mover" in a native foreign currency (since we have a series for all currencies), but series that cannot be converted via the query should never make it into that headline number I've been using [this spreadsheet](https://docs.google.com/spreadsheets/d/18LN5N-VLq4b49Mq1fNwF7_eBiHSQB46qQduRtdAEN98/edit#gid=0) to visualize the expected results and conversions a little bit better. Just throwing it out there in case it helps with this one. As this is a tricky one, I'm open to ideas! All the comments above are just my best guess at how we could approach it. ---------- Here's a _sketch_ of what I was thinking for the exchange rates (haven't tested thoroughly, using some hardcoded placeholders): ```sql WITH date_series AS ( SELECT gs.date, c.currency FROM generate_series('2024-02-23'::date, '2024-04-23'::date, '1 day') AS gs(date) CROSS JOIN (SELECT DISTINCT currency FROM transactions) AS c ), normalized AS ( SELECT ds.date, ds.currency, COALESCE(SUM(CASE WHEN t.amount > 0 THEN t.amount ELSE 0 END), 0) AS spending, COALESCE(SUM(CASE WHEN t.amount < 0 THEN -t.amount ELSE 0 END), 0) AS income FROM date_series ds LEFT JOIN transactions t ON t.date = ds.date AND t.currency = ds.currency GROUP BY ds.date, ds.currency ), rolling AS ( SELECT n.date, n.currency, SUM(n.spending) OVER (PARTITION BY n.currency ORDER BY n.date RANGE BETWEEN '30 days' PRECEDING AND CURRENT ROW) AS rolling_spend, SUM(n.income) OVER (PARTITION BY n.currency ORDER BY n.date RANGE BETWEEN '30 days' PRECEDING AND CURRENT ROW) AS rolling_income FROM normalized n ), converted_rolling AS ( SELECT r.date, SUM(COALESCE(r.rolling_spend * er.rate, 0)) AS converted_spend, SUM(COALESCE(r.rolling_income * er.rate, 0)) AS converted_income FROM rolling r LEFT JOIN exchange_rates er ON r.currency = er.base_currency AND er.converted_currency = 'USD' -- replace USD with whatever the Family's preferred currency is GROUP BY r.date ) SELECT * FROM converted_rolling ORDER BY date; ``` IMO, these CTEs read a little easier but I know it completely deviates from ActiveRecord convention. Not sure what the best approach is?
JoshAntBrown (Migrated from github.com) reviewed 2024-04-24 00:10:46 +08:00
app/models/family.rb
@@ -28,6 +28,49 @@ class Family < ApplicationRecord
}
JoshAntBrown (Migrated from github.com) commented 2024-04-24 00:10:46 +08:00
Copy Link

Yep makes sense to me, we can just count the days in period we're using and then use that to build the query. I'll get that updated.

Yep makes sense to me, we can just count the days in period we're using and then use that to build the query. I'll get that updated.
JoshAntBrown (Migrated from github.com) reviewed 2024-04-24 00:23:26 +08:00
app/models/family.rb
@@ -31,0 +69,4 @@
income_series: TimeSeries.new(income, favorable_direction: "up"),
spending_series: TimeSeries.new(spending, favorable_direction: "down")
}
end
JoshAntBrown (Migrated from github.com) commented 2024-04-24 00:23:26 +08:00
Copy Link

Thanks! Yeah it's been a tricky one, that makes sense regarding the currencies I was mirroring what was going on with the net worth chart but this makes more sense.

I'll take a look at working the currency conversion into things, and while I do that I'll see if I can do anything to improve the readability.

Thanks! Yeah it's been a tricky one, that makes sense regarding the currencies I was mirroring what was going on with the net worth chart but this makes more sense. I'll take a look at working the currency conversion into things, and while I do that I'll see if I can do anything to improve the readability.
zachgoll (Migrated from github.com) reviewed 2024-04-24 00:33:12 +08:00
app/models/family.rb
@@ -31,0 +69,4 @@
income_series: TimeSeries.new(income, favorable_direction: "up"),
spending_series: TimeSeries.new(spending, favorable_direction: "down")
}
end
zachgoll (Migrated from github.com) commented 2024-04-24 00:33:12 +08:00
Copy Link

Sounds good. Yeah, the NW chart benefits from having the account_balances table to read from which is "normalized" (which includes currency conversion) during the account sync process, which makes it easier to just filter by the family's currency (and assume any account balances that aren't in this subset couldn't be converted).

This one is definitely tricky since we have to normalize and convert on-the-fly

Sounds good. Yeah, the NW chart benefits from having the `account_balances` table to read from which is "normalized" (which includes currency conversion) during the account sync process, which makes it easier to just filter by the family's currency (and assume any account balances that aren't in this subset couldn't be converted). This one is definitely tricky since we have to normalize and convert on-the-fly
zachgoll (Migrated from github.com) reviewed 2024-04-24 00:41:48 +08:00
app/models/family.rb
@@ -31,0 +69,4 @@
income_series: TimeSeries.new(income, favorable_direction: "up"),
spending_series: TimeSeries.new(spending, favorable_direction: "down")
}
end
zachgoll (Migrated from github.com) commented 2024-04-24 00:41:47 +08:00
Copy Link

@JoshAntBrown I had previously experimented around with a DB table called family_snapshots that would essentially act as a daily "cache" table for these sorts of calculations (similar to account_balances):

date, net_worth, assets, liabilities, rolling_30_day_income, rolling_30_day_expense

We'd essentially be doing the same calculations here but rather than on-the-fly we'd have a daily cron running to calculate them and then here we'd just read the values.

Not sure if that would really make any of this easier but thought I'd float the idea in case it helped with this one at all.

@JoshAntBrown I had previously experimented around with a DB table called `family_snapshots` that would essentially act as a daily "cache" table for these sorts of calculations (similar to `account_balances`): ``` date, net_worth, assets, liabilities, rolling_30_day_income, rolling_30_day_expense ``` We'd essentially be doing the same calculations here but rather than on-the-fly we'd have a daily cron running to calculate them and then here we'd just read the values. Not sure if that would really make any of this easier but thought I'd float the idea in case it helped with this one at all.
JoshAntBrown commented 2024-04-24 04:53:47 +08:00 (Migrated from github.com)
Copy Link

I've had a bit of a refactor and added a test in a similar fashion to the other account snapshot.

Hopefully this is a bit more manageable now without straying too far from Active Record conventions.

Here's my copy of the test cases, updated to include spending and income on a rolling basis:
https://docs.google.com/spreadsheets/d/112tRiy0l8WWngEGMdEX2JprYJmYWUW--B2UDUiW9nOE/edit

Here's how it looks with the demo data:
Screenshot 2024-04-23 at 21 55 47

I've had a bit of a refactor and added a test in a similar fashion to the other account snapshot. Hopefully this is a bit more manageable now without straying too far from Active Record conventions. Here's my copy of the test cases, updated to include spending and income on a rolling basis: https://docs.google.com/spreadsheets/d/112tRiy0l8WWngEGMdEX2JprYJmYWUW--B2UDUiW9nOE/edit Here's how it looks with the demo data: <img width="1511" alt="Screenshot 2024-04-23 at 21 55 47" src="https://github.com/maybe-finance/maybe/assets/1793797/f96c15e0-dde0-4a24-bd2d-89d8442c0def">
zachgoll (Migrated from github.com) approved these changes 2024-04-24 20:29:39 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

Looks great, thanks for adding those test cases! I have copy/pasted the expected results into that main spreadsheet for now and we'll eventually get all those into an easier format to collaborate on.

Looks great, thanks for adding those test cases! I have copy/pasted the expected results into that main spreadsheet for now and we'll eventually get all those into an easier format to collaborate on.
Samuelodan commented 2024-04-24 21:13:53 +08:00 (Migrated from github.com)
Copy Link

Ah, that's an interesting solution. I was starting to get a hang of the models, the associations between them, and the TimeSeries, but even if I got a few more days, I doubt I would've completed the feature.

I guess I'll hang around in this repo and slowly learn how the whole thing is put together.

You all are doing amazing work.
Thanks for the opportunity to contribute. ✨

Ah, that's an interesting solution. I was starting to get a hang of the models, the associations between them, and the TimeSeries, but even if I got a few more days, I doubt I would've completed the feature. I guess I'll hang around in this repo and slowly learn how the whole thing is put together. You all are doing amazing work. Thanks for the opportunity to contribute. ✨
Sign in to join this conversation.
Reviewers
No Reviewers
JoshAntBrown
zachgoll
Labels
Clear labels
📆 Planned
Not ready to be worked on yet, but planned for future
 💻 Self Hosted only
Issues pertaining to self-hosted versions of Maybe
 ⚙️ Config
ℹ️ Needs info
Further information is requested
 📱 Mobile
Bugs exclusively happening on mobile devices
 1️⃣ High Priority
High priority issues will be worked on next
 🚨 Urgent
Issues that are urgent for app functionality
 3️⃣ Low priority
Contributions accepted, but Maybe team will not be working on this in the near term
 2️⃣ Medium Priority
Community contributions accepted, Maybe team only works on if there are no high priority items open
 ⚡ Performance
Performance issues to address
 🎨 UI / UX
Issues related to UI / UX interactions and design.
 💎 Bounty
💰 Rewarded
📊 Data issue
Issues where the app functionally works, but the data is missing or inaccurate.
 🙋 Bounty claim
$1.5K
Community
This is a great issue for community members to work on
 dependencies
Pull requests that update a dependency file
 Design
Hosted only
Hosted app only
 Required self hoster data migration
Self hosters must run data migrations to resolve this issue
 ruby
Pull requests that update Ruby code
No Label 🙋 Bounty claim
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
gavin
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: gavin/maybe#668
Delete Branch "feature/transaction-insight-cards"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?