[15.0][add] account_bank_statement_import_online_nordigen #496

JordiBForgeFlow · 2022-09-15T15:54:13Z

Integration with Nordigen

TODO:

Test scripts

github-actions · 2023-01-15T12:18:01Z

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.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

ayushin · 2023-02-06T08:58:22Z

is this still being worked on?

Rad0van · 2023-03-03T18:46:10Z

@JordiBForgeFlow this is a great initiative! Currently testing it and willing to provide feedback, improvements, etc...

Found one thing that prevents it from working for me.
I have created an account with Nordigen, created secret id and key, created bank account in Odoo, configured OCA provider and added secret and key.
When I click "Select Nordigen Bank Identifier" it lets me choose Country and Bank. Then it provides this screen - seems it read my name (radovan) from the id/key:

After clicking "I Agree" it redirects me here:

When I enter Nordigen username/password it provides some kind of demo/trial accounts...

This is kind of different from what you do inside Nordigen GUI, when you try to add a bank account. Country / Bank selection is basically the same. The next step is a bit different - as if it didn't know who I was:

After this step it asks for IBAN number:

Then it adds it to the list in Nordigen. But I cannot get to this step in Odoo... Any idea what may be going wrong?

Rad0van · 2023-03-06T12:50:52Z

@JordiBForgeFlow I have found out why that is... This line:
SANDBOX_INSTITUTION_ID = "SANDBOXFINANCE_SFIN0000"

OK, going to disable this and test again...

Entrepreneur-AJ · 2023-03-17T21:35:45Z

Ok, so I have been testing this for the past couple of days. I had to comment out SANDBOX_INSTITUTION_ID = "SANDBOXFINANCE_SFIN0000" as @Rad0van mentions to connect to my real bank account, which works.
I have stumbled across the following issues;

Scheduled Action doesn't pull any data using the cron worker
Scheduled Action doesn't pull any data when you push run manually
For some reason it although today's date is the 17th of March 2023 it is only pulling the 14th instead of the latest data.
When you need to reauthorise the connection to the bank the click here html link shows as code instead of a link
The account it matches to has to be in IBAN format I entered a UK sort code and account number at the add bank account part of odoo and it couldn't link them. Noticed it pulling IBAN numbers on auth so tried that and it pull's data (Maybe a Nordigen issue) (Maybe document this bit)

Maybe i'm doing something wrong, but it's worth a comment to see if anyone else is having the same problem.

ayushin · 2023-05-07T14:35:14Z

What does need to happen to have this merged?

Entrepreneur-AJ · 2023-05-07T14:40:15Z

Need 2 approvals after code review, I've been testing this since 14th of March with my issues commented above.

Update on previous comment;
Scheduled action is working just seems to work once a day only, whether that's a module or bank problem I don't know.

IBAN and BIC still a requirement.

Line 163 of https://github.com/ForgeFlow/bank-statement-import/blob/15.0-add-account_bank_statement_import_online_nordigen/account_bank_statement_import_online_nordigen/models/online_bank_statement_provider_nordigen.py needs removing to work with a real bank.

Documentation stating that you need to generate api keys on the nordigen website would be beneficial.

Entrepreneur-AJ

Remove the sandbox id on line 163 of the online_bank_statement_provider_nordigen.py file or convert it until a UI based config option.

Entrepreneur-AJ · 2023-05-07T15:17:39Z

account_bank_statement_import_online_nordigen/models/online_bank_statement_provider_nordigen.py

+            institution_id = SANDBOX_INSTITUTION_ID
+        self.nordigen_last_requisition_ref = str(uuid4())
+        # TO DELETE
+        institution_id = "SANDBOXFINANCE_SFIN0000"


Only thing I see stopping a merge is this hardcode, either it should be removed or made into a UI based switch for demo

Happy to approve after. (Been testing for 2 months)

OK, so who should comment that line out? :-)

@ayushin Normally the person who initially posted the PR.
@JordiBForgeFlow are you still wanting this PR or should it be forked to another PR

@Entrepreneur-AJ feel free to propose the fix in another PR. Then we can close this one.

Will look at sorting it after work tonight

Entrepreneur-AJ · 2023-05-11T19:48:40Z

As promised, a new PR clearing that sandbox id #592 Seems to have flagged a translation issue with pre-commit, so I am looking into a workaround for that now.

Entrepreneur-AJ · 2023-05-17T08:42:16Z

@JordiBForgeFlow @ChrisOForgeFlow @Rad0van @ayushin PR #592 ready for review this PR plus the sandbox line deleted, and satisfying translation issues given by pre-commit (pylint).

Rad0van · 2023-05-17T09:09:48Z

Update on previous comment; Scheduled action is working just seems to work once a day only, whether that's a module or bank problem I don't know.

Try having a look at the source code of the module account_bank_statement_import_online which is a base for this one.

IBAN and BIC still a requirement.

As it should. Nordigen implements PSD2 APIs which are standardized in European Union predominantly for SEPA system where IBAN and BIC are the cornerstones...

Documentation stating that you need to generate api keys on the nordigen website would be beneficial.

I see it there though... See last line of CONFIGURE.rst and appropriate section of generated README.rst

pedrobaeza · 2023-05-17T10:28:20Z

Please clean the commit history and we can merge.

JordiBForgeFlow · 2023-05-17T10:28:56Z

@Entrepreneur-AJ I have now done the fixes you asked for and closed your PR. In order to properly take up the PR from another person an continue you must clone my branch first, then make your changes in your local and finally push to your own repo, to then make a new PR. That way the commit history will be preserved.

Anyway this PR is not ready to merge because it lacks to add automated tests. Do you think that you @Entrepreneur-AJ are able to add automated tests?

pedrobaeza · 2023-05-17T10:38:28Z

Automated tests are not strictly needed, although always welcome and desirable. In this case, having connections to external systems, the tests should be mocked, so their effectiveness is limited, so I think is good to go if it works.

MiquelRForgeFlow

In fact, the commit history is already clean IMO 🤔 @pedrobaeza

MiquelRForgeFlow · 2023-05-17T10:43:13Z

Ah, needs a rebase

Rad0van · 2023-05-17T10:43:35Z

@JordiBForgeFlow we have found Nordigen API is in some cases behaving funny. For example when you add a month period from beginning of May to Beginning of June this year it returns nothing. The latest date can be today. Also the way the date ranges for daily pulls are generated in the account_bank_statement_import_online causes this to behave strange. So I am working with a colleague on improvements. You do not necessarily need to wait for these - we can always amend by new PRs. Just informing that we are working on this trying to make it better.

pedrobaeza

About the commit history:

And I have just seen the web_notify dependency, which is an undesired coupling dependency for a toolset like this. Please remove it.

Entrepreneur-AJ · 2023-05-20T12:10:01Z

@Rad0van

I see it there though... See last line of CONFIGURE.rst and appropriate section of generated README.rst
Must have overlooked this section.

@JordiBForgeFlow thank you for the updates and the quick run down for future reference, happy to look at automated tests when I will have time I honestly don't know so push forward for now. I will double-check the latest commit before editing my review/approval.

ayushin · 2023-06-01T07:44:24Z

This is such a useful integration, how can we get it merged?

Can somebody please produce a checklist of what needs to be done?

It feels we are nearly there but weeks go by and it would be great to have it backported to 14 & 13

rokmarko · 2023-08-24T08:21:39Z

Hi

This is very good tool for european customers. Is there any problem with this PR? Why is still not merged?

Can I help somehow to get this merged?

ayushin · 2023-09-02T16:08:58Z

@pedrobaeza Is the web_notify dependency the only issue holding back this PR merge?

pedrobaeza · 2023-11-03T16:26:44Z

Closed in favor of #635

ChrisOForgeFlow and others added 5 commits September 15, 2022 17:47

[14.0][ADD] account_bank_statement_import_online_nordigen

19d4b07

Varios fixes

23d0828

[ADD[ added icon nordigen

44b6b5d

[ADD] web_notify for missed configuration

4db5218

[MIG] account_bank_statement_import_online_nordigen: Migration to 15.0

e2a39d9

JordiBForgeFlow mentioned this pull request Nov 23, 2022

Nordigen Intergration #491

Closed

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jan 15, 2023

github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Feb 12, 2023

Entrepreneur-AJ suggested changes May 7, 2023

View reviewed changes

Entrepreneur-AJ mentioned this pull request May 11, 2023

[15.0] [ADD] account_bank_statement_import_online_nordigen #592

Closed

Entrepreneur-AJ mentioned this pull request May 17, 2023

Migration to version 15.0 #424

Closed

13 tasks

JordiBForgeFlow added 2 commits May 17, 2023 12:24

update copier

5d67aeb

[FIX] remove sanbox institution

f4f91ae

pedrobaeza added this to the 15.0 milestone May 17, 2023

MiquelRForgeFlow approved these changes May 17, 2023

View reviewed changes

pedrobaeza requested changes May 17, 2023

View reviewed changes

Entrepreneur-AJ approved these changes May 20, 2023

View reviewed changes

pedrobaeza mentioned this pull request Nov 3, 2023

[15.0][ADD] account_statement_import_online_gocardless #635

Merged

pedrobaeza closed this Nov 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[15.0][add] account_bank_statement_import_online_nordigen #496

[15.0][add] account_bank_statement_import_online_nordigen #496

JordiBForgeFlow commented Sep 15, 2022

github-actions bot commented Jan 15, 2023

ayushin commented Feb 6, 2023

Rad0van commented Mar 3, 2023

Rad0van commented Mar 6, 2023

Entrepreneur-AJ commented Mar 17, 2023

ayushin commented May 7, 2023

Entrepreneur-AJ commented May 7, 2023

Entrepreneur-AJ left a comment

Entrepreneur-AJ May 7, 2023

Entrepreneur-AJ May 7, 2023

ayushin May 11, 2023

Entrepreneur-AJ May 11, 2023

JordiBForgeFlow May 11, 2023

Entrepreneur-AJ May 11, 2023

Entrepreneur-AJ commented May 11, 2023

Entrepreneur-AJ commented May 17, 2023

Rad0van commented May 17, 2023 •

edited

Loading

pedrobaeza commented May 17, 2023

JordiBForgeFlow commented May 17, 2023

pedrobaeza commented May 17, 2023

MiquelRForgeFlow left a comment

MiquelRForgeFlow commented May 17, 2023

Rad0van commented May 17, 2023

pedrobaeza left a comment

Entrepreneur-AJ commented May 20, 2023

ayushin commented Jun 1, 2023

rokmarko commented Aug 24, 2023

ayushin commented Sep 2, 2023

pedrobaeza commented Nov 3, 2023

[15.0][add] account_bank_statement_import_online_nordigen #496

[15.0][add] account_bank_statement_import_online_nordigen #496

Conversation

JordiBForgeFlow commented Sep 15, 2022

github-actions bot commented Jan 15, 2023

ayushin commented Feb 6, 2023

Rad0van commented Mar 3, 2023

Rad0van commented Mar 6, 2023

Entrepreneur-AJ commented Mar 17, 2023

ayushin commented May 7, 2023

Entrepreneur-AJ commented May 7, 2023

Entrepreneur-AJ left a comment

Choose a reason for hiding this comment

Entrepreneur-AJ May 7, 2023

Choose a reason for hiding this comment

Entrepreneur-AJ May 7, 2023

Choose a reason for hiding this comment

ayushin May 11, 2023

Choose a reason for hiding this comment

Entrepreneur-AJ May 11, 2023

Choose a reason for hiding this comment

JordiBForgeFlow May 11, 2023

Choose a reason for hiding this comment

Entrepreneur-AJ May 11, 2023

Choose a reason for hiding this comment

Entrepreneur-AJ commented May 11, 2023

Entrepreneur-AJ commented May 17, 2023

Rad0van commented May 17, 2023 • edited Loading

pedrobaeza commented May 17, 2023

JordiBForgeFlow commented May 17, 2023

pedrobaeza commented May 17, 2023

MiquelRForgeFlow left a comment

Choose a reason for hiding this comment

MiquelRForgeFlow commented May 17, 2023

Rad0van commented May 17, 2023

pedrobaeza left a comment

Choose a reason for hiding this comment

Entrepreneur-AJ commented May 20, 2023

ayushin commented Jun 1, 2023

rokmarko commented Aug 24, 2023

ayushin commented Sep 2, 2023

pedrobaeza commented Nov 3, 2023

Rad0van commented May 17, 2023 •

edited

Loading