[16.0][ADD] mrp_subcontracting_auto_create_lot#1710
[16.0][ADD] mrp_subcontracting_auto_create_lot#1710MarinaAForgeFlow wants to merge 1 commit intoOCA:16.0from
Conversation
340add5 to
f5aa0ce
Compare
marcos-mendez
left a comment
There was a problem hiding this comment.
Automated Review -- Tests Failed
1. Root cause of the test failure
The test failure occurs because the subcontracting_record_component() method in mrp_production.py is calling self._set_auto_lot_producing() only if self._get_subcontract_move() returns a value. However, in some test cases, especially those where no subcontract move is created or the MO is not properly initialized for subcontracting, this condition fails and the lot is not set correctly, leading to a UserError or incorrect behavior.
2. Suggested fix
In mrp_subcontracting_auto_create_lot/models/mrp_production.py, the method subcontracting_record_component() should ensure that _set_auto_lot_producing() is called after confirming that a subcontract move exists, and also make sure that self.lot_producing_id is properly initialized.
Specific fix:
def subcontracting_record_component(self):
self.ensure_one()
subcontract_move = self._get_subcontract_move()
if subcontract_move:
self._set_auto_lot_producing()
return super().subcontracting_record_component()This ensures that _set_auto_lot_producing() is only called when a subcontract move exists, avoiding potential errors.
3. Additional code issues
-
Incorrect use of
self._get_subcontract_move()insubcontracting_record_component(): The method should be checked to ensure it's returning a valid move before calling_set_auto_lot_producing(). If_get_subcontract_move()returnsFalseorNone, the code should not attempt to set the lot. -
Missing error handling for
subcontracting_record_component(): The method should properly handle cases where the subcontract move is not found, especially whenis_subcontracted=False.
4. Test improvements
The existing tests cover basic scenarios, but should be improved to include:
- Test case for when
_get_subcontract_move()returnsFalse: This ensures the method doesn't crash when no subcontract move is present. - Test case for
is_subcontracted=False: This should be tested to make sure it raises aUserErroras expected, and not proceed to set the lot. - Test case with
product_tracking='none': Even ifauto_create_lotis enabled, no lot should be created if the product is not tracked. - Test case with
product_auto_create_lotcomputed field: Ensure that the computed field correctly reflects the product'sauto_create_lotandcateg_id.auto_create_lotvalues.
Use TransactionCase for all tests, and consider using SavepointCase if the tests involve complex data changes or need to be isolated from each other. Tag tests appropriately using @tag('post_install', 'mrp_subcontracting_auto_create_lot') for better test organization.
Example test case:
def test_no_lot_when_no_subcontract_move(self):
"""No lot is created if there is no subcontract move."""
mo = self._create_mo(self.tracked_auto_product, is_subcontracted=False)
with self.assertRaises(UserError):
mo.subcontracting_record_component()This ensures robustness of the logic in edge cases.
⏰ This PR has been open for 38 days.
💤 Last activity was 37 days ago.
Every ignored PR is a contributor who might not come back. Review time matters. (OCA Aging Report)
Reciprocal Review Request
Hi everyone! I found some test failures on this PR and left detailed feedback above. I am happy to discuss or help debug. In the meantime, if any of you get a chance, I would appreciate a look at my open PR(s):
My open PRs across OCA:
- server-tools#3554 [MIG] datetime_formatter: Migration to 18.0
- server-tools#3548 [18.0][MIG] base_kanban_stage: Migration to 18.0
- hr-attendance#262 [16.0][ADD] Hr_attendance_idsecure: iDSecure (ControliD) attendance integration
- stock-logistics-workflow#2276 [16.0][ADD] stock_move_line_devaluation
- stock-logistics-workflow#2275 [16.0][ADD] Stock move line analytic account
- stock-logistics-workflow#2268 [16.0][ADD] stock_move_line_picking_partner
- purchase-workflow#2694 [16.0][IMP]Purchase workflow added to review state & exception fix
Reviewing each other's work helps the whole community move forward. Thank you!
Environment via OCA Neural Reviewer: Minikube + K8s Job + oca-ci/py3.10-odoo16.0 | Odoo 16.0
Automated review by OCA Neural Reviewer + qwen3-coder:30b
f5aa0ce to
75fa693
Compare
|
If I am not wrong, the suggested change behaves exactly as my original code. Also tests are failing because this PR depends on another that is not merged yet. So, the method _set_auto_lot_producing does not exist yet and that is why it fails. Also the suggested changes on the tests do not make sense either due to the same. |
Depends on #1709