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

Commit013eeb1

Browse files
JelteFpolkerty
andcommitted
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 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 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.Co-Authored-By: Jacob Brazeal <jacob.brazeal@scale.com>
1 parentd54c83a commit013eeb1

File tree

6 files changed

+71
-66
lines changed

6 files changed

+71
-66
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/templates/patch_commands.inc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
<lirole="presentation"><ahref="close/reject/"onclick="return verify_reject()">Rejected</a></li>
2222
<lirole="presentation"><ahref="close/withdrawn/"onclick="return verify_withdrawn()">Withdrawn</a></li>
2323
<lirole="presentation"><ahref="close/feedback/"onclick="return verify_returned()">Returned with feedback</a></li>
24-
<lirole="presentation"><ahref="close/next/"onclick="return verify_next()">Move to next CF</a></li>
24+
<lirole="presentation"><ahref="close/next/?cfid={{cf.id}}"onclick="return verify_next()">Move to next CF</a></li>
2525
<lirole="presentation"><ahref="close/committed/"onclick="return flagCommitted({%if patch.committer%}'{{patch.committer}}'{%elif is_committer%}'{{user.username}}'{%else%}null{%endif%})">Committed</a></li>
2626
</ul>
2727
</div>
@@ -37,4 +37,4 @@
3737
</div>
3838
{%endif%}
3939

40-
</div>
40+
</div>

‎pgcommitfest/commitfest/views.py

Lines changed: 47 additions & 52 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+
332+
patch_commitfests=PatchOnCommitFest.objects.select_related('commitfest').filter(patch=patch).order_by('-commitfest__startdate').all()
333+
cf=patch_commitfests[0].commitfest
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())
@@ -431,7 +432,7 @@ def newpatch(request, cfid):
431432
# Now add the thread
432433
try:
433434
doAttachThread(cf,patch,form.cleaned_data['threadmsgid'],request.user)
434-
returnHttpResponseRedirect("/%s/%s/edit/"% (cf.id,patch.id))
435+
returnHttpResponseRedirect("/patch/%s/edit/"% (patch.id,))
435436
exceptHttp404:
436437
# Thread not found!
437438
# This is a horrible breakage of API layers
@@ -465,21 +466,12 @@ 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

474-
ifpoc.is_closed:
475-
# We allow modification of patches in closed CFs *only* if it's the
476-
# last CF that the patch is part of. If it's part of another CF, that
477-
# is later than this one, tell the user to go there instead.
478-
lastcf=PatchOnCommitFest.objects.filter(patch=patch).order_by('-commitfest__startdate')[0]
479-
ifpoc!=lastcf:
480-
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!")
481-
returnHttpResponseRedirect('..')
482-
483475
ifrequest.method=='POST':
484476
try:
485477
form=CommentForm(patch,poc,is_review,data=request.POST)
@@ -562,17 +554,10 @@ def comment(request, cfid, patchid, what):
562554

563555
@login_required
564556
@transaction.atomic
565-
defstatus(request,cfid,patchid,status):
566-
poc=get_object_or_404(PatchOnCommitFest.objects.select_related(),commitfest__id=cfid,patch__id=patchid)
567-
568-
ifpoc.is_closed:
569-
# We allow modification of patches in closed CFs *only* if it's the
570-
# last CF that the patch is part of. If it's part of another CF, that
571-
# is later than this one, tell the user to go there instead.
572-
lastcf=PatchOnCommitFest.objects.filter(patch__id=patchid).order_by('-commitfest__startdate')[0]
573-
ifpoc!=lastcf:
574-
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!")
575-
returnHttpResponseRedirect('/%s/%s/'% (poc.commitfest.id,poc.patch.id))
557+
defstatus(request,patchid,status):
558+
patch=get_object_or_404(Patch.objects.select_related(),pk=patchid)
559+
cf=patch.current_commitfest()
560+
poc=get_object_or_404(PatchOnCommitFest.objects.select_related(),commitfest__id=cf.id,patch__id=patchid)
576561

577562
ifstatus=='review':
578563
newstatus=PatchOnCommitFest.STATUS_REVIEW
@@ -592,22 +577,29 @@ def status(request, cfid, patchid, status):
592577

593578
PatchHistory(patch=poc.patch,by=request.user,what='New status: %s'%poc.statusstring).save_and_notify()
594579

595-
returnHttpResponseRedirect('/%s/%s/'% (poc.commitfest.id,poc.patch.id))
580+
returnHttpResponseRedirect('/patch/%s/'% (poc.patch.id))
596581

597582

598583
@login_required
599584
@transaction.atomic
600-
defclose(request,cfid,patchid,status):
601-
poc=get_object_or_404(PatchOnCommitFest.objects.select_related(),commitfest__id=cfid,patch__id=patchid)
602-
603-
ifpoc.is_closed:
604-
# We allow modification of patches in closed CFs *only* if it's the
605-
# last CF that the patch is part of. If it's part of another CF, that
606-
# is later than this one, tell the user to go there instead.
607-
lastcf=PatchOnCommitFest.objects.filter(patch__id=patchid).order_by('-commitfest__startdate')[0]
608-
ifpoc!=lastcf:
609-
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!")
610-
returnHttpResponseRedirect('/%s/%s/'% (poc.commitfest.id,poc.patch.id))
585+
defclose(request,patchid,status):
586+
patch=get_object_or_404(Patch.objects.select_related(),pk=patchid)
587+
cf=patch.current_commitfest()
588+
589+
try:
590+
request_cfid=int(request.GET.get('cfid',''))
591+
exceptValueError:
592+
# int() failed, ignore
593+
request_cfid=None
594+
595+
ifrequest_cfidisnotNoneandrequest_cfid!=cf.id:
596+
# The cfid parameter is only added to the /next/ link. That's the only
597+
# close operation where two people pressing the button at the same time
598+
# can have unintended effects.
599+
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.")
600+
returnHttpResponseRedirect('/%s/%s/'% (cf.id,patch.id))
601+
602+
poc=get_object_or_404(PatchOnCommitFest.objects.select_related(),commitfest__id=cf.id,patch__id=patchid)
611603

612604
poc.leavedate=datetime.now()
613605

@@ -695,8 +687,7 @@ def close(request, cfid, patchid, status):
695687

696688
@login_required
697689
@transaction.atomic
698-
defreviewer(request,cfid,patchid,status):
699-
get_object_or_404(CommitFest,pk=cfid)
690+
defreviewer(request,patchid,status):
700691
patch=get_object_or_404(Patch,pk=patchid)
701692

702693
is_reviewer=request.userinpatch.reviewers.all()
@@ -715,7 +706,6 @@ def reviewer(request, cfid, patchid, status):
715706
@login_required
716707
@transaction.atomic
717708
defcommitter(request,cfid,patchid,status):
718-
get_object_or_404(CommitFest,pk=cfid)
719709
patch=get_object_or_404(Patch,pk=patchid)
720710

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

741731
@login_required
742732
@transaction.atomic
743-
defsubscribe(request,cfid,patchid,sub):
744-
get_object_or_404(CommitFest,pk=cfid)
733+
defsubscribe(request,patchid,sub):
745734
patch=get_object_or_404(Patch,pk=patchid)
746735

747736
ifsub=='un':
@@ -754,6 +743,12 @@ def subscribe(request, cfid, patchid, sub):
754743
returnHttpResponseRedirect("../")
755744

756745

746+
defsend_patch_email(request,patchid):
747+
patch=get_object_or_404(Patch,pk=patchid)
748+
cf=patch.current_commitfest()
749+
returnsend_email(request,cf.id)
750+
751+
757752
@login_required
758753
@transaction.atomic
759754
defsend_email(request,cfid):

‎pgcommitfest/urls.py

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -19,25 +19,32 @@
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),
3737
re_path(r'^lookups/user/$',lookups.userlookup),
3838
re_path(r'^thread_notify/$',views.thread_notify),
3939
re_path(r'^cfbot_notify/$',views.cfbot_notify),
4040

41+
# Legacy email POST route. This can be safely removed in a few days from
42+
# the first time this is deployed. It's only puprose is not breaking
43+
# submissions from a previous page lood, during the deploy of the new
44+
# /patch/(\d+) routes. It would be a shame if someone lost their well
45+
# written email because of this.
46+
re_path(r'^\d+/(\d+)/send_email/$',views.send_patch_email),
47+
4148
# Auth system integration
4249
re_path(r'^(?:account/)?login/?$',pgcommitfest.auth.login),
4350
re_path(r'^(?:account/)?logout/?$',pgcommitfest.auth.logout),

0 commit comments

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