[JENKINS-69917] - Snippetizer/index.jelly javascript un-inlined. #599

Artmorse · 2022-10-25T07:51:04Z

The issue
You can find all the issue details here.

My updates
I've moved the onclick call from the Snippetizer/index.jelly file to the handle-prototype.js one.

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

jglick

Needs to be tested with snippet generator running in various contexts (root, job, multibranch project) and with different values for the Jenkins context path.

jglick · 2022-10-25T12:56:39Z

src/main/resources/org/jenkinsci/plugins/workflow/cps/Snippetizer/handle-prototype.js

+    if (!json) {
+        return; // just a separator
+    }
+    const url = '/jenkins/pipeline-syntax/generateSnippet' // ${rootURL}/${it.GENERATE_URL}


This will not work if the context path is set to anything other than /jenkins.

I don't see how I can use/pass template variables (${rootURL}) in the Javascript file...

I do not often work in JS, but there are various ways; IIRC the preferred technique is to define data attributes on HTML elements and look those up from JS. Search core sources for examples.

jglick · 2022-10-25T12:57:58Z

src/main/resources/org/jenkinsci/plugins/workflow/cps/Snippetizer/index.jelly

@@ -67,27 +67,10 @@ THE SOFTWARE.
                    <f:dropdownListBlock title="${%— Advanced/Deprecated —}"/>
                    <local:listSteps quasiDescriptors="${it.getQuasiDescriptors(true)}"/>
                </f:dropdownList>
-                <j:set var="id" value="${h.generateId()}"/>


Loses uniquification of the textarea. OTOH

workflow-cps-plugin/src/main/resources/org/jenkinsci/plugins/workflow/cps/Snippetizer/index.jelly

Line 78 in 6467f4f

var json = Object.toJSON(JSON.parse(document.forms.config.elements.json.value).prototype);

already worked only with the singleton

workflow-cps-plugin/src/main/resources/org/jenkinsci/plugins/workflow/cps/Snippetizer/index.jelly

Line 31 in 6467f4f

<f:form name="config">

so I guess it does not matter.

jglick · 2023-01-09T16:06:00Z

@Artmorse do you plan to fix this or should it be closed?

Artmorse · 2023-01-10T17:53:01Z

Yes, I'll find some time to have a look to it this week!

Artmorse · 2023-01-16T10:23:22Z

Needs to be tested with snippet generator running in various contexts (root, job, multibranch project) and with different values for the Jenkins context path.

Can you give me more explanation about how to test it ?
I've only some information in the JENKINS-69917.

jglick · 2023-01-17T18:14:43Z

The page is accessible from e.g.

/pipeline-syntax/ (Jenkins root, e.g. https://ci.jenkins.io/pipeline-syntax/)
that URL appended to a multibranch folder (do not have a publicly accessible live example for you)
that URL appended to a job (including one branch project under a multibranch folder)

And you need to check when the context root is empty (like in ci.jenkins.io) as well as when it has a value (like /jenkins prefix used when you mvn hpi:run this plugin).

Artmorse · 2023-01-18T15:10:26Z

I've some issues with mvn hpi:run so it's not tested yet.

jglick

Looks right. Tested successfully?

jglick · 2023-01-18T15:11:28Z

plugin/src/main/resources/org/jenkinsci/plugins/workflow/cps/Snippetizer/handle-prototype.js

+        method: 'POST',
+        parameters: {json: json},
+        onSuccess: function(r) {
+            document.getElementById('prototypeText').value = r.responseText;


also like #599 (comment)

plugin/src/main/resources/org/jenkinsci/plugins/workflow/cps/Snippetizer/index.jelly

plugin/src/main/resources/org/jenkinsci/plugins/workflow/cps/Snippetizer/handle-prototype.js

Co-authored-by: Jesse Glick <jglick@cloudbees.com>

jglick · 2023-10-30T18:05:42Z

@Artmorse do you plan to resolve the merge conflict with #702 and retest?

Artmorse · 2023-10-30T18:07:31Z

@jglick thanks for pinging me, I'll look at it this week!

Artmorse · 2023-11-06T13:12:00Z

plugin/src/main/resources/org/jenkinsci/plugins/workflow/cps/Snippetizer/index.jelly

+                    <input type="button" id="generatePipelineScript" value="${%Generate Pipeline Script}"
+                           class="submit-button primary"
+                           data-url="${rootURL}/${it.GENERATE_URL}"
+                           data-crumb="${h.getCrumb(request)}"/>


I've added the jenkins crumb to avoid 403.
That's the easiest way, I found, to pass it to the javascript script.

Could also just add a CrumbExclusion for

workflow-cps-plugin/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/Snippetizer.java

Line 487 in 4e2b31c

public HttpResponse doGenerateSnippet(StaplerRequest req, @QueryParameter String json) throws Exception {

since it takes POST only to handle larger quantities of input but it is in fact read-only so not a CSRF threat. Anyway, this is the straightforward translation of the original.

jglick

I do not think I am qualified to review this. Someone else?

plugin/src/main/resources/org/jenkinsci/plugins/workflow/cps/Snippetizer/handle-prototype.js

jglick

Seems to work.

jglick · 2023-12-07T13:21:46Z

plugin/src/main/resources/org/jenkinsci/plugins/workflow/cps/Snippetizer/index.jelly

+                    <input type="button" id="generatePipelineScript" value="${%Generate Pipeline Script}"
+                           class="submit-button primary"
+                           data-url="${rootURL}/${it.GENERATE_URL}"
+                           data-crumb="${h.getCrumb(request)}"/>


Could also just add a CrumbExclusion for

workflow-cps-plugin/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/Snippetizer.java

Line 487 in 4e2b31c

public HttpResponse doGenerateSnippet(StaplerRequest req, @QueryParameter String json) throws Exception {

since it takes POST only to handle larger quantities of input but it is in fact read-only so not a CSRF threat. Anyway, this is the straightforward translation of the original.

jglick · 2023-12-07T13:25:06Z

plugin/src/main/resources/org/jenkinsci/plugins/workflow/cps/Snippetizer/handle-prototype.js

+document.addEventListener('DOMContentLoaded', () => {
+
+    const generatePipelineScript = document.getElementById("generatePipelineScript");
+    const url = generatePipelineScript.getAttribute("data-url");
+    const crumb = generatePipelineScript.getAttribute("data-crumb");
+    generatePipelineScript.onclick = (_) => {


Using a class and Behaviour.specify is more conventional I think, but this should be fine as well.

jglick · 2023-12-07T13:33:20Z

plugin/src/main/resources/org/jenkinsci/plugins/workflow/cps/Snippetizer/index.jelly

@@ -67,36 +67,13 @@ THE SOFTWARE.
                    <f:dropdownListBlock title="${%— Advanced/Deprecated —}"/>
                    <local:listSteps quasiDescriptors="${it.getQuasiDescriptors(true)}"/>
                </f:dropdownList>
-                <j:set var="id" value="${h.generateId()}"/>


Note that removing generateId and reverting to a hard-coded id loses the ability for this code to exist in multiple places in a page. Not an issue in this case since it exists only in one place in a top-level page. The general approach is to use behaviors and look up elements via relative DOM path.

[JENKINS-69917] - Snippetizer/index.jelly javascript un-inlined.

56178e3

Artmorse requested a review from a team as a code owner October 25, 2022 07:51

jglick requested changes Oct 25, 2022

View reviewed changes

Merge branch 'master' into JENKINS-69917

55ab0c3

[JENKINS-69917] - Attribute used to pass url from jelly to JS.

5b059d2

jglick added the bug label Jan 18, 2023

jglick approved these changes Jan 18, 2023

View reviewed changes

[JENKINS-69917] - Apply suggestions from code review

d1cbe58

Co-authored-by: Jesse Glick <jglick@cloudbees.com>

jglick requested a review from a team January 18, 2023 16:19

mawinter69 mentioned this pull request Mar 1, 2023

[JENKINS-70695] add copy to clipboard button for snippet generator #666

Merged

6 tasks

Artmorse added 2 commits November 6, 2023 14:05

Merge branch 'master' into JENKINS-69917

7011ddc

[JENKINS-69917] - handle-prototype fixed.

caa7934

Artmorse commented Nov 6, 2023

View reviewed changes

Artmorse requested a review from jglick November 6, 2023 13:13

jglick reviewed Nov 15, 2023

View reviewed changes

plugin/src/main/resources/org/jenkinsci/plugins/workflow/cps/Snippetizer/handle-prototype.js Outdated Show resolved Hide resolved

Artmorse added 2 commits December 7, 2023 10:57

Merge branch 'master' into JENKINS-69917

48cd576

[JENKINS-69917] - handle-prototype use fetch.

196f96c

jglick approved these changes Dec 7, 2023

View reviewed changes

jglick reviewed Dec 7, 2023

View reviewed changes

jglick merged commit 9d3888c into jenkinsci:master Dec 7, 2023
14 checks passed

jglick mentioned this pull request Feb 27, 2024

Fix snippet generator when a custom crumb header is configured #861

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-69917] - Snippetizer/index.jelly javascript un-inlined. #599

[JENKINS-69917] - Snippetizer/index.jelly javascript un-inlined. #599

Artmorse commented Oct 25, 2022

jglick left a comment

jglick Oct 25, 2022

Artmorse Jan 16, 2023

jglick Jan 17, 2023

jglick Oct 25, 2022

jglick commented Jan 9, 2023

Artmorse commented Jan 10, 2023

Artmorse commented Jan 16, 2023

jglick commented Jan 17, 2023

Artmorse commented Jan 18, 2023

jglick left a comment

jglick Jan 18, 2023

jglick commented Oct 30, 2023

Artmorse commented Oct 30, 2023

Artmorse Nov 6, 2023

jglick Dec 7, 2023

jglick left a comment

jglick left a comment

jglick Dec 7, 2023

jglick Dec 7, 2023

jglick Dec 7, 2023

[JENKINS-69917] - Snippetizer/index.jelly javascript un-inlined. #599

[JENKINS-69917] - Snippetizer/index.jelly javascript un-inlined. #599

Conversation

Artmorse commented Oct 25, 2022

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Jan 9, 2023

Artmorse commented Jan 10, 2023

Artmorse commented Jan 16, 2023

jglick commented Jan 17, 2023

Artmorse commented Jan 18, 2023

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Oct 30, 2023

Artmorse commented Oct 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment