Skip to content
This repository has been archived by the owner on Jan 7, 2021. It is now read-only.

implement resetting of overview comments #162

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

t-8ch
Copy link
Contributor

@t-8ch t-8ch commented Nov 2, 2017

See #119

@t-8ch t-8ch force-pushed the reset_overview_comments branch from 09a70e8 to 7d50a97 Compare November 3, 2017 12:45

stashClient.deletePullRequestComment(pr, comment);
}
// FIXME delete tasks on file-wide comments
Copy link

Choose a reason for hiding this comment

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

MAJOR Take the required action to fix the issue indicated by this comment. rule

@@ -111,31 +112,29 @@ public void postCommentOnPullRequest(PullRequestRef pr, String report)
postCreate(request, json, MessageFormat.format(COMMENT_POST_ERROR_MESSAGE, pr.repository(), pr.pullRequestId()));
}

public Collection<StashComment> getPullRequestOverviewComments(PullRequestRef pr) throws StashClientException {
return getPaged(
MessageFormat.format(API_ONE_PR + "/activities", baseUrl, pr.project(), pr.repository(), pr.pullRequestId()),
Copy link

Choose a reason for hiding this comment

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

MAJOR Format specifiers should be used instead of string concatenation. rule


boolean doneLastPage = false;
int nextPageStart = 0;
// FIXME size/limit support
Copy link

Choose a reason for hiding this comment

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

MAJOR Take the required action to fix the issue indicated by this comment. rule

stashClient.deletePullRequestComment(pr, comment);
}
// FIXME delete tasks on file-wide comments
// resetComments(diffReport.getComments(), pr, sonarUser, stashClient);
Copy link

Choose a reason for hiding this comment

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

MAJOR This block of commented-out lines of code should be removed. rule

@ghost
Copy link

ghost commented Nov 3, 2017

SonarQube analysis reported 6 issues

  • MAJOR 6 major

Watch the comments in this conversation to review them.

1 extra issue

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. MAJOR StashRequestFacade.java#L397: Remove this unused method parameter "diffReport". rule

@@ -51,7 +54,10 @@ public static StashComment extractComment(JsonObject jsonComment, String path, L
JsonObject jsonAuthor = (JsonObject)jsonComment.get(AUTHOR);
StashUser stashUser = extractUser(jsonAuthor);

return new StashComment(id, message, path, line, stashUser, version);
StashComment result = new StashComment(id, message, path, line, stashUser, version);
// FIXME do this at some central place
Copy link

Choose a reason for hiding this comment

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

MAJOR Take the required action to fix the issue indicated by this comment. rule


while (!doneLastPage) {
if (nextPageStart > 0) {
requestBuilder.addQueryParam("start", String.valueOf(nextPageStart));

Choose a reason for hiding this comment

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

This will add an additional parameter "start" and not replace the previous one. With more activities you will end up with link params:
?start=25&start=50&start50&start-50...
which will at the end fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marekjagielski Good point. thanks!

@datasmurfen
Copy link

This function should be implemented if it's not 👍

@tonyfalabella
Copy link

The sonar.stash.comments.reset feature still does not work in version 1.5.1. I think it broken once Sonar moved to version 7.x. To get the feature to properly work I took what was in this PR and tweaked it a bit to work with the 1.5.1 codebase (since this PR looks to be based on an older version of sonar-stash). I'm not sure if it breaks anything else, but it does in fact fix the reset issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants