-
-
Notifications
You must be signed in to change notification settings - Fork 418
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
[13.0][MIG][IMP] adyen statement import #384
Conversation
Looks great, you've solved all the technical debt from the 12.0 version and greatly enhanced the functionality! Are you suggesting we keep all the individual commits or can some of them be squashed? |
@StefanRijnhart with the customer that needed the migration, it is not yet clear how they want to handle the downloaded statements. The clearing account is still in the migration. I just removed the dependency from the adyen import. But there is no obstacle to use the modules in combination. I will squash the commits where logical. |
Mmm... In our case, we implemented the import module first, then when testing it turned out that the financial logic required this clearing account mechanism. So I'm not sure removing the dependency is a good idea. |
1b28ba1
to
78cd582
Compare
@StefanRijnhart Zou je kunnen leven met een dringend advies in de README om ook de clearing_account module te installeren ipv een technische dependency? |
I'm curious if the Adyen module is useful without the clearing account mechanism, and if we don't know, then what is the reason that you decoupled those? |
@StefanRijnhart I discussed it and will put back the clearing_account dependency. I will also go through all the commits once more to minimize them even more and rectify a few oversights. |
ff35e7e
to
28be6f9
Compare
d183517
to
caafca9
Compare
@NL66278 I think this should be an OCA icon -> account_bank_statement_import_online_adyen/static/description/icon.png |
@lfreeke I did not change the icon, do you expect problems with this? |
@NL66278 It has the Ponto icon now. |
@lfreeke I replaced the Ponto logo with the Adyen logo |
fe4cf65
to
8342b59
Compare
423c568
to
5885781
Compare
Runboat sunk?
|
d4c9715
to
1ef50a7
Compare
e44ec2e
to
943d783
Compare
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
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.
👍 Functional test, works well for our customer.
@StefanRijnhart Could you re-open, and possibly merge this one? Working very well for our customer for a long time now. And I will be migrating it to 16.0. |
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.
Great work, thanks!
/ocabot merge nobump |
Hey, thanks for contributing! Proceeding to merge this for you. |
…t_clearing_account
If passed any csv file, paypal module would try to use it, even if clearly not a paypal file.
Prevent other statement import modules from processing the file. Without this for instance the Enterprise account_bank_statement_import_csv module will process a retrieved cv file. In addition this is more efficient as it saves a decoding step.
csv is part of the standard library. Defining this as an external dependency crashes runboat
Put more information in the generated statement line. Also put the for users more recognizable merchant ref in the name (Label) field, and put the psp reference in ref. With a small refactoring the information retrieved can also more easily be customized.
Move parsing code to separate class; Break up large and unwieldy parsing method in separate methods.
943d783
to
7a4da01
Compare
@StefanRijnhart Thanks for your actions and comments. I did the rebase and the linting problems should be fixed now. |
Alright! /ocabot merge nobump |
What a great day to merge this nice PR. Let's do it! |
Congratulations, your PR was merged at f8c9430. Thanks a lot for contributing to OCA. ❤️ |
Migration of account_bank_statement_import_adyen, but extended with possibility to import csv files, and with a module to automatically import them form the Adyen website.
This PR also contains some changes to other modules, that were made necessary by the unfortunate logic of the import modules to try the import in turns and then call super when failing. Not all modules where handling this in a proper way.
It would be better if, like the original modules in 7.0, an import was always linked to a certain format, so this dubious try..catch..pass on logic could be avoided.