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

[FIX] product_uom_measure_type: Fixed the issue, that the module cannot be installed #1778 #1779

Merged

Conversation

baguenth
Copy link
Member

[FIX] product_uom_measure_type: Fixed the issue, that the module cannot be installed on migrated databases which already have a column 'measure_type'

This pr fixed #1778

@dreispt @legalsylvain @rousseldenis

Kindly ask to check this pr :)

@OCA-git-bot
Copy link
Contributor

Hi @legalsylvain,
some modules you are maintaining are being modified, check this out!

…ot be installed on migrated databases which already have a column 'measure_type'
@baguenth baguenth force-pushed the product-uom-measure-type-install-fix branch from ba7e21c to c9eec75 Compare November 22, 2024 09:15
Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

Hi. thanks for fixing that issue !

note : The implementation is correct, but I wonder if it's not better to test if the column exist, and if yes, return and doesn't execute code. if avoids all the UPDATE records that are, I guess useless if the column still exists.
What do you think ?

Kind regards.

@baguenth
Copy link
Member Author

baguenth commented Nov 22, 2024

Hi @legalsylvain,

thank you for the quick feedback!

I think we still need to run the update queries. The reason is, that after 13.0 at leaset two new uom categories got introduced: uom_categ_wtime and uom_categ_surface

All three update functions are actually build on top of each other

  1. Update measure_type in uom_category --> Here we at least need to set measure_type for uom_categ_wtime and uom_categ_surface
  2. Update measure_type in uom_uom, based on uom_category --> Due to 1), there could be a change here
  3. Update measure_type in product_template, based on uom_uom --> Due to 2), there could be a change here

Technically we could reduce the update to only handle the new uom categories, in case the column measure_type already exists. This would increase the complexity of the hook a lot, though. Since the update functions do not really hurt and are also uncritical regarding performance, I would rather keep it simple.

Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your explanation !

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-1779-by-legalsylvain-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit e1e1022 into OCA:16.0 Nov 22, 2024
9 checks passed
@OCA-git-bot
Copy link
Contributor

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

@baguenth baguenth deleted the product-uom-measure-type-install-fix branch November 22, 2024 11:08
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.

Cannot install product_uom_measure_type on migrated databases
3 participants