Skip to content

[16.0][IMP] sale_elaboration: Add multilines and new report#3623

Open
AlvaroRM11 wants to merge 1 commit intoOCA:16.0from
BinhexTeam:16.0-imp-sale_elaboration
Open

[16.0][IMP] sale_elaboration: Add multilines and new report#3623
AlvaroRM11 wants to merge 1 commit intoOCA:16.0from
BinhexTeam:16.0-imp-sale_elaboration

Conversation

@AlvaroRM11
Copy link
Copy Markdown

@OCA-git-bot
Copy link
Copy Markdown
Contributor

Hi @rafaelbn, @CarlosRoca13, @yajo, @sergio-teruel,
some modules you are maintaining are being modified, check this out!

@AlvaroRM11 AlvaroRM11 force-pushed the 16.0-imp-sale_elaboration branch from 04d8f56 to 272be3e Compare March 13, 2025 13:43
Copy link
Copy Markdown
Contributor

@Shide Shide left a comment

Choose a reason for hiding this comment

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

When you doesn't click the line, the text is displayed inline, but when you click the line, it's in multiline format (that's ok).

But on the reports, is always displayed inline. We want to use the minimum space for this text, so...

  • Why do you want multiline?
  • Why show the field to salesman in a different look than the end customer sees it?

image

Do you have any example of a real multiline elaboration? Our customers usually uses few words to indicate a preference in preparation (for the inventory user).

Thanks!

/>
<field
name="elaboration_note"
widget="text"
Copy link
Copy Markdown
Contributor

@Shide Shide Mar 14, 2025

Choose a reason for hiding this comment

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

Why not change the original field from Char to Text?
All reports that use it will be changed and no widget is needed.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you for your review! I was concerned that switching from "Char" to "Text" might delete current users' database information, but after testing, I confirmed that everything is working fine.

@AlvaroRM11
Copy link
Copy Markdown
Author

When you doesn't click the line, the text is displayed inline, but when you click the line, it's in multiline format (that's ok).

But on the reports, is always displayed inline. We want to use the minimum space for this text, so...

* Why do you want multiline?

* Why show the field to salesman in a different look than the end customer sees it?

image

Do you have any example of a real multiline elaboration? Our customers usually uses few words to indicate a preference in preparation (for the inventory user). The

Thanks!

Thank you for your review! I have a customer who requires two separate lines. One for packaging and one for format. I've taken your suggestions into account.

@AlvaroRM11 AlvaroRM11 force-pushed the 16.0-imp-sale_elaboration branch from 272be3e to d056723 Compare March 14, 2025 11:04
Copy link
Copy Markdown

@christian-ramos-tecnativa christian-ramos-tecnativa left a comment

Choose a reason for hiding this comment

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

LGTM

@christian-ramos-tecnativa
Copy link
Copy Markdown

Any new thoughs @Shide?

@AlvaroRM11 AlvaroRM11 requested a review from Shide March 26, 2025 13:43
@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 BinhexTeam tmp-pr-3623:16.0-imp-sale_elaboration failed with output:

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

@rafaelbn
Copy link
Copy Markdown
Member

Please @AlvaroRM11 , review test failing 🔴 and a rebase to check it again. Thanks!

Copy link
Copy Markdown

@alexey-pelykh alexey-pelykh left a comment

Choose a reason for hiding this comment

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

Review: sale_elaboration - Add multilines and new report

Thanks for the contribution. The PR adds a useful feature (elaboration notes on sale order reports) and addresses the reviewer feedback about changing Char to Text. A few observations below.

Key Findings

# Severity File Finding
1 Moderate reports/report_saleorder.xml The <hr class="mt-2" /> is rendered unconditionally for every sale order line, regardless of whether the line has elaboration notes. When the group_elaboration_note_on_order_report group is disabled or the line has no elaboration, the user still sees an extra <hr> after every line. The <hr> should be inside the t-if / groups guard, or placed conditionally. Compare with report_deliveryslip.xml and report_picking_operations.xml which do not add extra visual separators.
2 Moderate reports/report_saleorder.xml The elaboration_notes template comment documents that record can be stock.move or stock.move.line, but here it is called with a sale.order.line. While it works (both have elaboration_ids and elaboration_note via the mixin), it would be good to update the comment in report_base.xml to also mention sale.order.line for clarity and maintainability.
3 Minor reports/report_saleorder.xml Missing XML declaration wrapper: the file uses <data> as root element, whereas the existing delivery slip report uses <odoo> as the root element. OCA convention for 16.0 typically uses <odoo> as the outer wrapper for data files. The report_base.xml and report_picking_operations.xml use <data>, so there is some inconsistency in the existing module, but matching <odoo> from the delivery slip report would be more standard.
4 Minor reports/report_saleorder.xml Missing copyright header. The other report XML files include copyright and license headers (e.g., report_base.xml and report_deliveryslip.xml have <!-- Copyright ... --> blocks). This new file should follow the same pattern.
5 Note __manifest__.py The version is not bumped. Since this PR adds a new feature (new security group, new config setting, new report, field type change), the version should be bumped per OCA guidelines.
6 Note CI Tests are currently failing (red) per the maintainer's comment. The author has been asked to investigate and rebase.
7 Note reports/report_saleorder.xml The xpath targets //t[@t-foreach='lines_to_report']/tr which depends on the structure of sale.report_saleorder_document. If upstream Odoo changes the report structure, this xpath may break. This is a standard risk for report inheritance and acceptable, just noting for awareness.

Summary

The core approach is sound: reusing the existing elaboration_notes template, adding a proper security group with a config toggle, and changing Char to Text as suggested by the prior reviewer. The main actionable issue is finding #1 -- the unconditional <hr> that will appear for every sale order line even when no elaboration notes are present. This should be addressed before merge.

I'm posting as COMMENT rather than REQUEST_CHANGES since this PR already has two approvals and the maintainer has asked the author to fix the failing tests and rebase. The unconditional <hr> issue is worth addressing in that same pass.

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.

7 participants