diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 00000000..b1eff40a --- /dev/null +++ b/.editorconfig @@ -0,0 +1,17 @@ +# top-most EditorConfig file +root = true + +# basic rules for all files +[*] +end_of_line = lf +insert_final_newline = true +charset = utf-8 +trim_trailing_whitespace = true + +[*.{py,js}] +indent_style = space +indent_size = 4 + +[*.html] +indent_style = space +indent_size = 1 diff --git a/README.md b/README.md index a379f53d..cbe9dd9a 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A commitfest is a collection of patches and reviews for a project and is part of ## The Application -This is a Django 3.2 application backed by PostgreSQL and running on Python 3.x. +This is a Django 4.2 application backed by PostgreSQL and running on Python 3.x. ## Getting Started diff --git a/media/commitfest/css/commitfest.css b/media/commitfest/css/commitfest.css index 9189b9a3..899f23df 100644 --- a/media/commitfest/css/commitfest.css +++ b/media/commitfest/css/commitfest.css @@ -37,6 +37,10 @@ div.form-group div.controls input.threadpick-input { display: inline; } +.dropdown-menu--up { + top: initial; + bottom: 100%; +} /* diff --git a/media/commitfest/js/commitfest.js b/media/commitfest/js/commitfest.js index aa4865c9..57b34cd5 100644 --- a/media/commitfest/js/commitfest.js +++ b/media/commitfest/js/commitfest.js @@ -222,7 +222,11 @@ function flagCommitted(committer) { function sortpatches(sortby) { - $('#id_sortkey').val(sortby); + if ($('#id_sortkey').val() == sortby) { + $('#id_sortkey').val(0); + } else { + $('#id_sortkey').val(sortby); + } $('#filterform').submit(); return false; diff --git a/pgcommitfest/commitfest/models.py b/pgcommitfest/commitfest/models.py index c4c4355d..0a7e9e01 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 1de741a7..787e231d 100644 --- a/pgcommitfest/commitfest/templates/commitfest.html +++ b/pgcommitfest/commitfest/templates/commitfest.html @@ -60,16 +60,17 @@

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

- + + - - - + + + {%if user.is_staff%} {%endif%} @@ -84,19 +85,21 @@

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

{%endifchanged%} {%endif%} - + + @@ -183,7 +183,9 @@

Annotations

{%if p.is_open%}Patch{%if sortkey == 0%}
{%endif%}{%endif%}		Patch{%if sortkey == 5%}
{%endif%}		ID{%if sortkey == 4%}
{%endif%}		 Status Ver CI status Author Reviewers Committer{%if p.is_open%}Num cfs{%if sortkey == 3%}
{%endif%}{%else%}Num cfs{%endif%}		{%if p.is_open%}Latest activity{%if sortkey == 1%}
{%endif%}{%else%}Latest activity{%endif%}		{%if p.is_open%}Latest mail{%if sortkey == 2%}
{%endif%}{%else%}Latest mail{%endif%}		Num cfs{%if sortkey == 3%}
{%endif%}		Latest activity{%if sortkey == 1%}
{%endif%}		Latest mail{%if sortkey == 2%}
{%endif%}		Select
{{p.name}}{{p.name}}{{p.id}} {{p.status|patchstatusstring}} {%if p.targetversion%}{{p.targetversion}}{%endif%} {%if not p.cfbot_results %} Not processed {%elif p.cfbot_results.needs_rebase %} - + Needs rebase! {%else%} - + {%if p.cfbot_results.failed > 0 %} diff --git a/pgcommitfest/commitfest/templates/patch.html b/pgcommitfest/commitfest/templates/patch.html index 82d15b45..400a26d3 100644 --- a/pgcommitfest/commitfest/templates/patch.html +++ b/pgcommitfest/commitfest/templates/patch.html @@ -18,25 +18,25 @@ {%if not cfbot_branch %} Not processed {%elif not cfbot_branch.commit_id %} - + Needs rebase! Additional links previous successfully applied patch (outdated): - - + + Summary {%else%} - - + + Summary {%for c in cfbot_tasks %} {%if c.status == 'COMPLETED'%} - + {%elif c.status == 'CREATED' %} - + {%elif c.status == 'EXECUTING' %} - + {%else %} - + {%endif%} {%endfor%} {%endif%} @@ -68,7 +68,7 @@
Status {%for c in patch_commitfests %} -
{{c.commitfest}}: {{c.statusstring}}
+
{{c.commitfest}}: {{c.statusstring}}
{%endfor%}
+{% with dropdown_mode="dropdown-menu--up" %} {%include "patch_commands.inc"%} +{% endwith %} {%comment%}commit dialog{%endcomment%} diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index 8d32ffd6..ec28d00e 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -187,6 +187,10 @@ def commitfest(request, cfid): orderby_str = 'lastmail, created' elif sortkey == 3: orderby_str = 'num_cfs DESC, modified, created' + elif sortkey == 4: + orderby_str = 'p.id' + elif sortkey == 5: + orderby_str = 'p.name, created' else: orderby_str = 'p.id' sortkey = 0 @@ -316,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) @@ -369,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()) @@ -461,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) @@ -558,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 @@ -588,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() @@ -691,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() @@ -711,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)) @@ -736,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': @@ -750,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 3230b047..733fcc54 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),