feat(spp_change_request_v2): add dynamic approval-aware conflict & duplicate detection #84
Conversation
Add dynamic approval framework to CR types and change requests: - CR type: use_dynamic_approval toggle and candidate_definition_ids - CR: selected_field_name/old_value/new_value for field tracking - Detail base: field_to_modify selection, sync to parent CR on write/create - Dynamic approval resolution via CEL condition evaluation on candidates - Value normalization for CEL (Many2one, vocabulary codes, hierarchical)
…icate detection Update conflict and duplicate detection to narrow scope for dynamic CRs: - Conflict field filtering uses only selected_field_name for dynamic types - Duplicate similarity compares only the selected field (0% for different fields) - Skip conflict checks at create time for dynamic CRs (field not set yet) - Trigger conflict re-check when selected_field_name is written
Add UI for dynamic approval configuration and field change display: - CR type form: dynamic approval toggle, candidate definitions, CEL help text - CR form: field change group showing selected field, old value, new value
…ion tests 28 tests covering: - Dynamic approval routing, CEL evaluation, field sync, value normalization - Multi-tier approval with dynamic routing, E2E workflow - Dynamic-aware conflict detection (field-level, cross-type, timing) - Dynamic-aware duplicate detection (similarity scoring)
Add USAGE.md with three progressive examples (basic, multi-tier, dynamic approval), CEL variable reference, and implementation checklist. Update DESCRIPTION.md to mention dynamic approval routing capability.
Auto-generated by oca-gen-addon-readme from updated DESCRIPTION.md and new USAGE.md fragments.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 19.0 #84 +/- ##
==========================================
+ Coverage 68.43% 69.15% +0.72%
==========================================
Files 562 628 +66
Lines 31853 35632 +3779
==========================================
+ Hits 21798 24643 +2845
- Misses 10055 10989 +934
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the change request system by introducing a dynamic approval framework. This allows for highly flexible and context-sensitive approval workflows, where the specific field being modified dictates the routing logic through Common Expression Language (CEL) conditions. Concurrently, the system's conflict and duplicate detection capabilities have been made 'approval-aware,' ensuring that these checks accurately reflect the dynamically selected field, thereby improving the precision and relevance of these safeguards. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
…thods Add 12 tests covering previously uncovered branches in change_request.py: - _normalize_value_for_cel: char, date, boolean, integer, many2many, falsy boolean/integer, None without record, hierarchical parent - _compute_field_values_for_cel: no field selected - _resolve_dynamic_approval: no selected field - _check_can_submit: dynamic CR without field selection
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-designed dynamic approval framework, allowing change request workflows to be determined by the specific field being modified. The implementation is robust, covering CEL-based condition evaluation, field change tracking, and updates to conflict and duplicate detection to be approval-aware. The addition of comprehensive tests (28 new tests) and detailed developer documentation in USAGE.md is excellent.
I have two suggestions for improvement:
- A high-severity issue in the duplicate detection logic for "mixed" cases (dynamic vs. static CRs), which could lead to inaccurate similarity scores.
- A medium-severity suggestion to refactor the
writemethod inchange_request_detail_base.pyfor better readability and maintainability.
Overall, this is a high-quality contribution that greatly enhances the flexibility of the change request system.
| # Dynamic approval: compare only the selected field | ||
| my_selected = self.selected_field_name | ||
| other_selected = other_cr.selected_field_name | ||
| if my_selected and other_selected: | ||
| # Different fields selected = not duplicates | ||
| if my_selected != other_selected: | ||
| return 0.0 | ||
| # Same field: compare that field's value only | ||
| if my_selected in my_detail._fields and my_selected in other_detail._fields: | ||
| my_value = self._normalize_field_value(getattr(my_detail, my_selected, None)) | ||
| other_value = self._normalize_field_value(getattr(other_detail, my_selected, None)) | ||
| if my_value == other_value: | ||
| return 100.0 | ||
| elif self._are_similar(my_value, other_value): | ||
| return 80.0 | ||
| return 0.0 | ||
|
|
||
| # Static CRs (or mixed): original logic | ||
| check_fields = config.get_check_fields_list() |
There was a problem hiding this comment.
The current logic for duplicate detection in _calculate_similarity correctly handles the case where both CRs are dynamic. However, it doesn't correctly handle the "mixed" case where one CR is dynamic and the other is static. In this scenario, it falls back to the original logic, which compares all configured check_fields. This can lead to inflated similarity scores because pre-filled fields on the dynamic CR (which are not being changed) will match pre-filled fields on the static CR.
For example, if a dynamic CR changes given_name and a static CR changes family_name, the pre-filled phone number on both will match, incorrectly increasing their similarity.
The logic should be updated to only compare the fields that are effectively being changed in each CR. For a dynamic CR, that's just the selected_field_name. For a static CR, it's the fields defined in the duplicate configuration. The comparison should be on the intersection of these effective fields.
Here is a suggested implementation for the entire _calculate_similarity method that addresses this:
def _calculate_similarity(self, other_cr, config):
self.ensure_one()
my_detail = self.get_detail()
other_detail = other_cr.get_detail()
if not my_detail or not other_detail:
return 0.0
my_selected = self.selected_field_name
other_selected = other_cr.selected_field_name
config_fields = set(config.get_check_fields_list())
# Determine effective fields to compare for each CR.
my_fields = {my_selected} if my_selected else config_fields
other_fields = {other_selected} if other_selected else config_fields
# Fallback for static CRs with no fields configured: compare all storable fields.
if not my_selected and not my_fields:
my_fields = {
f for f in my_detail._fields if my_detail._fields[f].store and f not in ('id', 'create_date', 'write_date', 'create_uid', 'write_uid', 'change_request_id', 'registrant_id', 'approval_state', 'is_applied')
}
if not other_selected and not other_fields:
other_fields = {
f for f in other_detail._fields if other_detail._fields[f].store and f not in ('id', 'create_date', 'write_date', 'create_uid', 'write_uid', 'change_request_id', 'registrant_id', 'approval_state', 'is_applied')
}
# The fields to compare are the intersection of what each CR is proposing to change.
fields_to_check = my_fields & other_fields
if not fields_to_check:
return 0.0
total_fields = len(fields_to_check)
matching_score = 0.0
for field_name in fields_to_check:
if field_name not in my_detail._fields or field_name not in other_detail._fields:
continue
my_value = self._normalize_field_value(getattr(my_detail, field_name, None))
other_value = self._normalize_field_value(getattr(other_detail, field_name, None))
if my_value == other_value:
matching_score += 1.0
elif self._are_similar(my_value, other_value):
matching_score += 0.8
return (matching_score / total_fields) * 100.0 if total_fields > 0 else 0.0| def write(self, vals): | ||
| result = super().write(vals) | ||
| if "field_to_modify" in vals: | ||
| for rec in self: | ||
| if rec.change_request_id: | ||
| rec._sync_field_to_modify() | ||
| else: | ||
| for rec in self: | ||
| if rec.field_to_modify and rec.field_to_modify in vals and rec.change_request_id: | ||
| rec._sync_field_to_modify() | ||
| return result |
There was a problem hiding this comment.
The write method can be simplified to improve readability and avoid iterating over self in two separate blocks. The current implementation correctly handles syncing when field_to_modify or its target field's value changes, but the logic can be combined into a single loop for better maintainability.
| def write(self, vals): | |
| result = super().write(vals) | |
| if "field_to_modify" in vals: | |
| for rec in self: | |
| if rec.change_request_id: | |
| rec._sync_field_to_modify() | |
| else: | |
| for rec in self: | |
| if rec.field_to_modify and rec.field_to_modify in vals and rec.change_request_id: | |
| rec._sync_field_to_modify() | |
| return result | |
| def write(self, vals): | |
| result = super().write(vals) | |
| for rec in self: | |
| if not rec.change_request_id: | |
| continue | |
| # Sync if the field selector is changed, or if the value of the selected field is changed. | |
| if "field_to_modify" in vals or (rec.field_to_modify and rec.field_to_modify in vals): | |
| rec._sync_field_to_modify() | |
| return result |
2 participants
Summary
Changes
Models
use_dynamic_approvaltoggle,candidate_definition_ids(Many2many to approval definitions with CEL conditions)selected_field_name,selected_field_old_value,selected_field_new_valuefields;_resolve_dynamic_approval()with CEL evaluation;_compute_field_values_for_cel()with valuenormalization (Many2one, vocabulary codes, hierarchical parents)
field_to_modifyselection field;write()/create()overrides to sync selection to parent CR_filter_by_field_conflicts()narrows to selected field for dynamic CRs;_calculate_similarity()compares only selected fieldselected_field_nameis writtenViews
Tests (28 new tests)
Documentation
USAGE.md: Developer guide with 3 progressive examples (basic, multi-tier, dynamic approval), CEL variable reference, and implementation checklistTest plan