-
-
Notifications
You must be signed in to change notification settings - Fork 730
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
[15.0][MIG] product_route_profile #1775
base: 15.0
Are you sure you want to change the base?
Conversation
51944b5
to
8c505e3
Compare
02a7d37
to
ba6b877
Compare
ab4e002
to
95773a9
Compare
Generated the icon with command:
|
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: stock-logistics-warehouse-14.0/stock-logistics-warehouse-14.0-product_route_profile Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-14-0/stock-logistics-warehouse-14-0-product_route_profile/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: stock-logistics-warehouse-14.0/stock-logistics-warehouse-14.0-product_route_profile Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-14-0/stock-logistics-warehouse-14-0-product_route_profile/
generated with oca icon generator.
5e90c9f
to
c0cbd2e
Compare
@pedrobaeza pre-commit was broken here.. |
/ocabot migration product_route_profile |
@francesco-ooops Can you please review? |
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.
Hi @bosd sorry but I don't understand well what this module does. I can only see a "Route Profile" menu where I can create a record and assign a name and nothing else
After you created a route profile, It can be assigned to a product. e.g. with one click, the correct values are set according to the defined route_profile. |
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.
Thanks! Using this as the basis of the migration to 17.0 at #2097
@@ -28,7 +28,7 @@ class ProductTemplate(models.Model): | |||
@api.depends("route_profile_id", "force_route_profile_id") | |||
@api.depends_context("company") | |||
def _compute_route_ids(self): | |||
for rec in self: | |||
for rec in self.sudo(): |
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.
@bosd Why is this 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.
I think it was needed to pass the tests.
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.
@bosd I still need an answer on this as just putting this for tests is not wanted.
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.
Good question. I don't remember, it has been a while :)
Maybe better to do functional test it without this. To see if it only is relevant for the tests. Or also required to achieve the functionality.
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.
Given that different routes may apply depending on the current company, it might be the case that there are route profiles (with an empty company_id) that provide routes for different companies. In that case it makes sense to escalate privileges when copying routes so that routes are copied even if the user does not have access to the all the companies that the routes belong to.
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 💯
Can we get a merge 🙏 |
@francesco-ooops @rousseldenis Is this PR ok for you now? |
@bosd great, this should go in the docs then :) |
Standard migration.