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

[16.0][OU-ADD] sale_purchase #4061

Closed
wants to merge 1 commit into from

Conversation

huguesdk
Copy link
Member

@huguesdk huguesdk commented Jul 6, 2023

No description provided.

@legalsylvain
Copy link
Contributor

legalsylvain commented Jul 6, 2023

/ocabot migration sale_purchase

Depends on :

Copy link
Contributor

@remytms remytms left a comment

Choose a reason for hiding this comment

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

LGTM.

---Models in module 'sale_purchase'---
---Fields in module 'sale_purchase'---
sale_purchase / product.template / service_to_purchase (boolean) : not stored anymore
# DONE: post-migration, convert to company-dependent field
Copy link
Member

Choose a reason for hiding this comment

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

For better reading (as this file doesn't have syntax highlighting, on contrary than on Python, where I recommend to not put empty lines inside methods due to this), please put an empty line in between each block:

Suggested change
# DONE: post-migration, convert to company-dependent field
# DONE: post-migration, convert to company-dependent field

# DONE: post-migration, convert to company-dependent field
---XML records in module 'sale_purchase'---
DEL ir.model.constraint: sale_purchase.constraint_product_template_service_to_purchase
# DONE: post-migration, delete XML-ID
Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines +8 to +17
# the product.template.service_to_purchase field became company-dependent.
# this means that it is no longer stored in the product_template table,
# but in the ir_property table.
#
# for such cases, openupgrade.convert_to_company_dependent() should
# normally be used, but that function does not seem to support converting
# a field to company-dependent without changing its name at the same time.
# moreover, it stores boolean values even when they are false (what odoo
# does not), and it creates values for all companies, which does not make
# sense when a record is linked to a particular company only.
Copy link
Member

Choose a reason for hiding this comment

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

This is something to be put as docstring of the method IMO, as it describes what the method is going to do.

Suggested change
# the product.template.service_to_purchase field became company-dependent.
# this means that it is no longer stored in the product_template table,
# but in the ir_property table.
#
# for such cases, openupgrade.convert_to_company_dependent() should
# normally be used, but that function does not seem to support converting
# a field to company-dependent without changing its name at the same time.
# moreover, it stores boolean values even when they are false (what odoo
# does not), and it creates values for all companies, which does not make
# sense when a record is linked to a particular company only.
"""The product.template.service_to_purchase field became company-dependent.
this means that it is no longer stored in the product_template table,
but in the ir_property table.
For such cases, openupgrade.convert_to_company_dependent() should
normally be used, but that function does not seem to support converting
a field to company-dependent without changing its name at the same time.
moreover, it stores boolean values even when they are false (what odoo
does not), and it creates values for all companies, which does not make
sense when a record is linked to a particular company only.
"""

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, your considerations are not totally correct:

but that function does not seem to support converting a field to company-dependent without changing its name at the same time

I don't fully understand what you mean, but the usual method is to rename the origin column in pre-migration (for preservation purposes, but also for not confusing people browsing DB schema thinking that the applied value is the one in the column), and then call this other method in post.

it stores boolean values even when they are false

This can be considered a bug of the method (or something that has changed across versions, and should be split the behavior inside the method), but it should be fixed there in openupgradelib.

It creates values for all companies, which does not make sense when a record is linked to a particular company only.

This is a temporary condition. If you empty the company field after migration, you expect to have the same value for the rest of the companies as it was previous the migration, so we can't limit it.

Maybe for not over-heading the table, a ir.property without company_id can be created for this, but we should not how Odoo acts when you change the value in one company in this case. Is it modified for all the companies as the existing ir.property has no company?

So in summary, we shouldn't add so many code for something that is a recurrent problem and should be handled in the library.

@pedrobaeza
Copy link
Member

Superseded by #4366

@pedrobaeza pedrobaeza closed this Mar 31, 2024
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.

5 participants