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

[16.0][ADD] partner_search_alias #1833

Open
wants to merge 2 commits into
base: 16.0
Choose a base branch
from

Conversation

AungKoKoLin1997
Copy link
Contributor

This module allows users to set non-official names for partners in the search_alias field, improving search usability by enabling searches using alternative names.

@qrtl QT4793

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-add-partner_search_alias branch from c055fed to f97d82d Compare August 22, 2024 06:29
Copy link
Member

@yostashiro yostashiro left a comment

Choose a reason for hiding this comment

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

@AungKoKoLin1997 Can you please also extend the partner search view to consider search_alias for the name field?

Please also make it clear in the readme file where in the form view users can find the added field.

<field name="inherit_id" ref="base.view_partner_tree" />
<field name="arch" type="xml">
<field name="translated_display_name" position="after">
<field name="search_alias" />
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<field name="search_alias" />
<field name="search_alias" optional="hide" />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

<field name="model">res.partner</field>
<field name="inherit_id" ref="base.view_partner_form" />
<field name="arch" type="xml">
<field name="vat" position="after">
Copy link
Member

Choose a reason for hiding this comment

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

Let's move the field somewhere less obtrusive.

Suggested change
<field name="vat" position="after">
<field name="ref" position="after">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the code.

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-add-partner_search_alias branch 2 times, most recently from fc36289 to 12ef300 Compare August 22, 2024 09:32
@AungKoKoLin1997
Copy link
Contributor Author

Hi community,
I am having trouble adding my new field search_alias to _name_search(). The challenging part is that I need to combine it with my new domain ('search_alias', operator, name) using an OR condition along with Odoo's standard _rec_names_search.
https://github.com/odoo/odoo/blob/16.0/odoo/models.py#L1633-L1640

With my current design, there might be issues if another module also extends _name_search() and calls super(), as my new domain will not be combined with the _rec_names_search fields using an OR condition. I believe overriding _rec_names_search is not a good solution, and I haven't seen any modules that override it. I welcome any suggestions.

Comment on lines 12 to 19
@api.model
def _name_search(
self, name="", args=None, operator="ilike", limit=100, name_get_uid=None
):
args = list(args or [])
if name:
args += ["|", ("search_alias", operator, name)]
return super()._name_search(name, args, operator, limit, name_get_uid)
Copy link
Member

Choose a reason for hiding this comment

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

@AungKoKoLin1997 Please try this.

Suggested change
@api.model
def _name_search(
self, name="", args=None, operator="ilike", limit=100, name_get_uid=None
):
args = list(args or [])
if name:
args += ["|", ("search_alias", operator, name)]
return super()._name_search(name, args, operator, limit, name_get_uid)
@property
def _rec_names_search(self):
return list(set(super()._rec_names_search + ["search_alias"]))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works. Thank you.

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-add-partner_search_alias branch 2 times, most recently from 36e4083 to 27995fc Compare August 26, 2024 04:03
Copy link
Member

@yostashiro yostashiro left a comment

Choose a reason for hiding this comment

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

Code review.

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-add-partner_search_alias branch from 27995fc to ecf3c6a Compare November 15, 2024 09:11
Copy link
Contributor

@kanda999 kanda999 left a comment

Choose a reason for hiding this comment

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

Code and functional review.

Comment on lines +27 to +29
filter_domain = filter_domain.replace(
"[", "['|', ('search_alias', 'ilike', self), "
)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we be using expression.OR() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To use expression.OR(), the XML string must be converted to a Python list using safe_eval. However, safe_eval fails if the string includes unsupported expressions like self. Seem like we have to replace self with some values and I think we can't do it. So, I prefer using current approach. How do you think?

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 we can leave the code as is on second thought. Thanks for checking.

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

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.

4 participants