diff --git a/bin/review-rot b/bin/review-rot index 2b3ff91..8d08703 100755 --- a/bin/review-rot +++ b/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]) diff --git a/requirements.txt b/requirements.txt index 1820da7..8208133 100644 --- a/requirements.txt +++ b/requirements.txt @@ -5,4 +5,4 @@ dateutils python-gitlab>=1.6.0 six Jinja2 -phabricator>=0.7.0' \ No newline at end of file +phabricator>=0.7.0 diff --git a/reviewrot/__init__.py b/reviewrot/__init__.py index af0eaaa..e74fe62 100644 --- a/reviewrot/__init__.py +++ b/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))] diff --git a/reviewrot/gerritstack.py b/reviewrot/gerritstack.py index 24920b6..fc1e676 100644 --- a/reviewrot/gerritstack.py +++ b/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) diff --git a/reviewrot/githubstack.py b/reviewrot/githubstack.py index d97fb56..6ec5a22 100644 --- a/reviewrot/githubstack.py +++ b/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, diff --git a/reviewrot/gitlabstack.py b/reviewrot/gitlabstack.py index d711177..cd607a3 100644 --- a/reviewrot/gitlabstack.py +++ b/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"], diff --git a/reviewrot/pagurestack.py b/reviewrot/pagurestack.py index e322770..a125c9b 100644 --- a/reviewrot/pagurestack.py +++ b/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) diff --git a/reviewrot/utils.py b/reviewrot/utils.py new file mode 100644 index 0000000..0a15dc8 --- /dev/null +++ b/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) diff --git a/test/github_tests/mock_github.py b/test/github_tests/mock_github.py index 025fb25..b1e6b35 100644 --- a/test/github_tests/mock_github.py +++ b/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: diff --git a/test/github_tests/test_github.py b/test/github_tests/test_github.py index bb1ec05..9ff79a3 100644 --- a/test/github_tests/test_github.py +++ b/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) diff --git a/test/gitlab_tests/test_gitlab.py b/test/gitlab_tests/test_gitlab.py index 77071c0..fe1b628 100644 --- a/test/gitlab_tests/test_gitlab.py +++ b/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) diff --git a/tox.ini b/tox.ini index a3a1716..0c1ea9d 100644 --- a/tox.ini +++ b/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