From 7c6b11ff5c3aec5195283c4c165225ea84ca9835 Mon Sep 17 00:00:00 2001 From: Lubomír Sedlář Date: Apr 26 2018 08:43:07 +0000 Subject: [PATCH 1/5] Allow deleting branch when PR is merged This is only allowed when branch deletion is not disabled, when user has commit access to the source repository and the pull request is not remote. --- diff --git a/pagure/forms.py b/pagure/forms.py index 0afd626..e505967 100644 --- a/pagure/forms.py +++ b/pagure/forms.py @@ -786,3 +786,11 @@ class SubscribtionForm(PagureForm): [wtforms.validators.optional()], false_values=FALSE_VALUES, ) + + +class MergePRForm(PagureForm): + delete_branch = wtforms.BooleanField( + 'Delete branch after merging', + [wtforms.validators.optional()], + false_values=FALSE_VALUES, + ) diff --git a/pagure/lib/tasks.py b/pagure/lib/tasks.py index 727e80b..8f9b93d 100644 --- a/pagure/lib/tasks.py +++ b/pagure/lib/tasks.py @@ -676,7 +676,7 @@ def refresh_pr_cache(self, session, name, namespace, user): @conn.task(queue=pagure_config.get('FAST_CELERY_QUEUE', None), bind=True) @pagure_task def merge_pull_request(self, session, name, namespace, user, requestid, - user_merger): + user_merger, delete_branch_after=False): """ Merge pull-request. """ project = pagure.lib._get_project( @@ -692,6 +692,15 @@ def merge_pull_request(self, session, name, namespace, user, requestid, pagure.lib.git.merge_pull_request( session, request, user_merger, pagure_config['REQUESTS_FOLDER']) + if delete_branch_after: + _log.debug('Will delete source branch of pull-request: %s/#%s', + request.project.fullname, request.id) + delete_branch.delay( + request.project_from.name, + request.project_from.namespace, + request.project_from.user.username if request.project_from.parent else None, + request.branch_from) + refresh_pr_cache.delay(name, namespace, user) return ret( 'ui_ns.view_repo', repo=name, username=user, namespace=namespace) diff --git a/pagure/templates/pull_request.html b/pagure/templates/pull_request.html index 13b1d1d..2f6ab1e 100644 --- a/pagure/templates/pull_request.html +++ b/pagure/templates/pull_request.html @@ -681,6 +681,9 @@ requestid=requestid) }}" method="POST"> {{ mergeform.csrf_token }} + {% if can_delete_branch %} + + {% endif %} diff --git a/pagure/ui/fork.py b/pagure/ui/fork.py index db8ef13..de40cf9 100644 --- a/pagure/ui/fork.py +++ b/pagure/ui/fork.py @@ -34,7 +34,7 @@ import pagure.forms from pagure.config import config as pagure_config from pagure.ui import UI_NS from pagure.utils import ( - login_required, __get_file_in_tree, get_parent_repo_path) + login_required, __get_file_in_tree, get_parent_repo_path, is_repo_committer) _log = logging.getLogger(__name__) @@ -232,7 +232,7 @@ def request_pull(repo, requestid, username=None, namespace=None): if diff: diff.find_similar() - form = pagure.forms.ConfirmationForm() + form = pagure.forms.MergePRForm() return flask.render_template( 'pull_request.html', @@ -247,6 +247,10 @@ def request_pull(repo, requestid, username=None, namespace=None): mergeform=form, subscribers=pagure.lib.get_watch_list(flask.g.session, request), tag_list=pagure.lib.get_tags_of_project(flask.g.session, repo), + can_delete_branch=(pagure_config.get('ALLOW_DELETE_BRANCH', True) + and not request.remote_git + and pagure.utils.is_repo_committer(request.project_from) + ) ) @@ -715,7 +719,7 @@ def merge_request_pull(repo, requestid, username=None, namespace=None): """ Create a pull request with the changes from the fork into the project. """ - form = pagure.forms.ConfirmationForm() + form = pagure.forms.MergePRForm() if not form.validate_on_submit(): flask.flash('Invalid input submitted', 'error') return flask.redirect(flask.url_for( @@ -764,12 +768,32 @@ def merge_request_pull(repo, requestid, username=None, namespace=None): 'ui_ns.request_pull', username=username, namespace=namespace, repo=repo.name, requestid=requestid)) + if form.delete_branch.data: + if not pagure_config.get('ALLOW_DELETE_BRANCH', True): + flask.flash( + 'This pagure instance does not allow branch deletion', 'error') + return flask.redirect(flask.url_for( + 'ui_ns.request_pull', username=username, namespace=namespace, + repo=repo.name, requestid=requestid)) + if not pagure.utils.is_repo_committer(request.project_from): + flask.flash( + 'You do not have permissions to delete the branch in the source repo', 'error') + return flask.redirect(flask.url_for( + 'ui_ns.request_pull', username=username, namespace=namespace, + repo=repo.name, requestid=requestid)) + if request.remote_git: + flask.flash( + 'You can not delete branch in remote repo', 'error') + return flask.redirect(flask.url_for( + 'ui_ns.request_pull', username=username, namespace=namespace, + repo=repo.name, requestid=requestid)) + _log.info('All checks in the controller passed') try: task = pagure.lib.tasks.merge_pull_request.delay( repo.name, namespace, username, requestid, - flask.g.fas_user.username) + flask.g.fas_user.username, delete_branch_after=form.delete_branch.data) return pagure.utils.wait_for_task( task, prev=flask.url_for('ui_ns.request_pull', From 7881005b79a50e977f093d22230a5de88200bc94 Mon Sep 17 00:00:00 2001 From: Lubomír Sedlář Date: Apr 26 2018 08:43:07 +0000 Subject: [PATCH 2/5] Hide branch delete checkbox if PR can not be merged --- diff --git a/pagure/templates/pull_request.html b/pagure/templates/pull_request.html index 2f6ab1e..23f2a27 100644 --- a/pagure/templates/pull_request.html +++ b/pagure/templates/pull_request.html @@ -1071,12 +1071,14 @@ function show_merge_status(){ $('#merge-alert').addClass("alert-danger"); $('#merge-alert-message').append(res.message); $('#merge-alert').show(); + $('#merge-alert label').hide(); } else if (res.code == 'NO_CHANGE') { $('#merge_btn').hide(); $('#merge-alert').addClass("alert-info"); $('#merge-alert-message').append(res.message); $('#merge-alert').show(); + $('#merge-alert label').hide(); } }; $('#spinner').show(); From 0c35e3e3d449c8fc471262991c6a1f28bbd04ee1 Mon Sep 17 00:00:00 2001 From: Lubomír Sedlář Date: Apr 26 2018 08:43:07 +0000 Subject: [PATCH 3/5] Remove extra label in the template --- diff --git a/pagure/templates/pull_request.html b/pagure/templates/pull_request.html index 23f2a27..68d55a0 100644 --- a/pagure/templates/pull_request.html +++ b/pagure/templates/pull_request.html @@ -681,16 +681,21 @@ requestid=requestid) }}" method="POST"> {{ mergeform.csrf_token }} - {% if can_delete_branch %} - - {% endif %} + + {% if can_delete_branch %} +
+ {{ mergeform.delete_branch }} {{ mergeform.delete_branch.label }} +
+ {% endif %} + {% else %} + {% endif %} - + {% if pull_request.status != 'Open'%}
Date: Apr 26 2018 08:43:07 +0000 Subject: [PATCH 4/5] Add tests for deleting branch after merge Two scenarios are tested: * branch successfully deleted after merge * merge rejected by conflict does not delete branch --- diff --git a/tests/test_pagure_flask_ui_fork.py b/tests/test_pagure_flask_ui_fork.py index 08d8265..41f9d32 100644 --- a/tests/test_pagure_flask_ui_fork.py +++ b/tests/test_pagure_flask_ui_fork.py @@ -482,6 +482,38 @@ class PagureFlaskForktests(tests.Modeltests): output.data) @patch('pagure.lib.notify.send_email') + def test_merge_request_pull_merge_with_delete_branch(self, send_email): + """ Test the merge_request_pull endpoint with a merge PR and delete source branch. """ + send_email.return_value = True + + tests.create_projects(self.session) + tests.create_projects_git( + os.path.join(self.path, 'requests'), bare=True) + self.set_up_git_repo( + new_project=None, branch_from='feature-branch', mtype='merge') + + user = tests.FakeUser() + user.username = 'pingou' + with tests.user_set(self.app.application, user): + output = self.app.get('/test/pull-request/1') + self.assertEqual(output.status_code, 200) + + data = { + 'csrf_token': self.get_csrf(output=output), + 'delete_branch': True, + } + + # Merge + output = self.app.post( + '/test/pull-request/1/merge', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + 'Overview - test - Pagure', output.data) + # Check the branch is not mentioned + self.assertNotIn( + 'PR#1\n' + ' PR from the feature-branch branch\n ', + output.data) + self.assertIn('Merge conflicts!', output.data) + + # Check the branch still exists + output = self.app.get('/test') + self.assertIn('feature-branch', output.data) + + @patch('pagure.lib.notify.send_email') def test_merge_request_pull_nochange(self, send_email): """ Test the merge_request_pull endpoint. """ send_email.return_value = True From ed7075b2f15e9adff88aa1a196c13bd2e2c3925b Mon Sep 17 00:00:00 2001 From: Lubomír Sedlář Date: Apr 26 2018 08:43:07 +0000 Subject: [PATCH 5/5] Fix PEP8 violations --- diff --git a/pagure/lib/tasks.py b/pagure/lib/tasks.py index 8f9b93d..c84ea61 100644 --- a/pagure/lib/tasks.py +++ b/pagure/lib/tasks.py @@ -695,10 +695,12 @@ def merge_pull_request(self, session, name, namespace, user, requestid, if delete_branch_after: _log.debug('Will delete source branch of pull-request: %s/#%s', request.project.fullname, request.id) + owner = (request.project_from.user.username + if request.project_from.parent else None) delete_branch.delay( request.project_from.name, request.project_from.namespace, - request.project_from.user.username if request.project_from.parent else None, + owner, request.branch_from) refresh_pr_cache.delay(name, namespace, user) diff --git a/pagure/ui/fork.py b/pagure/ui/fork.py index de40cf9..fc023ea 100644 --- a/pagure/ui/fork.py +++ b/pagure/ui/fork.py @@ -34,7 +34,7 @@ import pagure.forms from pagure.config import config as pagure_config from pagure.ui import UI_NS from pagure.utils import ( - login_required, __get_file_in_tree, get_parent_repo_path, is_repo_committer) + login_required, __get_file_in_tree, get_parent_repo_path) _log = logging.getLogger(__name__) @@ -234,6 +234,11 @@ def request_pull(repo, requestid, username=None, namespace=None): form = pagure.forms.MergePRForm() + can_delete_branch = ( + pagure_config.get('ALLOW_DELETE_BRANCH', True) + and not request.remote_git + and pagure.utils.is_repo_committer(request.project_from) + ) return flask.render_template( 'pull_request.html', select='requests', @@ -247,10 +252,7 @@ def request_pull(repo, requestid, username=None, namespace=None): mergeform=form, subscribers=pagure.lib.get_watch_list(flask.g.session, request), tag_list=pagure.lib.get_tags_of_project(flask.g.session, repo), - can_delete_branch=(pagure_config.get('ALLOW_DELETE_BRANCH', True) - and not request.remote_git - and pagure.utils.is_repo_committer(request.project_from) - ) + can_delete_branch=can_delete_branch, ) @@ -777,7 +779,8 @@ def merge_request_pull(repo, requestid, username=None, namespace=None): repo=repo.name, requestid=requestid)) if not pagure.utils.is_repo_committer(request.project_from): flask.flash( - 'You do not have permissions to delete the branch in the source repo', 'error') + 'You do not have permissions to delete the branch in the ' + 'source repo', 'error') return flask.redirect(flask.url_for( 'ui_ns.request_pull', username=username, namespace=namespace, repo=repo.name, requestid=requestid)) @@ -793,7 +796,8 @@ def merge_request_pull(repo, requestid, username=None, namespace=None): try: task = pagure.lib.tasks.merge_pull_request.delay( repo.name, namespace, username, requestid, - flask.g.fas_user.username, delete_branch_after=form.delete_branch.data) + flask.g.fas_user.username, + delete_branch_after=form.delete_branch.data) return pagure.utils.wait_for_task( task, prev=flask.url_for('ui_ns.request_pull',