[16.0][MIG] sale_fixed_discount: Migration to 16.0 #2614

PieterPaulussen · 2023-07-21T10:07:00Z

No description provided.

FrancoMaxime · 2023-07-23T17:28:39Z

sale_fixed_discount/__manifest__.py

+    "license": "AGPL-3",
+    "application": False,
+    "installable": True,
+    "depends": ["sale", "account_invoice_fixed_discount"],


Hi, this migration is linked to another pull request, you should make a link to this request by adding it in the requirement.txt file. You can take a look at this documentation https://github.com/OCA/maintainer-tools/wiki/Use-temporary-reference%28s%29-to-another-pull-request%28s%29 to have more information about the procedure

@FrancoMaxime Thanks for the feedback. Didn't know about the test-requirements.txt. Appreciated and added the requirement. I'm first going to fix up the dependency module though prior to completing this one. Going to mark it as WIP.

@PieterPaulussen You're welcome :)

rousseldenis · 2023-07-24T07:56:41Z

/ocabot migration sale_fixed_discount

rousseldenis · 2023-07-24T08:00:55Z

test-requirements.txt

@@ -1 +1,2 @@
 odoo_test_helper
+odoo-addon-sale_fixed_discount @ git+https://github.com/OCA/account-invoicing.git@refs/pull/1488/head#subdirectory=setup/sale_fixed_discount


Suggested change

odoo-addon-sale_fixed_discount @ git+https://github.com/OCA/account-invoicing.git@refs/pull/1488/head#subdirectory=setup/sale_fixed_discount

odoo-addon-account-invoice-fixed-discount @ git+https://github.com/OCA/account-invoicing.git@refs/pull/1488/head#subdirectory=setup/account_invoice_fixed_discount

@PieterPaulussen This line is intended to install the module that is not yet merged. So, account_invoice_fixed_discount.

@PieterPaulussen you still need to adapt the module name at the end of the string. I think you only changed the beginning.

Good work so far.. looking forward to do the testing/review.

I've refactored the flow of this module to align it with the new implementation of account_invoice_fixed_discount and the option arises to remove the requirement of the module entirely as both features can be used as stand-alone. The only problem is that the discount_fixed value does not get updated when invoicing the order.
Perhaps you know of a better way like an optional dependency or a new module entirely?

CRogos

There is a bug when creating the new SO with discount. First the fixed discount is removed, but when saving the form, it is not correct.
video (9).webm

CRogos · 2023-07-24T13:46:16Z

test-requirements.txt

@@ -1 +1,2 @@
 odoo_test_helper
+odoo-addon-sale_fixed_discount @ git+https://github.com/OCA/account-invoicing.git@refs/pull/1488/head#subdirectory=setup/sale_fixed_discount


@PieterPaulussen you still need to adapt the module name at the end of the string. I think you only changed the beginning.

Good work so far.. looking forward to do the testing/review.

CRogos · 2023-07-25T09:56:56Z

sale_fixed_discount/models/sale_order_line.py

+    @api.constrains("discount_fixed", "discount")
+    def _check_discounts(self):
+        """Check that the fixed discount and the discount percentage are consistent."""
+        for line in self:
+            if line.discount_fixed and line.discount:
+                currency = line.currency_id
+                calculated_fixed_discount = float_round(
+                    line._get_discount_from_fixed_discount(),
+                    precision_rounding=currency.rounding,
+                )

-    @api.onchange("discount_fixed")
+                if calculated_fixed_discount != line.discount:
+                    raise ValidationError(
+                        _(
+                            "The fixed discount %(fixed)s does not match the calculated "
+                            "discount %(discount)s %%. Please correct one of the discounts."
+                        )
+                        % {
+                            "fixed": line.discount_fixed,
+                            "discount": line.discount,
+                        }
+                    )


In v15, only discount or discount_fixed was allowed. In the calculation they where implemented additive (even though you could not enter both, and also the further tax calculation wouldn't work properly.)

In v16 they are treated as one and will be both be set. This leads also to a different visualization in the PDF documents, because now both (fixed and %) is added to the PDF:

I think the change is not perfect because of the PDF output.

I am not sure if we should allow fixed + % in future, when we get the tax calculation to handle this correctly. In the v15 tax calculation this wasn't easy to implement, but with the v16 changes this might be possible.
But when we allow both, we need to decide if we use:
price = (unit_price - fixed) * discount
or
price = (unit_price * discount) - fixed

I prefer the first one.

@CRogos Actually, I personally disagree. The fixed discount should only serve as a convenience implementation. All actual discounts are applied through the discount field. This also greatly simplifies the modules code.
So the workflow will always be price = unit_price * discount and nothing more.
I don't see the added benefit of splitting those.

I agree that the discount might be misleading on the PDF though. Perhaps we should not add a separate column for this and just do something like:
$ 100.00 (10.0%) Or the other way around. We could add the currency to it as clarification?

The initial bug you mentioned in your video also needs to be addressed. I'll follow up on the issue this Friday.

Thanks for testing!

@PieterPaulussen I also had a quick view in the new tax calculation and you are right that even in the new implementation we should not change price = unit_price * discount . I like your proposal regarding the PDF $100.00 (10%).

I have one more issue, when we use discount_fixed and discount as synonym. When you enter the discount_fixed, the discount is updated. But when you enter a discount, the dicount_fixed field stays on 0.00.
Shouldn't we make dicount_fixed as computed column which directly updates discount?

@CRogos I've added some additional changes to the module. Particularly one to stabilise the rounding differences for the calculated subtotals.

As for your specific issue where saving the sale order, I believe it is affected on the test environment by a different module also available in the sale-workflow repo. I've checked it locally and was unable to reproduce the error.

rousseldenis · 2023-07-26T08:39:52Z

/ocabot migration sale_fixed_discount

CRogos

@PieterPaulussen are you still working on this?
I've installed the code from your branch, and I ran into several errors.
Either it is not working or I did something wrong.

CRogos · 2023-08-09T16:34:32Z

sale_fixed_discount/views/sale_portal_templates.xml

+            </th>
+        </xpath>
+        <xpath
+            expr="//section[@id='details']//tbody[hasclass('sale_tbody')]//t[@t-foreach='lines_to_report']/tr//t[@t-if='not line.display_type']//td[3]/div"


I get the following error when I install the module from your branch.

Traceback (most recent call last): File "/src/odoo/odoo/http.py", line 1584, in _serve_db return service_model.retrying(self._serve_ir_http, self.env) File "/src/odoo/odoo/service/model.py", line 133, in retrying result = func() File "/src/odoo/odoo/http.py", line 1611, in _serve_ir_http response = self.dispatcher.dispatch(rule.endpoint, args) File "/src/odoo/odoo/http.py", line 1815, in dispatch result = self.request.registry['ir.http']._dispatch(endpoint) File "/src/odoo/odoo/addons/base/models/ir_http.py", line 154, in _dispatch result = endpoint(**request.params) File "/src/odoo/odoo/http.py", line 697, in route_wrapper result = endpoint(self, *args, **params_ok) File "/usr/lib/python3/dist-packages/odoo/addons/web/controllers/dataset.py", line 46, in call_button action = self._call_kw(model, method, args, kwargs) File "/usr/lib/python3/dist-packages/odoo/addons/web/controllers/dataset.py", line 33, in _call_kw return call_kw(request.env[model], method, args, kwargs) File "/src/odoo/odoo/api.py", line 461, in call_kw result = _call_kw_multi(method, model, args, kwargs) File "/src/odoo/odoo/api.py", line 448, in _call_kw_multi result = method(recs, *args, **kwargs) File "<decorator-gen-74>", line 2, in button_immediate_install File "/src/odoo/odoo/addons/base/models/ir_module.py", line 74, in check_and_log return method(self, *args, **kwargs) File "/src/odoo/odoo/addons/base/models/ir_module.py", line 456, in button_immediate_install return self._button_immediate_function(type(self).button_install) File "/src/odoo/odoo/addons/base/models/ir_module.py", line 580, in _button_immediate_function registry = modules.registry.Registry.new(self._cr.dbname, update_module=True) File "<decorator-gen-14>", line 2, in new File "/src/odoo/odoo/tools/func.py", line 87, in locked return func(inst, *args, **kwargs) File "/src/odoo/odoo/modules/registry.py", line 90, in new odoo.modules.load_modules(registry, force_demo, status, update_module) File "/src/odoo/odoo/modules/loading.py", line 488, in load_modules processed_modules += load_marked_modules(cr, graph, File "/src/odoo/odoo/modules/loading.py", line 372, in load_marked_modules loaded, processed = load_module_graph( File "/src/odoo/odoo/modules/loading.py", line 231, in load_module_graph load_data(cr, idref, mode, kind='data', package=package) File "/src/odoo/odoo/modules/loading.py", line 71, in load_data tools.convert_file(cr, package.name, filename, idref, mode, noupdate, kind) File "/src/odoo/odoo/tools/convert.py", line 763, in convert_file convert_xml_import(cr, module, fp, idref, mode, noupdate) File "/src/odoo/odoo/tools/convert.py", line 829, in convert_xml_import obj.parse(doc.getroot()) File "/src/odoo/odoo/tools/convert.py", line 749, in parse self._tag_root(de) File "/src/odoo/odoo/tools/convert.py", line 709, in _tag_root raise ParseError(msg) from None # Restart with "--log-handler odoo.tools.convert:DEBUG" for complete traceback odoo.tools.convert.ParseError: while parsing None:7 Error while validating view: Element '<xpath expr="//section[@id='details']//table/tbody//t[@t-foreach='sale_order.order_line']/tr//t[@t-if='not line.display_type']//td[3]/div">' cannot be located in parent view View error context: {'file': '/src/user/modules/oca/sale-workflow/sale_fixed_discount/views/sale_portal_templates.xml', 'line': 2, 'name': 'sale_order_portal_content_fixed_discount', 'view': ir.ui.view(870,), 'view.model': False, 'view.parent': ir.ui.view(861,), 'xmlid': 'sale_order_portal_content_fixed_discount'}

can't we change it to:

<xpath expr="//div[@t-field='line.price_unit']" position="replace" > <div t-if="line.discount >= 0 or line.fixed_discount >= 0" t-field="line.price_unit" t-att-style="(line.discount or line.discount_fixed) and 'text-decoration: line-through' or None" t-att-class="((line.discount or line.discount_fixed) and 'text-danger' or '') + ' text-right'" /> <div t-if="line.discount_fixed"> <t t-esc="line.price_unit - line.discount_fixed" t-options='{"widget": "float", "decimal_precision": "Product Price"}' /> </div> </xpath>

~~There's a warning from pylint-odoo when you use replace. I specifically had to refactor this in order to not get the warning.~~

I wasn't happy using replace either. Something which should be avoided so modules do not overwrite each other. But I did not have a good idea in this case.

To bypass this warning you can set a priority="100" in the template node.

I'll have a look on your solution.

Please disregard my previous comment. My assumption was wrong. I did however test the module again and did not come across your installation errors.
I ran it on a new demo database and only installed sale_fixed_discount and manually added sale_management. Nothing more.
Are you experiencing a side effect from another module in your installation?

CRogos · 2023-08-09T16:34:37Z

sale_fixed_discount/views/sale_portal_templates.xml

+            </div>
+        </xpath>
+        <xpath
+            expr="//section[@id='details']//table/tbody//t[@t-foreach='lines_to_report']/tr//t[@t-if='not line.display_type']//td[@t-if='display_discount']"


I also get here an error and replaced it with:
expr="//section[@id='details']//t[@t-if='not line.display_type']//td[@t-if='display_discount']"

PieterPaulussen · 2023-08-10T08:50:51Z

@PieterPaulussen are you still working on this? I've installed the code from your branch, and I ran into several errors. Either it is not working or I did something wrong.

I was unable to reproduce the issue. Are you experiencing a side effect from another module in your installation?

CRogos · 2023-08-10T09:03:14Z

Yes you are right... I proberbly had an installation issue on the sale-workflow part.

But there is the same issue you've fixed in account-invoicing part, but the behavior is a little different, because the (sub-)total is not chaging when you change the discount, after setting the fixed-discount.

And there is an error message on save.

PieterPaulussen · 2023-08-10T10:21:59Z

@CRogos I've run into an issue with the framework. Adding onchanges to both discount and discount_fixed has an adverse effect on the workflow.
I've added context values in the forms to trigger the proper workflow when either field is changed. Personally I don't really like the approach, but its a decent tradeoff to restore the proper onchange functionality.

CRogos · 2023-08-17T08:54:47Z

@PieterPaulussen are you still working on this? On my last test, setting the fixed discount to a value and than resetting it to 0 wasn't working.

instead of `sale.group_discount_per_so_line`

The sale order view in the portal was not displaying the fixed discount.

PieterPaulussen · 2023-09-11T12:41:59Z

Rebased and updated with new pre-commit changes

CRogos · 2023-09-11T13:40:04Z

@pedrobaeza can you merge this PR?

pedrobaeza · 2023-09-11T13:51:13Z

I let @rousseldenis to give their blessing

CRogos · 2023-09-19T07:52:11Z

@rousseldenis will you add some feedback on this PR?

CRogos · 2023-10-01T12:51:12Z

@pedrobaeza I don't think there will be further feedback... can we continue without @rousseldenis ?

pedrobaeza · 2023-10-01T20:33:49Z

/ocabot merge nobump

OCA-git-bot · 2023-10-01T20:33:57Z

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

OCA-git-bot · 2023-10-01T20:47:05Z

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

PieterPaulussen changed the title ~~16.0 mig sale fixed discount~~ [16.0][MIG] sale_fixed_discount: Migration to 16.0 Jul 21, 2023

PieterPaulussen mentioned this pull request Jul 21, 2023

Migration to version 16.0 #2215

Open

97 tasks

FrancoMaxime suggested changes Jul 23, 2023

View reviewed changes

PieterPaulussen force-pushed the 16.0-mig-sale_fixed_discount branch from 6115186 to 711e764 Compare July 24, 2023 07:25

PieterPaulussen changed the title ~~[16.0][MIG] sale_fixed_discount: Migration to 16.0~~ [WIP] [16.0][MIG] sale_fixed_discount: Migration to 16.0 Jul 24, 2023

OCA-git-bot added this to the 16.0 milestone Jul 24, 2023

rousseldenis reviewed Jul 24, 2023

View reviewed changes

PieterPaulussen force-pushed the 16.0-mig-sale_fixed_discount branch 4 times, most recently from f6e0ae6 to 027b356 Compare July 24, 2023 15:04

PieterPaulussen changed the title ~~[WIP] [16.0][MIG] sale_fixed_discount: Migration to 16.0~~ [16.0][MIG] sale_fixed_discount: Migration to 16.0 Jul 24, 2023

CRogos suggested changes Jul 25, 2023

View reviewed changes

rousseldenis added needs review migration labels Jul 26, 2023

PieterPaulussen force-pushed the 16.0-mig-sale_fixed_discount branch 4 times, most recently from 86a812f to 27b9a26 Compare August 4, 2023 09:40

CRogos suggested changes Aug 9, 2023

View reviewed changes

PieterPaulussen force-pushed the 16.0-mig-sale_fixed_discount branch from 27b9a26 to f4ee788 Compare August 10, 2023 10:20

PieterPaulussen force-pushed the 16.0-mig-sale_fixed_discount branch from f4ee788 to 7230972 Compare August 14, 2023 14:39

PieterPaulussen force-pushed the 16.0-mig-sale_fixed_discount branch 2 times, most recently from c4b1d4c to 77aac31 Compare August 18, 2023 12:35

JordiBForgeFlow and others added 17 commits September 11, 2023 14:40

[11.0][mig] sale_fixed_discount

ed51d34

[MIG] sale_fixed_discount: Migration to 12.0

9b92fed

[IMP] Simplify code

08f682c

[IMP] sale_fixed_discount: black, isort, prettier

0df4c03

[MIG] sale_fixed_discount: Migration to 13.0

150e4dd

Not update lines when SO is locked

441e53a

[UPD] Eficent -> ForgeFlow

e814adc

[FIX] sale_fixed_discount: using product.group_discount_per_so_line

08308a7

instead of `sale.group_discount_per_so_line`

[FIX] sale_fixed_discount: remove obsolete colspan calculation

8e3356d

[13.0][sale_fixed_discount] fix sale portal view

1eb2568

The sale order view in the portal was not displaying the fixed discount.

[IMP] sale_fixed_discount: black, isort, prettier

199814f

[MIG] sale_fixed_discount: Migration to 14.0

8441200

[MIG] sale_fixed_discount: Migration to 15.0

e83b441

[UPD] Update sale_fixed_discount.pot

ce88e20

[UPD] README.rst

d459dae

[IMP] : pre-commit stuff

5d0e395

[MIG] sale_fixed_discount: Migration to 16.0

548b852

PieterPaulussen force-pushed the 16.0-mig-sale_fixed_discount branch from b211163 to 548b852 Compare September 11, 2023 12:41

OCA-git-bot added the bot is merging ⏳ label Oct 1, 2023

OCA-git-bot merged commit 08a7deb into OCA:16.0 Oct 1, 2023
6 checks passed

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Oct 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[16.0][MIG] sale_fixed_discount: Migration to 16.0 #2614

[16.0][MIG] sale_fixed_discount: Migration to 16.0 #2614

PieterPaulussen commented Jul 21, 2023

FrancoMaxime Jul 23, 2023

PieterPaulussen Jul 24, 2023

FrancoMaxime Jul 27, 2023

rousseldenis commented Jul 24, 2023

rousseldenis Jul 24, 2023

CRogos Jul 24, 2023

PieterPaulussen Jul 24, 2023 •

edited

Loading

CRogos left a comment

CRogos Jul 24, 2023

CRogos Jul 25, 2023

PieterPaulussen Jul 26, 2023

CRogos Jul 26, 2023

PieterPaulussen Jul 31, 2023

rousseldenis commented Jul 26, 2023

CRogos left a comment

CRogos Aug 9, 2023

PieterPaulussen Aug 10, 2023 •

edited

Loading

CRogos Aug 10, 2023

PieterPaulussen Aug 10, 2023

CRogos Aug 9, 2023

PieterPaulussen commented Aug 10, 2023

CRogos commented Aug 10, 2023

PieterPaulussen commented Aug 10, 2023

CRogos commented Aug 17, 2023

PieterPaulussen commented Sep 11, 2023

CRogos commented Sep 11, 2023

pedrobaeza commented Sep 11, 2023

CRogos commented Sep 19, 2023

CRogos commented Oct 1, 2023

pedrobaeza commented Oct 1, 2023

OCA-git-bot commented Oct 1, 2023

OCA-git-bot commented Oct 1, 2023

		@@ -1 +1,2 @@
		odoo_test_helper
		odoo-addon-sale_fixed_discount @ git+https://github.com/OCA/account-invoicing.git@refs/pull/1488/head#subdirectory=setup/sale_fixed_discount

	odoo-addon-sale_fixed_discount @ git+https://github.com/OCA/account-invoicing.git@refs/pull/1488/head#subdirectory=setup/sale_fixed_discount
	odoo-addon-account-invoice-fixed-discount @ git+https://github.com/OCA/account-invoicing.git@refs/pull/1488/head#subdirectory=setup/account_invoice_fixed_discount

[16.0][MIG] sale_fixed_discount: Migration to 16.0 #2614

[16.0][MIG] sale_fixed_discount: Migration to 16.0 #2614

Conversation

PieterPaulussen commented Jul 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rousseldenis commented Jul 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PieterPaulussen Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

CRogos 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

rousseldenis commented Jul 26, 2023

CRogos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PieterPaulussen Aug 10, 2023 • edited Loading

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

PieterPaulussen commented Aug 10, 2023

CRogos commented Aug 10, 2023

PieterPaulussen commented Aug 10, 2023

CRogos commented Aug 17, 2023

PieterPaulussen commented Sep 11, 2023

CRogos commented Sep 11, 2023

pedrobaeza commented Sep 11, 2023

CRogos commented Sep 19, 2023

CRogos commented Oct 1, 2023

pedrobaeza commented Oct 1, 2023

OCA-git-bot commented Oct 1, 2023

OCA-git-bot commented Oct 1, 2023

PieterPaulussen Jul 24, 2023 •

edited

Loading

PieterPaulussen Aug 10, 2023 •

edited

Loading