OpenERP Community Backports (Addons)

Merge lp://staging/~wiz-keed/ocb-addons/ocb-7.0-fix-mrp-subproduct-unkink into lp://staging/ocb-addons

  1. ocb-7.0-fix-mrp-subproduct-unkink
  2. Merge into 7.0
Proposed by Paul Catinean
Status: Merged
Approved by: Holger Brunn (Therp)
Approved revision: no longer in the source branch.
Merged at revision: 10077
Proposed branch: lp://staging/~wiz-keed/ocb-addons/ocb-7.0-fix-mrp-subproduct-unkink
Merge into: lp://staging/ocb-addons
Diff against target: 12 lines (+1/-1)
1 file modified
mrp_byproduct/mrp_byproduct.py (+1/-1)
To merge this branch: bzr merge lp://staging/~wiz-keed/ocb-addons/ocb-7.0-fix-mrp-subproduct-unkink
Reviewer Review Type Date Requested Status
Holger Brunn (Therp) code review Approve
Pedro Manuel Baeza code review Approve
Alexandre Fayolle - camptocamp code review, no test Approve
Raphaël Valyi - http://www.akretion.com Approve
Review via email: mp+213086@code.staging.launchpad.net

Description of the change

Port to OCB of the following fix:

[FIX] Added ondelete='cascade' to mrp.subproduct when attatched bom is unlinked

https://code.launchpad.net/~wiz-keed/openobject-addons/7.0-fix-mrp-subproduct-unlink/+merge/213078

To post a comment you must log in.
Revision history for this message
Raphaël Valyi - http://www.akretion.com (rvalyi) wrote :

LGTM, thanks!

review: Approve
Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote :

This change is touchy, as it modifies the underlying database schema. Probably not OK for standard addons.

I'd be OK for having this in OCB, though.

Having an automated test showing the bug is fixed would be sooooooo nice.

review: Approve (code review, no test)
Revision history for this message
Pedro Manuel Baeza (pedro.baeza) wrote :

Do you think it's better to have cascade instead of restricted, because with this change, we will have records missed when we delete BoMs.

Regards.

review: Needs Information
Revision history for this message
Pedro Manuel Baeza (pedro.baeza) wrote :

Sorry, I didn't see context of the cascade. Now I see that it makes sense, so I approve it.

IMHO a test for this is excessive, where we only have to set an attribute that it's not going to be missed again, but...

Regards.

review: Approve (code review)
Revision history for this message
Holger Brunn (Therp) (hbrunn) :
review: Approve (code review)
Revision history for this message
Paul Catinean (wiz-keed) wrote :

Apologies, I have replied in the email client instead of writing comments here:

1st reply @rvalyi:

Glad to help out!

A local test showed no impact on a database that already had boms and mrp.subproducts created, automated tests would be more tedious instead of my isolated case though

2nd reply @pedro.baeza:

I kinda feel the same way to be honest since it's not a constraint that might be blocked by preexisting records.It will only affect delete statements after it has been placed on the table, and local tests show no problems

Up to you guys, cheers!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.