Move bank sync payee name normalisation from actual to actual-server
#353
Conversation
matt-fidd
changed the title
actual to actual-serveractual to actual-server
trafico-bot
bot
added
🚧 WIP
matt-fidd
force-pushed
the
payee-name
branch
2 times, most recently
from
27966c5
to
819f7ce
Compare
matt-fidd
changed the title
actual to actual-serveractual to actual-server
trafico-bot
bot
added
🔍 Ready for Review
|
As mentioned in actualbudget/actual#2721 maybe the field can be named |
|
And I do wonder, does this function need called on the SimpleFIN side too? |
…erring the correct field first
|
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 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. |
trafico-bot
bot
added
⚠️ Changes requested
trafico-bot
bot
added
🔍 Ready for Review
| name = | ||
| name || | ||
| trans.debtorName || | ||
| trans.creditorName || | ||
| trans.remittanceInformationUnstructured || | ||
| (trans.remittanceInformationUnstructuredArray || []).join(', ') || | ||
| trans.additionalInformation; |
There was a problem hiding this comment.
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.
| name = | |
| name || | |
| trans.debtorName || | |
| trans.creditorName || | |
| trans.remittanceInformationUnstructured || | |
| (trans.remittanceInformationUnstructuredArray || []).join(', ') || | |
| trans.additionalInformation; | |
| if (!name) { | |
| name = | |
| trans.remittanceInformationUnstructured || | |
| (trans.remittanceInformationUnstructuredArray || []).join(', ') || | |
| trans.additionalInformation; | |
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
trafico-bot
bot
added
⚠️ Changes requested
trafico-bot
bot
added
🔍 Ready for Review
This PR fixes #348 and takes the approach discussed to add a new
payeeNamefield to the transaction returned toactual.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
payeeNameas this will be required byactualto normalise the transactions on the client side.I've also had to copy the
titleutil thatactualused to format the payee names.Complimentary PR in Actual is: actualbudget/actual#2721