-
-
Notifications
You must be signed in to change notification settings - Fork 668
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
[12.0] [MIG] product_end_of_life #479
Conversation
Hey @tbaden, thank you for your Pull Request. It looks like some users haven't signed our Contributor License Agreement, yet.
Appreciation of efforts, |
ca53242
to
66db484
Compare
will push the history again. seems like I messed up my git user for the patch. |
a9e9cfb
to
571a537
Compare
found a missing newline... will fix it. otherwise it can be reviewed @max3903 |
571a537
to
15158d6
Compare
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.
Code review and functional test in runbot (haven't tested mailing feature). LGTM 👍
<field name="inherit_id" ref="product.product_variant_easy_edit_view"/> | ||
<field name="arch" type="xml"> | ||
<xpath expr="//group[@name='weight']" position="after"> | ||
<group></group> |
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.
@tbaden The empty group is needed?
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.
@nikul-serpentcs as I did not change this part I'm not 100% sure, but most likely for a correct placement inside the view.
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.
@tbaden Ok, Can you please check if need then keep as it is, otherwise remove it.
Thanks
@tbaden anything else to do? Ping reviewers if you think you are done. |
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.
LGTM!
This PR has the |
/ocabot merge nobump |
What a great day to merge this nice PR. Let's do it! |
/ocabot merge nobump |
On my way to merge this fine PR! |
@dreispt your merge command was aborted due to failed check(s), which you can inspect on this commit of 12.0-ocabot-merge-pr-479-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. |
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
/ocabot merge nobump |
What a great day to merge this nice PR. Let's do it! |
@simahawk your merge command was aborted due to failed check(s), which you can inspect on this commit of 12.0-ocabot-merge-pr-479-by-simahawk-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. |
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
@simahawk any idea why the merge failed? |
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.
test_search_read()
fails if the module product_model_viewer
is installed. i don’t know which module causes the error, though.
@api.model | ||
def name_search(self, name, args=None, operator='ilike', limit=100): | ||
args = args or [] | ||
domain = [] | ||
if name: | ||
domain = [ | ||
('name', operator, name) | ||
] | ||
if operator in expression.NEGATIVE_TERM_OPERATORS: | ||
domain = ['&', '!'] + domain[1:] | ||
products = self.search(domain + args, limit=limit) | ||
return products.name_get() |
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.
when i stumbled on this, i did not understand where this came from, and why it had to be added when migrating to 12.0. i then saw that in 11.0, product.product
redefined .name_get()
, and this called .search()
, but this has disappeared in 12.0.
this implementation is quite different from the default .name_get()
. isn’t there another way? if not, at least a comment explaining why it’s done like this and its tradeoffs would be nice.
/ocabot merge patch |
What a great day to merge this nice PR. Let's do it! |
Congratulations, your PR was merged at ba1de4a. Thanks a lot for contributing to OCA. ❤️ |
No description provided.