Skip to content
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

Add split distribution #2151

Merged
merged 9 commits into from
Jan 13, 2024

Conversation

NikxDa
Copy link
Contributor

@NikxDa NikxDa commented Jan 1, 2024

This PR adds support for distributing the remaining balance of a split over all empty splits. This feature was suggested in #1324 and partially implemented in #1271 but never merged.

Examples:

  • A transaction has an amount of $200, and three splits each with no amount. Clicking "Distribute" will assign $66.66 to the first two, and $66.67 to the third split, automatically.
  • A transaction has an amount of $200, and three splits, two of which have no amount. The third split has an amount of $10. Clicking "Distribute" will assign $95 to the two empty splits.

Demo:

CleanShot 2024-01-01 at 17 01 55

As far as I can tell, this is now ready to merge and includes the functionality for both the new transaction editor as well as in existing transactions.

Copy link

netlify bot commented Jan 1, 2024

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit 84e8422
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/65a31c4645ef150008cc710c
😎 Deploy Preview https://deploy-preview-2151.demo.actualbudget.org/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

github-actions bot commented Jan 1, 2024

Bundle Stats — desktop-client

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count Total bundle size % Changed
10 5.08 MB → 5.08 MB (+1.37 kB) +0.03%
Changeset
File Δ Size
src/components/transactions/TransactionsTable.jsx 📈 +2.7 kB (+5.16%) 52.19 kB → 54.88 kB
src/components/transactions/SelectedTransactions.jsx 📉 -51 B (-1.04%) 4.79 kB → 4.74 kB
src/components/schedules/ScheduleDetails.jsx 📉 -467 B (-1.67%) 27.24 kB → 26.78 kB
src/components/Modals.tsx 📉 -145 B (-1.72%) 8.22 kB → 8.08 kB
src/components/schedules/ScheduleLink.tsx 📉 -697 B (-23.93%) 2.84 kB → 2.16 kB
View detailed bundle breakdown

Added

Asset File Size % Changed
static/js/index-_yXEToBT.js 0 B → 2.63 MB (+2.63 MB) -
static/js/ReportRouter-6Yb6g6gJ.js 0 B → 1.95 MB (+1.95 MB) -
static/js/wide-fQM9kMtE.js 0 B → 238.99 kB (+238.99 kB) -
static/js/BackgroundImage-SinEaH_n.js 0 B → 122.29 kB (+122.29 kB) -
static/js/narrow-c3J4zwWM.js 0 B → 82.17 kB (+82.17 kB) -
static/js/FiltersMenu-FCXTliQP.js 0 B → 28.92 kB (+28.92 kB) -
static/js/BalanceTooltip-UwWirK_r.js 0 B → 6.07 kB (+6.07 kB) -
static/js/ButtonLink-EX8Rc8ei.js 0 B → 379 B (+379 B) -

Removed

Asset File Size % Changed
static/js/index-dF4_yrex.js 2.63 MB → 0 B (-2.63 MB) -100%
static/js/ReportRouter-gSSaNa0n.js 1.95 MB → 0 B (-1.95 MB) -100%
static/js/wide-cFb56fDi.js 236.35 kB → 0 B (-236.35 kB) -100%
static/js/BackgroundImage-1Yws_2oV.js 122.29 kB → 0 B (-122.29 kB) -100%
static/js/narrow-3-QpUkOh.js 82.17 kB → 0 B (-82.17 kB) -100%
static/js/FiltersMenu-VztzRwUc.js 28.92 kB → 0 B (-28.92 kB) -100%
static/js/BalanceTooltip-m6XuKOox.js 6.07 kB → 0 B (-6.07 kB) -100%
static/js/ButtonLink-N_7fj7Zs.js 379 B → 0 B (-379 B) -100%

Bigger

No assets were bigger

Smaller

No assets were smaller

Unchanged

Asset File Size % Changed
static/js/resize-observer-6FaCDon1.js 18.37 kB 0%
static/js/indexeddb-main-thread-worker-e59fee74-bhyX5Ad6.js 13.5 kB 0%

Copy link
Contributor

github-actions bot commented Jan 1, 2024

Bundle Stats — loot-core

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count Total bundle size % Changed
1 1.23 MB 0%

Changeset

No files were changed

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

No assets were bigger

Smaller

No assets were smaller

Unchanged

Asset File Size % Changed
kcab.worker.js 1.23 MB 0%

@youngcw youngcw linked an issue Jan 1, 2024 that may be closed by this pull request
2 tasks
@youngcw
Copy link
Member

youngcw commented Jan 1, 2024

Nice. This is working in my quick testing

@MatissJanis
Copy link
Member

👋 This feels like it's solving a very edge-case problem. Why is the logic to split them equally among the many transactions? The vast majority of purchases (for example: groceries) will not have items with equal value. The amounts will vary drastically. Hence why this feels to me like it's solving an edge-case problem that won't be applicable to most transactions and use-cases. Which is why I would argue against merging this.

However, I would not be against a "fill remaining balance" type button if only a single split transaction remains with empty balance (in which case it's simple to calculate the remaining balance and assign that).

@NikxDa
Copy link
Contributor Author

NikxDa commented Jan 7, 2024

@MatissJanis I strongly disagree that this is an edge case. Whenever you budget an expense into a reimbursement category, whether it be a set of concert tickets among friends, or a grocery trip with your partner, the amount needs to be split not by individual item but equally.

Examples:

  • Grocery shopping with a partner
  • Flight tickets for a group of N friends
  • Gas for a road trip with N friends

Introducing a "Fill remaining" button seems less useful to me, as the remaining balance is already displayed. Typing that in again is muss less effort than dividing random amounts by two, three, etc.. What makes this especially tedious is that if divisions don't work out, one needs to manually figure out the single cent remainders...

If you really think this isn't a mainstream problem, then can we put this behind a flag in the settings by chance? I know 4 people using Actual including me, and we could all regularly use this.

Personally, I think this would be fine. If its not needed, it's certainly not in the way.

@youngcw
Copy link
Member

youngcw commented Jan 8, 2024

@NikxDa Can you look into why the test is failing?

@MatissJanis
Copy link
Member

Thanks. Upon further thought I've reconsidered. Having a button that fills in the remaining amount for the last split transaction (1x) makes sense. And since we'd have that button.. we might as well extend it to do equal splits for multiple split transactions.

@NikxDa
Copy link
Contributor Author

NikxDa commented Jan 8, 2024

@MatissJanis Thanks, I'm glad to hear it! I can try to make the button text dynamic, as in, if there is only one split left, it will display "Fill", whereas if there are multiple it will display "Distribute" (could make it work so that if all splits are set but don't add up, it displays "Add remaining" and adds the extra split).

@youngcw I will check the tests shortly. Adding splits still works fine, so I'm not sure what's happening there.

@NikxDa
Copy link
Contributor Author

NikxDa commented Jan 10, 2024

Managed to fix the remaining test. Unclear to me how to fix the visual regression tests, any guidance appreciated.

@youngcw
Copy link
Member

youngcw commented Jan 10, 2024

This explains how to update the visual regression snapshots. https://github.com/actualbudget/actual/blob/master/packages/desktop-client/README.md

youngcw
youngcw previously approved these changes Jan 10, 2024
@NikxDa
Copy link
Contributor Author

NikxDa commented Jan 10, 2024

@youngcw Can you review again, please? I've added some code so that the "Distribute" button is disabled when there are no empty transactions, because it would appear to do nothing in that case.

youngcw
youngcw previously approved these changes Jan 11, 2024
@joel-jeremy joel-jeremy merged commit 44a5199 into actualbudget:master Jan 13, 2024
19 checks passed
@trafico-bot trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed ✅ Approved labels Jan 13, 2024
@github-actions github-actions bot mentioned this pull request Jan 13, 2024
2 tasks
@NikxDa NikxDa deleted the feature/distribute-splits branch January 29, 2024 20:02
@MattDemers
Copy link

This is cool; I wonder if there could be some kind of option that distributes the remaining amount proportionally to all the split categories, in order to cover a usecase for distributing the subtotal among categories, but having the tax amount left over.

I referenced that here.

FlorianLang06 pushed a commit to FlorianLang06/actual that referenced this pull request Mar 7, 2024
* Add split distribution feature

* Add upcoming release notes

* Fix tests

* Fix remaining test

* Disable distribute button when all transactions are filled

* Add canDistributeRemainder

---------

Co-authored-by: Matiss Janis Aboltins <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Merged Pull Request has been merged successfully
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Split into equal parts
5 participants