Show correct payee for scheduled transactions on budgeted accounts page #1379

kyrias · 2023-07-21T18:43:05Z

Previously the "budgeted accounts" page would show the account in the payee column for scheduled transactions. I'm not sure if this is the correct fix since I'm a bit confused by the exact intent with the various variables that appear to be used for multiple things, but this appears to at least fix the issue.

One weird thing I noticed is that scheduled transactions also show up on the "off budget accounts" page but they don't show up on the "all accounts" page.

Resolves #1333. Resolves #1026.

netlify · 2023-07-21T18:43:10Z

✅ Deploy Preview for actualbudget ready!

Name	Link
🔨 Latest commit	`f9b2718`
🔍 Latest deploy log	https://app.netlify.com/sites/actualbudget/deploys/64d2ba10afe6b100086030ae
😎 Deploy Preview	https://deploy-preview-1379.demo.actualbudget.org/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

github-actions · 2023-07-21T18:46:00Z

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
14	2.12 MB → 2.12 MB (-11 B)	-0.00%

Changeset

File	Δ	Size
`src/components/transactions/TransactionsTable.js`	📉 -180 B (-0.44%)	39.79 kB → 39.61 kB

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

No assets were bigger

Smaller

Asset	File Size	% Changed
static/js/wide-components.chunk.js	156.68 kB → 156.67 kB (-11 B)	-0.01%

Unchanged

Asset	File Size	% Changed
static/js/main.js	944.09 kB	0%
static/js/171.chunk.js	396.24 kB	0%
static/media/Inter-italic.var.woff2	239.29 kB	0%
static/media/Inter-roman.var.woff2	221.86 kB	0%
static/js/383.chunk.js	117.35 kB	0%
static/js/reports.chunk.js	40.96 kB	0%
static/js/narrow-components.chunk.js	24.62 kB	0%
static/js/969.chunk.js	12.94 kB	0%
static/js/resize-observer-polyfill.chunk.js	8.12 kB	0%
static/css/main.css	5.82 kB	0%
asset-manifest.json	1.78 kB	0%
index.html	1.66 kB	0%
static/media/browser-server.js	963 B	0%

github-actions · 2023-07-21T18:46:02Z

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
2	1.97 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	1012.13 kB	0%
xfo.xfo.kcab.worker.js	1004.04 kB	0%

j-f1 · 2023-07-24T20:19:27Z

This doesn’t seem to quite work right for me — I don’t see a payee name after switching a schedule from a normal payee to a transfer:

kyrias · 2023-07-24T20:46:24Z

This doesn’t seem to quite work right for me — I don’t see a payee name after switching a schedule from a normal payee to a transfer:

This seems to be due to this condition:

actual/packages/desktop-client/src/components/transactions/TransactionsTable.js

Lines 394 to 398 in 9c527e3

    
           } else if (payee && !payee.transfer_acct) { 
        
             // Check to make sure this isn't a transfer because in the rare 
        
             // occasion that the account has been deleted but the payee is 
        
             // still there, we don't want to show the name. 
        
             return payee.name;

I'm a bit confused by this comment though. From what I can tell payees whose accounts are tombstoned will not be returned by the backend and so should never show up here, so is this just remains from when that might've once not been the case?

Signed-off-by: Johannes Löthberg <[email protected]>

kyrias · 2023-08-08T21:57:13Z

Since, as per my previous comment, the check seems to have been unnecessary since before the code was released as open source, I decided to just remove it.

MatissJanis · 2023-08-09T07:54:06Z

👋 Cool. This seems to fix the payee name, but when you post the transaction - one of the posted transactions doesn't have a schedule icon (on master too).

It's a bit of a scope creep, but would you like to take a look at this thing too as part of this PR?

kyrias · 2023-08-09T14:41:40Z

I see roughly what needs to be done to fix it, but it seems to be a bit more complicated than I originally thought because the status needs to also be updated in both transactions when you link and unlink the transactions to the schedule, hmm.

kyrias · 2023-08-09T15:21:15Z

@MatissJanis Okay, so I think the best way to implement this is in PayeeIcons to check if the transaction has a schedule field, and otherwise if it has a transfer_id then get that transaction and check if that has a schedule field.

But there doesn't seem to be a way to get a specific transaction by ID in the frontend, so do I need to add an e.g. get-transaction handler to the loot-core server to implement this?

MatissJanis · 2023-08-10T16:46:26Z

@kyrias I think that would significantly impact performance for budgets with many schedules.

I actually think this might be a deeper issue in Actual. When we "post" a scheduled transfer transaction it seems that only 1x of the 2x transactions has "schedule id" set. I wonder why that is.. and if we could change it to set the schedule id on both of the transactions.

kyrias · 2023-08-10T16:58:17Z

@MatissJanis That's what I did at first and is easy enough to do, but there are more complicated additional things to consider of we do that:

Linking and unlinking transaction should link the other transaction as well, however the linking or unlinking happens.
It means that both transactions end up listed in the edit screen of the relevant schedule, which is unexpected and looks weird.

MatissJanis · 2023-08-10T18:51:44Z

Good observations. So this is definitely a bigger issue that will be more complex to fix properly. I don't want it to block merging this PR, so I created a separate issue here: #1506

Feel free to work on it if you like to (would definitely be nice), but don't feel obliged. :)

Thanks again!

…ounts page (#1379)" This reverts commit ba6eb26.

…dgeted accounts page (actualbudget#1379)" This partially reverts commit ba6eb26 which was an incorrect fix for the issue as it regresses transfer transactions. The `getPayeePretty` change was still correct however so I've left it in.

…ge (actualbudget#1379) Signed-off-by: Johannes Löthberg <[email protected]>

trafico-bot bot added the 🔍 Ready for Review label Jul 21, 2023

Show correct payee for scheduled transactions on budgeted accounts page

f9b2718

Signed-off-by: Johannes Löthberg <[email protected]>

kyrias force-pushed the fix-payee-on-multi-account-pages branch from 90872ce to f9b2718 Compare August 8, 2023 21:56

MatissJanis added this to the v23.9.0 milestone Aug 9, 2023

MatissJanis mentioned this pull request Aug 10, 2023

[Bug]: scheduled transactions missing schedule icon #1506

Open

1 task

MatissJanis approved these changes Aug 10, 2023

View reviewed changes

MatissJanis merged commit ba6eb26 into actualbudget:master Aug 10, 2023
15 checks passed

kyrias deleted the fix-payee-on-multi-account-pages branch August 10, 2023 18:52

trafico-bot bot added ✨ Merged Pull Request has been merged successfully ✅ Approved and removed 🔍 Ready for Review ✨ Merged Pull Request has been merged successfully labels Aug 10, 2023

MatissJanis added a commit that referenced this pull request Aug 13, 2023

Revert "Show correct payee for scheduled transactions on budgeted acc…

65b54c3

…ounts page (#1379)" This reverts commit ba6eb26.

MatissJanis mentioned this pull request Aug 13, 2023

Revert "Show correct payee for scheduled transactions on budgeted accounts page" #1523

Closed

kyrias mentioned this pull request Aug 14, 2023

Correctly fix schedule transaction payee #1526

Merged

FlorianLang06 pushed a commit to FlorianLang06/actual that referenced this pull request Mar 7, 2024

Show correct payee for scheduled transactions on budgeted accounts pa…

7239b45

…ge (actualbudget#1379) Signed-off-by: Johannes Löthberg <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show correct payee for scheduled transactions on budgeted accounts page #1379

Show correct payee for scheduled transactions on budgeted accounts page #1379

kyrias commented Jul 21, 2023 •

edited

Loading

netlify bot commented Jul 21, 2023 •

edited

Loading

github-actions bot commented Jul 21, 2023 •

edited

Loading

github-actions bot commented Jul 21, 2023 •

edited

Loading

j-f1 commented Jul 24, 2023

kyrias commented Jul 24, 2023

kyrias commented Aug 8, 2023

MatissJanis commented Aug 9, 2023

kyrias commented Aug 9, 2023

kyrias commented Aug 9, 2023

MatissJanis commented Aug 10, 2023

kyrias commented Aug 10, 2023

MatissJanis commented Aug 10, 2023

Show correct payee for scheduled transactions on budgeted accounts page #1379

Show correct payee for scheduled transactions on budgeted accounts page #1379

Conversation

kyrias commented Jul 21, 2023 • edited Loading

netlify bot commented Jul 21, 2023 • edited Loading

✅ Deploy Preview for actualbudget ready!

github-actions bot commented Jul 21, 2023 • edited Loading

Bundle Stats — desktop-client

github-actions bot commented Jul 21, 2023 • edited Loading

Bundle Stats — loot-core

j-f1 commented Jul 24, 2023

kyrias commented Jul 24, 2023

kyrias commented Aug 8, 2023

MatissJanis commented Aug 9, 2023

kyrias commented Aug 9, 2023

kyrias commented Aug 9, 2023

MatissJanis commented Aug 10, 2023

kyrias commented Aug 10, 2023

MatissJanis commented Aug 10, 2023

kyrias commented Jul 21, 2023 •

edited

Loading

netlify bot commented Jul 21, 2023 •

edited

Loading

github-actions bot commented Jul 21, 2023 •

edited

Loading

github-actions bot commented Jul 21, 2023 •

edited

Loading