From ebbf020f0b916bd2b95bbf5cd4055bcfbea7cb21 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio Date: Sun, 12 Jan 2025 23:25:23 +0100 Subject: [PATCH] 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 9f12a5e83 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. --- pgcommitfest/commitfest/models.py | 3 + .../commitfest/templates/commitfest.html | 2 +- pgcommitfest/commitfest/templates/patch.html | 2 +- .../commitfest/templates/patch_commands.inc | 4 +- pgcommitfest/commitfest/views.py | 97 +++++++++---------- pgcommitfest/urls.py | 27 ++++-- 6 files changed, 70 insertions(+), 65 deletions(-) diff --git a/pgcommitfest/commitfest/models.py b/pgcommitfest/commitfest/models.py index c4c4355..0a7e9e0 100644 --- 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 index 907b3cf..2504721 100644 --- a/pgcommitfest/commitfest/templates/commitfest.html +++ b/pgcommitfest/commitfest/templates/commitfest.html @@ -85,7 +85,7 @@

{{p.is_open|yesno:"Active patches,Closed patches"}}

{%endifchanged%} {%endif%} - {{p.name}} + {{p.name}} {{p.id}} {{p.status|patchstatusstring}} {%if p.targetversion%}{{p.targetversion}}{%endif%} diff --git a/pgcommitfest/commitfest/templates/patch.html b/pgcommitfest/commitfest/templates/patch.html index 82d15b4..db74b81 100644 --- a/pgcommitfest/commitfest/templates/patch.html +++ b/pgcommitfest/commitfest/templates/patch.html @@ -68,7 +68,7 @@ Status {%for c in patch_commitfests %} -
{{c.commitfest}}: {{c.statusstring}}
+
{{c.commitfest}}: {{c.statusstring}}
{%endfor%} diff --git a/pgcommitfest/commitfest/templates/patch_commands.inc b/pgcommitfest/commitfest/templates/patch_commands.inc index 8f18f71..9b78a68 100644 --- a/pgcommitfest/commitfest/templates/patch_commands.inc +++ b/pgcommitfest/commitfest/templates/patch_commands.inc @@ -21,7 +21,7 @@
  • Rejected
  • Withdrawn
  • Returned with feedback
    • -
  • Move to next CF
    • +
  • Move to next CF
  • Committed
    • @@ -37,4 +37,4 @@ {%endif%} - \ No newline at end of file + diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index 50b67f2..ec28d00 100644 --- 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) + + 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): diff --git a/pgcommitfest/urls.py b/pgcommitfest/urls.py index 3230b04..733fcc5 100644 --- 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\d+)/activity(?P\.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), @@ -38,6 +38,13 @@ 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),