[16.0][MIG] sale_margin_delivered: Migration to 16.0 #183

pilarvargas-tecnativa · 2023-09-12T08:43:30Z

Supersede: #176

@chienandalu @sergio-teruel please review

…l on delivery purchase price [UPD] README.rst [UPD] Update sale_margin_delivered.pot Update translation files Updated by Update PO files to match POT (msgmerge) hook in Weblate.

…not delivered qty (OCA#43) [FIX] sale_margin_delivered: Set % delivered percent to quantities ordered if the line has not delivered quantities [ADD] icon.png

…es (OCA#44)

…to compute delivered margin. Other module tests fails [UPD] README.rst

…s warning

…yer instead of stock move

… has not quantity delivered TT26105

…e to this Odoo commit OCA/OCB@8ca10a8

We don't want to compute margin based on stock moves for non storable products. This specially true for services and kits. TT33499

legalsylvain

code review.
Apart a dependency question. LGTM.
thanks !

legalsylvain · 2023-09-12T08:46:33Z

sale_margin_delivered/__manifest__.py

+    "website": "https://github.com/OCA/margin-analysis",
+    "category": "Sales",
+    "license": "AGPL-3",
+    "depends": ["sale_stock", "sale_margin_security"],


Suggested change

"depends": ["sale_stock", "sale_margin_security"],

"depends": ["sale_stock"],

Please do not force to install module that are not required.

But you need to depend on this module to be able to apply the permissions of the "sale_margin_security.group_sale_margin_security" group in the "sale_margin_delivered.view_order_form" view. Isn't it?

can be a glue module. something like sale_margin_delivered_security

module A : sale_margin_security deploy a restriction to hide margin to some users

module B : sale_margin_delivered displays real delivered margin. (other feature).

user can want feature B without feature A.
Making both modules working together can be done with a glue module IMO.

I agree with what you say Sylvain, someone who has the need to have those modules separately should feel free to make those changes.

ok ! So I'll propose a PR against this PR.

Done here : Tecnativa#2

sergio-teruel

LGTM!!
Functional review ok

legalsylvain

waiting for some refactoring in the the dependency.

legalsylvain

Functional review on runboat. One comment inline.

other question : why this module doesn't introduced delivered margin at sale.order level, next to the standard one ?

legalsylvain · 2023-09-14T12:30:16Z

sale_margin_delivered/views/sale_margin_delivered_view.xml

+                />
+                <field
+                    name="margin_delivered_percent"
+                    string="% Margin"


Suggested change

string="% Margin"

string="% Margin dlvd. (%)"

This string is error prone IMO. The standard margin (introduced in sale_margin) is name "Margin (%)".

if both fields are displayed, it is not possible for end user to know what are the field displayed. In my opinion, should "Margin dlvd. (%)"

You are right much clearer and avoids leading to errors, thanks for everything :)

pedrobaeza · 2023-09-14T14:04:59Z

Please do a rebase to remove the merge commit.

TT44910

…_margin_delivered_security

pedrobaeza · 2023-09-15T06:18:35Z

/ocabot migration sale_margin_delivered

pedrobaeza · 2023-09-15T06:18:12Z

sale_margin_delivered/migrations/16.0.1.0.0/post-migration.py

+@openupgrade.migrate()
+def migrate(env, version):
+    if env["ir.module.module"].search(
+        [("name", "=", "sale_margin_delivered"), ("state", "=", "installed")]


This is not needed, as if this is being executed is because the module is installed.

pedrobaeza · 2023-09-15T06:18:26Z

sale_margin_delivered/migrations/16.0.1.0.0/post-migration.py

+            ]
+        )
+        if sale_margin_delivered_security:
+            sale_margin_delivered_security.button_immediate_install()


Don't do this, but just change the state to to install.

corrected :)

pedrobaeza · 2023-09-15T06:41:35Z

sale_margin_delivered/migrations/16.0.1.0.0/post-migration.py

+        ]
+    )
+    if sale_margin_delivered_security:
+        sale_margin_delivered_security.state = "installed"


Suggested change

sale_margin_delivered_security.state = "installed"

sale_margin_delivered_security.state = "to install"

…ed_security TT44910

pedrobaeza · 2023-09-15T07:05:07Z

/ocabot merge nobump

OCA-git-bot · 2023-09-15T07:05:13Z

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-183-by-pedrobaeza-bump-nobump, awaiting test results.

OCA-git-bot · 2023-09-15T07:16:56Z

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

legalsylvain · 2023-09-15T13:33:13Z

Hi. I had a unanswered question here : #183 (review)
if anybody can answer...
Thanks !

pedrobaeza · 2023-09-15T13:36:03Z

Hi. I had a unanswered question here : #183 (review) if anybody can answer... Thanks !

It can be an improvement, yes

chienandalu and others added 16 commits September 12, 2023 08:30

[ADD] sale_margin_delivered: New Module

f9dda20

[IMP] sale_margin_delivered: Add new computed field for better contro…

ea49bde

…l on delivery purchase price [UPD] README.rst [UPD] Update sale_margin_delivered.pot Update translation files Updated by Update PO files to match POT (msgmerge) hook in Weblate.

[11.0][FIX] sale_margin_delivered: Set % delivered percent to 0.0 if …

8d367a3

…not delivered qty (OCA#43) [FIX] sale_margin_delivered: Set % delivered percent to quantities ordered if the line has not delivered quantities [ADD] icon.png

[11.0][FIX] sale_margin_delivered: Fix cost price unit from stock mov…

d5a82cb

…es (OCA#44)

[11.0][IMP] sale_margin_delivered: take an account returned moves

6e2fa18

[11.0][IMP] sale_margin_delivered: Compute avoid use product_uom_qty …

6a19b0a

…to compute delivered margin. Other module tests fails [UPD] README.rst

[IMP] stock_picking_comment_template: Change field label to fix travi…

eefdbc2

…s warning

[IMP] sale_margin_delivered: black, isort

ce8d756

[MIG] sale_margin_delivered: Migration to v13.0

50dad73

[FIX] sale_margin_delivered: Fix compute avco from stock valuation la…

c8614d4

…yer instead of stock move

[FIX] sale_margin_delivered: Fix cache miss error.

dc13a79

[FIX] sale_margin_delivered: Fix division by zero if no price_reduce

cd27202

[FIX] sale_margin_delivered: Delivered values not zero when sale line…

6091a1a

… has not quantity delivered TT26105

[FIX] sale_margin_delivered: Use Form in tests to avoid crash test du…

036585b

…e to this Odoo commit OCA/OCB@8ca10a8

[MIG] sale_margin_delivered: Migration to 15.0

2e56707

[FIX] sale_margin_delivered: non storable

3f31d53

We don't want to compute margin based on stock moves for non storable products. This specially true for services and kits. TT33499

pilarvargas-tecnativa mentioned this pull request Sep 12, 2023

[16.0][MIG] sale_margin_delivered #176

Closed

legalsylvain approved these changes Sep 12, 2023

View reviewed changes

sergio-teruel approved these changes Sep 12, 2023

View reviewed changes

OCA-git-bot added the approved label Sep 12, 2023

carlosdauden approved these changes Sep 12, 2023

View reviewed changes

legalsylvain suggested changes Sep 12, 2023

View reviewed changes

OCA-git-bot removed the approved label Sep 12, 2023

legalsylvain reviewed Sep 14, 2023

View reviewed changes

pilarvargas-tecnativa and others added 2 commits September 15, 2023 08:13

[MIG] sale_margin_delivered: Migration to 16.0

ce6e408

TT44910

[REF] split sale_margin_delivered into sale_margin_delivered and sale…

2fecd69

…_margin_delivered_security

pilarvargas-tecnativa force-pushed the 16.0-mig-sale_margin_delivered branch from 9cae375 to 8bb1c62 Compare September 15, 2023 06:14

OCA-git-bot added this to the 16.0 milestone Sep 15, 2023

pedrobaeza reviewed Sep 15, 2023

View reviewed changes

OCA-git-bot mentioned this pull request Sep 15, 2023

Migration to version 16.0 #142

Open

10 tasks

pilarvargas-tecnativa force-pushed the 16.0-mig-sale_margin_delivered branch from 8bb1c62 to 6ef326b Compare September 15, 2023 06:36

pedrobaeza reviewed Sep 15, 2023

View reviewed changes

[OU-ADD] sale_margin_delivered: Script to install sale_margin_deliver…

febd948

…ed_security TT44910

pilarvargas-tecnativa force-pushed the 16.0-mig-sale_margin_delivered branch from 6ef326b to febd948 Compare September 15, 2023 06:56

OCA-git-bot added the bot is merging ⏳ label Sep 15, 2023

OCA-git-bot merged commit 1c1083f into OCA:16.0 Sep 15, 2023
5 of 7 checks passed

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Sep 15, 2023

pedrobaeza deleted the 16.0-mig-sale_margin_delivered branch September 15, 2023 13:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[16.0][MIG] sale_margin_delivered: Migration to 16.0 #183

[16.0][MIG] sale_margin_delivered: Migration to 16.0 #183

pilarvargas-tecnativa commented Sep 12, 2023 •

edited

Loading

legalsylvain left a comment

legalsylvain Sep 12, 2023

pilarvargas-tecnativa Sep 12, 2023

legalsylvain Sep 12, 2023

carlosdauden Sep 12, 2023 •

edited

Loading

legalsylvain Sep 12, 2023

legalsylvain Sep 13, 2023

sergio-teruel left a comment

legalsylvain left a comment

legalsylvain left a comment

legalsylvain Sep 14, 2023

pilarvargas-tecnativa Sep 15, 2023

pedrobaeza commented Sep 14, 2023

pedrobaeza commented Sep 15, 2023

pedrobaeza Sep 15, 2023

pedrobaeza Sep 15, 2023

pilarvargas-tecnativa Sep 15, 2023

pedrobaeza Sep 15, 2023

pedrobaeza commented Sep 15, 2023

OCA-git-bot commented Sep 15, 2023

OCA-git-bot commented Sep 15, 2023

legalsylvain commented Sep 15, 2023

pedrobaeza commented Sep 15, 2023

	"depends": ["sale_stock", "sale_margin_security"],
	"depends": ["sale_stock"],

	sale_margin_delivered_security.state = "installed"
	sale_margin_delivered_security.state = "to install"

[16.0][MIG] sale_margin_delivered: Migration to 16.0 #183

[16.0][MIG] sale_margin_delivered: Migration to 16.0 #183

Conversation

pilarvargas-tecnativa commented Sep 12, 2023 • edited Loading

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

carlosdauden Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergio-teruel left a comment

Choose a reason for hiding this comment

legalsylvain 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

pedrobaeza commented Sep 14, 2023

pedrobaeza commented Sep 15, 2023

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

pedrobaeza commented Sep 15, 2023

OCA-git-bot commented Sep 15, 2023

OCA-git-bot commented Sep 15, 2023

legalsylvain commented Sep 15, 2023

pedrobaeza commented Sep 15, 2023

pilarvargas-tecnativa commented Sep 12, 2023 •

edited

Loading

carlosdauden Sep 12, 2023 •

edited

Loading