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.

Sign up for GitHub

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

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

Merged
merged 11 commits into from
Jul 9, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
69bf211
Move payee name normalization from actual to actual-server
matt-fidd May 6, 2024
34773a1
release note
matt-fidd May 6, 2024
1cf4f72
fix unnecessary variable payee_name assignment
matt-fidd May 25, 2024
311c756
Merge branch 'master' into payee-name
matt-fidd May 31, 2024
14a8a48
Merge branch 'master' into payee-name
matt-fidd Jun 10, 2024
cc9ad31
Merge branch 'master' into payee-name
matt-fidd Jun 17, 2024
9938aeb
Merge branch 'master' into payee-name
matt-fidd Jun 21, 2024
9a894a4
Merge branch 'master' into payee-name
matt-fidd Jun 22, 2024
eccce6a
Merge branch 'master' into payee-name
matt-fidd Jun 25, 2024
457e6ae
Merge branch 'master' into payee-name
matt-fidd Jul 1, 2024
38c8d0d
Merge branch 'master' into payee-name
matt-fidd Jul 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 5 additions & 49 deletions packages/loot-core/src/server/accounts/sync.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,55 +251,11 @@ async function normalizeBankSyncTransactions(transactions, acctId) {
throw new Error('`date` is required when adding a transaction');
}

let payee_name;
// When the amount is equal to 0, we need to determine
// if this is a "Credited" or "Debited" transaction. This means
// that it matters whether the amount is a positive or negative zero.
if (trans.amount > 0 || Object.is(Number(trans.amount), 0)) {
const nameParts = [];
const name =
trans.debtorName ||
trans.remittanceInformationUnstructured ||
(trans.remittanceInformationUnstructuredArray || []).join(', ') ||
trans.additionalInformation;

if (name) {
nameParts.push(title(name));
}
if (trans.debtorAccount && trans.debtorAccount.iban) {
nameParts.push(
'(' +
trans.debtorAccount.iban.slice(0, 4) +
' XXX ' +
trans.debtorAccount.iban.slice(-4) +
')',
);
}
payee_name = nameParts.join(' ');
} else {
const nameParts = [];
const name =
trans.creditorName ||
trans.remittanceInformationUnstructured ||
(trans.remittanceInformationUnstructuredArray || []).join(', ') ||
trans.additionalInformation;

if (name) {
nameParts.push(title(name));
}
if (trans.creditorAccount && trans.creditorAccount.iban) {
nameParts.push(
'(' +
trans.creditorAccount.iban.slice(0, 4) +
' XXX ' +
trans.creditorAccount.iban.slice(-4) +
')',
);
}
payee_name = nameParts.join(' ');
if (trans.payeeName == null) {
throw new Error('`payeeName` is required when adding a transaction');
}
Comment on lines +254 to 256
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

image

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


trans.imported_payee = trans.imported_payee || payee_name;
trans.imported_payee = trans.imported_payee || trans.payeeName;
if (trans.imported_payee) {
trans.imported_payee = trans.imported_payee.trim();
}
Expand All @@ -308,12 +264,12 @@ async function normalizeBankSyncTransactions(transactions, acctId) {
// when rules are run, they have the right data. Resolving payees
// also simplifies the payee creation process
trans.account = acctId;
trans.payee = await resolvePayee(trans, payee_name, payeesToCreate);
trans.payee = await resolvePayee(trans, trans.payeeName, payeesToCreate);

trans.cleared = Boolean(trans.booked);

normalized.push({
payee_name,
payee_name: trans.payeeName,
trans: {
amount: amountToInteger(trans.amount),
payee: trans.payee,
Expand Down
6 changes: 6 additions & 0 deletions upcoming-release-notes/2721.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
category: Maintenance
authors: [matt-fidd]
---

Move bank sync payee name normalization from actual to actual-server
Loading