#3152 Allow deleting branch when PR is merged
Merged 5 years ago by pingou. Opened 5 years ago by lsedlar.
lsedlar/pagure delete-pr-branch  into  master

file modified
+8
@@ -786,3 +786,11 @@ 

          [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,

+     )

file modified
+12 -1
@@ -676,7 +676,7 @@ 

  @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,17 @@ 

          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)

+         owner = (request.project_from.user.username

+                  if request.project_from.parent else None)

+         delete_branch.delay(

+             request.project_from.name,

+             request.project_from.namespace,

+             owner,

+             request.branch_from)

+ 

      refresh_pr_cache.delay(name, namespace, user)

      return ret(

          'ui_ns.view_repo', repo=name, username=user, namespace=namespace)

@@ -684,10 +684,18 @@ 

            <button id="merge_btn" type="submit"

              onclick="return confirm('Confirm merging this pull-request');"

              class="btn btn-block">Merge</button>

+           <small id="merge-alert-message"></small>

+           {% if can_delete_branch %}

+           <div class="small">

+           {{ mergeform.delete_branch }} {{ mergeform.delete_branch.label }}

+           </div>

+           {% endif %}

          </form>

         </div>

+       {% else %}

+       <small id="merge-alert-message"></small>

        {% endif %}

-         <small id="merge-alert-message"></small>

+ 

        </div>

        {% if pull_request.status != 'Open'%}

        <div class="alert {{'alert-success' if pull_request.status == 'Merged' else 'alert-danger'}}
@@ -1068,12 +1076,14 @@ 

          $('#merge-alert').addClass("alert-danger");

          $('#merge-alert-message').append(res.message);

          $('#merge-alert').show();

+         $('#merge-alert div.small').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 div.small').hide();

        }

    };

    $('#spinner').show();

file modified
+31 -3
@@ -232,8 +232,13 @@ 

      if diff:

          diff.find_similar()

  

-     form = pagure.forms.ConfirmationForm()

+     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,6 +252,7 @@ 

          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=can_delete_branch,

      )

  

  
@@ -715,7 +721,7 @@ 

      """ 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 +770,34 @@ 

              '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',

@@ -482,6 +482,38 @@ 

                  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(

+                 '<title>Overview - test - Pagure</title>', output.data)

+             # Check the branch is not mentioned

+             self.assertNotIn(

+                 '<a class="" href="/test/branch/feature-branch"', output.data)

+ 

+     @patch('pagure.lib.notify.send_email')

      def test_merge_request_pull_conflicts(self, send_email):

          """ Test the merge_request_pull endpoint with a conflicting PR. """

          send_email.return_value = True
@@ -515,6 +547,42 @@ 

              self.assertIn('Merge conflicts!', output.data)

  

      @patch('pagure.lib.notify.send_email')

+     def test_merge_request_pull_conflicts_with_delete_branch(self, send_email):

+         """ Test the merge_request_pull endpoint with a conflicting PR and request deletion of 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='conflicts')

+ 

+         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 conflicts

+             output = self.app.post(

+                 '/test/pull-request/1/merge', data=data, follow_redirects=True)

+             self.assertEqual(output.status_code, 200)

+             self.assertIn(

+                 '<h3><span class="label label-default">PR#1</span>\n'

+                 '  PR from the feature-branch branch\n     <span class="pull-xs-right">',

+                 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

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.

I guess this is a bit work-in-progress. If the idea is acceptable, it would require some tests.

Just to confirm, this will always delete the branch from, correct?

Does it also do it if the branch is in the fork?

There is a checkbox added above the merge button that allows users to choose if they want to delete the branch or not.

Now it should work from forks as well.

rebased onto 705751423a00ac434611b2a5f3cd55dd4d5f6599

5 years ago

The label tag here surprises me basically :(

None of the form helpers is really usable here. render_bootstrap_field kind of works, but looks really strange.

rebased onto 93b5c611a9c25738f92932547cef1a2f182fa9ae

5 years ago

None of the form helpers is really usable here. render_bootstrap_field kind of works, but looks really strange.

I can imagine this but label seems still wrong, I'll run this locally and see if I can help :)

My original idea was to replace the confirmation dialog with some bootstrap modal that would include the checkbox. Would you prefer that?

Let me see how it looks currently, I feel the dialog maybe a little tricky to implement

This crashes for remote PR, you'll likely have to invert the order

I like the feature and the idea. This is what I changed locally to make it (imho) prettier and working with remote PRs

diff --git a/ pagure/templates/pull_request.html b/ pagure/templates/pull_request.html
index 23f2a27a..48a45405 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 %}
-          <label>{{ mergeform.delete_branch }} {{ mergeform.delete_branch.label }}</label>
-          {% endif %}
           <button id="merge_btn" type="submit"
             onclick="return confirm('Confirm merging this pull-request');"
             class="btn btn-block">Merge</button>
+          <small id="merge-alert-message"></small>
+          {% if can_delete_branch %}
+          <div class="small">
+          {{ mergeform.delete_branch }} {{ mergeform.delete_branch.label }}
+          </div>
+          {% endif %}
         </form>
        </div>
+      {% else %}
+      <small id="merge-alert-message"></small>
       {% endif %}
-        <small id="merge-alert-message"></small>
+
       </div>
       {% if pull_request.status != 'Open'%}
       <div class="alert {{'alert-success' if pull_request.status == 'Merged' else 'alert-danger'}}
diff --git a/ pagure/ui/fork.py b/ pagure/ui/fork.py
index 204c15b0..b70805f1 100644
--- a/ pagure/ui/fork.py        
+++ b/ pagure/ui/fork.py        
@@ -248,8 +248,9 @@ def request_pull(repo, requestid, username=None, namespace=None):
         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)
-                           and not request.remote_git),
+                           ),
     )

Changes incorporated. I didn't notice that the .label property actually includes <label> tag. Also rebased on master. I'll work on some tests during the weekend.

rebased onto 1083b5936a0701fa26cdffd178cc7e3292cd1995

5 years ago

1 new commit added

  • Add tests for deleting branch after merge
5 years ago

I added basic tests for this feature. It would be cool to test it with a fork as well, but so far I'm having trouble with creating a fork in tests.

These are the errors found by the unit-tests:

14:05:08 Enforce PEP-8 compliance on the codebase. ... /root/pagure/pagure/lib/tasks.py:676:80: E501 line too long (88 > 79 characters)
14:05:08 /root/pagure/pagure/ui/fork.py:36:1: F401 'pagure.utils.is_repo_committer' imported but unused
14:05:08 /root/pagure/pagure/ui/fork.py:37:80: E501 line too long (80 > 79 characters)
14:05:08 /root/pagure/pagure/ui/fork.py:252:80: E501 line too long (83 > 79 characters)
14:05:08 /root/pagure/pagure/ui/fork.py:253:27: E124 closing bracket does not match visual indentation
14:05:08 /root/pagure/pagure/ui/fork.py:780:80: E501 line too long (95 > 79 characters)
14:05:08 /root/pagure/pagure/ui/fork.py:796:80: E501 line too long (83 > 79 characters)
14:05:08 FAIL

rebased onto c0f3f20c7a520e5e11bf5ba0f12660c0e2410681

5 years ago

5 new commits added

  • Fix PEP8 violations
  • Add tests for deleting branch after merge
  • Remove extra label in the template
  • Hide branch delete checkbox if PR can not be merged
  • Allow deleting branch when PR is merged
5 years ago

rebased onto 7c6b11f

5 years ago

Tests are passing and local testing as well, thanks :)

Pull-Request has been merged by pingou

5 years ago