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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-74025] Extract inline JavaScript from checkboxContent.jelly #374

Merged
merged 1 commit into from
Nov 23, 2024
Merged

[JENKINS-74025] Extract inline JavaScript from checkboxContent.jelly #374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/main/resources/org/biouno/unochoice/common/checkbox-content.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
window.addEventListener("DOMContentLoaded", () => {
document.querySelectorAll(".checkbox-content-data-holder").forEach((dataHolder) => {
const { maxCount, randomName } = dataHolder.dataset;

var height = 0;
var refElement = document.getElementById(`ecp_${randomName}_0`);
if (maxCount > 0 && refElement && refElement.offsetHeight != 0) {
for (var i = 0; i < maxCount; i++) {
height += refElement.offsetHeight + 3;
}
}
else {
height = maxCount * 25.5;
}

height = Math.floor(height);
document.getElementById(`ecp_${randomName}`).style.height = height + "px";
});
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the PRs to update this code pattern on this repo, @yaroslavafenkin !

I was looking at the changes on these PRs, and it occurred to me that if we moved the inline functions to this file as a function, then we could call that in these forEach loop, while also being able to write tests to verify those functions. WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I could look into that.

Copy link
Author

Choose a reason for hiding this comment

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

I'm trying to figure out to approach this.

If I just extract the function like this:

Index: src/main/resources/org/biouno/unochoice/common/checkbox-content.js
<+>UTF-8
===================================================================
diff --git a/src/main/resources/org/biouno/unochoice/common/checkbox-content.js b/src/main/resources/org/biouno/unochoice/common/checkbox-content.js
--- a/src/main/resources/org/biouno/unochoice/common/checkbox-content.js	(revision 91aee0743ce607987513b2823e23dfdbc6da2135)
+++ b/src/main/resources/org/biouno/unochoice/common/checkbox-content.js	(date 1731325784805)
@@ -2,18 +2,22 @@
     document.querySelectorAll(".checkbox-content-data-holder").forEach((dataHolder) => {
         const { maxCount, randomName } = dataHolder.dataset;
 
-        var height = 0;
-        var refElement = document.getElementById(`ecp_${randomName}_0`);
-        if (maxCount > 0 && refElement && refElement.offsetHeight != 0) {
-            for (var i = 0; i < maxCount; i++) {
-                height += refElement.offsetHeight + 3;
-            }
-        }
-        else {
-            height = maxCount * 25.5;
-        }
+        setHeight(maxCount, randomName);
+    });
+});
+
+function setHeight(maxCount, randomName) {
+    var height = 0;
+    var refElement = document.getElementById(`ecp_${randomName}_0`);
+    if (maxCount > 0 && refElement && refElement.offsetHeight != 0) {
+        for (var i = 0; i < maxCount; i++) {
+            height += refElement.offsetHeight + 3;
+        }
+    }
+    else {
+        height = maxCount * 25.5;
+    }
 
-        height = Math.floor(height);
-        document.getElementById(`ecp_${randomName}`).style.height = height + "px";
-    });
-});
+    height = Math.floor(height);
+    document.getElementById(`ecp_${randomName}`).style.height = height + "px";
+}

I won't have access to the setHeight function in *.test.js. I tried to mark it as exported so that I can access it in a unit test, but that makes Java tests fail with

org.htmlunit.corejs.javascript.EvaluatorException: identifier is a reserved word: export (http://localhost:54289/jenkins/adjuncts/a326e762/org/biouno/unochoice/common/checkbox-content.js#9)

and with a lot of entries in the stack trace containing htmlunit.

Another option I thought of is moving the setHeight function to Util.ts. But it's a little different that other functions there. It doesn't return anything, but just modifies the state of a DOM element depending also on some data in another DOM element. Without any further refactoring and given its reliance on DOM can this even be tested?

Copy link
Member

Choose a reason for hiding this comment

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

Hi @yaroslavafenkin

I think it'd be easier to use Jest with an update to its current config that only loads typescript files (or you can write it in TS if you prefer), e.g. after applying your diff (thanks for that!!!):

diff --git a/package.json b/package.json
index 4b962b2..b0f4f2c 100644
--- a/package.json
+++ b/package.json
@@ -67,7 +67,8 @@
             "jest-junit"
         ],
         "testMatch": [
-            "<rootDir>/src/test/js/*.test.ts"
+            "<rootDir>/src/test/js/*.test.ts",
+            "<rootDir>/src/test/js/*.test.js"
         ]
     }
 }
diff --git a/src/main/resources/org/biouno/unochoice/common/checkbox-content.js b/src/main/resources/org/biouno/unochoice/common/checkbox-content.js
index 39d8add..21bb678 100644
--- a/src/main/resources/org/biouno/unochoice/common/checkbox-content.js
+++ b/src/main/resources/org/biouno/unochoice/common/checkbox-content.js
@@ -2,18 +2,22 @@ window.addEventListener("DOMContentLoaded", () => {
     document.querySelectorAll(".checkbox-content-data-holder").forEach((dataHolder) => {
         const { maxCount, randomName } = dataHolder.dataset;
 
-        var height = 0;
-        var refElement = document.getElementById(`ecp_${randomName}_0`);
-        if (maxCount > 0 && refElement && refElement.offsetHeight != 0) {
-            for (var i = 0; i < maxCount; i++) {
-                height += refElement.offsetHeight + 3;
-            }
-        }
-        else {
-            height = maxCount * 25.5;
-        }
-
-        height = Math.floor(height);
-        document.getElementById(`ecp_${randomName}`).style.height = height + "px";
+        setHeight(maxCount, randomName);
     });
 });
+
+function setHeight(maxCount, randomName) {
+    var height = 0;
+    var refElement = document.getElementById(`ecp_${randomName}_0`);
+    if (maxCount > 0 && refElement && refElement.offsetHeight != 0) {
+        for (var i = 0; i < maxCount; i++) {
+            height += refElement.offsetHeight + 3;
+        }
+    }
+    else {
+        height = maxCount * 25.5;
+    }
+
+    height = Math.floor(height);
+    document.getElementById(`ecp_${randomName}`).style.height = height + "px";
+}

Then running yarn run test finds the extra test, and it passes successfully (although the test is not doing anything useful right now). I think we will have to use jsdom do prepare the elements for setHeight to be called, and then test a few scenarios 👍

Let me know if that helps.

Cheers

Copy link
Author

Choose a reason for hiding this comment

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

TS is fine for me to use.
I've created checkbox-content.test.ts in src/test/js. Here's its content (without imports, as those require tweaks depending on the approach taken):

describe('setHeight', () => {
    test('Sets the correct height on the element', () => {
        const randomName: string = "randomNameIndeed";
        const maxCount: number = 13;
        
        document.body.innerHTML = `<body><div id="ecp_${randomName}_0"></div><div id="ecp_${randomName}"></div></body>`;
        const target = document.querySelector(`#ecp_${randomName}`);
        
        expect(target.style.height).not.toBe("331px");
        setHeight(maxCount, randomName);
        expect(target.style.height).toBe("331px");
    });
});

Now there are few paths when it comes to code arrangement:

  1. Leaving setHeight in checkbox-content.js. To be able to call the function in a test I've tried adding the export keyword before the function keyword in checkbox-content.js. Running npm run test results in log telling me setHeight is not defined.
  2. I've tried to move the setHeight to Util.ts. With that I was able to write a test that passed just fine. I went to test whether everything still worked in the Jenkins UI, and it didn't. Util.js isn't included into the page as UnoChoice.js is. I could append Util.js somewhere here, then it would work. Would pollute the page with unneeded object though.
  3. I've tried to move the setHeight to UnoChoice.es6 as that one is exported and included into the page already. I cannot import the setHeight from UnoChoice.es6 in a test though. First of all .es6 isn't a module extension recognized by jest. I was able to get past that by changing jest's config, but then I get the following error:
C:\projects\active-choices-plugin\src\main\resources\org\biouno\unochoice\stapler\unochoice\UnoChoice.es6:24
    import Util from './Util.ts';
    ^^^^^^

    SyntaxError: Cannot use import statement outside a module

      1 | import {describe, test} from '@jest/globals';
    > 2 | import UnoChoice from '../../main/resources/org/biouno/unochoice/stapler/unochoice/UnoChoice.es6';
        | ^
      3 | import expect from "expect";
      4 |
      5 | import jQuery from "jquery";

Copy link
Member

@basil basil Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @kinow, I wanted to let you know that this plugin is now in the top 3 of plugins with CSP violations and unreleased CSP fixes by popularity (along with Blue Ocean and Artifactory). Our contract with Alpha Omega lasts until the end of the year, so ideally we could get this plugin released with CSP fixes in the next few weeks while we still have staffing to work on it. While the risk of regression is low with CSP changes, releasing a few weeks before the end of the year would also give us time to address any regressions in the unlikely case that any are reported. Historically Jenkins activity slows down a lot in the month of December as well.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @basil, the CSP violations are fixed by these inline extraction pull requests? If so, I'd be happy to review & merge them without the extra work to refactor it and write the tests (as these would be tests for existing code, and not for new code). I'd just open a new issue to work on that later.

WDYT?

BTW, @basil, I saw some plugins are using GitHub issues... would you know if it'd be possible for a plugin with JIRA issues to migrate to GitHub issues, please?

Thanks

Copy link
Member

@basil basil Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Releasing the existing PRs and working on refactoring for test coverage in a separate issue sounds great to me. That would enable us to get some soak time for the production change while the project is still being funded.

I believe there was a thread on the developer mailing list about migrating to GitHub issues but I do not remember the current status. I believe that you can enable GitHub issues for this repository in the GitHub UI and set a flag in jenkins-infra/repository-permissions-updater to show a link to GitHub issues on the plugin site. Not sure about migrating existing Jira issues to GitHub issues. (Indeed, a high-fidelity migration covering all existing use cases was my main concern the last time this topic was discussed.)

Copy link
Member

Choose a reason for hiding this comment

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

Releasing the existing PRs and working on refactoring for test coverage in a separate issue sounds great to me. That would enable us to get some soak time for the production change while the project is still being funded.

Roger that, then I can try to release it by the end of this weekend, so that we have it ready for testing over next week (90% sure I can do that, but family/life/$work/etc comes first, as you know).

I believe there was a thread on the developer mailing list about migrating to GitHub issues but I do not remember the current status. I believe that you can enable GitHub issues for this repository in the GitHub UI and set a flag in jenkins-infra/repository-permissions-updater to show a link to GitHub issues on the plugin site.

Some time ago I found a similar thread, I think I know how to enable it on a project.

Not sure about migrating existing Jira issues to GitHub issues. (Indeed, a high-fidelity migration covering all existing use cases was my main concern the last time this topic was discussed.)

Yeah, that's what I couldn't find before. I think the... GitLab plugin was the first plugin I saw using GitHub issues. But that's lower priority than releasing it. I will review & merge it, open the new issue in JIRA, do some triaging on the issues to see if there's anything else simple that could be added, and then cut the release. While others test it, then, I will see if I can work on moving this and maybe tap-plugin both to GitHub issues.

Cheers

});
30 changes: 4 additions & 26 deletions src/main/resources/org/biouno/unochoice/common/checkboxContent.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
<?jelly escape-by-default='true' ?>
<j:jelly xmlns:j="jelly:core"
xmlns:d="jelly:define"
xmlns:transitionWrapper="transitionWrapper">
xmlns:transitionWrapper="transitionWrapper"
xmlns:st="jelly:stapler">
<d:taglib uri="transitionWrapper">
<d:tag name="randomTableIdBlock">
<j:choose>
Expand Down Expand Up @@ -101,29 +102,6 @@
</j:forEach>
</transitionWrapper:randomTableIdBlock>
</div>
<script type="text/javascript">
<![CDATA[
(function() {
var f = function() {
var height = 0;
var maxCount = ${it.getVisibleItemCount()};

var refElement = document.getElementById("ecp_${h.escape(it.randomName)}_0");
if(maxCount > 0 && refElement && refElement.offsetHeight !=0) {
for(var i=0; i< maxCount; i++) {
height += refElement.offsetHeight + 3;
}
}
else {
height = maxCount * 25.5;
}

height = Math.floor(height);
document.getElementById("ecp_${h.escape(it.randomName)}").style.height = height + "px";
};

f();
})();
]]>
</script>
<span class="checkbox-content-data-holder" data-max-count="${it.getVisibleItemCount()}" data-random-name="${h.escape(it.randomName)}" />
<st:adjunct includes="org.biouno.unochoice.common.checkbox-content" />
</j:jelly>