-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[17.0][MIG] base_name_search_improved: Migration to 17.0 #2831
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
[17.0][MIG] base_name_search_improved: Migration to 17.0 #2831
Conversation
|
Same comment as in #2833: any idea why |
|
Hi! I was wondering - is there still ongoing work on this? Does it work functionally? |
|
@thomaspaulb Hey! I really I don't know how to solve the problem of coverage because it is working fine in my computer |
|
cb2c6b5 to
bd3b119
Compare
|
@thomaspaulb Hey! thanks! Now it should be correct Regards |
bd3b119 to
dc1e352
Compare
dc1e352 to
2f4c3cc
Compare
2f4c3cc to
a91048f
Compare
|
/ocabot merge nobump |
|
What a great day to merge this nice PR. Let's do it! |
|
@StefanRijnhart The merge process could not be finalized, because command |
|
Huh, looks like the README compilation is struggling with three images in two snippets. Can you play around to see if you can get it to work? |
a91048f to
6e1e404
Compare
6e1e404 to
bbbaff8
Compare
|
/ocabot merge nobump |
|
On my way to merge this fine PR! |
441e0e2 to
f1b908b
Compare
|
@StefanRijnhart Hey please disregard my previous message. I've tried to implement what you requested, but it didn't work properly. I added the translation in the smart_search field to false. I hope this solves the merge problem. |
|
@StefanRijnhart HI! Just a friendly reminder 😊 |
|
@StefanRijnhart Hi! I would like to ask you if you can check if its now ok |
|
/ocabot merge nobump |
|
This PR looks fantastic, let's merge it! |
| # TODO perhaps better to create only the field when enabled on the model | ||
| smart_search = fields.Char( | ||
| compute="_compute_smart_search", search="_search_smart_search", translate=False | ||
| ) |
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.
@rov-adhoc how did you try to implement @StefanRijnhart's suggestion? I'd expect it to work to override _setup_fields, check via sql if the model has the functionality enabled, and if so, add the field to self._fields.
Just setting translate to False can't work because it's not about the translations of data from the field, but about things like its label.
|
@hbrunn your merge command was aborted due to failed check(s), which you can inspect on this commit of 17.0-ocabot-merge-pr-2831-by-hbrunn-bump-nobump. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
|
/ocabot migration base_name_search_improved |
|
What a great day to merge this nice PR. Let's do it! |
|
@dreispt your merge command was aborted due to failed check(s), which you can inspect on this commit of 17.0-ocabot-merge-pr-2831-by-dreispt-bump-nobump. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
| def patch_name_search(): | ||
| @api.model | ||
| def _name_search( | ||
| self, name="", domain=None, operator="ilike", limit=100, order=None |
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.
The signature is incorrect, especially limit=100 gives incorrect results with domains like [("parent_id", "ilike", ...)])
|
Just checking in if something is holding this back from being merged? |
|
/ocabot merge nobump |
|
What a great day to merge this nice PR. Let's do it! |
|
Congratulations, your PR was merged at 465fae1. Thanks a lot for contributing to OCA. ❤️ |
No description provided.