Fix dropdowns at bottom of patch page

.editorconfig

@@ -0,0 +1,17 @@
+# top-most EditorConfig file
+root = true
+
+# basic rules for all files
+[*]
+end_of_line = lf
+insert_final_newline = true
+charset = utf-8
+trim_trailing_whitespace = true
+
+[*.{py,js}]
+indent_style = space
+indent_size = 4
+
+[*.html]
+indent_style = space
+indent_size = 1

README.md

@@ -6,7 +6,7 @@ A commitfest is a collection of patches and reviews for a project and is part of
 
 ## The Application
 
-This is a Django 3.2 application backed by PostgreSQL and running on Python 3.x.
+This is a Django 4.2 application backed by PostgreSQL and running on Python 3.x.
 
 ## Getting Started
 

media/commitfest/css/commitfest.css

@@ -37,6 +37,10 @@ div.form-group div.controls input.threadpick-input {
    display: inline;
 }
 
+.dropdown-menu--up {
+   top: initial;
+   bottom: 100%;
+}
 
 
 /*

media/commitfest/js/commitfest.js

@@ -222,7 +222,11 @@ function flagCommitted(committer) {
 
 
 function sortpatches(sortby) {
-   $('#id_sortkey').val(sortby);
+   if ($('#id_sortkey').val() == sortby) {
+      $('#id_sortkey').val(0);
+   } else {
+      $('#id_sortkey').val(sortby);
+   }
    $('#filterform').submit();
 
    return false;

pgcommitfest/commitfest/models.py

@@ -122,6 +122,9 @@ class Patch(models.Model, DiffableModel):
         'reviewers': 'reviewers_string',
     }
 
+    def current_commitfest(self):
+        return self.commitfests.order_by('-startdate').first()
+
     # Some accessors
     @property
     def authors_string(self):

pgcommitfest/commitfest/templates/commitfest.html

@@ -60,16 +60,17 @@ <h3>{{p.is_open|yesno:"Active patches,Closed patches"}}</h3>
 <table class="table table-striped table-bordered table-hover table-condensed">
  <thead>
   <tr>
-   <th>{%if p.is_open%}<a href="#" style="color:#333333;" onclick="return sortpatches(0);">Patch</a>{%if sortkey == 0%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}{%endif%}</th>
+   <th><a href="#" style="color:#333333;" onclick="return sortpatches(5);">Patch</a>{%if sortkey == 5%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}</th>
+   <th><a href="#" style="color:#333333;" onclick="return sortpatches(4);">ID</a>{%if sortkey == 4%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}</th>
    <th>Status</th>
    <th>Ver</th>
    <th>CI status</th>
    <th>Author</th>
    <th>Reviewers</th>
    <th>Committer</th>
-   <th>{%if p.is_open%}<a href="#" style="color:#333333;" onclick="return sortpatches(3);">Num cfs</a>{%if sortkey == 3%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}{%else%}Num cfs{%endif%}</th>
-   <th>{%if p.is_open%}<a href="#" style="color:#333333;" onclick="return sortpatches(1);">Latest activity</a>{%if sortkey == 1%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}{%else%}Latest activity{%endif%}</th>
-   <th>{%if p.is_open%}<a href="#" style="color:#333333;" onclick="return sortpatches(2);">Latest mail</a>{%if sortkey == 2%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}{%else%}Latest mail{%endif%}</th>
+   <th><a href="#" style="color:#333333;" onclick="return sortpatches(3);">Num cfs</a>{%if sortkey == 3%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}</th>
+   <th><a href="#" style="color:#333333;" onclick="return sortpatches(1);">Latest activity</a>{%if sortkey == 1%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}</th>
+   <th><a href="#" style="color:#333333;" onclick="return sortpatches(2);">Latest mail</a>{%if sortkey == 2%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}</th>
 {%if user.is_staff%}
    <th>Select</th>
 {%endif%}
@@ -84,19 +85,21 @@ <h3>{{p.is_open|yesno:"Active patches,Closed patches"}}</h3>
 {%endifchanged%}
 {%endif%}
   <tr>
-   <td><a href="{{p.id}}/">{{p.name}}</a></td>
+   <td><a href="/patch/{{p.id}}/">{{p.name}}</a></td>
+   <td>{{p.id}}</td>
    <td><span class="label label-{{p.status|patchstatuslabel}}">{{p.status|patchstatusstring}}</span></td>
    <td>{%if p.targetversion%}<span class="label label-default">{{p.targetversion}}</span>{%endif%}</td>
    <td class="cfbot-summary">
     {%if not p.cfbot_results %}
       <span class="label label-default">Not processed</span>
     {%elif p.cfbot_results.needs_rebase %}
-    <a href="{{p.cfbot_results.apply_url}}" title="View git apply logs">
+    <a href="{{p.cfbot_results.apply_url}}" target="_blank" title="View git apply logs">
      <span class="label label-warning">Needs rebase!</span>
     </a>
     {%else%}
-    <a href="https://github.com/postgresql-cfbot/postgresql/compare/cf/{{p.id}}~1...cf/{{p.id}}" title="View last patch set on GitHub"><img class="github-logo" src="/media/commitfest/github-mark.svg"/></a>
+    <a href="https://github.com/postgresql-cfbot/postgresql/compare/cf/{{p.id}}~1...cf/{{p.id}}" target="_blank" title="View last patch set on GitHub"><img class="github-logo" src="/media/commitfest/github-mark.svg"/></a>
     <a href="https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F{{p.id}}"
+       target="_blank"
        title="View CI history{%if p.cfbot_results.failed_task_names %}. Failed jobs: {{p.cfbot_results.failed_task_names}}{%endif%}">
       {%if p.cfbot_results.failed > 0 %}
       <img src="/media/commitfest/new_failure.svg"/>

pgcommitfest/commitfest/templates/patch.html

@@ -18,25 +18,25 @@
         {%if not cfbot_branch %}
         <span class="label label-default">Not processed</span></a>
         {%elif not cfbot_branch.commit_id %}
-          <a href="{{cfbot_branch.apply_url}}">
+          <a href="{{cfbot_branch.apply_url}}" target="_blank" title="View git apply logs">
           <span class="label label-warning" title="View git apply logs">Needs rebase!</span></a>
           Additional links previous successfully applied patch (outdated):
-          <a href="https://github.com/postgresql-cfbot/postgresql/compare/cf/{{patch.id}}~1...cf/{{patch.id}}" title="View previous successfully applied patch set on GitHub"><img class="github-logo" src="/media/commitfest/github-mark.svg"/></a>
-          <a href="https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F{{patch.id}}">
+          <a href="https://github.com/postgresql-cfbot/postgresql/compare/cf/{{patch.id}}~1...cf/{{patch.id}}" target="_blank" title="View previous successfully applied patch set on GitHub"><img class="github-logo" src="/media/commitfest/github-mark.svg"/></a>
+          <a href="https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F{{patch.id}}" target="_blank">
           <span class="label label-default">Summary</span></a>
         {%else%}
-        <a href="https://github.com/postgresql-cfbot/postgresql/compare/cf/{{patch.id}}~1...cf/{{patch.id}}" title="View last patch set on GitHub"><img class="github-logo" src="/media/commitfest/github-mark.svg"/></a>
-        <a href="https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F{{patch.id}}">
+        <a href="https://github.com/postgresql-cfbot/postgresql/compare/cf/{{patch.id}}~1...cf/{{patch.id}}" target="_blank" title="View last patch set on GitHub"><img class="github-logo" src="/media/commitfest/github-mark.svg"/></a>
+        <a href="https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F{{patch.id}}" target="_blank">
         <span class="label label-default">Summary</span></a>
         {%for c in cfbot_tasks %}
             {%if c.status == 'COMPLETED'%}
-            <a href="https://cirrusci.com/task/{{c.id}}" title="{{c.task_name}}: {{c.status}}"><img src="/media/commitfest/new_success.svg"/></a>
+            <a href="https://cirrusci.com/task/{{c.id}}" target="_blank" title="{{c.task_name}}: {{c.status}}"><img src="/media/commitfest/new_success.svg"/></a>
             {%elif c.status == 'CREATED' %}
-            <a href="https://cirrusci.com/task/{{c.id}}" title="{{c.task_name}}: {{c.status}}"><img src="/media/commitfest/waiting_to_start.svg"/></a>
+            <a href="https://cirrusci.com/task/{{c.id}}" target="_blank" title="{{c.task_name}}: {{c.status}}"><img src="/media/commitfest/waiting_to_start.svg"/></a>
             {%elif c.status == 'EXECUTING' %}
-            <a href="https://cirrusci.com/task/{{c.id}}" title="{{c.task_name}}: {{c.status}}"><img src="/media/commitfest/running.svg"/></a>
+            <a href="https://cirrusci.com/task/{{c.id}}" target="_blank" title="{{c.task_name}}: {{c.status}}"><img src="/media/commitfest/running.svg"/></a>
             {%else %}
-            <a href="https://cirrusci.com/task/{{c.id}}" title="{{c.task_name}}: {{c.status}}"><img src="/media/commitfest/new_failure.svg"/></a>
+            <a href="https://cirrusci.com/task/{{c.id}}" target="_blank" title="{{c.task_name}}: {{c.status}}"><img src="/media/commitfest/new_failure.svg"/></a>
             {%endif%}
         {%endfor%}
         {%endif%}
@@ -68,7 +68,7 @@
     <tr>
       <th>Status</th>
       <td>{%for c in patch_commitfests %}
-	<div style="margin-bottom: 3px;"><a href="/{{c.commitfest.id}}/{{patch.id}}/">{{c.commitfest}}</a>: <span class="label label-{{c.status|patchstatuslabel}}">{{c.statusstring}}</span></div>
+	<div style="margin-bottom: 3px;"><a href="/{{c.commitfest.id}}/">{{c.commitfest}}</a>: <span class="label label-{{c.status|patchstatuslabel}}">{{c.statusstring}}</span></div>
 	{%endfor%}
       </td>
     </tr>
@@ -183,7 +183,9 @@ <h4>Annotations</h4>
   </tbody>
 </table>
 
+{% with dropdown_mode="dropdown-menu--up" %}
 {%include "patch_commands.inc"%}
+{% endwith %}
 
 {%comment%}commit dialog{%endcomment%}
 <div class="modal fade" id="commitModal" role="dialog">

pgcommitfest/commitfest/templates/patch_commands.inc

@@ -3,15 +3,15 @@
 
 <div class="btn-group">
  <a class="btn btn-default dropdown-toggle" data-toggle="dropdown" href="#">Comment/Review <span class="caret"></span></a>
- <ul class="dropdown-menu">
+ <ul class="dropdown-menu {{ dropdown_mode }}">
   <li><a href="comment/">Comment</a>
   <li><a href="review/">Review</a>
  </ul>
 </div>
 
 <div class="btn-group">
  <a class="btn btn-default dropdown-toggle" data-toggle="dropdown" href="#">Change Status <span class="caret"></span></a>
- <ul class="dropdown-menu" role="menu">
+ <ul class="dropdown-menu {{ dropdown_mode }}" role="menu">
   <li role="presentation" class="dropdown-header">Open statuses</li>
   <li role="presentation"><a href="status/review/">Needs review</a></li>
   <li role="presentation"><a href="status/author/">Waiting on Author</a></li>
@@ -21,20 +21,20 @@
   <li role="presentation"><a href="close/reject/" onclick="return verify_reject()">Rejected</a></li>
   <li role="presentation"><a href="close/withdrawn/" onclick="return verify_withdrawn()">Withdrawn</a></li>
   <li role="presentation"><a href="close/feedback/" onclick="return verify_returned()">Returned with feedback</a></li>
-  <li role="presentation"><a href="close/next/" onclick="return verify_next()">Move to next CF</a></li>
+  <li role="presentation"><a href="close/next/?cfid={{cf.id}}" onclick="return verify_next()">Move to next CF</a></li>
   <li role="presentation"><a href="close/committed/" onclick="return flagCommitted({%if patch.committer%}'{{patch.committer}}'{%elif is_committer%}'{{user.username}}'{%else%}null{%endif%})">Committed</a></li>
  </ul>
 </div>
 
 {%if request.user.is_staff%}
 <div class="btn-group">
  <a class="btn btn-default dropdown-toggle" data-toggle="dropdown" href="#">Send private mail <span class="caret"></span></a>
- <ul class="dropdown-menu">
+ <ul class="dropdown-menu {{ dropdown_mode }}">
   <li><a href="send_email/?authors={{patch.id}}">Send mail to authors</a></li>
   <li><a href="send_email/?reviewers={{patch.id}}">Send mail to reviewers</a></li>
   <li><a href="send_email/?authors={{patch.id}}&reviewers={{patch.id}}">Send mail to authors and reviewers</a></li>
  </ul>
 </div>
 {%endif%}
 
-</div>
+</div>

pgcommitfest/commitfest/views.py

@@ -187,6 +187,10 @@ def commitfest(request, cfid):
             orderby_str = 'lastmail, created'
         elif sortkey == 3:
             orderby_str = 'num_cfs DESC, modified, created'
+        elif sortkey == 4:
+            orderby_str = 'p.id'
+        elif sortkey == 5:
+            orderby_str = 'p.name, created'
         else:
             orderby_str = 'p.id'
             sortkey = 0
@@ -316,17 +320,18 @@ def global_search(request):
     })
 
 
-def patch_redirect(request, patchid):
-    last_commitfest = PatchOnCommitFest.objects.select_related('commitfest').filter(patch_id=patchid).order_by('-commitfest__startdate').first()
-    if not last_commitfest:
-        raise Http404("Patch not found")
-    return HttpResponseRedirect(f'/{last_commitfest.commitfest_id}/{patchid}/')
+def patch_legacy_redirect(request, cfid, patchid):
+    # Previously we would include the commitfest id in the URL. This is no
+    # longer the case.
+    return HttpResponseRedirect(f'/patch/{patchid}/')
 
 
-def patch(request, cfid, patchid):
-    cf = get_object_or_404(CommitFest, pk=cfid)
-    patch = get_object_or_404(Patch.objects.select_related(), pk=patchid, commitfests=cf)
-    patch_commitfests = PatchOnCommitFest.objects.select_related('commitfest').filter(patch=patch).order_by('-commitfest__startdate')
+def patch(request, patchid):
+    patch = get_object_or_404(Patch.objects.select_related(), pk=patchid)
+
+    patch_commitfests = PatchOnCommitFest.objects.select_related('commitfest').filter(patch=patch).order_by('-commitfest__startdate').all()
+    cf = patch_commitfests[0].commitfest
+
     committers = Committer.objects.filter(active=True).order_by('user__last_name', 'user__first_name')
 
     cfbot_branch = getattr(patch, 'cfbot_branch', None)
@@ -369,9 +374,9 @@ def patch(request, cfid, patchid):
 
 @login_required
 @transaction.atomic
-def patchform(request, cfid, patchid):
-    cf = get_object_or_404(CommitFest, pk=cfid)
-    patch = get_object_or_404(Patch, pk=patchid, commitfests=cf)
+def patchform(request, patchid):
+    patch = get_object_or_404(Patch, pk=patchid)
+    cf = patch.current_commitfest()
 
     prevreviewers = list(patch.reviewers.all())
     prevauthors = list(patch.authors.all())
@@ -461,21 +466,12 @@ def _review_status_string(reviewstatus):
 
 @login_required
 @transaction.atomic
-def comment(request, cfid, patchid, what):
-    cf = get_object_or_404(CommitFest, pk=cfid)
+def comment(request, patchid, what):
     patch = get_object_or_404(Patch, pk=patchid)
+    cf = patch.current_commitfest()
     poc = get_object_or_404(PatchOnCommitFest, patch=patch, commitfest=cf)
     is_review = (what == 'review')
 
-    if poc.is_closed:
-        # We allow modification of patches in closed CFs *only* if it's the
-        # last CF that the patch is part of. If it's part of another CF, that
-        # is later than this one, tell the user to go there instead.
-        lastcf = PatchOnCommitFest.objects.filter(patch=patch).order_by('-commitfest__startdate')[0]
-        if poc != lastcf:
-            messages.add_message(request, messages.INFO, "The status of this patch cannot be changed in this commitfest. You must modify it in the one where it's open!")
-            return HttpResponseRedirect('..')
-
     if request.method == 'POST':
         try:
             form = CommentForm(patch, poc, is_review, data=request.POST)
@@ -558,17 +554,10 @@ def comment(request, cfid, patchid, what):
 
 @login_required
 @transaction.atomic
-def status(request, cfid, patchid, status):
-    poc = get_object_or_404(PatchOnCommitFest.objects.select_related(), commitfest__id=cfid, patch__id=patchid)
-
-    if poc.is_closed:
-        # We allow modification of patches in closed CFs *only* if it's the
-        # last CF that the patch is part of. If it's part of another CF, that
-        # is later than this one, tell the user to go there instead.
-        lastcf = PatchOnCommitFest.objects.filter(patch__id=patchid).order_by('-commitfest__startdate')[0]
-        if poc != lastcf:
-            messages.add_message(request, messages.INFO, "The status of this patch cannot be changed in this commitfest. You must modify it in the one where it's open!")
-            return HttpResponseRedirect('/%s/%s/' % (poc.commitfest.id, poc.patch.id))
+def status(request, patchid, status):
+    patch = get_object_or_404(Patch.objects.select_related(), pk=patchid)
+    cf = patch.current_commitfest()
+    poc = get_object_or_404(PatchOnCommitFest.objects.select_related(), commitfest__id=cf.id, patch__id=patchid)
 
     if status == 'review':
         newstatus = PatchOnCommitFest.STATUS_REVIEW
@@ -588,22 +577,29 @@ def status(request, cfid, patchid, status):
 
         PatchHistory(patch=poc.patch, by=request.user, what='New status: %s' % poc.statusstring).save_and_notify()
 
-    return HttpResponseRedirect('/%s/%s/' % (poc.commitfest.id, poc.patch.id))
+    return HttpResponseRedirect('/patch/%s/' % (poc.patch.id))
 
 
 @login_required
 @transaction.atomic
-def close(request, cfid, patchid, status):
-    poc = get_object_or_404(PatchOnCommitFest.objects.select_related(), commitfest__id=cfid, patch__id=patchid)
-
-    if poc.is_closed:
-        # We allow modification of patches in closed CFs *only* if it's the
-        # last CF that the patch is part of. If it's part of another CF, that
-        # is later than this one, tell the user to go there instead.
-        lastcf = PatchOnCommitFest.objects.filter(patch__id=patchid).order_by('-commitfest__startdate')[0]
-        if poc != lastcf:
-            messages.add_message(request, messages.INFO, "The status of this patch cannot be changed in this commitfest. You must modify it in the one where it's open!")
-            return HttpResponseRedirect('/%s/%s/' % (poc.commitfest.id, poc.patch.id))
+def close(request, patchid, status):
+    patch = get_object_or_404(Patch.objects.select_related(), pk=patchid)
+    cf = patch.current_commitfest()
+
+    try:
+        request_cfid = int(request.GET.get('cfid', ''))
+    except ValueError:
+        # int() failed, ignore
+        request_cfid = None
+
+    if request_cfid is not None and request_cfid != cf.id:
+        # The cfid parameter is only added to the /next/ link. That's the only
+        # close operation where two people pressing the button at the same time
+        # can have unintended effects.
+        messages.error(request, "The patch was moved to a new commitfest by someone else. Please double check if you still want to retry this operation.")
+        return HttpResponseRedirect('/%s/%s/' % (cf.id, patch.id))
+
+    poc = get_object_or_404(PatchOnCommitFest.objects.select_related(), commitfest__id=cf.id, patch__id=patchid)
 
     poc.leavedate = datetime.now()
 
@@ -691,8 +687,7 @@ def close(request, cfid, patchid, status):
 
 @login_required
 @transaction.atomic
-def reviewer(request, cfid, patchid, status):
-    get_object_or_404(CommitFest, pk=cfid)
+def reviewer(request, patchid, status):
     patch = get_object_or_404(Patch, pk=patchid)
 
     is_reviewer = request.user in patch.reviewers.all()
@@ -711,7 +706,6 @@ def reviewer(request, cfid, patchid, status):
 @login_required
 @transaction.atomic
 def committer(request, cfid, patchid, status):
-    get_object_or_404(CommitFest, pk=cfid)
     patch = get_object_or_404(Patch, pk=patchid)
 
     committer = list(Committer.objects.filter(user=request.user, active=True))
@@ -736,8 +730,7 @@ def committer(request, cfid, patchid, status):
 
 @login_required
 @transaction.atomic
-def subscribe(request, cfid, patchid, sub):
-    get_object_or_404(CommitFest, pk=cfid)
+def subscribe(request, patchid, sub):
     patch = get_object_or_404(Patch, pk=patchid)
 
     if sub == 'un':
@@ -750,6 +743,12 @@ def subscribe(request, cfid, patchid, sub):
     return HttpResponseRedirect("../")
 
 
+def send_patch_email(request, patchid):
+    patch = get_object_or_404(Patch, pk=patchid)
+    cf = patch.current_commitfest()
+    return send_email(request, cf.id)
+
+
 @login_required
 @transaction.atomic
 def send_email(request, cfid):