[JENKINS-71909]: Revert https://github.com/jenkinsci/active-choices-p… #298

kinow · 2024-07-06T09:41:45Z

…lugin/pull/79

The merge request mentions JENKINS-71365 (prototype.JS is removed from Jenkins core) but reverting this should not impact that.

What we are doing is removing the async calls and promises, and instead using the synchronous code again. This is so that Active Choices parameters are rendering in order as they were before.

Whilst rendering them asynchronously seemed like a good idea to provide a more responsive UI, in reality we oversaw an important aspect: users write their scripts relying on the order parameters are loaded.

Users wrote their code using document.getElementId, for instance, which worked previously as users would put the parameter doing that after the parameter from which the ID was being used.

There were several issues rendered, so this is not likely to be added back again any time soon. In order to have a more smart reactivity, we would need places to hook user callback code (like Vue or React do), but provided by Jenkins UI, or in a way that could take some time to create in the plug-in without breaking things, and that works well.

Testing done

Submitter checklist

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

kinow · 2024-07-06T09:47:38Z

Looks like more UiAcceptanceTest changes will have to be reverted. But haven't tested it the OP bug is fixed yet. Issue with the case to replicate it: https://issues.jenkins.io/browse/JENKINS-71909

kinow · 2024-08-03T23:38:08Z

Setting up new laptop; old one had temperature issues -- will update it over next days (more likely near next weekend).

asc3ns10n · 2024-08-14T18:03:23Z

Hi @kinow,

Thanks for working on getting this fixed. We are being affected by this issue and having to do several workarounds to keep 2.6.5 running in our environment. I tried testing the plugin and ran into a few issues which I've mentioned here. I was able to get the plugin working and confirmed that it did resolve our issues and the example in JENKINS-71909.

kinow · 2024-08-14T20:30:40Z

Hi @asc3ns10n did you write a workaround on top of this change (which is pending a js/prototype issue), or did you come up with another solution for the time being?

src/main/resources/org/biouno/unochoice/stapler/unochoice/UnoChoice.es6

asc3ns10n · 2024-08-14T18:03:36Z

src/main/resources/org/biouno/unochoice/stapler/unochoice/UnoChoice.es6

+                let a = [];
+                for (let i=0; i<args.length-(callback!=null?1:0); i++)
+                    a.push(args[i]);
+                if(window.jQuery3 === window.$) { //Is jQuery the active framework?


With the dependencies on Prototype/jQuery having been removed from Stapler in favor of using native JavaScript, should the same be done here and just use XMLHttpRequest?

Stapler moved to fetch, so let's try to use that. Not sure if making the proxy async and then await'ing the fetch will work here, but I think it's worth a try. It should behave the same as XMLHttpRequest + async: false that we had before 🤞

I am testing this in our environment and seeing the same issues as 2.8.3. It still seems to be loading the params out of order in some of our jobs.

Thanks for testing this, @asc3ns10n

src/main/resources/org/biouno/unochoice/stapler/unochoice/UnoChoice.es6

kinow · 2024-08-18T13:40:27Z

Tested a job with more parameters that was failing with this pull request, and after @asc3ns10n 's suggestions it rendered everything correctly.

Found some issues in tests (silly things, mostly cosmetics), and one bug in radiobutton.jelly. But there are still tests failing that appear to be either/both due to the change here, or to Selenium update/API. Will continue investigating another day.

kinow · 2024-08-18T19:12:25Z

FWIW, one of the errors from the tests run on Jenkins:

Enclosed exception:
org.htmlunit.corejs.javascript.EvaluatorException: missing formal parameter (http://localhost:43587/jenkins/adjuncts/222c6f97/org/biouno/unochoice/stapler/unochoice/UnoChoice.js#1)

Looks related to this one, HtmlUnit/htmlunit#232, but there may be some solution if the error is happening after adding the new code for stapler... maybe just rewrite it so htmlunit is happy....

kinow · 2024-08-30T09:15:37Z

(rebased)

kinow · 2024-08-18T18:08:08Z

src/main/resources/org/biouno/unochoice/common/radioContent.jelly

@@ -113,7 +113,7 @@
              maxCount = ${it.visibleItemCount};
          }

-          var refElement = document.getElementById("${id}");
+          var refElement = document.getElementById("ecp_${h.escape(it.randomName)}_0");


This var doesn't exit, resulting in a warning during runtime (browser console) and no element found.

kinow · 2024-08-18T18:09:31Z

src/main/resources/org/biouno/unochoice/DynamicReferenceParameter/config.jelly

    <f:textbox name="parameter.referencedParameters" value="${instance.referencedParameters}" />
  </f:entry>
  <f:advanced>
-    <f:entry title="${%Omit value field}" field="omitValueField" help="${rootURL}/../plugin/uno-choice/help-omitValueField.html">
+    <f:entry title="${%Omit value field}" field="omitValueField" help="/plugin/uno-choice/help-omitValueField.html">


A e2e test was failing on the IDE (didn't check if it was failing on Jenkins). There seems to be a new convention where parameters like omitValueField automatically load if there is a file sitting next to the Jelly file with name omitValueField-help.html, I think. But instead of trying that I just used another recent example (gitlab-plugin) as reference to update these.

kinow · 2024-08-31T20:59:48Z

src/main/resources/org/biouno/unochoice/stapler/unochoice/UnoChoice.es6

+                let a = [];
+                for (let i=0; i<args.length-(callback!=null?1:0); i++)
+                    a.push(args[i]);
+                if(window.jQuery3 === window.$) { //Is jQuery the active framework?


Thanks for testing this, @asc3ns10n

kinow · 2024-09-28T19:34:30Z

FWIW, one of the errors from the tests run on Jenkins:

Enclosed exception:
org.htmlunit.corejs.javascript.EvaluatorException: missing formal parameter (http://localhost:43587/jenkins/adjuncts/222c6f97/org/biouno/unochoice/stapler/unochoice/UnoChoice.js#1)

Looks related to this one, HtmlUnit/htmlunit#232, but there may be some solution if the error is happening after adding the new code for stapler... maybe just rewrite it so htmlunit is happy....

Refs:

kinow · 2024-09-28T19:41:53Z

All tests passed on my local environment with the latest commit. If GH Actions build pass too, then the next step is to try to reproduce the bug again with this latest change. If I cannot reproduce, then will try with master branch one more time, confirming I can reproduce with latest version, and if everything goes as expected, then it should be ready to be merged and released. Now let's wait...

kinow · 2024-09-28T20:03:03Z

Couldn't reproduce the issue with the latest code. Could someone else try it, please? Maybe @asc3ns10n?

When I have some time to volunteer here again, I will,

try to add a unit test to avoid a regression on this, using the test case from the issue linked
squash commits
add changelog
merge (if no one reports that the error is still happening)
release

👋

kinow · 2024-10-06T15:37:23Z

Doing the easy steps first, so commits all squashed, with a changelog included. Just pending a unit test now, then it's ready to be released 🎉

kinow · 2024-10-06T20:50:12Z

Test written, used the config.xml shared in the issue, and then wrote the test that replicates what OP tried to achieve without success (but the test passes as the fix is hopefully correct 🤞 😬 ). If GH Actions passes, then we will be ready to start this week with a new version of the plug-in.

The merge request mentions JENKINS-71365 (prototype.JS is removed from Jenkins core) but reverting this should not impact that. What we are doing is removing the async calls and promises, and instead using the synchronous code again. This is so that Active Choices parameters are rendering in order as they were before. Whilst rendering them asynchronously seemed like a good idea to provide a more responsive UI, in reality we oversaw an important aspect: users write their scripts relying on the order parameters are loaded. Users wrote their code using document.getElementId, for instance, which worked previously as users would put the parameter doing that after the parameter from which the ID was being used. There were several issues rendered, so this is not likely to be added back again any time soon. In order to have a more smart reactivity, we would need places to hook user callback code (like Vue or React do), but provided by Jenkins UI, or in a way that could take some time to create in the plug-in without breaking things, and that works well.

kinow · 2024-10-06T21:10:49Z

Tests failing on Windows, but appears to be something with the CI node.

kinow requested a review from a team as a code owner July 6, 2024 09:41

kinow force-pushed the JENKINS-71909 branch from 77620ca to 70e0ffa Compare July 27, 2024 22:28

asc3ns10n reviewed Aug 14, 2024

View reviewed changes

kinow force-pushed the JENKINS-71909 branch 2 times, most recently from f965476 to 4618620 Compare August 18, 2024 12:24

kinow force-pushed the JENKINS-71909 branch from 789e799 to 4ec716a Compare August 30, 2024 09:15

kinow commented Aug 31, 2024

View reviewed changes

kinow force-pushed the JENKINS-71909 branch from 4ec716a to 7f8c60e Compare September 28, 2024 16:17

kinow force-pushed the JENKINS-71909 branch from d87768b to 2e57cd2 Compare October 6, 2024 15:35

kinow force-pushed the JENKINS-71909 branch from 2e57cd2 to 21d0496 Compare October 6, 2024 20:48

kinow force-pushed the JENKINS-71909 branch from 21d0496 to 8f2899f Compare October 6, 2024 20:59

kinow merged commit 3e30ea2 into master Oct 6, 2024
13 of 16 checks passed

kinow deleted the JENKINS-71909 branch October 6, 2024 21:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-71909]: Revert https://github.com/jenkinsci/active-choices-p… #298

[JENKINS-71909]: Revert https://github.com/jenkinsci/active-choices-p… #298

kinow commented Jul 6, 2024

kinow commented Jul 6, 2024

kinow commented Aug 3, 2024

asc3ns10n commented Aug 14, 2024

kinow commented Aug 14, 2024

asc3ns10n Aug 14, 2024

kinow Aug 18, 2024

asc3ns10n Aug 19, 2024

kinow Aug 31, 2024

kinow commented Aug 18, 2024

kinow commented Aug 18, 2024

kinow commented Aug 30, 2024

kinow Aug 18, 2024

kinow Aug 18, 2024

kinow Aug 31, 2024

kinow commented Sep 28, 2024

kinow commented Sep 28, 2024

kinow commented Sep 28, 2024 •

edited

Loading

kinow commented Oct 6, 2024

kinow commented Oct 6, 2024

kinow commented Oct 6, 2024

[JENKINS-71909]: Revert https://github.com/jenkinsci/active-choices-p… #298

[JENKINS-71909]: Revert https://github.com/jenkinsci/active-choices-p… #298

Conversation

kinow commented Jul 6, 2024

Testing done

Submitter checklist

kinow commented Jul 6, 2024

kinow commented Aug 3, 2024

asc3ns10n commented Aug 14, 2024

kinow commented Aug 14, 2024

asc3ns10n Aug 14, 2024

Choose a reason for hiding this comment

kinow Aug 18, 2024

Choose a reason for hiding this comment

asc3ns10n Aug 19, 2024

Choose a reason for hiding this comment

kinow Aug 31, 2024

Choose a reason for hiding this comment

kinow commented Aug 18, 2024

kinow commented Aug 18, 2024

kinow commented Aug 30, 2024

kinow Aug 18, 2024

Choose a reason for hiding this comment

kinow Aug 18, 2024

Choose a reason for hiding this comment

kinow Aug 31, 2024

Choose a reason for hiding this comment

kinow commented Sep 28, 2024

kinow commented Sep 28, 2024

kinow commented Sep 28, 2024 • edited Loading

kinow commented Oct 6, 2024

kinow commented Oct 6, 2024

kinow commented Oct 6, 2024

kinow commented Sep 28, 2024 •

edited

Loading