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

[IMP] account: add account.reconcile.model.line #38119

Open
wants to merge 1 commit into
base: master
from

Conversation

@william-andre
Copy link
Contributor

william-andre commented Oct 7, 2019

Instead of having all the fields duplicated with second_*, we have now a
o2m allowing us to

  • have more than 2 lines
  • reduce duplicated code
  • fix bugs and add features at only one place

We also remove the computation of writeoff and suggestions from the
client side as some code was 4-upled before (twice in in client and
twice in server side). The logic is now only at one place.

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo robodoo added the seen 🙂 label Oct 7, 2019
@C3POdoo C3POdoo added the RD label Oct 7, 2019
@william-andre william-andre force-pushed the odoo-dev:master-reconcile-model-lines-wan branch 2 times, most recently from ce1b9b1 to 9511c15 Oct 8, 2019
@robodoo robodoo added the CI 🤖 label Oct 8, 2019
@william-andre william-andre force-pushed the odoo-dev:master-reconcile-model-lines-wan branch from 9511c15 to 7ea0921 Oct 8, 2019
@robodoo robodoo removed the CI 🤖 label Oct 8, 2019
@william-andre william-andre force-pushed the odoo-dev:master-reconcile-model-lines-wan branch 5 times, most recently from fb20b4f to 911ef60 Oct 8, 2019
@robodoo robodoo added the CI 🤖 label Oct 9, 2019
@qdp-odoo

This comment has been minimized.

Copy link
Contributor

qdp-odoo commented Oct 21, 2019

needs a rebase

@william-andre william-andre force-pushed the odoo-dev:master-reconcile-model-lines-wan branch from 911ef60 to 74d48e1 Oct 22, 2019
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Oct 22, 2019
@william-andre william-andre force-pushed the odoo-dev:master-reconcile-model-lines-wan branch from 74d48e1 to 61e0b08 Oct 22, 2019
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Oct 22, 2019
@KangOl KangOl force-pushed the odoo:master branch from 86c80d3 to ab6d0c3 Nov 6, 2019
Copy link
Contributor

qdp-odoo left a comment

review done.

The objects are ok but some code needs rework and I wasn't able to have it working

addons/account/models/chart_template.py Outdated Show resolved Hide resolved
addons/account/models/account_reconcile_model.py Outdated Show resolved Hide resolved
addons/account/models/account_reconcile_model.py Outdated Show resolved Hide resolved
@@ -185,6 +165,25 @@ def _onchange_match_total_amount_param(self):
# RECONCILIATION PROCESS
####################################################

def _make_line_for_widget(self, line):

This comment has been minimized.

Copy link
@qdp-odoo

qdp-odoo Nov 7, 2019

Contributor

what's that for?

This comment has been minimized.

Copy link
@william-andre

william-andre Nov 7, 2019

Author Contributor

The client needs more info than the server to manage the data in the reconciliation widget. This is adding the value needed.

new_aml_dicts += self._get_taxes_move_lines_dict(tax, second_writeoff_line)
new_aml_dicts += self._get_taxes_move_lines_dict(tax, writeoff_line)

if for_widget:

This comment has been minimized.

Copy link
@qdp-odoo

qdp-odoo Nov 7, 2019

Contributor

mysterious code. Dubious. Me no like 😕

This comment has been minimized.

Copy link
@qdp-odoo

qdp-odoo Nov 8, 2019

Contributor

this last piece of code could be placed in get_reconciliation_dict_for_widget, if my previous comment is right, which would allow to remove the for_widget parameter (did I already mention I do not like it?)

This comment has been minimized.

Copy link
@william-andre

william-andre Nov 8, 2019

Author Contributor

I am also putting get_reconciliation_dict_for_widget in enterprise as the widget has moved. No need to see that in community...

This comment has been minimized.

Copy link
@qdp-odoo

qdp-odoo Nov 8, 2019

Contributor

good idea

addons/account/models/chart_template.py Outdated Show resolved Hide resolved
@william-andre william-andre force-pushed the odoo-dev:master-reconcile-model-lines-wan branch from 61e0b08 to eee02a3 Nov 7, 2019
@robodoo robodoo removed the CI 🤖 label Nov 7, 2019
@william-andre william-andre force-pushed the odoo-dev:master-reconcile-model-lines-wan branch 6 times, most recently from 6172517 to 04480ab Nov 7, 2019
@robodoo robodoo added the CI 🤖 label Nov 7, 2019
Copy link
Contributor

qdp-odoo left a comment

one last thing to check

tax = self.second_tax_ids
new_aml_dicts.append(writeoff_line)
if for_widget:
writeoff_line['to_check'] = line.model_id.to_check

This comment has been minimized.

Copy link
@qdp-odoo

qdp-odoo Nov 8, 2019

Contributor

why only if for_widget==True, and not always?

This comment has been minimized.

Copy link
@william-andre

william-andre Nov 8, 2019

Author Contributor

There is a hack because the field to_check is actually on the account.move in the database, but we have to put it virtually in the account.move.line in the reconciliation widget because of the way the reconciliation models work.

new_aml_dicts += self._get_taxes_move_lines_dict(tax, second_writeoff_line)
new_aml_dicts += self._get_taxes_move_lines_dict(tax, writeoff_line)

if for_widget:

This comment has been minimized.

Copy link
@qdp-odoo

qdp-odoo Nov 8, 2019

Contributor

this last piece of code could be placed in get_reconciliation_dict_for_widget, if my previous comment is right, which would allow to remove the for_widget parameter (did I already mention I do not like it?)

@william-andre william-andre force-pushed the odoo-dev:master-reconcile-model-lines-wan branch from 04480ab to 290f053 Nov 8, 2019
@robodoo robodoo removed the CI 🤖 label Nov 8, 2019
@william-andre william-andre force-pushed the odoo-dev:master-reconcile-model-lines-wan branch 3 times, most recently from 7632e30 to f533f96 Nov 8, 2019
@robodoo robodoo added the CI 🤖 label Nov 12, 2019
Task 2046908
Instead of having all the fields duplicated with second_*, we have now a
o2m allowing us to
* have more than 2 lines
* reduce duplicated code
* fix bugs and add features at only one place

We also remove the computation of writeoff and suggestions from the
client side as some code was 4-upled before (twice in in client and
twice in server side). The logic is now only at one place.
@william-andre william-andre force-pushed the odoo-dev:master-reconcile-model-lines-wan branch from f533f96 to 7b0b5bb Nov 13, 2019
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.