PR#4367: When rebasing a PR in the API indicate who did the rebase in the notifications - pagure

pagure

#4367 When rebasing a PR in the API indicate who did the rebase in the notifications

Merged 5 years ago by pingou. Opened 5 years ago by pingou.

rebase_user into master

Fix typo in the log

Pierre-Yves Chibon • 5 years ago

0b785c1

When asking for the merge status keep who asked for it

Pierre-Yves Chibon • 5 years ago

dc73ab9

When rebasing a PR in the API, refresh its cached merge_status as well

Pierre-Yves Chibon • 5 years ago

b983f9b

pagure/api/fork.py

file modified

+5 -1

		`@@ -659,7 +659,11 @@`
		`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}`

pagure/internal/__init__.py

file modified

+4 -1

		`@@ -314,11 +314,14 @@`

		`merge_status = request.merge_status`
		`if not merge_status or force:`
		`+ username = None`
		`+ if flask.g.authenticated:`
		`+ username = flask.g.fas_user.username`
		`try:`
		`merge_status = pagure.lib.git.merge_pull_request(`
		`session=flask.g.session,`
		`request=request,`
		`- username=None,`
		`+ username=username,`
		`domerge=False,`
		`)`
		`except pygit2.GitError as err:`

pagure/lib/git.py

file modified

+28 -4

		`@@ -1514,7 +1514,12 @@`

		`# 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))`

		`@@ -1823,7 +1828,7 @@`
		`Raises: pagure.exceptions.PagureException`

		`"""`
		`- _log.info("%s asked to rebased the pull-request: %s", username, request)`
		`+ _log.info("%s asked to rebase the pull-request: %s", username, request)`

		`if request.remote:`
		`# Get the fork`
		`@@ -2097,7 +2102,13 @@`


		`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 @@`
		`: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 @@`
		`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 @@`
		`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 @@`
		`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 @@`
		`filename=None,`
		`row=None,`
		`comment="%s" % commenttext,`
		`- user=request.user.username,`
		`+ user=username or request.user.username,`
		`notify=False,`
		`notification=True,`
		`)`

pagure/lib/tasks.py

file modified

+6 -2

		`@@ -726,6 +726,7 @@`
		`)`
		`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 @@`

		`@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 @@`

		`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)`

tests/test_pagure_flask_rebase.py

file modified

+90 -2

		`@@ -45,7 +45,10 @@`
		`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 @@`

		`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 @@`
		`{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 @@`
		`}`
		`)`

		`+ 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. """`