fix(change_request): let a routing key be served by several mappings - #462
Conversation
Apply on a dynamic-approval type is narrowed to the field the request was routed and approved on, matched against the mapping's source_field. That assumed every value returned by _get_field_to_modify_selection() is a physical source field. They need not be. A name may be offered as a single choice but stored as separate components, so one selectable value legitimately drives several mappings — and matching on source_field alone matched none of them, so such a request applied nothing at all. It did so silently until applying with nothing to write began raising. A mapping can now declare the selectable value it serves through routing_field, defaulting to source_field, so existing configurations are unchanged and no migration is needed. The narrowing itself is untouched, and tested in both directions: a mapping belonging to a different routing key is still not applied, and a non-dynamic type is still not narrowed at all.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #462 +/- ##
==========================================
- Coverage 76.18% 76.02% -0.16%
==========================================
Files 661 627 -34
Lines 44109 43025 -1084
==========================================
- Hits 33603 32711 -892
+ Misses 10506 10314 -192
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
One small must-fix (inline), otherwise this is an approve — the mechanism is right and the blast radius is genuinely zero for existing configs.
Verified:
- The premise holds:
selected_field_namecomes from_get_field_to_modify_selection(), which nothing requires to be physical source fields, sosource_field-only matching cannot express one-choice-several-components and matched nothing. - The #264 narrowing is preserved, and more importantly the #422 detection lockstep is untouched:
_proposed_changed_fieldsdeliberately iterates every configured mapping by physicalsource_field, so widening what apply's routing key can name doesn't desync detection — mappings serving a compound key still carry physical source fields and are detected per-field. - No privilege change:
routing_fieldis mapping config, authored at the same trust level assource_field/target_fieldthemselves. - Tests pin both directions (multi-mapping applies, foreign-key mapping doesn't), the default (
source_fieldwhen unset), the shadowing semantic (set means replaces, not adds — matching the help text), and non-dynamic types staying un-narrowed. - Version
19.0.3.1.13correctly leaves.11/.12to #459/#461; HISTORY conflicts only textually if merged out of order, as flagged. - Skipping the config-time constraint is the right call for the XML-load reason given; a post-load lint is worth its own issue so it isn't lost.
| required=True, | ||
| help="Field name on detail model", | ||
| ) | ||
| routing_field = fields.Char( |
There was a problem hiding this comment.
Must fix: routing_field is not exposed in any view. The inline mapping list in change_request_type_views.xml (~line 161) shows sequence / source_field / target_field / transform / transform_expression — an admin configuring a type through the UI can neither set this field nor see it. That cuts both ways: the feature is XML/API-only, and — per your own test_routing_field_shadows_source_field_for_matching — a value set via API makes source_field silently stop matching with nothing in the form explaining why apply now refuses. One line fixes it:
<field name="routing_field" optional="hide" />(optional="hide" keeps the common case uncluttered while making the shadowing visible to whoever goes looking.)
There was a problem hiding this comment.
Fixed in baa249f — exactly the one line suggested, <field name="routing_field" optional="hide" />, placed right after source_field since that is the field it shadows. Also pinned with a test (test_routing_field_is_exposed_in_the_type_form_view) so it cannot silently drop out of the view again. Full module suite: 401 tests, 0 failures.
…list routing_field shadows source_field for matching, so a value set only through XML or the API made apply refuse with nothing in the form explaining why. Show it in the inline mapping list, hidden by default to keep the common case uncluttered.
#459 and #461 were squash-merged (481cc27, ac0e38e), conflicting with this branch in the version line, HISTORY, tests/__init__.py, and the generated README/index.html. Resolved to 19.0.3.1.13 with the HISTORY entries stacked (3.1.13 over 3.1.12 over 3.1.11), both test imports kept, and README/index.html regenerated from the resolved fragments with CI's table-width rendering applied.
2 participants
A selectable field on a dynamic-approval type can only be applied if a mapping's
source_fieldmatches it exactly. When one selectable value is applied through several mappings, nothing matches and the request applies nothing.The assumption
Apply is narrowed to the field a request was routed and approved on — correct and load-bearing, since the whole point is that only the approved change is written:
But
selectedcomes fromselected_field_name, which is synced from the detail'sfield_to_modify— and that is chosen from_get_field_to_modify_selection(), an arbitrary list of selectable values. Nothing requires those values to be physical source fields.A legitimate and not unusual shape: a name presented to the operator as one choice, but captured and applied as separate components. One selectable value, several mappings, none of whose
source_fieldequals it. The filter matches nothing, and the request applies nothing at all.Until recently that failed silently — apply returned success having written nothing, and the request was stamped applied. It now raises, which is how this became visible, but the underlying configuration still cannot be expressed.
The fix
A mapping may declare the selectable value it serves:
Empty means today's behaviour, so existing configurations are unchanged and no migration is needed.
The narrowing is unchanged
This widens what a routing key can name, not what gets applied. Both directions are tested:
routing_fieldor no.Full
spp_change_request_v2suite: 400 tests, 0 failures.Considered and rejected: a config-time constraint
A check that every selectable value resolves to at least one mapping would catch this at install rather than at apply — for
field_mappingtypes an unmapped selectable value can never be applied, so it is always a misconfiguration.I did not add one. As an
@api.constrainsit would fire during XML data loading, where the type record is created before its mapping records exist, and fail a clean install. Making it robust needs a post-load report or a lint rather than a constraint, which is worth doing separately rather than bundling here.Merge order
19.0.3.1.10 → 19.0.3.1.13, leaving3.1.11to #459 and3.1.12to #461. Independent of both in content; only the version and changelog conflict, and only if merged out of order.