fix(spp_cr_type_assign_program): scope duplicate-assignment conflict to (registrant, program)#208
Conversation
…to (registrant, program) The rule uses scope='custom', so the base conflict domain does not pre-filter by registrant. The hook only narrowed candidates by program_id, which incorrectly blocked submitting a CR whenever any other in-flight CR targeted the same program, even for a different registrant. Filter candidates by both program_id and registrant_id to match the rule's intent (and the override's docstring). Add a regression test covering the different-registrants/same-program case.
There was a problem hiding this comment.
Code Review
This pull request refines the conflict detection logic for assigning registrants to programs, ensuring that conflicts are only flagged when the same registrant is assigned to the same program. The changes include an update to the _check_custom_conflicts method to filter by registrant ID, improved documentation, and a new test case to verify the logic. Feedback was provided to optimize the filtering logic for better performance through short-circuiting and to include a defensive check for missing detail records.
| return candidates.filtered( | ||
| lambda c: c.detail_res_id in matching_detail_ids and c.registrant_id == self.registrant_id | ||
| ) |
There was a problem hiding this comment.
Consider swapping the conditions in the filtered lambda to leverage short-circuiting. Since candidates may contain many records for different registrants, checking the registrant_id first is more efficient as it avoids the set lookup for non-matching registrants. Additionally, per repository guidelines, defensively filter out any records where the detail record (detail_res_id) is missing to handle edge cases like newly created records.
| return candidates.filtered( | |
| lambda c: c.detail_res_id in matching_detail_ids and c.registrant_id == self.registrant_id | |
| ) | |
| return candidates.filtered( | |
| lambda c: c.detail_res_id and c.registrant_id == self.registrant_id and c.detail_res_id in matching_detail_ids | |
| ) |
References
- When processing records that should have related detail records, defensively filter out any records where the detail record is missing to handle edge cases like newly created records.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #208 +/- ##
==========================================
+ Coverage 71.23% 71.27% +0.03%
==========================================
Files 968 976 +8
Lines 57634 57736 +102
==========================================
+ Hits 41057 41150 +93
- Misses 16577 16586 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Summary
scope='custom', so the base conflict domain does not pre-filter by registrant. The hook only narrowed candidates byprogram_id, so submitting a second CR was blocked whenever any other in-flightassign_programCR targeted the same program — even for a different registrant.program_idandregistrant_id, matching the rule's intent (and the override's docstring): conflicts trigger only on the same(registrant, program)pair.test_f5_two_crs_for_different_registrants_same_program_allowedand bump the module to19.0.1.0.1.Test plan
./scripts/test_single_module.sh spp_cr_type_assign_program— 19 passed, 0 failedpre-commit run --files spp_cr_type_assign_program/{__manifest__.py,models/change_request.py,tests/test_assign_program.py}— all hooks passtest_f2); same registrant + different programs still allowed (test_f3)