[16.0][MIG] account_invoice_fixed_discount: Migration to 16.0 #1488

PieterPaulussen · 2023-06-19T14:48:08Z

No description provided.

CRogos · 2023-07-20T13:06:30Z

@PieterPaulussen are you still working on this migration?

PieterPaulussen · 2023-07-20T13:14:09Z

@CRogos Hi, sorry, left this PR a bit hanging at the moment. There are a few fixes that still need to be applied to the code. If you want, you can fork the repo and apply the fixes. I'll see if I can do it later this evening myself otherwise.

CRogos · 2023-07-20T13:29:27Z

Thank you for the quick response. If you can do it in the near future, I'll happy to wait. Contact me if you need some help or when you are ready for review.

FrancoMaxime · 2023-07-23T17:34:07Z

account_invoice_fixed_discount/models/account_move_line.py

+from odoo import api, fields, models
+
+
+class AccountMove(models.Model):


I think it would be better to split the file into two. The first for account_move and the second for account_move_line

FrancoMaxime · 2023-07-23T17:44:56Z

account_invoice_fixed_discount/views/account_move_view.xml

@@ -22,12 +22,6 @@
            >
                <field name="discount_fixed" groups="base.group_no_one" />
            </xpath>
-            <xpath


Why do you remove this?

FrancoMaxime · 2023-07-23T17:45:18Z

account_invoice_fixed_discount/tests/test_account_fixed_discount.py

-from odoo.tests import TransactionCase
+from odoo.tests import SavepointCase


-class TestInvoiceFixedDiscount(TransactionCase):
+class TestInvoiceFixedDiscount(SavepointCase):


SavepointCase class is now deprecated, you should use TransactionCase instead.

FrancoMaxime · 2023-07-23T17:45:53Z

Thanks for contributing @PieterPaulussen

rousseldenis · 2023-07-24T07:59:12Z

@PieterPaulussen This is a duplicate as there is already #1481

CRogos · 2023-07-24T08:24:11Z

Thx for the link to the other PR. Didn't find it, too. But the test of the other PR are only green, because the were all deleted. Nevertheless there is some good changes which can be added here, too.

PieterPaulussen · 2023-07-24T08:33:34Z

@rousseldenis @CRogos I believe the other PR is insufficient. I'm testing the module locally for the migration and noticed that most of the upstream methods do not exist at all. So all the super() calls there seem faulty. They are also not tested using a Form which is suboptimal but only visible during a manual test.

The implementation of the price fields has completely changed in V16.0 compared to the last version of this module by refactoring all the fields to computed fields with inverse onchanges.

My interpretation of the module is that this serves as a 'convenience' module to the user to give explicit discounts. Therefor I'm going to suggest to change the implementation. By reverse calculating the discount percentage per line when setting the fixed_discount and then relying on the core to apply the appropriate downstream logic to update the discount and price_unit fields.

Any thoughts on that?

rousseldenis · 2023-07-24T08:36:16Z

@rousseldenis @CRogos I believe the other PR is insufficient. I'm testing the module locally for the migration and noticed that most of the upstream methods do not exist at all. So all the super() calls there seem faulty. They are also not tested using a Form which is suboptimal but only visible during a manual test.

The implementation of the price fields has completely changed in V16.0 compared to the last version of this module by refactoring all the fields to computed fields with inverse onchanges.

My interpretation of the module is that this serves as a 'convenience' module to the user to give explicit discounts. Therefor I'm going to suggest to change the implementation. By reverse calculating the discount percentage per line when setting the fixed_discount and then relying on the core to apply the appropriate downstream logic to update the discount and price_unit fields.

Any thoughts on that?

Indeed, I've just saw that.

A refactoring is needed. You take care of that ?

PieterPaulussen · 2023-07-24T08:37:33Z

@rousseldenis working on it now 👍

rafaelbn · 2023-08-03T21:22:39Z

/ocabot migration account_invoice_fixed_discount

OCA-git-bot · 2023-08-03T21:22:46Z

The migration issue (#1250) has not been updated to reference the current pull request because a previous pull request (#1481) is not closed.
Perhaps you should check that there is no duplicate work.
CC @TITUS6304658

rafaelbn · 2023-08-03T21:24:01Z

/ocabot migration account_invoice_fixed_discount

PieterPaulussen · 2023-08-10T09:03:19Z

Sorry, found one more thing which is not 100% perfect.

Can you update discount_fixed, when it is != 0 and the discount field is updated?

video.13.webm

Fixed in latest update.

CRogos · 2023-08-10T09:29:38Z

It is now updated when you set the dicount field to 0, but if you change it from 10% to 20% it stays on 75€

PieterPaulussen · 2023-08-10T10:23:01Z

@CRogos Same issue here as with the sale_fixed_discount regarding the onchanges. I've applied the same onchange context values here in order to support the workflow.

PieterPaulussen · 2023-08-10T10:57:54Z

Great the tests are botched now because of the onchanges. Apparently the O2MForm class does not support inserting context values. I'll have to think of something else.

CRogos · 2023-08-10T11:26:15Z

oh no... I really appreciate all the effort you've already put here.

Have you tried to make discount_fixed a computed attribute (non stored) with an inverse function setting discount?

CRogos · 2023-08-10T15:08:59Z

account_invoice_fixed_discount/reports/report_account_invoice.xml

+        <xpath expr="//t[@t-set='display_discount']" position="after">
+            <t
+                t-set="display_discount_fixed"
+                t-value="any([l.discount_fixed for l in o.invoice_line_ids])"
+            />
+        </xpath>
+        <xpath expr="//th[@t-if='display_discount']" position="before">
+            <th
+                t-if="display_discount_fixed"
+                t-attf-class="text-end {{ 'd-none d-md-table-cell' if report_type == 'html' else '' }}"
+            >
+                <span>Disc. Fixed Amount</span>
+            </th>
+        </xpath>
+        <xpath expr="//td[@t-if='display_discount']" position="before">
+            <td
+                t-if="display_discount_fixed"
+                t-attf-class="text-end {{ 'd-none d-md-table-cell' if report_type == 'html' else '' }}"
+            >
+                <span t-field="line.discount_fixed" />
+            </td>
+        </xpath>


We already talked about to combine the value in the PDF. I would prefer this.

PieterPaulussen · 2023-08-11T11:48:37Z

@CRogos Another update for this module. I've refactored it to improve the usability and now the expected workflow should be supported. Had to dig deep into the tax calculation of Odoo to account for the rounding differences (similar to the behavior of the sale_fixed_discount module. Feel free to try it again.

CRogos · 2023-08-21T11:56:44Z

@PieterPaulussen here are still the issues like in the sale OCA/sale-workflow#2614

Setting the discount to fixed 32€ changes the discount to 10%, but changing it back to 0, leaves the discount to 10%
In the PDF layout Fixed discount and % are not merged into 1 column as it is now in sale

PieterPaulussen · 2023-08-22T07:27:58Z

@PieterPaulussen here are still the issues like in the sale OCA/sale-workflow#2614

Setting the discount to fixed 32€ changes the discount to 10%, but changing it back to 0, leaves the discount to 10%

In the PDF layout Fixed discount and % are not merged into 1 column as it is now in sale

@CRogos Sorry, I made the changes to the code but never pushed it. You can re-test it now.

CRogos · 2023-08-22T11:49:46Z

Yes can confirm it is working.

I discovered one small difference between invoice and quotation. In one PDF we have the currency, in the other it is missing. Not sure which is the better solution, but I think it should be implemented the same.

CRogos · 2023-09-07T16:00:39Z

@pedrobaeza I think this can be merged.

pedrobaeza · 2023-09-07T16:03:50Z

OK, merging blindly due to reviews:

/ocabot merge nobump

OCA-git-bot · 2023-09-07T16:04:02Z

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-1488-by-pedrobaeza-bump-nobump, awaiting test results.

OCA-git-bot · 2023-09-07T16:14:58Z

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

PieterPaulussen force-pushed the 16.0-mig-account_invoice_fixed_discount branch from 32df308 to 2720195 Compare June 20, 2023 08:07

PieterPaulussen force-pushed the 16.0-mig-account_invoice_fixed_discount branch 4 times, most recently from 1469dc0 to 532213c Compare July 23, 2023 09:58

FrancoMaxime suggested changes Jul 23, 2023

View reviewed changes

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

PieterPaulussen force-pushed the 16.0-mig-account_invoice_fixed_discount branch from 532213c to 0f2a152 Compare July 24, 2023 07:57

CRogos mentioned this pull request Jul 24, 2023

[16.0][MIG] account invoice fixed discount: Migration to 16.0 #1481

Closed

PieterPaulussen force-pushed the 16.0-mig-account_invoice_fixed_discount branch 2 times, most recently from d2dc5cd to f06e932 Compare July 24, 2023 11:00

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

PieterPaulussen force-pushed the 16.0-mig-account_invoice_fixed_discount branch 3 times, most recently from 5bbe096 to 97aeb01 Compare July 31, 2023 11:36

OCA-git-bot added this to the 16.0 milestone Aug 3, 2023

OCA-git-bot mentioned this pull request Aug 3, 2023

Migration to version 16.0 #1250

Open

64 tasks

rafaelbn added the needs review label Aug 3, 2023

PieterPaulussen force-pushed the 16.0-mig-account_invoice_fixed_discount branch from 3b073bd to 85b6c40 Compare August 10, 2023 09:03

PieterPaulussen force-pushed the 16.0-mig-account_invoice_fixed_discount branch from 85b6c40 to 3bb50ca Compare August 10, 2023 10:18

PieterPaulussen force-pushed the 16.0-mig-account_invoice_fixed_discount branch from 3bb50ca to b90fb4b Compare August 10, 2023 10:28

CRogos reviewed Aug 10, 2023

View reviewed changes

PieterPaulussen force-pushed the 16.0-mig-account_invoice_fixed_discount branch 2 times, most recently from f5a9fd6 to 48cd1f5 Compare August 11, 2023 11:46

PieterPaulussen force-pushed the 16.0-mig-account_invoice_fixed_discount branch 4 times, most recently from 5c969a8 to b98fa2a Compare August 14, 2023 14:38

[MIG] account_invoice_fixed_discount: Migration to 16.0

3384403

PieterPaulussen force-pushed the 16.0-mig-account_invoice_fixed_discount branch from b98fa2a to 3384403 Compare August 22, 2023 07:26

rousseldenis approved these changes Aug 30, 2023

View reviewed changes

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

OCA-git-bot merged commit 8256eae into OCA:16.0 Sep 7, 2023
6 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[16.0][MIG] account_invoice_fixed_discount: Migration to 16.0 #1488

[16.0][MIG] account_invoice_fixed_discount: Migration to 16.0 #1488

PieterPaulussen commented Jun 19, 2023

CRogos commented Jul 20, 2023

PieterPaulussen commented Jul 20, 2023

CRogos commented Jul 20, 2023

FrancoMaxime Jul 23, 2023

FrancoMaxime Jul 23, 2023

FrancoMaxime Jul 23, 2023

FrancoMaxime commented Jul 23, 2023

rousseldenis commented Jul 24, 2023

CRogos commented Jul 24, 2023

PieterPaulussen commented Jul 24, 2023

rousseldenis commented Jul 24, 2023

PieterPaulussen commented Jul 24, 2023

rafaelbn commented Aug 3, 2023

OCA-git-bot commented Aug 3, 2023

rafaelbn commented Aug 3, 2023

PieterPaulussen commented Aug 10, 2023

CRogos commented Aug 10, 2023

PieterPaulussen commented Aug 10, 2023

PieterPaulussen commented Aug 10, 2023

CRogos commented Aug 10, 2023

CRogos Aug 10, 2023

PieterPaulussen commented Aug 11, 2023

CRogos commented Aug 21, 2023

PieterPaulussen commented Aug 22, 2023

CRogos commented Aug 22, 2023

CRogos commented Sep 7, 2023

pedrobaeza commented Sep 7, 2023

OCA-git-bot commented Sep 7, 2023

OCA-git-bot commented Sep 7, 2023

		from odoo import api, fields, models


		class AccountMove(models.Model):

[16.0][MIG] account_invoice_fixed_discount: Migration to 16.0 #1488

[16.0][MIG] account_invoice_fixed_discount: Migration to 16.0 #1488

Conversation

PieterPaulussen commented Jun 19, 2023

CRogos commented Jul 20, 2023

PieterPaulussen commented Jul 20, 2023

CRogos commented Jul 20, 2023

FrancoMaxime Jul 23, 2023

Choose a reason for hiding this comment

FrancoMaxime Jul 23, 2023

Choose a reason for hiding this comment

FrancoMaxime Jul 23, 2023

Choose a reason for hiding this comment

FrancoMaxime commented Jul 23, 2023

rousseldenis commented Jul 24, 2023

CRogos commented Jul 24, 2023

PieterPaulussen commented Jul 24, 2023

rousseldenis commented Jul 24, 2023

PieterPaulussen commented Jul 24, 2023

rafaelbn commented Aug 3, 2023

OCA-git-bot commented Aug 3, 2023

rafaelbn commented Aug 3, 2023

PieterPaulussen commented Aug 10, 2023

CRogos commented Aug 10, 2023

PieterPaulussen commented Aug 10, 2023

PieterPaulussen commented Aug 10, 2023

CRogos commented Aug 10, 2023

CRogos Aug 10, 2023

Choose a reason for hiding this comment

PieterPaulussen commented Aug 11, 2023

CRogos commented Aug 21, 2023

PieterPaulussen commented Aug 22, 2023

CRogos commented Aug 22, 2023

CRogos commented Sep 7, 2023

pedrobaeza commented Sep 7, 2023

OCA-git-bot commented Sep 7, 2023

OCA-git-bot commented Sep 7, 2023