RelationController behavior: support for deeply nested relations#1499
Open
mjauvin wants to merge 7 commits into
Open
RelationController behavior: support for deeply nested relations#1499mjauvin wants to merge 7 commits into
mjauvin wants to merge 7 commits into
Conversation
mjauvin
added
enhancement
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR extends
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant RelationController
participant resolveNestedContext
participant resolveModelForHops
participant aliasNestedConfig
RelationController->>resolveNestedContext: initRelation(model, field)
resolveNestedContext->>resolveModelForHops: walk bracket hops
resolveModelForHops-->>resolveNestedContext: resolved leaf model
RelationController->>aliasNestedConfig: map nested config key
aliasNestedConfig-->>RelationController: aliased config
Related issues: None specified. Related PRs: None specified. Suggested labels: backend, javascript, tests, enhancement Suggested reviewers: Winter CMS backend maintainers familiar with 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
modules/system/assets/ui/js/popup.js (1)
319-319: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGlobal selector counts unrelated, non-nested popups.
$('.control-popup.in')counts every currently open popup document-wide, not just ancestors of the popup being shown. Two unrelated popups opened independently (not nested within each other) would still cascade-offset each other. This may be acceptable for the feature's intended nested-relation use case, but worth confirming it matches the desired UX for unrelated simultaneous popups.Also applies to: 331-331
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/system/assets/ui/js/popup.js` at line 319, The popup stacking logic in popup.js is using a document-wide selector to compute stack depth, so unrelated open popups are being counted as if they were nested. Update the logic around the stackDepth calculation and the related offset behavior to count only ancestor popups for the popup being shown, using the relevant popup show/positioning code paths instead of a global $('.control-popup.in') match. Verify the same fix is applied to the companion stack-depth usage at the other occurrence so simultaneous independent popups do not cascade-offset each other.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/backend/behaviors/RelationController.php`:
- Around line 654-677: The nested hop lookup in resolveModelForHops is bypassing
the current parent relation scope by querying the related model directly, which
can resolve records outside the intended chain. Update the lookup inside
resolveModelForHops to use the relation instance from $model->{$relationName}()
when finding the hop by id, so each hop remains constrained to its parent
relation; keep the existing relation validation and fallback to a new instance
for non-numeric ids.
- Around line 679-725: Avoid mutating $this->originalConfig in
RelationController::aliasNestedConfig(), since the nested alias can leak into
later initRelation() calls and collide with a real root-level field. Update the
aliasing logic to write the leaf-name entry onto the cloned $this->config used
by getConfig()/makeConfig(), while leaving $this->originalConfig unchanged; keep
the existing config key checks and exception paths intact.
In `@modules/system/assets/ui/js/popup.js`:
- Around line 330-336: The Popup positioning logic in the show/reload flow
leaves stale inline offsets on reused dialogs because `top` and `left` are only
applied when `stackDepth > 0`. Update the offset handling in `Popup` so that
when a popup is shown with `stackDepth === 0`, any previously set `top`/`left`
styles are cleared before centering, ensuring reused instances from `$.fn.popup`
and `data.reload()` do not keep an old stacked position.
---
Nitpick comments:
In `@modules/system/assets/ui/js/popup.js`:
- Line 319: The popup stacking logic in popup.js is using a document-wide
selector to compute stack depth, so unrelated open popups are being counted as
if they were nested. Update the logic around the stackDepth calculation and the
related offset behavior to count only ancestor popups for the popup being shown,
using the relevant popup show/positioning code paths instead of a global
$('.control-popup.in') match. Verify the same fix is applied to the companion
stack-depth usage at the other occurrence so simultaneous independent popups do
not cascade-offset each other.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03a5e5e1-f7e1-4bad-95f8-316be00b1c43
📒 Files selected for processing (7)
modules/backend/behaviors/RelationController.phpmodules/backend/behaviors/relationcontroller/partials/_button_delete.phpmodules/backend/behaviors/relationcontroller/partials/_button_remove.phpmodules/backend/behaviors/relationcontroller/partials/_button_unlink.phpmodules/backend/formwidgets/RelationManager.phpmodules/backend/tests/behaviors/RelationControllerNestedTest.phpmodules/system/assets/ui/js/popup.js
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
mjauvin
changed the title
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
1 participant
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.
We can now use this in config_relation.yaml:
Which would create a popup to manage Items in an Order, along with another nested popup to manage Taxes within that Items popup (see screenshot):
Note: deeper nesting can be achieved as well (e.g. items[taxes][other])
The nested popups now have an offset which makes it easier to see the nesting.
Summary by CodeRabbit
Summary
New Features
Bug Fixes
Tests