Skip to content

Commit

Permalink
Remove old Gitlab and Azure Devops summary comments on new decoration
Browse files Browse the repository at this point in the history
The plugin historically left old comments in place but resolved
conversations where comments had become outdated or the underlying issue
had been resolved. However, in Gitlab, the summary comments always
remained visible even when resolved as they were the first comment in
the thread so were not minimised by the Gitlab UI. For a merge request
being scanned multiple times as issues are being fixed, other review
comments responded to, and rebasing activities performed, this can lead
to a number of summary comments being added where the last comment is
typically only the one that developers are about.

As editing comments is not good practice since it's unclear what any
resulting comments in the thread are referring to and Gitlab does not
send emails to notify that comments have changed, the summary comment
is continuing to be posted as a new comment, but the old summary
comments are now being deleted. Where a thread has spawned from an old
summary comment, that comment will not be deleted, but a note added to
notify the users that the summary comment is outdated and the thread can
be resolved once the discussion reaches a conclusion.
  • Loading branch information
mc1arke committed Nov 17, 2024
1 parent eaad89e commit 3ce13fe
Show file tree
Hide file tree
Showing 13 changed files with 1,137 additions and 622 deletions.
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2021 Michael Clarke
* Copyright (C) 2021-2024 Michael Clarke
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
Expand All @@ -20,6 +20,7 @@

import com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.model.CommentThread;
import com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.model.Commit;
import com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.model.ConnectionData;
import com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.model.CreateCommentRequest;
import com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.model.CreateCommentThreadRequest;
import com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.model.GitPullRequestStatus;
Expand All @@ -46,4 +47,9 @@ public interface AzureDevopsClient {
void submitPullRequestStatus(String projectName, String repositoryName, int pullRequestId, GitPullRequestStatus status) throws IOException;

Repository getRepository(String projectName, String repositoryName) throws IOException;

void deletePullRequestThreadComment(String projectName, String repositoryName, int pullRequestId, int threadId, int commentId) throws IOException;

ConnectionData getConnectionData() throws IOException;

}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2021-2023 Michael Clarke
* Copyright (C) 2021-2024 Michael Clarke
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
Expand All @@ -23,6 +23,7 @@
import com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.model.CommentThreadResponse;
import com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.model.Commit;
import com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.model.Commits;
import com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.model.ConnectionData;
import com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.model.CreateCommentRequest;
import com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.model.CreateCommentThreadRequest;
import com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.model.GitPullRequestStatus;
Expand Down Expand Up @@ -104,6 +105,13 @@ public void resolvePullRequestThread(String projectId, String repositoryName, in
execute(url, "patch", objectMapper.writeValueAsString(commentThread), null);
}

@Override
public void deletePullRequestThreadComment(String projectId, String repositoryName, int pullRequestId, int threadId, int commentId) throws IOException {
String url = String.format("%s/%s/_apis/git/repositories/%s/pullRequests/%s/threads/%s/comments/%s?api-version=%s", apiUrl, encode(projectId), encode(repositoryName), pullRequestId, threadId, commentId, API_VERSION);

execute(url, "delete", null, null);
}

@Override
public PullRequest retrievePullRequest(String projectId, String repositoryName, int pullRequestId) throws IOException {
String url = String.format("%s/%s/_apis/git/repositories/%s/pullRequests/%s?api-version=%s", apiUrl, encode(projectId), encode(repositoryName), pullRequestId, API_VERSION);
Expand All @@ -116,6 +124,12 @@ public List<Commit> getPullRequestCommits(String projectId, String repositoryNam
return Objects.requireNonNull(execute(url, "get", null, Commits.class)).getValue();
}

@Override
public ConnectionData getConnectionData() throws IOException {
String url = String.format("%s/_apis/ConnectionData?api-version=%s", apiUrl, API_VERSION_PREVIEW);
return Objects.requireNonNull(execute(url, "get", null, ConnectionData.class));
}


private <T> T execute(String url, String method, String content, Class<T> type) throws IOException {
RequestBuilder requestBuilder = RequestBuilder.create(method)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,21 @@
/*
* Copyright (C) 2020-2024 Michael Clarke
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*
*/
package com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.model;

import com.fasterxml.jackson.annotation.JsonCreator;
Expand All @@ -10,21 +28,24 @@
*/
public class Comment {

private final int id;
private final String content;
private final IdentityRef author;
private final CommentType commentType;

@JsonCreator
public Comment(@JsonProperty("content") String content, @JsonProperty("author") IdentityRef author,
public Comment(@JsonProperty("id") int id, @JsonProperty("content") String content, @JsonProperty("author") IdentityRef author,
@JsonProperty("commentType") CommentType commentType) {
this.id = id;
this.content = content;
this.author = author;
this.commentType = commentType;
}

/**
* The comment content.
*/
public int getId() {
return id;
}

public String getContent() {
return this.content;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*
* Copyright (C) 2024 Michael Clarke
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*
*/
package com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.model;

import com.fasterxml.jackson.annotation.JsonProperty;

public class ConnectionData {

private final Identity authenticatedUser;

public ConnectionData(@JsonProperty("authenticatedUser") Identity authenticatedUser) {
this.authenticatedUser = authenticatedUser;
}

public Identity getAuthenticatedUser() {
return authenticatedUser;
}

public static class Identity {

private final String id;

public Identity(@JsonProperty("id") String id) {
this.id = id;
}

public String getId() {
return id;
}
}

}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2021 Michael Clarke
* Copyright (C) 2021-2024 Michael Clarke
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
Expand Down Expand Up @@ -48,4 +48,6 @@ public interface GitlabClient {
void setMergeRequestPipelineStatus(long projectId, String commitRevision, PipelineStatus status) throws IOException;

Project getProject(String projectSlug) throws IOException;

void deleteMergeRequestDiscussionNote(long projectId, long mergeRequestIid, String discussionId, long noteId) throws IOException;
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.apache.http.HttpResponse;
import org.apache.http.NameValuePair;
import org.apache.http.client.entity.UrlEncodedFormEntity;
import org.apache.http.client.methods.HttpDelete;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.client.methods.HttpPost;
import org.apache.http.client.methods.HttpPut;
Expand Down Expand Up @@ -133,6 +134,14 @@ public void resolveMergeRequestDiscussion(long projectId, long mergeRequestIid,
entity(httpPut, null);
}

@Override
public void deleteMergeRequestDiscussionNote(long projectId, long mergeRequestIid, String discussionId, long noteId) throws IOException {
String discussionIdUrl = String.format("%s/projects/%s/merge_requests/%s/discussions/%s/notes/%s", baseGitlabApiUrl, projectId, mergeRequestIid, discussionId, noteId);

HttpDelete httpDelete = new HttpDelete(discussionIdUrl);
entity(httpDelete, null, x -> validateResponse(x, 204, "Commit discussions note deleted"));
}

@Override
public void setMergeRequestPipelineStatus(long projectId, String commitRevision, PipelineStatus status) throws IOException {
List<NameValuePair> entityFields = new ArrayList<>(Arrays.asList(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2021-2022 Michael Clarke
* Copyright (C) 2021-2024 Michael Clarke
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
Expand Down Expand Up @@ -49,9 +49,13 @@ public abstract class DiscussionAwarePullRequestDecorator<C, P, U, D, N> impleme
private static final String RESOLVED_ISSUE_NEEDING_CLOSED_MESSAGE =
"This issue no longer exists in SonarQube, but due to other comments being present in this discussion, the discussion is not being being closed automatically. " +
"Please manually resolve this discussion once the other comments have been reviewed.";
private static final String RESOLVED_SUMMARY_NEEDING_CLOSED_MESSAGE =
"This summary note is outdated, but due to other comments being present in this discussion, the discussion is not being being removed. " +
"Please manually resolve this discussion once the other comments have been reviewed.";

private static final String VIEW_IN_SONARQUBE_LABEL = "View in SonarQube";
private static final Pattern NOTE_MARKDOWN_VIEW_LINK_PATTERN = Pattern.compile("^\\[" + VIEW_IN_SONARQUBE_LABEL + "]\\((.*?)\\)$");
private static final String DECORATOR_SUMMARY_COMMENT = "decorator-summary-comment";

private final ScmInfoRepository scmInfoRepository;
private final ReportGenerator reportGenerator;
Expand Down Expand Up @@ -137,6 +141,8 @@ protected abstract void submitCommitNoteForIssue(C client, P pullRequest, PostAn

protected abstract void resolveDiscussion(C client, D discussion, P pullRequest);

protected abstract void deleteDiscussion(C client, D discussion, P pullRequest, List<N> notesForDiscussion);

protected abstract void submitSummaryNote(C client, P pullRequest, AnalysisDetails analysis, AnalysisSummary analysisSummary);

protected abstract List<D> getDiscussions(C client, P pullRequest);
Expand Down Expand Up @@ -179,14 +185,21 @@ private List<Triple<D, N, Optional<ProjectIssueIdentifier>>> findOpenSonarqubeCo
return commentsForDiscussion.stream()
.findFirst()
.filter(note -> isNoteFromCurrentUser(note, currentUser))
.filter(note -> !isResolved(client, discussion, commentsForDiscussion, currentUser))
.filter(note -> !isResolved(client, discussion, commentsForDiscussion, currentUser) || isSummaryComment(client, commentsForDiscussion.stream().findFirst().orElse(null)))
.map(note -> new ImmutableTriple<>(discussion, note, parseIssueDetails(client, note)));
})
.filter(Optional::isPresent)
.map(Optional::get)
.collect(Collectors.toList());
}

private boolean isSummaryComment(C client, N note) {
return Optional.of(note)
.flatMap(message -> parseIssueDetails(client, message))
.filter(projectIssueIdentifier -> DECORATOR_SUMMARY_COMMENT.equals(projectIssueIdentifier.getIssueKey()))
.isPresent();
}

private List<String> closeOldDiscussionsAndExtractRemainingKeys(C client, U currentUser,
List<Triple<D, N, Optional<ProjectIssueIdentifier>>> openSonarqubeComments,
List<PostAnalysisIssueVisitor.ComponentIssue> openIssues,
Expand All @@ -205,7 +218,9 @@ private List<String> closeOldDiscussionsAndExtractRemainingKeys(C client, U curr
}

String issueKey = noteIdentifier.get().getIssueKey();
if (!openIssueKeys.contains(issueKey)) {
if (DECORATOR_SUMMARY_COMMENT.equals(issueKey)) {
deleteOrPlaceFinalCommentOnDiscussion(client, currentUser, discussion, pullRequest);
} else if (!openIssueKeys.contains(issueKey)) {
resolveOrPlaceFinalCommentOnDiscussion(client, currentUser, discussion, pullRequest);
} else {
remainingCommentKeys.add(issueKey);
Expand All @@ -218,7 +233,8 @@ private List<String> closeOldDiscussionsAndExtractRemainingKeys(C client, U curr
private boolean isResolved(C client, D discussion, List<N> notesInDiscussion, U currentUser) {
return isClosed(discussion, notesInDiscussion) || notesInDiscussion.stream()
.filter(message -> isNoteFromCurrentUser(message, currentUser))
.anyMatch(message -> RESOLVED_ISSUE_NEEDING_CLOSED_MESSAGE.equals(getNoteContent(client, message)));
.map(message -> getNoteContent(client, message))
.anyMatch(content -> RESOLVED_ISSUE_NEEDING_CLOSED_MESSAGE.equals(content) || RESOLVED_SUMMARY_NEEDING_CLOSED_MESSAGE.equals(content));
}

private void resolveOrPlaceFinalCommentOnDiscussion(C client, U currentUser, D discussion, P pullRequest) {
Expand All @@ -232,12 +248,28 @@ private void resolveOrPlaceFinalCommentOnDiscussion(C client, U currentUser, D d

}

private void deleteOrPlaceFinalCommentOnDiscussion(C client, U currentUser, D discussion, P pullRequest) {
List<N> notesForDiscussion = getNotesForDiscussion(client, discussion);
if (notesForDiscussion.stream()
.filter(this::isUserNote)
.anyMatch(note -> !isNoteFromCurrentUser(note, currentUser))) {
addNoteToDiscussion(client, discussion, pullRequest, RESOLVED_SUMMARY_NEEDING_CLOSED_MESSAGE);
} else {
deleteDiscussion(client, discussion, pullRequest, notesForDiscussion);
}

}

protected Optional<ProjectIssueIdentifier> parseIssueDetails(C client, N note) {
return parseIssueDetails(client, note, VIEW_IN_SONARQUBE_LABEL, NOTE_MARKDOWN_VIEW_LINK_PATTERN);
}

protected Optional<ProjectIssueIdentifier> parseIssueDetails(C client, N note, String label, Pattern pattern) {
try (BufferedReader reader = new BufferedReader(new StringReader(getNoteContent(client, note)))) {
String noteContent = getNoteContent(client, note);
if (noteContent == null) {
return Optional.empty();
}
try (BufferedReader reader = new BufferedReader(new StringReader(noteContent))) {
return reader.lines()
.filter(line -> line.contains(label))
.map(line -> parseIssueLineDetails(line, pattern))
Expand Down Expand Up @@ -274,7 +306,7 @@ private static Optional<ProjectIssueIdentifier> parseIssueIdFromUrl(String issue
String projectId = optionalProjectId.get();

if (url.getPath().endsWith("/dashboard")) {
return Optional.of(new ProjectIssueIdentifier(projectId, "decorator-summary-comment"));
return Optional.of(new ProjectIssueIdentifier(projectId, DECORATOR_SUMMARY_COMMENT));
} else if (url.getPath().endsWith("security_hotspots")) {
return parameters.stream()
.filter(parameter -> "hotspots".equals(parameter.getName()))
Expand Down
Loading

0 comments on commit 3ce13fe

Please sign in to comment.