UI renovation #506

mawinter69 · 2024-02-24T00:09:25Z

Adjust all UI elements to use styling consistent with current Jenkins
This affects all tables, input fields, checkboxes, icons, buttons, ...
Follow guidelines from the design library and remove any backlinks in the sidebar
Use ionicons where possible
Only use Jenkins defined color names to ensure compatibility with dark theme

remove blockwrapper for compatibility with old Jenkins

remove some validation methods for integer values that can be covered by using f:number instead of f:textbox

reimplement the gerrit-server table with modern javascript (removes the old YUI related things). This requires to disable the jslint4jvava (10 year old version) as this doesn't understand modern js (e.g. no template literals, params with default values, const and let)

extract inline style definitions to separate css

remove inline javascript (onClick attribute of td) by using Behaviour.specify
checkurl without inline js

ScreenShots After

Search: ![image](https://github.com/jenkinsci/gerrit-trigger-plugin/assets/17767050/2a163e21-067f-4354-911c-847188d3f7fc) ![image](https://github.com/jenkinsci/gerrit-trigger-plugin/assets/17767050/965d1b09-6ac3-400d-968d-b0eee0c19414)

Gerrit Management:

Gerrit Server:

Diagnostics:

Project Trigger configuration:

Testing done

Manual testing of UI

### Submitter checklist
- [x] Make sure you are opening from a **topic/feature/bugfix branch** (right side) and not your main branch!
- [x] Ensure that the pull request title represents the desired changelog entry
- [x] 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

Adjust all UI elements to use styling consistent with current Jenkins This affects all tables, input fields, checkboxes, icons, buttons, ... Follow guidelines from the design library and remove any backlinks in the sidebar Use ionicons where possible only use Jenkins defined color names to ensure compatibility with dark theme remove blockwrapper for compatibility with old Jenkins remove some validation methods for integer values that can be covered by using f:number instead of f:textbox reimplement the gerrit-server table with modern javascript (removes the old YUI related things). This requires to disable the jslint4jvava (10 year old version) as this doesn't understand modern js (e.g. no template literals, params with default values) extract inline style definitions to separate css remove inline javascript (onClick attribute of td) by using Behaviour.specify

avoid undefined when not connected readd skipVote name htmlunit doesn't understand default values for js methods jelly escape by default

panicking · 2024-02-24T09:07:46Z

@mawinter69 Really nice. I found out two codestyle violation on the patcheset that I have fixed locally

panicking · 2024-02-24T09:40:46Z

@mawinter69 the UI let me lost my server configuratoin that does not appear anymore

panicking · 2024-02-24T10:06:56Z

Please apply those fix

diff --git a/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/diagnostics/Diagnostics.java b/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/diagnostics/Diagnostics.java
index 00bc61b0..9363699f 100644
--- a/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/diagnostics/Diagnostics.java
+++ b/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/diagnostics/Diagnostics.java
@@ -88,8 +88,9 @@ public class Diagnostics implements ModelObjectWithChildren, ModelObjectWithCont
                          .withIconClass("symbol-clipboard-outline plugin-ionicons-api")
                          .withDisplayName(Messages.EventListenersReport_DisplayName()));
         if (isDebugMode()) {
-            MenuItem item = new MenuItem().withUrl("triggerDebugEvent").withIconClass("symbol-warning plugin-ionicons-api")
-                .withDisplayName("Trigger Debug");
+            MenuItem item = new MenuItem()
+                                    .withUrl("triggerDebugEvent").withIconClass("symbol-warning plugin-ionicons-api")
+                                    .withDisplayName("Trigger Debug");
             item.requiresConfirmation = true;
             menu.add(item);
         }
diff --git a/src/main/webapp/js/server-table.js b/src/main/webapp/js/server-table.js
index ff90c209..1a69dd42 100644
--- a/src/main/webapp/js/server-table.js
+++ b/src/main/webapp/js/server-table.js
@@ -30,9 +30,9 @@ function refreshServerTable() {
         return icons.content.querySelector(`#${name}`).cloneNode(true);
     }
 
-    function createLink(url, icon, tagName, callback, disabled) {
+    function createLink(url, icon, tagName, callback, disable) {
         tagName = tagName == null ? 'a' : tagName;
-        disabled = disable == null ? false : disabled;
+        disabled = disable == null ? false : disable;
         const td = document.createElement("td");
         td.classList.add("gt-table--center");
 
diff --git a/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/actions/manual/ManualTriggerActionApprovalTest.java b/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/actions/manual/ManualTriggerActionApprovalTest.java
index 1c0ba7e7..8470f0af 100644
--- a/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/actions/manual/ManualTriggerActionApprovalTest.java
+++ b/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/actions/manual/ManualTriggerActionApprovalTest.java
@@ -26,7 +26,6 @@ package com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.actions.man
 
 import org.htmlunit.html.DomNode;
 import org.htmlunit.html.HtmlForm;
-import org.htmlunit.html.HtmlImage;
 import org.htmlunit.html.HtmlPage;
 import org.htmlunit.html.HtmlSpan;
 import org.htmlunit.html.HtmlTable;

panicking · 2024-02-24T11:25:45Z

If the server name contains special char like & then it does not get show as should be. I found this fix but I don't think that is the best way

+    function unEntity(str){
+        return str.replace(/&amp;/g, "&").replace(/&lt;/g, "<").replace(/&gt;/g, ">");
+    }
+
     function createTableRow(server, tbody) {
         const row = document.createElement("tr");
         row.setAttribute("data-server-url", server.serverUrl);
@@ -86,7 +90,7 @@ function refreshServerTable() {
             a.href = server.frontEndUrl;
             a.target = "_blank";
             a.classList.add("gt-external-link");
-            a.appendChild(document.createTextNode(server.name));
+            a.appendChild(document.createTextNode(unEntity(server.name)));

mawinter69 · 2024-02-24T12:51:50Z

Thanks for the feedback

If the server name contains special char like & then it does not get show as should be. I found this fix but I don't think that is the best way

Fixed it in the correct way by using innerHTML

panicking · 2024-02-24T13:03:47Z

Thanks for the feedback

If the server name contains special char like & then it does not get show as should be. I found this fix but I don't think that is the best way

Fixed it in the correct way by using innerHTML

Is possible for you to squash the latest 3 commits and clean the commit message. I mean if you have some fixes on not merged patches they can go back to the initial patch

panicking · 2024-02-24T13:07:08Z

:+1 I have deployed it and it works as far I can test

panicking · 2024-02-24T16:14:02Z

pom.xml

@@ -304,7 +304,6 @@
                <artifactId>maven-surefire-plugin</artifactId>
                <configuration>
                    <reuseForks>false</reuseForks>
-                    <forkCount>${concurrency}</forkCount>
                </configuration>


Is this required?

it prevented that the forkCount can be set by the ci and that you can set it via command line with -DforkCount ro othat the forkCount setting from line 68 was effective

panicking · 2024-02-24T16:14:26Z

...s/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritCause/description.jelly

                    <j:choose>
-                        <j:when test="${other.hasBuild()}">


Can you point out, where this change impact?

when another build is referenced but that build has already been deleted the image was missing

panicking · 2024-02-24T16:14:36Z

...n/hudson/plugins/gerrit/trigger/hudsontrigger/actions/manual/ManualTriggerAction/index.jelly

@@ -3,7 +3,7 @@
    <l:layout title="${%Gerrit Manual trigger}" norefresh="true" permission="${it.requiredPermission}">
        <l:header>
            <script src="${rootURL}${it.getJsUrl('gerrit-search.js')}" type="text/javascript"/>
-          <link rel="stylesheet" href="${rootURL}/plugin/gerrit-trigger/css/gerrit.css" type="text/css"/>
+            <link rel="stylesheet" href="${rootURL}/plugin/gerrit-trigger/css/gerrit.css" type="text/css"/>
        </l:header>


Is this relevant?

it caused problems with using dark theme. Also to be conform with CSP, there should be no inline style tags

panicking · 2024-02-24T16:15:16Z

src/main/webapp/js/server-table.js

@@ -86,7 +86,7 @@ function refreshServerTable() {
            a.href = server.frontEndUrl;
            a.target = "_blank";
            a.classList.add("gt-external-link");
-            a.appendChild(document.createTextNode(server.name));
+            a.innerHTML = server.name;
            a.appendChild(generateSVGIcon("gerrit-symbol-link"));


Any security risk for server name?

No, It was using innerHTML before, the server.name is already escaped

rsandell · 2024-03-08T14:45:03Z

...in/resources/com/sonyericsson/hudson/plugins/gerrit/trigger/GerritManagement/newServer.jelly

@@ -28,12 +28,6 @@ THE SOFTWARE.
 <?jelly escape-by-default='true'?>
 <j:jelly xmlns:j="jelly:core" xmlns:l="/lib/layout">
  <l:layout norefresh="true" permission="${app.ADMINISTER}" title="${%Add New Server}">
-    <l:side-panel>


Why remove the side panel? It's nothing important, just curious.

Following the the design library recommendations (https://weekly.ci.jenkins.io/design-library/Layouts/) hierarchical links should be avoided

rsandell · 2024-03-08T14:47:48Z

src/main/resources/com/sonyericsson/hudson/plugins/gerrit/trigger/GerritServer/index.jelly

                    <f:entry title="${%Started}"
                             help="/plugin/gerrit-trigger/help-GerritBuildStartedVerified.html">
                        <f:textbox name="gerritBuildStartedVerifiedValue"
                                   value="${it.config.gerritBuildStartedVerifiedValue}"
-                                   checkUrl="'${rootURL}/${serverURL}/emptyOrIntegerCheck?value='+escape(this.value)"/>
+                                   clazz="number"/>


does this allow empty values?

rsandell · 2024-03-08T14:49:12Z

src/main/resources/com/sonyericsson/hudson/plugins/gerrit/trigger/GerritServer/index.jelly

+                             help="/plugin/gerrit-trigger/help-GerritVerifiedCmdBuildSuccessful.html">
+                        <f:textarea name="gerritVerifiedCmdBuildSuccessful"
+                                    value="${it.config.gerritCmdBuildSuccessful}"
+                                    default="gerrit review &lt;CHANGE&gt;,&lt;PATCHSET&gt; --message 'Build Successful &lt;BUILDS_STATS&gt;' --verified &lt;VERIFIED&gt; --code-review &lt;CODE_REVIEW&gt;"/>


I just merged a PR that added a parameter here to the defaults, so I guess there will be a merge conflict that needs to be resolved.

Or maybe I didn't 🤔

rsandell

Thank you so much for doing this! You are awesome!

And sorry for not reviewing until now.

mawinter69 force-pushed the ui-refresh branch from 06eaf49 to 4e9dee7 Compare February 24, 2024 00:33

mawinter69 added 3 commits February 24, 2024 02:48

fixes

78f787b

avoid undefined when not connected readd skipVote name htmlunit doesn't understand default values for js methods jelly escape by default

test fix

9128f42

pass as string to avoid quoting

d6d0986

mawinter69 force-pushed the ui-refresh branch from b506c41 to d6d0986 Compare February 24, 2024 02:34

fix bugs and checkstyle

b8df274

panicking suggested changes Feb 24, 2024

View reviewed changes

panicking reviewed Feb 24, 2024

View reviewed changes

use div for cause description

2a60fea

mawinter69 force-pushed the ui-refresh branch from 7d85955 to 2a60fea Compare February 24, 2024 17:28

rsandell added the ui label Mar 8, 2024

rsandell reviewed Mar 8, 2024

View reviewed changes

rsandell approved these changes Mar 8, 2024

View reviewed changes

rsandell and others added 3 commits March 8, 2024 15:56

Merge branch 'master' into ui-refresh

0fb2141

Merge branch 'master' into ui-refresh

4387bb8

increase timeout

c77cd6d

rsandell approved these changes Mar 11, 2024

View reviewed changes

rsandell merged commit 633df2b into jenkinsci:master Mar 11, 2024

UI renovation #506

UI renovation #506

Uh oh!

Conversation

mawinter69 commented Feb 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing done

Uh oh!

panicking commented Feb 24, 2024

Uh oh!

panicking commented Feb 24, 2024

Uh oh!

panicking commented Feb 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

panicking commented Feb 24, 2024

Uh oh!

mawinter69 commented Feb 24, 2024

Uh oh!

panicking commented Feb 24, 2024

Uh oh!

panicking commented Feb 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsandell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mawinter69 commented Feb 24, 2024 •

edited

Loading

panicking commented Feb 24, 2024 •

edited

Loading