-
-
Notifications
You must be signed in to change notification settings - Fork 486
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
[14.0][ADD] mrp_subcontracting_purchase #1082
[14.0][ADD] mrp_subcontracting_purchase #1082
Conversation
@pedrobaeza could you please provide us instructions on how to correctly bring to merge this PR? eg. regarding authorship and commits, as this is the backporting to v14 of odoo module https://github.com/odoo/odoo/tree/15.0/addons/mrp_subcontracting_purchase Thanks! |
First, you should add a commit attributed to "Odoo SA odoo@odoo.com" with the contents as they are in the new version, and then add your commits with the changes needed to work on this version. |
cc876fc
to
f0fd491
Compare
@pedrobaeza To make mrp_subcontracting_purchase work as per v15, we also had to port code from v15 of Odoo modules: mrp_subcontracting_purchase, how do you suggest to proceed for code from those modules? |
You have to include the needed changes in your module, trying to be the most compatible possible. I needed to do similar things when I backported f32ce8b#diff-96c2f57c6a07eb25b5e8b5cbc49574423aa16aa8fbb1c50672de67d9a25aca56R150 |
bb17a10
to
e91ef71
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check my comments and upper your tests coverage.
def _prepare_purchase_order(self, company_id, origins, values): | ||
"""Returns prepared data for create PO""" | ||
if ( | ||
"partner_id" not in values[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value
may be empty? If value
will be empty than you need check this before this condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value is checking here
if po.origin: | ||
missing_origins = origins - set(po.origin.split(", ")) | ||
if missing_origins: | ||
po.write({"origin": po.origin + ", " + ", ".join(missing_origins)}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use format
function for preparing string value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
def _pre_button_mark_done(self): | ||
if self._get_subcontract_move(): | ||
return True | ||
return super()._pre_button_mark_done() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do like this (optional):
return True if self._get_subcontract_move() else super()._pre_button_mark_done()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applied, thanks
# which checks whether the subcontracting has been recorded or not | ||
|
||
def filter_in(mo): | ||
if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can inverse this big condition and write return
without if:
return not (mo.state in ("done", "cancel") .......)`
Please create a separate order for these.""" | ||
) | ||
+ "\n\n" | ||
+ "\n".join(not_in_bom_products.mapped("name")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not good idea, you must use format like this for one variable _("....... \n\n%s", value)
.
def _compute_show_subcontracting_details_visible(self): | ||
"""Compute if the action button in order to see moves raw is visible""" | ||
self.show_subcontracting_details_visible = False | ||
for move in self: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think need add the filter for self
for move in self.filtered("is_subcontract"):
....
show details button is visible. | ||
""" | ||
res = super(StockMove, self)._compute_show_details_visible() | ||
for move in self: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too
5c50253
to
8dc1fe4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional ok!
We should be good here, full test coverage and documentation
@pedrobaeza can we ask for your feedback? Thanks!
@david-fleischmann would this help your flow? |
I'm not seeing an initial commit authored by Odoo SA with the content as is on the next version, and then, all your modifications to make it work in v14. |
Add a new module to add smart buttons between a subcontracting purchase order and the ressuply component delivery. task-2486811 Part-of: odoo/odoo#75041
8dc1fe4
to
51c7c06
Compare
51c7c06
to
0765765
Compare
@pedrobaeza Could you check repository structure please? |
@@ -0,0 +1,2 @@ | |||
This module is a backport from Odoo SA and as such, it is not included in the OCA CLA. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module is a backport from Odoo SA and as such, it is not included in the OCA CLA. | |
This bridge module adds some smart buttons between Purchase and Subcontracting | |
**DISCLAIMER:** This module is a backport from Odoo SA and as such, it is not included in the OCA CLA. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see that you have a DESCRIPTION.rst in the readme
folder, so you have to introduce the disclaimer there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see that you have a DESCRIPTION.rst in the
readme
folder, so you have to introduce the disclaimer there.
Resolved
mrp_subcontracting_purchase/demo/0001_demo_product_category.xml
Outdated
Show resolved
Hide resolved
a1c6816
to
93cd12b
Compare
@@ -0,0 +1,5 @@ | |||
This bridge module adds some smart buttons between Purchase and Subcontracting | |||
|
|||
**DISCLAIMER:** This module is a backport from Odoo SA and as such, it is not included in the OCA CLA. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be put on readme/DESCRIPTION.rst
, not here.
/ocabot merge nobump |
This PR looks fantastic, let's merge it! |
Congratulations, your PR was merged at c2c7aaa. Thanks a lot for contributing to OCA. ❤️ |
Hi @pedrobaeza , thanks for your support on this one. Do you have any idea why the "Configuration" file is not appearing in readme here?: https://github.com/OCA/manufacture/tree/14.0/mrp_subcontracting_purchase Do you think it's a .md issue? |
It should be |
@francesco-ooops thank you! Just found that notifications from github in my spam folder... sorry for that late reply! |
No description provided.