From 7c80c98a94686b3b617aa96223a8d7326f8107f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20Wei=C3=9Fschuh?= Date: Wed, 1 Nov 2017 15:39:49 +0100 Subject: [PATCH] report issues with no direct line in the diff view this may happen if the issue is reported on a line, that is not directly involved in the PR Closes #135 #137 --- .../plugins/stash/StashRequestFacade.java | 21 ++++++++++--------- .../plugins/stash/issue/MarkdownPrinter.java | 8 +++++++ .../plugins/stash/issue/StashDiffReport.java | 10 ++++++++- .../stash/issue/StashDiffReportTest.java | 4 ++-- 4 files changed, 30 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/sonar/plugins/stash/StashRequestFacade.java b/src/main/java/org/sonar/plugins/stash/StashRequestFacade.java index 06452827..03e8d371 100644 --- a/src/main/java/org/sonar/plugins/stash/StashRequestFacade.java +++ b/src/main/java/org/sonar/plugins/stash/StashRequestFacade.java @@ -193,7 +193,6 @@ private void postIssueComment(PullRequestRef pr, String issueKey = issue.key(); String path = getIssuePath(issue); StashCommentReport comments = commentsByFile.get(path); - String commentContent = getMarkdownPrinter().printIssueMarkdown(issue); Integer issueLine = issue.line(); if (issueLine == null) { issueLine = 0; @@ -201,14 +200,9 @@ private void postIssueComment(PullRequestRef pr, // Surprisingly this syntax does not trigger the squid:NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE rule // but it does if you transform that into a ternary operator at the assignment level :/ - // if comment not already pushed to Stash - if (comments != null && comments.contains(commentContent, path, issueLine)) { - LOGGER.debug("Comment \"{}\" already pushed on file {} ({})", issueKey, path, issueLine); - return; - } + IssueType type = diffReport.getType(path, issueLine, config.issueVicinityRange()); // check if issue belongs to the Stash diff view - IssueType type = diffReport.getType(path, issueLine, config.issueVicinityRange()); if (type == null) { LOGGER.info( "Comment \"{}\" cannot be pushed to Stash like it does not belong to diff view - {} (line: {})", @@ -216,13 +210,20 @@ private void postIssueComment(PullRequestRef pr, return; } - long line = diffReport.getLine(path, issueLine); + long diffLine = diffReport.getLine(path, issueLine); + String commentContent = getMarkdownPrinter().printIssueMarkdown(issue, diffLine == 0); + + // if comment not already pushed to Stash + if (comments != null && comments.contains(commentContent, path, diffLine)) { + LOGGER.debug("Comment \"{}\" already pushed on file {} ({})", issueKey, path, diffLine); + return; + } StashComment comment = stashClient - .postCommentLineOnPullRequest(pr, commentContent, path, line, type); + .postCommentLineOnPullRequest(pr, commentContent, path, diffLine, type); LOGGER - .debug("Comment \"{}\" has been created ({}) on file {} ({})", issueKey, type, path, line); + .debug("Comment \"{}\" has been created ({}) on file {} ({})", issueKey, type, path, diffLine); // Create task linked to the comment if configured diff --git a/src/main/java/org/sonar/plugins/stash/issue/MarkdownPrinter.java b/src/main/java/org/sonar/plugins/stash/issue/MarkdownPrinter.java index 8d3df01c..a18d5278 100644 --- a/src/main/java/org/sonar/plugins/stash/issue/MarkdownPrinter.java +++ b/src/main/java/org/sonar/plugins/stash/issue/MarkdownPrinter.java @@ -62,6 +62,10 @@ public static String printIssueNumberBySeverityMarkdown(Collection } public String printIssueMarkdown(PostJobIssue issue) { + return printIssueMarkdown(issue, false); + } + + public String printIssueMarkdown(PostJobIssue issue, boolean includeLineNumber) { StringBuilder sb = new StringBuilder(); String message = issue.message(); @@ -76,6 +80,10 @@ public String printIssueMarkdown(PostJobIssue issue) { sonarQubeURL + "/" + CODING_RULES_RULE_KEY + issue.ruleKey())) .append("]"); + if (includeLineNumber) { + sb.append(" (Line ").append(issue.line()).append(")"); + } + return sb.toString(); } diff --git a/src/main/java/org/sonar/plugins/stash/issue/StashDiffReport.java b/src/main/java/org/sonar/plugins/stash/issue/StashDiffReport.java index 471552df..48d40e6e 100644 --- a/src/main/java/org/sonar/plugins/stash/issue/StashDiffReport.java +++ b/src/main/java/org/sonar/plugins/stash/issue/StashDiffReport.java @@ -44,8 +44,11 @@ private static boolean includeVicinityIssuesForDiff(StashDiff diff, long destina } public IssueType getType(String path, long destination, int vicinityRange) { + boolean foundFile = false; + for (StashDiff diff : diffs) { if (Objects.equals(diff.getPath(), path)) { + foundFile = true; // Line 0 never belongs to Stash Diff view. // It is a global comment with a type set to CONTEXT. if (destination == 0) { @@ -56,7 +59,12 @@ public IssueType getType(String path, long destination, int vicinityRange) { } } } - return null; + + if (foundFile) { + return IssueType.CONTEXT; + } else { + return null; + } } /** diff --git a/src/test/java/org/sonar/plugins/stash/issue/StashDiffReportTest.java b/src/test/java/org/sonar/plugins/stash/issue/StashDiffReportTest.java index 3274c39b..9c9c170b 100755 --- a/src/test/java/org/sonar/plugins/stash/issue/StashDiffReportTest.java +++ b/src/test/java/org/sonar/plugins/stash/issue/StashDiffReportTest.java @@ -81,8 +81,8 @@ public void testGetType() { assertEquals(IssueType.CONTEXT, report1.getType("path/to/diff1", 20, StashDiffReport.VICINITY_RANGE_NONE)); assertEquals(IssueType.ADDED, report1.getType("path/to/diff2", 30, StashDiffReport.VICINITY_RANGE_NONE)); - assertEquals(null, report1.getType("path/to/diff2", 20, StashDiffReport.VICINITY_RANGE_NONE)); - assertEquals(null, report1.getType("path/to/diff1", 30, StashDiffReport.VICINITY_RANGE_NONE)); + assertEquals(IssueType.CONTEXT, report1.getType("path/to/diff2", 20, StashDiffReport.VICINITY_RANGE_NONE)); + assertEquals(IssueType.CONTEXT, report1.getType("path/to/diff1", 30, StashDiffReport.VICINITY_RANGE_NONE)); assertEquals(null, report1.getType("path/to/diff4", 60, StashDiffReport.VICINITY_RANGE_NONE)); }