From b983f9b6a521b846c27be33d1c1114da40378e44 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 22 2019 09:37:12 +0000 Subject: When rebasing a PR in the API, refresh its cached merge_status as well Doing this in the same API call allows to specify the user who has asked for the rebase and thus we are able to use this information in the notification about the rebase in the UI, instead of relying on the name of the user's whose project the PR is opened against. Fixes https://pagure.io/pagure/issue/4316 Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/api/fork.py b/pagure/api/fork.py index e676e4d..9451f94 100644 --- a/pagure/api/fork.py +++ b/pagure/api/fork.py @@ -659,7 +659,11 @@ def api_pull_request_rebase(repo, requestid, username=None, namespace=None): raise pagure.exceptions.APIError(403, error_code=APIERROR.ENOPRCLOSE) task = pagure.lib.tasks.rebase_pull_request.delay( - repo.name, namespace, username, requestid, flask.g.fas_user.username + repo.name, + namespace, + username, + requestid, + user_rebaser=flask.g.fas_user.username, ) output = {"message": "Rebasing queued", "taskid": task.id} diff --git a/pagure/lib/git.py b/pagure/lib/git.py index ae5a971..f3415b3 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -1514,7 +1514,12 @@ def merge_pull_request(session, request, username, domerge=True): # Update the start and stop commits in the DB, one last time diff_commits = diff_pull_request( - session, request, fork_obj, new_repo, with_diff=False + session, + request, + fork_obj, + new_repo, + with_diff=False, + username=username, ) _log.info(" %s commit to merge", len(diff_commits)) @@ -2097,7 +2102,13 @@ def get_diff_info(repo_obj, orig_repo, branch_from, branch_to, prid=None): def diff_pull_request( - session, request, repo_obj, orig_repo, with_diff=True, notify=True + session, + request, + repo_obj, + orig_repo, + with_diff=True, + notify=True, + username=None, ): """ Returns the diff and the list of commits between the two git repos mentionned in the given pull-request. @@ -2109,9 +2120,11 @@ def diff_pull_request( :arg orig_repo: The pygit2.Repository object of the second git repo :arg with_diff: A boolean on whether to return the diff with the list of commits (or just the list of commits) + :arg username: The username of the user diffing the pull-request """ + _log.debug("pagure.lib.git.diff_pull_request, started") diff = None diff_commits = [] diff, diff_commits, _ = get_diff_info( @@ -2121,8 +2134,10 @@ def diff_pull_request( request.branch, prid=request.id, ) + _log.debug("pagure.lib.git.diff_pull_request, diff done") if request.status == "Open" and diff_commits: + _log.debug("pagure.lib.git.diff_pull_request, PR open and with a diff") first_commit = diff_commits[-1] # Check if we can still rely on the merge_status commenttext = None @@ -2163,6 +2178,9 @@ def diff_pull_request( request.commit_stop = diff_commits[0].oid.hex session.add(request) session.commit() + _log.debug( + "pagure.lib.git.diff_pull_request, commenttext: %s", commenttext + ) pagure.lib.tasks.sync_pull_ref.delay( request.project.name, @@ -2185,6 +2203,12 @@ def diff_pull_request( agent="pagure", ), ) + _log.debug( + "pagure.lib.git.diff_pull_request: adding notification: %s" + "as user: %s", + commenttext, + username or request.user.username, + ) pagure.lib.query.add_pull_request_comment( session, request, @@ -2193,7 +2217,7 @@ def diff_pull_request( filename=None, row=None, comment="%s" % commenttext, - user=request.user.username, + user=username or request.user.username, notify=False, notification=True, ) diff --git a/pagure/lib/tasks.py b/pagure/lib/tasks.py index 50a9b9a..14d0604 100644 --- a/pagure/lib/tasks.py +++ b/pagure/lib/tasks.py @@ -726,6 +726,7 @@ def rebase_pull_request( ) pagure.lib.git.rebase_pull_request(request, user_rebaser) + update_pull_request(request.uid, username=user_rebaser) # Schedule refresh of all opened PRs pagure.lib.query.reset_status_pull_request(session, request.project) @@ -870,7 +871,7 @@ def sync_pull_ref(self, session, name, namespace, user, requestid): @conn.task(queue=pagure_config.get("FAST_CELERY_QUEUE", None), bind=True) @pagure_task -def update_pull_request(self, session, pr_uid): +def update_pull_request(self, session, pr_uid, username=None): """ Updates a pull-request in the DB once a commit was pushed to it in git. """ @@ -886,7 +887,10 @@ def update_pull_request(self, session, pr_uid): try: pagure.lib.git.merge_pull_request( - session=session, request=request, username=None, domerge=False + session=session, + request=request, + username=username, + domerge=False, ) except pagure.exceptions.PagureException as err: _log.debug(err) diff --git a/tests/test_pagure_flask_rebase.py b/tests/test_pagure_flask_rebase.py index f651d7e..b1dabd8 100644 --- a/tests/test_pagure_flask_rebase.py +++ b/tests/test_pagure_flask_rebase.py @@ -45,7 +45,10 @@ class PagureRebasetests(tests.Modeltests): os.path.join(self.path, 'requests'), bare=True) tests.add_content_to_git( os.path.join(self.path, 'repos', 'test.git'), - branch='test', content="foobar") + branch='master', content="foobarbaz", filename="testfile") + tests.add_content_to_git( + os.path.join(self.path, 'repos', 'test.git'), + branch='test', content="foobar", filename="sources") tests.add_readme_git_repo( os.path.join(self.path, 'repos', 'test.git')) @@ -144,6 +147,22 @@ class PagureRebasetests(tests.Modeltests): user = tests.FakeUser(username='pingou') with tests.user_set(self.app.application, user): + # Get the merge status first so it's cached and can be refreshed + csrf_token = self.get_csrf() + data = {'requestid': self.request.uid, 'csrf_token': csrf_token} + output = self.app.post('/pv/pull-request/merge', data=data) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + { + u'code': u'MERGE', + u'message': u'The pull-request can be merged with ' + u'a merge commit', + u'short_code': u'With merge' + } + ) + output = self.app.post('/api/0/test/pull-request/1/rebase') self.assertEqual(output.status_code, 200) data = json.loads(output.get_data(as_text=True)) @@ -152,7 +171,7 @@ class PagureRebasetests(tests.Modeltests): {u'message': u'Pull-request rebased'} ) - data = {'requestid': self.request.uid, 'csrf_token': self.get_csrf()} + data = {'requestid': self.request.uid, 'csrf_token': csrf_token} output = self.app.post('/pv/pull-request/merge', data=data) self.assertEqual(output.status_code, 200) data = json.loads(output.get_data(as_text=True)) @@ -166,6 +185,75 @@ class PagureRebasetests(tests.Modeltests): } ) + output = self.app.get('/test/pull-request/1') + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertIn('rebased onto', output_text) + repo = pagure.lib.query._get_project(self.session, 'test') + self.assertEqual( + repo.requests[0].comments[0].user.username, 'pingou') + + def test_rebase_api_ui_logged_in_different_user(self): + """ Test the rebase PR API endpoint when logged in from the UI and + its outcome. """ + # Add 'foo' to the project 'test' so 'foo' can rebase the PR + repo = pagure.lib.query._get_project(self.session, 'test') + msg = pagure.lib.query.add_user_to_project( + session=self.session, + project=repo, + new_user='foo', + user='pingou', + ) + self.session.commit() + self.assertEqual(msg, 'User added') + + user = tests.FakeUser(username='foo') + with tests.user_set(self.app.application, user): + # Get the merge status first so it's cached and can be refreshed + csrf_token = self.get_csrf() + data = {'requestid': self.request.uid, 'csrf_token': csrf_token} + output = self.app.post('/pv/pull-request/merge', data=data) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + { + u'code': u'MERGE', + u'message': u'The pull-request can be merged with ' + u'a merge commit', + u'short_code': u'With merge' + } + ) + + output = self.app.post('/api/0/test/pull-request/1/rebase') + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + {u'message': u'Pull-request rebased'} + ) + + data = {'requestid': self.request.uid, 'csrf_token': csrf_token} + output = self.app.post('/pv/pull-request/merge', data=data) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + { + u'code': u'FFORWARD', + u'message': u'The pull-request can be merged and ' + u'fast-forwarded', + u'short_code': u'Ok' + } + ) + + output = self.app.get('/test/pull-request/1') + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertIn('rebased onto', output_text) + repo = pagure.lib.query._get_project(self.session, 'test') + self.assertEqual(repo.requests[0].comments[0].user.username, 'foo') + def test_rebase_api_api_logged_in(self): """ Test the rebase PR API endpoint when using an API token and its outcome. """