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.

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

[contract_payment_auto] Lack of robustness - missing commits after each payment (?) #166

Closed
fcayre opened this issue May 23, 2018 · 4 comments

Comments

@fcayre
Copy link
Member

fcayre commented May 23, 2018

Hi folks,

I recently faced an important issue (see #165) which rolled back the creation of a bunch of payment transactions in odoo, while their acquirer counterpart operations were sent to the bank. At next cron execution time, all these operations were then re-executed, resulting in a double bank payment.

The consequences would have been mitigated if a commit had been executed in the recurring task loop. Do you agree? Where in the code would you put that commit? In AccountAnalyticAccount.recurring_create_invoice in contract/models/account_analytic_account.py? Any advice before I give it a try and submit a pull request?

@pedrobaeza
Copy link
Member

No, that's not a good technique. You should never commit inside a transaction, or you can have an inconsistent situation.

@fcayre
Copy link
Member Author

fcayre commented May 23, 2018

Hello @pedrobaeza thanks for your answer. However, present issue describes a problem (global rollback of operations that are independent by nature) which is not solved, so I don't think it should be closed so fast as there is room for improvement here.

The AccountAnalyticAccount.cron_recurring_create_invoice method from contract/models/account_analytic_account.py might be the right place to use the api.Environment.manage context manager mentioned in https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#never-commit-the-transaction to create isolated valid transactions (one for each contract).

What do you think?

@nhomar nhomar reopened this Oct 1, 2018
@pedrobaeza
Copy link
Member

@nhomar why do you reopen? You know that there should never be a commit in the middle of the operation.

fcayre added a commit to fcayre/contract that referenced this issue Oct 14, 2018
…action on recurring invoice creation and auto pay retry errors

Fixes OCA#166.

Rather save the database state before each invoice creation (or auto pay
retry) so that only those in failure are rollbacked.

Before the change, the whole transaction was rollbacked on error
although the recurring invoice creations (resp. auto pay retries) are
independant operations.

This was leading to specially bad bugs when using an HTTP-driven service
for auto payments, as the service could register payments that odoo would
rollback on a later invoice creation crash in the same cron execution,
thus auto paying them again at next cron execution although they were
already registered as paid in the distant service.
@pedrobaeza
Copy link
Member

Closing as there's an opened PR about this and being discussed there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants