Submit Summary Note before issue comments

gradle.properties

@@ -1 +1 @@
-version=1.15.0-SNAPSHOT
+version=1.14.2-SNAPSHOT

src/main/java/com/github/mc1arke/sonarqube/plugin/CommunityBranchPlugin.java

@@ -73,6 +73,7 @@
 public class CommunityBranchPlugin implements Plugin, CoreExtension {
 
     public static final String IMAGE_URL_BASE = "com.github.mc1arke.sonarqube.plugin.branch.image-url-base";
+    public static final String PR_SUMMARY_NOTE_FIRST = "com.github.mc1arke.sonarqube.plugin.branch.pullrequest.summary.first";
 
     @Override
     public String getName() {
@@ -157,6 +158,15 @@ public void load(CoreExtension.Context context) {
                                           .build(),
                 MonoRepoFeature.class);
 
+            context.addExtensions(PropertyDefinition.builder(PR_SUMMARY_NOTE_FIRST)
+                    .category(getName())
+                    .subCategory("Merge Request Decoration")
+                    .onQualifiers(Qualifiers.PROJECT)
+                    .name("Submit summary note first")
+                    .description("Submit summary discussion thread before issue comments.")
+                    .type(PropertyType.BOOLEAN)
+                    .defaultValue(String.valueOf(false))
+                    .build());
         }
     }
 

src/main/java/com/github/mc1arke/sonarqube/plugin/ce/pullrequest/DiscussionAwarePullRequestDecorator.java

@@ -94,15 +94,22 @@ public DecorationResult decorateQualityGateStatus(AnalysisDetails analysis, AlmS
                 .filter(issue -> isIssueFromCommitInCurrentRequest(issue.getLeft(), commitIds, scmInfoRepository))
                 .collect(Collectors.toList());
 
+        AnalysisSummary analysisSummary = reportGenerator.createAnalysisSummary(analysis);
+        boolean isSummaryNoteFirst = isSummaryNoteFirstEnabled(analysis);
+        if (isSummaryNoteFirst) {
+            submitSummaryNote(client, pullRequest, analysis, analysisSummary);
+        }
+
         uncommentedIssues.forEach(issue -> submitCommitNoteForIssue(client,
                 pullRequest,
                 issue.getLeft(),
                 issue.getRight(),
                 analysis,
                 reportGenerator.createAnalysisIssueSummary(issue.getLeft(), analysis)));
 
-        AnalysisSummary analysisSummary = reportGenerator.createAnalysisSummary(analysis);
-        submitSummaryNote(client, pullRequest, analysis, analysisSummary);
+        if (!isSummaryNoteFirst) {
+            submitSummaryNote(client, pullRequest, analysis, analysisSummary);
+        }
         submitPipelineStatus(client, pullRequest, analysis, analysisSummary);
 
         DecorationResult.Builder builder = DecorationResult.builder();

src/main/java/com/github/mc1arke/sonarqube/plugin/ce/pullrequest/PullRequestBuildStatusDecorator.java

@@ -18,6 +18,7 @@
  */
 package com.github.mc1arke.sonarqube.plugin.ce.pullrequest;
 
+import com.github.mc1arke.sonarqube.plugin.CommunityBranchPlugin;
 import org.sonar.db.alm.setting.ALM;
 import org.sonar.db.alm.setting.AlmSettingDto;
 import org.sonar.db.alm.setting.ProjectAlmSettingDto;
@@ -30,4 +31,10 @@ DecorationResult decorateQualityGateStatus(AnalysisDetails analysisDetails, AlmS
                                    ProjectAlmSettingDto projectAlmSettingDto);
 
     List<ALM> alm();
+
+    default boolean isSummaryNoteFirstEnabled(AnalysisDetails analysisDetails) {
+        return analysisDetails.getScannerProperty(CommunityBranchPlugin.PR_SUMMARY_NOTE_FIRST)
+                .map(Boolean::parseBoolean)
+                .orElse(false);
+    }
 }

src/main/java/com/github/mc1arke/sonarqube/plugin/scanner/ScannerPullRequestPropertySensor.java

@@ -18,6 +18,7 @@
  */
 package com.github.mc1arke.sonarqube.plugin.scanner;
 
+import com.github.mc1arke.sonarqube.plugin.CommunityBranchPlugin;
 import com.github.mc1arke.sonarqube.plugin.ce.pullrequest.gitlab.GitlabMergeRequestDecorator;
 import org.sonar.api.batch.sensor.Sensor;
 import org.sonar.api.batch.sensor.SensorContext;
@@ -54,6 +55,8 @@ public void execute(SensorContext sensorContext) {
         Optional.ofNullable(system2.property(GitlabMergeRequestDecorator.PULLREQUEST_GITLAB_PIPELINE_ID)).ifPresent(
                 v -> sensorContext.addContextProperty(GitlabMergeRequestDecorator.PULLREQUEST_GITLAB_PIPELINE_ID, v));
 
+        sensorContext.config().get(CommunityBranchPlugin.PR_SUMMARY_NOTE_FIRST)
+                .ifPresent(p -> sensorContext.addContextProperty(CommunityBranchPlugin.PR_SUMMARY_NOTE_FIRST, p));
     }
 
 }

src/test/java/com/github/mc1arke/sonarqube/plugin/ce/pullrequest/gitlab/DiscussionMock.java

@@ -0,0 +1,86 @@
+package com.github.mc1arke.sonarqube.plugin.ce.pullrequest.gitlab;
+
+import com.github.mc1arke.sonarqube.plugin.almclient.gitlab.model.Discussion;
+import com.github.mc1arke.sonarqube.plugin.almclient.gitlab.model.Note;
+import com.github.mc1arke.sonarqube.plugin.almclient.gitlab.model.User;
+
+import java.util.*;
+import java.util.function.Supplier;
+import java.util.stream.Collectors;
+
+class DiscussionMock {
+    private static final String PROJECT_KEY = "projectKey";
+    private static final String SONARQUBE_USERNAME = "sonarqube@gitlab.dummy";
+    private static final String OLD_SONARQUBE_ISSUE_COMMENT = "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 User sonarqubeUser = new User(SONARQUBE_USERNAME);
+
+    private static Discussion createIssueComment() {
+        Note note = createUnresolvedSonarQubeNote(
+                "Reported issue\n[View in SonarQube](http://domain.url/sonar/issue?issues=issueKey1&id=" +
+                        PROJECT_KEY + ")");
+        return new Discussion("issue-comment-id", Collections.singletonList(note));
+    }
+
+    private static Discussion createResolvedIssueComment() {
+        Note note = createResolvedSonarQubeNote(
+                "Reported issue\n[View in SonarQube](http://domain.url/sonar/issue?issues=issueKey1&id=" +
+                        PROJECT_KEY + ")");
+        return new Discussion("resolved-issue-comment-id", Collections.singletonList(note));
+    }
+
+    private static Discussion createResolvedByCommentIssueComment() {
+        Note note = createUnresolvedSonarQubeNote(
+                "Reported issue\n[View in SonarQube](http://domain.url/sonar/issue?issues=issueKey1&id=" +
+                        PROJECT_KEY + ")");
+        Note note2 = createUnresolvedSonarQubeNote(OLD_SONARQUBE_ISSUE_COMMENT);
+        Note note3 = createUnresolvedSonarQubeNote("Some additional comment");
+        return new Discussion("issue-with-resolved-comment-id", Arrays.asList(note, note2, note3));
+    }
+
+    private static Discussion createUnresolvedSummaryNote() {
+        Note note = createUnresolvedSonarQubeNote(
+                "Analysis Details\n[View in SonarQube](http://domain.url/sonar/dashboard?id=" + PROJECT_KEY + ")");
+        return new Discussion("summary-note-id", Collections.singletonList(note));
+    }
+
+    private static Discussion createResolvedSummaryNote() {
+        Note note = createResolvedSonarQubeNote(
+                "Analysis Details\n[View in SonarQube](http://domain.url/sonar/dashboard?id=" + PROJECT_KEY + ")");
+        return new Discussion("summary-note-id", Collections.singletonList(note));
+    }
+
+    private static Note createUnresolvedSonarQubeNote(String body) {
+        return new Note(new Random().nextLong(), false, sonarqubeUser, body, false, true);
+    }
+
+    private static Note createResolvedSonarQubeNote(String body) {
+        return new Note(new Random().nextLong(), false, sonarqubeUser, body, true, true);
+    }
+
+    public static Map<DiscussionType, Discussion> getDiscussionsMap(DiscussionType... discussions) {
+        return Arrays.stream(discussions)
+                .collect(Collectors.toMap(k -> k, DiscussionType::create, (e1, e2) -> e1, LinkedHashMap::new));
+    }
+
+    enum DiscussionType {
+        RESOLVED_SUMMARY_NOTE(DiscussionMock::createResolvedSummaryNote),
+        UNRESOLVED_SUMMARY_NOTE(DiscussionMock::createUnresolvedSummaryNote),
+        ISSUE_COMMENT(DiscussionMock::createIssueComment),
+        RESOLVED_ISSUE_COMMENT(DiscussionMock::createResolvedIssueComment),
+        RESOLVED_BY_COMMENT_ISSUE_COMMENT(DiscussionMock::createResolvedByCommentIssueComment);
+
+        private final Supplier<Discussion> creationMethod;
+
+        DiscussionType(Supplier<Discussion> creationMethod) {
+            this.creationMethod = creationMethod;
+        }
+
+        public Discussion create() {
+            return creationMethod.get();
+        }
+    }
+
+}

src/test/java/com/github/mc1arke/sonarqube/plugin/ce/pullrequest/gitlab/GitlabMergeRequestDecoratorTest.java

@@ -18,6 +18,7 @@
  */
 package com.github.mc1arke.sonarqube.plugin.ce.pullrequest.gitlab;
 
+import com.github.mc1arke.sonarqube.plugin.CommunityBranchPlugin;
 import com.github.mc1arke.sonarqube.plugin.almclient.gitlab.GitlabClient;
 import com.github.mc1arke.sonarqube.plugin.almclient.gitlab.GitlabClientFactory;
 import com.github.mc1arke.sonarqube.plugin.almclient.gitlab.model.Commit;
@@ -36,8 +37,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.junit.Before;
-import org.junit.Test;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
 import org.mockito.ArgumentCaptor;
 import org.sonar.api.ce.posttask.QualityGate;
 import org.sonar.api.issue.Issue;
@@ -54,9 +55,11 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.Map;
 import java.util.Optional;
 import java.util.stream.Collectors;
 
+import static com.github.mc1arke.sonarqube.plugin.ce.pullrequest.gitlab.DiscussionMock.DiscussionType.*;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.ArgumentMatchers.any;
@@ -101,7 +104,7 @@ public class GitlabMergeRequestDecoratorTest {
 
     private final GitlabMergeRequestDecorator underTest = new GitlabMergeRequestDecorator(scmInfoRepository, gitlabClientFactory, reportGenerator, markdownFormatterFactory);
 
-    @Before
+    @BeforeEach
     public void setUp() throws IOException {
         when(analysisSummary.format(any())).thenReturn("Summary Comment");
         when(reportGenerator.createAnalysisSummary(any())).thenReturn(analysisSummary);
@@ -589,6 +592,82 @@ public void shouldNotStartNewDiscussionForIssueWithExistingCommentFromCommitInMe
         assertThat(mergeRequestNoteArgumentCaptor.getValue()).isNotInstanceOf(CommitNote.class);
     }
 
+    @Test
+    public void shouldStartNewDiscussionForIssueIfExistingCommentResolvedInMergeRequest() throws IOException {
+        testStartNewDiscussionForIssueWhenCommentExists(RESOLVED_ISSUE_COMMENT);
+    }
+
+    @Test
+    public void shouldStartNewDiscussionForIssueIfResolvedBySonarQubeCommentInMergeRequest() throws IOException {
+        testStartNewDiscussionForIssueWhenCommentExists(RESOLVED_BY_COMMENT_ISSUE_COMMENT);
+    }
+
+    private void testStartNewDiscussionForIssueWhenCommentExists(DiscussionMock.DiscussionType discussionType) throws IOException {
+        reportIssue();
+        existingMergeRequestDiscussions(discussionType);
+
+        underTest.decorateQualityGateStatus(analysisDetails, almSettingDto, projectAlmSettingDto);
+
+        verify(gitlabClient, never()).resolveMergeRequestDiscussion(anyLong(), anyLong(), any());
+        verify(gitlabClient, never()).addMergeRequestDiscussionNote(anyLong(), anyLong(), any(), any());
+
+        ArgumentCaptor<MergeRequestNote> mergeRequestNoteArgumentCaptor = ArgumentCaptor.forClass(MergeRequestNote.class);
+        verify(gitlabClient, times(2)).addMergeRequestDiscussion(eq(PROJECT_ID), eq(MERGE_REQUEST_IID), mergeRequestNoteArgumentCaptor.capture());
+
+        assertThat(mergeRequestNoteArgumentCaptor.getAllValues().get(0))
+                .usingRecursiveComparison()
+                .isEqualTo(new CommitNote("Issue Summary", BASE_SHA, START_SHA, HEAD_SHA, "path-to-file", "path-to-file", 999));
+        assertThat(mergeRequestNoteArgumentCaptor.getAllValues().get(1)).isNotInstanceOf(CommitNote.class);
+    }
+
+    private void reportIssue() {
+        PostAnalysisIssueVisitor.LightIssue lightIssue = mock(PostAnalysisIssueVisitor.LightIssue.class);
+        when(lightIssue.key()).thenReturn("issueKey1");
+        when(lightIssue.getStatus()).thenReturn(Issue.STATUS_OPEN);
+        when(lightIssue.getLine()).thenReturn(999);
+
+        Component component = mock(Component.class);
+
+        PostAnalysisIssueVisitor.ComponentIssue componentIssue = mock(PostAnalysisIssueVisitor.ComponentIssue.class);
+        when(componentIssue.getIssue()).thenReturn(lightIssue);
+        when(componentIssue.getComponent()).thenReturn(component);
+        when(componentIssue.getScmPath()).thenReturn(Optional.of("path-to-file"));
+
+        when(analysisDetails.getScmReportableIssues()).thenReturn(Collections.singletonList(componentIssue));
+
+        Changeset changeset = mock(Changeset.class);
+        when(changeset.getRevision()).thenReturn("DEF");
+
+        ScmInfo scmInfo = mock(ScmInfo.class);
+        when(scmInfo.hasChangesetForLine(999)).thenReturn(true);
+        when(scmInfo.getChangesetForLine(999)).thenReturn(changeset);
+        when(scmInfoRepository.getScmInfo(component)).thenReturn(Optional.of(scmInfo));
+    }
+
+    @Test
+    public void shouldSubmitSummaryNoteBeforeIssueCommentWhenSummaryNoteFirstEnabled() throws IOException {
+        enableScannerProperty(CommunityBranchPlugin.PR_SUMMARY_NOTE_FIRST);
+        reportIssue();
+        noDiscussionsExist();
+
+        underTest.decorateQualityGateStatus(analysisDetails, almSettingDto, projectAlmSettingDto);
+
+        verify(gitlabClient, never()).resolveMergeRequestDiscussion(anyLong(), anyLong(), any());
+        verify(gitlabClient, never()).addMergeRequestDiscussionNote(anyLong(), anyLong(), any(), any());
+
+        ArgumentCaptor<MergeRequestNote> mergeRequestNoteArgumentCaptor = ArgumentCaptor.forClass(MergeRequestNote.class);
+        verify(gitlabClient, times(2)).addMergeRequestDiscussion(eq(PROJECT_ID), eq(MERGE_REQUEST_IID), mergeRequestNoteArgumentCaptor.capture());
+
+        assertThat(mergeRequestNoteArgumentCaptor.getAllValues().get(0)).isInstanceOf(MergeRequestNote.class);
+        assertThat(mergeRequestNoteArgumentCaptor.getAllValues().get(1))
+                .usingRecursiveComparison()
+                .isEqualTo(new CommitNote("Issue Summary", BASE_SHA, START_SHA, HEAD_SHA, "path-to-file", "path-to-file", 999));
+    }
+
+    private void noDiscussionsExist() throws IOException {
+        when(gitlabClient.getMergeRequestDiscussions(anyLong(), anyLong())).thenReturn(new ArrayList<>());
+    }
+
     @Test
     public void shouldNotCreateCommentsForIssuesWithNoLineNumbers() throws IOException {
         PostAnalysisIssueVisitor.LightIssue lightIssue = mock(PostAnalysisIssueVisitor.LightIssue.class);
@@ -617,6 +696,17 @@ public void shouldNotCreateCommentsForIssuesWithNoLineNumbers() throws IOExcepti
         assertThat(mergeRequestNoteArgumentCaptor.getValue()).isNotInstanceOf(CommitNote.class);
     }
 
+    private void enableScannerProperty(String property) {
+        when(analysisDetails.getScannerProperty(property)).thenReturn(Optional.of(String.valueOf(true)));
+    }
+
+    private Map<DiscussionMock.DiscussionType, Discussion> existingMergeRequestDiscussions(DiscussionMock.DiscussionType... discussions) throws IOException {
+        var discussionsMap = DiscussionMock.getDiscussionsMap(discussions);
+        when(gitlabClient.getMergeRequestDiscussions(anyLong(), anyLong()))
+                .thenReturn(new ArrayList<>(discussionsMap.values()));
+        return discussionsMap;
+    }
+
     @Test
     public void shouldSubmitSuccessfulPipelineStatusAndResolvedSummaryCommentOnSuccessAnalysis() throws IOException {
         when(analysisDetails.getQualityGateStatus()).thenReturn(QualityGate.Status.OK);
@@ -625,6 +715,9 @@ public void shouldSubmitSuccessfulPipelineStatusAndResolvedSummaryCommentOnSucce
         when(analysisSummary.format(any())).thenReturn("Summary comment");
         when(analysisSummary.getDashboardUrl()).thenReturn("https://sonarqube.dummy/dashboard?id=projectKey&pullRequest=123");
 
+        var discussions = existingMergeRequestDiscussions(ISSUE_COMMENT, UNRESOLVED_SUMMARY_NOTE);
+        var summaryNote = discussions.get(UNRESOLVED_SUMMARY_NOTE);
+
         Discussion discussion = mock(Discussion.class);
         when(discussion.getId()).thenReturn("dicussion id");
         when(gitlabClient.addMergeRequestDiscussion(anyLong(), anyLong(), any())).thenReturn(discussion);
@@ -633,6 +726,7 @@ public void shouldSubmitSuccessfulPipelineStatusAndResolvedSummaryCommentOnSucce
 
         ArgumentCaptor<MergeRequestNote> mergeRequestNoteArgumentCaptor = ArgumentCaptor.forClass(MergeRequestNote.class);
         verify(gitlabClient).addMergeRequestDiscussion(eq(PROJECT_ID), eq(MERGE_REQUEST_IID), mergeRequestNoteArgumentCaptor.capture());
+        verify(gitlabClient).resolveMergeRequestDiscussion(PROJECT_ID, MERGE_REQUEST_IID, summaryNote.getId());
         verify(gitlabClient).resolveMergeRequestDiscussion(PROJECT_ID, MERGE_REQUEST_IID, discussion.getId());
         ArgumentCaptor<PipelineStatus> pipelineStatusArgumentCaptor = ArgumentCaptor.forClass(PipelineStatus.class);
         verify(gitlabClient).setMergeRequestPipelineStatus(eq(PROJECT_ID), eq("commitsha"), pipelineStatusArgumentCaptor.capture());