Include additionalInformation in notes #1490
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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
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
Smaller No assets were smaller Unchanged
|
There was a problem hiding this comment.
👋
The additionalInformation field is localized to each bank. There is no standard for what we get in this field. For example: my bank just returns an ID of some sort in this field.
Instead of applying a patch in the UI - what you can do is create a custom mapper for your specific bank in the server. Here's an example how it can be done: actualbudget/actual-server#239
trafico-bot
bot
added
⚠️ Changes requested
Hi, thanks for the review. |
Don't build it in the default bank integration as the The existing mappers are here: https://github.com/actualbudget/actual-server/tree/c78bb084de618e89256db17d1114f290b0de73f6/src/app-gocardless/banks |
| export function looselyParseAdditionalInformation(additionalInformation) { | ||
| let additionalInformationJson; | ||
| try { | ||
| additionalInformationJson = JSON.parse(additionalInformation); | ||
| } catch (e) { | ||
| try { | ||
| // It was not valid JSON, attempt to make the payload valid JSON using regex. | ||
| let result = {}; | ||
| let matches = additionalInformation.matchAll( | ||
| /(, )?([^:]+): ((\[.*?\])|([^,]*))/g, | ||
| ); | ||
| for (let match of matches) { | ||
| let key = match[2].trim(); | ||
| let value = (match[4] || match[5]).trim(); | ||
| // Remove square brackets and single quotes and commas | ||
| value = value.replace(/[[\]',]/g, ''); | ||
| result[key] = value; | ||
| } | ||
| additionalInformationJson = result; | ||
| } catch (e) { | ||
| additionalInformationJson = {}; | ||
| } | ||
| } | ||
| return additionalInformationJson; | ||
| } |
There was a problem hiding this comment.
A couple of things:
- I've never heard of a single case in which this field would contain serialized JSON, so trying to parse it as JSON seems pointless to me.
- I'm not really a fan of using regex to try to parse something like this, especially without a specification of the actual format used that we can validate it against. Your current regexes would break if e.g. one of the fields can contain
], which without a specification we don't know if it's valid.
Though to be clear, to do that kind of mapping you need the normalization of transaction commit that's currently in both of my bank integration PRs, so to start working on it you'll need to base it off of one of those two branches. |
Ok, the normalizetransaction makes sense. I'll start from actualbudget/actual-server#237. Regarding your review: I assumed some banks would return JSON, but as I will now try to switch to a custom bank mapper, that argument is irrelevant. Then again, what alternatives would you recommend to parsing it with regex? |
|
Ok, I propose to close this pull request in favor of actualbudget/actual-server#242 |
|
Thanks @CharlieMK ! |
Implements #1466
Nothing changes for users who use a bank that does not use the additionalInformation field. For banks that do, the remittanceInformation is combined with the information from the atmPosName and narrative subfields of the additionalInformation field.