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

[JENKINS-62448] Enhance information displayed in approval page #300

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

[JENKINS-62448] Enhance information displayed in approval page #300

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
cbfedb6
[JENKINS-62448] Enhance information displayed in approval page
Wadeck May 25, 2020
b7a3312
Update src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/Scr…
Wadeck May 29, 2020
4c583a4
Update src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/Scr…
Wadeck May 29, 2020
349ac04
Update src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/met…
Wadeck May 29, 2020
c559fe8
Update src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/met…
Wadeck May 29, 2020
1d39eff
Update src/main/resources/org/jenkinsci/plugins/scriptsecurity/script…
Wadeck May 29, 2020
de369d6
Update src/main/resources/org/jenkinsci/plugins/scriptsecurity/script…
Wadeck May 29, 2020
e4708f7
Update src/main/resources/org/jenkinsci/plugins/scriptsecurity/script…
Wadeck May 29, 2020
380deda
Update src/main/resources/org/jenkinsci/plugins/scriptsecurity/script…
Wadeck May 29, 2020
79b1995
Update src/main/resources/org/jenkinsci/plugins/scriptsecurity/script…
Wadeck May 29, 2020
0ea0c93
Update src/main/resources/org/jenkinsci/plugins/scriptsecurity/script…
Wadeck May 29, 2020
e978559
Update src/main/resources/org/jenkinsci/plugins/scriptsecurity/script…
Wadeck May 29, 2020
eadf75f
Apply suggestions from code review
Wadeck May 29, 2020
9bcceb4
Various updates after reviews
Wadeck Aug 18, 2020
1c5493f
Merge branch 'master' into UX_revamp
Wadeck Aug 18, 2020
3ee25d8
Adjust wording with Josh's proposal
Wadeck Aug 18, 2020
d934b69
Merge remote-tracking branch 'origin/UX_revamp' into UX_revamp
Wadeck Aug 18, 2020
0b0b624
Adding migration tests
Wadeck Aug 19, 2020
fa17df8
Adjusting existing tests
Wadeck Aug 19, 2020
8f55c06
Adjusting existing tests
Wadeck Aug 20, 2020
65926ce
Adjust grammar
Wadeck Aug 20, 2020
9dc032c
Adjust another wording
Wadeck Aug 20, 2020
18bcb49
Merge remote-tracking branch 'origin/UX_revamp' into UX_revamp
Wadeck Aug 20, 2020
fcb41b7
Adjustment after review
Wadeck Sep 2, 2020
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
4 changes: 2 additions & 2 deletions src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@ ScriptApproval.tab.fullScriptPending=Scripts - pending approval
ScriptApproval.tab.fullScriptApproved=Scripts - approved
ScriptApproval.tab.signaturePending=Signatures - pending approval
ScriptApproval.tab.signatureApproved=Signatures - approved
ScriptApproval.tab.classPathPending=Class paths - pending approval
ScriptApproval.tab.classPathApproved=Class paths - approved
ScriptApproval.tab.classPathPending=Classpaths - pending approval
ScriptApproval.tab.classPathApproved=Classpaths - approved
8 changes: 0 additions & 8 deletions ...main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/_resources.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,6 @@
background-color: #6c757d;
}

.script-approval-page .custom-pane-frame-padding {
padding: 8px 12px;

/* border part stolen from pane-frame */
border: solid 1px #f0f0f0;
border-radius: 4px;
}

.script-approval-page h2 ~ h2 {
margin-top: 35px;
}
Expand Down
3 changes: 0 additions & 3 deletions ...in/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/index.properties
View file
Open in desktop

This file was deleted.

4 changes: 3 additions & 1 deletion ...org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/classPath_approved.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@
}
});
block.insert(deleteButton);
block.insert("<code title='" + e.hash + "'>" + e.path + "</code>");
var code = new Element('code', { 'title': e.hash });
code.textContent = e.path;
block.insert(code);
$('approvedClasspathEntries').insert(block);
});
$('approvedClasspathEntries').show();
Expand Down
4 changes: 3 additions & 1 deletion .../org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/classPath_pending.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@
});
block.insert(approveButton);
block.insert(denyButton);
block.insert("<code title='" + e.hash + "'>" + e.path + "</code>");
var code = new Element('code', { 'title': e.hash });
code.textContent = e.path;
block.insert(code);

$('pendingClasspathEntries').insert(block);
});
Expand Down
376 changes: 182 additions & 194 deletions ...rg/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_approved.jelly
View file
Open in desktop

Large diffs are not rendered by default.

14 changes: 5 additions & 9 deletions ...nkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_approved.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,13 @@
approvedScripts=Approved scripts

# approved scripts
metadataGatheringDisabled=The metadata gathering was disabled. \
metadataGatheringDisabled=Metadata gathering disabled. \
<p>You can re-enable it permanently by removing the system property: <br />\
<code>org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval.metadataGathering</code> </p>\
<p>or temporarily (until restart) by running this in the Script Console: <br />\
<code>org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval.METADATA_GATHERING = true</code></p>

## table headers
approvedHash=Hash
approvedHash_tooltip=The hash could be seen as a signature of the script. It's used as an identifier to reference the content of the script.
approvedExpandCode=Code
approvedLanguage=Language
approvedScriptLength=Length
approvedScriptLength_tooltip=Length of the script in characters
usageCount=# of uses
usageCount_tooltip=Uses since the introduction of metadata
Wadeck marked this conversation as resolved.
Show resolved Hide resolved
lastTimeUsed=Date of last use
Expand All @@ -25,7 +19,8 @@ lastKnownApproverLogin=Last approver

## table content
noRequesterSinceMetadata=N/A
noRequesterSinceMetadata_tooltip=The requester was not recorded. It could be due to the request being created before the metadata introduction or the code calling this script did not provide user information.
noRequesterSinceMetadata_tooltip=The requester was not recorded. \
It could be due to the request being created before the metadata introduction or the code calling this script did not provide user information.
Wadeck marked this conversation as resolved.
Show resolved Hide resolved
noRequestApprovalTimeSinceMetadata=N/A
noRequestApprovalTimeSinceMetadata_tooltip=It was requested before the introduction of metadata.
noContextItemSinceMetadata=N/A
Expand All @@ -50,7 +45,8 @@ noKnownApproverSinceMetadata=N/A
noKnownApproverSinceMetadata_tooltip=It was not approved since the introduction of metadata.
Copy link
Member

Choose a reason for hiding this comment

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

When are these shown?

Copy link

Choose a reason for hiding this comment

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

Fwiw while I suggest changing a has been elsewhere to was, these should be has not been.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a script is approved before the metadata introduction, and then is used. We do not have the information on who approved it.


revokeApproval=Revoke all selected
Wadeck marked this conversation as resolved.
Show resolved Hide resolved
revokeApproval_tooltip=Revoking an approved script can have an impact on the jobs using it. The next time an attempt to execute the script is made, a new approval will be required.
revokeApproval_tooltip=Revoking an approved script can have an impact on the jobs using it. \
The next time an attempt to execute the script is made, a new approval will be required.
revokeApproval_confirm=Really delete all selected approvals? Any existing scripts will need to be requeued and reapproved.
Copy link

Choose a reason for hiding this comment

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

Suggested change
revokeApproval_confirm=Really delete all selected approvals? Any existing scripts will need to be requeued and reapproved.
revokeApproval_confirm=Really remove all selected approvals? Any existing scripts will need to be requeued and reapproved.

I'm not a fan of selected approvals -- in general, I'd prefer approval of selected ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my mind we are listing in the table the different approvals, but saying it's the list of scripts, that are already approved, make actually more sense.

I will keep aside the delete/revoke/remove as it seems we don't have a consensus yet.

Wadeck marked this conversation as resolved.
Show resolved Hide resolved
approvedNoSelectedItemError=Please select at least one line.

Expand Down
207 changes: 98 additions & 109 deletions ...org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_pending.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,117 +1,106 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form" xmlns:i="jelly:fmt">
<div class="custom-pane-frame-padding">
<div id="pending-approval">
<j:set var="pendingScripts" value="${it.getPendingScriptsSorted()}" />

<h2>
${%pendingScripts}
</h2>
<div id="pending-approval">
<j:choose>
<j:when test="${pendingScripts.isEmpty()}">
<p class="no-pending-script-approvals">
<i>${%noPendingScripts}</i>
</p>
</j:when>
<j:otherwise>
<table class="pane bigtable">
<j:choose>
<j:when test="${pendingScripts.isEmpty()}">
<p class="no-pending-script-approvals">
<i>${%noPendingScripts}</i>
</p>
</j:when>
<j:otherwise>
<table class="pane bigtable">
<tr>
<th width="1%"><!-- checkbox --></th>
<th>${%pendingExpandCode}</th>
<th>${%pendingLanguage}</th>
<th>${%contextUser}</th>
<th>${%contextItem}</th>
<th title="${%requestApprovalTime_tooltip}" class="info-cursor">${%requestApprovalTime}</th>
</tr>
<j:forEach var="ps" items="${pendingScripts}">
<tr class="js-selectable-row">
<td>
<input type="checkbox" value="" name="pending-hash"
data-hash="${ps.hash}"
id="check-pending-${ps.hash}"
class="js-checkbox-hash js-bypass-click" />
</td>
<td>
<div class="toggle-parent" data-expand-target-id="js-${ps.hash}-row" data-expand-codemirror="true">
<span class="js-toggle-show-icon js-bypass-click action-link" title="${%pendingExpand_tooltip}">${%pendingExpand}</span>
<span class="js-toggle-hide-icon js-bypass-click hidden-element action-link" title="${%pendingMinimize_tooltip}">${%pendingMinimize}</span>
</div>
</td>
<td>${ps.language.displayName}</td>
<td>
<j:set var="user" value="${ps.context.user}" />
<j:choose>
<j:when test="${user != null}">
<a href="${rootURL}/user/${user}" class="model-link">${user}</a>
</j:when>
<j:otherwise>
<span title="${%noRequesterSinceMetadata_tooltip}" class="info-cursor">
${%noRequesterSinceMetadata}
</span>
</j:otherwise>
</j:choose>
</td>
<td>
<j:set var="contextItem" value="${ps.context.item}" />
<j:choose>
<j:when test="${contextItem != null}">
<a href="${rootURL}/${contextItem.url}" class="model-link">${contextItem.fullDisplayName}</a>
</j:when>
<j:otherwise>
<span title="${%noContextItemSinceMetadata_tooltip}" class="info-cursor">
${%noContextItemSinceMetadata}
</span>
</j:otherwise>
</j:choose>
</td>
<td>
<j:set var="approvalRequestTime" value="${ps.approvalRequestTimeDate}" />
<j:choose>
<j:when test="${approvalRequestTime != null}">
<i:formatDate value="${approvalRequestTime}" type="both" dateStyle="medium" timeStyle="medium"/>
</j:when>
<j:otherwise>
<span title="${%noRequestApprovalTimeSinceMetadata_tooltip}" class="info-cursor">
${%noRequestApprovalTimeSinceMetadata}
</span>
</j:otherwise>
</j:choose>
</td>
</tr>
<tr>
<th width="1%"><!-- checkbox --></th>
<th tooltip="${%pendingHash_tooltip}" class="info-cursor">${%pendingHash}</th>
<th>${%pendingExpandCode}</th>
<th>${%pendingLanguage}</th>
<th title="${%pendingScriptLength_tooltip}" class="info-cursor">${%pendingScriptLength}</th>
<th>${%contextUser}</th>
<th>${%contextItem}</th>
<th title="${%requestApprovalTime_tooltip}" class="info-cursor">${%requestApprovalTime}</th>
<td colspan="6" id="js-${ps.hash}-row" class="js-async-hidden-element">
<f:textarea readonly="readonly" codemirror-mode="${ps.language.codeMirrorMode}" codemirror-config='"readOnly": true' rows="10" cols="80" value="${ps.script}"/>
</td>
</tr>
<j:forEach var="ps" items="${pendingScripts}">
<tr class="js-selectable-row">
<td>
<input type="checkbox" value="" name="pending-hash"
data-hash="${ps.hash}"
id="check-pending-${ps.hash}"
class="js-checkbox-hash js-bypass-click" />
</td>
<td class="hash">
<label for="check-pending-${ps.hash}" title="${ps.hash}" class="js-bypass-click">${ps.hash.substring(0, 8)}</label>
</td>
<td>
<div class="toggle-parent" data-expand-target-id="js-${ps.hash}-row" data-expand-codemirror="true">
<span class="js-toggle-show-icon js-bypass-click action-link" title="${%pendingExpand_tooltip}">${%pendingExpand}</span>
<span class="js-toggle-hide-icon js-bypass-click hidden-element action-link" title="${%pendingMinimize_tooltip}">${%pendingMinimize}</span>
</div>
</td>
<td>${ps.language.displayName}</td>
<td>${ps.script.size()}</td>
<td>
<j:set var="user" value="${ps.context.user}" />
<j:choose>
<j:when test="${user != null}">
<a href="${rootURL}/user/${user}" class="model-link">${user}</a>
</j:when>
<j:otherwise>
<span title="${%noRequesterSinceMetadata_tooltip}" class="info-cursor">
${%noRequesterSinceMetadata}
</span>
</j:otherwise>
</j:choose>
</td>
<td>
<j:set var="contextItem" value="${ps.context.item}" />
<j:choose>
<j:when test="${contextItem != null}">
<a href="${rootURL}/${contextItem.url}" class="model-link">${contextItem.fullDisplayName}</a>
</j:when>
<j:otherwise>
<span title="${%noContextItemSinceMetadata_tooltip}" class="info-cursor">
${%noContextItemSinceMetadata}
</span>
</j:otherwise>
</j:choose>
</td>
<td>
<j:set var="approvalRequestTime" value="${ps.approvalRequestTimeDate}" />
<j:choose>
<j:when test="${approvalRequestTime != null}">
<i:formatDate value="${approvalRequestTime}" type="both" dateStyle="medium" timeStyle="medium"/>
</j:when>
<j:otherwise>
<span title="${%noRequestApprovalTimeSinceMetadata_tooltip}" class="info-cursor">
${%noRequestApprovalTimeSinceMetadata}
</span>
</j:otherwise>
</j:choose>
</td>
</tr>
<tr>
<td colspan="8" id="js-${ps.hash}-row" class="js-async-hidden-element">
<f:textarea readonly="readonly" codemirror-mode="${ps.language.codeMirrorMode}" codemirror-config='"readOnly": true' rows="10" cols="80" value="${ps.script}"/>
</td>
</tr>
</j:forEach>
</table>

<p class="js-error-no-selected hidden-element error">
${%pendingNoSelectedItemError}
</p>

<div class="action-panel">
<span class="yui-button">
<button class="js-button-pending-approve-all" title="${%approvePending_tooltip}"
data-container-id="pending-approval"
data-action-confirm-message="${%approvePending_confirm}"
data-action-url="${rootURL}/scriptApproval/approvePendingScripts">${%approvePending}</button>
</span>
<span class="yui-button">
<button class="js-button-pending-deny-all" title="${%denyPending_confirm}"
data-container-id="pending-approval"
data-action-confirm-message="${%denyPending_confirm}"
data-action-url="${rootURL}/scriptApproval/approvePendingScripts">${%denyPending}</button>
</span>
</div>
</j:otherwise>
</j:choose>
</div>
</j:forEach>
</table>

<p class="js-error-no-selected hidden-element error">
${%pendingNoSelectedItemError}
</p>

<div class="action-panel">
<span class="yui-button">
<button class="js-button-pending-approve-all" title="${%approvePending_tooltip}"
data-container-id="pending-approval"
data-action-confirm-message="${%approvePending_confirm}"
data-action-url="${rootURL}/scriptApproval/approvePendingScripts">${%approvePending}</button>
</span>
<span class="yui-button">
<button class="js-button-pending-deny-all" title="${%denyPending_confirm}"
data-container-id="pending-approval"
data-action-confirm-message="${%denyPending_confirm}"
data-action-url="${rootURL}/scriptApproval/approvePendingScripts">${%denyPending}</button>
</span>
</div>
</j:otherwise>
</j:choose>
</div>
</j:jelly>
9 changes: 2 additions & 7 deletions ...enkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_pending.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,16 @@
pendingScripts=Scripts pending approval

# pending scripts
## table headers
pendingHash=Hash
pendingHash_tooltip=The hash could be seen as a signature of the script. It's used as an identifier to reference the content of the script.
pendingExpandCode=Code
pendingLanguage=Language
pendingScriptLength=Length
pendingScriptLength_tooltip=Length of the script in characters
contextUser=Requester
contextItem=Context
requestApprovalTime=Request date
requestApprovalTime_tooltip=Inform about when the approval request was made.

## table content
noRequesterSinceMetadata=N/A
noRequesterSinceMetadata_tooltip=The requester was not recorded. It could be due to the request being created before the metadata introduction or the code calling this script did not provide user information.
noRequesterSinceMetadata_tooltip=The requester was not recorded. \
It could be due to the request being created before the metadata introduction or the code calling this script did not provide user information.
Wadeck marked this conversation as resolved.
Show resolved Hide resolved
noRequestApprovalTimeSinceMetadata=N/A
noRequestApprovalTimeSinceMetadata_tooltip=It was requested before the introduction of metadata.
noContextItemSinceMetadata=N/A
Expand Down
2 changes: 1 addition & 1 deletion ...org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/signature_approved.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler">
<div class="custom-pane-frame-padding">
<div class="pane-frame">
<div>Signatures already approved:</div>
Wadeck marked this conversation as resolved.
Show resolved Hide resolved
<textarea readonly="readonly" id="approvedSignatures" rows="10" cols="80">
<j:forEach var="line" items="${it.approvedSignatures}">${line}<st:out value="&#10;"/></j:forEach>
Expand Down
2 changes: 1 addition & 1 deletion .../org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/signature_pending.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler">
<div class="custom-pane-frame-padding">
<div class="pane-frame">
<j:choose>
<j:when test="${it.pendingSignatures.isEmpty()}">
<p>
Expand Down