Hacktoberfest [JENKINS-66730] Un-inlining AsynchPeople/index.jelly #5835

P4uline · 2021-10-19T12:04:39Z

See JENKINS-66730.

Proposed changelog entries

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

Proposed upgrade guidelines

N/A

Submitter checklist

(If applicable) Jira issue is well described
[n/a] 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
[n/a] Appropriate autotests or explanation to why this change has no tests
[n/a] 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).

timja · 2021-10-19T19:38:16Z

core/src/main/resources/hudson/model/View/AsynchPeople/people-resources.js

+
+        var d = document.createElement('td');
+        var a = document.createElement('a');
+        a.href = '${rootURL}/' + e.url;


I don't think rootURL is available like this, cc @Wadeck

Looks to be the same issue as #5787 (comment)

Do you think this link could be relative instead of using ${rootURL} ?

Be careful, the ${rootURL} is NOT the root URL :'( it's the contextPath.

jenkins/core/src/main/java/hudson/Functions.java

Lines 278 to 285 in 632033d

String rootURL = currentRequest.getContextPath();

Functions h = new Functions();

context.setVariable("h", h);

// The path starts with a "/" character but does not end with a "/" character.

context.setVariable("rootURL", rootURL);

(yes this is crappy :D)

aHenryJard · 2021-10-22T11:15:12Z

core/src/main/resources/hudson/model/View/AsynchPeople/people-resources.js

+        a.className = 'model-link inside';
+        var i = document.createElement('img');
+        i.src = e.avatar;
+        i.className = 'icon${iconSize}';


is this ${iconSize} an issue too maybe ?

Wadeck · 2021-10-23T09:30:17Z

Thanks @P4uline for the contribution :)

For the variables, you have to use https://www.jenkins.io/doc/developer/security/xss-prevention/#passing-values-to-javascript to pass the value from Java context to JavaScript.

In this particular PR, you're lucky that rootURL is provided by layout.jelly#L103. You can do something similar to buildTimeTrend_resources.js#L7 to retrieve the value stored by the layout.jelly.

For the iconSize, you're unlucky ^^ It's a bit more complicated. The variable is defined by (t:setIconSize)[https://github.com/jenkinsci/jenkins/blob/ae557780a59d372d09dc71a5490da98b577da397/core/src/main/resources/hudson/model/View/AsynchPeople/index.jelly#L29], which create a variable inside the context of the page. To get access to this variable, you have to set its value as explained in the first link from this comment.

You can change the <p>${%blurb}</p> for example, to contain the value you want to pass to the JavaScript:

Example of modification

💡 I recommend that you test your code when you are updating pages, for example this feature is available at http://localhost:8080/jenkins/asynchPeople/ and you can see the different variables not being replaced by the expected value.

Screenshot

I hope that the explanations are clear otherwise just ask questions ;)

Wadeck

Just to be sure this PR is not merged as is, the variables are not replaced as expected.

💡 I updated the "submitter" checklist, it's expected to be filled by the author ;)

P4uline · 2021-10-30T19:38:43Z

Thank you for your feedbacks. I just pushed some changes.

Wadeck

Unfortunately your screenshot does not include the "full page" and thus, does not show that the "blurb" was duplicated with the latest change.

🎉 The good news: the variables are working correctly 👍

Screenshot

I tested locally, if you apply my proposed changes (both!) your PR will be working fine :)

core/src/main/resources/hudson/model/View/AsynchPeople/index.jelly

…elly Co-authored-by: Wadeck Follonier <Wadeck@users.noreply.github.com>

oleg-nenashev

The new version looks mfine

Wadeck

Not manually re-tested but the change is what I expected, thanks for the changes! 🚀

timja · 2021-11-08T13:21:58Z

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

Thanks!

Will merge in time for weekly if no objections

[JENKINS-66730] Un-inlining AsynchPeople/index.jelly

c10c037

P4uline changed the title ~~[JENKINS-66730] Un-inlining AsynchPeople/index.jelly~~ Hacktoberfest [JENKINS-66730] Un-inlining AsynchPeople/index.jelly Oct 19, 2021

timja reviewed Oct 19, 2021

View reviewed changes

P4uline marked this pull request as ready for review October 22, 2021 11:12

aHenryJard reviewed Oct 22, 2021

View reviewed changes

Wadeck requested changes Oct 23, 2021

View reviewed changes

Wadeck added skip-changelog Should not be shown in the changelog squash-merge-me Unclean or useless commit history, should be merged only with squash-merge labels Oct 23, 2021

fix variables

29732c6

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

Wadeck requested changes Nov 1, 2021

View reviewed changes

core/src/main/resources/hudson/model/View/AsynchPeople/index.jelly Outdated Show resolved Hide resolved

core/src/main/resources/hudson/model/View/AsynchPeople/index.jelly Outdated Show resolved Hide resolved

P4uline and others added 2 commits November 1, 2021 21:41

Update core/src/main/resources/hudson/model/View/AsynchPeople/index.j…

94582db

…elly Co-authored-by: Wadeck Follonier <Wadeck@users.noreply.github.com>

Update core/src/main/resources/hudson/model/View/AsynchPeople/index.j…

330b59c

…elly Co-authored-by: Wadeck Follonier <Wadeck@users.noreply.github.com>

timja requested a review from Wadeck November 4, 2021 07:07

oleg-nenashev approved these changes Nov 6, 2021

View reviewed changes

oleg-nenashev added the hacktoberfest-accepted label Nov 6, 2021

Wadeck approved these changes Nov 8, 2021

View reviewed changes

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 Nov 8, 2021

timja merged commit e8f2ae4 into jenkinsci:master Nov 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hacktoberfest [JENKINS-66730] Un-inlining AsynchPeople/index.jelly #5835

Hacktoberfest [JENKINS-66730] Un-inlining AsynchPeople/index.jelly #5835

P4uline commented Oct 19, 2021 •

edited by Wadeck

Loading

timja Oct 19, 2021

timja Oct 19, 2021

aHenryJard Oct 22, 2021

Wadeck Oct 23, 2021

aHenryJard Oct 22, 2021

Wadeck commented Oct 23, 2021

Wadeck left a comment •

edited

Loading

P4uline commented Oct 30, 2021 •

edited

Loading

Wadeck left a comment

oleg-nenashev left a comment

Wadeck left a comment

timja commented Nov 8, 2021

	String rootURL = currentRequest.getContextPath();

	Functions h = new Functions();
	context.setVariable("h", h);


	// The path starts with a "/" character but does not end with a "/" character.
	context.setVariable("rootURL", rootURL);

Hacktoberfest [JENKINS-66730] Un-inlining AsynchPeople/index.jelly #5835

Hacktoberfest [JENKINS-66730] Un-inlining AsynchPeople/index.jelly #5835

Conversation

P4uline commented Oct 19, 2021 • edited by Wadeck Loading

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

timja Oct 19, 2021

Choose a reason for hiding this comment

timja Oct 19, 2021

Choose a reason for hiding this comment

aHenryJard Oct 22, 2021

Choose a reason for hiding this comment

Wadeck Oct 23, 2021

Choose a reason for hiding this comment

aHenryJard Oct 22, 2021

Choose a reason for hiding this comment

Wadeck commented Oct 23, 2021

Wadeck left a comment • edited Loading

Choose a reason for hiding this comment

P4uline commented Oct 30, 2021 • edited Loading

Wadeck left a comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

Wadeck left a comment

Choose a reason for hiding this comment

timja commented Nov 8, 2021

P4uline commented Oct 19, 2021 •

edited by Wadeck

Loading

Wadeck left a comment •

edited

Loading

P4uline commented Oct 30, 2021 •

edited

Loading