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

[FIX] purchase: real time average multicurrency valuation #32180

Closed

Conversation

Projects
None yet
4 participants
@kebeclibre
Copy link
Contributor

kebeclibre commented Mar 27, 2019

Do a PO in a foreign currency
Invoice it in the same foreign currency
Validate the invoice

Before this commit, The account move of the invoice
contained a price difference line
which is wrong, since the price has not changed in any ways

After this commit, the move has only two lines
one in the stock input, one in the payable

Also, the case where the price does change
due to a change in currency rate is handled

OPW 1955315

@robodoo robodoo added the seen 🙂 label Mar 27, 2019

@kebeclibre

This comment has been minimized.

Copy link
Contributor Author

kebeclibre commented Mar 27, 2019

@nim-odoo
It is a WIP obviously
I'm gonna write a couple of tests:

  • one for the use case in OPW 1955315
  • one for a PO and invoices having different dates

@C3POdoo C3POdoo added the RD label Mar 27, 2019

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

@kebeclibre kebeclibre force-pushed the odoo-dev:11.0-purchase-realtime-average-lpe branch from fa0acaf to 83e0836 Mar 28, 2019

@kebeclibre kebeclibre changed the title Fixeroo [FIX] purchase: real time average multicurrency valuation Mar 28, 2019

@kebeclibre kebeclibre requested review from sle-odoo and nim-odoo Mar 28, 2019

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

@nim-odoo
Copy link
Contributor

nim-odoo left a comment

What if the PO currency, the invoice currency and the company currency are all different?

Show resolved Hide resolved addons/purchase/tests/test_stockvaluation.py Outdated
Show resolved Hide resolved addons/purchase/tests/test_stockvaluation.py
Show resolved Hide resolved addons/purchase/tests/test_stockvaluation.py Outdated
Show resolved Hide resolved addons/purchase/tests/test_stockvaluation.py Outdated
Show resolved Hide resolved addons/purchase/tests/test_stockvaluation.py Outdated
Show resolved Hide resolved addons/purchase/tests/test_stockvaluation.py Outdated
Show resolved Hide resolved addons/purchase/models/account_invoice.py Outdated
Show resolved Hide resolved addons/purchase/models/account_invoice.py Outdated
Show resolved Hide resolved addons/purchase/models/account_invoice.py Outdated
Show resolved Hide resolved addons/purchase/models/account_invoice.py

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

@kebeclibre kebeclibre force-pushed the odoo-dev:11.0-purchase-realtime-average-lpe branch from 2688cd3 to 615c0d0 Mar 28, 2019

@kebeclibre

This comment has been minimized.

Copy link
Contributor Author

kebeclibre commented Mar 28, 2019

@nim-odoo
changes made

@kebeclibre kebeclibre force-pushed the odoo-dev:11.0-purchase-realtime-average-lpe branch 3 times, most recently from f85e595 to 925f451 Mar 28, 2019

Show resolved Hide resolved addons/purchase/tests/test_stockvaluation.py Outdated
Show resolved Hide resolved addons/purchase/tests/test_stockvaluation.py Outdated
Show resolved Hide resolved addons/purchase/models/account_invoice.py
Show resolved Hide resolved addons/purchase/models/account_invoice.py Outdated

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

@kebeclibre kebeclibre force-pushed the odoo-dev:11.0-purchase-realtime-average-lpe branch from 925f451 to b4b15de Mar 28, 2019

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

@kebeclibre kebeclibre force-pushed the odoo-dev:11.0-purchase-realtime-average-lpe branch from b4b15de to d90fc10 Mar 29, 2019

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

@kebeclibre

This comment has been minimized.

Copy link
Contributor Author

kebeclibre commented Mar 29, 2019

@nim-odoo
@sle-odoo
@qdp-odoo

I'm pretty sure I'm right about the analysis in the comments of the second test, but I'd like some confirmation anyways :D

@nim-odoo
Copy link
Contributor

nim-odoo left a comment

Looks better to me now.

Testing could still be improved, though. You tested the 'Average' costing method, but not FIFO or standard. I think it is worth testing, since you also modified the logic for the standard costing method in https://github.com/odoo/odoo/pull/32180/files#diff-0f1ebb3dd2d9f9251c966ddc2226def0R189

I don't think it's too much extra work since you could have a PO with 3 products, one for each costing method.

Show resolved Hide resolved addons/purchase/tests/test_stockvaluation.py Outdated

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

@nim-odoo

This comment has been minimized.

Copy link
Contributor

nim-odoo commented Mar 29, 2019

@lap-odoo
There are some use cases here that are worth testing: stock valuation with multi-currency. The PO and the vendor bills are created in a currency different from the company currency.

I think we now cover the 3 costing methods, possibly with some exchange rate fluctuations in the process. Although not tested, I think the PO, the vendor bill and the company currency could all be different as well.

We could also sprinkle multiple UOM, and it's gonna be everything you've ever dreamed about.

@kebeclibre

This comment has been minimized.

Copy link
Contributor Author

kebeclibre commented Apr 1, 2019

@oco-odoo
shit, just saw https://github.com/odoo/odoo/pull/22483/files

Also, my use cases are not tested in the your saas-11.3 revision

@robodoo robodoo removed the CI 🤖 label Apr 2, 2019

@kebeclibre

This comment has been minimized.

Copy link
Contributor Author

kebeclibre commented Apr 2, 2019

@lap-odoo
Multicurrency, average, fifo, standard valuation in realtime
The branch is up for testing

Also, If you are up to it and don't mind. I'd like feedback on my analysis at https://github.com/odoo/odoo/pull/32180/files#diff-992a918f29175f122ff7fdd224ed4c64R638

and
https://github.com/odoo/odoo/pull/32180/files#diff-992a918f29175f122ff7fdd224ed4c64R804

@kebeclibre kebeclibre force-pushed the odoo-dev:11.0-purchase-realtime-average-lpe branch from 50a568a to adf2180 Apr 2, 2019

@robodoo robodoo added the CI 🤖 label Apr 2, 2019

@nim-odoo
Copy link
Contributor

nim-odoo left a comment

I think you are getting closer to the end.

There is an extra remark: the method is becoming more and more difficult to understand, and variable names could be more meaningful, e.g.:

valuation_price_unit => valuation_pu_company_curr
line_price_unit_company_currency => inv_line_pu_company_curr
line['price_unit'] => use a new variable po_line_pu_company_curr

So from the variable name, you know:

  • which price you are referring to
  • in which currency

That would require a bit of refactoring, but I think it is for the best.

if order_id.currency_id != order_id.company_id.currency_id:
valuation_price_unit = (
order_id.currency_id
.with_context(date=i_line.purchase_line_id.order_id.date_approve)

This comment has been minimized.

Copy link
@nim-odoo

nim-odoo Apr 5, 2019

Contributor

.with_context(date=order_id.date_approve) is enough

)

# Valuation price unit and line_price_unit_company_currency in Company Currency
# line['price_unit'] and i_line.price_unit both in the currency of the invoice, foreign or not

This comment has been minimized.

Copy link
@nim-odoo

nim-odoo Apr 5, 2019

Contributor

I don't think we can make this assumption, although it's going to be true in most cases.

@@ -194,6 +220,14 @@ def _anglo_saxon_purchase_move_lines(self, i_line, res):
for child in tax.children_tax_ids:
if child.type_tax_use != 'none':
tax_ids.append((4, child.id, None))

if inv.currency_id.id != company_currency.id:
valuation_price_unit = (

This comment has been minimized.

Copy link
@nim-odoo

nim-odoo Apr 5, 2019

Contributor

So the same variable name now changes currency... not very clear. Maybe use a new variable:
valuation_pu_inv_curr

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

@kebeclibre kebeclibre force-pushed the odoo-dev:11.0-purchase-realtime-average-lpe branch from 8363c7d to 2a656bb Apr 11, 2019

@robodoo robodoo removed the CI 🤖 label Apr 11, 2019

@kebeclibre

This comment has been minimized.

Copy link
Contributor Author

kebeclibre commented Apr 11, 2019

@nim-odoo
@oco-odoo
I confirmed the values with @lap-odoo
Also #32616 takes care of the issue in saas-11.3, and also confirms the values and logic with minimal diff
Provided that those PR appear green, I'm merging next monday

@robodoo robodoo added the CI 🤖 label Apr 11, 2019

@nim-odoo

This comment has been minimized.

Copy link
Contributor

nim-odoo commented Apr 15, 2019

@nim-odoo
@oco-odoo
I confirmed the values with @lap-odoo
Also #32616 takes care of the issue in saas-11.3, and also confirms the values and logic with minimal diff
Provided that those PR appear green, I'm merging next monday

I stil lthink a slight renaming of the variables would be useful for future modifications.

@robodoo robodoo removed the CI 🤖 label Apr 15, 2019

@kebeclibre

This comment has been minimized.

Copy link
Contributor Author

kebeclibre commented Apr 15, 2019

@nim-odoo
Variables are renamed, and yes, this PR shouldn't be forwardported, and I wrote that in the code

@robodoo robodoo added the CI 🤖 label Apr 15, 2019

@nim-odoo nim-odoo self-requested a review Apr 15, 2019

@nim-odoo
Copy link
Contributor

nim-odoo left a comment

OK then, just squash and merge!

[FIX] purchase: real time average multicurrency valuation
Do a PO in a foreign currency
Invoice it in the same foreign currency
Validate the invoice

Before this commit, The account move of the invoice
contained a price difference line
which is wrong, since the price has not changed in any ways

After this commit, the move has only two lines
one in the stock input, one in the payable

Also, the case where the price does change
due to a change in currency rate is handled

OPW 1955315

@kebeclibre kebeclibre force-pushed the odoo-dev:11.0-purchase-realtime-average-lpe branch from c7c38ed to dcf8848 Apr 16, 2019

@robodoo robodoo removed the CI 🤖 label Apr 16, 2019

@kebeclibre

This comment has been minimized.

Copy link
Contributor Author

kebeclibre commented Apr 16, 2019

robodoo r+

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

[FIX] purchase: real time average multicurrency valuation
Do a PO in a foreign currency
Invoice it in the same foreign currency
Validate the invoice

Before this commit, The account move of the invoice
contained a price difference line
which is wrong, since the price has not changed in any ways

After this commit, the move has only two lines
one in the stock input, one in the payable

Also, the case where the price does change
due to a change in currency rate is handled

OPW 1955315

closes #32180

Signed-off-by: Lucas Perais (lpe) <lpe@odoo.com>
@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Apr 16, 2019

Merged, thanks!

@robodoo robodoo closed this Apr 16, 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.