Move bank sync payee name normalisation from `actual` to `actual-server` #2721

matt-fidd · 2024-05-06T23:13:02Z

This PR makes the required changes to actual to move the bank sync payee normalisation into actual-server.

Discussion issue: actualbudget/actual-server#348

actual-server PR: actualbudget/actual-server#353

netlify · 2024-05-06T23:13:18Z

✅ Deploy Preview for actualbudget ready!

Name	Link
🔨 Latest commit	`38c8d0d`
🔍 Latest deploy log	https://app.netlify.com/sites/actualbudget/deploys/6685999ac7d5d80008d5d089
😎 Deploy Preview	https://deploy-preview-2721.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 · 2024-05-06T23:14:14Z

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
9	4.91 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
static/js/resize-observer.js	18.37 kB	0%
static/js/indexeddb-main-thread-worker-e59fee74.js	13.5 kB	0%
static/js/usePreviewTransactions.js	790 B	0%
static/js/AppliedFilters.js	27.12 kB	0%
static/js/BackgroundImage.js	122.29 kB	0%
static/js/narrow.js	76.96 kB	0%
static/js/wide.js	273.19 kB	0%
static/js/ReportRouter.js	1.23 MB	0%
static/js/index.js	3.16 MB	0%

github-actions · 2024-05-06T23:14:15Z

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.13 MB → 1.13 MB (-555 B)	-0.05%

Changeset

File	Δ	Size
`packages/loot-core/src/server/accounts/sync.ts`	📉 -1.27 kB (-5.76%)	22.04 kB → 20.77 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
kcab.worker.js	1.13 MB → 1.13 MB (-555 B)	-0.05%

Unchanged

No assets were unchanged

psybers · 2024-05-25T17:05:25Z

packages/loot-core/src/server/accounts/sync.ts

+    if (trans.payeeName == null) {
+      throw new Error('`payeeName` is required when adding a transaction');
    }


This will break SimpleFIN support as it does not provide this field.

Note that SimpleFIN seems to provide payee as a field. I am not familiar with the GoCardless side, but could it just be renamed to payee instead of payeeName?

Maybe the issue is that the SimpleFIN server code needs to call the moved function and generate a payeeName?

I'm not sure if we'd want to overwrite an field already provided by the APIs, so I added payeeName to the simplefin /transactions endpoint in the actual-server complimentary PR. Could you try running this PR with the server PR (actualbudget/actual-server#353) to confirm if this issue persists?

https://github.com/actualbudget/actual-server/blob/ffdc9684201d3d342dcabf75f23ad8532426b81b/src/app-simplefin/app-simplefin.js#L112

Happy to switch approaches if needed?

So something like newTrans.payeeName = formatPayeeName(trans); instead.

I am not entirely sure the SimpleFIN transactions will actually change the name via that function, but at least the same code path will run as it does currently.

Yes, definitely - good spot. Will get the util moved down a directory and call from both.

That might also imply the function should live in another folder, instead of under the GoCardless code, since it is shared across two different bank syncs.

Should be fixed in actual-server now, util moved up a directory and run on the simplefin transacion

packages/loot-core/src/server/accounts/sync.ts

psybers · 2024-06-01T03:19:40Z

Since the utils for title() were moved to the server, should they be removed here?

matt-fidd · 2024-06-01T11:00:52Z

Since the utils for title() were moved to the server, should they be removed here?

It's still used in another function further up normalizeTransactions()

actual/packages/loot-core/src/server/accounts/sync.ts

Lines 205 to 213 in 08aff05

    
           let payee_name = originalPayeeName; 
        
           if (payee_name) { 
        
             const trimmed = payee_name.trim(); 
        
             if (trimmed === '') { 
        
               payee_name = null; 
        
             } else { 
        
               payee_name = rawPayeeName ? trimmed : title(trimmed); 
        
             } 
        
           }

psybers · 2024-06-01T20:53:59Z

Since the utils for title() were moved to the server, should they be removed here?

It's still used in another function further up normalizeTransactions()

actual/packages/loot-core/src/server/accounts/sync.ts

Lines 205 to 213 in 08aff05

let payee_name = originalPayeeName;

if (payee_name) {

const trimmed = payee_name.trim();

if (trimmed === '') {

payee_name = null;

} else {

payee_name = rawPayeeName ? trimmed : title(trimmed);

}

}

Ah, that is too bad. So now there is duplicated code across actual-server and actual. Is there a way to avoid that duplication?

matt-fidd · 2024-06-17T08:45:09Z

Since the utils for title() were moved to the server, should they be removed here?

It's still used in another function further up normalizeTransactions()

actual/packages/loot-core/src/server/accounts/sync.ts

Lines 205 to 213 in 08aff05

let payee_name = originalPayeeName;

if (payee_name) {

const trimmed = payee_name.trim();

if (trimmed === '') {

payee_name = null;

} else {

payee_name = rawPayeeName ? trimmed : title(trimmed);

}

}

Ah, that is too bad. So now there is duplicated code across actual-server and actual. Is there a way to avoid that duplication?

Unfortunately I can't think of a nice way out of this, apart from exporting it as part of actual then importing into actual-server but that feels "not so great" either.

I would welcome an alternative approach for anyone more skilled in this area

joel-jeremy · 2024-06-21T17:41:57Z

I think we should merge this for now. We don't really have a shared library between frontend and backend and I don't think we need to create one right now. Thoughts @psybers?

psybers · 2024-06-21T17:45:02Z

I think we should merge this for now. We don't really have a shared library between frontend and backend and I don't think we need to create one right now. Thoughts @psybers?

@joel-jeremy I'm totally fine with that. Was just pointing out the issue.

joel-jeremy

LGTM!

MatissJanis

LGTM, but this has to be merged at the same time as the actual-server change.

github-actions bot changed the title ~~Move bank sync payee name normalisation from actual to actual-server~~ [WIP] Move bank sync payee name normalisation from actual to actual-server May 6, 2024

trafico-bot bot added 🚧 WIP and removed 🚧 WIP labels May 6, 2024

trafico-bot bot added 🚧 WIP and removed 🔍 Ready for Review labels May 6, 2024

matt-fidd changed the title ~~[WIP] Move bank sync payee name normalisation from actual to actual-server~~ Move bank sync payee name normalisation from actual to actual-server May 6, 2024

trafico-bot bot added 🔍 Ready for Review and removed 🚧 WIP labels May 6, 2024

This was referenced May 15, 2024

Move bank sync payee name normalisation from actual to actual-server actualbudget/actual-server#353

Merged

Use GoCardless remittanceInformationStructured for notes if available #2760

Closed

psybers suggested changes May 25, 2024

View reviewed changes

trafico-bot bot added ⚠️ Changes requested Pull Request needs changes before it can be reviewed again and removed 🔍 Ready for Review labels May 25, 2024

matt-fidd added 2 commits May 25, 2024 17:26

Move payee name normalization from actual to actual-server

69bf211

release note

34773a1

matt-fidd force-pushed the payee-name branch from 347a257 to 34773a1 Compare May 25, 2024 17:44

trafico-bot bot added 🔍 Ready for Review and removed ⚠️ Changes requested Pull Request needs changes before it can be reviewed again labels May 25, 2024

psybers suggested changes May 25, 2024

View reviewed changes

packages/loot-core/src/server/accounts/sync.ts Outdated Show resolved Hide resolved

trafico-bot bot added ⚠️ Changes requested Pull Request needs changes before it can be reviewed again and removed 🔍 Ready for Review labels May 25, 2024

fix unnecessary variable payee_name assignment

1cf4f72

trafico-bot bot added 🔍 Ready for Review and removed ⚠️ Changes requested Pull Request needs changes before it can be reviewed again labels May 25, 2024

Merge branch 'master' into payee-name

311c756

matt-fidd added 2 commits June 11, 2024 00:30

Merge branch 'master' into payee-name

14a8a48

Merge branch 'master' into payee-name

cc9ad31

matt-fidd added 3 commits June 21, 2024 19:35

Merge branch 'master' into payee-name

9938aeb

Merge branch 'master' into payee-name

9a894a4

Merge branch 'master' into payee-name

eccce6a

joel-jeremy approved these changes Jun 27, 2024

View reviewed changes

Merge branch 'master' into payee-name

457e6ae

github-actions bot added ✅ Approved and removed 🔍 Ready for Review labels Jul 1, 2024

Merge branch 'master' into payee-name

38c8d0d

MatissJanis approved these changes Jul 7, 2024

View reviewed changes

MatissJanis merged commit 771c01c into actualbudget:master Jul 9, 2024
20 checks passed

github-actions bot added ✨ Merged Pull Request has been merged successfully and removed ✅ Approved labels Jul 9, 2024

matt-fidd deleted the payee-name branch July 29, 2024 17:25

psybers mentioned this pull request Aug 7, 2024

[Bug]: Wrong payeeNamee since 24.8.0 (logic) #3216

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move bank sync payee name normalisation from `actual` to `actual-server` #2721

Move bank sync payee name normalisation from `actual` to `actual-server` #2721

matt-fidd commented May 6, 2024

netlify bot commented May 6, 2024 •

edited

Loading

github-actions bot commented May 6, 2024 •

edited

Loading

github-actions bot commented May 6, 2024 •

edited

Loading

psybers May 25, 2024

psybers May 25, 2024

psybers May 25, 2024

psybers May 25, 2024

matt-fidd May 25, 2024

psybers May 25, 2024

psybers May 25, 2024

matt-fidd May 25, 2024

psybers May 25, 2024

matt-fidd May 25, 2024

psybers commented Jun 1, 2024

matt-fidd commented Jun 1, 2024

psybers commented Jun 1, 2024

matt-fidd commented Jun 17, 2024

joel-jeremy commented Jun 21, 2024

psybers commented Jun 21, 2024

joel-jeremy left a comment

MatissJanis left a comment

Move bank sync payee name normalisation from actual to actual-server #2721

Move bank sync payee name normalisation from actual to actual-server #2721

Conversation

matt-fidd commented May 6, 2024

netlify bot commented May 6, 2024 • edited Loading

✅ Deploy Preview for actualbudget ready!

github-actions bot commented May 6, 2024 • edited Loading

Bundle Stats — desktop-client

github-actions bot commented May 6, 2024 • edited Loading

Bundle Stats — loot-core

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psybers commented Jun 1, 2024

matt-fidd commented Jun 1, 2024

psybers commented Jun 1, 2024

matt-fidd commented Jun 17, 2024

joel-jeremy commented Jun 21, 2024

psybers commented Jun 21, 2024

joel-jeremy left a comment

Choose a reason for hiding this comment

MatissJanis left a comment

Choose a reason for hiding this comment

Move bank sync payee name normalisation from `actual` to `actual-server` #2721

Move bank sync payee name normalisation from `actual` to `actual-server` #2721

netlify bot commented May 6, 2024 •

edited

Loading

github-actions bot commented May 6, 2024 •

edited

Loading

github-actions bot commented May 6, 2024 •

edited

Loading