Skip to content

Conversation

@antonioburic
Copy link
Member

@antonioburic antonioburic commented Aug 6, 2024

superseeds #3008

includes this fix from v14 too #2947

edit: tests fail seemingly for an unrelated reason

@OCA-git-bot
Copy link
Contributor

Hi @smaciaosi, @dreispt, @ckolobow,
some modules you are maintaining are being modified, check this out!

@StefanRijnhart
Copy link
Member

/ocabot migration sale_mrp_bom

@antonioburic sale_mrp_bom looking good with all the tests running and no warnings from this module, but there are linting changes to the RST files of some other modules. Maybe it's best to remove them from this PR, and if you feel like going all the way, propose them separately for bonus points. What do you think?

@OCA-git-bot OCA-git-bot added this to the 17.0 milestone Aug 6, 2024
@antonioburic antonioburic force-pushed the 17.0-mig-sale_mrp_bom branch from 2e82374 to 5a5e2d0 Compare August 6, 2024 20:00
@antonioburic
Copy link
Member Author

/ocabot migration sale_mrp_bom

@antonioburic sale_mrp_bom looking good with all the tests running and no warnings from this module, but there are linting changes to the RST files of some other modules. Maybe it's best to remove them from this PR, and if you feel like going all the way, propose them separately for bonus points. What do you think?

@StefanRijnhart nice catch, oca-port automation and I missed to notice :)
corrected, will create a separate PR for those changes

Copy link
Member

@StefanRijnhart StefanRijnhart left a comment

Choose a reason for hiding this comment

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

Thanks!

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

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

continue
raise ValidationError(
_(
"Please select BoM that has matched product with the line `{}`"
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to use translate function args:

Suggested change
"Please select BoM that has matched product with the line `{}`"
"Please select BoM that has matched product with the line `%(product_name)s`", product_name=line_product.name)



class TestSaleMrpLink(TransactionCase):
def setUp(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you switch to class method instead ?

# Check manufacture order
mos = self.env["mrp.production"].search([("origin", "=", so.name)])
for mo in mos:
self.assertEqual(mo.bom_id, self.boms.get(mo.product_id.id))
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this line is not reached by tests.

Could you check why ?

Copy link

@CasVissers-360ERP CasVissers-360ERP left a comment

Choose a reason for hiding this comment

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

Functional review (after constraint is fixed)

OCA-git-bot and others added 6 commits September 12, 2024 09:20
This commit fixes a bug where the module would not accept a BoM on a
Sale Order Line if the product in question had multiple variants and
the BoM did not specify a variant.

Now, regardless of whether the product has multiple variants or not,
if the variant is specified on the BoM, it must match the
product on the Sale Order Line (like before), but if it's
not specified, matching on the product template is enough.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.