Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-73875] CSP compatibility for NewNodeConsoleNote #477

Merged
merged 11 commits into from
Oct 10, 2024
Merged

[JENKINS-73875] CSP compatibility for NewNodeConsoleNote #477

Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5570acc
remove inline onClick handler
shlomomdahan Oct 8, 2024
5d44b49
add onClick handler directly after link is created
shlomomdahan Oct 8, 2024
4688829
remove unnecessary if statement
shlomomdahan Oct 10, 2024
31c9c88
Merge branch 'master' into JENKINS-73875
shlomomdahan Oct 10, 2024
ce2feb6
update to select last child of pipeline-show-hide
shlomomdahan Oct 10, 2024
6474384
Merge branch 'JENKINS-73875' of https://github.com/shlomomdahan/workf…
shlomomdahan Oct 10, 2024
019a02b
improve toggle selection
shlomomdahan Oct 10, 2024
99a6a0d
Remove unnecessary line break changes
basil Oct 10, 2024
40b4e3a
use 'toggle' instead of 'this'
shlomomdahan Oct 10, 2024
68d1c2e
use 'toggle' instead of 'this'
shlomomdahan Oct 10, 2024
8656bca
Keep comment after show/hide functionality as in original
basil Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/main/resources/org/jenkinsci/plugins/workflow/job/console/NewNodeConsoleNote/script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,18 @@ Behaviour.specify("span.pipeline-new-node", 'NewNodeConsoleNote', 0, function(e)
var nodeId = e.getAttribute('nodeId')
var startId = e.getAttribute('startId')
if (startId == null || startId == nodeId) {
e.innerHTML = e.innerHTML.replace(/.+/, '$&<span class="pipeline-show-hide"> (<a href="#" onclick="showHidePipelineSection(this); return false">hide</a>)</span>')
e.innerHTML = e.innerHTML.replace(/.+/, '$&<span class="pipeline-show-hide"> (<a href="#" class="pipeline-toggle">hide</a>)</span>')
// TODO automatically hide second and subsequent branches: namely, in case a node has the same parent as an earlier one

const toggle = e.querySelector('.pipeline-toggle');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this is always matching the <span class="pipeline-show-hide"> that we just added on line 17 above? That code is appending <span class="pipeline-show-hide"> elements as children of e, so I am wondering if there might ever be two such children or if there is always one as in your test. Did you try testing nested parallel blocks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@basil

using the below script configuration

parallel first: {
    stage('First Nested') {
        parallel nestedA: {}, nestedB: {}
    }
}, second: {
    stage('"full" details') {
        parallel nestedC: {}, nestedD: {}
    }
}

I recorded the behavior of the toggle links:

https://www.loom.com/share/2acb8c6b041a449793132e3f4a433d70

Everything seems to be working okay. Can you confirm if this is the intended behavior?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I am glad the testing is passing. To make this bulletproof, I think we should select the last child with the pipeline-show-hide class rather than the first. Since the /.+/ regular expression is always appending the new <span> element to the end of the list of child nodes, selecting the last child should always match the node we just added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining that detail. New commit addresses this

if (toggle) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would this element ever not be present if we just added it a few lines earlier? I cannot see the purpose of this if statement.

toggle.addEventListener('click', function(event) {
event.preventDefault();
showHidePipelineSection(this);
});
}
}

// The CSS rule for branch names only needs to be added once per node, so we
// check in case we are viewing the truncated log and have already processed
// a duplicate synthetic span element for this node.
Expand Down Expand Up @@ -126,4 +135,4 @@ function showHidePipelineSection(link) {
}
}
}
}
}