Skip to content
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_profile #546

Merged
merged 47 commits into from
Jul 17, 2020
Merged

Conversation

kevinkhao
Copy link
Contributor

No description provided.

@legalsylvain
Copy link
Contributor

CC : original authors : @EBII, @bealdav, @sebastienbeau

Copy link
Member

@nikul-serpentcs nikul-serpentcs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpicking

"summary": "Allow to configure a product in 1 click",
"category": "product",
"depends": ["sale"],
"website": "http://www.akretion.com/",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Nikul !

<field name="inherit_id" ref="product.product_template_form_view"/>
<field name="arch" type="xml">
<xpath expr="//h1" position="after">
<group>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<group>
<group name='profile'>

Copy link
Contributor

@EBII EBII left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anything else seems Ok

"author": "Akretion, Odoo Community Association (OCA)",
"summary": "Allow to configure a product in 1 click",
"category": "product",
"depends": ["sale"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in 12.0 i think you need to depends sale_managment instead sale to get product menu in sale app and not just invoice app..



class ProductProfile(models.Model):
_name = "product.profile"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add _descritpion =" xx" to remove warnings



class ProductMixinProfile(models.AbstractModel):
_name = "product.mixin.profile"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add _description to remove warnings

{"profile_id": self.profile_id.id}
)
for field, value in values.items():
try:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think is necessary to avoid id that generate error message on change profile on product

Suggested change
try:
if field != 'id':
try:
self[field] = value

Copy link
Contributor Author

@kevinkhao kevinkhao Jan 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi EBII, thank you for your feedback!

I think that field should be excluded already because of exclusion of fields , did you manage to trigger an error message ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes @kevinkhao it generate the error message in the except

get_profile_fields_to_exclude() is only call on write method.. You can call it On the on change instead of just filter the id.
but not sure it is necessary or it's work

Suggested change
try:
excludable_fields = get_profile_fields_to_exclude()
for field, value in values.items():
if field not in excludable_fields:
try:

Copy link
Contributor

@EBII EBII left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for your work.

@EBII
Copy link
Contributor

EBII commented Jan 9, 2020

@bealdav

)
explanation = fields.Text(
required=True,
oldname="description",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove oldname

@@ -0,0 +1,2 @@
"id","name","model_id:id","group_id:id","perm_read","perm_write","perm_create","perm_unlink"
"access_product_brand_product_manager","product.profile","model_product_profile","base.group_user",1,0,0,0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

product_brand_product_manager ?

missing a security rule to update/create profiles

def create(self, vals):
if vals.get("profile_id"):
vals.update(self._get_vals_from_profile(vals))
return super(ProductMixinProfile, self).create(vals)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in python 3 you may have super().create() each time super has self arg

The same things in other parts of the script

@kevinkhao kevinkhao force-pushed the 12.0-mig-product_profile branch 4 times, most recently from b08c1bc to fe4b989 Compare January 14, 2020 16:41
@kevinkhao kevinkhao force-pushed the 12.0-mig-product_profile branch 2 times, most recently from 7194494 to a3782a3 Compare February 3, 2020 11:37
@sebastienbeau
Copy link
Member

In order to improve the readability and make easier to track teh change it will be great if you can do the migration in two commit.
One for the structural change (splitting the file product.py in two file and running black)
One for the real logic change (improving readme, migration of the code....)

Thanks for your work
Note travis is read

@kevinkhao
Copy link
Contributor Author

In order to improve the readability and make easier to track teh change it will be great if you can do the migration in two commit.
One for the structural change (splitting the file product.py in two file and running black)
One for the real logic change (improving readme, migration of the code....)

Thanks for your work
Note travis is read

Noted, will do, thanks

@kevinkhao kevinkhao force-pushed the 12.0-mig-product_profile branch 4 times, most recently from 9d6ae37 to 2e67995 Compare February 13, 2020 04:20
@kevinkhao kevinkhao force-pushed the 12.0-mig-product_profile branch 4 times, most recently from 20f8384 to 60ca1d3 Compare February 23, 2020 21:46
Copy link
Member

@bealdav bealdav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except typo ok for me
Review and test UI

product_profile/models/product.py Outdated Show resolved Hide resolved
product_profile/models/product.py Outdated Show resolved Hide resolved
product_profile/models/product_profile.py Outdated Show resolved Hide resolved
product_profile/models/product_profile.py Outdated Show resolved Hide resolved
product_profile/models/product_profile.py Outdated Show resolved Hide resolved
product_profile/models/product_profile.py Outdated Show resolved Hide resolved
@rousseldenis rousseldenis added this to the 12.0 milestone Jun 22, 2020
@rousseldenis
Copy link
Sponsor Contributor

@EBII @nikul-serpentcs Could you approve ?

Copy link
Member

@nikul-serpentcs nikul-serpentcs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rousseldenis Now Code LGTM 👍

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

1 similar comment
@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@bealdav
Copy link
Member

bealdav commented Jul 17, 2020

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

Sorry @bealdav you are not allowed to merge.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

@bealdav
Copy link
Member

bealdav commented Jul 17, 2020

Ok not yet maintainer

@rousseldenis
Copy link
Sponsor Contributor

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 12.0-ocabot-merge-pr-546-by-rousseldenis-bump-major, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 39ab783 into OCA:12.0 Jul 17, 2020
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at cb7f48b. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet