-
-
Notifications
You must be signed in to change notification settings - Fork 427
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
[16.0][ADD] account_statement_import_online_plaid #696
base: 16.0
Are you sure you want to change the base?
[16.0][ADD] account_statement_import_online_plaid #696
Conversation
7976fe8
to
2391203
Compare
2391203
to
e0c6c67
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.
Great solution for bank statements import in USA 👍🏻
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 OK. LGTM!
This PR has the |
b6aee78
to
9962ba4
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.
Technically seems correct, however, I would make a small change
37120c4
to
6f48d2f
Compare
Hello @carolinafernandez-tecnativa , sorry for the delay. I have checked the mentioned issue, and it seems to be unrelated to this addon, as the problem persists even after uninstalling this addon in the runboat. |
@pedrobaeza @etobella anything else we need here to push this through ? |
@adasatorres @pedrobaeza @etobella I'm starting the migration to v17 from this PR. As this is not merged yet, is there any guidelines to follow? |
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.
Code Review LGTM
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.
I miss the plaid-python version before. Please accept my suggestion.
72ce058
to
6d0e168
Compare
Checking these days. Please wait a bit. |
For international companies using banks in different country
|
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.
This doesn't serve without Plaid giving the prod API keys, which is not easy and very pricey, so it shouldn't be merged for not creating false expectations.
That's not correct, Plaid prod API keys for transactions are possible on entry tier - my company pays less than a dollar a month for 10 accounts |
Well, I want to know how can that possible nowadays. I wasn't able. Maybe you have some instructions to follow? |
@pedrobaeza You just choose "Pay As You Go" plan and that's it. |
I requested that, but still no access: And same things seem to happen for others: https://www.reddit.com/r/fintech/comments/bxhvs3/plaid_pay_as_you_go_pricing/?rdt=52816 |
from my side, I've tested and everything works. the only issue i had is that the bank is in the different country than my company and i suggested a fix above, other than that it all seem to work |
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.
Good morning,
I have tested this PR repeatedly and it's performance has always been correct. This LGTM👍
6d0e168
to
0793b89
Compare
country_code = country_code = ( | ||
self.journal_id.bank_id.country.code | ||
or self.env.user.company_id.country_id.code | ||
) |
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.
country_code = country_code = ( | |
self.journal_id.bank_id.country.code | |
or self.env.user.company_id.country_id.code | |
) | |
country_code = country_code = ( | |
self.journal_id.company_id.country_id.code | |
or self.env.company.country_id.code | |
) |
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.
Good afternoon @pedrobaeza , why not use the country code configured in the bank specified in the journal? In my opinion, I think it's a good idea since you could have banks in different countries than your company. Additionally, I have been researching, and it is recommended to use the country or region code where the bank you are connecting to operates. For example, to determine the available financial institutions, configure the environment and ensure regulatory compliance, select the language and currency, define the structure of banking credentials, and so on.
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.
You may not have the bank defined. And is country
a field in standard Odoo or is it something of l10n_es_partner
? At minimum, you need to put that fallback to company's country.
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.
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.
OK about the first one, but the fallback is incorrect. self.env.user.company_id
is not the current company, but the default company of the user. You have to use self.env.company.country_id.code
instead, and I insist that the first fallback must be the company's journal, as you may have selected several companies at the same time, and being linking a journal not from the main company, so self.env.company
won't be correct as well.
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.
Yes, you're right. It doesn't make sense to use the country code of either the user or the current company. The company associated with the journal should be used.
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.
Would it be a good idea to configure a system parameter with a default country code? There will always be a possibility that the country field is False.
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.
The country is mandatory in the company, so I don't think so. You shouldn't over-complicate this case. If you want, you can issue a UserError
if no country code as a way to stop the process.
@ayushin Good morning, and thank you for the suggestion, I just added it. @rrebollo Can you add this suggestion to the following PR? #742 |
6e3977b
to
b9146c1
Compare
b9146c1
to
ed31240
Compare
@adasatorres can you pin me when the requested changes are done, to forward-port them to v17 PR. |
Good morning @rrebollo , The changes have already been uploaded; I just need to add a test for the new method I created, which is called _check_country. Additionally, I need to modify the translation file for the exception string. I’ll let you know as soon as I have those changes ready. |
Hi @adasatorres , while testing in a live environment, I noticed that when the user has the English language set, the following error occurs: |
Hi @jelenapoblet , when I get a chance, I will review it. There are languages that Plaid does not support, but US English should work . I need to create a method to verify those languages. |
This addon allow get your transactions using plaid.com platform.