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][MIG] product_secondary_unit #2 #1325

Merged
merged 52 commits into from
Jun 7, 2023

Conversation

remihb
Copy link
Contributor

@remihb remihb commented Apr 18, 2023

Adding changes of #1246

In replacement of #1193

Changes

Question

  • What about the odoo_test_helper needed in manifest ? (not in base odoo requirements)

@remihb remihb changed the title 16.0 mig product secondary unit [16.0][MIG] product_secondary_unit #2 Apr 18, 2023
@remihb remihb force-pushed the 16.0-mig-product_secondary_unit branch from 6f73826 to 062a481 Compare April 18, 2023 15:31
@BT-aleonard
Copy link

BT-aleonard commented Apr 19, 2023

Hi @remihb ,
Why is #1193 replaced by this PR?
Thanks.

<field name="model">product.template</field>
<field name="inherit_id" ref="product.product_template_form_view" />
<field name="arch" type="xml">
<xpath expr="//page[@name='general_information']/group[2]" position="after">

Choose a reason for hiding this comment

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

The only thing I'd change here is to use a relative reference.
And set columns for the group and also for the field as well.
<xpath expr="//page[@name='general_information']/group[last()]" position="after" > <group string="Secondary Unit of Measure" col="4" groups="uom.group_uom" > <field name="secondary_uom_ids" colspan="4" nolabel="1">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok done

@remihb remihb force-pushed the 16.0-mig-product_secondary_unit branch from 062a481 to 559f120 Compare April 26, 2023 15:44
@remihb
Copy link
Contributor Author

remihb commented Apr 26, 2023

Hi @remihb , Why is #1193 replaced by this PR? Thanks.

As I mentioned, #1193 doesn't include de product variants part, and more important, without precompute=True on ProductSecondaryUnitMixin.secondary_uom_qty, other modules like sale_order_secondary_unit or stock_secondary_unit will fail at install

@rousseldenis
Copy link
Contributor

/ocabot migration product_secondary_unit

@rousseldenis
Copy link
Contributor

@otmanelagy Do you mind if we swtitch to this ?

@OCA-git-bot
Copy link
Contributor

The migration issue (#1157) has been updated to reference the current pull request.
however, a previous pull request was referenced : #1193.
Perhaps you should check that there is no duplicate work.
CC : @otmanelagy

Copy link
Contributor

@rousseldenis rousseldenis left a comment

Choose a reason for hiding this comment

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

Code review

@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). 🤖

Comment on lines 1 to 2
# Copyright 2021 Tecnativa - Sergio Teruel
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl).
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove the License?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An error, corrected

Comment on lines 2 to 3
<!-- Copyright 2018 Tecnativa - Sergio Teruel
License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). -->
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove the License?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

idem, corrected

requirements.txt Outdated
@@ -1,2 +1,3 @@
# generated from manifests external_dependencies
odoo_test_helper
Copy link
Member

Choose a reason for hiding this comment

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

see comment in the manifest file

Choose a reason for hiding this comment

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

The correct way to add this is in a file test-requirements.txt

@@ -12,6 +12,7 @@
"application": False,
"installable": True,
"depends": ["product"],
"external_dependencies": {"python": ["odoo_test_helper"]},
Copy link
Member

Choose a reason for hiding this comment

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

Its a bad idea to add odoo_test_helpder in the external_dependencies key since its only use for the unit test. A better way to do that is to use test-requirements.txt file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@BT-aleonard
Copy link

BT-aleonard commented May 22, 2023

If you set precompute=True on the field secondary_uom_qty of the mixin, the models that are not using precompute yet (for example purchase.order.line) will fail when this fields depends on them

@BT-aleonard
Copy link

Hi,
Can you please check this PR, please?
@sergio-teruel

@sergio-teruel
Copy link
Contributor

@BT-aleonard Have you seen this PR (#1336)? About the secondary_uom_qty default?

<odoo>
<record id="product_template_form_view" model="ir.ui.view">
<field name="name">Product template Secondary Unit</field>
<field name="model">product.template</field>
<field name="inherit_id" ref="product.product_template_form_view" />
<field name="groups_id" eval="[(4, ref('uom.group_uom'))]" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove the groups? Is the better way to not load the view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it will raise this error Inherited view cannot have 'Groups' define on the record. Use 'groups' attributes inside the view definition

@@ -37,7 +41,6 @@
<field name="name">Product Secondary Unit</field>
<field name="model">product.product</field>
<field name="inherit_id" ref="product.product_normal_form_view" />
<field name="groups_id" eval="[(4, ref('uom.group_uom'))]" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Idem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idem :)

@BT-aleonard
Copy link

BT-aleonard commented Jun 1, 2023

@sergio-teruel
I didn't see it before.
But if we implement that PR (default value for secondary_uom_qty), then we can get rid of the precompute=True.
I'll suggest the changes.

@remihb remihb force-pushed the 16.0-mig-product_secondary_unit branch from 5c085db to e49e1e5 Compare June 1, 2023 12:28
@remihb remihb force-pushed the 16.0-mig-product_secondary_unit branch from e49e1e5 to e18f69c Compare June 1, 2023 12:33
Copy link
Member

@FrancoMaxime FrancoMaxime left a comment

Choose a reason for hiding this comment

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

LGTM: code review

@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). 🤖

@john-herholz-dt
Copy link

@sergio-teruel , can you merge please? I am working on another migration depending on this module.
Thank you!

@sergio-teruel
Copy link
Contributor

ping @pedrobaeza

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-1325-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit cdbd8ab into OCA:16.0 Jun 7, 2023
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 137afe1. 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.