api: comment_pull_request, added commit_id parameter to validate status changed on particular commit.

- when using the API and creating a comment with status change it's now possible to pass in
commit_id, this will allow validation if status change of pull_request is allowed based on the given
commit_id. This solves the case when long running test sends approval of pull request which was
already updated several times. The commit_id will now validate for which state the approval was made,
and prevent accidental aproval of outdated pull requests.

rhodecode/api/tests/test_comment_pull_request.py

@@ -58,7 +58,7 b' class TestCommentPullRequest(object):'
         expected = {
             'pull_request_id': pull_request.pull_request_id,
             'comment_id': comments[-1].comment_id,
-            'status': None
+            'status': {'given': None, 'was_changed': None}
         }
         assert_ok(id_, expected, response.body)
 
@@ -88,7 +88,56 b' class TestCommentPullRequest(object):'
         expected = {
             'pull_request_id': pull_request.pull_request_id,
             'comment_id': comments[-1].comment_id,
-            'status': 'rejected'
+            'status':  {'given': 'rejected', 'was_changed': True}
+        }
+        assert_ok(id_, expected, response.body)
+
+    @pytest.mark.backends("git", "hg")
+    def test_api_comment_pull_request_change_status_with_specific_commit_id(
+            self, pr_util, no_notifications):
+        pull_request = pr_util.create_pull_request()
+        pull_request_id = pull_request.pull_request_id
+        latest_commit_id = 'test_commit'
+        # inject additional revision, to fail test the status change on
+        # non-latest commit
+        pull_request.revisions = pull_request.revisions + ['test_commit']
+
+        id_, params = build_data(
+            self.apikey, 'comment_pull_request',
+            repoid=pull_request.target_repo.repo_name,
+            pullrequestid=pull_request.pull_request_id,
+            status='approved', commit_id=latest_commit_id)
+        response = api_call(self.app, params)
+        pull_request = PullRequestModel().get(pull_request_id)
+
+        expected = {
+            'pull_request_id': pull_request.pull_request_id,
+            'comment_id': None,
+            'status':  {'given': 'approved', 'was_changed': False}
+        }
+        assert_ok(id_, expected, response.body)
+
+    @pytest.mark.backends("git", "hg")
+    def test_api_comment_pull_request_change_status_with_specific_commit_id(
+            self, pr_util, no_notifications):
+        pull_request = pr_util.create_pull_request()
+        pull_request_id = pull_request.pull_request_id
+        latest_commit_id = pull_request.revisions[0]
+
+        id_, params = build_data(
+            self.apikey, 'comment_pull_request',
+            repoid=pull_request.target_repo.repo_name,
+            pullrequestid=pull_request.pull_request_id,
+            status='approved', commit_id=latest_commit_id)
+        response = api_call(self.app, params)
+        pull_request = PullRequestModel().get(pull_request_id)
+
+        comments = ChangesetCommentsModel().get_comments(
+            pull_request.target_repo.repo_id, pull_request=pull_request)
+        expected = {
+            'pull_request_id': pull_request.pull_request_id,
+            'comment_id': comments[-1].comment_id,
+            'status':  {'given': 'approved', 'was_changed': True}
         }
         assert_ok(id_, expected, response.body)
 
@@ -103,7 +152,7 b' class TestCommentPullRequest(object):'
             pullrequestid=pull_request_id)
         response = api_call(self.app, params)
 
-        expected = 'message and status parameter missing'
+        expected = 'Both message and status parameters are missing. At least one is required.'
         assert_error(id_, expected, given=response.body)
 
     @pytest.mark.backends("git", "hg")
@@ -118,7 +167,7 b' class TestCommentPullRequest(object):'
             status='42')
         response = api_call(self.app, params)
 
-        expected = 'unknown comment status`42`'
+        expected = 'Unknown comment status: `42`'
         assert_error(id_, expected, given=response.body)
 
     @pytest.mark.backends("git", "hg")
@@ -144,3 +193,17 b' class TestCommentPullRequest(object):'
 
         expected = 'userid is not the same as your user'
         assert_error(id_, expected, given=response.body)
+
+    @pytest.mark.backends("git", "hg")
+    def test_api_comment_pull_request_wrong_commit_id_error(self, pr_util):
+        pull_request = pr_util.create_pull_request()
+        id_, params = build_data(
+            self.apikey_regular, 'comment_pull_request',
+            repoid=pull_request.target_repo.repo_name,
+            status='approved',
+            pullrequestid=pull_request.pull_request_id,
+            commit_id='XXX')
+        response = api_call(self.app, params)
+
+        expected = 'Invalid commit_id `XXX` for this pull request.'
+        assert_error(id_, expected, given=response.body)

rhodecode/api/views/pull_request_api.py

@@ -361,6 +361,7 b' def close_pull_request(request, apiuser,'
 @jsonrpc_method()
 def comment_pull_request(request, apiuser, repoid, pullrequestid,
                          message=Optional(None), status=Optional(None),
+                         commit_id=Optional(None),
                          userid=Optional(OAttr('apiuser'))):
     """
     Comment on the pull request specified with the `pullrequestid`,
@@ -382,6 +383,10 b' def comment_pull_request(request, apiuse'
         * rejected
         * under_review
     :type status: str
+    :param commit_id: Specify the commit_id for which to set a comment. If
+        given commit_id is different than latest in the PR status
+        change won't be performed.
+    :type commit_id: str
     :param userid: Comment on the pull request as this user
     :type userid: Optional(str or int)
 
@@ -393,7 +398,9 b' def comment_pull_request(request, apiuse'
       result :
         {
             "pull_request_id":  "<Integer>",
-            "comment_id":       "<Integer>"
+            "comment_id":       "<Integer>",
+            "status": {"given": <given_status>,
+                       "was_changed": <bool status_was_actually_changed> },
         }
       error :  null
     """
@@ -412,24 +419,42 b' def comment_pull_request(request, apiuse'
         raise JSONRPCError('repository `%s` does not exist' % (repoid,))
     message = Optional.extract(message)
     status = Optional.extract(status)
+    commit_id = Optional.extract(commit_id)
+
     if not message and not status:
-        raise JSONRPCError('message and status parameter missing')
+        raise JSONRPCError(
+            'Both message and status parameters are missing. '
+            'At least one is required.')
 
     if (status not in (st[0] for st in ChangesetStatus.STATUSES) and
             status is not None):
-        raise JSONRPCError('unknown comment status`%s`' % status)
+        raise JSONRPCError('Unknown comment status: `%s`' % status)
+
+    if commit_id and commit_id not in pull_request.revisions:
+        raise JSONRPCError(
+            'Invalid commit_id `%s` for this pull request.' % commit_id)
 
     allowed_to_change_status = PullRequestModel().check_user_change_status(
         pull_request, apiuser)
+
+    # if commit_id is passed re-validated if user is allowed to change status
+    # based on latest commit_id from the PR
+    if commit_id:
+        commit_idx = pull_request.revisions.index(commit_id)
+        if commit_idx != 0:
+            allowed_to_change_status = False
+
     text = message
+    status_label = ChangesetStatus.get_status_lbl(status)
     if status and allowed_to_change_status:
-        st_message = (('Status change %(transition_icon)s %(status)s')
-                      % {'transition_icon': '>',
-                         'status': ChangesetStatus.get_status_lbl(status)})
+        st_message = ('Status change %(transition_icon)s %(status)s'
+                      % {'transition_icon': '>', 'status': status_label})
         text = message or st_message
 
     rc_config = SettingsModel().get_all_settings()
     renderer = rc_config.get('rhodecode_markup_renderer', 'rst')
+
+    status_change = status and allowed_to_change_status
     comment = ChangesetCommentsModel().create(
         text=text,
         repo=pull_request.target_repo.repo_id,
@@ -437,10 +462,8 b' def comment_pull_request(request, apiuse'
         pull_request=pull_request.pull_request_id,
         f_path=None,
         line_no=None,
-        status_change=(ChangesetStatus.get_status_lbl(status)
-                       if status and allowed_to_change_status else None),
-        status_change_type=(status
-                            if status and allowed_to_change_status else None),
+        status_change=(status_label if status_change else None),
+        status_change_type=(status if status_change else None),
         closing_pr=False,
         renderer=renderer
     )
@@ -458,8 +481,8 b' def comment_pull_request(request, apiuse'
     Session().commit()
     data = {
         'pull_request_id': pull_request.pull_request_id,
-        'comment_id': comment.comment_id,
-        'status': status
+        'comment_id': comment.comment_id if comment else None,
+        'status': {'given': status, 'was_changed': status_change},
     }
     return data