From 2796367aa8ae42cea8c9ea8db5267030846a6f71 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio Date: Sun, 12 Jan 2025 21:42:49 +0100 Subject: [PATCH 01/12] CFbot integration This adds our own custom replication between the CFbot and the commitfest app. We currently keep the last CFbot runs in our database, in an attempt to keep the queries easy and efficient. The CFbot results are now shown on the overview page of each commitfest, as well as on the page for each specific patch. --- media/commitfest/css/commitfest.css | 4 + media/commitfest/needs_rebase_success.svg | 4 + media/commitfest/new_failure.svg | 5 + media/commitfest/new_success.svg | 4 + media/commitfest/old_failure.svg | 5 + media/commitfest/old_success.svg | 4 + media/commitfest/running.svg | 4 + media/commitfest/waiting_to_start.svg | 3 + .../migrations/0006_cfbot_integration.py | 80 ++++++++++ pgcommitfest/commitfest/models.py | 51 +++++++ .../commitfest/templates/commitfest.html | 30 +++- pgcommitfest/commitfest/templates/patch.html | 28 +++- pgcommitfest/commitfest/views.py | 139 +++++++++++++++++- pgcommitfest/urls.py | 1 + 14 files changed, 357 insertions(+), 5 deletions(-) create mode 100644 media/commitfest/needs_rebase_success.svg create mode 100644 media/commitfest/new_failure.svg create mode 100644 media/commitfest/new_success.svg create mode 100644 media/commitfest/old_failure.svg create mode 100644 media/commitfest/old_success.svg create mode 100644 media/commitfest/running.svg create mode 100644 media/commitfest/waiting_to_start.svg create mode 100644 pgcommitfest/commitfest/migrations/0006_cfbot_integration.py diff --git a/media/commitfest/css/commitfest.css b/media/commitfest/css/commitfest.css index 07bfa102..e1a10f01 100644 --- a/media/commitfest/css/commitfest.css +++ b/media/commitfest/css/commitfest.css @@ -65,3 +65,7 @@ div.form-group div.controls input.threadpick-input { #annotateMessageBody.loading * { display: none; } + +.cfbot-summary img { + margin-top: -3px; +} diff --git a/media/commitfest/needs_rebase_success.svg b/media/commitfest/needs_rebase_success.svg new file mode 100644 index 00000000..7f4113ff --- /dev/null +++ b/media/commitfest/needs_rebase_success.svg @@ -0,0 +1,4 @@ + + + + \ No newline at end of file diff --git a/media/commitfest/new_failure.svg b/media/commitfest/new_failure.svg new file mode 100644 index 00000000..ff3012d0 --- /dev/null +++ b/media/commitfest/new_failure.svg @@ -0,0 +1,5 @@ + + + + + \ No newline at end of file diff --git a/media/commitfest/new_success.svg b/media/commitfest/new_success.svg new file mode 100644 index 00000000..a0d9b7c4 --- /dev/null +++ b/media/commitfest/new_success.svg @@ -0,0 +1,4 @@ + + + + \ No newline at end of file diff --git a/media/commitfest/old_failure.svg b/media/commitfest/old_failure.svg new file mode 100644 index 00000000..9d91d6c0 --- /dev/null +++ b/media/commitfest/old_failure.svg @@ -0,0 +1,5 @@ + + + + + \ No newline at end of file diff --git a/media/commitfest/old_success.svg b/media/commitfest/old_success.svg new file mode 100644 index 00000000..2de4117e --- /dev/null +++ b/media/commitfest/old_success.svg @@ -0,0 +1,4 @@ + + + + \ No newline at end of file diff --git a/media/commitfest/running.svg b/media/commitfest/running.svg new file mode 100644 index 00000000..a137d410 --- /dev/null +++ b/media/commitfest/running.svg @@ -0,0 +1,4 @@ + + + + diff --git a/media/commitfest/waiting_to_start.svg b/media/commitfest/waiting_to_start.svg new file mode 100644 index 00000000..efd371d4 --- /dev/null +++ b/media/commitfest/waiting_to_start.svg @@ -0,0 +1,3 @@ + + + \ No newline at end of file diff --git a/pgcommitfest/commitfest/migrations/0006_cfbot_integration.py b/pgcommitfest/commitfest/migrations/0006_cfbot_integration.py new file mode 100644 index 00000000..ac84969a --- /dev/null +++ b/pgcommitfest/commitfest/migrations/0006_cfbot_integration.py @@ -0,0 +1,80 @@ +# Generated by Django 4.2.17 on 2024-12-21 14:15 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('commitfest', '0005_history_dateindex'), + ] + + operations = [ + migrations.CreateModel( + name='CfbotBranch', + fields=[ + ('patch', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, related_name='cfbot_branch', serialize=False, to='commitfest.patch')), + ('branch_id', models.IntegerField()), + ('branch_name', models.TextField()), + ('commit_id', models.TextField(blank=True, null=True)), + ('apply_url', models.TextField()), + ('status', models.TextField(choices=[('testing', 'Testing'), ('finished', 'Finished'), ('failed', 'Failed'), ('timeout', 'Timeout')])), + ('created', models.DateTimeField(auto_now_add=True)), + ('modified', models.DateTimeField(auto_now=True)), + ], + ), + migrations.CreateModel( + name='CfbotTask', + fields=[ + ('id', models.BigAutoField(primary_key=True, serialize=False)), + ('task_id', models.TextField(unique=True)), + ('task_name', models.TextField()), + ('branch_id', models.IntegerField()), + ('position', models.IntegerField()), + ('status', models.TextField(choices=[('CREATED', 'Created'), ('NEEDS_APPROVAL', 'Needs Approval'), ('TRIGGERED', 'Triggered'), ('EXECUTING', 'Executing'), ('FAILED', 'Failed'), ('COMPLETED', 'Completed'), ('SCHEDULED', 'Scheduled'), ('ABORTED', 'Aborted'), ('ERRORED', 'Errored')])), + ('created', models.DateTimeField(auto_now_add=True)), + ('modified', models.DateTimeField(auto_now=True)), + ('patch', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='cfbot_tasks', to='commitfest.patch')), + ], + ), + migrations.RunSQL( + """ + CREATE TYPE cfbotbranch_status AS ENUM ( + 'testing', + 'finished', + 'failed', + 'timeout' + ); + """ + ), + migrations.RunSQL( + """ + CREATE TYPE cfbottask_status AS ENUM ( + 'CREATED', + 'NEEDS_APPROVAL', + 'TRIGGERED', + 'EXECUTING', + 'FAILED', + 'COMPLETED', + 'SCHEDULED', + 'ABORTED', + 'ERRORED' + ); + """ + ), + migrations.RunSQL( + """ + ALTER TABLE commitfest_cfbotbranch + ALTER COLUMN status TYPE cfbotbranch_status + USING status::cfbotbranch_status; + """ + ), + migrations.RunSQL( + """ + ALTER TABLE commitfest_cfbottask + ALTER COLUMN status TYPE cfbottask_status + USING status::cfbottask_status; + """ + ), + ] diff --git a/pgcommitfest/commitfest/models.py b/pgcommitfest/commitfest/models.py index 28722f06..32602424 100644 --- a/pgcommitfest/commitfest/models.py +++ b/pgcommitfest/commitfest/models.py @@ -341,3 +341,54 @@ class PatchStatus(models.Model): class PendingNotification(models.Model): history = models.ForeignKey(PatchHistory, blank=False, null=False, on_delete=models.CASCADE) user = models.ForeignKey(User, blank=False, null=False, on_delete=models.CASCADE) + + +class CfbotBranch(models.Model): + STATUS_CHOICES = [ + ('testing', 'Testing'), + ('finished', 'Finished'), + ('failed', 'Failed'), + ('timeout', 'Timeout'), + ] + + patch = models.OneToOneField(Patch, on_delete=models.CASCADE, related_name="cfbot_branch", primary_key=True) + branch_id = models.IntegerField(null=False) + branch_name = models.TextField(null=False) + commit_id = models.TextField(null=True, blank=True) + apply_url = models.TextField(null=False) + # Actually a postgres enum column + status = models.TextField(choices=STATUS_CHOICES, null=False) + created = models.DateTimeField(auto_now_add=True) + modified = models.DateTimeField(auto_now=True) + + +class CfbotTask(models.Model): + STATUS_CHOICES = [ + ('CREATED', 'Created'), + ('NEEDS_APPROVAL', 'Needs Approval'), + ('TRIGGERED', 'Triggered'), + ('EXECUTING', 'Executing'), + ('FAILED', 'Failed'), + ('COMPLETED', 'Completed'), + ('SCHEDULED', 'Scheduled'), + ('ABORTED', 'Aborted'), + ('ERRORED', 'Errored'), + ] + + # This id is only used by Django. Using text type for primary keys, has + # historically caused problems. + id = models.BigAutoField(primary_key=True) + # This is the id used by the external CI system. Currently with CirrusCI + # this is an integer, and thus we could probably store it as such. But + # given that we might need to change CI providers at some point, and that + # CI provider might use e.g. UUIDs, we prefer to consider the format of the + # ID opaque and store it as text. + task_id = models.TextField(unique=True) + task_name = models.TextField(null=False) + patch = models.ForeignKey(Patch, on_delete=models.CASCADE, related_name="cfbot_tasks") + branch_id = models.IntegerField(null=False) + position = models.IntegerField(null=False) + # Actually a postgres enum column + status = models.TextField(choices=STATUS_CHOICES, null=False) + created = models.DateTimeField(auto_now_add=True) + modified = models.DateTimeField(auto_now=True) diff --git a/pgcommitfest/commitfest/templates/commitfest.html b/pgcommitfest/commitfest/templates/commitfest.html index 63f793a6..6af61f39 100644 --- a/pgcommitfest/commitfest/templates/commitfest.html +++ b/pgcommitfest/commitfest/templates/commitfest.html @@ -63,6 +63,7 @@

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

{%if p.is_open%}Patch{%if sortkey == 0%}
{%endif%}{%endif%} Status Ver + CI status Author Reviewers Committer @@ -79,13 +80,38 @@

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

{%if grouping%} {%ifchanged p.topic%} - {{p.topic}} -{%endifchanged%} + {{p.topic}} +{%endifchanged%} {%endif%} {{p.name}} {{p.status|patchstatusstring}} {%if p.targetversion%}{{p.targetversion}}{%endif%} + + {%if p.cfbot_results %} + + {%if p.cfbot_results.needs_rebase %} + Needs rebase! + {%else%} + {%if p.cfbot_results.failed > 0 %} + + {%elif p.cfbot_results.completed < p.cfbot_results.total %} + + {%else%} + + {%endif%} + {{p.cfbot_results.completed}}/{{p.cfbot_results.total}} + {%endif%} + + {% else %} + Not processed + {%endif%} + {{p.author_names|default:''}} {{p.reviewer_names|default:''}} {{p.committer|default:''}} diff --git a/pgcommitfest/commitfest/templates/patch.html b/pgcommitfest/commitfest/templates/patch.html index 33670cf2..7cbd4408 100644 --- a/pgcommitfest/commitfest/templates/patch.html +++ b/pgcommitfest/commitfest/templates/patch.html @@ -12,6 +12,32 @@ Title {{patch.name}} + + CI (CFBot) + + {%if not cfbot_branch %} + Not processed + {%elif not cfbot_branch.commit_id %} + + Needs rebase! + {%else%} + + Summary + {%for c in cfbot_tasks %} + {%if c.status == 'COMPLETED'%} + + {%elif c.status == 'CREATED' %} + + {%elif c.status == 'EXECUTING' %} + + {%else %} + + {%endif%} + {%endfor%} + {%endif%} + + + Topic {{patch.topic}} @@ -138,7 +164,7 @@

Annotations

- {%for h in patch.history %} + {%for h in patch.history %} {{h.date}} {{h.by_string}} diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index 2c21cd3d..962770ee 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -14,12 +14,13 @@ from email.mime.text import MIMEText from email.utils import formatdate, make_msgid import json +import hmac import urllib from pgcommitfest.mailqueue.util import send_mail, send_simple_mail from pgcommitfest.userprofile.util import UserWrapper -from .models import CommitFest, Patch, PatchOnCommitFest, PatchHistory, Committer +from .models import CommitFest, Patch, PatchOnCommitFest, PatchHistory, Committer, CfbotBranch, CfbotTask from .models import MailThread from .forms import PatchForm, NewPatchForm, CommentForm, CommitFestFilterForm from .forms import BulkEmailForm @@ -213,7 +214,23 @@ def commitfest(request, cfid): (poc.status=ANY(%(openstatuses)s)) AS is_open, (SELECT string_agg(first_name || ' ' || last_name || ' (' || username || ')', ', ') FROM auth_user INNER JOIN commitfest_patch_authors cpa ON cpa.user_id=auth_user.id WHERE cpa.patch_id=p.id) AS author_names, (SELECT string_agg(first_name || ' ' || last_name || ' (' || username || ')', ', ') FROM auth_user INNER JOIN commitfest_patch_reviewers cpr ON cpr.user_id=auth_user.id WHERE cpr.patch_id=p.id) AS reviewer_names, -(SELECT count(1) FROM commitfest_patchoncommitfest pcf WHERE pcf.patch_id=p.id) AS num_cfs +(SELECT count(1) FROM commitfest_patchoncommitfest pcf WHERE pcf.patch_id=p.id) AS num_cfs, +( + SELECT row_to_json(t) as cfbot_results + from ( + SELECT + count(*) FILTER (WHERE task.status = 'COMPLETED') as completed, + count(*) FILTER (WHERE task.status in ('CREATED', 'SCHEDULED', 'EXECUTING')) running, + count(*) FILTER (WHERE task.status in ('ABORTED', 'ERRORED', 'FAILED')) failed, + count(*) total, + string_agg(task.task_name, ', ') FILTER (WHERE task.status in ('ABORTED', 'ERRORED', 'FAILED')) as failed_task_names, + branch.commit_id IS NULL as needs_rebase + FROM commitfest_cfbotbranch branch + LEFT JOIN commitfest_cfbottask task ON task.branch_id = branch.branch_id + WHERE branch.patch_id=p.id + GROUP BY branch.commit_id + ) t +) FROM commitfest_patch p INNER JOIN commitfest_patchoncommitfest poc ON poc.patch_id=p.id INNER JOIN commitfest_topic t ON t.id=p.topic_id @@ -311,6 +328,9 @@ def patch(request, cfid, patchid): patch_commitfests = PatchOnCommitFest.objects.select_related('commitfest').filter(patch=patch).order_by('-commitfest__startdate') committers = Committer.objects.filter(active=True).order_by('user__last_name', 'user__first_name') + cfbot_branch = getattr(patch, 'cfbot_branch', None) + cfbot_tasks = patch.cfbot_tasks.order_by('position') if cfbot_branch else [] + # XXX: this creates a session, so find a smarter way. Probably handle # it in the callback and just ask the user then? if request.user.is_authenticated: @@ -333,6 +353,8 @@ def patch(request, cfid, patchid): 'cf': cf, 'patch': patch, 'patch_commitfests': patch_commitfests, + 'cfbot_branch': cfbot_branch, + 'cfbot_tasks': cfbot_tasks, 'is_committer': is_committer, 'is_this_committer': is_this_committer, 'is_reviewer': is_reviewer, @@ -788,6 +810,119 @@ def _user_and_mail(u): }) +@transaction.atomic +def cfbot_ingest(message): + """Ingest a single message status update message receive from cfbot. It + should be a Python dictionary, decoded from JSON already.""" + + cursor = connection.cursor() + + branch_status = message["branch_status"] + patch_id = branch_status["submission_id"] + branch_id = branch_status["branch_id"] + + try: + Patch.objects.get(pk=patch_id) + except Patch.DoesNotExist: + # If the patch doesn't exist, there's nothing to do. This should never + # happen in production, but on the test system it's possible because + # not it doesn't contain the newest patches that the CFBot knows about. + return + + # Every message should have a branch_status, which we will INSERT + # or UPDATE. We do this first, because cfbot_task refers to it. + # Due to the way messages are sent/queued by cfbot it's possible that it + # sends the messages out-of-order. To handle this we we only update in two + # cases: + # 1. The created time of the branch is newer than the one in our database: + # This is a newer branch + # 2. If it's the same branch that we already have, but the modified time is + # newer: This is a status update for the current branch that we received + # in-order. + cursor.execute("""INSERT INTO commitfest_cfbotbranch (patch_id, branch_id, + branch_name, commit_id, + apply_url, status, + created, modified) + VALUES (%s, %s, %s, %s, %s, %s, %s, %s) + ON CONFLICT (patch_id) DO UPDATE + SET status = EXCLUDED.status, + modified = EXCLUDED.modified, + branch_id = EXCLUDED.branch_id, + branch_name = EXCLUDED.branch_name, + commit_id = EXCLUDED.commit_id, + apply_url = EXCLUDED.apply_url, + created = EXCLUDED.created + WHERE commitfest_cfbotbranch.created < EXCLUDED.created + OR (commitfest_cfbotbranch.branch_id = EXCLUDED.branch_id + AND commitfest_cfbotbranch.modified < EXCLUDED.modified) + """, + ( + patch_id, + branch_id, + branch_status["branch_name"], + branch_status["commit_id"], + branch_status["apply_url"], + branch_status["status"], + branch_status["created"], + branch_status["modified"]) + ) + + # Now we check what we have in our database. If that contains a different + # branch_id than what we just tried to insert, then apparently this is a + # status update for an old branch and we don't care about any of the + # contents of this message. + branch_in_db = CfbotBranch.objects.get(pk=patch_id) + if branch_in_db.branch_id != branch_id: + return + + # Most messages have a task_status. It might be missing in rare cases, like + # when cfbot decides that a whole branch has timed out. We INSERT or + # UPDATE. + if "task_status" in message: + task_status = message["task_status"] + cursor.execute("""INSERT INTO commitfest_cfbottask (task_id, task_name, patch_id, branch_id, + position, status, + created, modified) + VALUES (%s, %s, %s, %s, %s, %s, %s, %s) + ON CONFLICT (task_id) DO UPDATE + SET status = EXCLUDED.status, + modified = EXCLUDED.modified + WHERE commitfest_cfbottask.modified < EXCLUDED.modified""", + ( + task_status["task_id"], + task_status["task_name"], + patch_id, + branch_id, + task_status["position"], + task_status["status"], + task_status["created"], + task_status["modified"]) + ) + + # Remove any old tasks that are not related to this branch. These should + # only be left over when we just updated the branch_id. Knowing if we just + # updated the branch_id was is not trivial though, because INSERT ON + # CONFLICT does not allow us to easily return the old value of the row. + # So instead we always delete all tasks that are not related to this + # branch. This is fine, because doing so is very cheap in the no-op case + # because we have an index on patch_id and there's only a handful of tasks + # per patch. + cursor.execute("DELETE FROM commitfest_cfbottask WHERE patch_id=%s AND branch_id != %s", (patch_id, branch_id)) + + +@csrf_exempt +def cfbot_notify(request): + if request.method != 'POST': + return HttpResponseForbidden("Invalid method") + + j = json.loads(request.body) + if not hmac.compare_digest(j['shared_secret'], settings.CFBOT_SECRET): + return HttpResponseForbidden("Invalid API key") + + cfbot_ingest(j) + return HttpResponse(status=200) + + @csrf_exempt def thread_notify(request): if request.method != 'POST': diff --git a/pgcommitfest/urls.py b/pgcommitfest/urls.py index 53fce1c1..3230b047 100644 --- a/pgcommitfest/urls.py +++ b/pgcommitfest/urls.py @@ -36,6 +36,7 @@ re_path(r'^ajax/(\w+)/$', ajax.main), re_path(r'^lookups/user/$', lookups.userlookup), re_path(r'^thread_notify/$', views.thread_notify), + re_path(r'^cfbot_notify/$', views.cfbot_notify), # Auth system integration re_path(r'^(?:account/)?login/?$', pgcommitfest.auth.login), From 8e33e1e998d24ef92e9a34aab1103c0f100fa78b Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio Date: Sun, 12 Jan 2025 21:42:54 +0100 Subject: [PATCH 02/12] Integrate previous CFbot links with new code In previous commits (2ada851a24 & b6010e9cd2) some initial links and git checkout instructions were added to the patch page. I received some in person feedback at PgConf EU that those used quite some vertical space on the page. Now that we have a dedicated row in the table for CFbot statuses, it seems natural to integrate those previous links and checkout instructions into that row. Both to keep all CFbot content together, as well as to reduce vertical space needed. To reduce the necessary space needed for the checkout instructions, these previously explicit checkout instructions have been changed to button that copies the required commands to the clipboard. Finally, this also makes "Needs rebase!" link to CFbot logs instead of CI results. I updated these logs recently on the CFbot side to become much more useful to look at, by making them actually show the conflicts. --- media/commitfest/css/commitfest.css | 4 +++ media/commitfest/github-mark.svg | 1 + media/commitfest/js/commitfest.js | 7 +++++ .../commitfest/templates/commitfest.html | 25 +++++++++-------- pgcommitfest/commitfest/templates/patch.html | 27 ++++++++++--------- pgcommitfest/commitfest/views.py | 5 ++-- 6 files changed, 41 insertions(+), 28 deletions(-) create mode 100644 media/commitfest/github-mark.svg diff --git a/media/commitfest/css/commitfest.css b/media/commitfest/css/commitfest.css index e1a10f01..9189b9a3 100644 --- a/media/commitfest/css/commitfest.css +++ b/media/commitfest/css/commitfest.css @@ -69,3 +69,7 @@ div.form-group div.controls input.threadpick-input { .cfbot-summary img { margin-top: -3px; } + +.github-logo { + height: 20px; +} diff --git a/media/commitfest/github-mark.svg b/media/commitfest/github-mark.svg new file mode 100644 index 00000000..37fa923d --- /dev/null +++ b/media/commitfest/github-mark.svg @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/media/commitfest/js/commitfest.js b/media/commitfest/js/commitfest.js index 2f1eddae..aa4865c9 100644 --- a/media/commitfest/js/commitfest.js +++ b/media/commitfest/js/commitfest.js @@ -306,3 +306,10 @@ function searchUserListChanged() { $('#doSelectUserButton').addClass('disabled'); } } + +function addGitCheckoutToClipboard(patchId) { + navigator.clipboard.writeText(`git remote add commitfest https://github.com/postgresql-cfbot/postgresql.git +git fetch commitfest cf/${patchId} +git checkout commitfest/cf/${patchId} +`); +} diff --git a/pgcommitfest/commitfest/templates/commitfest.html b/pgcommitfest/commitfest/templates/commitfest.html index 6af61f39..1de741a7 100644 --- a/pgcommitfest/commitfest/templates/commitfest.html +++ b/pgcommitfest/commitfest/templates/commitfest.html @@ -87,17 +87,17 @@

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

{{p.name}} {{p.status|patchstatusstring}} {%if p.targetversion%}{{p.targetversion}}{%endif%} - - {%if p.cfbot_results %} - - {%if p.cfbot_results.needs_rebase %} + + {%if not p.cfbot_results %} + Not processed + {%elif p.cfbot_results.needs_rebase %} + Needs rebase! - {%else%} + + {%else%} + + {%if p.cfbot_results.failed > 0 %} {%elif p.cfbot_results.completed < p.cfbot_results.total %} @@ -105,11 +105,10 @@

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

{%else%} {%endif%} + {{p.cfbot_results.completed}}/{{p.cfbot_results.total}} - {%endif%} + - {% else %} - Not processed {%endif%} {{p.author_names|default:''}} diff --git a/pgcommitfest/commitfest/templates/patch.html b/pgcommitfest/commitfest/templates/patch.html index 7cbd4408..82d15b45 100644 --- a/pgcommitfest/commitfest/templates/patch.html +++ b/pgcommitfest/commitfest/templates/patch.html @@ -18,9 +18,14 @@ {%if not cfbot_branch %} Not processed {%elif not cfbot_branch.commit_id %} - - Needs rebase! + + Needs rebase! + Additional links previous successfully applied patch (outdated): + + + Summary {%else%} + Summary {%for c in cfbot_tasks %} @@ -34,6 +39,12 @@ {%endif%} {%endfor%} + {%endif%} + {%if cfbot_branch %} + {%endif%} @@ -81,21 +92,11 @@ Links - CFbot results (CirrusCI) - CFbot GitHub{%if patch.wikilink%} + {%if patch.wikilink%} Wiki{%endif%}{%if patch.gitlink%} Git {%endif%} - - Checkout latest CFbot patchset - - Go to your local checkout of the PostgreSQL repository and run: - 
git remote add commitfest https://github.com/postgresql-cfbot/postgresql.git
-git fetch commitfest cf/{{patch.id}}
-git checkout commitfest/cf/{{patch.id}}
- - Emails diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index 962770ee..f9e6c266 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -224,11 +224,12 @@ def commitfest(request, cfid): count(*) FILTER (WHERE task.status in ('ABORTED', 'ERRORED', 'FAILED')) failed, count(*) total, string_agg(task.task_name, ', ') FILTER (WHERE task.status in ('ABORTED', 'ERRORED', 'FAILED')) as failed_task_names, - branch.commit_id IS NULL as needs_rebase + branch.commit_id IS NULL as needs_rebase, + branch.apply_url FROM commitfest_cfbotbranch branch LEFT JOIN commitfest_cfbottask task ON task.branch_id = branch.branch_id WHERE branch.patch_id=p.id - GROUP BY branch.commit_id + GROUP BY branch.patch_id ) t ) FROM commitfest_patch p From 6a96e781d9d857f9cc9c4ef585b8876580a2238f Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio Date: Sun, 12 Jan 2025 21:43:00 +0100 Subject: [PATCH 03/12] Make CFbot add a history item when patch needs rebase This is needed to send notifications to a patch author that it subscribed. We don't send updates to other subscribers to the patch, as this information is generally only useful for the author themselves. By using the "history item" infrastructure it's also displayed on the patch page in the "History" table. That seems quite useful too, because it makes it clear how long the patch has needed a rebase. --- .../migrations/0007_needs_rebase_emails.py | 47 +++++++++++++++ pgcommitfest/commitfest/models.py | 60 ++++++++++++------- .../templates/mail/patch_notify.txt | 2 +- pgcommitfest/commitfest/views.py | 20 ++++++- 4 files changed, 105 insertions(+), 24 deletions(-) create mode 100644 pgcommitfest/commitfest/migrations/0007_needs_rebase_emails.py diff --git a/pgcommitfest/commitfest/migrations/0007_needs_rebase_emails.py b/pgcommitfest/commitfest/migrations/0007_needs_rebase_emails.py new file mode 100644 index 00000000..42740aa4 --- /dev/null +++ b/pgcommitfest/commitfest/migrations/0007_needs_rebase_emails.py @@ -0,0 +1,47 @@ +# Generated by Django 4.2.17 on 2024-12-25 11:17 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ("commitfest", "0006_cfbot_integration"), + ] + + operations = [ + migrations.AddField( + model_name="cfbotbranch", + name="needs_rebase_since", + field=models.DateTimeField(blank=True, null=True), + ), + migrations.AddField( + model_name="patchhistory", + name="by_cfbot", + field=models.BooleanField(default=False), + ), + migrations.AlterField( + model_name="patchhistory", + name="by", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + to=settings.AUTH_USER_MODEL, + ), + ), + migrations.AddConstraint( + model_name="patchhistory", + constraint=models.CheckConstraint( + check=models.Q( + models.Q(("by_cfbot", True), ("by__isnull", True)), + models.Q(("by_cfbot", False), ("by__isnull", False)), + _connector="OR", + ), + name="check_by", + ), + ), + ] diff --git a/pgcommitfest/commitfest/models.py b/pgcommitfest/commitfest/models.py index 32602424..c4c4355d 100644 --- a/pgcommitfest/commitfest/models.py +++ b/pgcommitfest/commitfest/models.py @@ -224,11 +224,15 @@ class Meta: class PatchHistory(models.Model): patch = models.ForeignKey(Patch, blank=False, null=False, on_delete=models.CASCADE) date = models.DateTimeField(blank=False, null=False, auto_now_add=True, db_index=True) - by = models.ForeignKey(User, blank=False, null=False, on_delete=models.CASCADE) + by = models.ForeignKey(User, blank=True, null=True, on_delete=models.CASCADE) + by_cfbot = models.BooleanField(null=False, blank=False, default=False) what = models.CharField(max_length=500, null=False, blank=False) @property def by_string(self): + if (self.by_cfbot): + return "CFbot" + return "%s %s (%s)" % (self.by.first_name, self.by.last_name, self.by.username) def __str__(self): @@ -236,34 +240,45 @@ def __str__(self): class Meta: ordering = ('-date', ) + constraints = [ + models.CheckConstraint( + check=( + models.Q(by_cfbot=True) & models.Q(by__isnull=True) + ) | ( + models.Q(by_cfbot=False) & models.Q(by__isnull=False) + ), + name='check_by', + ), + ] def save_and_notify(self, prevcommitter=None, - prevreviewers=None, prevauthors=None): + prevreviewers=None, prevauthors=None, authors_only=False): # Save this model, and then trigger notifications if there are any. There are # many different things that can trigger notifications, so try them all. self.save() recipients = [] - recipients.extend(self.patch.subscribers.all()) - - # Current or previous committer wants all notifications - try: - if self.patch.committer and self.patch.committer.user.userprofile.notify_all_committer: - recipients.append(self.patch.committer.user) - except UserProfile.DoesNotExist: - pass - - try: - if prevcommitter and prevcommitter.user.userprofile.notify_all_committer: - recipients.append(prevcommitter.user) - except UserProfile.DoesNotExist: - pass - - # Current or previous reviewers wants all notifications - recipients.extend(self.patch.reviewers.filter(userprofile__notify_all_reviewer=True)) - if prevreviewers: - # prevreviewers is a list - recipients.extend(User.objects.filter(id__in=[p.id for p in prevreviewers], userprofile__notify_all_reviewer=True)) + if not authors_only: + recipients.extend(self.patch.subscribers.all()) + + # Current or previous committer wants all notifications + try: + if self.patch.committer and self.patch.committer.user.userprofile.notify_all_committer: + recipients.append(self.patch.committer.user) + except UserProfile.DoesNotExist: + pass + + try: + if prevcommitter and prevcommitter.user.userprofile.notify_all_committer: + recipients.append(prevcommitter.user) + except UserProfile.DoesNotExist: + pass + + # Current or previous reviewers wants all notifications + recipients.extend(self.patch.reviewers.filter(userprofile__notify_all_reviewer=True)) + if prevreviewers: + # prevreviewers is a list + recipients.extend(User.objects.filter(id__in=[p.id for p in prevreviewers], userprofile__notify_all_reviewer=True)) # Current or previous authors wants all notifications recipients.extend(self.patch.authors.filter(userprofile__notify_all_author=True)) @@ -358,6 +373,7 @@ class CfbotBranch(models.Model): apply_url = models.TextField(null=False) # Actually a postgres enum column status = models.TextField(choices=STATUS_CHOICES, null=False) + needs_rebase_since = models.DateTimeField(null=True, blank=True) created = models.DateTimeField(auto_now_add=True) modified = models.DateTimeField(auto_now=True) diff --git a/pgcommitfest/commitfest/templates/mail/patch_notify.txt b/pgcommitfest/commitfest/templates/mail/patch_notify.txt index 1ab838c4..5f3d7443 100644 --- a/pgcommitfest/commitfest/templates/mail/patch_notify.txt +++ b/pgcommitfest/commitfest/templates/mail/patch_notify.txt @@ -5,7 +5,7 @@ have received updates in the PostgreSQL commitfest app: {{p.patch.name}} https://commitfest.postgresql.org/{{p.patch.patchoncommitfest_set.all.0.commitfest.id}}/{{p.patch.id}}/ {%for h in p.entries%} -* {{h.what}} ({{h.by}}){%endfor%} +* {{h.what}} by {{h.by_string()}}{%endfor%} {%endfor%} diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index f9e6c266..8d32ffd6 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -823,7 +823,7 @@ def cfbot_ingest(message): branch_id = branch_status["branch_id"] try: - Patch.objects.get(pk=patch_id) + patch = Patch.objects.get(pk=patch_id) except Patch.DoesNotExist: # If the patch doesn't exist, there's nothing to do. This should never # happen in production, but on the test system it's possible because @@ -910,6 +910,24 @@ def cfbot_ingest(message): # per patch. cursor.execute("DELETE FROM commitfest_cfbottask WHERE patch_id=%s AND branch_id != %s", (patch_id, branch_id)) + # We change the needs_rebase field using a separate UPDATE because this way + # we can find out what the previous state of the field was (sadly INSERT ON + # CONFLICT does not allow us to return that). We need to know the previous + # state so we can skip sending notifications if the needs_rebase status did + # not change. + needs_rebase = branch_status['commit_id'] is None + if bool(branch_in_db.needs_rebase_since) is not needs_rebase: + if needs_rebase: + branch_in_db.needs_rebase_since = datetime.now() + else: + branch_in_db.needs_rebase_since = None + branch_in_db.save() + + if needs_rebase: + PatchHistory(patch=patch, by=None, by_cfbot=True, what='Patch needs rebase').save_and_notify(authors_only=True) + else: + PatchHistory(patch=patch, by=None, by_cfbot=True, what='Patch does not need rebase anymore').save_and_notify(authors_only=True) + @csrf_exempt def cfbot_notify(request): From a9f9a193d18ceff6d773ff17c5b8f6f9c473ed2e Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio Date: Sun, 12 Jan 2025 21:43:05 +0100 Subject: [PATCH 04/12] 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 df8c7a30b2751fea58d070715df189dd77247cd1 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio Date: Mon, 13 Jan 2025 00:18:26 +0100 Subject: [PATCH 05/12] 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 1de741a7..aec3fbed 100644 --- a/pgcommitfest/commitfest/templates/commitfest.html +++ b/pgcommitfest/commitfest/templates/commitfest.html @@ -61,6 +61,7 @@

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

{%if p.is_open%}Patch{%if sortkey == 0%}
{%endif%}{%endif%} + {%if p.is_open%}ID{%if sortkey == 4%}
{%endif%}{%else%}ID{%endif%} Status Ver CI status @@ -85,6 +86,7 @@

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

{%endif%} {{p.name}} + {{p.id}} {{p.status|patchstatusstring}} {%if p.targetversion%}{{p.targetversion}}{%endif%} diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index 8d32ffd6..3add6c75 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 199bc75e98b7a8539bf04245bfa30add7df86c05 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio Date: Mon, 13 Jan 2025 01:20:56 +0100 Subject: [PATCH 06/12] 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 057dad4dfb91bfb1efa0e8d84901841071069d31 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio Date: Mon, 13 Jan 2025 01:21:27 +0100 Subject: [PATCH 07/12] 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 aec3fbed..8c80b077 100644 --- a/pgcommitfest/commitfest/templates/commitfest.html +++ b/pgcommitfest/commitfest/templates/commitfest.html @@ -60,7 +60,7 @@

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

- + diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index 3add6c75..50b67f29 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 7e10ee3a3bdf5a334db4c4c7cdf13015774a4c54 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio Date: Mon, 13 Jan 2025 01:27:10 +0100 Subject: [PATCH 08/12] 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 8c80b077..907b3cfb 100644 --- a/pgcommitfest/commitfest/templates/commitfest.html +++ b/pgcommitfest/commitfest/templates/commitfest.html @@ -60,17 +60,17 @@

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

{%if p.is_open%}Patch{%if sortkey == 0%}
{%endif%}{%endif%}		{%if p.is_open%}Patch{%if sortkey == 5%}
{%endif%}{%endif%}		 {%if p.is_open%}ID{%if sortkey == 4%}
{%endif%}{%else%}ID{%endif%}		 Status Ver
- - + + - - - + + + {%if user.is_staff%} {%endif%} From d54c83a1401541b3de34f953228dd288a2afef3f Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio Date: Mon, 13 Jan 2025 09:42:43 +0100 Subject: [PATCH 09/12] Update readme with correct django version --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From 013eeb1befa3cf3f38b7ccb7977881f2c7c0f38d Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio Date: Sun, 12 Jan 2025 23:25:23 +0100 Subject: [PATCH 10/12] 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 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 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. Co-Authored-By: Jacob Brazeal --- pgcommitfest/commitfest/models.py | 3 + .../commitfest/templates/commitfest.html | 2 +- pgcommitfest/commitfest/templates/patch.html | 2 +- .../commitfest/templates/patch_commands.inc | 4 +- pgcommitfest/commitfest/views.py | 99 +++++++++---------- pgcommitfest/urls.py | 27 +++-- 6 files changed, 71 insertions(+), 66 deletions(-) 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 907b3cfb..25047215 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%} - + diff --git a/pgcommitfest/commitfest/templates/patch.html b/pgcommitfest/commitfest/templates/patch.html index 82d15b45..db74b81a 100644 --- a/pgcommitfest/commitfest/templates/patch.html +++ b/pgcommitfest/commitfest/templates/patch.html @@ -68,7 +68,7 @@ diff --git a/pgcommitfest/commitfest/templates/patch_commands.inc b/pgcommitfest/commitfest/templates/patch_commands.inc index 8f18f712..9b78a686 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 50b67f29..4b52864e 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()) @@ -431,7 +432,7 @@ def newpatch(request, cfid): # Now add the thread try: doAttachThread(cf, patch, form.cleaned_data['threadmsgid'], request.user) - return HttpResponseRedirect("/%s/%s/edit/" % (cf.id, patch.id)) + return HttpResponseRedirect("/patch/%s/edit/" % (patch.id,)) except Http404: # Thread not found! # This is a horrible breakage of API layers @@ -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 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), From f660a4715cd93f07c77edc65b29aee20b0bf403b Mon Sep 17 00:00:00 2001 From: Jacob Brazeal Date: Mon, 20 Jan 2025 05:46:16 -0800 Subject: [PATCH 11/12] Adjust direction of dropdowns at the bottom of the page (#4) The bottom dropdowns on the patch page would expand downwards, requiring the user to scroll down to see and click any of the buttons in the dropdown. With this change these are changed into "dropup" menus, so the expand upwards. --- pgcommitfest/commitfest/templates/patch.html | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pgcommitfest/commitfest/templates/patch.html b/pgcommitfest/commitfest/templates/patch.html index db74b81a..69d72bcf 100644 --- a/pgcommitfest/commitfest/templates/patch.html +++ b/pgcommitfest/commitfest/templates/patch.html @@ -183,7 +183,9 @@

    Annotations

    {%if p.is_open%}Patch{%if sortkey == 5%}
    {%endif%}{%endif%}    		{%if p.is_open%}ID{%if sortkey == 4%}
    {%endif%}{%else%}ID{%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%}
    Status {%for c in patch_commitfests %} -
    {{c.commitfest}}: {{c.statusstring}}
    +
    {{c.commitfest}}: {{c.statusstring}}
    {%endfor%}
    +
    {%include "patch_commands.inc"%} +
    {%comment%}commit dialog{%endcomment%}