Repository: smarkets/marge-bot

Issue:
Improve error message when assigned to MR with unresolved threads
Currently, the message posted by marge on when attempting to merge an MR with outstanding threads is only:

> I couldn't merge this branch: Merge request was rejected by GitLab: 'Branch cannot be merged'

and the log message is:

>  | 2021-07-08 11:01:26,671 WARNING I couldn't merge this branch: Merge request was rejected by GitLab: 'Branch cannot be merged'

This could be improved by extending 

    def ensure_mergeable_mr(self, merge_request):

by retrieving all notes in the mr, and checking that all notes that are resolvable are also resolved.

Discussion:

Patch:
diff --git a/marge/bot.py b/marge/bot.py
index 0996962..465e960 100644
--- a/marge/bot.py
+++ b/marge/bot.py
@@ -139,7 +139,7 @@ class Bot:
             [mr.web_url for mr in source_filtered_mrs]
         )
         source_filtered_out = set(filtered_mrs) - set(source_filtered_mrs)
-        if filtered_out:
+        if source_filtered_out:
             log.debug(
                 'MRs that do not match source_branch_regexp: %s',
                 [mr.web_url for mr in source_filtered_out]
diff --git a/marge/job.py b/marge/job.py
index 696aefa..9b33aae 100644
--- a/marge/job.py
+++ b/marge/job.py
@@ -55,6 +55,9 @@ class MergeJob:
                 '(have: {0.approver_usernames} missing: {0.approvals_left})'.format(approvals)
             )
 
+        if not merge_request.blocking_discussions_resolved:
+            raise CannotMerge("Sorry, I can't merge requests which have unresolved discussions!")
+
         state = merge_request.state
         if state not in ('opened', 'reopened', 'locked'):
             if state in ('merged', 'closed'):
diff --git a/marge/merge_request.py b/marge/merge_request.py
index 98a14eb..030b79b 100644
--- a/marge/merge_request.py
+++ b/marge/merge_request.py
@@ -151,6 +151,10 @@ class MergeRequest(gitlab.Resource):
     def web_url(self):
         return self.info['web_url']
 
+    @property
+    def blocking_discussions_resolved(self):
+        return self.info['blocking_discussions_resolved']
+
     @property
     def force_remove_source_branch(self):
         return self.info['force_remove_source_branch']


Test patch:
diff --git a/tests/gitlab_api_mock.py b/tests/gitlab_api_mock.py
index a682d52..012fb64 100644
--- a/tests/gitlab_api_mock.py
+++ b/tests/gitlab_api_mock.py
@@ -58,6 +58,7 @@ class MockLab:  # pylint: disable=too-few-public-methods
             'force_remove_source_branch': True,
             'target_branch': 'master',
             'work_in_progress': False,
+            'blocking_discussions_resolved': True,
             'web_url': 'http://git.example.com/group/project/merge_request/666',
         }
         if merge_request_options is not None:
diff --git a/tests/test_job.py b/tests/test_job.py
index 77f5961..4818b29 100644
--- a/tests/test_job.py
+++ b/tests/test_job.py
@@ -130,6 +130,20 @@ class TestJob:
 
         assert exc_info.value.reason == "Sorry, I can't merge requests marked as Work-In-Progress!"
 
+    def test_ensure_mergeable_mr_unresolved_discussion(self):
+        merge_job = self.get_merge_job()
+        merge_request = self._mock_merge_request(
+            assignee_ids=[merge_job._user.id],
+            state='opened',
+            work_in_progress=False,
+            blocking_discussions_resolved=False,
+        )
+        merge_request.fetch_approvals.return_value.sufficient = True
+        with pytest.raises(CannotMerge) as exc_info:
+            merge_job.ensure_mergeable_mr(merge_request)
+
+        assert exc_info.value.reason == "Sorry, I can't merge requests which have unresolved discussions!"
+
     def test_ensure_mergeable_mr_squash_and_trailers(self):
         merge_job = self.get_merge_job(options=MergeJobOptions.default(add_reviewers=True))
         merge_request = self._mock_merge_request(