Move bank sync payee name normalisation from actual to actual-server
#2721
Conversation
github-actions
bot
changed the title
actual to actual-serveractual to actual-server
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
trafico-bot
bot
added
🚧 WIP
Bundle Stats — desktop-clientHey 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
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey 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
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller
Unchanged No assets were unchanged |
trafico-bot
bot
added
🚧 WIP
matt-fidd
changed the title
actual to actual-serveractual to actual-server
trafico-bot
bot
added
🔍 Ready for Review
| if (trans.payeeName == null) { | ||
| throw new Error('`payeeName` is required when adding a transaction'); | ||
| } |
There was a problem hiding this comment.
This will break SimpleFIN support as it does not provide this field.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Maybe the issue is that the SimpleFIN server code needs to call the moved function and generate a payeeName?
There was a problem hiding this comment.
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?
Happy to switch approaches if needed?
There was a problem hiding this comment.
So something like newTrans.payeeName = formatPayeeName(trans); instead.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, definitely - good spot. Will get the util moved down a directory and call from both.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Should be fixed in actual-server now, util moved up a directory and run on the simplefin transacion
trafico-bot
bot
added
⚠️ Changes requested
trafico-bot
bot
added
🔍 Ready for Review
trafico-bot
bot
added
⚠️ Changes requested
trafico-bot
bot
added
🔍 Ready for Review
This PR makes the required changes to
actualto move the bank sync payee normalisation intoactual-server.Discussion issue: actualbudget/actual-server#348
actual-serverPR: actualbudget/actual-server#353