From a0fa12ef563c6685edbf055b1c8349c5f343e939 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <postgres@jeltef.nl>
Date: Sun, 12 Jan 2025 21:43:02 +0100
Subject: [PATCH 1/8] Use target=_blank for all cfbot links
---
.../commitfest/templates/commitfest.html | 5 +++--
pgcommitfest/commitfest/templates/patch.html | 18 +++++++++---------
2 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/pgcommitfest/commitfest/templates/commitfest.html b/pgcommitfest/commitfest/templates/commitfest.html
index 1de741a7..28392a6b 100644
--- a/pgcommitfest/commitfest/templates/commitfest.html
+++ b/pgcommitfest/commitfest/templates/commitfest.html
@@ -91,12 +91,13 @@ <h3>{{p.is_open|yesno:"Active patches,Closed patches"}}</h3>
{%if not p.cfbot_results %}
<span class="label label-default">Not processed</span>
{%elif p.cfbot_results.needs_rebase %}
- <a href="{{p.cfbot_results.apply_url}}" title="View git apply logs">
+ <a href="{{p.cfbot_results.apply_url}}" target="_blank" title="View git apply logs">
<span class="label label-warning">Needs rebase!</span>
</a>
{%else%}
- <a href="https://github.com/postgresql-cfbot/postgresql/compare/cf/{{p.id}}~1...cf/{{p.id}}" title="View last patch set on GitHub"><img class="github-logo" src="/media/commitfest/github-mark.svg"/></a>
+ <a href="https://github.com/postgresql-cfbot/postgresql/compare/cf/{{p.id}}~1...cf/{{p.id}}" target="_blank" title="View last patch set on GitHub"><img class="github-logo" src="/media/commitfest/github-mark.svg"/></a>
<a href="https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F{{p.id}}"
+ target="_blank"
title="View CI history{%if p.cfbot_results.failed_task_names %}. Failed jobs: {{p.cfbot_results.failed_task_names}}{%endif%}">
{%if p.cfbot_results.failed > 0 %}
<img src="/media/commitfest/new_failure.svg"/>
diff --git a/pgcommitfest/commitfest/templates/patch.html b/pgcommitfest/commitfest/templates/patch.html
index 82d15b45..09103274 100644
--- a/pgcommitfest/commitfest/templates/patch.html
+++ b/pgcommitfest/commitfest/templates/patch.html
@@ -18,25 +18,25 @@
{%if not cfbot_branch %}
<span class="label label-default">Not processed</span></a>
{%elif not cfbot_branch.commit_id %}
- <a href="{{cfbot_branch.apply_url}}">
+ <a href="{{cfbot_branch.apply_url}}" target="_blank" title="View git apply logs">
<span class="label label-warning" title="View git apply logs">Needs rebase!</span></a>
Additional links previous successfully applied patch (outdated):
- <a href="https://github.com/postgresql-cfbot/postgresql/compare/cf/{{patch.id}}~1...cf/{{patch.id}}" title="View previous successfully applied patch set on GitHub"><img class="github-logo" src="/media/commitfest/github-mark.svg"/></a>
- <a href="https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F{{patch.id}}">
+ <a href="https://github.com/postgresql-cfbot/postgresql/compare/cf/{{patch.id}}~1...cf/{{patch.id}}" target="_blank" title="View previous successfully applied patch set on GitHub"><img class="github-logo" src="/media/commitfest/github-mark.svg"/></a>
+ <a href="https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F{{patch.id}}" target="_blank">
<span class="label label-default">Summary</span></a>
{%else%}
- <a href="https://github.com/postgresql-cfbot/postgresql/compare/cf/{{patch.id}}~1...cf/{{patch.id}}" title="View last patch set on GitHub"><img class="github-logo" src="/media/commitfest/github-mark.svg"/></a>
- <a href="https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F{{patch.id}}">
+ <a href="https://github.com/postgresql-cfbot/postgresql/compare/cf/{{patch.id}}~1...cf/{{patch.id}}" target="_blank" title="View last patch set on GitHub"><img class="github-logo" src="/media/commitfest/github-mark.svg"/></a>
+ <a href="https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F{{patch.id}}" target="_blank">
<span class="label label-default">Summary</span></a>
{%for c in cfbot_tasks %}
{%if c.status == 'COMPLETED'%}
- <a href="https://cirrusci.com/task/{{c.id}}" title="{{c.task_name}}: {{c.status}}"><img src="/media/commitfest/new_success.svg"/></a>
+ <a href="https://cirrusci.com/task/{{c.id}}" target="_blank" title="{{c.task_name}}: {{c.status}}"><img src="/media/commitfest/new_success.svg"/></a>
{%elif c.status == 'CREATED' %}
- <a href="https://cirrusci.com/task/{{c.id}}" title="{{c.task_name}}: {{c.status}}"><img src="/media/commitfest/waiting_to_start.svg"/></a>
+ <a href="https://cirrusci.com/task/{{c.id}}" target="_blank" title="{{c.task_name}}: {{c.status}}"><img src="/media/commitfest/waiting_to_start.svg"/></a>
{%elif c.status == 'EXECUTING' %}
- <a href="https://cirrusci.com/task/{{c.id}}" title="{{c.task_name}}: {{c.status}}"><img src="/media/commitfest/running.svg"/></a>
+ <a href="https://cirrusci.com/task/{{c.id}}" target="_blank" title="{{c.task_name}}: {{c.status}}"><img src="/media/commitfest/running.svg"/></a>
{%else %}
- <a href="https://cirrusci.com/task/{{c.id}}" title="{{c.task_name}}: {{c.status}}"><img src="/media/commitfest/new_failure.svg"/></a>
+ <a href="https://cirrusci.com/task/{{c.id}}" target="_blank" title="{{c.task_name}}: {{c.status}}"><img src="/media/commitfest/new_failure.svg"/></a>
{%endif%}
{%endfor%}
{%endif%}
From b697c34b6ee4b197f56ffa4a5d0592b09f5a22a5 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <postgres@jeltef.nl>
Date: Sun, 12 Jan 2025 21:43:05 +0100
Subject: [PATCH 2/8] Add a basic editorconfig file
This is mostly useful to avoid people from adding trailing whitespace to
files.
---
.editorconfig | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
create mode 100644 .editorconfig
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
From c5c10615b10d164763361c7557321ffa61cdeb74 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <postgres@jeltef.nl>
Date: Sun, 12 Jan 2025 23:25:23 +0100
Subject: [PATCH 3/8] 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 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 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.
---
.../commitfest/templates/commitfest.html | 2 +-
pgcommitfest/commitfest/templates/patch.html | 2 +-
pgcommitfest/commitfest/views.py | 19 ++++++++++---------
pgcommitfest/urls.py | 4 ++--
4 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/pgcommitfest/commitfest/templates/commitfest.html b/pgcommitfest/commitfest/templates/commitfest.html
index 28392a6b..b09cd893 100644
--- a/pgcommitfest/commitfest/templates/commitfest.html
+++ b/pgcommitfest/commitfest/templates/commitfest.html
@@ -84,7 +84,7 @@ <h3>{{p.is_open|yesno:"Active patches,Closed patches"}}</h3>
{%endifchanged%}
{%endif%}
<tr>
- <td><a href="{{p.id}}/">{{p.name}}</a></td>
+ <td><a href="/patch/{{p.id}}/">{{p.name}}</a></td>
<td><span class="label label-{{p.status|patchstatuslabel}}">{{p.status|patchstatusstring}}</span></td>
<td>{%if p.targetversion%}<span class="label label-default">{{p.targetversion}}</span>{%endif%}</td>
<td class="cfbot-summary">
diff --git a/pgcommitfest/commitfest/templates/patch.html b/pgcommitfest/commitfest/templates/patch.html
index 09103274..b028c6b4 100644
--- a/pgcommitfest/commitfest/templates/patch.html
+++ b/pgcommitfest/commitfest/templates/patch.html
@@ -68,7 +68,7 @@
<tr>
<th>Status</th>
<td>{%for c in patch_commitfests %}
- <div style="margin-bottom: 3px;"><a href="/{{c.commitfest.id}}/{{patch.id}}/">{{c.commitfest}}</a>: <span class="label label-{{c.status|patchstatuslabel}}">{{c.statusstring}}</span></div>
+ <div style="margin-bottom: 3px;"><a href="/{{c.commitfest.id}}/">{{c.commitfest}}</a>: <span class="label label-{{c.status|patchstatuslabel}}">{{c.statusstring}}</span></div>
{%endfor%}
</td>
</tr>
diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py
index 8d32ffd6..4f69ab26 100644
--- a/pgcommitfest/commitfest/views.py
+++ b/pgcommitfest/commitfest/views.py
@@ -316,17 +316,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)
diff --git a/pgcommitfest/urls.py b/pgcommitfest/urls.py
index 3230b047..e45bff53 100644
--- a/pgcommitfest/urls.py
+++ b/pgcommitfest/urls.py
@@ -19,8 +19,8 @@
re_path(r'^(\d+)/$', views.commitfest),
re_path(r'^(open|inprogress|current)/(.*)$', views.redir),
re_path(r'^(?P<cfid>\d+)/activity(?P<rss>\.rss)?/$', views.activity),
- re_path(r'^patch/(\d+)/$', views.patch_redirect),
- re_path(r'^(\d+)/(\d+)/$', views.patch),
+ re_path(r'^patch/(\d+)/$', views.patch),
+ re_path(r'^(\d+)/(\d+)/$', views.patch_legacy_redirect),
re_path(r'^(\d+)/(\d+)/edit/$', views.patchform),
re_path(r'^(\d+)/new/$', views.newpatch),
re_path(r'^(\d+)/(\d+)/status/(review|author|committer)/$', views.status),
From 226959af801d91a4a4ba301969a362bdba7184fa Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <postgres@jeltef.nl>
Date: Mon, 13 Jan 2025 00:18:26 +0100
Subject: [PATCH 4/8] Add ID column to commitfest page
The ID of a CF entry is the only stable identifier (people can change
the name). That's why tooling such as CFbot uses the ID of the patch for
a lot of things (including showing it on the cfbot overview page).
Having it visible on the page is quite useful for debugging purposes
In eee60a53 the ID was added to the title of the entry, but that made the
the title of the page itself less prominent. So this is an attempt to
have the information available, but not too prominently visible.
---
pgcommitfest/commitfest/templates/commitfest.html | 2 ++
pgcommitfest/commitfest/views.py | 2 ++
2 files changed, 4 insertions(+)
diff --git a/pgcommitfest/commitfest/templates/commitfest.html b/pgcommitfest/commitfest/templates/commitfest.html
index b09cd893..97ee5edb 100644
--- a/pgcommitfest/commitfest/templates/commitfest.html
+++ b/pgcommitfest/commitfest/templates/commitfest.html
@@ -61,6 +61,7 @@ <h3>{{p.is_open|yesno:"Active patches,Closed patches"}}</h3>
<thead>
<tr>
<th>{%if p.is_open%}<a href="#" style="color:#333333;" onclick="return sortpatches(0);">Patch</a>{%if sortkey == 0%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}{%endif%}</th>
+ <th>{%if p.is_open%}<a href="#" style="color:#333333;" onclick="return sortpatches(4);">ID</a>{%if sortkey == 4%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}{%else%}ID{%endif%}</th>
<th>Status</th>
<th>Ver</th>
<th>CI status</th>
@@ -85,6 +86,7 @@ <h3>{{p.is_open|yesno:"Active patches,Closed patches"}}</h3>
{%endif%}
<tr>
<td><a href="/patch/{{p.id}}/">{{p.name}}</a></td>
+ <td>{{p.id}}</td>
<td><span class="label label-{{p.status|patchstatuslabel}}">{{p.status|patchstatusstring}}</span></td>
<td>{%if p.targetversion%}<span class="label label-default">{{p.targetversion}}</span>{%endif%}</td>
<td class="cfbot-summary">
diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py
index 4f69ab26..5c67cb07 100644
--- a/pgcommitfest/commitfest/views.py
+++ b/pgcommitfest/commitfest/views.py
@@ -187,6 +187,8 @@ 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'
else:
orderby_str = 'p.id'
sortkey = 0
From eb200f81f94e2d2a531105b6c156716fc7a8f1fe Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <postgres@jeltef.nl>
Date: Mon, 13 Jan 2025 01:20:56 +0100
Subject: [PATCH 5/8] Make sorting a toggle
If the commitfest entries are sorted by a column clicking the header
again will now remove the sort. In a future commit, it would be nice to
also support for reverse sorting.
---
media/commitfest/js/commitfest.js | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
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;
From 8b22e3ee119074af90419f580089557e2183ecb3 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <postgres@jeltef.nl>
Date: Mon, 13 Jan 2025 01:21:27 +0100
Subject: [PATCH 6/8] Allow sorting by name
It was pretty confusing that clicking the patch name header would sort
by created time, grouped by topic. This makes that sort by name.
---
pgcommitfest/commitfest/templates/commitfest.html | 2 +-
pgcommitfest/commitfest/views.py | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/pgcommitfest/commitfest/templates/commitfest.html b/pgcommitfest/commitfest/templates/commitfest.html
index 97ee5edb..1d17f2c7 100644
--- a/pgcommitfest/commitfest/templates/commitfest.html
+++ b/pgcommitfest/commitfest/templates/commitfest.html
@@ -60,7 +60,7 @@ <h3>{{p.is_open|yesno:"Active patches,Closed patches"}}</h3>
<table class="table table-striped table-bordered table-hover table-condensed">
<thead>
<tr>
- <th>{%if p.is_open%}<a href="#" style="color:#333333;" onclick="return sortpatches(0);">Patch</a>{%if sortkey == 0%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}{%endif%}</th>
+ <th>{%if p.is_open%}<a href="#" style="color:#333333;" onclick="return sortpatches(5);">Patch</a>{%if sortkey == 5%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}{%endif%}</th>
<th>{%if p.is_open%}<a href="#" style="color:#333333;" onclick="return sortpatches(4);">ID</a>{%if sortkey == 4%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}{%else%}ID{%endif%}</th>
<th>Status</th>
<th>Ver</th>
diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py
index 5c67cb07..4ae8738d 100644
--- a/pgcommitfest/commitfest/views.py
+++ b/pgcommitfest/commitfest/views.py
@@ -189,6 +189,8 @@ def commitfest(request, cfid):
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
From 5a2ff4479d6aced96601abe45f1749ad930af39a Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <postgres@jeltef.nl>
Date: Mon, 13 Jan 2025 01:27:10 +0100
Subject: [PATCH 7/8] Allow sorting using the header of the closed patches too
Sorting order impacts closed patches too. So not showing the arrow that
indicates sort direction on that header is confusing. While we're at it,
we might as well allow people to toggle sorting direction using that
header too.
This also automatically fixes the problem that the cell in the closed
patches header did not contain any title at all.
---
pgcommitfest/commitfest/templates/commitfest.html | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/pgcommitfest/commitfest/templates/commitfest.html b/pgcommitfest/commitfest/templates/commitfest.html
index 1d17f2c7..787e231d 100644
--- a/pgcommitfest/commitfest/templates/commitfest.html
+++ b/pgcommitfest/commitfest/templates/commitfest.html
@@ -60,17 +60,17 @@ <h3>{{p.is_open|yesno:"Active patches,Closed patches"}}</h3>
<table class="table table-striped table-bordered table-hover table-condensed">
<thead>
<tr>
- <th>{%if p.is_open%}<a href="#" style="color:#333333;" onclick="return sortpatches(5);">Patch</a>{%if sortkey == 5%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}{%endif%}</th>
- <th>{%if p.is_open%}<a href="#" style="color:#333333;" onclick="return sortpatches(4);">ID</a>{%if sortkey == 4%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}{%else%}ID{%endif%}</th>
+ <th><a href="#" style="color:#333333;" onclick="return sortpatches(5);">Patch</a>{%if sortkey == 5%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}</th>
+ <th><a href="#" style="color:#333333;" onclick="return sortpatches(4);">ID</a>{%if sortkey == 4%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}</th>
<th>Status</th>
<th>Ver</th>
<th>CI status</th>
<th>Author</th>
<th>Reviewers</th>
<th>Committer</th>
- <th>{%if p.is_open%}<a href="#" style="color:#333333;" onclick="return sortpatches(3);">Num cfs</a>{%if sortkey == 3%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}{%else%}Num cfs{%endif%}</th>
- <th>{%if p.is_open%}<a href="#" style="color:#333333;" onclick="return sortpatches(1);">Latest activity</a>{%if sortkey == 1%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}{%else%}Latest activity{%endif%}</th>
- <th>{%if p.is_open%}<a href="#" style="color:#333333;" onclick="return sortpatches(2);">Latest mail</a>{%if sortkey == 2%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}{%else%}Latest mail{%endif%}</th>
+ <th><a href="#" style="color:#333333;" onclick="return sortpatches(3);">Num cfs</a>{%if sortkey == 3%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}</th>
+ <th><a href="#" style="color:#333333;" onclick="return sortpatches(1);">Latest activity</a>{%if sortkey == 1%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}</th>
+ <th><a href="#" style="color:#333333;" onclick="return sortpatches(2);">Latest mail</a>{%if sortkey == 2%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%endif%}</th>
{%if user.is_staff%}
<th>Select</th>
{%endif%}
From 0957e96ec151daa336589e7dc1fc685dd1a98277 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <postgres@jeltef.nl>
Date: Mon, 13 Jan 2025 09:42:43 +0100
Subject: [PATCH 8/8] Change the direction of the dropdowns at the bottom of
the patch page.
---
README.md | 2 +-
media/commitfest/css/commitfest.css | 4 +
pgcommitfest/commitfest/models.py | 3 +
pgcommitfest/commitfest/templates/patch.html | 2 +
.../commitfest/templates/patch_commands.inc | 10 +--
pgcommitfest/commitfest/views.py | 78 +++++++++----------
pgcommitfest/urls.py | 25 +++---
7 files changed, 67 insertions(+), 57 deletions(-)
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/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/patch.html b/pgcommitfest/commitfest/templates/patch.html
index b028c6b4..400a26d3 100644
--- a/pgcommitfest/commitfest/templates/patch.html
+++ b/pgcommitfest/commitfest/templates/patch.html
@@ -183,7 +183,9 @@ <h4>Annotations</h4>
</tbody>
</table>
+{% with dropdown_mode="dropdown-menu--up" %}
{%include "patch_commands.inc"%}
+{% endwith %}
{%comment%}commit dialog{%endcomment%}
<div class="modal fade" id="commitModal" role="dialog">
diff --git a/pgcommitfest/commitfest/templates/patch_commands.inc b/pgcommitfest/commitfest/templates/patch_commands.inc
index 8f18f712..b03e291f 100644
--- a/pgcommitfest/commitfest/templates/patch_commands.inc
+++ b/pgcommitfest/commitfest/templates/patch_commands.inc
@@ -3,7 +3,7 @@
<div class="btn-group">
<a class="btn btn-default dropdown-toggle" data-toggle="dropdown" href="#">Comment/Review <span class="caret"></span></a>
- <ul class="dropdown-menu">
+ <ul class="dropdown-menu {{ dropdown_mode }}">
<li><a href="comment/">Comment</a>
<li><a href="review/">Review</a>
</ul>
@@ -11,7 +11,7 @@
<div class="btn-group">
<a class="btn btn-default dropdown-toggle" data-toggle="dropdown" href="#">Change Status <span class="caret"></span></a>
- <ul class="dropdown-menu" role="menu">
+ <ul class="dropdown-menu {{ dropdown_mode }}" role="menu">
<li role="presentation" class="dropdown-header">Open statuses</li>
<li role="presentation"><a href="status/review/">Needs review</a></li>
<li role="presentation"><a href="status/author/">Waiting on Author</a></li>
@@ -21,7 +21,7 @@
<li role="presentation"><a href="close/reject/" onclick="return verify_reject()">Rejected</a></li>
<li role="presentation"><a href="close/withdrawn/" onclick="return verify_withdrawn()">Withdrawn</a></li>
<li role="presentation"><a href="close/feedback/" onclick="return verify_returned()">Returned with feedback</a></li>
- <li role="presentation"><a href="close/next/" onclick="return verify_next()">Move to next CF</a></li>
+ <li role="presentation"><a href="close/next/?cfid={{cf.id}}" onclick="return verify_next()">Move to next CF</a></li>
<li role="presentation"><a href="close/committed/" onclick="return flagCommitted({%if patch.committer%}'{{patch.committer}}'{%elif is_committer%}'{{user.username}}'{%else%}null{%endif%})">Committed</a></li>
</ul>
</div>
@@ -29,7 +29,7 @@
{%if request.user.is_staff%}
<div class="btn-group">
<a class="btn btn-default dropdown-toggle" data-toggle="dropdown" href="#">Send private mail <span class="caret"></span></a>
- <ul class="dropdown-menu">
+ <ul class="dropdown-menu {{ dropdown_mode }}">
<li><a href="send_email/?authors={{patch.id}}">Send mail to authors</a></li>
<li><a href="send_email/?reviewers={{patch.id}}">Send mail to reviewers</a></li>
<li><a href="send_email/?authors={{patch.id}}&reviewers={{patch.id}}">Send mail to authors and reviewers</a></li>
@@ -37,4 +37,4 @@
</div>
{%endif%}
-</div>
\ No newline at end of file
+</div>
diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py
index 4ae8738d..ec28d00e 100644
--- a/pgcommitfest/commitfest/views.py
+++ b/pgcommitfest/commitfest/views.py
@@ -374,9 +374,9 @@ def patch(request, 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())
@@ -466,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)
@@ -563,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
@@ -593,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()
@@ -696,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()
@@ -716,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))
@@ -741,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':
@@ -755,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 e45bff53..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<cfid>\d+)/activity(?P<rss>\.rss)?/$', views.activity),
- re_path(r'^patch/(\d+)/$', views.patch),
re_path(r'^(\d+)/(\d+)/$', views.patch_legacy_redirect),
- re_path(r'^(\d+)/(\d+)/edit/$', views.patchform),
+ 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),