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.

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-70934] Remove Prototype usages from buildListTable.jelly #7830

Merged
merged 5 commits into from
Apr 19, 2023

Conversation

basil
Copy link
Member

@basil basil commented Apr 10, 2023

See JENKINS-70934 Remove Prototype usages from buildListTable.jelly

Context

See JENKINS-70906. Jenkins core currently uses Prototype 1.7, released on November 15, 2010. The latest version is Prototype 1.7.3, released on September 22, 2015. When an attempt was made to upgrade to 1.7.3 in 2018 in JENKINS-49319, the change had to be reverted. Since this library has been unmaintained for the past 8 years, we ought to eliminate our dependency on it in favor of modern JavaScript APIs.

Problem

See JENKINS-70934. Usages of Prototype remain in buildListTable.jelly.

Solution

Replace these usages with modern JavaScript APIs.

Bonus

While I was here I fixed two CSP warnings by moving this code out of Jelly and into a JavaScript file.

Testing done

Ran mvn clean verify -Dtest=jenkins.widgets.BuildTimeTrendTest,jenkins.widgets.BuildListTableTest. Verified interactively in the UI that the build time trend and the Computer view could still display the list correctly, and clicked on the console link successfully. Verified that the script-src-attr/inline/javascript:tl.getBand(0).scrollToCenter(… and script-src-elem/inline/function displayBuilds(data) { CSP warnings were present before this PR and gone after this PR. Noted that the script-src-elem/inline/Timeline.DateTime… CSP warning was present before and after this PR; this is technical debt that I am explicitly declining to clean up in this PR.

Proposed changelog entries

N/A

Proposed upgrade guidelines

N/A

Submitter checklist

  • The Jira issue, if it exists, is well-described.
  • The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples).
    • Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
  • There is automated testing or an explanation as to why this change has no tests.
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
  • New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
  • New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
  • For dependency updates, there are links to external changelogs and, if possible, full differentials.
  • For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@mention

Maintainer checklist

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

  • There are at least two (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 is not blocking the change.
  • Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
  • Proper changelog labels are set so that the changelog can be generated automatically.
  • If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see 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).

@basil basil added the skip-changelog Should not be shown in the changelog label Apr 10, 2023
@basil basil requested a review from a team April 10, 2023 19:35
@basil basil added the needs-security-review Awaiting review by a security team member label Apr 10, 2023
@daniel-beck
Copy link
Member

CSP warnings were present before this PR and gone after this PR.

Thanks!

Noted that the script-src-elem/inline/Timeline.DateTime… CSP warning was present before and after this PR; this is technical debt that I am explicitly declining to clean up in this PR.

FTR https://github.com/stapler/stapler-adjunct-timeline/blob/c092701bd2856444f5012532f8b1a6d00efef092/src/main/resources/org/kohsuke/stapler/simile/timeline.jelly and #6869 (although the latter might not strictly be necessary, haven't delved deep enough into the widget).

Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

Thanks for using DOM instead of string concatenation!

@basil basil closed this Apr 17, 2023
@basil
Copy link
Member Author

basil commented Apr 17, 2023

@daniel-beck requested changes

I have no interest in accommodating your request.

@daniel-beck daniel-beck self-assigned this Apr 17, 2023
@basil basil reopened this Apr 17, 2023
@daniel-beck daniel-beck self-requested a review April 17, 2023 18:33
@daniel-beck
Copy link
Member

JS now works 👍


While not using innerHTML is generally preferable, the way the job name is displayed doesn't allow word breaks, unlike e.g. the main ListView index, which uses the same <wbr> to break the job name in JobColumn/column.jelly via l:breakable.jelly. I'm unaware of an existing way to do the equivalent in JS directly, so perhaps easiest to undo the latest change here, use innerHTML instead of textContent, and perhaps note a TODO to make it nicer in the future?

Screenshots

Before

Screenshot

After

Screenshot

List View index

Screenshot 2023-04-17 at 22 09 45

@basil
Copy link
Member Author

basil commented Apr 17, 2023

commit 275eebc

@daniel-beck daniel-beck added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Apr 17, 2023
@daniel-beck
Copy link
Member

This PR is now ready for merge. We will merge it after ~24 hours if there is no negative feedback.
Please see the merge process documentation for more information about the merge process.
Thanks!

@daniel-beck daniel-beck added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Apr 17, 2023
Copy link
Contributor

@Wadeck Wadeck left a comment

Choose a reason for hiding this comment

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

As I don't think this PR satisfies the "low-risk" change from exception#3 due to the regressions previously found in the PR, I preferred to just review the PR and approve it to move forward with the two approvals.

I manually tested the change, everything I tried was as expected 👍 Including, the onClick handler, the parentFullDisplayName.

var a3 = document.createElement("a");
a3.classList.add("jenkins-table__button");
a3.href = rootUrl + "/" + e.url + "console";
a3.innerHTML = p.dataset.consoleOutputIcon;
Copy link
Contributor

Choose a reason for hiding this comment

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

For future readers, just in case, the dataset contains the data-console-output-icon attribute converted into consoleOutputIcon. Don't be fooled by the Java variable in the jelly page that is called consoleOutputIcon

window.displayBuilds = function (data) {
var rootUrl = document.head.getAttribute("data-rooturl");
var p = document.getElementById("projectStatus");
p.style.display = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

FTR this change is an inlining of the method from prototype.js:

show: function(element) {
element = $(element);
element.style.display = '';
return element;
},

@@ -122,3 +122,85 @@ function generateSVGIcon(iconName, iconSizeClass) {

return span;
}

/**
* Public method to be called by progressiveRendering's callback
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI this is the same comment as the first method in this file, which is used for the global build trend.
Not ideal, but not blocking.

@daniel-beck daniel-beck merged commit ebc9aa6 into jenkinsci:master Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback security-approved @jenkinsci/core-security-review reviewed this PR for security issues skip-changelog Should not be shown in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants