-
-
Notifications
You must be signed in to change notification settings - Fork 10
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.
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
martin/static-mapper #45
Conversation
1948e15
to
c3d9255
Compare
No bank seems to use the same Nordigen fields and the rudimentary best effort guess on which fields to use is not gonna float for all banks. This adds a structure for mapping every bank independently but defaults to using or existing mapping to have no breaking changes.
c3d9255
to
066b566
Compare
@hpernu I updated the code slightly, please check again. To your point, firstly, I don't think this will be a maintenance hurdle on just me, it's an open-source project and the maintenance is fundamentally on every user as they see fit. But one does not exclude the other, we can have static mappings for some banks but strive to keep as many banks as possible in the default mapper, for as long as it's feasible. By the looks of it, we can simply add |
Use real NORDEA_NDEADKKK transaction from Nordigen which includes detail in the AdditionalInformation field.
Support getting the payee from AdditionalInformation if its present in the Nordigen transaction and no creditor/debtor nor remittance information is available. Which is the case for SEB_KORT_AB_NO_SKHSFI21 for example. Fixes #10
8765e25
to
061d1f4
Compare
Yes, I think so too. It would be fairly trivial to just put it as one of the options for payee source. When I initially implemented this feature, I'd never have guessed that additionalInformation field could have held just the actual payee. In all of my banks (five or so) it either contains the freeform message input by the payer in bank software or a collection of more verbose information (which is often too much) neither of which is useful as payee by itself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried this with my banks and it just works as previously.
So the changes look okay to me but please check the changes to READMEs.
Add list of supported Nordigen banks to README and a warning log statement for users of v1 import IDs. Remove Kubernetes as the information was outdated and update the example config.
061d1f4
to
d16e8fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems okay although some of the README stuff still might be better but then documentation can always be improved.
This is a draft of my proposed way to solve the issues we've been seeing in various places including #10
An alternative solution could be to use the reflect package and allow the user complete control over the mappings. I tried to hack a bit on it but I could not get anything reasonable to work. I'd be very interested in seeing others' attempts at true dynamic mapping.