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

[FIX] [16.0] partner_industry_secondary: Add an option to display Child Industries first on it's name #1958

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from

Conversation

Shide
Copy link
Contributor

@Shide Shide commented Jan 13, 2025

Added an option to display Child Industries first on it's name.

closes #1954

MT-8595 @moduon @rafaelbn @simahawk @yajo please review if you want :)

@Shide Shide force-pushed the 16.0-better_display_of_industries-partner_industry_secondary branch from abc6a21 to aad9c44 Compare January 13, 2025 11:59
@Shide Shide force-pushed the 16.0-better_display_of_industries-partner_industry_secondary branch from aad9c44 to 8219032 Compare January 13, 2025 12:15
Copy link
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.

Some details in code, nothing very important.

Functionally tested, it works. One slight problem (maybe not?) with whitespace:
image

image

image

image

display_last_child_first = fields.Boolean(
string="Display Child Industries first",
help="Set if you want to show the last child industries first "
"when searching for industries.\nChild (Dad < Grampa < Ancestor)",
Copy link
Member

Choose a reason for hiding this comment

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

issue: this affects display, not search AFAICS. Besides, let's be a bit more formal here 😁

Suggested change
"when searching for industries.\nChild (Dad < Grampa < Ancestor)",
"when displaying industries.\nChild (Parent < Grandparent)",

cat_name = f"{cat_name} ({' < '.join(parent_cats)})"
result.append((cat.id, cat_name))
return result
# Default display (Ancestor / Grandpa / Dad / Child)
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: just for the sake of uniformity

Suggested change
# Default display (Ancestor / Grandpa / Dad / Child)
# Default display (Grandparent / Parent / Child)

@@ -7,3 +7,4 @@
* Jordi Ballester Alomar <[email protected]>
* Miquel Raïch <[email protected]>
* Cristina Martin R.
* Eduardo de Miguel `Moduon <https://www.moduon.team/>`_
Copy link
Member

Choose a reason for hiding this comment

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

I think the official template suggest to put the company within parenthesis:

Suggested change
* Eduardo de Miguel `Moduon <https://www.moduon.team/>`_
* Eduardo de Miguel (`Moduon <https://www.moduon.team/>`__)

if (
self.env["ir.config_parameter"]
.sudo()
.get_param("partner_industry_secondary.display_last_child_first")
Copy link
Member

Choose a reason for hiding this comment

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

thought: don't you believe the "children first" format would be a better default value?

cc @rafaelbn

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

Successfully merging this pull request may close these issues.

[16.0][IMP] l10n_eu_nace: make industry readable
2 participants