-
-
Notifications
You must be signed in to change notification settings - Fork 171
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
[16.0][IMP] partner_event: use order in search by email #428
Conversation
LGTM 👍 Could you please run |
0e03021
to
bf283b4
Compare
Could you review this, please? |
The lint was fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this 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?
But that default order may select a better 1 record than by ID, isn't it? |
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. |
It looks good to me since that Odoo is sorting search with fields without index and it is slow for big tables Using Also, using Checking the following discussion about: |
|
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.
bf283b4
to
ca91709
Compare
The commit and MR description were updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ocabot merge patch
On my way to merge this fine PR! |
Please forward-port it to upper versions |
Congratulations, your PR was merged at d61825c. Thanks a lot for contributing to OCA. ❤️ |
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:
The partner result of the search must match with the email search, so any result is useful for this case.