[JENKINS-60866] Advanced button, build health, confirmation link, URI encoding monitor #5515

daniel-beck · 2021-05-22T08:50:03Z

See JENKINS-60866.

Testing Notes

confirmationLink is e.g. the "Reload from Disk" item on /manage, the script controls the confirmation dialog.
buildHealth is the weather column and the weather icon in the build history title, the script controls hover behavior.
URICheckEncodingMonitor is only available on /manage. It shows fairly easily for me (who has Latin 1?) but the error I could only trigger by changing params in JS so that actual/expected are different. The monitor has nonstandard appearance since we styled them differently from 1.x, out of scope for this.
advanced is the script that controls whether the "something was edited" message appears to the left of the button; this is trivially the case with Mailer plugin's "Send a test email" misuse of this control.

Proposed changelog entries

Entry 1: Issue, Human-readable Text
...

Proposed upgrade guidelines

N/A

Submitter checklist

(If applicable) Jira issue is well described
Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
- Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
Appropriate autotests or explanation to why this change has no tests
For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

There are at least 2 approvals for the pull request and no outstanding requests for change
Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
Changelog entries in the PR title and/or Proposed changelog entries are correct
Proper changelog labels are set so that the changelog can be generated automatically
If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

… monitor

test/src/test/java/lib/layout/ConfirmationLinkTest.java

timja

How is this related to https://issues.jenkins.io/browse/JENKINS-60881 ?

daniel-beck · 2021-05-24T19:52:27Z

@timja Thanks, updated.

Wadeck

The missing wrapping behavior could be potentially problematic but I have no examples to demonstrate that :-)

🐛 Only one where I have serious doubts about is the healthReport in the widget. The adjunct seems not loaded/effective.

Working proofs

In case something was wrong but I was not aware :p

URICheckEncodingMonitor

Advanced button

Validate button

BuildHealth in the job table

The effect is working fine and the adjunct is inserted.

Hover working thanks to the added/removed class

Adjunct added as expected

Confirmation link

Not working proofs

🐛 BuildHealth in the left widget

[Disclaimer] To see anything, you need to have at least one build in the history of the job you are looking, otherwise the tooltip will not exist.

The effect is working because of :hover, not because of the .hover class added/removed by the script.
The script itself does not seem to be loaded, even if using the debugging I saw the code from AdjunctsInPage#generate being called.

💡 AFAICT the problem is coming from the widget/ajax nature of that feature.

Be careful, the "desired effect" is working, but not in the way you would expect, and thus could lead to other errors in the future that will be very complicated to solve as the code will "seem to work".

No hover, but tooltip displayed thanks to :hover

No adjunct for BuildHealth

On ci.jenkins.io

As you can see, the hover class is added and you can even see the mouseenter/out event inlined at that point.

Debug information

I compared the confirmationLink adjunct with the buildHealth. The first one is included correctly in the page, compared to the second that is not. The difference I have seen is about the out, being an HTMLWriterOutput for confirmationLink (and all other adjuncts) and an XMLOutput for buildHealth.

Debug with intelligence (headache included for free 🎁)

We are changing HTMLWriterOutput to XMLOutput in the frames at ParseTag#parseBody:

More precisely at HistoryWidget/index.jelly#L28, that is then executed in HistoryWidget/index.jelly#L62.

The problem is that AdjunctsInPage#generate is writing the script "directly" to the output, meaning in text instead of as a tag. When the ParseTag#parseBody does the endDocument, it erases the textBuffer and so, delete the just added script. To correct this behavior we should write the script as a tag, like:

Which, is working fine.

💡 The quick workaround is to call manually the adjunct from buildHealth before the f:parse (manualy tested).

Wadeck · 2021-05-25T06:21:37Z

core/src/main/resources/lib/form/validateButton/adjunct.js

@@ -0,0 +1,3 @@
+Behaviour.specify('.Behaviour-validateButton', 'Behaviour-validateButton', 0, function(element) {
+    element.onclick = function() { safeValidateButton(this); };


💡 Compared to current behavior, this could be executed after another script that is taken care of the existing onClick and wrap it, to have 2 listeners. Leading to erasing the previously (other) registered listener.

I am not aware of any of those for validateButton.onClick in Jenkins from memory, but if that could trigger someone's memory, that could be nice :)

Example of wrapping behavior: UploadedKeyStoreSource/config.jelly#L50-L55, AFAICT this one is executed after "inline" (like current) and Behaviour (proposed one).

core/src/main/resources/lib/hudson/buildHealth.jelly

test/src/test/java/lib/layout/ConfirmationLinkTest.java

Wadeck · 2021-05-25T08:28:56Z

core/src/main/resources/lib/layout/confirmationLink/adjunct.js

@@ -0,0 +1,18 @@
+Behaviour.specify("A.confirmation-link", 'confirmation-link', 0, function (element) {
+    element.onclick = function(event) {


Same comment as for validateButton

oleg-nenashev · 2021-09-28T13:45:51Z

@Wadeck did you have a chance to re-review it?

Wadeck

Working fine and correctly now, thanks for the correct 👍

oleg-nenashev

Approved once the merge conflict is resolved

timja · 2021-10-15T13:27:24Z

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

daniel-beck · 2021-10-15T19:16:54Z

Working fine and correctly now, thanks for the correct 👍

@Wadeck I noticed this PR still removes https://github.com/jenkinsci/jenkins/pull/5515/files#diff-633560a936835daf74727cace9bff8b456cce93c71f367710bd7865ff3d4d311L41-L43, do you know whether this is fine? I do not remember 😞

Wadeck

(from Daniel) do you know whether this is fine? I do not remember 😞

(from my previous tests) The effect is working because of :hover, not because of the .hover class added/removed by the script.

So, it's a cleanup => 👍

Also, I checked the related CSS to this "feature", it's expecting any of :hover or .hover. So another cleanup task could be to remove the .hover part as a follow-up :) (code: style.less#L787-L792

(additional manual testing with the new BuildHistory from Jan 🎉)

[JENKINS-60881] Advanced button, build health, confirmation link, URI…

8b5002b

… monitor

daniel-beck requested a review from Wadeck May 22, 2021 08:50

daniel-beck mentioned this pull request May 22, 2021

Fix redirect link #5516

Merged

10 tasks

daniel-beck added 2 commits May 22, 2021 20:05

Fix test

9c9a65b

Fix test

633ea08

daniel-beck commented May 22, 2021

View reviewed changes

test/src/test/java/lib/layout/ConfirmationLinkTest.java Show resolved Hide resolved

daniel-beck added the squash-merge-me Unclean or useless commit history, should be merged only with squash-merge label May 22, 2021

timja reviewed May 24, 2021

View reviewed changes

daniel-beck changed the title ~~[JENKINS-60881] Advanced button, build health, confirmation link, URI encoding monitor~~ [JENKINS-60866] Advanced button, build health, confirmation link, URI encoding monitor May 24, 2021

Wadeck requested changes May 25, 2021

View reviewed changes

Remove unnecessary JS

a0b6587

daniel-beck requested a review from Wadeck May 26, 2021 15:42

jglick mentioned this pull request Jun 1, 2021

Adjunct not working when inside j:parse jenkinsci/stapler#234

Open

Wadeck approved these changes Oct 1, 2021

View reviewed changes

oleg-nenashev added the skip-changelog Should not be shown in the changelog label Oct 15, 2021

oleg-nenashev approved these changes Oct 15, 2021

View reviewed changes

Merge branch 'master' into more-js-cleanup

d43b425

timja added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Oct 15, 2021

daniel-beck removed the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Oct 15, 2021

daniel-beck requested a review from Wadeck October 23, 2021 10:02

Wadeck approved these changes Nov 1, 2021

View reviewed changes

Wadeck added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Nov 1, 2021

timja merged commit 791447a into jenkinsci:master Nov 4, 2021

timja mentioned this pull request Jan 22, 2022

JENKINS-67552 Use same animation as core for pipeline logs jenkinsci/workflow-job-plugin#235

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-60866] Advanced button, build health, confirmation link, URI encoding monitor #5515

[JENKINS-60866] Advanced button, build health, confirmation link, URI encoding monitor #5515

daniel-beck commented May 22, 2021 •

edited

timja left a comment

daniel-beck commented May 24, 2021

Wadeck left a comment

Wadeck May 25, 2021

Wadeck May 25, 2021

oleg-nenashev commented Sep 28, 2021

Wadeck left a comment

oleg-nenashev left a comment

timja commented Oct 15, 2021

daniel-beck commented Oct 15, 2021

Wadeck left a comment

		@@ -0,0 +1,3 @@
		Behaviour.specify('.Behaviour-validateButton', 'Behaviour-validateButton', 0, function(element) {
		element.onclick = function() { safeValidateButton(this); };

		@@ -0,0 +1,18 @@
		Behaviour.specify("A.confirmation-link", 'confirmation-link', 0, function (element) {
		element.onclick = function(event) {

[JENKINS-60866] Advanced button, build health, confirmation link, URI encoding monitor #5515

[JENKINS-60866] Advanced button, build health, confirmation link, URI encoding monitor #5515

Conversation

daniel-beck commented May 22, 2021 • edited

Testing Notes

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

timja left a comment

Choose a reason for hiding this comment

daniel-beck commented May 24, 2021

Wadeck left a comment

Choose a reason for hiding this comment

URICheckEncodingMonitor

Advanced button

Validate button

BuildHealth in the job table

Hover working thanks to the added/removed class

Adjunct added as expected

Confirmation link

🐛 BuildHealth in the left widget

No hover, but tooltip displayed thanks to :hover

No adjunct for BuildHealth

On ci.jenkins.io

Debug information

Debug with intelligence (headache included for free 🎁)

Wadeck May 25, 2021

Choose a reason for hiding this comment

Wadeck May 25, 2021

Choose a reason for hiding this comment

oleg-nenashev commented Sep 28, 2021

Wadeck left a comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

timja commented Oct 15, 2021

daniel-beck commented Oct 15, 2021

Wadeck left a comment

Choose a reason for hiding this comment

daniel-beck commented May 22, 2021 •

edited