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),