diff --git a/bot/reviewbot/processing/review.py b/bot/reviewbot/processing/review.py index 09e2eee26f2587663356fd9431d774f696e6f918..79aec12ac24af40ce5efad929862200f7bb95a8e 100644 --- a/bot/reviewbot/processing/review.py +++ b/bot/reviewbot/processing/review.py @@ -255,6 +255,7 @@ class Review(object): self.review_request_id = review_request_id self.diff_revision = diff_revision self.comments = [] + self.general_comments = [] # Get the list of files. self.files = [] @@ -265,22 +266,46 @@ class Review(object): self.files = [File(self, f) for f in files] + def general_comment(self, text, issue=None, rich_text=False): + """Make a general comment. + + Args: + text (unicode): + The text of the comment. + + issue (bool, optional): + Whether an issue should be opened. + + rich_text (bool, optional): + Whether the comment text should be formatted using Markdown. + """ + self.general_comments.append({ + 'text': text, + 'issue_opened': issue or self.settings['open_issues'], + 'rich_text': rich_text, + }) + def publish(self): """Upload the review to Review Board.""" # Truncate comments to the maximum permitted amount to avoid # overloading the review and freezing the browser. max_comments = self.settings['max_comments'] + num_comments = len(self.comments) + len(self.general_comments) - if len(self.comments) > max_comments: + if num_comments > max_comments: warning = ('**Warning:** Showing %d of %d failures.' - % (max_comments, len(self.comments))) + % (max_comments, num_comments)) if self.body_top: self.body_top = '%s\n%s' % (self.body_top, warning) else: self.body_top = warning - del self.comments[max_comments:] + if len(self.general_comments) > max_comments: + del self.general_comments[max_comments:] + del self.comments[:] + else: + del self.comments[max_comments - len(self.general_comments):] bot_reviews = self.api_root.get_extension( extension_name='reviewbotext.extension.ReviewBotExtension' @@ -291,7 +316,13 @@ class Review(object): body_top=self.body_top, body_top_rich_text=True, body_bottom=self.body_bottom, - diff_comments=json.dumps(self.comments)) + diff_comments=json.dumps(self.comments), + general_comments=json.dumps(self.general_comments)) + + @property + def has_comments(self): + """Whether the review has comments.""" + return len(self.comments) + len(self.general_comments) != 0 @property def patch_contents(self): diff --git a/bot/reviewbot/tasks.py b/bot/reviewbot/tasks.py index 162f6efe6a8e4448757c7fc64970f0e7a624aef7..00fa4c61ed71fd40cd8b97f9960b90db54316553 100644 --- a/bot/reviewbot/tasks.py +++ b/bot/reviewbot/tasks.py @@ -201,7 +201,7 @@ def RunTool(server_url='', url_text='Tool console output') try: - if len(review.comments) == 0: + if not review.has_comments: status_update.update(state=DONE_SUCCESS, description='passed.') else: diff --git a/extension/reviewbotext/resources.py b/extension/reviewbotext/resources.py index b5a29e06c85c9a773f5dd1f418cc3c957bb546e5..375854deaac9827caec304fd2eb2193e8d88476a 100644 --- a/extension/reviewbotext/resources.py +++ b/extension/reviewbotext/resources.py @@ -1,6 +1,7 @@ from __future__ import unicode_literals import json +import logging from django.core.exceptions import ObjectDoesNotExist from djblets.webapi.decorators import (webapi_login_required, @@ -18,6 +19,19 @@ from reviewboard.webapi.resources import resources, WebAPIResource from reviewbotext.models import Tool +class InvalidFormDataError(Exception): + """Error that signals to return INVALID_FORM_DATA with attached data.""" + + def __init__(self, data): + """Initialize the Error. + + Args: + data (dict): + The data that should be returned from the webapi method. + """ + self.data = data + + class ToolResource(WebAPIResource): """Resource for workers to update the installed tools list. @@ -180,6 +194,11 @@ class ReviewBotReviewResource(WebAPIResource): 'type': str, 'description': 'A JSON payload containing the diff comments.', }, + 'general_comments': { + 'type': str, + 'description': + 'A JSON payload containing the general comments.', + }, }, ) def create(self, @@ -191,8 +210,51 @@ class ReviewBotReviewResource(WebAPIResource): body_bottom='', body_bottom_rich_text=False, diff_comments=None, + general_comments=None, *args, **kwargs): - """Creates a new review and publishes it.""" + """Creates a new review and publishes it. + + Args: + request (reviewboard.reviews.models.review_request. + ReviewRequest): + The review request the review is filed against. + + review_request_id (int): + The ID of the review request being reviewed (ID for use in the + API, which is the "display_id" field). + + ship_it (bool, optional): + The Ship It state for the review. + + body_top (unicode, optional): + The text for the ``body_top`` field. + + body_top_rich_text (unicode, optional): + Whether the body_top text should be formatted using Markdown. + + body_bottom (unicode, optional): + The text for the ``body_bottom`` field. + + body_bottom_rich_text (unicode, optional): + Whether the body_bottom text should be formatted using + Markdown. + + diff_comments (string, optional): + A JSON payload containing the diff comments. + + general_comments (string, optional): + A JSON payload containing the general comments. + + *args (tuple): + Positional arguments to set in the review. + + **kwargs (dict): + Additional attributes to set in the review. + + Returns: + tuple: + A 2-tuple containing an HTTP return code and the API payload. + """ try: review_request = resources.review_request.get_object( request, @@ -207,6 +269,44 @@ class ReviewBotReviewResource(WebAPIResource): if not body_bottom: body_bottom = '' + try: + diff_comment_keys = ['filediff_id', 'first_line', 'num_lines'] + diff_comments = self._normalizeCommentsJSON( + 'diff_comments', diff_comment_keys, diff_comments) + + general_comments = self._normalizeCommentsJSON( + 'general_comments', [], general_comments) + + filediff_pks = { + comment['filediff_id'] + for comment in diff_comments + } + + filediffs = { + filediff.pk: filediff + for filediff in FileDiff.objects.filter( + pk__in=filediff_pks, + diffset__history__review_request=review_request + ) + } + + for comment in diff_comments: + filediff_id = comment.pop('filediff_id') + + try: + comment['filediff'] = filediffs[filediff_id] + comment['interfilediff'] = None + except KeyError: + return INVALID_FORM_DATA, { + 'fields': { + 'diff_comments': [ + 'Invalid filediff ID: %s' % filediff_id, + ], + }, + } + except InvalidFormDataError as e: + return INVALID_FORM_DATA, e.data + new_review = Review.objects.create( review_request=review_request, user=request.user, @@ -216,45 +316,11 @@ class ReviewBotReviewResource(WebAPIResource): body_bottom_rich_text=body_bottom_rich_text, ship_it=ship_it) - if diff_comments: - try: - diff_comments = json.loads(diff_comments) - - for comment in diff_comments: - filediff = FileDiff.objects.get( - pk=comment['filediff_id'], - diffset__history__review_request=review_request) - - if comment['issue_opened']: - issue = True - issue_status = BaseComment.OPEN - else: - issue = False - issue_status = None - - new_review.comments.create( - filediff=filediff, - interfilediff=None, - text=comment['text'], - first_line=comment['first_line'], - num_lines=comment['num_lines'], - issue_opened=issue, - issue_status=issue_status, - rich_text=comment['rich_text']) - - except KeyError: - # TODO: Reject the DB transaction. - return INVALID_FORM_DATA, { - 'fields': { - 'diff_comments': 'Diff comments were malformed', - }, - } - except ObjectDoesNotExist: - return INVALID_FORM_DATA, { - 'fields': { - 'diff_comments': 'Invalid filediff_id', - }, - } + for comment_type, comments in ( + (new_review.comments, diff_comments), + (new_review.general_comments, general_comments)): + for comment in comments: + comment_type.create(**comment) new_review.publish(user=request.user) @@ -262,5 +328,60 @@ class ReviewBotReviewResource(WebAPIResource): self.item_result_key: new_review, } + def _normalizeCommentsJSON(self, comment_type, extra_keys, comments): + """Normalize all the comments. + + Args: + comment_type (string): + Type of the comment. + + extra_keys (list): + Extra comment keys expected beyond the base comment keys. + + comments (string): + A JSON payload containing the comments. + + Returns: + list: + A list of the decoded and normalized comments. + """ + base_comment_keys = ['issue_opened', 'text', 'rich_text'] + expected_keys = set(base_comment_keys + extra_keys) + + try: + comments = json.loads(comments or '[]') + except ValueError: + raise InvalidFormDataError({ + 'fields': { + comment_type: 'Malformed JSON.', + } + }) + + for comment in comments: + comment_keys = set(comment.keys()) + missing_keys = expected_keys - comment_keys + + if missing_keys: + missing_keys = ', '.join(missing_keys) + raise InvalidFormDataError({ + 'fields': { + comment_type: [ + 'Element missing keys "%s".' % missing_keys + ], + } + }) + + for key in comment_keys: + if key not in expected_keys: + logging.warning('%s field ignored.', key) + del comment[key] + + if comment['issue_opened']: + comment['issue_status'] = BaseComment.OPEN + else: + comment['issue_status'] = None + + return comments + review_bot_review_resource = ReviewBotReviewResource()