Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commitbf5f948

Browse files
committed
Make /patch/{id} the URL for a patch
Previously we'd include the ID of the commitfest in the URL of thepatch. In9f12a5e we introduced a stable URL for patches that wouldredirect to the one for the latest commitfest. This starts to use thatURL as the valid only URL for a patch (with the previous URL redirectingto this one).The reasoning behind this is that the old approach resulted in Ndifferent URLs for each patch, which all showed the exact same patchinformation. The only difference between all these URLs would be thebreadcrumb at the top of the page.The only benefit of that approach is that if you're on an oldcommitfest, and click a link there, then the breadcrumb will bring youback to where you came from. Since people rarely have a reason to browseclosed commitfests, the that benefit seems pretty small. Especiallybecause people can just as well press their browser back button, in thatcase.The problems that these N links cause seem much more impactful to mostusers: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 ofthe commitfests that a patch has been part of. Those links were alreadyrather useless, since all they effectively did was change thebreadcrumb. But with this new commit, they wouldn't even do that anymore,and simply redirect to the current page. So now they start pointing tothe commitfest itself, which seems more useful behaviour anyway.
1 parentb4dff24 commitbf5f948

File tree

5 files changed

+46
-35
lines changed

5 files changed

+46
-35
lines changed

‎pgcommitfest/commitfest/models.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,9 @@ class Patch(models.Model, DiffableModel):
122122
'reviewers':'reviewers_string',
123123
}
124124

125+
defcurrent_commitfest(self):
126+
returnself.commitfests.order_by('-startdate').first()
127+
125128
# Some accessors
126129
@property
127130
defauthors_string(self):

‎pgcommitfest/commitfest/templates/commitfest.html

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ <h3>{{p.is_open|yesno:"Active patches,Closed patches"}}</h3>
8585
{%endifchanged%}
8686
{%endif%}
8787
<tr>
88-
<td><ahref="{{p.id}}/">{{p.name}}</a></td>
88+
<td><ahref="/patch/{{p.id}}/">{{p.name}}</a></td>
8989
<td>{{p.id}}</td>
9090
<td><spanclass="label label-{{p.status|patchstatuslabel}}">{{p.status|patchstatusstring}}</span></td>
9191
<td>{%if p.targetversion%}<spanclass="label label-default">{{p.targetversion}}</span>{%endif%}</td>

‎pgcommitfest/commitfest/templates/patch.html

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@
6868
<tr>
6969
<th>Status</th>
7070
<td>{%for c in patch_commitfests %}
71-
<divstyle="margin-bottom: 3px;"><ahref="/{{c.commitfest.id}}/{{patch.id}}/">{{c.commitfest}}</a>:<spanclass="label label-{{c.status|patchstatuslabel}}">{{c.statusstring}}</span></div>
71+
<divstyle="margin-bottom: 3px;"><ahref="/{{c.commitfest.id}}/">{{c.commitfest}}</a>:<spanclass="label label-{{c.status|patchstatuslabel}}">{{c.statusstring}}</span></div>
7272
{%endfor%}
7373
</td>
7474
</tr>

‎pgcommitfest/commitfest/views.py

Lines changed: 31 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -320,17 +320,18 @@ def global_search(request):
320320
})
321321

322322

323-
defpatch_redirect(request,patchid):
324-
last_commitfest=PatchOnCommitFest.objects.select_related('commitfest').filter(patch_id=patchid).order_by('-commitfest__startdate').first()
325-
ifnotlast_commitfest:
326-
raiseHttp404("Patch not found")
327-
returnHttpResponseRedirect(f'/{last_commitfest.commitfest_id}/{patchid}/')
323+
defpatch_legacy_redirect(request,cfid,patchid):
324+
# Previously we would include the commitfest id in the URL. This is no
325+
# longer the case.
326+
returnHttpResponseRedirect(f'/patch/{patchid}/')
328327

329328

330-
defpatch(request,cfid,patchid):
331-
cf=get_object_or_404(CommitFest,pk=cfid)
332-
patch=get_object_or_404(Patch.objects.select_related(),pk=patchid,commitfests=cf)
333-
patch_commitfests=PatchOnCommitFest.objects.select_related('commitfest').filter(patch=patch).order_by('-commitfest__startdate')
329+
defpatch(request,patchid):
330+
patch=get_object_or_404(Patch.objects.select_related(),pk=patchid)
331+
cf=patch.current_commitfest()
332+
333+
patch_commitfests=PatchOnCommitFest.objects.select_related('commitfest').filter(patch=patch).order_by('-commitfest__startdate').all()
334+
334335
committers=Committer.objects.filter(active=True).order_by('user__last_name','user__first_name')
335336

336337
cfbot_branch=getattr(patch,'cfbot_branch',None)
@@ -373,9 +374,9 @@ def patch(request, cfid, patchid):
373374

374375
@login_required
375376
@transaction.atomic
376-
defpatchform(request,cfid,patchid):
377-
cf=get_object_or_404(CommitFest,pk=cfid)
378-
patch=get_object_or_404(Patch,pk=patchid,commitfests=cf)
377+
defpatchform(request,patchid):
378+
patch=get_object_or_404(Patch,pk=patchid)
379+
cf=patch.current_commitfest()
379380

380381
prevreviewers=list(patch.reviewers.all())
381382
prevauthors=list(patch.authors.all())
@@ -465,9 +466,9 @@ def _review_status_string(reviewstatus):
465466

466467
@login_required
467468
@transaction.atomic
468-
defcomment(request,cfid,patchid,what):
469-
cf=get_object_or_404(CommitFest,pk=cfid)
469+
defcomment(request,patchid,what):
470470
patch=get_object_or_404(Patch,pk=patchid)
471+
cf=patch.current_commitfest()
471472
poc=get_object_or_404(PatchOnCommitFest,patch=patch,commitfest=cf)
472473
is_review= (what=='review')
473474

@@ -562,8 +563,10 @@ def comment(request, cfid, patchid, what):
562563

563564
@login_required
564565
@transaction.atomic
565-
defstatus(request,cfid,patchid,status):
566-
poc=get_object_or_404(PatchOnCommitFest.objects.select_related(),commitfest__id=cfid,patch__id=patchid)
566+
defstatus(request,patchid,status):
567+
patch=get_object_or_404(Patch.objects.select_related(),pk=patchid)
568+
cf=patch.current_commitfest()
569+
poc=get_object_or_404(PatchOnCommitFest.objects.select_related(),commitfest__id=cf.id,patch__id=patchid)
567570

568571
ifpoc.is_closed:
569572
# We allow modification of patches in closed CFs *only* if it's the
@@ -597,8 +600,10 @@ def status(request, cfid, patchid, status):
597600

598601
@login_required
599602
@transaction.atomic
600-
defclose(request,cfid,patchid,status):
601-
poc=get_object_or_404(PatchOnCommitFest.objects.select_related(),commitfest__id=cfid,patch__id=patchid)
603+
defclose(request,patchid,status):
604+
patch=get_object_or_404(Patch.objects.select_related(),pk=patchid)
605+
cf=patch.current_commitfest()
606+
poc=get_object_or_404(PatchOnCommitFest.objects.select_related(),commitfest__id=cf.id,patch__id=patchid)
602607

603608
ifpoc.is_closed:
604609
# We allow modification of patches in closed CFs *only* if it's the
@@ -695,8 +700,7 @@ def close(request, cfid, patchid, status):
695700

696701
@login_required
697702
@transaction.atomic
698-
defreviewer(request,cfid,patchid,status):
699-
get_object_or_404(CommitFest,pk=cfid)
703+
defreviewer(request,patchid,status):
700704
patch=get_object_or_404(Patch,pk=patchid)
701705

702706
is_reviewer=request.userinpatch.reviewers.all()
@@ -715,7 +719,6 @@ def reviewer(request, cfid, patchid, status):
715719
@login_required
716720
@transaction.atomic
717721
defcommitter(request,cfid,patchid,status):
718-
get_object_or_404(CommitFest,pk=cfid)
719722
patch=get_object_or_404(Patch,pk=patchid)
720723

721724
committer=list(Committer.objects.filter(user=request.user,active=True))
@@ -740,8 +743,7 @@ def committer(request, cfid, patchid, status):
740743

741744
@login_required
742745
@transaction.atomic
743-
defsubscribe(request,cfid,patchid,sub):
744-
get_object_or_404(CommitFest,pk=cfid)
746+
defsubscribe(request,patchid,sub):
745747
patch=get_object_or_404(Patch,pk=patchid)
746748

747749
ifsub=='un':
@@ -754,6 +756,12 @@ def subscribe(request, cfid, patchid, sub):
754756
returnHttpResponseRedirect("../")
755757

756758

759+
defsend_patch_email(request,patchid):
760+
patch=get_object_or_404(Patch,pk=patchid)
761+
cf=patch.current_commitfest()
762+
returnsend_email(request,cf.id)
763+
764+
757765
@login_required
758766
@transaction.atomic
759767
defsend_email(request,cfid):

‎pgcommitfest/urls.py

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -19,18 +19,18 @@
1919
re_path(r'^(\d+)/$',views.commitfest),
2020
re_path(r'^(open|inprogress|current)/(.*)$',views.redir),
2121
re_path(r'^(?P<cfid>\d+)/activity(?P<rss>\.rss)?/$',views.activity),
22-
re_path(r'^patch/(\d+)/$',views.patch_redirect),
23-
re_path(r'^(\d+)/(\d+)/$',views.patch),
24-
re_path(r'^(\d+)/(\d+)/edit/$',views.patchform),
22+
re_path(r'^(\d+)/(\d+)/$',views.patch_legacy_redirect),
23+
re_path(r'^patch/(\d+)/$',views.patch),
24+
re_path(r'^patch/(\d+)/edit/$',views.patchform),
2525
re_path(r'^(\d+)/new/$',views.newpatch),
26-
re_path(r'^(\d+)/(\d+)/status/(review|author|committer)/$',views.status),
27-
re_path(r'^(\d+)/(\d+)/close/(reject|withdrawn|feedback|committed|next)/$',views.close),
28-
re_path(r'^(\d+)/(\d+)/reviewer/(become|remove)/$',views.reviewer),
29-
re_path(r'^(\d+)/(\d+)/committer/(become|remove)/$',views.committer),
30-
re_path(r'^(\d+)/(\d+)/(un)?subscribe/$',views.subscribe),
31-
re_path(r'^(\d+)/(\d+)/(comment|review)/',views.comment),
26+
re_path(r'^patch/(\d+)/status/(review|author|committer)/$',views.status),
27+
re_path(r'^patch/(\d+)/close/(reject|withdrawn|feedback|committed|next)/$',views.close),
28+
re_path(r'^patch/(\d+)/reviewer/(become|remove)/$',views.reviewer),
29+
re_path(r'^patch/(\d+)/committer/(become|remove)/$',views.committer),
30+
re_path(r'^patch/(\d+)/(un)?subscribe/$',views.subscribe),
31+
re_path(r'^patch/(\d+)/(comment|review)/',views.comment),
3232
re_path(r'^(\d+)/send_email/$',views.send_email),
33-
re_path(r'^(\d+)/\d+/send_email/$',views.send_email),
33+
re_path(r'^patch/(\d+)/send_email/$',views.send_patch_email),
3434
re_path(r'^(\d+)/reports/authorstats/$',reports.authorstats),
3535
re_path(r'^search/$',views.global_search),
3636
re_path(r'^ajax/(\w+)/$',ajax.main),

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp