Skip to content

[18.0][MIG] sale_order_report_hide_tax: migration to 18.0#350

Open
sabrinaRMartin wants to merge 4 commits intoOCA:18.0from
moduon:18.0-mig-sale_order_report_hide_tax
Open

[18.0][MIG] sale_order_report_hide_tax: migration to 18.0#350
sabrinaRMartin wants to merge 4 commits intoOCA:18.0from
moduon:18.0-mig-sale_order_report_hide_tax

Conversation

@sabrinaRMartin
Copy link

@sabrinaRMartin sabrinaRMartin commented Dec 22, 2025

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Watchout your commit structure:

Image

@sabrinaRMartin sabrinaRMartin force-pushed the 18.0-mig-sale_order_report_hide_tax branch 2 times, most recently from c532c18 to 37cb5b3 Compare December 22, 2025 16:43
Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Thanks for the tests. Some changes requested to improve them:

@sabrinaRMartin sabrinaRMartin force-pushed the 18.0-mig-sale_order_report_hide_tax branch 3 times, most recently from d95e5f3 to 9a92de5 Compare January 2, 2026 14:42
Copy link

@Gelojr Gelojr left a comment

Choose a reason for hiding this comment

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

The pull request is not approved. A review is requested.

Functional tests performed

The following functional tests have been executed and their results are correct:
Test 1: Hide tax column when all order lines share the same tax group — OK.
Test 2: Show tax column when different tax groups are used across order lines — OK.
Test 3: Ignore section and note lines when deciding tax column visibility — OK.
Test 4: Multiple taxes in the same tax group on a single line correctly hide the tax column — OK.
Test 6: Single order line with a single tax group behaves correctly — OK.

Test not passed

Test 5: Spanish localization with fiscal position adding withholding tax — NOT OK.

Steps to reproduce the issue

  1. Install and configure the Spanish localization.
  2. Use a Fiscal Position that applies a withholding tax (retención) in SO.
  3. Create a Sales Order for a customer and assign this Fiscal Position to the order.
  4. Add a product line with VAT 21% as the product tax.
  5. Verify that Odoo automatically adds the corresponding withholding tax to the sales order line due to the Fiscal Position.
  6. Print or preview the Sales Order / Quotation report.

Observed behavior

The tax column is not printed in the report, and taxes are not displayed, even though the order line contains taxes belonging to different tax groups (VAT and withholding).

Expected behavior

The tax column should be displayed in the report, since the sales order line includes taxes from different tax groups.

Based on the above, the pull request is not approved and a revision is requested to address this scenario.

@chienandalu
Copy link
Member

Test 5: Spanish localization with fiscal position adding withholding tax — NOT OK.

@sabrinaRMartin please make a test for this and debug the reason why. At first glance, I'd say it should work, but it isn't.

@sabrinaRMartin sabrinaRMartin force-pushed the 18.0-mig-sale_order_report_hide_tax branch from 9a92de5 to 020779a Compare January 7, 2026 09:32
@sabrinaRMartin
Copy link
Author

Test 5: Spanish localization with fiscal position adding withholding tax — NOT OK.

@sabrinaRMartin please make a test for this and debug the reason why. At first glance, I'd say it should work, but it isn't.

@chienandalu The problem arose because we were comparing one recordset with another, and it always returned the same values. In other words, for a line that has group A, B, the tax_group_id also returns group A, B, so the condition for displaying the column was not being met because first_line_tax_group and tax_group_id had the same value.

@sabrinaRMartin
Copy link
Author

@Gelojr @chienandalu could you please check the PR again? Everything should be working fine now.

@sabrinaRMartin sabrinaRMartin force-pushed the 18.0-mig-sale_order_report_hide_tax branch from 020779a to 8cbc119 Compare January 14, 2026 11:32
Copy link

@Gelojr Gelojr left a comment

Choose a reason for hiding this comment

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

After re-running the test where two taxes from different tax groups are applied to the sales order line due to the fiscal position, the taxes column is still not printed in the report despite the taxes belonging to different tax groups.

Image Image Image

@sabrinaRMartin
Copy link
Author

@Gelojr when we have a single product line with taxes from different groups, so the column should not be displayed.
Since a tax position is defined in the sales order, all products are subject to the tax defined by the tax position. In any case, the column should be displayed if the tax on the products belongs to different tax groups.

For example:
A. The column is displayed:

  1. You configure two taxes in different groups (Group A and Group B).
  2. You create a quote with two product lines that have a tax (one from each group).
  3. You define a tax position for the sales order (or directly for the customer).

Here the column should be displayed.

B. The column is NOT displayed:

  1. You create a quote with 2 product lines that have a tax (from the same group).
  2. You define a tax position for the sales order (or directly on the client).

Here, the column should NOT be displayed.

I will modify the README to make it clear how it works.

Copy link

@Gelojr Gelojr left a comment

Choose a reason for hiding this comment

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

Great work on this contribution @sabrinaRMartin. The functional behavior is well thought out and aligned with the goal of avoiding redundant information in the report.

The following tests have been executed to validate the module behavior after the latest changes:

  • Test 1: Hide tax column when all order lines use taxes from the same tax group — OK
  • Test 2: Show tax column when different tax groups are used across order lines — OK
  • Test 3: Ignore section and note lines when evaluating tax groups — OK
  • Test 4: When two taxes from different tax groups are applied on one line or across multiple lines, the tax column is still hidden as long as the tax groups are the same for the whole lines of the sale order (to avoid duplication with the tax summary). Example: Spanish localization fiscal position adds withholding tax (retención) consistently together with VAT — OK
  • Test 5: Different taxes belonging to the same tax group on different lines correctly hide the tax column — OK

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Thanks @sabrinaRMartin !

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

Copy link

@Andrii9090-tecnativa Andrii9090-tecnativa left a comment

Choose a reason for hiding this comment

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

Code&functional review 👍

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants