From aac55742eaf3daf4bf3c22e97c2fe00e3e1515de Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 28 2018 07:52:31 +0000 Subject: [PATCH 1/6] Return a 418 error if the task did not return anything itself We need this endpoint to always return something but there are tasks that are not returning an endpoint, so we can't redirect the users to anywhere as we do for the other tasks. So instead, in these case, we'll raise a 418 error. This was faced while working on tests and did not happen in real life. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/utils.py b/pagure/utils.py index 48360ab..2b7e673 100644 --- a/pagure/utils.py +++ b/pagure/utils.py @@ -331,9 +331,13 @@ def get_task_redirect_url(task, prev): flask.flash('Your task failed: %s' % str(result)) task.forget() return prev - endpoint = result.pop('endpoint') - task.forget() - return flask.url_for(endpoint, **result) + if isinstance(result, dict): + endpoint = result.pop('endpoint') + task.forget() + return flask.url_for(endpoint, **result) + else: + task.forget() + flask.abort(418) def wait_for_task(task, prev=None): From 58bcf49429c7132590e1fc169f2ec80158388861 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 28 2018 07:52:31 +0000 Subject: [PATCH 2/6] Include the filename when showing the diff of remote PRs Fixes https://pagure.io/pagure/issue/2972 Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/templates/pull_request.html b/pagure/templates/pull_request.html index 17851a5..c297985 100644 --- a/pagure/templates/pull_request.html +++ b/pagure/templates/pull_request.html @@ -425,11 +425,14 @@ title="View file as of {{ patch_new_id|short }}">{{ filepath | unicode }} {% elif pull_request and pull_request.remote %} + {{ filepath | unicode }} +
this is a remote pull-request, so we cannot provide you with a direct link to the file but you can always checkout the pull-request locally to review it entirely. +
{% endif %} {% endmacro %} From 9873aaf89dbdd557c723fc364c2298e381019919 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 28 2018 07:52:31 +0000 Subject: [PATCH 3/6] Specify the parent repo, even when creating a remote PR Otherwise the template misses it. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/ui/fork.py b/pagure/ui/fork.py index 1424130..70e5e4b 100644 --- a/pagure/ui/fork.py +++ b/pagure/ui/fork.py @@ -1353,6 +1353,7 @@ def new_remote_request_pull(repo, username=None, namespace=None): branch_to=branch_to, branch_from=branch_from, remote_git=remote_git, + parent=repo, ) try: From 517d6947ca47627c404c43196c0d4bf743916521 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 28 2018 07:52:31 +0000 Subject: [PATCH 4/6] Add support in the tests for task returning to a POST query Cf pagure.utils.wait_for_task_post Signed-off-by: Pierre-Yves Chibon --- diff --git a/tests/__init__.py b/tests/__init__.py index 99f8240..f93407e 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- """ - (c) 2015-2017 - Copyright Red Hat Inc + (c) 2015-2018 - Copyright Red Hat Inc Authors: Pierre-Yves Chibon @@ -102,10 +102,41 @@ def get_wait_target(html): return found[-1] +POST_REGEX = re.compile("""
') +def get_post_args(html): + """ This parses from the wait page the hidden arguments for the form. """ + found = INPUT_REGEX.findall(html) + if len(found) == 0: + raise Exception("Not able to get the POST arguments in %s" % html) + output = {} + for key, val in found: + output[key] = val + return output + + def create_maybe_waiter(method, getter): def maybe_waiter(*args, **kwargs): """ A wrapper for self.app.get()/.post() that will resolve wait's """ result = method(*args, **kwargs) + + # Handle the POST wait case + form_url = None + form_args = None + if 'id="waitform"' in result.data: + form_url = get_post_target(result.data) + form_args = get_post_args(result.data) + form_args['csrf_token'] = result.data.split( + 'name="csrf_token" type="hidden" value="')[1].split('">')[0] + count = 0 while 'We are waiting for your task to finish.' in result.data: # Resolve wait page @@ -118,6 +149,8 @@ def create_maybe_waiter(method, getter): if count > 50: raise Exception('Had to wait too long') else: + if form_url and form_args: + return method(form_url, data=form_args, follow_redirects=True) return result return maybe_waiter From 9f16a23a67a8acd0472fa6a9200ceee4a33ec4c6 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 28 2018 07:52:31 +0000 Subject: [PATCH 5/6] Add unit-tests for creating new remote PR This also ensure we're showing the name of the files changed in the remote PR. Signed-off-by: Pierre-Yves Chibon Signed-off-by: Pierre-Yves Chibon --- diff --git a/tests/test_pagure_flask_ui_remote_pr.py b/tests/test_pagure_flask_ui_remote_pr.py new file mode 100644 index 0000000..952c83a --- /dev/null +++ b/tests/test_pagure_flask_ui_remote_pr.py @@ -0,0 +1,269 @@ +# -*- coding: utf-8 -*- + +""" + (c) 2018 - Copyright Red Hat Inc + + Authors: + Pierre-Yves Chibon + +""" + +__requires__ = ['SQLAlchemy >= 0.8'] +import pkg_resources + +import json +import unittest +import shutil +import sys +import tempfile +import time +import os + +import pygit2 +from mock import patch, MagicMock +from bs4 import BeautifulSoup + +sys.path.insert(0, os.path.join(os.path.dirname( + os.path.abspath(__file__)), '..')) + +import pagure +import pagure.lib +import tests +from pagure.lib.repo import PagureRepo + + +class PagureRemotePRtests(tests.Modeltests): + """ Tests for remote PRs in pagure """ + + def setUp(self): + """ Set up the environment. """ + super(PagureRemotePRtests, self).setUp() + + self.newpath = tempfile.mkdtemp(prefix='pagure-fork-test') + self.old_value = pagure.config.config['REMOTE_GIT_FOLDER'] + pagure.config.config['REMOTE_GIT_FOLDER'] = os.path.join( + self.path, 'remotes') + + def tearDown(self): + """ Clear things up. """ + super(PagureRemotePRtests, self).tearDown() + + pagure.config.config['REMOTE_GIT_FOLDER'] = self.old_value + shutil.rmtree(self.newpath) + + def set_up_git_repo(self, new_project=None, branch_from='feature'): + """ Set up the git repo and create the corresponding PullRequest + object. + """ + + # Create a git repo to play with + gitrepo = os.path.join(self.path, 'repos', 'test.git') + repo = pygit2.init_repository(gitrepo, bare=True) + + repopath = os.path.join(self.newpath, 'test') + clone_repo = pygit2.clone_repository(gitrepo, repopath) + + # Create a file in that git repo + with open(os.path.join(repopath, 'sources'), 'w') as stream: + stream.write('foo\n bar') + clone_repo.index.add('sources') + clone_repo.index.write() + + try: + com = repo.revparse_single('HEAD') + prev_commit = [com.oid.hex] + except: + prev_commit = [] + + # Commits the files added + tree = clone_repo.index.write_tree() + author = pygit2.Signature( + 'Alice Author', 'alice@authors.tld') + committer = pygit2.Signature( + 'Cecil Committer', 'cecil@committers.tld') + clone_repo.create_commit( + 'refs/heads/master', # the name of the reference to update + author, + committer, + 'Add sources file for testing', + # binary string representing the tree object ID + tree, + # list of binary strings representing parents of the new commit + prev_commit + ) + time.sleep(1) + refname = 'refs/heads/master:refs/heads/master' + ori_remote = clone_repo.remotes[0] + PagureRepo.push(ori_remote, refname) + + first_commit = repo.revparse_single('HEAD') + + with open(os.path.join(repopath, '.gitignore'), 'w') as stream: + stream.write('*~') + clone_repo.index.add('.gitignore') + clone_repo.index.write() + + # Commits the files added + tree = clone_repo.index.write_tree() + author = pygit2.Signature( + 'Alice Äuthòr', 'alice@äuthòrs.tld') + committer = pygit2.Signature( + 'Cecil Cõmmîttër', 'cecil@cõmmîttërs.tld') + clone_repo.create_commit( + 'refs/heads/master', + author, + committer, + 'Add .gitignore file for testing', + # binary string representing the tree object ID + tree, + # list of binary strings representing parents of the new commit + [first_commit.oid.hex] + ) + refname = 'refs/heads/master:refs/heads/master' + ori_remote = clone_repo.remotes[0] + PagureRepo.push(ori_remote, refname) + + # Set the second repo + + new_gitrepo = repopath + if new_project: + # Create a new git repo to play with + new_gitrepo = os.path.join(self.newpath, new_project.fullname) + if not os.path.exists(new_gitrepo): + os.makedirs(new_gitrepo) + new_repo = pygit2.clone_repository(gitrepo, new_gitrepo) + + repo = pygit2.Repository(new_gitrepo) + + # Edit the sources file again + with open(os.path.join(new_gitrepo, 'sources'), 'w') as stream: + stream.write('foo\n bar\nbaz\n boose') + repo.index.add('sources') + repo.index.write() + + # Commits the files added + tree = repo.index.write_tree() + author = pygit2.Signature( + 'Alice Author', 'alice@authors.tld') + committer = pygit2.Signature( + 'Cecil Committer', 'cecil@committers.tld') + repo.create_commit( + 'refs/heads/%s' % branch_from, + author, + committer, + 'A commit on branch %s' % branch_from, + tree, + [first_commit.oid.hex] + ) + refname = 'refs/heads/%s' % (branch_from) + ori_remote = repo.remotes[0] + PagureRepo.push(ori_remote, refname) + + @patch('pagure.lib.notify.send_email', MagicMock(return_value=True)) + def test_new_remote_pr_unauth(self): + """ Test creating a new remote PR un-authenticated. """ + + tests.create_projects(self.session) + tests.create_projects_git( + os.path.join(self.path, 'requests'), bare=True) + self.set_up_git_repo() + + # Before + project = pagure.lib.get_authorized_project(self.session, 'test') + self.assertEqual(len(project.requests), 0) + + # Try creating a remote PR + output = self.app.get('/test/diff/remote') + self.assertEqual(output.status_code, 302) + self.assertIn( + u'You should be redirected automatically to target URL: ' + u'New remote pull-request', output.data) + + csrf_token = self.get_csrf(output=output) + data = { + 'csrf_token': csrf_token, + 'title': 'Remote PR title', + 'branch_from': 'feature', + 'branch_to': 'master', + 'git_repo': os.path.join(self.newpath, 'test'), + } + output = self.app.post('/test/diff/remote', data=data) + self.assertEqual(output.status_code, 200) + self.assertIn(u'

Create pull request

', output.data) + self.assertIn( + u'
', output.data) + self.assertNotIn( + u'
', output.data) + + # Not saved yet + self.session = pagure.lib.create_session(self.dbpath) + project = pagure.lib.get_authorized_project(self.session, 'test') + self.assertEqual(len(project.requests), 0) + + data = { + 'csrf_token': csrf_token, + 'title': 'Remote PR title', + 'branch_from': 'feature', + 'branch_to': 'master', + 'git_repo': os.path.join(self.newpath, 'test'), + 'confirm': 1, + } + output = self.app.post( + '/test/diff/remote', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + u'

PR#1', + output.data) + self.assertIn( + u'
', output.data) + self.assertNotIn( + u'
', output.data) + + # Show the filename in the diff view + self.assertIn( + u'''
+ .gitignore +
+ this is a remote pull-request, so we cannot provide you''', + output.data) + self.assertIn( + u'''
+ sources +
+ this is a remote pull-request, so we cannot provide you''', + output.data) + # Show the filename in the Changes summary + self.assertIn( + u'.gitignore', output.data) + self.assertIn( + u'sources', output.data) + + # Remote PR Created + self.session = pagure.lib.create_session(self.dbpath) + project = pagure.lib.get_authorized_project(self.session, 'test') + self.assertEqual(len(project.requests), 1) + + +if __name__ == '__main__': + unittest.main(verbosity=2) From 675b89947a7722b780eb986b078bba825c60df4a Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 28 2018 07:52:31 +0000 Subject: [PATCH 6/6] Make the tests use beautifulsoup rather than regex Signed-off-by: Pierre-Yves Chibon --- diff --git a/tests/__init__.py b/tests/__init__.py index f93407e..520e6d3 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -38,6 +38,7 @@ import mock import pygit2 import redis +from bs4 import BeautifulSoup from celery.app.task import EagerResult from sqlalchemy import create_engine from sqlalchemy.orm import sessionmaker @@ -102,24 +103,25 @@ def get_wait_target(html): return found[-1] -POST_REGEX = re.compile("""') def get_post_args(html): - """ This parses from the wait page the hidden arguments for the form. """ - found = INPUT_REGEX.findall(html) - if len(found) == 0: - raise Exception("Not able to get the POST arguments in %s" % html) + """ This parses the wait page for the hidden arguments of the form. """ + soup = BeautifulSoup(html, 'html.parser') output = {} - for key, val in found: - output[key] = val + inputs = soup.find_all('input') + if not inputs: + raise Exception("Not able to get the POST arguments in %s" % html) + for inp in inputs: + if inp.get('type') == 'hidden': + output[inp.get('name')] = inp.get('value') return output