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

[MIG] product_operating_unit: Migration to 17.0 #668

Merged
merged 34 commits into from
Jul 19, 2024

Conversation

jdidderen-nsi
Copy link
Contributor

@jdidderen-nsi jdidderen-nsi commented May 28, 2024

Some changes (security rules) are based on changes made in the base modules #667

I removed domains on operating units fields for two reasons:

  • A operating unit manager is not necessarily assigned to a operating unit (
    if user._origin.has_group("operating_unit.group_manager_operating_unit"):
    ) so in this case, the user id is not present in the user_ids field of the operating unit
  • The security rules already filter the units I can see.

Depends on:

@AaronHForgeFlow
Copy link
Contributor

/ocabot migration product_operating_unit

@OCA-git-bot OCA-git-bot added this to the 17.0 milestone May 28, 2024
@OCA-git-bot OCA-git-bot mentioned this pull request May 28, 2024
16 tasks
@jdidderen-nsi jdidderen-nsi force-pushed the 17.0-mig-product_operating_unit branch from cb48d89 to 493f37d Compare June 3, 2024 08:36
Copy link

@SilvioC2C SilvioC2C left a comment

Choose a reason for hiding this comment

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

Small improvements suggested.
CI is red, though.

Comment on lines 25 to 29
category_ou_ids = rec.operating_unit_ids.ids
for product in products:
ou_ids = product.operating_unit_ids.ids
ou_ids.extend(category_ou_ids)
product.operating_unit_ids = [Command.set(ou_ids)]

Choose a reason for hiding this comment

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

Nitpicking: might as well use records directly, instead of ids

Choose a reason for hiding this comment

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

at least it's worth it to try if it fails on this

Copy link
Contributor Author

@jdidderen-nsi jdidderen-nsi Jun 11, 2024

Choose a reason for hiding this comment

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

Logic updated with records instead of ids. Thanks for the review 👍

Comment on lines 28 to 29
if self.categ_id and self.categ_id.operating_unit_ids:
return [(6, 0, self.categ_id.operating_unit_ids.ids)]

Choose a reason for hiding this comment

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

Since this method is used as a default method, self.categ_id will always be empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, I simply removed the if statement but I'm wondering if the default is still needed with the compute based on the categ_id

@api.depends("categ_id")
def _compute_operating_unit_ids(self):
for record in self:
if record.categ_id.operating_unit_ids:

Choose a reason for hiding this comment

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

I think you can remove this if statement: if the category is changed to a category with no OU, the field can be safely emptied

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Statement removed 👍

@yankinmax
Copy link

yankinmax commented Jun 8, 2024

you need to add test-requirements.txt file in operating-unit folder with such content

odoo-addon-operating_unit @ git+https://github.com/OCA/operating-unit.git@refs/pull/667/head#subdirectory=operating_unit

this should fix the build

@jdidderen-nsi jdidderen-nsi force-pushed the 17.0-mig-product_operating_unit branch from 1153f91 to 88faccc Compare June 11, 2024 09:01
@yankinmax
Copy link

@jdidderen-nsi thanks for the migration

Copy link

@Camille0907 Camille0907 left a comment

Choose a reason for hiding this comment

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

Migration LGTM, thanks

compute="_compute_operating_unit_ids",
store=True,
readonly=False,
default=lambda self: self._default_operating_unit_ids(),

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was one of my thoughts, given the compute based on the categ_id
So piece of code is removed

@jdidderen-nsi jdidderen-nsi force-pushed the 17.0-mig-product_operating_unit branch from 88faccc to aec48e1 Compare June 24, 2024 09:21
@gurneyalex
Copy link
Member

#667 is now merged 😸

@yankinmax
Copy link

yankinmax commented Jul 8, 2024

@jdidderen-nsi can you please remove dependency commit and rebase?
operating_unit is merged:

jesus01x and others added 18 commits July 8, 2024 21:51
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: operating-unit-16.0/operating-unit-16.0-product_operating_unit
Translate-URL: https://translation.odoo-community.org/projects/operating-unit-16-0/operating-unit-16-0-product_operating_unit/
Currently translated at 100.0% (7 of 7 strings)

Translation: operating-unit-16.0/operating-unit-16.0-product_operating_unit
Translate-URL: https://translation.odoo-community.org/projects/operating-unit-16-0/operating-unit-16-0-product_operating_unit/es/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: operating-unit-16.0/operating-unit-16.0-product_operating_unit
Translate-URL: https://translation.odoo-community.org/projects/operating-unit-16-0/operating-unit-16-0-product_operating_unit/
Currently translated at 14.2% (1 of 7 strings)

Translation: operating-unit-16.0/operating-unit-16.0-product_operating_unit
Translate-URL: https://translation.odoo-community.org/projects/operating-unit-16-0/operating-unit-16-0-product_operating_unit/ja/
This PR fixes an issue with the method operating_unit_default_get
where it would return an operating unit even for a company that was
not active, which would cause all sorts of issues

Steps to reproduce on runboat,
taking as example purchase_operating_unit:

- User has a default ou belonging to Company 1
- Switch to Company 2 (make sure Company 1 is not active at all)
- Try to create a Purchase Order

The issue arises because the operating_unit field is pre-compiled
on the PO with an Operating Unit whose company is inactive,
and the onchanges cannot find or access related data.
Currently translated at 100.0% (7 of 7 strings)

Translation: operating-unit-16.0/operating-unit-16.0-product_operating_unit
Translate-URL: https://translation.odoo-community.org/projects/operating-unit-16-0/operating-unit-16-0-product_operating_unit/it/
@jdidderen-nsi jdidderen-nsi force-pushed the 17.0-mig-product_operating_unit branch from aec48e1 to f589b76 Compare July 8, 2024 19:52
@jdidderen-nsi
Copy link
Contributor Author

@gurneyalex @yankinmax Rebased and good to go 👍

@gurneyalex
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 17.0-ocabot-merge-pr-668-by-gurneyalex-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 0f07bf5 into OCA:17.0 Jul 19, 2024
5 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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