From bd4cd5ebf98da20163a312f7e84b5a0e7a4eb188 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 08 2018 10:00:41 +0000 Subject: [PATCH 1/8] When creating a new PR, allow updating the branch from In the same way we currently allow updating the branch targeted by the pull-request, this allows updating the branch from which the changes are pulled. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/templates/pull_request.html b/pagure/templates/pull_request.html index c01e751..e091d60 100644 --- a/pagure/templates/pull_request.html +++ b/pagure/templates/pull_request.html @@ -176,7 +176,14 @@ - {{ branch_from }} + into @@ -221,7 +228,8 @@

- + {{ form.csrf_token }} Date: Mar 08 2018 10:00:41 +0000 Subject: [PATCH 2/8] Allow pull changes from a different repo than the parent one This will allow opening pull-request from a fork to another fork without the first fork being a fork of the second, they only share the original parent. This could also work for people wanting to open a pull-request against multiple forks of the same repo. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 81dc479..ca25983 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -4973,3 +4973,37 @@ def get_authorized_project( return None return repo + + +def get_project_family(session, project): + ''' Retrieve the family of the specified project, ie: all the forks + of the main project. + If the specified project is a fork, let's work our way up the chain + until we find the main project so we can go down and get all the forks + and the forks of the forks (but not one level more). + + :arg session: The SQLAlchemy session to use + :type session: sqlalchemy.orm.session.Session + :arg project: The project whose family is searched + :type project: pagure.lib.model.Project + + ''' + parent = project + while parent.is_fork: + parent = parent.parent + + sub = session.query( + sqlalchemy.distinct(model.Project.id), + ).filter( + model.Project.parent_id == parent.id, + ) + query = session.query( + model.Project, + ).filter( + sqlalchemy.or_( + model.Project.parent_id.in_(sub.subquery()), + model.Project.parent_id == parent.id, + ) + ) + + return query.all() + [parent] diff --git a/pagure/templates/pull_request.html b/pagure/templates/pull_request.html index e091d60..fab410e 100644 --- a/pagure/templates/pull_request.html +++ b/pagure/templates/pull_request.html @@ -187,8 +187,7 @@ into - {% if repo.is_fork -%}{{ repo.parent.fullname }}{% - else %}{{ repo.fullname }}{% endif %} + {{ parent.fullname }}   ', + output.data) + + csrf_token = self.get_csrf(output=output) + + # Case 1 - Add an initial comment + data = { + 'csrf_token': csrf_token, + 'title': 'foo bar PR', + 'initial_comment': 'Test Initial Comment', + } + + output = self.app.post( + '/fork/ralph/test/diff/master..feature', + data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + 'PR#1: foo bar PR - fork/foo/test\n - Pagure', + output.data) + self.assertIn('

Test Initial Comment

', output.data) + + @patch('pagure.lib.notify.send_email') def test_new_request_pull_empty_repo(self, send_email): """ Test the new_request_pull endpoint against an empty repo. """ send_email.return_value = True @@ -1833,8 +2000,8 @@ index 0000000..2a552bb follow_redirects=True) self.assertEqual(output.status_code, 400) self.assertIn( - '

No branch from which to pull or local PR reference ' - 'were found

', output.data) + u'

Fork is empty, there are no commits to create a pull ' + u'request with

', output.data) output = self.app.get('/test/new_issue') csrf_token = self.get_csrf(output=output) @@ -1848,8 +2015,8 @@ index 0000000..2a552bb '/test/diff/master..feature', data=data, follow_redirects=True) self.assertEqual(output.status_code, 400) self.assertIn( - '

No branch from which to pull or local PR reference ' - 'were found

', output.data) + u'

Fork is empty, there are no commits to create a pull ' + u'request with

', output.data) shutil.rmtree(newpath) @@ -1883,8 +2050,8 @@ index 0000000..2a552bb '/fork/foo/test/diff/master..master', follow_redirects=True) self.assertEqual(output.status_code, 400) self.assertIn( - '

No branch from which to pull or local PR reference ' - 'were found

', output.data) + u'

Fork is empty, there are no commits to create a pull ' + u'request with

', output.data) shutil.rmtree(newpath) From b8e1026ff03903b0e2128249fd853fcf03266624 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 08 2018 10:05:22 +0000 Subject: [PATCH 5/8] Fix opening a pull-request against another project than the parent one Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/ui/fork.py b/pagure/ui/fork.py index 1932c8f..06bdd02 100644 --- a/pagure/ui/fork.py +++ b/pagure/ui/fork.py @@ -1102,15 +1102,6 @@ def new_request_pull( if repo.parent: parent = repo.parent - if not parent.settings.get('pull_requests', True): - flask.abort(404, 'No pull-request allowed on this project') - - if parent.settings.get( - 'Enforce_signed-off_commits_in_pull-request', False): - flask.flash( - 'This project enforces the Signed-off-by statement on all ' - 'commits') - repo_obj = flask.g.repo_obj if not project_to: @@ -1121,8 +1112,8 @@ def new_request_pull( p_username = None p_name = None project_to = project_to.rstrip('/') - if project_to.startswith('forks/'): - tmp = project_to.split('forks/')[1] + if project_to.startswith('fork/'): + tmp = project_to.split('fork/')[1] p_username, left = tmp.split('/', 1) else: left = project_to @@ -1139,19 +1130,28 @@ def new_request_pull( ) if parent: family = [ - p.fullname for p in + p.url_path for p in pagure.lib.get_project_family(flask.g.session, repo) ] - if parent.fullname not in family: + if parent.url_path not in family: flask.abort( 400, '%s is not part of %s\'s family' % ( - project_to, repo.fullname)) + project_to, repo.url_path)) orig_repo = pygit2.Repository(os.path.join( pagure_config['GIT_FOLDER'], parent.path)) else: flask.abort(404, 'No project found for %s' % project_to) + if not parent.settings.get('pull_requests', True): + flask.abort(404, 'No pull-request allowed on this project') + + if parent.settings.get( + 'Enforce_signed-off_commits_in_pull-request', False): + flask.flash( + 'This project enforces the Signed-off-by statement on all ' + 'commits') + try: diff, diff_commits, orig_commit = pagure.lib.git.get_diff_info( repo_obj, orig_repo, branch_from, branch_to) From 031d540137f44c48527bae706e44fa75a7cb18c6 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 08 2018 10:05:22 +0000 Subject: [PATCH 6/8] Add unit-tests testing opening a PR from a fork against another This without the two forks being related at any other levels than being from the same family. Signed-off-by: Pierre-Yves Chibon --- diff --git a/tests/test_pagure_flask_ui_fork.py b/tests/test_pagure_flask_ui_fork.py index 9524efb..92c3eeb 100644 --- a/tests/test_pagure_flask_ui_fork.py +++ b/tests/test_pagure_flask_ui_fork.py @@ -1971,6 +1971,188 @@ index 0000000..2a552bb self.assertIn('

Test Initial Comment

', output.data) @patch('pagure.lib.notify.send_email') + def test_new_request_pull_fork_to_other_fork(self, send_email): + """ Test creating a PR from fork to a fork of the same family. """ + send_email.return_value = True + + self.test_fork_project() + + # Create a 3rd user + item = pagure.lib.model.User( + user='ralph', + fullname='Ralph bar', + password='ralph_foo', + default_email='ralph@bar.com', + ) + self.session.add(item) + item = pagure.lib.model.UserEmail( + user_id=3, + email='ralph@bar.com') + self.session.add(item) + self.session.commit() + + user = tests.FakeUser() + user.username = 'ralph' + with tests.user_set(self.app.application, user): + csrf_token = self.get_csrf() + data = { + 'csrf_token': csrf_token, + } + + output = self.app.post( + '/do_fork/test', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 200) + + # Check that Ralph's fork do exist + output = self.app.get('/fork/ralph/test') + self.assertEqual(output.status_code, 200) + + tests.create_projects_git( + os.path.join(self.path, 'requests'), bare=True) + + # Turn on pull-request on the fork + repo = pagure.lib.get_authorized_project( + self.session, 'test', user='foo') + settings = repo.settings + settings['pull_requests'] = True + repo.settings = settings + self.session.add(repo) + self.session.commit() + + # Add some content to the parents + self.set_up_git_repo( + new_project=repo, branch_from='master', mtype='FF') + self.set_up_git_repo( + new_project=repo, branch_from='master', mtype='FF', + name_from=repo.fullname, prid=2) + + fork = pagure.lib.get_authorized_project( + self.session, 'test', user='ralph') + + self.set_up_git_repo( + new_project=fork, branch_from='feature', mtype='FF', + prid=3, name_from=fork.fullname) + + # Try opening a pull-request + output = self.app.get( + '/fork/ralph/test/diff/master..feature?project_to=fork/foo/test') + self.assertEqual(output.status_code, 200) + self.assertIn( + 'Create new Pull Request for master - fork/ralph/test\n - ' + 'Pagure', output.data) + self.assertIn( + '', + output.data) + + csrf_token = self.get_csrf(output=output) + + # Case 1 - Opening PR to fork/foo/test + data = { + 'csrf_token': csrf_token, + 'title': 'foo bar PR', + 'initial_comment': 'Test Initial Comment', + } + + output = self.app.post( + '/fork/ralph/test/diff/master..feature?project_to=fork/foo/test', + data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + 'PR#1: foo bar PR - fork/foo/test\n - Pagure', + output.data) + self.assertIn('

Test Initial Comment

', output.data) + + # Case 1 - Opening PR to parent repo, shows project_to works + output = self.app.post( + '/fork/ralph/test/diff/master..feature', + data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + 'PR#4: foo bar PR - test\n - Pagure', + output.data) + self.assertIn('

Test Initial Comment

', output.data) + + @patch('pagure.lib.notify.send_email') + def test_new_request_pull_fork_to_other_unrelated_fork(self, send_email): + """ Test creating a PR from fork to fork that isn't from the same + family. + """ + send_email.return_value = True + + self.test_fork_project() + + # Create a 3rd user + item = pagure.lib.model.User( + user='ralph', + fullname='Ralph bar', + password='ralph_foo', + default_email='ralph@bar.com', + ) + self.session.add(item) + item = pagure.lib.model.UserEmail( + user_id=3, + email='ralph@bar.com') + self.session.add(item) + self.session.commit() + + user = tests.FakeUser() + user.username = 'ralph' + with tests.user_set(self.app.application, user): + csrf_token = self.get_csrf() + data = { + 'csrf_token': csrf_token, + } + + output = self.app.post( + '/do_fork/test2', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 200) + + # Check that Ralph's fork do exist + output = self.app.get('/fork/ralph/test2') + self.assertEqual(output.status_code, 200) + + tests.create_projects_git( + os.path.join(self.path, 'requests'), bare=True) + + # Turn on pull-request on the fork + repo = pagure.lib.get_authorized_project( + self.session, 'test', user='foo') + settings = repo.settings + settings['pull_requests'] = True + repo.settings = settings + self.session.add(repo) + self.session.commit() + + # Add some content to the parent + self.set_up_git_repo( + new_project=repo, branch_from='master', mtype='FF', + name_from=repo.fullname) + + fork = pagure.lib.get_authorized_project( + self.session, 'test2', user='ralph') + + self.set_up_git_repo( + new_project=fork, branch_from='feature', mtype='FF', + prid=2, name_from=fork.fullname) + + # Case 1 - Opening PR to fork/foo/test + data = { + 'csrf_token': csrf_token, + 'title': 'foo bar PR', + 'initial_comment': 'Test Initial Comment', + } + + output = self.app.post( + '/fork/ralph/test2/diff/master..feature?project_to=fork/foo/test', + data=data, follow_redirects=True) + self.assertEqual(output.status_code, 400) + self.assertIn( + u"

fork/foo/test is not part of fork/ralph/test2's " + u"family

", output.data) + + @patch('pagure.lib.notify.send_email') def test_new_request_pull_empty_repo(self, send_email): """ Test the new_request_pull endpoint against an empty repo. """ send_email.return_value = True From 340c0e3d4860c5c10d855d2e9d7a908f65cbbd3c Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 08 2018 10:05:22 +0000 Subject: [PATCH 7/8] Add UI support for opening pull-requests against other family members This way users can change the target of the pull-request from either the same or main project to another family member (ie: main project, the direct forks and the forks of the forks, we don't go further than that). Fixes https://pagure.io/pagure/issue/2964 Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/static/pagure.css b/pagure/static/pagure.css index 1a95640..34ac8c6 100644 --- a/pagure/static/pagure.css +++ b/pagure/static/pagure.css @@ -596,6 +596,7 @@ blockquote { margin-top: -4px; color: #999; } + .msg-git-hash{ font-family:monospace; background-color:#eee; @@ -696,6 +697,11 @@ a.nav-link.btn{ background-color:#ddd; } +.semi_link { + color: #0275d8; + cursor: pointer; +} + .codehilite .hll { background-color: #ffffcc } .codehilite { background: #f8f8f8; } .codehilite .c { color: #8f5902; font-style: italic } /* Comment */ diff --git a/pagure/templates/pull_request.html b/pagure/templates/pull_request.html index 8f17577..ad79d65 100644 --- a/pagure/templates/pull_request.html +++ b/pagure/templates/pull_request.html @@ -163,7 +163,9 @@ repo=repo.name, username=username, namespace=repo.namespace, - commitid=commitid, branch_from=branch_from, + commitid=commitid, + branch_from=branch_from, + project_to=project_to, branch_to=branch_to) }}" method="post"> {% endif %}
@@ -187,7 +189,10 @@ into - {{ parent.fullname }} + + {{ parent.fullname }} +   {{ form.csrf_token }}
' + + res.family[el] + + '' + + '
'; + _t = _t.replace('---', res.family[el]); + _text += _t; + } + var _el = $('#family_list'); + _el.html(_text); + }, + }); + }); + {% endif %} }); From 6a17d806b69b244fd133f40b96281f4a93625ac2 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 08 2018 10:05:22 +0000 Subject: [PATCH 8/8] If the repo is set, include a generic "Open Pull-Request" button Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/templates/repo_info.html b/pagure/templates/repo_info.html index 6130351..c49d430 100644 --- a/pagure/templates/repo_info.html +++ b/pagure/templates/repo_info.html @@ -155,6 +155,18 @@ git push -u origin master {% endfor %} + {% if head %} +
+ Open Pull-Request + +
+ {% endif %}
Source GIT URLs{% if (authenticated and g.repo_committer) or (config['DOC_APP_URL'] and repo and diff --git a/tests/test_pagure_flask_ui_repo.py b/tests/test_pagure_flask_ui_repo.py index b424d40..b30e35f 100644 --- a/tests/test_pagure_flask_ui_repo.py +++ b/tests/test_pagure_flask_ui_repo.py @@ -4940,6 +4940,33 @@ index 0000000..fb7093d self.session, project_name='test', namespace='foo') self.assertEqual(project.reports, {}) + def test_open_pr_button_empty_repo(self): + """ Test "Open Pull-Request" button on empty project. """ + + tests.create_projects(self.session) + tests.create_projects_git(os.path.join(self.path, 'repos'), bare=True) + + output = self.app.get('/test') + self.assertEqual(output.status_code, 200) + self.assertIn(u'

This repo is brand new!

', output.data) + self.assertNotIn( + u'href="/test/diff/master..master">Open Pull-Request', + output.data) + + def test_open_pr_button(self): + """ Test "Open Pull-Request" button on non-empty project. """ + + tests.create_projects(self.session) + tests.create_projects_git(os.path.join(self.path, 'repos'), bare=True) + path = os.path.join(self.path, 'repos', 'test.git') + tests.add_content_git_repo(path) + + output = self.app.get('/test') + self.assertEqual(output.status_code, 200) + self.assertNotIn(u'

This repo is brand new!

', output.data) + self.assertIn( + u'href="/test/diff/master..master">Open Pull-Request', + output.data) if __name__ == '__main__': unittest.main(verbosity=2)