pagure

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

Download 3152.patch

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

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

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

1 new commit added

  • Add tests for deleting branch after merge

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

rebased onto 7c6b11ff5c3aec5195283c4c165225ea84ca9835

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

Pull-Request has been merged by pingou
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.