Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[12.0][ADD] account_bank_statement_import_online_paypal #234

Merged
merged 1 commit into from
Mar 30, 2020
Merged

[12.0][ADD] account_bank_statement_import_online_paypal #234

merged 1 commit into from
Mar 30, 2020

Conversation

alexey-pelykh
Copy link
Contributor

@alexey-pelykh alexey-pelykh commented Nov 2, 2019 •
edited
Loading

Adds online bank statements from PayPal

@alexey-pelykh alexey-pelykh changed the title [12.0][ADD] account_bank_statement_import_online_paypal [WIP][12.0][ADD] account_bank_statement_import_online_paypal Nov 2, 2019
@alexey-pelykh alexey-pelykh force-pushed the 12.0-add-account_bank_statement_import_online_paypal branch from 9258ec2 to d9a2005 Compare November 3, 2019 17:03
@alexey-pelykh alexey-pelykh changed the title [WIP][12.0][ADD] account_bank_statement_import_online_paypal [12.0][ADD] account_bank_statement_import_online_paypal Nov 3, 2019
@alexey-pelykh alexey-pelykh force-pushed the 12.0-add-account_bank_statement_import_online_paypal branch 25 times, most recently from 4ff8e27 to 9979023 Compare November 4, 2019 06:56
@alexey-pelykh
Copy link
Contributor Author

@pedrobaeza potentially, online sync is also interesting for Tecnativa

@pedrobaeza pedrobaeza added this to the 12.0 milestone Nov 6, 2019
@alexey-pelykh alexey-pelykh force-pushed the 12.0-add-account_bank_statement_import_online_paypal branch 3 times, most recently from 053419a to d5991b8 Compare November 16, 2019 08:12
@alexey-pelykh alexey-pelykh force-pushed the 12.0-add-account_bank_statement_import_online_paypal branch from d5991b8 to ffa200c Compare February 12, 2020 15:39
@alexey-pelykh
Copy link
Contributor Author

@OSevangelist it would be awesome to understand if that's useful for you

artemsenko
artemsenko approved these changes Feb 12, 2020
View reviewed changes
Copy link

@artemsenko artemsenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

functional tests

sweenya
sweenya approved these changes Feb 13, 2020
View reviewed changes
Copy link

@sweenya sweenya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functional test

sweenya
sweenya approved these changes Feb 13, 2020
View reviewed changes
Copy link

@sweenya sweenya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functional test

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@OCA-git-bot
Copy link
Contributor

Sorry @alexey-pelykh you are not allowed to merge.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

@rvalyi
Copy link
Member

rvalyi commented Feb 13, 2020

@alexey-pelykh I'm following your online bank statetement import work with interest. But for this particular Paypal case, I was wondering if no logic could be shared between this new module and the existing one which works from the file (and not via API):
https://github.com/OCA/bank-statement-import/tree/12.0/account_bank_statement_import_paypal
Then may be some refactor is possible instead of duplicating the mapping logic. Frankly I don't know, just asking.

@alexey-pelykh
Copy link
Contributor Author

@rvalyi thanks! I've looked into that module in terms of sharing code and there's even a #236 regarding it. My conclusion is: not worthy, data structures are different

@rvalyi
Copy link
Member

rvalyi commented Feb 13, 2020

ok thank you for this analysis and the other PR. I pinged @sebastienbeau who is the person most skilled on these modules at Akretion.

@alexey-pelykh
Copy link
Contributor Author

@OCA/banking-maintainers would it be possible to make further steps about this module?

@alexey-pelykh alexey-pelykh force-pushed the 12.0-add-account_bank_statement_import_online_paypal branch from ffa200c to 9f504d7 Compare March 23, 2020 07:27
@rafaelbn
Copy link
Member

Are you using this PR in production?

@alexey-pelykh
Copy link
Contributor Author

@rafaelbn yep, though we’re using PayPal less often than before. On top of that, PayPal has some glitch on every Monday’s start of day that causes false-positive error message, their engineers are looking into that right now

@alexey-pelykh
Copy link
Contributor Author

The issue is confirmed to be on PayPal side, nothing to be fixed in the module itself, so I'll just merge it.

@alexey-pelykh
Copy link
Contributor Author

/ocabot merge

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 12.0-ocabot-merge-pr-234-by-alexey-pelykh-bump-no, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 346637c into OCA:12.0 Mar 30, 2020
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at abf9871. Thanks a lot for contributing to OCA. ❤️

@alexey-pelykh alexey-pelykh deleted the 12.0-add-account_bank_statement_import_online_paypal branch March 30, 2020 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artemsenko artemsenko artemsenko approved these changes

@sweenya sweenya sweenya approved these changes

@sbidoul sbidoul Awaiting requested review from sbidoul

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
12.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@alexey-pelykh @pedrobaeza @OCA-git-bot @rvalyi @rafaelbn @artemsenko @sweenya