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

Master clean reinvoice jem #28939

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
4 participants
@jem-odoo
Copy link
Contributor

jem-odoo commented Nov 22, 2018

@robodoo robodoo added the CI 🤖 label Nov 22, 2018

@jem-odoo jem-odoo added RD and removed CI 🤖 labels Nov 22, 2018

@jem-odoo jem-odoo force-pushed the odoo-dev:master-clean-reinvoice-jem branch Nov 22, 2018

@jem-odoo

This comment has been minimized.

Copy link
Contributor Author

jem-odoo commented Nov 22, 2018

@qdp-odoo @csnauwaert I might need your input here on 2 things:
1/ are you ok to create analytic line in batch when posting a move.line ? (first commit)
2/ Will it cause problem (2nd commit) for the account.move.line to create the sale order line and the analytic lines ? Instead of doing it the AAL creation. Is that shocking ? :)
Thanks ;)

@robodoo robodoo added the CI 🤖 label Nov 22, 2018

@jem-odoo jem-odoo force-pushed the odoo-dev:master-clean-reinvoice-jem branch Jan 24, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Jan 24, 2019

@jem-odoo jem-odoo force-pushed the odoo-dev:master-clean-reinvoice-jem branch Jan 30, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Jan 30, 2019

@jem-odoo jem-odoo force-pushed the odoo-dev:master-clean-reinvoice-jem branch Mar 4, 2019

@robodoo robodoo removed the CI 🤖 label Mar 4, 2019

@jem-odoo jem-odoo force-pushed the odoo-dev:master-clean-reinvoice-jem branch 2 times, most recently to 8577e66 Mar 4, 2019

@robodoo robodoo added the CI 🤖 label Mar 4, 2019

@jem-odoo jem-odoo force-pushed the odoo-dev:master-clean-reinvoice-jem branch from 8577e66 to 8f4ac1c Mar 19, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 19, 2019

@dbeguin
Copy link
Contributor

dbeguin left a comment

Rrrrrrrevuuuuuu

Show resolved Hide resolved addons/sale/models/account_move.py Outdated
sale_line = existing_sale_line_cache.get(map_entry_key)
if sale_line: # already search, so reuse it. sale_line can be sale.order.line record or index of a "to create values" in `sale_line_values_to_create`
map_move_sale_line[move_line.id] = sale_line
existing_sale_line_cache[map_entry_key] = sale_line

This comment has been minimized.

Copy link
@dbeguin

dbeguin Mar 25, 2019

Contributor

As you can easily understand with the comment, you can directly assign both values
map_move_sale_line[move_line.id] = existing_sale_line_cache[map_entry_key] = sale_line

# store it in the cache of existing ones
existing_sale_line_cache[map_entry_key] = len(sale_line_values_to_create) - 1 # save the index of the value to create sale line
# store it in the map_move_sale_line map
map_move_sale_line[move_line.id] = len(sale_line_values_to_create) - 1 # save the index of the value to create sale line

This comment has been minimized.

Copy link
@dbeguin

dbeguin Mar 25, 2019

Contributor

same principle

Show resolved Hide resolved addons/sale/models/account_move.py Outdated
if analytic_accounts: # first, search for the open sales order
sale_orders = self.env['sale.order'].search([('analytic_account_id', 'in', analytic_accounts.ids), ('state', '=', 'sale')], order='create_date DESC')
for sale_order in sale_orders:
mapping[sale_order.analytic_account_id.id] = sale_order

This comment has been minimized.

Copy link
@dbeguin

dbeguin Mar 25, 2019

Contributor

use dict comprehension as you dict is empty

Show resolved Hide resolved addons/sale/models/account_move.py Outdated
Show resolved Hide resolved addons/sale_expense/models/account_move.py

@jem-odoo jem-odoo force-pushed the odoo-dev:master-clean-reinvoice-jem branch from 8f4ac1c to e241c43 Mar 26, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 26, 2019

@jem-odoo jem-odoo force-pushed the odoo-dev:master-clean-reinvoice-jem branch from e241c43 to 6fa5e8e Mar 26, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 26, 2019

@tde-banana-odoo
Copy link
Contributor

tde-banana-odoo left a comment

Code LGTM. It stays a bit complicated to read but computation itself is not easy. So for a cleaning and batch-ization according to me it is ok.

move_to_reinvoice = self.env['account.move.line']
for index, move_line in enumerate(self):
values = values_list[index]
if 'so_line' not in values and (move_line.credit or 0.0) <= (move_line.debit or 0.0) and move_line.product_id.expense_policy not in [False, 'no']:

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 28, 2019

Contributor

Shouldn't we use float compare ?

pricelist=order.pricelist_id.id,
uom=self.product_uom_id.id
).price
if unit_amount == 0.0:

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 28, 2019

Contributor

Float compare ? (or float is zero that I think exists)

This comment has been minimized.

Copy link
@jem-odoo

jem-odoo Apr 1, 2019

Author Contributor

Probably, but with which precision ?
As the product uom is not required on account.move.line, should we use the rounding of the UoM and fallback on dp.get_precision('Product Unit of Measure')?

jem-odoo added some commits Nov 20, 2018

[IMP] account: generate analytic entries in batch
When posting an account.move.line, the ones with an analytic account set
will create analytic entries. As this is purely and simply data
generation,
they might be create in batch, using the new "create multi", in order to
perform only one INSERT query for all the lines to generate.
This commit aims to optimize the analytic.line creation by doing it in
batch.

Task-1911898
[REF] sale,sale_expense: clean and optimize reinvoice code
In the previous commit the analytic entries generation when posting a
move was optimized to be done in batch. The performance is break
when sale module is installed. Indeed, in case of reinvoicing a
analytic line, AAL creation will trigger a sale.line creation.

This mecanism is very inefficient because
1/ AAL creation and Sales line creation are not batched
2/ sale line creation is done after the AAL creation, and then linked
with a `write` operation. So one `create` and one `write` per AAL to
reinvoice.
3/ To determine on which Sales Order to reinvoice, many `search` can
be performed per AAL.

Also, this looks strange that AAL creation might result into a sales line
creation.

This commit changes this in order to optimize and clean the code:
- the account.move.line (that creates the AAL) will also create the
Sales lines
- SO lines creation will be done in batch
- minimize the number of `search` done during the process
- the SO line will be linked to the AAL by passing the SO line id in the
create values of AAL (no `write` operation one AAL and SOL are created).

This was the last part of legacy code of 'sale_analytic.py' that we need
to get rid of. There are still work to do, but I think now, the
business case handled here can now breathe and have a peacefull life.

Task-1911898

@jem-odoo jem-odoo force-pushed the odoo-dev:master-clean-reinvoice-jem branch from 6fa5e8e to a9ff666 Apr 1, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Apr 1, 2019

@jem-odoo

This comment has been minimized.

Copy link
Contributor Author

jem-odoo commented Apr 2, 2019

@robodoo rebase-ff r+

@robodoo robodoo added the r+ 👌 label Apr 2, 2019

robodoo pushed a commit that referenced this pull request Apr 2, 2019

[REF] sale,sale_expense: clean and optimize reinvoice code
In the previous commit the analytic entries generation when posting a
move was optimized to be done in batch. The performance is break
when sale module is installed. Indeed, in case of reinvoicing a
analytic line, AAL creation will trigger a sale.line creation.

This mecanism is very inefficient because
1/ AAL creation and Sales line creation are not batched
2/ sale line creation is done after the AAL creation, and then linked
with a `write` operation. So one `create` and one `write` per AAL to
reinvoice.
3/ To determine on which Sales Order to reinvoice, many `search` can
be performed per AAL.

Also, this looks strange that AAL creation might result into a sales line
creation.

This commit changes this in order to optimize and clean the code:
- the account.move.line (that creates the AAL) will also create the
Sales lines
- SO lines creation will be done in batch
- minimize the number of `search` done during the process
- the SO line will be linked to the AAL by passing the SO line id in the
create values of AAL (no `write` operation one AAL and SOL are created).

This was the last part of legacy code of 'sale_analytic.py' that we need
to get rid of. There are still work to do, but I think now, the
business case handled here can now breathe and have a peacefull life.

Task-1911898

closes #28939

Signed-off-by: Jérome Maes (jem) <jem@openerp.com>
@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Apr 2, 2019

Merge method set to rebase and fast-forward

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Apr 2, 2019

Merged, thanks!

@robodoo robodoo closed this Apr 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.