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][IMP] partner_event: use order in search by email #428

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

fernandahf
Copy link

@fernandahf fernandahf commented Jan 29, 2025

The search by email is limited to 1 record and without the order clause is using the default order that is defined in https://github.com/odoo/odoo/blob/7f3863bdeb21ecfb7ac1859641bddf3de5dd8643/odoo/addons/base/models/res_partner.py#L178 and it could be customized.

Since the default order could cause a slow query this adds an "order" clause in the search to prevent the use of the default order and use the ID instead.

For example, in this case where without the order clause is using display_name in the order by default:

Screenshot 2025-01-29 at 12 42 48 p m

The partner result of the search must match with the email search, so any result is useful for this case.

@OCA-git-bot
Copy link
Contributor

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

@moylop260
Copy link

LGTM 👍

Could you please run pre-commit run locally to auto-fix the lint?

@fernandahf fernandahf force-pushed the 16.0-use-order-search-partner-fer branch from 0e03021 to bf283b4 Compare January 29, 2025 17:37
@fernandahf
Copy link
Author

@luisg123v @imanie383

Could you review this, please?

@fernandahf
Copy link
Author

@moylop260

The lint was fixed.

Copy link

@moylop260 moylop260 left a comment

Choose a reason for hiding this comment

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

👍

Copy link

@luisg123v luisg123v left a comment

Choose a reason for hiding this comment

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

I think commit description could be improved to express better what do you want to achieve and what do you mean by "the default order". If you order by ID, won't that add an ORDER BY clause anyway?

@pedrobaeza pedrobaeza added this to the 16.0 milestone Jan 29, 2025
@pedrobaeza
Copy link
Member

But that default order may select a better 1 record than by ID, isn't it?

@pedrobaeza pedrobaeza changed the title [IMP] partner_event: use order in search by email [16.0][IMP] partner_event: use order in search by email Jan 29, 2025
@fernandahf
Copy link
Author

fernandahf commented Jan 29, 2025

@luisg123v

The default order is based on this https://github.com/odoo/odoo/blob/7f3863bdeb21ecfb7ac1859641bddf3de5dd8643/odoo/addons/base/models/res_partner.py#L178

But if some of those fields are heavy, it's going to slow the search query, so, as the search is using a limited record set, it's better to have the order only by ID.

@moylop260
Copy link

It looks good to me since that Odoo is sorting search with fields without index and it is slow for big tables

Using order="id" it is ensure that a indexed field is used

Also, using order=None should work but Odoo auto-add the default value if it doesn't have value

Checking the following discussion about:

@luisg123v
Copy link

@fernandahf,

  1. As commented by @pedrobaeza, be aware the result could be different, as the used order is different too. Could that be a problem?
  2. I'm not against the change, but I think commit description should be inproved. If it weren't for your comments, it wouldn't fully get the point.

@pedrobaeza
Copy link
Member

Yes, it may impact in edge cases. Just document it and it's good to go, as I'm not opposing to it. It's just to highlight that the sentence "so the default order is unnecessary" is not true at all.

The search by email is limited to 1 record and without the order clause
is using the default order that is defined in
https://github.com/odoo/odoo/blob/7f3863bdeb21ecfb7ac1859641bddf3de5dd8643/odoo/addons/base/models/res_partner.py#L178
and it could be customized since the default order could cause a slow
query this adds an "order" clause in the search to prevent the use of
the default order and use the ID instead.

The partner got from the search has to match with the email searched,
so any result is useful for this case.
@fernandahf fernandahf force-pushed the 16.0-use-order-search-partner-fer branch from bf283b4 to ca91709 Compare January 29, 2025 18:39
@fernandahf
Copy link
Author

@pedrobaeza @luisg123v

The commit and MR description were updated.

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-428-by-pedrobaeza-bump-patch, awaiting test results.

@pedrobaeza
Copy link
Member

Please forward-port it to upper versions

@OCA-git-bot OCA-git-bot merged commit a58ad00 into OCA:16.0 Jan 29, 2025
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

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