[MIG] product_pricelist_simulation_margin: Migration to 16.0 #1354

hugo-cordoba · 2023-06-07T11:57:35Z

ADD new module product_pricelist_simulation_margin into version 16.0

legalsylvain

partial review.

could you take a look on the red CI ?
ideally, adding a test could be great.
(I think you can recover code base here https://github.com/OCA/margin-analysis/blob/12.0/product_pricelist_margin/tests/test_module.py and adapt with the new name of the transient model).

legalsylvain · 2023-06-07T12:11:14Z

product_pricelist_simulation_margin/wizards/wizard_preview_pricelist.py

+        readonly=True,
+    )
+
+    bg_color = fields.Char(compute="_compute_bg_color")


I wonder if we should remove bg_color and computation and so one. That was a try in V12, but in fact, the result is not good, and my user asked me to disable the backgrounds.

Not a blocking point. What do you think ?

legalsylvain · 2023-06-07T12:14:27Z

Side question :
do we have to put that module in product-attribute (as it depends on module product_pricelist_simulation) or do we have to put it in margin-analysis, because it is related to margin ?
No idea on that topic. Maybe @pedrobaeza has a point of view.

hugo-cordoba · 2023-06-07T13:22:05Z

hi @legalsylvain.

I haven't finished migrating the module yet, I stopped for lunch and left the PR uploaded so as not to lose it. That's why I hadn't pinged you yet. I'm going to add the test and fix the pre-commit bugs.

Regarding what you comment about 'bg_color', I agree with you, I don't see sense that this is still there, in my experience, I have never used it, nor do I find that it brings a great added value.

Finally, in my opinion, the module should stay in this repository.

pedrobaeza · 2023-06-07T13:27:43Z

Yes, being such tiny extension to a module being hosted here, it would be weird to not be located in the same place.

legalsylvain · 2023-06-07T13:48:59Z

I haven't finished migrating the module yet, I stopped for lunch and left the PR uploaded so as not to lose it. That's why I hadn't pinged you yet. I'm going to add the test and fix the pre-commit bugs.

sorry for the nagging! I was impatient...
Note : at the start of the PR, you can choose to set "draft PR", so it's explicit. Unfortunately, you can not put a non draft PR to draft state, once the PR is created.

Yes, being such tiny extension to a module being hosted here, it would be weird to not be located in the same place.

👍

Regarding what you comment about 'bg_color', I agree with you, I don't see sense that this is still there, in my experience, I have never used it, nor do I find that it brings a great added value.

I think you can so remove it. Less code to maintain.

hugo-cordoba · 2023-06-07T15:17:15Z

@legalsylvain you are correct, I did not put the pr in draft. I have uploaded an update, it should be all correct. I am waiting for your feedback :)

Best regards!

legalsylvain

Just tested on runboat. some things are missing. (or maybe I missed something...)

Configuration :

Result

Problem

The column "Price" should be hidden
The column "Cost" (standard_price) should be visible
The margin is not computed
The margin % is not computed

Thanks !

hugo-cordoba · 2023-06-15T15:12:54Z

Hi @legalsylvain, I haven't forgotten about this, but this week it has been impossible for me to get busy. I'll check it tomorrow!

legalsylvain · 2023-06-15T15:33:17Z

no problem ! take your time. Just ping me when you want a review.

Thanks !

hugo-cordoba · 2023-06-19T10:20:25Z

Hi @legalsylvain, how are you?

I have made the changes you proposed, when you have time, can you check that everything works fine?

Regards!

aliciagaarzo

Code review LGTM!

legalsylvain

functional review. LGTM ! Thanks !

Some minor propositions inline.

regards.

legalsylvain · 2023-06-20T09:14:47Z

product_pricelist_simulation_margin/wizards/wizard_preview_pricelist.py

+        )
+        rslt.update(
+            {
+                "pricelist_id": pricelist.id,


no need to return that value, as the super function return the value. See :
https://github.com/OCA/product-attribute/blob/16.0/product_pricelist_simulation/wizards/wizard_preview_pricelist.py#L59-L67

don't you think ?

legalsylvain · 2023-06-20T09:16:24Z

product_pricelist_simulation_margin/wizards/wizard_preview_pricelist.py

+    _inherit = "wizard.preview.pricelist.line"
+    _description = "wizard - Preview Pricelist Line"
+
+    price_vat_excl = fields.Float(string="Unit Sales Price (Excl.)", readonly=True)


Suggested change

price_vat_excl = fields.Float(string="Unit Sales Price (Excl.)", readonly=True)

price_vat_excl = fields.Monetary(

string="Unit Sales Price (Excl.)",

readonly=True,

currency_field="currency_id",

digits="Price",

)

legalsylvain · 2023-06-20T09:17:04Z

product_pricelist_simulation_margin/wizards/wizard_preview_pricelist.py

+    _description = "wizard - Preview Pricelist Line"
+
+    price_vat_excl = fields.Float(string="Unit Sales Price (Excl.)", readonly=True)
+    price_vat_incl = fields.Float(string="Unit Sales Price (Incl.)", readonly=True)


Suggested change

price_vat_incl = fields.Float(string="Unit Sales Price (Incl.)", readonly=True)

price_vat_incl = fields.Monetary(

string="Unit Sales Price (Incl.)",

readonly=True,

currency_field="currency_id",

digits="Price",

)

legalsylvain · 2023-06-20T09:17:30Z

product_pricelist_simulation_margin/wizards/wizard_preview_pricelist.py

+    price_vat_excl = fields.Float(string="Unit Sales Price (Excl.)", readonly=True)
+    price_vat_incl = fields.Float(string="Unit Sales Price (Incl.)", readonly=True)
+
+    margin = fields.Float(


Suggested change

margin = fields.Float(

margin = fields.Monetary(

legalsylvain · 2023-06-20T09:19:58Z

product_pricelist_simulation_margin/wizards/wizard_preview_pricelist.py

+
+    margin = fields.Float(
+        store=True,
+        digits=dp.get_precision("Product Price"),


Suggested change

digits=dp.get_precision("Product Price"),

digits="Price",

currency_field="currency_id",

legalsylvain · 2023-06-20T09:20:19Z

product_pricelist_simulation_margin/wizards/wizard_preview_pricelist.py

+    margin_percent = fields.Float(
+        string="Margin (%)",
+        store=True,
+        digits=dp.get_precision("Product Price"),


Suggested change

digits=dp.get_precision("Product Price"),

digits="Price",

legalsylvain · 2023-06-20T09:20:40Z

product_pricelist_simulation_margin/wizards/wizard_preview_pricelist.py

+
+from odoo import fields, models
+
+from odoo.addons import decimal_precision as dp


Suggested change

from odoo.addons import decimal_precision as dp

hugo-cordoba · 2023-06-20T10:35:49Z

Hi @legalsylvain! Thanks for the review. I have just made the changes.

I have a question, what is the difference between creating a float field and adding the monetary widget or making the field 'monetary' directly?

legalsylvain · 2023-06-20T10:55:47Z

I have a question, what is the difference between creating a float field and adding the monetary widget or making the field 'monetary' directly?

Float + digits will set allways the same number of digits. Not work in multi currency context.
Monetary + currency will set the number of digits defined in the current currency. For money fields, monetary field is better.

legalsylvain

Thanks for your work !

hugo-cordoba · 2023-06-20T11:04:49Z

Understood, thanks for the explanation @legalsylvain.

@pedrobaeza can you check this pr? Thank you very much!!

pedrobaeza · 2023-06-20T11:17:15Z

/ocabot migration product_pricelist_simulation_margin
/ocabot merge nobump

OCA-git-bot · 2023-06-20T11:17:22Z

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-1354-by-pedrobaeza-bump-nobump, awaiting test results.

OCA-git-bot · 2023-06-20T11:23:12Z

Congratulations, your PR was merged at bc0f253. Thanks a lot for contributing to OCA. ❤️

legalsylvain reviewed Jun 7, 2023

View reviewed changes

hugo-cordoba marked this pull request as draft June 7, 2023 14:40

hugo-cordoba force-pushed the 16.0-mig-product_pricelist_simulation_margin branch 2 times, most recently from db2ca77 to 2e1a8e6 Compare June 7, 2023 15:12

hugo-cordoba marked this pull request as ready for review June 7, 2023 15:15

hugo-cordoba mentioned this pull request Jun 7, 2023

Migration to version 16.0 #1157

Open

55 tasks

hugo-cordoba requested a review from legalsylvain June 8, 2023 11:44

legalsylvain requested changes Jun 8, 2023

View reviewed changes

hugo-cordoba force-pushed the 16.0-mig-product_pricelist_simulation_margin branch 3 times, most recently from 15d326b to 8c8fa24 Compare June 19, 2023 10:14

hugo-cordoba requested a review from legalsylvain June 19, 2023 10:19

aliciagaarzo approved these changes Jun 19, 2023

View reviewed changes

legalsylvain requested changes Jun 20, 2023

View reviewed changes

[MIG] product_pricelist_simulation_margin: Migration to 16.0

ded4554

hugo-cordoba force-pushed the 16.0-mig-product_pricelist_simulation_margin branch from 8c8fa24 to ded4554 Compare June 20, 2023 10:32

legalsylvain approved these changes Jun 20, 2023

View reviewed changes

OCA-git-bot added the approved label Jun 20, 2023

OCA-git-bot added this to the 16.0 milestone Jun 20, 2023

OCA-git-bot added the bot is merging ⏳ label Jun 20, 2023

OCA-git-bot merged commit 890fe23 into OCA:16.0 Jun 20, 2023

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Jun 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MIG] product_pricelist_simulation_margin: Migration to 16.0 #1354

[MIG] product_pricelist_simulation_margin: Migration to 16.0 #1354

hugo-cordoba commented Jun 7, 2023

legalsylvain left a comment

legalsylvain Jun 7, 2023

legalsylvain commented Jun 7, 2023 •

edited

Loading

hugo-cordoba commented Jun 7, 2023 •

edited

Loading

pedrobaeza commented Jun 7, 2023

legalsylvain commented Jun 7, 2023

hugo-cordoba commented Jun 7, 2023

legalsylvain left a comment

hugo-cordoba commented Jun 15, 2023

legalsylvain commented Jun 15, 2023

hugo-cordoba commented Jun 19, 2023

aliciagaarzo left a comment

legalsylvain left a comment

legalsylvain Jun 20, 2023

legalsylvain Jun 20, 2023

legalsylvain Jun 20, 2023

legalsylvain Jun 20, 2023

legalsylvain Jun 20, 2023

legalsylvain Jun 20, 2023

legalsylvain Jun 20, 2023

hugo-cordoba commented Jun 20, 2023

legalsylvain commented Jun 20, 2023

legalsylvain left a comment

hugo-cordoba commented Jun 20, 2023

pedrobaeza commented Jun 20, 2023

OCA-git-bot commented Jun 20, 2023

OCA-git-bot commented Jun 20, 2023

	digits=dp.get_precision("Product Price"),
	digits="Price",
	currency_field="currency_id",


		from odoo import fields, models

		from odoo.addons import decimal_precision as dp

[MIG] product_pricelist_simulation_margin: Migration to 16.0 #1354

[MIG] product_pricelist_simulation_margin: Migration to 16.0 #1354

Conversation

hugo-cordoba commented Jun 7, 2023

legalsylvain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legalsylvain commented Jun 7, 2023 • edited Loading

hugo-cordoba commented Jun 7, 2023 • edited Loading

pedrobaeza commented Jun 7, 2023

legalsylvain commented Jun 7, 2023

hugo-cordoba commented Jun 7, 2023

legalsylvain left a comment

Choose a reason for hiding this comment

hugo-cordoba commented Jun 15, 2023

legalsylvain commented Jun 15, 2023

hugo-cordoba commented Jun 19, 2023

aliciagaarzo left a comment

Choose a reason for hiding this comment

legalsylvain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hugo-cordoba commented Jun 20, 2023

legalsylvain commented Jun 20, 2023

legalsylvain left a comment

Choose a reason for hiding this comment

hugo-cordoba commented Jun 20, 2023

pedrobaeza commented Jun 20, 2023

OCA-git-bot commented Jun 20, 2023

OCA-git-bot commented Jun 20, 2023

legalsylvain commented Jun 7, 2023 •

edited

Loading

hugo-cordoba commented Jun 7, 2023 •

edited

Loading