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

[14.0] account_payment_order: Performance problem when posting + reconciling transit moves #966

Closed
pedrobaeza opened this issue Oct 6, 2022 · 3 comments
Labels

Comments

@pedrobaeza
Copy link
Member

pedrobaeza commented Oct 6, 2022

Steps to reproduce:

  • Have a big bunch of invoices (200+).
  • Include all of them in a payment order.
  • Configure the payment mode to post transit moves.
  • Confirm the order, generate file, and click on "File successfully uploaded".

The operation will take a lot of time, or even not being able to finish depending on the timeouts.

Tracing down the problem, it's a proportional performance degradation on each loop of:

Selección_043

And it's due to a call over _compute_amount:

https://github.com/odoo/odoo/blob/062fb69f346873fe05ebb5d429ab3535fd1df5cc/addons/account/models/account_move.py#L1406

coming from:

https://github.com/odoo/odoo/blob/062fb69f346873fe05ebb5d429ab3535fd1df5cc/addons/account/models/account_move.py#L4997

that contains each time current invoices plus the old ones.

_compute_amount account.move(204346, 204864)
_compute_amount account.move(204320, 204346, 204864, 204319)
_compute_amount account.move(204318, 204864, 204320, 204346, 204319)
_compute_amount account.move(204317, 204864, 204320, 204346, 204318, 204319)
_compute_amount account.move(204315, 204864, 204320, 204346, 204317, 204318, 204319)
_compute_amount account.move(204313, 204864, 204320, 204346, 204315, 204317, 204318, 204319)
_compute_amount account.move(204312, 204320, 204864, 204313, 204346, 204315, 204317, 204318, 204319)
_compute_amount account.move(203154, 204864, 204320, 204312, 204313, 204346, 204315, 204317, 204318, 204319)
_compute_amount account.move(203153, 204864, 204320, 203154, 204312, 204313, 204346, 204315, 204317, 204318, 204319)
_compute_amount account.move(203152, 204320, 204864, 203153, 203154, 204312, 204313, 204346, 204315, 204317, 204318, 204319)
_compute_amount account.move(203151, 204864, 204320, 203152, 203153, 203154, 204312, 204313, 204346, 204315, 204317, 204318, 204319)
_compute_amount account.move(203148, 204864, 202753, 204320, 203151, 203152, 203153, 203154, 204312, 204313, 204346, 204315, 204317, 204318, 204319)
_compute_amount account.move(203145, 204864, 204320, 202753, 203148, 203151, 203152, 203153, 203154, 204312, 204313, 204346, 204315, 204317, 204318, 204319)
...

This can be an effect of the depends for the field or bad handling of the ORM, but anyway, there's no clean solution having some parts on Odoo core and not having clear the root source.

My first option was to aggregate all move lines to reconcile and call it once, but this may do undesired reconciliations when having same amounts, mixing partners and so on.

@Tecnativa TT39832

@pedrobaeza pedrobaeza added the bug label Oct 6, 2022
@pedrobaeza
Copy link
Member Author

Have you experimented this, @sbidoul @alexis-via ?

@alexis-via
Copy link
Contributor

I haven't experimented this, but I probably never had such a high volume of payment lines in a single payment order.

pedrobaeza added a commit to Tecnativa/bank-payment that referenced this issue Nov 6, 2022
The previous approach creates manually the journal entries and does all
the hard work, plus not being 100% compatible with the bank statement
reconciliation widget (requiring a patch on OCB to see blue lines).

That decision made sense on the moment it was done (v9), where the
native payment model (account.payment) was very limited, and wasn't able
to store all the needed information for the bank transaction.

Now that the limitations are gone, we can get rid off this extra model,
and generate instead `account.payment` records, using both the native
model + methods to perform the same operations.

This serves also to workaround the problem found in OCA#966.

All the code, views and tests of main module have been adapted to this
new approach in this commit. Later commits will adapt the rest of the
modules of the suite, and add migration scripts to transit from the
previous approach to this new one.

TT39832
pedrobaeza added a commit to Tecnativa/bank-payment that referenced this issue Nov 6, 2022
The previous approach creates manually the journal entries and does all
the hard work, plus not being 100% compatible with the bank statement
reconciliation widget (requiring a patch on OCB to see blue lines).

That decision made sense on the moment it was done (v9), where the
native payment model (account.payment) was very limited, and wasn't able
to store all the needed information for the bank transaction.

Now that the limitations are gone, we can get rid off this extra model,
and generate instead `account.payment` records, using both the native
model + methods to perform the same operations.

This serves also to workaround the problem found in OCA#966.

All the code, views and tests of main module have been adapted to this
new approach in this commit. Later commits will adapt the rest of the
modules of the suite, and add migration scripts to transit from the
previous approach to this new one.

TT39832
pedrobaeza added a commit to Tecnativa/bank-payment that referenced this issue Nov 7, 2022
The previous approach creates manually the journal entries and does all
the hard work, plus not being 100% compatible with the bank statement
reconciliation widget (requiring a patch on OCB to see blue lines).

That decision made sense on the moment it was done (v9), where the
native payment model (account.payment) was very limited, and wasn't able
to store all the needed information for the bank transaction.

Now that the limitations are gone, we can get rid off this extra model,
and generate instead `account.payment` records, using both the native
model + methods to perform the same operations.

This serves also to workaround the problem found in OCA#966.

All the code, views and tests of main module have been adapted to this
new approach in this commit. Later commits will adapt the rest of the
modules of the suite, and add migration scripts to transit from the
previous approach to this new one.

TT39832
pedrobaeza added a commit to Tecnativa/bank-payment that referenced this issue Dec 25, 2022
The previous approach creates manually the journal entries and does all
the hard work, plus not being 100% compatible with the bank statement
reconciliation widget (requiring a patch on OCB to see blue lines).

That decision made sense on the moment it was done (v9), where the
native payment model (account.payment) was very limited, and wasn't able
to store all the needed information for the bank transaction.

Now that the limitations are gone, we can get rid off this extra model,
and generate instead `account.payment` records, using both the native
model + methods to perform the same operations.

This serves also to workaround the problem found in OCA#966.

All the code, views and tests of main module have been adapted to this
new approach in this commit. Later commits will adapt the rest of the
modules of the suite, and add migration scripts to transit from the
previous approach to this new one.

TT39832
@pedrobaeza
Copy link
Member Author

Resolved in #979

wpichler pushed a commit to wpichler/bank-payment that referenced this issue Jan 21, 2023
The previous approach creates manually the journal entries and does all
the hard work, plus not being 100% compatible with the bank statement
reconciliation widget (requiring a patch on OCB to see blue lines).

That decision made sense on the moment it was done (v9), where the
native payment model (account.payment) was very limited, and wasn't able
to store all the needed information for the bank transaction.

Now that the limitations are gone, we can get rid off this extra model,
and generate instead `account.payment` records, using both the native
model + methods to perform the same operations.

This serves also to workaround the problem found in OCA#966.

All the code, views and tests of main module have been adapted to this
new approach in this commit. Later commits will adapt the rest of the
modules of the suite, and add migration scripts to transit from the
previous approach to this new one.

TT39832
wpichler pushed a commit to wpichler/bank-payment that referenced this issue Jan 27, 2023
The previous approach creates manually the journal entries and does all
the hard work, plus not being 100% compatible with the bank statement
reconciliation widget (requiring a patch on OCB to see blue lines).

That decision made sense on the moment it was done (v9), where the
native payment model (account.payment) was very limited, and wasn't able
to store all the needed information for the bank transaction.

Now that the limitations are gone, we can get rid off this extra model,
and generate instead `account.payment` records, using both the native
model + methods to perform the same operations.

This serves also to workaround the problem found in OCA#966.

All the code, views and tests of main module have been adapted to this
new approach in this commit. Later commits will adapt the rest of the
modules of the suite, and add migration scripts to transit from the
previous approach to this new one.

TT39832
hhgabelgaard pushed a commit to steingabelgaard/bank-payment that referenced this issue Feb 16, 2023
The previous approach creates manually the journal entries and does all
the hard work, plus not being 100% compatible with the bank statement
reconciliation widget (requiring a patch on OCB to see blue lines).

That decision made sense on the moment it was done (v9), where the
native payment model (account.payment) was very limited, and wasn't able
to store all the needed information for the bank transaction.

Now that the limitations are gone, we can get rid off this extra model,
and generate instead `account.payment` records, using both the native
model + methods to perform the same operations.

This serves also to workaround the problem found in OCA#966.

All the code, views and tests of main module have been adapted to this
new approach in this commit. Later commits will adapt the rest of the
modules of the suite, and add migration scripts to transit from the
previous approach to this new one.

TT39832
hhgabelgaard pushed a commit to steingabelgaard/bank-payment that referenced this issue Feb 16, 2023
The previous approach creates manually the journal entries and does all
the hard work, plus not being 100% compatible with the bank statement
reconciliation widget (requiring a patch on OCB to see blue lines).

That decision made sense on the moment it was done (v9), where the
native payment model (account.payment) was very limited, and wasn't able
to store all the needed information for the bank transaction.

Now that the limitations are gone, we can get rid off this extra model,
and generate instead `account.payment` records, using both the native
model + methods to perform the same operations.

This serves also to workaround the problem found in OCA#966.

All the code, views and tests of main module have been adapted to this
new approach in this commit. Later commits will adapt the rest of the
modules of the suite, and add migration scripts to transit from the
previous approach to this new one.

TT39832
hhgabelgaard pushed a commit to steingabelgaard/bank-payment that referenced this issue Feb 16, 2023
The previous approach creates manually the journal entries and does all
the hard work, plus not being 100% compatible with the bank statement
reconciliation widget (requiring a patch on OCB to see blue lines).

That decision made sense on the moment it was done (v9), where the
native payment model (account.payment) was very limited, and wasn't able
to store all the needed information for the bank transaction.

Now that the limitations are gone, we can get rid off this extra model,
and generate instead `account.payment` records, using both the native
model + methods to perform the same operations.

This serves also to workaround the problem found in OCA#966.

All the code, views and tests of main module have been adapted to this
new approach in this commit. Later commits will adapt the rest of the
modules of the suite, and add migration scripts to transit from the
previous approach to this new one.

TT39832
wpichler added a commit to wpichler/bank-payment that referenced this issue Feb 18, 2023
Fixed tests. Removed overwriten _get_default_journal function - it does not exists any more in original object. Removed sepa_in journal in tests - it was never in the manifest as a dependencie - and it does not really belong here

Update account_payment_order/models/account_payment.py

Co-authored-by: dzungtran89 <46039081+dzungtran89@users.noreply.github.com>

Update account_payment_order/models/account_payment.py

Co-authored-by: dzungtran89 <46039081+dzungtran89@users.noreply.github.com>

Update account_payment_order/models/account_payment_order.py

Co-authored-by: dzungtran89 <46039081+dzungtran89@users.noreply.github.com>

Update account_payment_order/wizard/account_payment_line_create.py

Co-authored-by: dzungtran89 <46039081+dzungtran89@users.noreply.github.com>

Update account_payment_order/wizard/account_payment_line_create.py

Co-authored-by: dzungtran89 <46039081+dzungtran89@users.noreply.github.com>

Update account_payment_order/models/account_move.py

Co-authored-by: dzungtran89 <46039081+dzungtran89@users.noreply.github.com>

Update account_payment_order/models/account_move_line.py

account.account internal_type changed to account type with receivable=asset_receivable and payable=liability_payable

Co-authored-by: dzungtran89 <46039081+dzungtran89@users.noreply.github.com>

[FIX] account_payment_order: Error in Batch Payments when deleting a Batch Content line

[REF+IMP] account_payment_order: Use native payments

The previous approach creates manually the journal entries and does all
the hard work, plus not being 100% compatible with the bank statement
reconciliation widget (requiring a patch on OCB to see blue lines).

That decision made sense on the moment it was done (v9), where the
native payment model (account.payment) was very limited, and wasn't able
to store all the needed information for the bank transaction.

Now that the limitations are gone, we can get rid off this extra model,
and generate instead `account.payment` records, using both the native
model + methods to perform the same operations.

This serves also to workaround the problem found in OCA#966.

All the code, views and tests of main module have been adapted to this
new approach in this commit. Later commits will adapt the rest of the
modules of the suite, and add migration scripts to transit from the
previous approach to this new one.

TT39832

[OU-ADD] account_payment_order: Migration scripts to native payment ref

This adapts the old bank.payment.line records to account.payment records
according the new approach.

[FIX] Fixed bug from cherry-pick in the tests

[FIX] Make pylint_odoo happy - removed string attribute with same name as the variable

[FIX] Fixed account_internal_type usage

[FIX] Fixed bugs seen in the tests

[14.0][IMP] account_payment_order: Add existing payment references to communication

If some movements have been reconciled with the original invoice,
their references should be added in communication too.

e.g.: Manual credit notes

[14.0][FIX] account_payment_order: Don't duplicate communication reference

[IMP] Pre Commit Changes

[IMP] create with @api.model_create_multi tag
[MIG] migration scripts
[IMP] report account_payment_order use t-field
[IMP] field company_id invisible on view account_payment_line
pedrobaeza added a commit to Tecnativa/bank-payment that referenced this issue Mar 4, 2023
The previous approach creates manually the journal entries and does all
the hard work, plus not being 100% compatible with the bank statement
reconciliation widget (requiring a patch on OCB to see blue lines).

That decision made sense on the moment it was done (v9), where the
native payment model (account.payment) was very limited, and wasn't able
to store all the needed information for the bank transaction.

Now that the limitations are gone, we can get rid off this extra model,
and generate instead `account.payment` records, using both the native
model + methods to perform the same operations.

This serves also to workaround the problem found in OCA#966.

All the code, views and tests of main module have been adapted to this
new approach in this commit. Later commits will adapt the rest of the
modules of the suite, and add migration scripts to transit from the
previous approach to this new one.

TT39832
pedrobaeza added a commit to Tecnativa/bank-payment that referenced this issue Mar 4, 2023
The previous approach creates manually the journal entries and does all
the hard work, plus not being 100% compatible with the bank statement
reconciliation widget (requiring a patch on OCB to see blue lines).

That decision made sense on the moment it was done (v9), where the
native payment model (account.payment) was very limited, and wasn't able
to store all the needed information for the bank transaction.

Now that the limitations are gone, we can get rid off this extra model,
and generate instead `account.payment` records, using both the native
model + methods to perform the same operations.

This serves also to workaround the problem found in OCA#966.

All the code, views and tests of main module have been adapted to this
new approach in this commit. Later commits will adapt the rest of the
modules of the suite, and add migration scripts to transit from the
previous approach to this new one.

TT39832
pedrobaeza added a commit to Tecnativa/bank-payment that referenced this issue Mar 4, 2023
The previous approach creates manually the journal entries and does all
the hard work, plus not being 100% compatible with the bank statement
reconciliation widget (requiring a patch on OCB to see blue lines).

That decision made sense on the moment it was done (v9), where the
native payment model (account.payment) was very limited, and wasn't able
to store all the needed information for the bank transaction.

Now that the limitations are gone, we can get rid off this extra model,
and generate instead `account.payment` records, using both the native
model + methods to perform the same operations.

This serves also to workaround the problem found in OCA#966.

All the code, views and tests of main module have been adapted to this
new approach in this commit. Later commits will adapt the rest of the
modules of the suite, and add migration scripts to transit from the
previous approach to this new one.

TT39832
agyamuta pushed a commit to ursais/bank-payment that referenced this issue Sep 11, 2023
The previous approach creates manually the journal entries and does all
the hard work, plus not being 100% compatible with the bank statement
reconciliation widget (requiring a patch on OCB to see blue lines).

That decision made sense on the moment it was done (v9), where the
native payment model (account.payment) was very limited, and wasn't able
to store all the needed information for the bank transaction.

Now that the limitations are gone, we can get rid off this extra model,
and generate instead `account.payment` records, using both the native
model + methods to perform the same operations.

This serves also to workaround the problem found in OCA#966.

All the code, views and tests of main module have been adapted to this
new approach in this commit. Later commits will adapt the rest of the
modules of the suite, and add migration scripts to transit from the
previous approach to this new one.

TT39832
angelmoya pushed a commit to SidooSL/bank-payment that referenced this issue Jan 3, 2024
The previous approach creates manually the journal entries and does all
the hard work, plus not being 100% compatible with the bank statement
reconciliation widget (requiring a patch on OCB to see blue lines).

That decision made sense on the moment it was done (v9), where the
native payment model (account.payment) was very limited, and wasn't able
to store all the needed information for the bank transaction.

Now that the limitations are gone, we can get rid off this extra model,
and generate instead `account.payment` records, using both the native
model + methods to perform the same operations.

This serves also to workaround the problem found in OCA#966.

All the code, views and tests of main module have been adapted to this
new approach in this commit. Later commits will adapt the rest of the
modules of the suite, and add migration scripts to transit from the
previous approach to this new one.

TT39832
ramiadavid pushed a commit to ramiadavid/bank-payment that referenced this issue Jan 13, 2024
The previous approach creates manually the journal entries and does all
the hard work, plus not being 100% compatible with the bank statement
reconciliation widget (requiring a patch on OCB to see blue lines).

That decision made sense on the moment it was done (v9), where the
native payment model (account.payment) was very limited, and wasn't able
to store all the needed information for the bank transaction.

Now that the limitations are gone, we can get rid off this extra model,
and generate instead `account.payment` records, using both the native
model + methods to perform the same operations.

This serves also to workaround the problem found in OCA#966.

All the code, views and tests of main module have been adapted to this
new approach in this commit. Later commits will adapt the rest of the
modules of the suite, and add migration scripts to transit from the
previous approach to this new one.

TT39832
ramiadavid pushed a commit to ramiadavid/bank-payment that referenced this issue Feb 26, 2024
The previous approach creates manually the journal entries and does all
the hard work, plus not being 100% compatible with the bank statement
reconciliation widget (requiring a patch on OCB to see blue lines).

That decision made sense on the moment it was done (v9), where the
native payment model (account.payment) was very limited, and wasn't able
to store all the needed information for the bank transaction.

Now that the limitations are gone, we can get rid off this extra model,
and generate instead `account.payment` records, using both the native
model + methods to perform the same operations.

This serves also to workaround the problem found in OCA#966.

All the code, views and tests of main module have been adapted to this
new approach in this commit. Later commits will adapt the rest of the
modules of the suite, and add migration scripts to transit from the
previous approach to this new one.

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

No branches or pull requests

2 participants