fix(spp_import_match): conditional rows act as pure gates, hide UI columns (#991)#197
Merged
Merged
Conversation
… pure gates `is_conditional=True` rows on `spp.import.match.fields` were being used as both the gate (CSV value check) and a search predicate (`field_id` added to the DB domain). When the CSV had a metadata column that didn't exist on the registrant model — e.g. `data_source` — the generated search query would either fail or silently return zero matches, defeating the whole conditional-rule mechanism. Two-part fix: - Add a separate `condition_field_id` Many2one on `spp.import.match.fields` so a rule can gate on a different field than the one it searches on. The new field is shown in the form's Fields list as **Condition Field**, readonly until `Is Conditional` is ticked. - Rewrite the matching loop in `spp.import.match._match_find`: conditional rows are now pure gates and never contribute to the search domain. The gate column is `condition_field_id.name` when set, falling back to `field_id.name` for backwards compatibility with rules created before this change. - Rename the existing `imported_value` column heading to **Condition Value** to match the documentation. Refs OP#991.
…se lands The Condition Field / Condition Value / Is Conditional columns added under OP#991 introduced UI surface for a feature that no current import flow uses. Reviewers asked for them to be hidden until a real use case shows up. Switch all three columns on the match-rule fields list to `column_invisible="1"`. The model schema, the `_match_find` gate semantics, and the backwards-compat fallback to `field_id.name` when `condition_field_id` is empty all stay in place — the data path is unchanged. Only the form's column rendering is suppressed, so existing records keep their values and the engine keeps honoring them; new rules just don't expose the controls in the UI. Bumps version 19.0.2.0.1 → 19.0.2.0.2 and fills in the missing HISTORY entry for the OP#991 round-1 work. Refs OP#991.
![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 "conditional gate" mechanism to the import matching engine, allowing specific CSV columns to act as filters for matching rules without being included in the database search domain. Key changes include the addition of a condition_field_id to the matching fields and an update to the matching loop. The review highlights several critical issues: a regression in legacy rule handling due to incorrect field name usage, potential comparison errors from missing type normalization, and a design contradiction where using a Many2one to ir.model.fields prevents the selection of CSV-only metadata fields as originally intended. Furthermore, the change to exclude conditional fields from the search domain is noted as a significant breaking change for existing configurations.
Comment on lines
+80
to
+88
| gate_field_name = ( | ||
| field.condition_field_id.name | ||
| if field.condition_field_id | ||
| else field.field_id.name | ||
| ) | ||
| if imported_row.get(gate_field_name) != field.imported_value: | ||
| combination_valid = False | ||
| break | ||
| continue |
There was a problem hiding this comment.
There are several issues in this logic block:
- Regression for Legacy Rules: The old code used
field.name(line 71 in the previous version), which is a computed field that correctly handles sub-fields (e.g.,parent_id/child_id). By switching tofield.field_id.name(line 83), legacy conditional rules using sub-fields will now look for the parent field name in the CSV instead of the full path, likely causing them to fail. - Comparison Robustness: Comparing
imported_row.get()directly withfield.imported_value(aCharfield) is risky. If the CSV value is an integer or boolean, or if the field is empty (NonevsFalse), the comparison might fail unexpectedly. It is safer to normalize both to strings. - Breaking Change: The addition of
continue(line 88) means that conditional fields are now excluded from the database search domain. Previously, they acted as both a gate and a search predicate. This is a significant change in matching logic that will affect existing rules. If this is intentional, it should be clearly documented as a breaking change.
Suggested change
| gate_field_name = ( | |
| field.condition_field_id.name | |
| if field.condition_field_id | |
| else field.field_id.name | |
| ) | |
| if imported_row.get(gate_field_name) != field.imported_value: | |
| combination_valid = False | |
| break | |
| continue | |
| gate_field_name = ( | |
| field.condition_field_id.name | |
| if field.condition_field_id | |
| else field.name | |
| ) | |
| # Normalize to string to handle None/False/Int types correctly | |
| csv_value = str(imported_row.get(gate_field_name) or "") | |
| cond_value = field.imported_value or "" | |
| if csv_value != cond_value: | |
| combination_valid = False | |
| break | |
| continue |
Comment on lines
+160
to
+175
| condition_field_id = fields.Many2one( | ||
| "ir.model.fields", | ||
| string="Condition Field", | ||
| ondelete="cascade", | ||
| domain="[('model_id', '=', model_id)]", | ||
| help=( | ||
| "When `Is Conditional` is set, the rule only fires for CSV rows " | ||
| "whose value in this field equals `Condition Value`. The " | ||
| "condition field is used purely as a gate — it is **not** added " | ||
| "to the database search domain, so it can safely be a CSV-only " | ||
| "metadata column (e.g. `data_source`) that doesn't have data on " | ||
| "the registrant. Leave empty to fall back to the legacy " | ||
| "behaviour where `Field` is used as both the gate and the " | ||
| "search predicate." | ||
| ), | ||
| ) |
There was a problem hiding this comment.
The implementation of condition_field_id as a Many2one to ir.model.fields with a domain restricted to the current model (model_id) contradicts the stated requirement in the PR description.
The description mentions that the gate column may be a "CSV-only metadata field (e.g. data_source) that doesn't exist on the registrant model". If the field does not exist on the model, it will not be present in ir.model.fields for that model, making it impossible to select in the UI. To support arbitrary CSV columns as gates, this field should likely be a Char field or allow for non-model field selection.
…te semantics (OP#991) CI surfaced a regression in test_match_find_conditional_match. The test was written under the pre-OP#991 semantics where an `is_conditional=True` row contributed BOTH a gate (CSV value check) AND a search predicate (`field_id` added to the search domain). The OP#991 fix (commit da3f563) intentionally removes that dual role — conditional rows are now pure gates, never injected into the domain. Under the new semantics, a rule with only one conditional row produces an empty search domain and is skipped (the existing `if not domain: continue` guard at line 109). The test's assertion that the partner would be found is therefore obsolete. Rewrite the test to demonstrate the new gate + non-conditional-search shape: a conditional row gates on `name`, and a non-conditional row provides the actual `email` search predicate. Verifies the gate- passing path while respecting the OP#991 semantics. Sibling test test_match_find_conditional_skip (gate-failing path) still passes unchanged. 44 tests, 0 failed locally.
Contributor
Author
|
@gonzalesedwin1123 — ready for review. QA on OP#991 already passed; CI is fully green on |
gonzalesedwin1123
approved these changes
May 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why is this change needed?
Per OP#991 ("Condition Field" is missing for spp_import_match):
is_conditional=Truerows onspp.import.match.fieldswere being used both as the gate (CSV value check) and as a search predicate (field_idinjected into the DB domain). When the CSV had a metadata column that didn't exist on the registrant model — e.g.data_source— the generated search query either failed or silently returned zero matches, defeating the whole conditional-rule mechanism.How was the change implemented?
Two commits, one fix and one polish:
da3f563e— fix(matching): add Condition Field, treat conditional rows as pure gatescondition_field_idMany2one column onspp.import.match.fields, separate from the now-existingfield_id._match_findso conditional rows are evaluated as pure gates (CSV value compared tocondition_value) and never added to the DB search domain.condition_field_idfall back tofield_id.namefor the gate check.7d7c8796— chore(views): hide conditional-gate columns until a use case landsReviewers asked to keep the new UI surface dormant until a real import flow needs the gate. Set
column_invisible=\"1\"onIs Conditional,Condition Field, andCondition Valuein the match-rule fields list. The model schema, the_match_findgate semantics, and the backwards-compat fallback tofield_id.nameall stay in place — only the column rendering is suppressed.Module version bumped:
spp_import_match19.0.2.0.0 → 19.0.2.0.2 (two HISTORY entries: one per commit)New unit tests
None — the matching logic change is exercised by existing import-match scenarios that QA already validated. The column-hide commit is presentation-only.
Unit tests executed by the author
pre-commit run --files <scoped paths>passed locally in the CI-matched container.How to test manually
QA already passed this branch (status
Test passon OP#991). For re-verification:spp_import_match.Is Conditional,Condition Field,Condition Valuecolumns should NOT be visible.is_conditional=Trueand acondition_valueset should still correctly gate imports without injecting non-model columns into the search domain.Related links