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

[matt-fidd]

This PR fixes #348 and takes the approach discussed to add a new payeeName field to the transaction returned to actual.

It is designed to fix any inconsistencies in data returned from GoCardless with regards to the variety of payee fields that are used, and should result in the "best chance" of a human readable and non-transaction specific payee if it is available.

I have tested these with the bank that I originally spotted the issue with (Santander) and payee names are now correctly created in actual. Banks that I can test with that already worked as expected have not regressed and payees are still reflected correctly.

I can not test the custom bank handlers but all I have done to them is make sure they now return a payeeName as this will be required by actual to normalise the transactions on the client side.

I've also had to copy the title util that actual used to format the payee names.

Complimentary PR in Actual is: actualbudget/actual#2721

[psybers]

As mentioned in actualbudget/actual#2721 maybe the field can be named payee instead of payeeName? That way it should work with SimpleFIN too.

[psybers]

And I do wonder, does this function need called on the SimpleFIN side too?

[matt-fidd]

Now that I'm thinking about this again, I'm not sure this is the best approach for gocardless. Previously each custom handler had this payee standardisation done regardless, but this change puts the onus on the contributer to make sure the field is included in the handler.

Maybe I could move the formatPayeeName call into gocardless-service to retain the behaviour from before and run on all transactions by default?

actual-server/src/app-gocardless/services/gocardless-service.js

Line 448 in db6b4e4

getTransactions: async ({ institutionId, accountId, startDate, endDate }) => {

Any thoughts?

[matt-fidd]

Now that I'm thinking about this again, I'm not sure this is the best approach for gocardless. Previously each custom handler had this payee standardisation done regardless, but this change puts the onus on the contributer to make sure the field is included in the handler.

Maybe I could move the formatPayeeName call into gocardless-service to retain the behaviour from before and run on all transactions by default?

actual-server/src/app-gocardless/services/gocardless-service.js

Line 448 in db6b4e4

getTransactions: async ({ institutionId, accountId, startDate, endDate }) => {

Any thoughts?

Actually I reckon the above is the better way to go, it retains the behaviour from before and simplifies bank handlers. Will refactor now.

[psybers]

Since the if/else above sets name to something (possibly), you can avoid the assignment here and simplify it quite a bit in that case it found a name already.

Suggested change

	name =
	name ||
	trans.debtorName ||
	trans.creditorName ||
	trans.remittanceInformationUnstructured ||
	(trans.remittanceInformationUnstructuredArray || []).join(', ') ||
	trans.additionalInformation;
	if (!name) {
	name =
	trans.remittanceInformationUnstructured ||
	(trans.remittanceInformationUnstructuredArray || []).join(', ') ||
	trans.additionalInformation;
	}

[matt-fidd]

This changes the logic, the key functional part of this change is to allow creditorName to be used even if GoCardless /should/ be using debtorName instead and vice versa.

Happy to put the whole assignment into an if and drop the first name || line if that aligns more closely with the style in this project?

[psybers]

That is curious logic! I guess if that is what needs to happen for GoCardless, the existing code is probably fine. I am not sure if there is a style preference between using an if to avoid name = name or leave it as is.

[MatissJanis]

💬 suggestion: ‏instead of doing this uniformly for ALL banks the same way - I think we should allow the bank-providers to do their own payee normalisation (if they wish to).

So my suggestion here is to move this payee name normalisation inside here.

Other than that - LGTM!

[matt-fidd]

That was my initial approach but I turned away from it because this way replicates the behaviour prior to this change.
If needed creditorName/debtorName can be altered in the bank handler to change the payee name, but I think that the default behaviour to fall back on other fields and append the IBAN when present is best retained.

Let me know what you reckon, happy to change if you disagree.

[MatissJanis]

We can still keep the same behavior as before. But just have the normalization logic in a different location (the generic bank handler + the overrides).

IMO that would be more future proof solution since it would allow to change this logic on a per-bank basis.

[MatissJanis]

👏 praise: ‏thanks!

[MatissJanis]

Before merging this in: @psybers do you have any other feedback here? Or good to go on your end too?

[psybers]

@MatissJanis I think for now this is fine. I am a bit worried about having payeeName: formatPayee(transaction) all over the place, and specifically that it might get missed on new bank overrides. But we can probably find a cleaner way to handle that and refactor it down the road.