Change the direction of the dropdowns at the bottom of the patch page.

Author: JelteF

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%;
+}
 
 
 /*

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/patch.html

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

@@ -374,9 +374,9 @@ def patch(request, 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())
@@ -466,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)
@@ -563,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
@@ -593,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()
 
@@ -696,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()
@@ -716,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))
@@ -741,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':
@@ -755,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):

pgcommitfest/urls.py

@@ -19,25 +19,32 @@
     re_path(r'^(\d+)/$', views.commitfest),
     re_path(r'^(open|inprogress|current)/(.*)$', views.redir),
     re_path(r'^(?P<cfid>\d+)/activity(?P<rss>\.rss)?/$', views.activity),
-    re_path(r'^patch/(\d+)/$', views.patch),
     re_path(r'^(\d+)/(\d+)/$', views.patch_legacy_redirect),
-    re_path(r'^(\d+)/(\d+)/edit/$', views.patchform),
+    re_path(r'^patch/(\d+)/$', views.patch),
+    re_path(r'^patch/(\d+)/edit/$', views.patchform),
     re_path(r'^(\d+)/new/$', views.newpatch),
-    re_path(r'^(\d+)/(\d+)/status/(review|author|committer)/$', views.status),
-    re_path(r'^(\d+)/(\d+)/close/(reject|withdrawn|feedback|committed|next)/$', views.close),
-    re_path(r'^(\d+)/(\d+)/reviewer/(become|remove)/$', views.reviewer),
-    re_path(r'^(\d+)/(\d+)/committer/(become|remove)/$', views.committer),
-    re_path(r'^(\d+)/(\d+)/(un)?subscribe/$', views.subscribe),
-    re_path(r'^(\d+)/(\d+)/(comment|review)/', views.comment),
+    re_path(r'^patch/(\d+)/status/(review|author|committer)/$', views.status),
+    re_path(r'^patch/(\d+)/close/(reject|withdrawn|feedback|committed|next)/$', views.close),
+    re_path(r'^patch/(\d+)/reviewer/(become|remove)/$', views.reviewer),
+    re_path(r'^patch/(\d+)/committer/(become|remove)/$', views.committer),
+    re_path(r'^patch/(\d+)/(un)?subscribe/$', views.subscribe),
+    re_path(r'^patch/(\d+)/(comment|review)/', views.comment),
     re_path(r'^(\d+)/send_email/$', views.send_email),
-    re_path(r'^(\d+)/\d+/send_email/$', views.send_email),
+    re_path(r'^patch/(\d+)/send_email/$', views.send_patch_email),
     re_path(r'^(\d+)/reports/authorstats/$', reports.authorstats),
     re_path(r'^search/$', views.global_search),
     re_path(r'^ajax/(\w+)/$', ajax.main),
     re_path(r'^lookups/user/$', lookups.userlookup),
     re_path(r'^thread_notify/$', views.thread_notify),
     re_path(r'^cfbot_notify/$', views.cfbot_notify),
 
+    # Legacy email POST route. This can be safely removed in a few days from
+    # the first time this is deployed. It's only puprose is not breaking
+    # submissions from a previous page lood, during the deploy of the new
+    # /patch/(\d+) routes. It would be a shame if someone lost their well
+    # written email because of this.
+    re_path(r'^\d+/(\d+)/send_email/$', views.send_patch_email),
+
     # Auth system integration
     re_path(r'^(?:account/)?login/?$', pgcommitfest.auth.login),
     re_path(r'^(?:account/)?logout/?$', pgcommitfest.auth.logout),