Add support for Draft flag on GitHub

bin/review-rot

@@ -206,7 +206,7 @@ def main():
         # output maximum 20 merge requests
         N = 20
         for i, result in enumerate(sorted_results[:N]):
-            irc_bot.send_msg(result.format(style="irc", i=i, N=N))
+            irc_bot.send_msg(result.format(style="irc", i=i, n=N))
 
         if len(sorted_results) > 20:
             irc_bot.send_msg(
@@ -224,7 +224,7 @@ def main():
         print(report_prefixes[formatting])
         for i, result in enumerate(sorted_results):
             print(result.format(
-                style=formatting, i=i, N=len(results),
+                style=formatting, i=i, n=len(results),
                 show_last_comment=arguments.get('show_last_comment')))
         print(report_suffixes[formatting])
 

requirements.txt

@@ -5,4 +5,4 @@ dateutils
 python-gitlab>=1.6.0
 six
 Jinja2
-phabricator>=0.7.0'
+phabricator>=0.7.0

reviewrot/__init__.py

@@ -15,6 +15,7 @@
 from reviewrot.gitlabstack import GitlabService
 from reviewrot.pagurestack import PagureService
 from reviewrot.phabricatorstack import PhabricatorService
+from reviewrot.utils import is_wip
 from six import iteritems
 from six.moves import input
 import yaml
@@ -90,7 +91,6 @@ def get_arguments(cli_arguments, config):
             command_line_args.get(argument) is None
             or command_line_args.get(argument) is False
         ):
-
             config_value = config_arguments.get(argument)
             if is_valid_choice(argument, config_value):
                 parsed_arguments[argument] = config_value
@@ -400,7 +400,7 @@ def dict_constructor(loader, node):
     #  format the output to print a blank scalar rather than null
     def represent_none(self, _):
         """TODO: docstring goes here."""
-        return self.represent_scalar("tag:yaml.org,2002:null", u"")
+        return self.represent_scalar("tag:yaml.org,2002:null", "")
 
     yaml.add_representer(type(None), represent_none)
 
@@ -421,12 +421,4 @@ def remove_wip(results):
         res (list): list of BaseReview instances with WIP
                     reviews removed
     """
-    res = []
-    for result in results:
-        match = re.match(
-            r"^(\[WIP\]\s*|WIP:\s*|WIP\s+|Draft:)+\s*", str(result.title), re.IGNORECASE
-        )
-        if not match:
-            res.append(result)
-
-    return res
+    return [result for result in results if not is_wip(str(result.title))]

reviewrot/gerritstack.py

@@ -265,7 +265,6 @@ def format_response(self, decoded_responses, age, show_last_comment):
         """
         res_ = []
         for decoded_response in decoded_responses:
-
             time_format = "%Y-%m-%d %H:%M:%S.%f"
             created_date = datetime.strptime(
                 decoded_response["created"][:-3], time_format
@@ -279,7 +278,16 @@ def format_response(self, decoded_responses, age, show_last_comment):
                 self.url, str(decoded_response["id"])
             )
 
-            comments_response = self._call_api(comments_request_url)
+            try:
+                comments_response = self._call_api(comments_request_url)
+            except requests.exceptions.HTTPError as e:
+                if e.response.status_code == 404:
+                    log.warning(
+                        "Reviews with multiple changes are unsupported; got error: %s",
+                        e.response.text,
+                    )
+                    continue
+                raise
 
             last_comment = self.get_last_comment(comments_response)
 

reviewrot/githubstack.py

@@ -4,6 +4,7 @@
 from github import Github
 from github.GithubException import UnknownObjectException
 from reviewrot.basereview import BaseReview, BaseService, LastComment
+from reviewrot.utils import is_wip
 
 log = logging.getLogger(__name__)
 
@@ -24,7 +25,7 @@ def request_reviews(
         show_last_comment=None,
         token=None,
         host=None,
-        **kwargs
+        **kwargs,
     ):
         """
         Creates a github object.
@@ -159,9 +160,13 @@ def get_reviews(self, uname, repo_name, age=None, show_last_comment=None):
                     )
                     continue
 
+            title = pr.title
+            if pr.draft and not is_wip(title):
+                title = f"WIP: {title}"
+
             res = GithubReview(
                 user=pr.user.login,
-                title=pr.title,
+                title=title,
                 url=pr.html_url,
                 time=pr.created_at,
                 updated_time=pr.updated_at,

reviewrot/gitlabstack.py

@@ -74,12 +74,17 @@ def request_reviews(
                 raise Exception(
                     "Project %s not found for user %s" % (repo_name, user_name)
                 )
+
             # get merge requests for specified username and project name
             res = self.get_reviews(
                 uname=user_name,
                 project=project,
                 age=age,
                 show_last_comment=show_last_comment,
+                image=(
+                    project.avatar_url
+                    or gl.groups.get(project.namespace["id"]).avatar_url
+                ),
             )
             # extend in case of a non empty result
             if res:
@@ -100,16 +105,20 @@ def request_reviews(
 
             # get merge requests for all projects for specified group
             for group_project in group_projects:
-
                 project = gl.projects.get(group_project.id)
-                res = self.get_reviews(uname=user_name, project=project, age=age)
+                res = self.get_reviews(
+                    uname=user_name,
+                    project=project,
+                    age=age,
+                    image=project.avatar_url or group.avatar_url,
+                )
 
                 # extend in case of a non empty result
                 if res:
                     response.extend(res)
         return response
 
-    def get_reviews(self, uname, project, age=None, show_last_comment=None):
+    def get_reviews(self, uname, project, age=None, show_last_comment=None, image=None):
         """
         Fetches merge requests for specified username(groupname) and repo(project) name.
 
@@ -146,7 +155,6 @@ def get_reviews(self, uname, project, age=None, show_last_comment=None):
             log.debug("No open merge requests found for %s/%s ", uname, project.name)
         res_ = []
         for mr in merge_requests:
-
             last_comment = self.get_last_comment(mr)
 
             try:
@@ -193,9 +201,7 @@ def get_reviews(self, uname, project, age=None, show_last_comment=None):
                 time=mr_date,
                 updated_time=mr_updated_date,
                 comments=mr.user_notes_count,
-                # XXX - I don't know how to find gitlab avatars
-                # for now.  Can we figure this out later?
-                image=GitlabReview.logo,
+                image=image or GitlabReview.logo,
                 last_comment=last_comment,
                 project_name=project.name,
                 project_url=project.web_url,
@@ -216,7 +222,7 @@ def get_last_comment(self, mr):
             last comment (LastComment): Returns namedtuple LastComment
             with data related to last comment
         """
-        for note in mr.notes.list():
+        for note in mr.notes.list(iterator=True):
             if not note.system:
                 return LastComment(
                     author=note.author["username"],

reviewrot/pagurestack.py

@@ -216,7 +216,7 @@ def _avatar(self, username, ssl_verify=True):
                 "Fallback to construct based on username"
             )
             query = urllib.parse.urlencode(avatar_query)
-            openid = u"http://%s.id.fedoraproject.org/" % username
+            openid = "http://%s.id.fedoraproject.org/" % username
             idx = hashlib.sha256(openid.encode("utf-8")).hexdigest()
             avatar_url = "https://seccdn.libravatar.org/avatar/%s?%s" % (idx, query)
 

reviewrot/utils.py

@@ -0,0 +1,19 @@
+"""Simple helper functions."""
+import re
+
+WIP_REGEX_PATTERN = r"^(\[(WIP|Draft)\]|(WIP|Draft))(:|\s)?"
+
+
+def is_wip(title):
+    """
+    Returns True only if title indicates work-in-progress.
+
+    Args:
+        title (str): Review/pull-request/merge-request title
+
+    Returns:
+        True only if the title indicates work-in-progress,
+        False otherwise
+    """
+    match = re.match(WIP_REGEX_PATTERN, title, re.IGNORECASE)
+    return bool(match)

test/github_tests/mock_github.py

@@ -63,6 +63,7 @@ class MockPull:
     updated_at = "dummy_update"
     review_comments = "dummy_comments"
     comments = "dummy_comments"
+    draft = False
 
 
 class MockRepo:

test/github_tests/test_github.py

@@ -370,3 +370,53 @@ def test_request_reviews_without_repo(self, mock_get_reviews, mock_github_patch)
         mock_github_instance.get_user.assert_called_with("dummy_user")
         mock_github_patch.assert_called_with("dummy_token")
         self.assertEqual(["1"], response)
+
+    @patch(PATH + "GithubService.check_request_state")
+    @patch(PATH + "GithubService.get_last_comment")
+    def test_get_reviews_with_draft_status(
+        self,
+        mock_last_comment,
+        mock_check_request_state,
+    ):
+        """
+        Tests get_reviews() function.
+
+        Where getting the pull requests returns single PR flagged as draft.
+        """
+        # Set up mock return values and side effects
+        mock_uname = MagicMock()
+        mock_repo = MagicMock()
+        mock_pr = mock_github.MockPull()
+        mock_pr.draft = True
+        mock_repo.get_pulls.return_value = [mock_pr]
+        mock_uname.get_repo.return_value = mock_repo
+        mock_check_request_state.return_value = True
+
+        for title, expected_title in [
+            ("dummy_title", "WIP: dummy_title"),
+            ("WIP: dummy_title", "WIP: dummy_title"),
+            ("Draft: dummy_title", "Draft: dummy_title"),
+            ("draft: dummy_title", "draft: dummy_title"),
+            ("[WIP] dummy_title", "[WIP] dummy_title"),
+            ("[wip] dummy_title", "[wip] dummy_title"),
+        ]:
+            with self.subTest():
+                mock_pr.title = title
+
+                # Call function
+                response = GithubService().get_reviews(
+                    uname=mock_uname,
+                    repo_name="dummy_repo",
+                    age=None,
+                    show_last_comment=None,
+                )
+
+                # Validate function calls and response
+                mock_uname.get_repo.assert_called_with("dummy_repo")
+                mock_check_request_state.assert_called_with(
+                    "dummy_createdAt",
+                    None,
+                )
+                self.assertEqual(1, len(response))
+                actual_review = response[0]
+                self.assertEqual(expected_title, actual_review.title)

test/gitlab_tests/test_gitlab.py

@@ -318,13 +318,14 @@ def test_get_reviews_no_age_with_last_comment(
     def test_request_reviews_ssl_error_no_repo(self, mock_get_reviews, mock_gitlab):
         """Test 'request_reviews' function where there is an SSL error and no repos."""
         # Set up mock return values and side effects
+        mock_gitlab_project = MagicMock()
         mock_gitlab_group = MagicMock(name="mock_gitlab_group")
         mock_gitlab_group.id = 1
         mock_gitlab_group_projects = MagicMock()
         mock_gitlab_group_projects.projects.list.return_value = [mock_gitlab_group]
         mock_gitlab_instance = MagicMock(name="mock_gitlab_instance")
         mock_gitlab_instance.groups.get.return_value = mock_gitlab_group_projects
-        mock_gitlab_instance.projects.get.return_value = "dummy_project"
+        mock_gitlab_instance.projects.get.return_value = mock_gitlab_project
         mock_gitlab_instance.auth.side_effect = SSLError
         mock_get_reviews.return_value = "1"
         mock_gitlab.return_value = mock_gitlab_instance
@@ -343,7 +344,10 @@ def test_request_reviews_ssl_error_no_repo(self, mock_get_reviews, mock_gitlab):
         mock_gitlab_instance.groups.get.assert_called_with("dummy_user")
         mock_gitlab_instance.projects.get.assert_called_with(1)
         mock_get_reviews.assert_called_with(
-            uname="dummy_user", project="dummy_project", age=None
+            uname="dummy_user",
+            project=mock_gitlab_project,
+            age=None,
+            image=mock_gitlab_project.avatar_url,
         )
         self.assertEqual(["1"], response)
 
@@ -411,7 +415,9 @@ def test_request_reviews_with_repo_success(self, mock_get_reviews, mock_gitlab):
         """Tests 'request_reviews' function where we have repos and no errors."""
         # Set up mock return values and side effects
         mock_gitlab_instance = MagicMock(name="mock_gitlab_instance")
-        mock_gitlab_instance.projects.get.return_value = "dummy_project"
+        mock_gitlab_project = MagicMock()
+        mock_gitlab_instance.projects.get.return_value = mock_gitlab_project
+        mock_gitlab_instance.projects.namespace.return_value = {"id": 123}
         mock_gitlab.return_value = mock_gitlab_instance
         mock_get_reviews.return_value = "1"
 
@@ -431,8 +437,9 @@ def test_request_reviews_with_repo_success(self, mock_get_reviews, mock_gitlab):
         mock_gitlab_instance.projects.get.assert_called_with("dummy_user/dummy_repo")
         mock_get_reviews.assert_called_with(
             uname="dummy_user",
-            project="dummy_project",
+            project=mock_gitlab_project,
             age=None,
             show_last_comment=None,
+            image=mock_gitlab_project.avatar_url,
         )
         self.assertEqual(["1"], response)

tox.ini

@@ -28,6 +28,7 @@ commands =
         reviewrot/ test/
 
 [testenv:black]
+skip_install = true
 deps =
     black
 commands =
@@ -36,6 +37,12 @@ commands =
         --diff \
         reviewrot/ test/
 
+[testenv:black-format]
+skip_install = true
+deps =
+    black
+commands =
+    black reviewrot/ test/
 
 [testenv:clean]
 deps = coverage