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.

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

Fixes: JENKINS-71794 Redesign build queue | UX #339

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

Conversation

niralmaruda
Copy link

@niralmaruda niralmaruda commented Sep 3, 2023

Fixes: JENKINS-71794
The current build queue layout contains a lot of unnecessary free space. Refer JENKINS-71794 for more details.

The changes are implemented in both PR:core as well as PR:workflow-durable-task-step-plugin to maintain consistency in design. But if any downgraded version of core is used then user will still see the inconsistency in design as shown in second image.

Testing done

image

This is how it'll display if used with downgraded core version.

image

Submitter checklist

Preview Give feedback

@jglick
Copy link
Member

jglick commented Sep 7, 2023

This is how it'll display if used with downgraded core version.

The screenshots confused me initially because they are showing different states (one with two executors busy, one with one busy and one idle). Would be clearer to show a pair of before and after screenshots adjusting only the software version but otherwise displaying identical executor data.

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Mainly depends on whether the core PR is accepted.

<j:if test="${rAuth != null}">
<a href="${rootURL}/${r.url}" class="model-link inside"><l:breakable value="${r.displayName}"/></a>
<st:nbsp/>
<div>
<j:set var="label" value="${it.parent.enclosingLabel}"/>
Copy link
Member

Choose a reason for hiding this comment

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

You are changing the logic for when this part is visible (depending on rAuth), which may introduce a security regression (displaying possibly sensitive information to unauthorized or anonymous users).

Copy link
Author

Choose a reason for hiding this comment

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

@jglick I see, it does make sense to add a check for rAuth. Currently I have added a conditional statement for label so I'll also have to add an conditional statement if rAuth != null as well. That way user will only be able to see label if user is authorized and label is not equal to null. Am I correct?

Copy link
Member

Choose a reason for hiding this comment

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

Something like that. Just ensure that the authorization logic is identical to the previous state.

@jglick
Copy link
Member

jglick commented Sep 12, 2023

jenkinsci/jenkins#8459 (comment) applies here as well.

Added conditional statement to show label only if the user is Authorized and label != null.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants