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

[MIG] account_global_discount: 16.0 #1338

Closed
wants to merge 49 commits into from

Conversation

ferran-S73
Copy link
Contributor

@ferran-S73 ferran-S73 commented Jan 4, 2023

Depends on:

Hi I'm gonna need some help migrating this module @OCA/accounting-maintainers @kirca @omar7r @pedrobaeza

Many accounting code has been refactored in v16 and so my approach here, since there's no _recompute_tax_lines nor _recompute_dynamic_lines is to do this module's calculations whenever an invoice is created or the field global_discount_ids is written following indications from Odoo's commit description.

I'm not quite getting the desired results and I was wondering if you could help me pointing if this is a viable approach or if you have any other idea.
I also looked at this migration but can't seem to figure how could I do something similar here.

Thanks in advance to anyone who can help

In order to adapt this module to the new way of handling invoices in 16.0 I moved the creation / update of discount lines to the create and write methods of the account.move. We no longer need the recalc the bas amount for the discount lines since it is done automatically by Odoo as well as the total amounts of the invoice.

Also, for the field global_discount_item which was needed because invoice_global_discount_id wasn't properly filled, I checked and that last field is now handled correctly. I'm not removing the global_discount_item altogether for compatibility reasons with old invoices since there's no clear way to migrate it to fill invoice_global_discount_id.

@pedrobaeza
Copy link
Member

I can't say too much. Maybe you have to override _inverse_tax_totals.

@pedrobaeza pedrobaeza added this to the 16.0 milestone Jan 4, 2023
@ferran-S73 ferran-S73 force-pushed the 16.0-mig-account_global_discount branch 3 times, most recently from 4c057fc to c44a654 Compare January 19, 2023 08:22
@ferran-S73
Copy link
Contributor Author

Hey @OCA/accounting-maintainers this should be ready for review now! 🥳

@tafaRU
Copy link
Member

tafaRU commented Feb 10, 2023

/ocabot migration account_global_discount

@OCA-git-bot OCA-git-bot mentioned this pull request Feb 10, 2023
64 tasks
@ferran-S73
Copy link
Contributor Author

Hi @OCA/accounting-maintainers I'm having trouble migrating the tests for this module, for some reason when a global discount is added a new tax line is added too instead of updating the existing one. Does anyone have encountered a similar problem? I'd be very thankful if anyone could help me out here. Thanks in advance

@BT-srieskamp
Copy link

@ferran-S73 not sure if this is the right place for feedback to that module. I tested everything and it looks quite good from a users perspective. But the two new lines at the bottom of the Sale Order and Invoice are not in the right place. Maybe this is already fixed? Can you reproduce this? I added an attachment.
Screenshot 2023-04-25 at 14 01 45

@ferran-S73
Copy link
Contributor Author

@ferran-S73 not sure if this is the right place for feedback to that module. I tested everything and it looks quite good from a users perspective. But the two new lines at the bottom of the Sale Order and Invoice are not in the right place. Maybe this is already fixed? Can you reproduce this? I added an attachment. Screenshot 2023-04-25 at 14 01 45

Hi I'll try to reproduce with the same currency and reach you back with anything I find out @BT-skettler

@Rferri44-S73 Rferri44-S73 force-pushed the 16.0-mig-account_global_discount branch from e455d65 to bb3d463 Compare June 23, 2023 09:07
@ferran-S73 ferran-S73 force-pushed the 16.0-mig-account_global_discount branch 5 times, most recently from 8299c87 to 1c0979a Compare June 28, 2023 15:37
@ferran-S73
Copy link
Contributor Author

@OCA/accounting-maintainers

@rafaelbn
Copy link
Member

rafaelbn commented Aug 3, 2023

/ocabot rebase

@OCA-git-bot
Copy link
Contributor

@rafaelbn The rebase process failed, because command git push --force Studio73 tmp-pr-1338:16.0-mig-account_global_discount failed with output:

remote: Permission to Studio73/account-invoicing.git denied to OCA-git-bot.
fatal: unable to access 'https://github.com/Studio73/account-invoicing/': The requested URL returned error: 403

@rafaelbn
Copy link
Member

rafaelbn commented Aug 3, 2023

Hello @ferran-S73 ,

Is this whay you except?

imagen

I have beed testing and I don't like to much this module, complicated. REAMDE in configuration should be reviewed.

Any case, testing is OK no error, just some maybe UX?

Up to you!

Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functional basic test in runbot 👍🏼

@rafaelbn
Copy link
Member

rafaelbn commented Aug 3, 2023

You can review here @BT-skettler ! 🥳

@BT-srieskamp
Copy link

@ferran-S73 I hope I tested it correctly. On runbot I could see that the summary below the sale order line is aligned (with currency CHF).
Thanks a lot for you effort!

@victoralmau
Copy link
Member

Please, cherry-pick #1531 to commit history

@ferran-S73 ferran-S73 force-pushed the 16.0-mig-account_global_discount branch 2 times, most recently from 5630ec1 to af02d49 Compare August 17, 2023 06:21
omar7r and others added 16 commits March 15, 2024 13:39
… amount_currency and it could fail with constraint
Currently translated at 100.0% (36 of 36 strings)

Translation: account-invoicing-14.0/account-invoicing-14.0-account_global_discount
Translate-URL: https://translation.odoo-community.org/projects/account-invoicing-14-0/account-invoicing-14-0-account_global_discount/it/
odoo/odoo@c9d5f06
has changed the number of arguments on the method `_recompute_tax_lines`,
so we need to adapt the overrides to such change.
Currently translated at 100.0% (35 of 35 strings)

Translation: account-invoicing-14.0/account-invoicing-14.0-account_global_discount
Translate-URL: https://translation.odoo-community.org/projects/account-invoicing-14-0/account-invoicing-14-0-account_global_discount/pt/
In a foreign currency invoice the amounts (amount_total,
amount_untaxed) are expressed in the `invoice` currency while the
accounting entry lines are expressed in the `company` currency.

The base_before_global_discounts has to be expressed in the invoice
currency as well in such invoices because the discounts need to be
computed in the same currency as the other amounts.

Also set the invoice currency_id on the tax line so that it doesn't
default to the company currency, and convert the discount journal entry
amount to the company currnecy.
- Depending on the installed set of modules, the company currency may
  be USD or EUR. If the second case, these tests will fail, so we make
  sure that the company currency is USD for our tests, doing the change
  by SQL, as there's a Python constraint that prevents it.

Not needed in v17 due to: odoo/odoo#107113.
@miguel-S73 miguel-S73 force-pushed the 16.0-mig-account_global_discount branch from 2fd9f4e to df03898 Compare March 15, 2024 12:41
@miguel-S73
Copy link
Contributor

@PauBForgeFlow cherry-pick added.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functional review LGTM :) Thanks!
TT45863

@rafaelbn
Copy link
Member

rafaelbn commented Apr 1, 2024

@@ -0,0 +1 @@
odoo-addon-base_global_discount @ git+https://github.com/OCA/server-backend.git@refs/pull/221/head#subdirectory=setup/base_global_discount

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module is already merged, could you please remove this file in order to pass the tests in Github Actions.

@carolinafernandez-tecnativa

Is there anything else to do? so we can unlock some migrations!
Thanks!

ping @pedrobaeza

@pedrobaeza
Copy link
Member

For unlocking the situation, I have extracted the migration from this PR and remove the depending change in #1724

Closing this one. When the other PR gets unlocked, you can propose the improvement in this module in a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.