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

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
Prev Previous commit
Next Next commit
Adjust another wording
  • Loading branch information
@Wadeck
Wadeck committed Aug 20, 2020
commit 9dc032c51e9dbc145e8b7a3bee3a256e9afe9701
6 changes: 3 additions & 3 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
Expand Up @@ -28,7 +28,7 @@ noContextItemSinceMetadata_tooltip=The context item was not provided by the code

emptyMetadata=There is no metadata for this approval
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
emptyMetadata=There is no metadata for this approval
emptyMetadata=This script has been approved before Script Security 1.75 and no additional metadata has been recorded yet. Once this script is executed, additional metadata will be added.

Copy link

Choose a reason for hiding this comment

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

Suggested change
emptyMetadata=There is no metadata for this approval
emptyMetadata=This script was approved before Script Security 1.75 and no additional metadata has been recorded yet. Once this script is executed, additional metadata will be added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both proposals seem to pollute the space due to the size of the sentence.

Proof

After

Screenshot_2020-09-02_083609_001

Before (with the icon for tooltip discoverability)

Screenshot_2020-09-02_083719_001

Before (with just the cursor: help)

Screenshot_2020-09-02_084251_001

Going with no modification. This could be improved in a follow up PR.

emptyMetadata_tooltip=If the script is used after the metadata introduction, some metadata will be displayed here. \
Copy link
Member

Choose a reason for hiding this comment

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

This tooltip lacks discoverability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried with a help icon but it will require to have newer version of core to have the latest help icon, so keeping it simple for the moment with cursor: help

Cursor

Screenshot_2020-09-02_084251_001

It means that an unused approval will not have metadata and could be reasonably revoked after a certain period of time.
It means that an unused approval will not have metadata and could be reasonably removed after a certain period of time.
noScriptCodeSinceMetadata=N/A
noScriptCodeSinceMetadata_tooltip=The approval was given while the metadata was not gathered.
approvedContextUser=Requester
Expand All @@ -44,8 +44,8 @@ notApprovedSinceMetadata_tooltip=It was not approved since the introduction of m
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
revokeApproval_tooltip=Revoking an approved script can have an impact on the jobs using it. \
revokeApproval=Remove selected
revokeApproval_tooltip=Remove 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