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.
postgres · JelteF · Jan 12, 2025 · Jan 12, 2025 · Jan 12, 2025 · Jan 12, 2025
commit bf5f948d61579b2e6b51b96e88cbcbc79a49f0b0
diff --git a/pgcommitfest/commitfest/models.py b/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):

diff --git a/pgcommitfest/commitfest/templates/commitfest.html b/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>

diff --git a/pgcommitfest/commitfest/templates/patch.html b/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>

diff --git a/pgcommitfest/commitfest/views.py b/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):

diff --git a/pgcommitfest/urls.py b/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),