-
-
Notifications
You must be signed in to change notification settings - Fork 720
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
[ADD] product_top_category #1173
[ADD] product_top_category #1173
Conversation
|
||
# what if there is a change in parent of parent ? how to trigger the compute ? | ||
# note : the field is store as related in product.template | ||
@api.depends('parent_id') |
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.
You could dépend on parent_id.top_categ_id. Dont you think ?
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.
Yes, good idea thanks :)
@legalsylvain any idea why travis is failing ? The error is
|
Travis is totally obsolete. I guess that switching to github action could solve the problem, updating technical files in the root folder (in a dedicated PR). otherwise, you can ask to @carmenbianca I think. |
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.
Small comments otherwise LGTM 👍
def _get_top_category(self): | ||
self.ensure_one() | ||
if self.parent_id: | ||
return(self.parent_id._get_top_category()) |
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.
Why the parentheses ?
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.
ah, don't know, it just came naturally ^^ I remove them
@@ -0,0 +1,3 @@ | |||
For all categories, compute their top categories. |
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.
Maybe add that this field is useful for reports and pivot views ?
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.
Yes, done
@robinkeunen thanks for the review ! |
Hi. could you solve travis issue, updating files in another PR via copier template ? (then rebasing). thanks ! |
a02153c
to
0f7aa53
Compare
Done here, it still doesn't seem to work though. @carmenbianca do you have an idea why ? |
@victor-champonnois In the meantime, the template broke again 😅 OCA/oca-addons-repo-template#181 Need to bump template again |
0f7aa53
to
571d527
Compare
@carmenbianca updated the copier update branch and rebased on it ! |
@victor-champonnois Easier to do a copier update PR as this one could last again some time. The copier PR could be merged fast. Do you take care of that ? |
Ok, I see it. I'll do the merge |
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. |
Rebasing this. |
571d527
to
2148e0f
Compare
2148e0f
to
e8b44b6
Compare
@rousseldenis this is ready to be merged |
.pre-commit-config.yaml
Outdated
@@ -38,6 +38,7 @@ repos: | |||
rev: v3.4.1 | |||
hooks: | |||
- id: flake8 | |||
language_version: python3 |
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.
@victor-champonnois You should not modify manually.
Maybe could you propose this change to https://github.com/OCA/oca-addons-repo-template/blob/master/src/.pre-commit-config.yaml.jinja ?
Thanks
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 removed the commit, it wasn't needed actually.
e8b44b6
to
94032a0
Compare
94032a0
to
5d1f6c0
Compare
@rousseldenis It seams that this PR is now ok to merge. :) |
for categ in self: | ||
categ.top_categ_id = categ._get_top_category() | ||
|
||
top_categ_id = fields.Many2one('product.category', |
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.
@victor-champonnois Could you put fields on ... top :-) ? Thanks
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.
done, thanks for the review :)
04599f2
to
618f78d
Compare
618f78d
to
c7bd592
Compare
This PR has the |
/ocabot merge patch |
Hey, thanks for contributing! Proceeding to merge this for you. |
Congratulations, your PR was merged at 5c22fb3. Thanks a lot for contributing to OCA. ❤️ |
No description provided.