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] mrp_production_service: Migration to 12.0 #358

Closed
wants to merge 11 commits into from

Conversation

skukered
Copy link
Contributor

@skukered skukered commented May 1, 2019

@pedrobaeza pedrobaeza added this to the 12.0 milestone May 1, 2019
@OCA-git-bot OCA-git-bot mentioned this pull request May 1, 2019
17 tasks
@sergiocorato
Copy link
Contributor

sergiocorato commented Sep 16, 2019

@skukered purchase-workflow repo is missing in oca_dependencies.txt for module subcontracted_service, this will solve start issue in runbot/travis
Thanks for the contribution!

boms, lines = production.bom_id.explode(
production.product_id, factor,
picking_type=production.bom_id.picking_type_id)
for line in lines:
Copy link
Member

Choose a reason for hiding this comment

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

You could just do a filtered() here in order to avoid looping through everything and remove the line below.

Copy link
Member

Choose a reason for hiding this comment

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

Also, it seems that the products of type service are removed when you create a new Manufacturing Order here https://github.com/OCA/OCB/blob/12.0/addons/mrp/models/mrp_production.py#L446

Copy link
Member

Choose a reason for hiding this comment

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

Could you attend this suggestion, I think it is a good improvement.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could just do a filtered() here in order to avoid looping through everything and remove the line below.

lines is [(record), {...}] so filtered() is not an option

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it seems that the products of type service are removed when you create a new Manufacturing Order here https://github.com/OCA/OCB/blob/12.0/addons/mrp/models/mrp_production.py#L446

This is true, service is ignored in mrp production. This port to a production order incomplete, as the service is ordered but missing in production order.

This logic is better implemented in mrp_subcontracting, so I think this PR should be closed.

@HviorForgeFlow
Copy link
Member

@skukered could you attend comments to move this forward?

Copy link
Member

@HviorForgeFlow HviorForgeFlow left a comment

Choose a reason for hiding this comment

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

LGTM ~ Minor comments.

@@ -0,0 +1,2 @@
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).
from . import test_mrp_production_service
Copy link
Member

Choose a reason for hiding this comment

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

Add blank line

boms, lines = production.bom_id.explode(
production.product_id, factor,
picking_type=production.bom_id.picking_type_id)
for line in lines:
Copy link
Member

Choose a reason for hiding this comment

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

Could you attend this suggestion, I think it is a good improvement.

@Tardo
Copy link
Member

Tardo commented Dec 16, 2019

Is it expected to give the last push?

@elvise
Copy link

elvise commented Jan 19, 2020

Hi, any good news for merge...?
If I can help please let me know...

@sergiocorato
Copy link
Contributor

@OCA/manufacturing-maintainers merge?

Copy link
Member

@marcelsavegnago marcelsavegnago left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment

Choose a reason for hiding this comment

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

@skukered Can you clean commit history and do a proper rebase so you don't include merge commits (286d5cc)?

@LoisRForgeFlow
Copy link
Contributor

@daramousk Is your suggestion covered? can you update your review?

@pedrobaeza
Copy link
Member

Isn't this similar to mrp_subcontracting?

Copy link

@ypapouin ypapouin left a comment

Choose a reason for hiding this comment

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

Functional testing is Ok. LGTM
Note that there is a dependency on subcontracted_service module in order to have a buy route.

@sergiocorato
Copy link
Contributor

Isn't this similar to mrp_subcontracting?

No, this one adds ability to subcontract a simple service (without need of stock moves).

Copy link
Contributor

@sergiocorato sergiocorato left a comment

Choose a reason for hiding this comment

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

This PR is superseeded by mrp_subcontracting

@MiquelRForgeFlow
Copy link
Contributor

@sergiocorato first you said no, now you say yes. why did your opinion change?

@sergiocorato
Copy link
Contributor

@sergiocorato first you said no, now you say yes. why did your opinion change?

Because as I wrote here the logic of this module is a duplication of an existing one (with the little difference that this one add ability to use services, but in a way they are not traced in production as noticed here), which is already implemented in future version, so in my opinion it is better to concentrate there.
My2cents

@MiquelRForgeFlow
Copy link
Contributor

which is already implemented in future version

In which version exactly?

@sergiocorato
Copy link
Contributor

mrp_subcontracting module from 12 and so on

@github-actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Dec 19, 2021
@github-actions github-actions bot closed this Jan 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs fixing stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet