search

OCA / sale-workflow

Odoo Sales, Workflow and Organization
GNU Affero General Public License v3.0
297 stars 1.03k forks source link

[15.0] [MIG]: sale_mrp_bom #3380

Open rrebollo opened 4 weeks ago

rrebollo commented 4 weeks ago

Reusing work from #2234 . Currently refactoring tests.

pedrobaeza commented 4 weeks ago

/ocabot migration sale_mrp_bom

rrebollo commented 3 weeks ago

@pedrobaeza sorry to bother U, the failing test is pointing to pre-commit stuff which aren't in the addon I'm upgrading so what can I do about it?

pedrobaeza commented 3 weeks ago

That's because you have updated the copier template in this PR, and thus, new linter requirements. You have to either not update the copier template, or update it, and fix the linter new complaints.

arielbarreiros96 commented 3 weeks ago

I've tested the module, and overall it performs well, as it does in other versions. However, I suggest enhancing the bom_id field by extending the domain to allow BOM selection only when the product is configured with the right routes. Specifically, the product should have either:

This improvement would help ensure that defining a BOM on the Sales Order leads to a Manufacturing Order upon confirmation (otherwise it only serves to have the reference), other, aligning with expected behavior. I noticed this feature isn’t present in v16 either, so this adjustment could done across versions.

What do you think?

rrebollo commented 3 weeks ago

That's because you have updated the copier template in this PR, and thus, new linter requirements. You have to either not update the copier template, or update it, and fix the linter new complaints.

I had to. The repo template has fixed node to v14 and it was raising eslint invalid syntax errors. Same for autoflake and flake8 hooks for pre-commit. The first one was raising "no module name distutils", the second one "AttributeError: 'EntryPoints' object has no attribute 'get'".

I'll fix the pre-commit complaints.

bosd commented 2 weeks ago

LGTM, Can you squash the translation commits?

rrebollo commented 2 weeks ago

LGTM, Can you squash the translation commits?

Better now?

rrebollo commented 2 weeks ago

@renatonlima @hailangvn sorry to bother U guys. Would U be so kind to maybe do a quick code review of this addon. I think U were involved with it.

rrebollo commented 1 week ago

@rousseldenis would U be so kind to review this?. I'm available to review anything U suggest me.

rousseldenis commented 1 week ago

@rrebollo Thanks for this. Could you do a PR with your changes on repo configuration and fixes commit ? I will merge it quickly but it will easy the review part. Thanks

rrebollo commented 1 week ago

@rousseldenis the commit updating repo template and pre-commit hooks went to #3411. Better now?

OCA-git-bot commented 1 week ago

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