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 in the email
   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. Places that use the stable URLs require an extra round-trip to
   actually get to the patch page.
3. 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.
4. 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/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)
+    cf = patch.current_commitfest()
+
+    patch_commitfests = PatchOnCommitFest.objects.select_related('commitfest').filter(patch=patch).order_by('-commitfest__startdate').all()
+
     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,9 +466,9 @@ 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')
 
@@ -562,8 +563,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)
+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 poc.is_closed:
         # We allow modification of patches in closed CFs *only* if it's the
@@ -597,8 +600,10 @@ def status(request, cfid, patchid, status):
 
 @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)
+def close(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 poc.is_closed:
         # We allow modification of patches in closed CFs *only* if it's the
@@ -695,8 +700,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 +719,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 +743,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 +756,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,18 +19,18 @@
     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),