From aaa2d1b5f5640795fcf28a46fd080c2905d52cdb Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 25 2018 20:56:46 +0000 Subject: [PATCH 1/2] Change the way votes are recorded Before this commit there would only be one vote per person and we would sum up the values for each, so if you voted once -1 and once +1 your action would result in a 0. With this change, we record your last vote, so if you voted once -1 and then +1, your vote would lead to a +1. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/model.py b/pagure/lib/model.py index aaae389..4785d14 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -2036,19 +2036,18 @@ class PullRequest(BASE): An user can only give one +1 and one -1. """ - positive = set() - negative = set() + votes = {} for comment in self.discussion: for word in ["+1", ":thumbsup:"]: if word in comment.comment: - positive.add(comment.user_id) + votes[comment.user_id] = 1 break for word in ["-1", ":thumbsdown:"]: if word in comment.comment: - negative.add(comment.user_id) + votes[comment.user_id] = -1 break - return len(positive) - len(negative) + return sum(votes.values()) @property def remote(self): diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py index cdb2033..e9b7507 100644 --- a/tests/test_pagure_lib.py +++ b/tests/test_pagure_lib.py @@ -3106,7 +3106,7 @@ class PagureLibtests(tests.Modeltests): self.assertEqual(msg, 'Comment added') self.assertEqual(len(request.discussion), 3) - self.assertEqual(request.score, 1) + self.assertEqual(request.score, 2) def test_add_group(self): """ Test the add_group method of pagure.lib.query. """ From 2233c1cd627de203135f27234b94f3862694fefe Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 25 2018 20:56:46 +0000 Subject: [PATCH 2/2] Include whether the PR passed the threshold or not in the API data Basically, if the project requires a minimal score to merge PR, the data returned by the API will be either a ``True`` or ``False``. If the project does not enforce any score to merge the PR, the data returned by the API will be ``None``. Fixes https://pagure.io/pagure/issue/4014 Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/model.py b/pagure/lib/model.py index 4785d14..79b7ce6 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -2050,6 +2050,21 @@ class PullRequest(BASE): return sum(votes.values()) @property + def threshold_reached(self): + """ Return whether the pull-request has reached the threshold above + which it is allowed to be merged, if the project requests a minimal + score on pull-request, otherwise returns None. + + """ + threshold = self.project.settings.get( + "Minimum_score_to_merge_pull-request", -1 + ) + if threshold is None or threshold < 0: + return None + else: + return int(self.score) >= int(threshold) + + @property def remote(self): """ Return whether the current PullRequest is a remote pull-request or not. @@ -2095,6 +2110,7 @@ class PullRequest(BASE): else None, "initial_comment": self.initial_comment, "cached_merge_status": self.merge_status or "unknown", + "threshold_reached": self.threshold_reached, } comments = [] diff --git a/tests/test_pagure_flask_api_fork.py b/tests/test_pagure_flask_api_fork.py index 0964ce2..d68236c 100644 --- a/tests/test_pagure_flask_api_fork.py +++ b/tests/test_pagure_flask_api_fork.py @@ -387,6 +387,7 @@ class PagureFlaskApiForktests(tests.Modeltests): } }, "status": "Open", + "threshold_reached": None, "title": "test pull-request", "uid": "1431414800", "updated_on": "1431414800", @@ -605,6 +606,7 @@ class PagureFlaskApiForktests(tests.Modeltests): } }, "status": "Open", + "threshold_reached": None, "title": "test pull-request", "uid": "1431414800", "updated_on": "1431414800", @@ -765,6 +767,7 @@ class PagureFlaskApiForktests(tests.Modeltests): } }, "status": "Open", + "threshold_reached": None, "title": "test pull-request", "uid": uid, "updated_on": "1431414800", @@ -2583,6 +2586,7 @@ class PagureFlaskApiForktests(tests.Modeltests): 'url_path': 'test', 'user': {'fullname': 'PY C', 'name': 'pingou'}}, 'status': 'Open', + 'threshold_reached': None, 'title': 'Test PR', 'uid': 'e8b68df8711648deac67c3afed15a798', 'updated_on': '1516348115', @@ -2695,6 +2699,7 @@ class PagureFlaskApiForktests(tests.Modeltests): 'url_path': 'test', 'user': {'fullname': 'PY C', 'name': 'pingou'}}, 'status': 'Open', + 'threshold_reached': None, 'title': 'Test PR', 'uid': 'e8b68df8711648deac67c3afed15a798', 'updated_on': '1516348115', @@ -2872,6 +2877,362 @@ class PagureFlaskApiForkPRDiffStatstests(tests.Modeltests): } ) +class PagureApiThresholdReachedTests(tests.Modeltests): + """ Test the behavior of the threshold_reached value returned by the API. + """ + maxDiff = None + + def _clean_data(self, data): + data['project']['date_created'] = '1516348115' + data['project']['date_modified'] = '1516348115' + data['repo_from']['date_created'] = '1516348115' + data['repo_from']['date_modified'] = '1516348115' + data['uid'] = 'e8b68df8711648deac67c3afed15a798' + data['commit_start'] = '114f1b468a5f05e635fcb6394273f3f907386eab' + data['commit_stop'] = '114f1b468a5f05e635fcb6394273f3f907386eab' + data['date_created'] = '1516348115' + data['last_updated'] = '1516348115' + data['updated_on'] = '1516348115' + data['comments'] = [] # Let's not check the comments + return data + + @patch('pagure.lib.notify.send_email', MagicMock(return_value=True)) + def setUp(self): + """ Set up the environment for the tests. """ + super(PagureApiThresholdReachedTests, self).setUp() + + tests.create_projects(self.session) + tests.create_projects_git(os.path.join(self.path, 'repos'), bare=True) + tests.create_projects_git(os.path.join(self.path, 'requests'), + bare=True) + tests.add_readme_git_repo(os.path.join(self.path, 'repos', 'test.git')) + tests.add_commit_git_repo(os.path.join(self.path, 'repos', 'test.git'), + branch='test') + tests.create_tokens(self.session) + tests.create_tokens_acl(self.session) + + # Add a token for user `foo` + item = pagure.lib.model.Token( + id='aaabbbcccddd_foo', + user_id=2, + project_id=1, + expiration=datetime.datetime.utcnow() + + datetime.timedelta(days=30) + ) + self.session.add(item) + self.session.commit() + tests.create_tokens_acl(self.session, token_id="aaabbbcccddd_foo") + + # Add a minimal required score: + repo = pagure.lib.query._get_project(self.session, 'test') + settings = repo.settings + settings['Minimum_score_to_merge_pull-request'] = 2 + repo.settings = settings + self.session.add(repo) + self.session.commit() + + headers = {'Authorization': 'token aaabbbcccddd'} + data = { + 'title': 'Test PR', + 'initial_comment': 'Nothing much, the changes speak for themselves', + 'branch_to': 'master', + 'branch_from': 'test', + } + + output = self.app.post( + '/api/0/test/pull-request/new', headers=headers, data=data) + self.assertEqual(output.status_code, 200) + + self.expected_data = { + 'assignee': None, + 'branch': 'master', + 'branch_from': 'test', + 'cached_merge_status': 'unknown', + 'closed_at': None, + 'closed_by': None, + 'comments': [], + 'commit_start': '114f1b468a5f05e635fcb6394273f3f907386eab', + 'commit_stop': '114f1b468a5f05e635fcb6394273f3f907386eab', + 'date_created': '1516348115', + 'id': 1, + 'initial_comment': 'Nothing much, the changes speak for themselves', + 'last_updated': '1516348115', + 'project': {'access_groups': {'admin': [], + 'commit': [], + 'ticket':[]}, + 'access_users': {'admin': [], + 'commit': [], + 'owner': ['pingou'], + 'ticket': []}, + 'close_status': ['Invalid', + 'Insufficient data', + 'Fixed', + 'Duplicate'], + 'custom_keys': [], + 'date_created': '1516348115', + 'date_modified': '1516348115', + 'description': 'test project #1', + 'fullname': 'test', + 'id': 1, + 'milestones': {}, + 'name': 'test', + 'namespace': None, + 'parent': None, + 'priorities': {}, + 'tags': [], + 'url_path': 'test', + 'user': {'fullname': 'PY C', 'name': 'pingou'}}, + 'remote_git': None, + 'repo_from': {'access_groups': {'admin': [], + 'commit': [], + 'ticket': []}, + 'access_users': {'admin': [], + 'commit': [], + 'owner': ['pingou'], + 'ticket': []}, + 'close_status': ['Invalid', + 'Insufficient data', + 'Fixed', + 'Duplicate'], + 'custom_keys': [], + 'date_created': '1516348115', + 'date_modified': '1516348115', + 'description': 'test project #1', + 'fullname': 'test', + 'id': 1, + 'milestones': {}, + 'name': 'test', + 'namespace': None, + 'parent': None, + 'priorities': {}, + 'tags': [], + 'url_path': 'test', + 'user': {'fullname': 'PY C', 'name': 'pingou'}}, + 'status': 'Open', + 'threshold_reached': None, + 'title': 'Test PR', + 'uid': 'e8b68df8711648deac67c3afed15a798', + 'updated_on': '1516348115', + 'user': {'fullname': 'PY C', 'name': 'pingou'} + } + + def test_api_pull_request_no_comments(self): + """ Check the value of threshold_reach when the PR has no comments. + """ + + # Check the PR with 0 comment: + output = self.app.get('/api/0/test/pull-request/1') + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + data = self._clean_data(data) + self.expected_data["threshold_reached"] = False + self.assertDictEqual(data, self.expected_data) + + def test_api_pull_request_one_comments(self): + """ Check the value of threshold_reach when the PR has one comment. + """ + # Check the PR with 1 comment: + headers = {'Authorization': 'token aaabbbcccddd'} + data = { + 'comment': 'This is a very interesting solution :thumbsup:', + } + output = self.app.post( + '/api/0/test/pull-request/1/comment', data=data, headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual( + data, + {'message': 'Comment added'} + ) + + output = self.app.get('/api/0/test/pull-request/1') + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + data = self._clean_data(data) + self.expected_data["threshold_reached"] = False + self.assertDictEqual(data, self.expected_data) + + def test_api_pull_request_two_comments_one_person(self): + """ Check the value of threshold_reach when the PR has two comments + but from the same person. + """ + # Add two comments from the same user: + headers = {'Authorization': 'token aaabbbcccddd'} + data = { + 'comment': 'This is a very interesting solution :thumbsup:', + } + output = self.app.post( + '/api/0/test/pull-request/1/comment', data=data, headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual( + data, + {'message': 'Comment added'} + ) + + headers = {'Authorization': 'token aaabbbcccddd'} + data = { + 'comment': 'Indeed it is :thumbsup:', + } + output = self.app.post( + '/api/0/test/pull-request/1/comment', data=data, headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual( + data, + {'message': 'Comment added'} + ) + + output = self.app.get('/api/0/test/pull-request/1') + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + data = self._clean_data(data) + self.expected_data["threshold_reached"] = False + self.assertDictEqual(data, self.expected_data) + + def test_api_pull_request_two_comments_two_persons(self): + """ Check the value of threshold_reach when the PR has two comments + from two different persons. + """ + # Add two comments from two users: + headers = {'Authorization': 'token aaabbbcccddd'} + data = { + 'comment': 'This is a very interesting solution :thumbsup:', + } + output = self.app.post( + '/api/0/test/pull-request/1/comment', data=data, headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual( + data, + {'message': 'Comment added'} + ) + + headers = {'Authorization': 'token aaabbbcccddd_foo'} + data = { + 'comment': 'Indeed it is :thumbsup:', + } + output = self.app.post( + '/api/0/test/pull-request/1/comment', data=data, headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual( + data, + {'message': 'Comment added'} + ) + + output = self.app.get('/api/0/test/pull-request/1') + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + data = self._clean_data(data) + data['comments'] = [] # Let's not check the comments + self.expected_data["threshold_reached"] = True + self.assertDictEqual(data, self.expected_data) + + def test_api_pull_request_three_comments_two_persons_changed_to_no(self): + """ Check the value of threshold_reach when the PR has three + comments from two person among which one changed their mind from + +1 to -1. + """ + # Add three comments from two users: + headers = {'Authorization': 'token aaabbbcccddd'} + data = { + 'comment': 'This is a very interesting solution :thumbsup:', + } + output = self.app.post( + '/api/0/test/pull-request/1/comment', data=data, headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual( + data, + {'message': 'Comment added'} + ) + + headers = {'Authorization': 'token aaabbbcccddd_foo'} + data = { + 'comment': 'Indeed it is :thumbsup:', + } + output = self.app.post( + '/api/0/test/pull-request/1/comment', data=data, headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual( + data, + {'message': 'Comment added'} + ) + + data = { + 'comment': 'Nevermind the bug is elsewhere in fact :thumbsdown:', + } + output = self.app.post( + '/api/0/test/pull-request/1/comment', data=data, headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual( + data, + {'message': 'Comment added'} + ) + + output = self.app.get('/api/0/test/pull-request/1') + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + data = self._clean_data(data) + data['comments'] = [] # Let's not check the comments + self.expected_data["threshold_reached"] = False + self.assertDictEqual(data, self.expected_data) + + def test_api_pull_request_three_comments_two_persons_changed_to_yes(self): + """ Check the value of threshold_reach when the PR has three + comments from two person among which one changed their mind from + -1 to +1 + """ + # Add three comments from two users: + headers = {'Authorization': 'token aaabbbcccddd'} + data = { + 'comment': 'This is a very interesting solution :thumbsup:', + } + output = self.app.post( + '/api/0/test/pull-request/1/comment', data=data, headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual( + data, + {'message': 'Comment added'} + ) + + headers = {'Authorization': 'token aaabbbcccddd_foo'} + data = { + 'comment': 'I think the bug is elsewhere :thumbsdown:', + } + output = self.app.post( + '/api/0/test/pull-request/1/comment', data=data, headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual( + data, + {'message': 'Comment added'} + ) + + data = { + 'comment': 'Nevermind it is here :thumbsup:', + } + output = self.app.post( + '/api/0/test/pull-request/1/comment', data=data, headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual( + data, + {'message': 'Comment added'} + ) + + output = self.app.get('/api/0/test/pull-request/1') + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + data = self._clean_data(data) + data['comments'] = [] # Let's not check the comments + self.expected_data["threshold_reached"] = True + self.assertDictEqual(data, self.expected_data) + if __name__ == '__main__': unittest.main(verbosity=2) diff --git a/tests/test_pagure_flask_api_ui_private_repo.py b/tests/test_pagure_flask_api_ui_private_repo.py index c4511ad..9ff217d 100644 --- a/tests/test_pagure_flask_api_ui_private_repo.py +++ b/tests/test_pagure_flask_api_ui_private_repo.py @@ -1594,6 +1594,7 @@ class PagurePrivateRepotest(tests.Modeltests): } }, "status": "Open", + "threshold_reached": None, "title": "test pull-request", "uid": "1431414800", "updated_on": "1431414800", @@ -1723,6 +1724,7 @@ class PagurePrivateRepotest(tests.Modeltests): } }, "status": "Open", + "threshold_reached": None, "title": "test pull-request", "uid": "1431414800", "updated_on": "1431414800", diff --git a/tests/test_pagure_lib_git.py b/tests/test_pagure_lib_git.py index 281545f..88637ec 100644 --- a/tests/test_pagure_lib_git.py +++ b/tests/test_pagure_lib_git.py @@ -1697,7 +1697,7 @@ new file mode 100644 index 0000000..60f7480 --- /dev/null +++ b/456 -@@ -0,0 +1,143 @@ +@@ -0,0 +1,144 @@ +{ + "assignee": null, + "branch": "master", @@ -1828,6 +1828,7 @@ index 0000000..60f7480 + } + }, + "status": "Open", ++ "threshold_reached": null, + "title": "test PR", + "uid": "foobar", + "updated_on": null,