8000 Do not include income transactions in liability accounts for savings rate by tonyvince · Pull Request #1385 · maybe-finance/maybe · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Do not include income transactions in liability accounts for savings rate #1385

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 31, 2024

Conversation

tonyvince
Copy link
Contributor
@tonyvince tonyvince commented Oct 28, 2024

Fix #1383

@tonyvince tonyvince changed the title Do not include income transactions in liability accounts for savings … Do not include income transactions in liability accounts for savings rate Oct 28, 2024
@tonyvince
Copy link
Contributor Author

@zachgoll what do you think about this approach?

@@ -17,6 +17,7 @@ class Account::Entry < ApplicationRecord
scope :chronological, -> { order(:date, :created_at) }
scope :reverse_chronological, -> { order(date: :desc, created_at: :desc) }
scope :without_transfers, -> { where(marked_as_transfer: false) }
scope :without_income_transactions_in_liability_accounts, -> { joins(:account).where(amount: 0..).where.not(accounts: { classification: Account.classifications[:liability] }).distinct }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're on the right track here, but I wouldn't make a scope for this given how specific it is to the snapshot_transactions method.

Also, "Income" would be anything less than 0, so I think we need to flip that condition here

@tonyvince tonyvince marked this pull request as ready for review October 30, 2024 12:58
@tonyvince tonyvince requested a review from zachgoll October 30, 2024 12:59
Copy link
Collaborator
@zachgoll zachgoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I think this should fix it. We'll likely have other little edge cases pop up but I think this is a good step forward.

@zachgoll zachgoll merged commit 2b61821 into maybe-finance:main Oct 31, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Liability accounts are included in "Savings Rate" insight card on dashboard
2 participants
0