WIP: Make /patch/{id} the URL for a patch

Previously we'd include the ID of the commitfest in the URL of the
patch. In 9f12a5e we introduced a stable URL for patches that would
redirect to the one for the latest commitfest. This starts to use that
URL as the valid only URL for a patch (with the previous URL redirecting
to this one).

The reasoning behind this is that the old approach resulted in N
different URLs for each patch, which all showed the exact same patch
information. The only difference between all these URLs would be the
breadcrumb at the top of the page.

The only benefit of that approach is that if you're on an old
commitfest, and click a link there, then the breadcrumb will bring you
back to where you came from. Since people rarely have a reason to browse
closed commitfests, the that benefit seems pretty small. Especially
because people can just as well press their browser back button, in that
case.

The problems that these N links cause seem much more impactful to most
users:

1. If you click an old link to a cf entry (e.g. one from an email in the
   archives), then the breadcrumb will contain some arbitrarily old
   commitfest. It seems much more useful to have the breadcrumb show the
   commitfest that the patch is currently active in (or got
   committed/rejected in).
2. If you arrive on such an old link you also won't be able to change
   the status. Instead you'd get a message like: "The status of this
   patch cannot be changed in this commitfest. You must modify it in the
   one where it's open!". Which requires you to go to the latest page.
3. Places that use the stable URLs require an extra round-trip to
   actually get to the patch page.
4. It's a bit confusing that old pages of a patch still get updated with
   all the new information, i.e. why have all these pages if they
   contain the exact same content.
5. Problem 3 is generally also bad for Search Engine Optimization (SEO),
   for now we don't care much about that though.

Finally this also changes the links on the patch page itself for each of
the commitfests that a patch has been part of. Those links were already
rather useless, since all they effectively did was change the
breadcrumb. But with this new commit, they wouldn't even do that anymore,
and simply redirect to the current page. So now they start pointing to
the commitfest itself, which seems more useful behaviour anyway.

Author: JelteF

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

@@ -85,7 +85,7 @@ <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>

pgcommitfest/commitfest/templates/patch.html

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

pgcommitfest/commitfest/templates/patch_commands.inc

@@ -21,7 +21,7 @@
   <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>
@@ -37,4 +37,4 @@
 </div>
 {%endif%}
 
-</div>
+</div>

pgcommitfest/commitfest/views.py

@@ -320,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)
@@ -373,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())
@@ -465,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)
@@ -562,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
@@ -592,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()
 
@@ -695,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()
@@ -715,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))
@@ -740,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':
@@ -754,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_redirect),
-    re_path(r'^(\d+)/(\d+)/$', views.patch),
-    re_path(r'^(\d+)/(\d+)/edit/$', views.patchform),
+    re_path(r'^(\d+)/(\d+)/$', views.patch_legacy_redirect),
+    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),