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-60082] Show pull request title in name column #476

Merged
merged 10 commits into from
Oct 25, 2024
Merged

[JENKINS-60082] Show pull request title in name column #476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0ef8d9b
Show pull request title
janfaracik Aug 16, 2024
13cedc4
Update column.jelly
janfaracik Aug 16, 2024
a37b5d3
Update ItemColumn.java
janfaracik Aug 16, 2024
c3620a4
Don't wrap PR IDs
janfaracik Aug 17, 2024
9de361d
Update MultiBranchProject.java
janfaracik Aug 28, 2024
315ce8d
Another take on displaying titles
janfaracik Aug 28, 2024
02109d5
Some progress
timja Oct 13, 2024
8718f94
Fix tests
timja Oct 13, 2024
c6198ed
Discard changes to src/test/java/integration/MigrationTest.java
timja Oct 13, 2024
4a3ebec
Merge branch 'master' into show-pr-title
janfaracik Oct 25, 2024
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
25 changes: 0 additions & 25 deletions src/main/java/jenkins/branch/ItemColumn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,31 +100,6 @@ public boolean isOrphaned(Object item) {
return false;
}

/**
* Gets the tool-tip title of a job.
*
* @param job the job.
* @return the tool-tip title unescaped for use in an attribute.
*/
@SuppressWarnings("unused") // used via Jelly EL binding
public String getTitle(Object job) {
Copy link
Member

Choose a reason for hiding this comment

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

Removing an old public method signature.

// Jelly will take care of quote and ampersand escaping for us
if (job instanceof Actionable) {
Actionable actionable = (Actionable) job;
ObjectMetadataAction action = actionable.getAction(ObjectMetadataAction.class);
if (action != null) {
String displayName = action.getObjectDisplayName();
if (StringUtils.isBlank(displayName) || displayName.equals(actionable.getDisplayName())) {
// if the display name is the same, then the description is more useful
String description = action.getObjectDescription();
return description != null ? description : displayName;
}
return displayName;
}
}
return null;
}

/**
* Our extension.
*/
Expand Down
23 changes: 9 additions & 14 deletions src/main/java/jenkins/branch/MultiBranchProject.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2104,22 +2104,16 @@
throw new IllegalStateException(
"Name of created project " + project + " did not match expected " + encodedName);
}
// HACK ALERT
// ==========
// We don't want to trigger a save, so we will do some trickery to ensure that observer.created(project)
// performs the save
BulkChange bc = new BulkChange(project);
try {
project.setDisplayName(getProjectDisplayName(project, rawName));
} catch (IOException e) {
// ignore even if it does happen we didn't want a save
} finally {
bc.abort();
}
// decorate contract is to ensure it does not trigger a save
_factory.decorate(project);
// ok it is now up to the observer to ensure it does the actual save.
observer.created(project);
try (BulkChange bc = new BulkChange(project);) {
observer.created(project);
project.setDisplayName(getProjectDisplayName(project, rawName));
bc.commit();
} catch (IOException e) {

Check warning on line 2114 in src/main/java/jenkins/branch/MultiBranchProject.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line 

Line 2114 is not covered by tests
// Ignored
}
Comment on lines +2110 to +2116
Copy link
Member

@timja timja Oct 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this change fixed the tests. the observer.created is what creates the RunMap that was null before. (noticed by walking back through a stacktrace of a successful one to see where it was created).

I'm not 100% sure on if there's any impact here and why it was so keen to avoid saving / why a bulk change couldn't be used around everything...

doAutomaticBuilds(head, revision, rawName, project, revisionActions, null, null);
}

Expand All @@ -2137,8 +2131,9 @@
.orElse(null);
}

// Default to displaying the project's display name if a trait hasn't been provided
if (naming == null) {
return rawName;
naming = MultiBranchProjectDisplayNamingStrategy.RAW_AND_OBJECT_DISPLAY_NAME;
timja marked this conversation as resolved.
Show resolved Hide resolved
}

final ObjectMetadataAction action = naming.needsObjectDisplayName()
Expand Down
9 changes: 8 additions & 1 deletion src/main/java/jenkins/branch/MultiBranchProjectDisplayNamingStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,14 @@
return rawName;
}

return format("%s - %s", rawName, displayName);
// The raw name provided here in the context of pull requests is the pull request ID
// We tidy up the ID so that they display consistently between SCMs
String cleanedUpBranchName = rawName;
if (cleanedUpBranchName.startsWith("MR-") || cleanedUpBranchName.startsWith("PR-")) {

Check warning on line 66 in src/main/java/jenkins/branch/MultiBranchProjectDisplayNamingStrategy.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 66 is only partially covered, 2 branches are missing
Copy link
Member

Choose a reason for hiding this comment

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

may need a regex that it's PR-\d+ so its less likely to hit an actual branch name

Copy link
Member

Choose a reason for hiding this comment

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

a branch name won't be hit here because the rawName and displayName would be the same so the above isn't a concern.

Copy link
Member

Choose a reason for hiding this comment

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

this would appear to be putting SCM specifics into an SCM agnostic piece of code?
I believe other systems exist and Gerrit uses C-xxxx

Copy link
Member

Choose a reason for hiding this comment

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

Is this a blocker, and if so would this mean an API likely needs to be created where the SCMs set their prefix?

Copy link
Member

@jtnord jtnord Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So initially I was not thinking it was a blocker, but something that should be addressed (in the future as a follow up)

But now I am thinking is this just confusing? we use #nnn for build numbers in Jenkins and do I really want or need consistency across SCMs? Indeed having PR-123 in one job and MR-245 in another is a reminder that one project is in my GitHub.com account and the other is in GitLab on premise.

in other words could a user expect that by clicking that link below they get taken to build #7 of the job?
image

Copy link
Member

Choose a reason for hiding this comment

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

I think in context with more than one pull request it will be really clear and not going to get confusing at all, e.g.:

image

I don't think most users will care where the SCM is when looking at a job.

cleanedUpBranchName = "#" + cleanedUpBranchName.substring(3);

Check warning on line 67 in src/main/java/jenkins/branch/MultiBranchProjectDisplayNamingStrategy.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line 

Line 67 is not covered by tests
}

return format("%s (%s)", displayName, cleanedUpBranchName);
}
},
;
Expand Down
22 changes: 12 additions & 10 deletions src/main/resources/jenkins/branch/ItemColumn/column.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,26 +25,28 @@ THE SOFTWARE.
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout"
xmlns:t="/lib/hudson" xmlns:f="/lib/form" xmlns:i="jelly:fmt">
<td style="${indenter.getCss(job)}">
<d:taglib uri="local">
<d:tag name="link">
<a href="${jobBaseUrl}${job.shortUrl}" class='model-link jenkins-table__link'>
Copy link
Member

Choose a reason for hiding this comment

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

extracts the same code into one place and makes minor changes to the classes

Copy link
Member

Choose a reason for hiding this comment

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

Why inside removal though? It's present in https://github.com/jenkinsci/jenkins/blob/e1f99468fd81d6aa52dc434fefc6e0420e1cb449/core/src/main/resources/hudson/views/JobColumn/column.jelly#L29

<l:breakable value="${h.getRelativeDisplayNameFrom(job, itemGroup)}"/>
</a>
</d:tag>
</d:taglib>

<td style="${indenter.getCss(job)}" xmlns:local="local">
<j:choose>
<j:when test="${it.isOrphaned(job)}">
<s>
<a href="${jobBaseUrl}${job.shortUrl}" class='model-link inside jenkins-table_link' title="${it.getTitle(job)}">
Copy link
Member

Choose a reason for hiding this comment

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

@janfaracik Why did you remove the inside class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't do anything from what I've seen from core.

Copy link
Member

@daniel-beck daniel-beck Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It controlled whether the context menu dropdown chevron was considered part of the area of the model-link. If it doesn't do anything anymore, that's an oversight likely in jenkinsci/jenkins#6084.

2.332.x inside:
Screenshot 2024-10-29 at 13 23 29

2.332.x otherwise (ugly but this hover effect was fairly short-lived):
Screenshot 2024-10-29 at 13 23 22

<l:breakable value="${h.getRelativeDisplayNameFrom(job, itemGroup)}"/>
</a>
<local:link />
</s>
</j:when>
<j:when test="${it.isPrimary(job)}">
<strong>
<a href="${jobBaseUrl}${job.shortUrl}" class='model-link inside jenkins-table_link' title="${it.getTitle(job)}">
<l:breakable value="${h.getRelativeDisplayNameFrom(job, itemGroup)}"/>
</a>
<local:link />
</strong>
</j:when>
<j:otherwise>
<a href="${jobBaseUrl}${job.shortUrl}" class='model-link inside jenkins-table_link' title="${it.getTitle(job)}">
<l:breakable value="${h.getRelativeDisplayNameFrom(job, itemGroup)}"/>
</a>
<local:link />
</j:otherwise>
</j:choose>
</td>
Expand Down
14 changes: 7 additions & 7 deletions src/test/java/integration/CategorizationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ public void given_multibranch_when_changeRequestsWanted_then_onlyUncategorizedAn
assertThat(prj.getItems(),
containsInAnyOrder(
hasProperty("displayName", is("master")),
hasProperty("displayName", is("CR-" + crNum))
hasProperty("displayName", is("Change request #1 (CR-1)"))
)
);
assertThat(prj.getViews(),
Expand Down Expand Up @@ -204,7 +204,7 @@ public void given_multibranch_when_noBranchesWanted_then_uncategorizedViewPresen
assertThat(prj.getItems(),
containsInAnyOrder(
hasProperty("displayName", is("master-1.0")),
hasProperty("displayName", is("CR-" + crNum))
hasProperty("displayName", is("Change request #1 (CR-1)"))
)
);
assertThat(prj.getViews(),
Expand All @@ -222,7 +222,7 @@ public void given_multibranch_when_noBranchesWanted_then_uncategorizedViewPresen
allOf(
instanceOf(MultiBranchProjectViewHolder.ViewImpl.class),
hasProperty("viewName", is(ChangeRequestSCMHeadCategory.DEFAULT.getName())),
hasProperty("items", contains(hasProperty("displayName", is("CR-" + crNum))))
hasProperty("items", contains(hasProperty("displayName", is("Change request #1 (CR-1)"))))
)
));
}
Expand Down Expand Up @@ -253,8 +253,8 @@ public void given_multibranch_when_wantsEverything_then_hasEverything()
hasProperty("displayName", is("feature")),
hasProperty("displayName", is("master-1.0")),
hasProperty("displayName", is("master-1.1")),
hasProperty("displayName", is("CR-" + crNum1)),
hasProperty("displayName", is("CR-" + crNum2))
hasProperty("displayName", is("Change request #1 (CR-1)")),
hasProperty("displayName", is("Change request #2 (CR-2)"))
)
);
assertThat(prj.getViews(),
Expand All @@ -279,8 +279,8 @@ public void given_multibranch_when_wantsEverything_then_hasEverything()
instanceOf(MultiBranchProjectViewHolder.ViewImpl.class),
hasProperty("viewName", is(ChangeRequestSCMHeadCategory.DEFAULT.getName())),
hasProperty("items", containsInAnyOrder(
hasProperty("displayName", is("CR-" + crNum2)),
hasProperty("displayName", is("CR-" + crNum1))
hasProperty("displayName", is("Change request #1 (CR-1)")),
hasProperty("displayName", is("Change request #2 (CR-2)"))
))
)
));
Expand Down