Fix: Discovery review selection lost due to duplicate tree-data field#19
Open
jpedane wants to merge 1 commit into
Open
Fix: Discovery review selection lost due to duplicate tree-data field#19jpedane wants to merge 1 commit into
jpedane wants to merge 1 commit into
Conversation
Symptom: In Discovery > Task list > Review, selecting devices/modules
and pressing OK always returns "No changes. Re-Scheduled" and no
agents are created, even though valid devices were selected.
Root cause chain:
1. load_modal() in pandora_ui.js executes every inline <script> in a
modal's AJAX response TWICE. Its success handler regex-extracts the
script bodies and runs them through $.globalEval() after calling
settings.target.html(data) -- but jQuery's .html() already executes
embedded scripts during insertion (it only strips the <script> tags
from the resulting DOM, which is what the misleading comment above
the globalEval loop refers to). Every modal inline script therefore
runs twice per open, deterministically.
2. The review tree script emitted by HTML::printTree() appends a hidden
<input name="tree-data-{target}"> into the tree container each time
it runs, with no existence check. Because of (1) it runs twice, so
two same-named inputs end up in the form. The tree widget's onChange
writes the selection via $("#tree-data-tree"), which matches only
the FIRST input; the second stays empty forever.
3. On OK, load_modal's serializer walks '#review :input' and appends
every matching element to the multipart FormData individually, so
both fields are submitted -- populated first, empty second.
4. PHP keeps only the LAST value for a repeated POST field name, so
parseTaskReview() reads get_parameter('tree-data-tree', '') as an
empty string, matches nothing, and answers "No changes.
Re-Scheduled".
Fix (this commit): make printTree() idempotent -- only append the
hidden input if it does not already exist. With the guard, both script
executions share one input, the selection survives serialization, and
parseTaskReview() receives it. Also fixes an adjacent bug on the same
lines where the tree root selector was emitted unquoted (el: $(tree)
instead of el: $("#tree")), which only worked by accident via the
browser's named-element window globals.
Not fixed here (follow-up candidate): the underlying double execution
in load_modal() still runs every modal inline script twice; for this
modal it still renders the device tree twice into the container
(cosmetic after this fix). Removing the redundant globalEval loop in
pandora_ui.js would fix that globally but affects every modal in the
console, so it deserves its own change and testing.
Verified end-to-end with jsdom + the exact jquery-3.6.0.min.js and
simTree.js shipped in this repo, replaying load_modal's injection and
serialization code verbatim against the real printTree() output from
PHP CLI:
BEFORE: 2 tree-data-tree inputs in DOM, 2 fields serialized, second
empty -> PHP receives '' -> "No changes. Re-Scheduled"
AFTER: 1 input, 1 field serialized with the selection JSON ->
agents/modules scheduled for creation
php -l passes on the modified file.
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
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.
Symptom: In Discovery > Task list > Review, selecting devices/modules and pressing OK always returns "No changes. Re-Scheduled" and no agents are created, even though valid devices were selected.
Root cause chain:
load_modal() in pandora_ui.js executes every inline <script> in a modal's AJAX response TWICE. Its success handler regex-extracts the script bodies and runs them through $.globalEval() after calling settings.target.html(data) but jQuery's .html() already executes embedded scripts during insertion (it only strips the <script> tags from the resulting DOM, which is what the misleading comment above the globalEval loop refers to). Every modal inline script therefore runs twice per open, deterministically.
The review tree script emitted by HTML::printTree() appends a hidden into the tree container each time it runs, with no existence check. Because of (1) it runs twice, so two same-named inputs end up in the form. The tree widget's onChange writes the selection via $("#tree-data-tree"), which matches only the FIRST input; the second stays empty forever.
On OK, load_modal's serializer walks '#review :input' and appends every matching element to the multipart FormData individually, so both fields are submitted populated first, empty second.
PHP keeps only the LAST value for a repeated POST field name, so parseTaskReview() reads get_parameter('tree-data-tree', '') as an empty string, matches nothing, and answers "No changes. Re-Scheduled".
Fix (this commit): make printTree() idempotent only append the hidden input if it does not already exist. With the guard, both script executions share one input, the selection survives serialization, and parseTaskReview() receives it. Also fixes an adjacent bug on the same lines where the tree root selector was emitted unquoted (el:$(tree) instead of el: $ ("#tree")), which only worked by accident via the browser's named-element window globals.
Not fixed here (follow-up candidate): the underlying double execution in load_modal() still runs every modal inline script twice; for this modal it still renders the device tree twice into the container (cosmetic after this fix). Removing the redundant globalEval loop in pandora_ui.js would fix that globally but affects every modal in the console, so it deserves its own change and testing.
Verified end-to-end with jsdom + the exact jquery-3.6.0.min.js and simTree.js shipped in this repo, replaying load_modal's injection and serialization code verbatim against the real printTree() output from PHP CLI:
BEFORE: 2 tree-data-tree inputs in DOM, 2 fields serialized, second
empty -> PHP receives '' -> "No changes. Re-Scheduled"
AFTER: 1 input, 1 field serialized with the selection JSON ->
agents/modules scheduled for creation
php -l passes on the modified file.