Skip to content

[16.0][ADD] sale_order_line_no_print: new module#3733

Merged
OCA-git-bot merged 1 commit intoOCA:16.0from
moduon:16.0-add-sale_order_line_hide_in_report
Sep 12, 2025
Merged

[16.0][ADD] sale_order_line_no_print: new module#3733
OCA-git-bot merged 1 commit intoOCA:16.0from
moduon:16.0-add-sale_order_line_hide_in_report

Conversation

@chienandalu
Copy link
Copy Markdown
Member

@chienandalu chienandalu commented May 27, 2025

cc @moduon MT-9255

@rafaelbn @yajo

image

@chienandalu chienandalu force-pushed the 16.0-add-sale_order_line_hide_in_report branch from 5ef80b9 to a46f680 Compare May 28, 2025 11:49
@chienandalu chienandalu requested a review from yajo May 28, 2025 12:59
@ypapouin
Copy link
Copy Markdown
Contributor

@chienandalu , you also need to swap the display logic: add="and line.display_in_report"

@chienandalu
Copy link
Copy Markdown
Member Author

@chienandalu , you also need to swap the display logic: add="and line.display_in_report"

Sure 🤦‍♂️

@chienandalu chienandalu force-pushed the 16.0-add-sale_order_line_hide_in_report branch from a46f680 to 742d226 Compare May 28, 2025 15:19
@chienandalu chienandalu force-pushed the 16.0-add-sale_order_line_hide_in_report branch from 742d226 to b713f8e Compare May 29, 2025 09:14
Copy link
Copy Markdown
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Now it all looks good. However, I'm wondering... Shouldn't we require that you can only hide lines without amount? 🤔 Otherwise it's easy to shoot your own foot, hiding something with value and then the reports make no sense.

Well, one could then add another line that compensates the first one, so the final result is still 0. But that is weird 😵

WDYT @rafaelbn?

@chienandalu
Copy link
Copy Markdown
Member Author

Shouldn't we require that you can only hide lines without amount?

I agree. Changed

@rousseldenis rousseldenis added this to the 16.0 milestone May 30, 2025
widget="boolean_toggle"
options="{'autosave': False}"
optional="hide"
attrs="{'readonly': [('price_total', '!=', 0)], 'invisible': [('price_total', '!=', 0)]}"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thought: I'm not sure if this is enough... I could set total to zero, hide line, and change total. Maybe we should have a constraint in the model itself.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'd be afraid of performance for such case...

Copy link
Copy Markdown
Contributor

@Jaimermaccione Jaimermaccione left a comment

Choose a reason for hiding this comment

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

@chienandalu Functional Review on Runbot environment. The module works as expected, but it does not allow modifying the report display status field when the document is a confirmed invoice.

While this behavior is understood to be intentional, it would be advisable to display a warning message to inform the user that the field cannot be edited in this state, in order to avoid confusion.

Thank you!

@rousseldenis
Copy link
Copy Markdown
Contributor

@chienandalu
Copy link
Copy Markdown
Member Author

Could you explain the difference with

@rousseldenis this goes on the line level without the need of defining sections. It's also meant to be passed to the stock.picking so it's possible to automatically hide some of the lines automatically (there isn't the concept of sections there)

@chienandalu
Copy link
Copy Markdown
Member Author

it does not allow modifying the report display status field when the document is a confirmed invoice

Indeed, we're discussing internally whether to let hiding anything at all in the invoice as it could be problematic

@chienandalu chienandalu force-pushed the 16.0-add-sale_order_line_hide_in_report branch 2 times, most recently from 444345a to be9809d Compare June 9, 2025 15:35
@yajo
Copy link
Copy Markdown
Member

yajo commented Jun 12, 2025

Please check OCA/sale-reporting#329, which complements this PR.

@rafaelbn
Copy link
Copy Markdown
Member

/ocabot rebase

@OCA-git-bot
Copy link
Copy Markdown
Contributor

@rafaelbn The rebase process failed, because command git push --force moduon tmp-pr-3733:16.0-add-sale_order_line_hide_in_report failed with output:

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

@rafaelbn
Copy link
Copy Markdown
Member

Please @chienandalu rebase. And check test please

@rousseldenis we study deeply but it doesn't fit or expand it would be complicated as @chienandalu said.

@chienandalu chienandalu force-pushed the 16.0-add-sale_order_line_hide_in_report branch from be9809d to 648516c Compare June 24, 2025 07:11
@chienandalu chienandalu requested a review from yajo June 24, 2025 07:21
@chienandalu
Copy link
Copy Markdown
Member Author

Ready for review

@chienandalu chienandalu force-pushed the 16.0-add-sale_order_line_hide_in_report branch from 648516c to d596bab Compare June 30, 2025 10:15
just the beggining of the condition-->
<xpath
expr="//t[starts-with(@t-if, 'not line.display_type')]"
position="replace"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@chienandalu Don't you have any solution without replace?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well, this is not really replacing the node, but wrapping it inside a <t> tag. So it's equivalent to:

<xpath expr="//t[starts-with(@t-if, 'not line.display_type')]" position="before">
  <t t-if="line.display_in_report" />
</xpath>
<xpath expr="//t[@t-if='line.display_in_report']" position="inside">
  <xpath expr="//t[starts-with(@t-if, 'not line.display_type')]" position="move">
</xpath>

@chienandalu chienandalu force-pushed the 16.0-add-sale_order_line_hide_in_report branch from d596bab to 58ac276 Compare July 3, 2025 06:42
@OCA-git-bot
Copy link
Copy Markdown
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). 🤖

@chienandalu chienandalu force-pushed the 16.0-add-sale_order_line_hide_in_report branch 2 times, most recently from aa5cc07 to 41951fd Compare July 16, 2025 12:30
@EmilioPascual EmilioPascual force-pushed the 16.0-add-sale_order_line_hide_in_report branch 2 times, most recently from 2542d1b to 1420caf Compare July 22, 2025 14:49
Copy link
Copy Markdown
Contributor

@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.

Congratulations on the work @chienandalu great job!
The following tests were executed and all passed:
Test 1: On a quotation for “Chiquito de la Calzada SA”, hid a 0.00€ info line via Display in report; line did not appear on PDF/portal and totals unchanged — OK.
Test 2: On a quotation for “Lola Flores SL”, attempted to hide a 120.00€ line; toggle was not editable/shown and the line appeared on the report — OK.
Test 3: On a quotation for “Alaska y Dinarama”, hid a 0.00€ line and then set price to 10.00€; the line became visible automatically on PDF/portal — OK.
Test 4: From SO for “Mortadelo y Filemón S.A.” with a hidden 0.00€ line, created an invoice; the hidden line did not appear on the invoice PDF — OK.
Test 5: On a customer invoice for “David Bisbal”, hid a 0.00€ line via Display in report; the line did not appear on the invoice PDF and totals remained the same — OK.

@rafaelbn
Copy link
Copy Markdown
Member

/ocabot merge patch

@OCA-git-bot
Copy link
Copy Markdown
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-3733-by-rafaelbn-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 5c21d83 into OCA:16.0 Sep 12, 2025
15 checks passed
@OCA-git-bot
Copy link
Copy Markdown
Contributor

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

@yajo yajo deleted the 16.0-add-sale_order_line_hide_in_report branch September 15, 2025 08:00
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.

8 participants