From 56c3184debf62fba0b23b083d8fa4c3ae2df1382 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 16 2020 10:47:00 +0000 Subject: [PATCH 1/4] When returning the commits flags in the API, returned them by update date Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/query.py b/pagure/lib/query.py index 64612de..1396615 100644 --- a/pagure/lib/query.py +++ b/pagure/lib/query.py @@ -1619,7 +1619,7 @@ def get_commit_flag(session, project, commit_hash): session.query(model.CommitFlag) .filter(model.CommitFlag.project_id == project.id) .filter(model.CommitFlag.commit_hash == commit_hash) - ) + ).order_by(model.CommitFlag.date_updated) return query.all() From 622764a89fbe92ab0a89c1df12a73e755c74c188 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 16 2020 10:47:00 +0000 Subject: [PATCH 2/4] Change the PR flag API endpoints to use commit flags Basically, instead of flagging the PR object, we're flagging the commit at the top of the PR. Similarly, when retrieving PR flags, retrieve commit flags instead. Fixes https://pagure.io/pagure/issue/4835 Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/api/fork.py b/pagure/api/fork.py index 1185ce8..8b5dbe1 100644 --- a/pagure/api/fork.py +++ b/pagure/api/fork.py @@ -1064,9 +1064,27 @@ def api_pull_request_add_flag(repo, requestid, username=None, namespace=None): else pagure_config["FLAG_FAILURE"] ) try: + if not request.commit_stop: + repopath = None + parentpath = pagure.utils.get_repo_path(request.project) + if request.remote: + repopath = pagure.utils.get_remote_repo_path( + request.remote_git, request.branch_from + ) + elif request.project_from: + repopath = pagure.utils.get_repo_path(request.project_from) + + repo_obj = None + if repopath: + repo_obj = pygit2.Repository(repopath) + orig_repo = pygit2.Repository(parentpath) + pagure.lib.git.diff_pull_request( + flask.g.session, request, repo_obj, orig_repo + ) + # New Flag message, uid = pagure.lib.query.add_pull_request_flag( - flask.g.session, + session=flask.g.session, request=request, username=username, status=status, @@ -1078,8 +1096,8 @@ def api_pull_request_add_flag(repo, requestid, username=None, namespace=None): token=flask.g.token.id, ) flask.g.session.commit() - pr_flag = pagure.lib.query.get_pull_request_flag_by_uid( - flask.g.session, request, uid + pr_flag = pagure.lib.query.get_commit_flag_by_uid( + flask.g.session, request.commit_stop, uid ) output["message"] = message output["uid"] = uid @@ -1182,9 +1200,31 @@ def api_pull_request_get_flag(repo, requestid, username=None, namespace=None): _check_pull_request(repo) request = _get_request(repo, requestid) + if not request.commit_stop: + repopath = None + parentpath = pagure.utils.get_repo_path(request.project) + if request.remote: + repopath = pagure.utils.get_remote_repo_path( + request.remote_git, request.branch_from + ) + elif request.project_from: + repopath = pagure.utils.get_repo_path(request.project_from) + + repo_obj = None + if repopath: + repo_obj = pygit2.Repository(repopath) + orig_repo = pygit2.Repository(parentpath) + pagure.lib.git.diff_pull_request( + flask.g.session, request, repo_obj, orig_repo + ) + output = {"flags": []} - for flag in request.flags: + flags = pagure.lib.query.get_commit_flag( + flask.g.session, request.project, request.commit_stop + ) + + for flag in flags: output["flags"].append(flag.to_json(public=True)) jsonout = flask.jsonify(output) diff --git a/pagure/lib/notify.py b/pagure/lib/notify.py index 02c277c..92eb0bd 100644 --- a/pagure/lib/notify.py +++ b/pagure/lib/notify.py @@ -1016,7 +1016,7 @@ def notify_pull_request_comment(comment, user): ) -def notify_pull_request_flag(flag, user): +def notify_pull_request_flag(flag, request, user): """ Notify the people following a pull-request that a new flag was added to it. """ @@ -1026,31 +1026,29 @@ def notify_pull_request_flag(flag, user): %s """ % ( flag.username, - flag.pull_request.title, + request.title, flag.status, flag.comment, _build_url( pagure_config["APP_URL"], - _fullname_to_url(flag.pull_request.project.fullname), + _fullname_to_url(request.project.fullname), "pull-request", - flag.pull_request.id, + request.id, ), ) - mail_to = _get_emails_for_obj(flag.pull_request) + mail_to = _get_emails_for_obj(request) - assignee = ( - flag.pull_request.assignee.user if flag.pull_request.assignee else None - ) + assignee = request.assignee.user if request.assignee else None send_email( text, - "PR #%s - %s: %s" % (flag.pull_request.id, flag.username, flag.status), + "PR #%s - %s: %s" % (request.id, flag.username, flag.status), ",".join(mail_to), mail_id=flag.mail_id, - in_reply_to=flag.pull_request.mail_id, - project_name=flag.pull_request.project.fullname, + in_reply_to=request.mail_id, + project_name=request.project.fullname, user_from=flag.username, - reporter=flag.pull_request.user.user, + reporter=request.user.user, assignee=assignee, ) diff --git a/pagure/lib/query.py b/pagure/lib/query.py index 1396615..f80927b 100644 --- a/pagure/lib/query.py +++ b/pagure/lib/query.py @@ -1502,35 +1502,26 @@ def add_pull_request_flag( """ Add a flag to a pull-request. """ user_obj = get_user(session, user) - action = "added" - pr_flag = None - if uid: - pr_flag = get_pull_request_flag_by_uid(session, request, uid) - if pr_flag: - action = "updated" - pr_flag.comment = comment - pr_flag.status = status - pr_flag.percent = percent - pr_flag.url = url - else: - pr_flag = model.PullRequestFlag( - pull_request_uid=request.uid, - uid=uid or uuid.uuid4().hex, - username=username, - percent=percent, - comment=comment, - status=status, - url=url, - user_id=user_obj.id, - token_id=token, - ) - request.updated_on = datetime.datetime.utcnow() - session.add(pr_flag) - # Make sure we won't have SQLAlchemy error before we continue - session.flush() + action, flag_uid = add_commit_flag( + session=session, + repo=request.project, + commit_hash=request.commit_stop, + username=username, + status=status, + percent=percent, + comment=comment, + url=url, + uid=uid, + user=user, + token=token, + ) + + pr_flag = pagure.lib.query.get_commit_flag_by_uid( + session, request.commit_stop, flag_uid + ) if request.project.settings.get("notify_on_pull-request_flag"): - pagure.lib.notify.notify_pull_request_flag(pr_flag, username) + pagure.lib.notify.notify_pull_request_flag(pr_flag, request, username) pagure.lib.git.update_git(request, repo=request.project) @@ -1544,6 +1535,8 @@ def add_pull_request_flag( ), ) + action = action.replace("Flag ", "") + return ("Flag %s" % action, pr_flag.uid) diff --git a/tests/test_pagure_flask_api_pr_flag.py b/tests/test_pagure_flask_api_pr_flag.py index 70443ab..8acb575 100644 --- a/tests/test_pagure_flask_api_pr_flag.py +++ b/tests/test_pagure_flask_api_pr_flag.py @@ -65,6 +65,9 @@ class PagureFlaskApiPRFlagtests(tests.Modeltests): request = pagure.lib.query.search_pull_requests( self.session, project_id=1, requestid=1 ) + request.commit_stop = "hash_commit_stop" + self.session.add(request) + self.session.commit() self.assertEqual(len(request.flags), 0) def test_invalid_project(self): @@ -244,8 +247,8 @@ class PagureFlaskApiPRFlagtests(tests.Modeltests): data = json.loads(output.get_data(as_text=True)) data["flag"]["date_created"] = "1510742565" data["flag"]["date_updated"] = "1510742565" - pr_uid = data["flag"]["pull_request_uid"] - data["flag"]["pull_request_uid"] = "62b49f00d489452994de5010565fab81" + commit_hash = data["flag"]["commit_hash"] + data["flag"]["commit_hash"] = "62b49f00d489452994de5010565fab81" data["avatar_url"] = "https://seccdn.libravatar.org/avatar/..." self.assertDictEqual( data, @@ -255,7 +258,7 @@ class PagureFlaskApiPRFlagtests(tests.Modeltests): "date_created": "1510742565", "date_updated": "1510742565", "percent": None, - "pull_request_uid": "62b49f00d489452994de5010565fab81", + "commit_hash": "62b49f00d489452994de5010565fab81", "status": "pending", "url": "http://jenkins.cloud.fedoraproject.org/", "user": { @@ -279,9 +282,13 @@ class PagureFlaskApiPRFlagtests(tests.Modeltests): request = pagure.lib.query.search_pull_requests( self.session, project_id=1, requestid=1 ) - self.assertEqual(len(request.flags), 1) - self.assertEqual(request.flags[0].comment, "Tests running") - self.assertEqual(request.flags[0].percent, None) + self.assertEqual(len(request.flags), 0) + + flags = pagure.lib.query.get_commit_flag( + self.session, request.project, commit_hash + ) + self.assertEqual(flags[0].comment, "Tests running") + self.assertEqual(flags[0].percent, None) # Check the notification sent mock_email.assert_called_once_with( @@ -291,8 +298,8 @@ class PagureFlaskApiPRFlagtests(tests.Modeltests): "PR #1 - Jenkins: pending", "bar@pingou.com", assignee=None, - in_reply_to="test-pull-request-" + pr_uid, - mail_id="test-pull-request-" + pr_uid + "-1", + in_reply_to="test-pull-request-%s" % request.uid, + mail_id="test-commit-1-1", project_name="test", reporter="pingou", user_from="Jenkins", @@ -317,17 +324,16 @@ class PagureFlaskApiPRFlagtests(tests.Modeltests): data = json.loads(output.get_data(as_text=True)) data["flag"]["date_created"] = "1510742565" data["flag"]["date_updated"] = "1510742565" - data["flag"]["pull_request_uid"] = "62b49f00d489452994de5010565fab81" data["avatar_url"] = "https://seccdn.libravatar.org/avatar/..." self.assertDictEqual( data, { "flag": { "comment": "Tests running", + "commit_hash": "hash_commit_stop", "date_created": "1510742565", "date_updated": "1510742565", "percent": None, - "pull_request_uid": "62b49f00d489452994de5010565fab81", "status": "pending", "url": "http://jenkins.cloud.fedoraproject.org/", "user": { @@ -351,9 +357,13 @@ class PagureFlaskApiPRFlagtests(tests.Modeltests): request = pagure.lib.query.search_pull_requests( self.session, project_id=1, requestid=1 ) - self.assertEqual(len(request.flags), 1) - self.assertEqual(request.flags[0].comment, "Tests running") - self.assertEqual(request.flags[0].percent, None) + self.assertEqual(len(request.flags), 0) + + flags = pagure.lib.query.get_commit_flag( + self.session, request.project, "hash_commit_stop" + ) + self.assertEqual(flags[0].comment, "Tests running") + self.assertEqual(flags[0].percent, None) # Update flag - w/o providing the status data = { @@ -371,17 +381,16 @@ class PagureFlaskApiPRFlagtests(tests.Modeltests): data = json.loads(output.get_data(as_text=True)) data["flag"]["date_created"] = "1510742565" data["flag"]["date_updated"] = "1510742565" - data["flag"]["pull_request_uid"] = "62b49f00d489452994de5010565fab81" data["avatar_url"] = "https://seccdn.libravatar.org/avatar/..." self.assertDictEqual( data, { "flag": { "comment": "Tests passed", + "commit_hash": "hash_commit_stop", "date_created": "1510742565", "date_updated": "1510742565", "percent": 100, - "pull_request_uid": "62b49f00d489452994de5010565fab81", "status": "success", "url": "http://jenkins.cloud.fedoraproject.org/", "user": { @@ -405,9 +414,13 @@ class PagureFlaskApiPRFlagtests(tests.Modeltests): request = pagure.lib.query.search_pull_requests( self.session, project_id=1, requestid=1 ) - self.assertEqual(len(request.flags), 1) - self.assertEqual(request.flags[0].comment, "Tests passed") - self.assertEqual(request.flags[0].percent, 100) + self.assertEqual(len(request.flags), 0) + + flags = pagure.lib.query.get_commit_flag( + self.session, request.project, "hash_commit_stop" + ) + self.assertEqual(flags[0].comment, "Tests passed") + self.assertEqual(flags[0].percent, 100) def test_adding_two_flags(self): """ Test the adding two flags to a PR. """ @@ -430,17 +443,16 @@ class PagureFlaskApiPRFlagtests(tests.Modeltests): data = json.loads(output.get_data(as_text=True)) data["flag"]["date_created"] = "1510742565" data["flag"]["date_updated"] = "1510742565" - data["flag"]["pull_request_uid"] = "62b49f00d489452994de5010565fab81" data["avatar_url"] = "https://seccdn.libravatar.org/avatar/..." self.assertDictEqual( data, { "flag": { "comment": "Tests passed", + "commit_hash": "hash_commit_stop", "date_created": "1510742565", "date_updated": "1510742565", "percent": 100, - "pull_request_uid": "62b49f00d489452994de5010565fab81", "status": "success", "url": "http://jenkins.cloud.fedoraproject.org/", "user": { @@ -459,14 +471,19 @@ class PagureFlaskApiPRFlagtests(tests.Modeltests): }, ) - # One flag added + # One flag added - but no longer on the request object self.session.commit() request = pagure.lib.query.search_pull_requests( self.session, project_id=1, requestid=1 ) - self.assertEqual(len(request.flags), 1) - self.assertEqual(request.flags[0].comment, "Tests passed") - self.assertEqual(request.flags[0].percent, 100) + self.assertEqual(len(request.flags), 0) + + flags = pagure.lib.query.get_commit_flag( + self.session, request.project, "hash_commit_stop" + ) + self.assertEqual(len(flags), 1) + self.assertEqual(flags[0].comment, "Tests passed") + self.assertEqual(flags[0].percent, 100) data = { "username": "Jenkins", @@ -482,7 +499,6 @@ class PagureFlaskApiPRFlagtests(tests.Modeltests): data = json.loads(output.get_data(as_text=True)) data["flag"]["date_created"] = "1510742565" data["flag"]["date_updated"] = "1510742565" - data["flag"]["pull_request_uid"] = "62b49f00d489452994de5010565fab81" self.assertNotEqual(data["uid"], "jenkins_build_pagure_100+seed") data["uid"] = "jenkins_build_pagure_100+seed" data["avatar_url"] = "https://seccdn.libravatar.org/avatar/..." @@ -491,10 +507,10 @@ class PagureFlaskApiPRFlagtests(tests.Modeltests): { "flag": { "comment": "Tests running again", + "commit_hash": "hash_commit_stop", "date_created": "1510742565", "date_updated": "1510742565", "percent": None, - "pull_request_uid": "62b49f00d489452994de5010565fab81", "status": "pending", "url": "http://jenkins.cloud.fedoraproject.org/", "user": { @@ -513,16 +529,21 @@ class PagureFlaskApiPRFlagtests(tests.Modeltests): }, ) - # Two flag added + # Two flags added - but no longer on the request object self.session.commit() request = pagure.lib.query.search_pull_requests( self.session, project_id=1, requestid=1 ) - self.assertEqual(len(request.flags), 2) - self.assertEqual(request.flags[0].comment, "Tests running again") - self.assertEqual(request.flags[0].percent, None) - self.assertEqual(request.flags[1].comment, "Tests passed") - self.assertEqual(request.flags[1].percent, 100) + self.assertEqual(len(request.flags), 0) + + flags = pagure.lib.query.get_commit_flag( + self.session, request.project, "hash_commit_stop" + ) + self.assertEqual(len(flags), 2) + self.assertEqual(flags[1].comment, "Tests running again") + self.assertEqual(flags[1].percent, None) + self.assertEqual(flags[0].comment, "Tests passed") + self.assertEqual(flags[0].percent, 100) @patch.dict( "pagure.config.config", @@ -642,6 +663,9 @@ class PagureFlaskApiPRFlagUserTokentests(tests.Modeltests): request = pagure.lib.query.search_pull_requests( self.session, project_id=1, requestid=1 ) + request.commit_stop = "hash_commit_stop" + self.session.add(request) + self.session.commit() self.assertEqual(len(request.flags), 0) def test_no_pr(self): @@ -787,17 +811,16 @@ class PagureFlaskApiPRFlagUserTokentests(tests.Modeltests): data = json.loads(output.get_data(as_text=True)) data["flag"]["date_created"] = "1510742565" data["flag"]["date_updated"] = "1510742565" - data["flag"]["pull_request_uid"] = "62b49f00d489452994de5010565fab81" data["avatar_url"] = "https://seccdn.libravatar.org/avatar/..." self.assertDictEqual( data, { "flag": { "comment": "Tests failed", + "commit_hash": "hash_commit_stop", "date_created": "1510742565", "date_updated": "1510742565", "percent": 0, - "pull_request_uid": "62b49f00d489452994de5010565fab81", "status": "failure", "url": "http://jenkins.cloud.fedoraproject.org/", "user": { @@ -821,9 +844,14 @@ class PagureFlaskApiPRFlagUserTokentests(tests.Modeltests): request = pagure.lib.query.search_pull_requests( self.session, project_id=1, requestid=1 ) - self.assertEqual(len(request.flags), 1) - self.assertEqual(request.flags[0].comment, "Tests failed") - self.assertEqual(request.flags[0].percent, 0) + self.assertEqual(len(request.flags), 0) + + flags = pagure.lib.query.get_commit_flag( + self.session, request.project, "hash_commit_stop" + ) + self.assertEqual(len(flags), 1) + self.assertEqual(flags[0].comment, "Tests failed") + self.assertEqual(flags[0].percent, 0) # no notifications sent mock_email.assert_not_called() @@ -849,17 +877,16 @@ class PagureFlaskApiPRFlagUserTokentests(tests.Modeltests): data = json.loads(output.get_data(as_text=True)) data["flag"]["date_created"] = "1510742565" data["flag"]["date_updated"] = "1510742565" - data["flag"]["pull_request_uid"] = "62b49f00d489452994de5010565fab81" data["avatar_url"] = "https://seccdn.libravatar.org/avatar/..." self.assertDictEqual( data, { "flag": { "comment": "Tests failed", + "commit_hash": "hash_commit_stop", "date_created": "1510742565", "date_updated": "1510742565", "percent": None, - "pull_request_uid": "62b49f00d489452994de5010565fab81", "status": "failure", "url": "http://jenkins.cloud.fedoraproject.org/", "user": { @@ -883,9 +910,14 @@ class PagureFlaskApiPRFlagUserTokentests(tests.Modeltests): request = pagure.lib.query.search_pull_requests( self.session, project_id=1, requestid=1 ) - self.assertEqual(len(request.flags), 1) - self.assertEqual(request.flags[0].comment, "Tests failed") - self.assertEqual(request.flags[0].percent, None) + self.assertEqual(len(request.flags), 0) + + flags = pagure.lib.query.get_commit_flag( + self.session, request.project, "hash_commit_stop" + ) + self.assertEqual(len(flags), 1) + self.assertEqual(flags[0].comment, "Tests failed") + self.assertEqual(flags[0].percent, None) # Update flag data = { @@ -904,17 +936,16 @@ class PagureFlaskApiPRFlagUserTokentests(tests.Modeltests): data = json.loads(output.get_data(as_text=True)) data["flag"]["date_created"] = "1510742565" data["flag"]["date_updated"] = "1510742565" - data["flag"]["pull_request_uid"] = "62b49f00d489452994de5010565fab81" data["avatar_url"] = "https://seccdn.libravatar.org/avatar/..." self.assertDictEqual( data, { "flag": { "comment": "Tests passed", + "commit_hash": "hash_commit_stop", "date_created": "1510742565", "date_updated": "1510742565", "percent": 100, - "pull_request_uid": "62b49f00d489452994de5010565fab81", "status": "success", "url": "http://jenkins.cloud.fedoraproject.org/", "user": { @@ -938,9 +969,14 @@ class PagureFlaskApiPRFlagUserTokentests(tests.Modeltests): request = pagure.lib.query.search_pull_requests( self.session, project_id=1, requestid=1 ) - self.assertEqual(len(request.flags), 1) - self.assertEqual(request.flags[0].comment, "Tests passed") - self.assertEqual(request.flags[0].percent, 100) + self.assertEqual(len(request.flags), 0) + + flags = pagure.lib.query.get_commit_flag( + self.session, request.project, "hash_commit_stop" + ) + self.assertEqual(len(flags), 1) + self.assertEqual(flags[0].comment, "Tests passed") + self.assertEqual(flags[0].percent, 100) class PagureFlaskApiGetPRFlagtests(tests.Modeltests): @@ -983,6 +1019,9 @@ class PagureFlaskApiGetPRFlagtests(tests.Modeltests): request = pagure.lib.query.search_pull_requests( self.session, project_id=1, requestid=1 ) + request.commit_stop = "hash_commit_stop" + self.session.add(request) + self.session.commit() self.assertEqual(len(request.flags), 0) def test_invalid_project(self): @@ -1060,8 +1099,13 @@ class PagureFlaskApiGetPRFlagtests(tests.Modeltests): self.assertEqual(msg, ("Flag added", "jenkins_build_pagure_34")) self.session.commit() - self.assertEqual(len(request.flags), 1) - self.assertEqual(request.flags[0].token_id, "aaabbbcccddd") + self.assertEqual(len(request.flags), 0) + + flags = pagure.lib.query.get_commit_flag( + self.session, request.project, "hash_commit_stop" + ) + self.assertEqual(len(flags), 1) + self.assertEqual(flags[0].token_id, "aaabbbcccddd") # 1 flag output = self.app.get("/api/0/test/pull-request/1/flag") @@ -1069,19 +1113,16 @@ class PagureFlaskApiGetPRFlagtests(tests.Modeltests): data = json.loads(output.get_data(as_text=True)) data["flags"][0]["date_created"] = "1541413645" data["flags"][0]["date_updated"] = "1541413645" - data["flags"][0][ - "pull_request_uid" - ] = "72a61033c2fc464aa9ef514c057aa62c" self.assertDictEqual( data, { "flags": [ { "comment": "Build passes", + "commit_hash": "hash_commit_stop", "date_created": "1541413645", "date_updated": "1541413645", "percent": None, - "pull_request_uid": "72a61033c2fc464aa9ef514c057aa62c", "status": "success", "url": "http://jenkins.cloud.fedoraproject.org", "user": { @@ -1132,9 +1173,14 @@ class PagureFlaskApiGetPRFlagtests(tests.Modeltests): self.assertEqual(msg, ("Flag added", "travis_build_pagure_34")) self.session.commit() - self.assertEqual(len(request.flags), 2) - self.assertEqual(request.flags[1].token_id, "aaabbbcccddd") - self.assertEqual(request.flags[0].token_id, "aaabbbcccddd") + self.assertEqual(len(request.flags), 0) + + flags = pagure.lib.query.get_commit_flag( + self.session, request.project, "hash_commit_stop" + ) + self.assertEqual(len(flags), 2) + self.assertEqual(flags[1].token_id, "aaabbbcccddd") + self.assertEqual(flags[0].token_id, "aaabbbcccddd") # 1 flag output = self.app.get("/api/0/test/pull-request/1/flag") @@ -1142,47 +1188,41 @@ class PagureFlaskApiGetPRFlagtests(tests.Modeltests): data = json.loads(output.get_data(as_text=True)) data["flags"][0]["date_created"] = "1541413645" data["flags"][0]["date_updated"] = "1541413645" - data["flags"][0][ - "pull_request_uid" - ] = "72a61033c2fc464aa9ef514c057aa62c" data["flags"][1]["date_created"] = "1541413645" data["flags"][1]["date_updated"] = "1541413645" - data["flags"][1][ - "pull_request_uid" - ] = "72a61033c2fc464aa9ef514c057aa62c" self.assertDictEqual( data, { "flags": [ { - "comment": "Build pending", + "comment": "Build passes", + "commit_hash": "hash_commit_stop", "date_created": "1541413645", "date_updated": "1541413645", "percent": None, - "pull_request_uid": "72a61033c2fc464aa9ef514c057aa62c", - "status": "pending", - "url": "http://travis.io", + "status": "success", + "url": "http://jenkins.cloud.fedoraproject.org", "user": { "fullname": "foo bar", "name": "foo", "url_path": "user/foo", }, - "username": "travis", + "username": "jenkins", }, { - "comment": "Build passes", + "comment": "Build pending", + "commit_hash": "hash_commit_stop", "date_created": "1541413645", "date_updated": "1541413645", "percent": None, - "pull_request_uid": "72a61033c2fc464aa9ef514c057aa62c", - "status": "success", - "url": "http://jenkins.cloud.fedoraproject.org", + "status": "pending", + "url": "http://travis.io", "user": { "fullname": "foo bar", "name": "foo", "url_path": "user/foo", }, - "username": "jenkins", + "username": "travis", }, ] }, diff --git a/tests/test_pagure_flask_api_ui_private_repo.py b/tests/test_pagure_flask_api_ui_private_repo.py index b4a56eb..03b5994 100644 --- a/tests/test_pagure_flask_api_ui_private_repo.py +++ b/tests/test_pagure_flask_api_ui_private_repo.py @@ -1985,6 +1985,8 @@ class PagurePrivateRepotest(tests.Modeltests): title="test pull-request", user="pingou", ) + req.commit_stop = "hash_commit_stop" + self.session.add(req) self.session.commit() self.assertEqual(req.id, 1) self.assertEqual(req.title, "test pull-request") @@ -2041,17 +2043,16 @@ class PagurePrivateRepotest(tests.Modeltests): data = json.loads(output.get_data(as_text=True)) data["flag"]["date_created"] = "1510742565" data["flag"]["date_updated"] = "1510742565" - data["flag"]["pull_request_uid"] = "62b49f00d489452994de5010565fab81" data["avatar_url"] = "https://seccdn.libravatar.org/avatar/..." self.assertDictEqual( data, { "flag": { "comment": "Tests failed", + "commit_hash": "hash_commit_stop", "date_created": "1510742565", "date_updated": "1510742565", "percent": 0, - "pull_request_uid": "62b49f00d489452994de5010565fab81", "status": "failure", "url": "http://jenkins.cloud.fedoraproject.org/", "user": { @@ -2075,9 +2076,14 @@ class PagurePrivateRepotest(tests.Modeltests): request = pagure.lib.query.search_pull_requests( self.session, project_id=1, requestid=1 ) - self.assertEqual(len(request.flags), 1) - self.assertEqual(request.flags[0].comment, "Tests failed") - self.assertEqual(request.flags[0].percent, 0) + self.assertEqual(len(request.flags), 0) + + flags = pagure.lib.query.get_commit_flag( + self.session, request.project, "hash_commit_stop" + ) + self.assertEqual(len(flags), 1) + self.assertEqual(flags[0].comment, "Tests failed") + self.assertEqual(flags[0].percent, 0) # Update flag data = { @@ -2095,17 +2101,16 @@ class PagurePrivateRepotest(tests.Modeltests): data = json.loads(output.get_data(as_text=True)) data["flag"]["date_created"] = "1510742565" data["flag"]["date_updated"] = "1510742565" - data["flag"]["pull_request_uid"] = "62b49f00d489452994de5010565fab81" data["avatar_url"] = "https://seccdn.libravatar.org/avatar/..." self.assertDictEqual( data, { "flag": { "comment": "Tests passed", + "commit_hash": "hash_commit_stop", "date_created": "1510742565", "date_updated": "1510742565", "percent": 100, - "pull_request_uid": "62b49f00d489452994de5010565fab81", "status": "success", "url": "http://jenkins.cloud.fedoraproject.org/", "user": { @@ -2129,9 +2134,14 @@ class PagurePrivateRepotest(tests.Modeltests): request = pagure.lib.query.search_pull_requests( self.session, project_id=1, requestid=1 ) - self.assertEqual(len(request.flags), 1) - self.assertEqual(request.flags[0].comment, "Tests passed") - self.assertEqual(request.flags[0].percent, 100) + self.assertEqual(len(request.flags), 0) + + flags = pagure.lib.query.get_commit_flag( + self.session, request.project, "hash_commit_stop" + ) + self.assertEqual(len(flags), 1) + self.assertEqual(flags[0].comment, "Tests passed") + self.assertEqual(flags[0].percent, 100) @patch("pagure.lib.notify.send_email") def test_api_private_repo_pr_close(self, send_email): diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py index 1076bb6..72e31c2 100644 --- a/tests/test_pagure_lib.py +++ b/tests/test_pagure_lib.py @@ -2655,6 +2655,12 @@ class PagureLibtests(tests.Modeltests): request = pagure.lib.query.search_pull_requests( self.session, requestid=1 ) + request.commit_stop = "hash_commit_stop" + self.session.add(request) + self.session.commit() + request = pagure.lib.query.search_pull_requests( + self.session, requestid=1 + ) self.assertEqual(len(request.flags), 0) msg = pagure.lib.query.add_pull_request_flag( @@ -2672,8 +2678,14 @@ class PagureLibtests(tests.Modeltests): self.assertEqual(msg, ("Flag added", "jenkins_build_pagure_34")) self.session.commit() - self.assertEqual(len(request.flags), 1) - self.assertEqual(request.flags[0].token_id, "aaabbbcccddd") + # We're no longer adding a PR flag but instead a commit flag to the + # latest commit of the PR + self.assertEqual(len(request.flags), 0) + flags = pagure.lib.query.get_commit_flag( + self.session, request.project, "hash_commit_stop" + ) + self.assertEqual(flags[0].comment, "Build passes") + self.assertEqual(flags[0].token_id, "aaabbbcccddd") @patch("pagure.lib.notify.send_email", MagicMock(return_value=True)) def test_search_pull_requests(self): From 4e2372f0495ef25a9a6037091f05a57462cb88fc Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 16 2020 10:47:00 +0000 Subject: [PATCH 3/4] Rework how PR flags are show in the UI Now that the PR flag API actually flag commit rather than PR objects, we presentation of these flags in the UI is changed as well. We both include the flags in the list of commits, as we do on the commit list and in addition, we show the flags of the last commit on the PR just above the main comment area. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/templates/repo_pull_request.html b/pagure/templates/repo_pull_request.html index 5eec6b8..ce1562b 100644 --- a/pagure/templates/repo_pull_request.html +++ b/pagure/templates/repo_pull_request.html @@ -97,7 +97,7 @@ {% if pull_request.project_from.is_fork -%} {{ pull_request.project_from.user.user }}/ {%- endif -%} - {{ pull_request.project_from.name }} + {{ pull_request.project_from.name }} {% elif pull_request.project_from is none and pull_request.remote_git is none %} @@ -336,7 +336,8 @@
+ data-commithash="{{ commit.hex }}" + class="btn btn-outline-primary font-weight-bold {{'disabled' if not commit_link}} commithash"> {{ commit.hex|short }} @@ -409,6 +410,12 @@ +
+
+
+
+ + {% if g.authenticated and mergeform %}
{% if g.authenticated %} @@ -458,7 +465,7 @@ {% endif %} -
+

Pull this pull-request locally @@ -470,9 +477,8 @@

{% endif %} - -
+
@@ -580,7 +586,6 @@ {% endif %}
- {% if pull_request.flags %}
@@ -882,7 +887,101 @@ function show_merge_status(){ } {% endif %} +var statusesLabels = {{ flag_statuses_labels|safe }} + $(document).ready(function() { + + {# Show latest tag #} + {% if pull_request.commit_stop %}; + var url = '{{ url_for("api_ns.api_commit_flags", + repo=repo.name, + username=repo.user.user if repo.is_fork else none, + namespace=repo.namespace, + commit_hash=pull_request.commit_stop) }}' + var commitUrl = '{{ url_for("ui_ns.view_commit", + repo=repo.name, + username=username, + namespace=repo.namespace, + commitid=pull_request.commit_stop) }}'; + $.ajax({ + url: url, + type: 'GET', + dataType: 'json', + success: function(res) { + var statusesLabels = {{ flag_statuses_labels|safe }}; + var flags_html = ''; + + for (var i in res['flags']) { + var f = res['flags'][i]; + var t = f.date_created == f.date_updated ? "Created at": "Updated at"; + var d = new Date(f.date_updated * 1000); + var html = '' + + '
' + + ' ' + f.username + + ' ' + + '
' + + ' ' + f.status + + ' ' + + '
' + + '
' + + '
' + + ' ' + f.comment + '' + + '
' + + ' ' + d.toUTCString() + '
' + + '
' + + ' ' + + ''; + flags_html += html; + } + $("#flags").html(flags_html); + } + }); + {% endif %} + + {# Show flags in commit list #} + $(".commithash").each(function(idx, item) { + var url = '{{ url_for("api_ns.api_commit_flags", + repo=repo.name, + username=repo.user.user if repo.is_fork else none, + namespace=repo.namespace, + commit_hash='COMMIT_HASH') }}' + var commitUrl = '{{ url_for("ui_ns.view_commit", + repo=repo.name, + username=username, + namespace=repo.namespace, + commitid="COMMIT_HASH") }}'; + var commithash= $(item).attr('data-commithash'); + url = url.replace("COMMIT_HASH", commithash); + commitUrl = commitUrl.replace("COMMIT_HASH", commithash); + $.ajax({ + url: url, + type: 'GET', + dataType: 'json', + success: function(res) { + var statuses = {} + for (var i in res['flags']) { + var f = res['flags'][i] + if (!(f['status'] in statuses)) { + statuses[f['status']] = [] + } + statuses[f['status']].push(f) + } + var html = '' + var sortedStatuses = Object.keys(statuses).sort() + for (var i in sortedStatuses) { + s = sortedStatuses[i] + numStatuses = statuses[s].length + html += '' + html += statuses[s].length + '\n' + } + $(html).insertBefore(item); + } + }); + }); + + $('#merge_btn').click(function() { return confirm('Confirm merging this pull-request'); }); diff --git a/pagure/ui/fork.py b/pagure/ui/fork.py index 46388f7..b11a08d 100644 --- a/pagure/ui/fork.py +++ b/pagure/ui/fork.py @@ -19,6 +19,7 @@ from __future__ import unicode_literals, absolute_import import logging +import json import os from math import ceil @@ -369,6 +370,7 @@ def request_pull(repo, requestid, username=None, namespace=None): can_delete_branch=can_delete_branch, trigger_ci=trigger_ci, trigger_ci_pr_form=trigger_ci_pr_form, + flag_statuses_labels=json.dumps(pagure_config["FLAG_STATUSES_LABELS"]), ) From 9e5a42942081431829ef9c35ba5ef6a357df91e4 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 16 2020 10:47:00 +0000 Subject: [PATCH 4/4] Document the risk of race-condition in the PR flag API endpoint Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/api/fork.py b/pagure/api/fork.py index 8b5dbe1..abe458e 100644 --- a/pagure/api/fork.py +++ b/pagure/api/fork.py @@ -928,6 +928,13 @@ def api_pull_request_add_flag(repo, requestid, username=None, namespace=None): ------------------- Add or edit flags on a pull-request. + This is an utility method which add a flag to the latest commit of the + specified pull-request. + + Note that using it has a risk of race-condition if the pull-request changes + at the time the flag is being added. Using the commit flag endpoint prevents + this risk of race-condition. + :: POST /api/0//pull-request//flag