diff --git a/rhodecode/controllers/changeset.py b/rhodecode/controllers/changeset.py --- a/rhodecode/controllers/changeset.py +++ b/rhodecode/controllers/changeset.py @@ -40,7 +40,7 @@ from rhodecode.lib.compat import Ordered from rhodecode.lib.exceptions import StatusChangeOnClosedPullRequestError import rhodecode.lib.helpers as h from rhodecode.lib.utils import jsonify -from rhodecode.lib.utils2 import safe_unicode +from rhodecode.lib.utils2 import safe_unicode, safe_int from rhodecode.lib.vcs.backends.base import EmptyCommit from rhodecode.lib.vcs.exceptions import ( RepositoryError, CommitDoesNotExistError, NodeDoesNotExistError) @@ -431,15 +431,19 @@ class ChangesetController(BaseRepoContro @auth.CSRFRequired() @jsonify def delete_comment(self, repo_name, comment_id): - comment = ChangesetComment.get(comment_id) + comment = ChangesetComment.get_or_404(safe_int(comment_id)) if not comment: log.debug('Comment with id:%s not found, skipping', comment_id) # comment already deleted in another call probably return True - owner = (comment.author.user_id == c.rhodecode_user.user_id) is_repo_admin = h.HasRepoPermissionAny('repository.admin')(c.repo_name) - if h.HasPermissionAny('hg.admin')() or is_repo_admin or owner: + super_admin = h.HasPermissionAny('hg.admin')() + comment_owner = (comment.author.user_id == c.rhodecode_user.user_id) + is_repo_comment = comment.repo.repo_name == c.repo_name + comment_repo_admin = is_repo_admin and is_repo_comment + + if super_admin or comment_owner or comment_repo_admin: CommentsModel().delete(comment=comment, user=c.rhodecode_user) Session().commit() return True diff --git a/rhodecode/controllers/pullrequests.py b/rhodecode/controllers/pullrequests.py --- a/rhodecode/controllers/pullrequests.py +++ b/rhodecode/controllers/pullrequests.py @@ -987,25 +987,30 @@ class PullrequestsController(BaseRepoCon @auth.CSRFRequired() @jsonify def delete_comment(self, repo_name, comment_id): - return self._delete_comment(comment_id) + comment = ChangesetComment.get_or_404(safe_int(comment_id)) + if not comment: + log.debug('Comment with id:%s not found, skipping', comment_id) + # comment already deleted in another call probably + return True - def _delete_comment(self, comment_id): - comment_id = safe_int(comment_id) - co = ChangesetComment.get_or_404(comment_id) - if co.pull_request.is_closed(): + if comment.pull_request.is_closed(): # don't allow deleting comments on closed pull request raise HTTPForbidden() - is_owner = co.author.user_id == c.rhodecode_user.user_id is_repo_admin = h.HasRepoPermissionAny('repository.admin')(c.repo_name) - if h.HasPermissionAny('hg.admin')() or is_repo_admin or is_owner: - old_calculated_status = co.pull_request.calculated_review_status() - CommentsModel().delete(comment=co, user=c.rhodecode_user) + super_admin = h.HasPermissionAny('hg.admin')() + comment_owner = comment.author.user_id == c.rhodecode_user.user_id + is_repo_comment = comment.repo.repo_name == c.repo_name + comment_repo_admin = is_repo_admin and is_repo_comment + + if super_admin or comment_owner or comment_repo_admin: + old_calculated_status = comment.pull_request.calculated_review_status() + CommentsModel().delete(comment=comment, user=c.rhodecode_user) Session().commit() - calculated_status = co.pull_request.calculated_review_status() + calculated_status = comment.pull_request.calculated_review_status() if old_calculated_status != calculated_status: PullRequestModel()._trigger_pull_request_hook( - co.pull_request, c.rhodecode_user, 'review_status_change') + comment.pull_request, c.rhodecode_user, 'review_status_change') return True else: raise HTTPForbidden() diff --git a/rhodecode/tests/functional/test_pullrequests.py b/rhodecode/tests/functional/test_pullrequests.py --- a/rhodecode/tests/functional/test_pullrequests.py +++ b/rhodecode/tests/functional/test_pullrequests.py @@ -1060,6 +1060,17 @@ class TestPullrequestsControllerDelete(o response.mustcontain('id="delete_pullrequest"') response.mustcontain(no=['Confirm to delete this pull request']) + def test_delete_comment_returns_404_if_comment_does_not_exist( + self, autologin_user, pr_util, user_admin): + + pull_request = pr_util.create_pull_request( + author=user_admin.username, enable_notifications=False) + + self.app.get(url( + controller='pullrequests', action='delete_comment', + repo_name=pull_request.target_repo.scm_instance().name, + comment_id=1024404), status=404) + def assert_pull_request_status(pull_request, expected_status): status = ChangesetStatusModel().calculated_review_status( @@ -1081,13 +1092,3 @@ def test_redirects_to_repo_summary_for_s # URL adds leading slash and path doesn't have it assert redirected.request.path == summary_url - - -def test_delete_comment_returns_404_if_comment_does_not_exist(pylonsapp): - # TODO: johbo: Global import not possible because models.forms blows up - from rhodecode.controllers.pullrequests import PullrequestsController - controller = PullrequestsController() - patcher = mock.patch( - 'rhodecode.model.db.BaseModel.get', return_value=None) - with pytest.raises(HTTPNotFound), patcher: - controller._delete_comment(1)