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 16, 2024
1 parent fd5d52e commit 6fbf8f4
Show file tree
Hide file tree
Showing 13 changed files with 1,194 additions and 662 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);
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 @@ -205,7 +211,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 +226,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,6 +241,18 @@ 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);
}
Expand Down Expand Up @@ -274,7 +295,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
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2020-2023 Markus Heberling, Michael Clarke
* Copyright (C) 2020-2024 Markus Heberling, 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 @@ -18,6 +18,25 @@
*/
package com.github.mc1arke.sonarqube.plugin.ce.pullrequest.azuredevops;

import java.io.IOException;
import java.net.URI;
import java.net.URISyntaxException;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

import org.apache.commons.lang.StringUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.sonar.api.ce.posttask.QualityGate;
import org.sonar.ce.task.projectanalysis.scm.ScmInfoRepository;
import org.sonar.db.alm.setting.ALM;
import org.sonar.db.alm.setting.AlmSettingDto;
import org.sonar.db.alm.setting.ProjectAlmSettingDto;
import org.sonar.db.protobuf.DbIssues;

import com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.AzureDevopsClient;
import com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.AzureDevopsClientFactory;
import com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.model.Comment;
Expand All @@ -41,26 +60,8 @@
import com.github.mc1arke.sonarqube.plugin.ce.pullrequest.report.AnalysisIssueSummary;
import com.github.mc1arke.sonarqube.plugin.ce.pullrequest.report.AnalysisSummary;
import com.github.mc1arke.sonarqube.plugin.ce.pullrequest.report.ReportGenerator;
import org.apache.commons.lang.StringUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.sonar.api.ce.posttask.QualityGate;
import org.sonar.ce.task.projectanalysis.scm.ScmInfoRepository;
import org.sonar.db.alm.setting.ALM;
import org.sonar.db.alm.setting.AlmSettingDto;
import org.sonar.db.alm.setting.ProjectAlmSettingDto;
import org.sonar.db.protobuf.DbIssues;

import java.io.IOException;
import java.net.URI;
import java.net.URISyntaxException;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

public class AzureDevOpsPullRequestDecorator extends DiscussionAwarePullRequestDecorator<AzureDevopsClient, PullRequest, Void, CommentThread, Comment> implements PullRequestBuildStatusDecorator {
public class AzureDevOpsPullRequestDecorator extends DiscussionAwarePullRequestDecorator<AzureDevopsClient, PullRequest, String, CommentThread, Comment> implements PullRequestBuildStatusDecorator {

private static final Logger logger = LoggerFactory.getLogger(AzureDevOpsPullRequestDecorator.class);
private static final Pattern NOTE_MARKDOWN_LEGACY_SEE_LINK_PATTERN = Pattern.compile("^\\[See in SonarQube]\\((.*?)\\)$");
Expand Down Expand Up @@ -115,8 +116,14 @@ protected PullRequest getPullRequest(AzureDevopsClient client, AlmSettingDto alm
}

@Override
protected Void getCurrentUser(AzureDevopsClient client) {
return null;
protected String getCurrentUser(AzureDevopsClient client) {
try {
return client.getConnectionData().getAuthenticatedUser().getId();
} catch (Exception e) {
logger.warn("Could not retrieve authenticated user", e);
// historically we didn't handle users here so always returned null. This is a fallback to that behaviour.
return null;
}
}

@Override
Expand Down Expand Up @@ -194,8 +201,8 @@ protected List<CommentThread> getDiscussions(AzureDevopsClient client, PullReque
}

@Override
protected boolean isNoteFromCurrentUser(Comment note, Void user) {
return true;
protected boolean isNoteFromCurrentUser(Comment note, String user) {
return note.getAuthor().getId().equals(user);
}

@Override
Expand Down Expand Up @@ -236,6 +243,17 @@ protected void resolveDiscussion(AzureDevopsClient client, CommentThread discuss
}
}

@Override
protected void deleteDiscussion(AzureDevopsClient client, CommentThread discussion, PullRequest pullRequest, List<Comment> notesForDiscussion) {
try {
for (Comment note : notesForDiscussion) {
client.deletePullRequestThreadComment(pullRequest.getRepository().getProject().getName(), pullRequest.getRepository().getName(), pullRequest.getId(), discussion.getId(), note.getId());
}
} catch (IOException ex) {
throw new IllegalStateException("Could not delete Pull Request comment thread on Azure Devops", ex);
}
}

@Override
protected Optional<ProjectIssueIdentifier> parseIssueDetails(AzureDevopsClient client, Comment note) {
Optional<ProjectIssueIdentifier> issueIdentifier = super.parseIssueDetails(client, note);
Expand Down
Loading

0 comments on commit 6fbf8f4

Please sign in to comment.