From ff941223ce9ac8eb38559a78231a5c7654486865 Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Mon, 10 Mar 2025 09:06:16 -0700 Subject: [PATCH 1/4] activity: use new /patch/ URLs Simplify the Activity query now that patch links no longer need a commitfest ID. --- pgcommitfest/commitfest/feeds.py | 7 +------ pgcommitfest/commitfest/templates/activity.html | 2 +- pgcommitfest/commitfest/views.py | 2 +- 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/pgcommitfest/commitfest/feeds.py b/pgcommitfest/commitfest/feeds.py index 9aff9025..38aee7a0 100644 --- a/pgcommitfest/commitfest/feeds.py +++ b/pgcommitfest/commitfest/feeds.py @@ -30,12 +30,7 @@ def item_description(self, item): ) def item_link(self, item): - if self.cfid: - return "https://commitfest.postgresql.org/{0}/{1}/".format( - self.cfid, item["patchid"] - ) - else: - return "https://commitfest.postgresql.org/{cfid}/{patchid}/".format(**item) + return "https://commitfest.postgresql.org/patch/{0}/".format(item["patchid"]) def item_pubdate(self, item): return item["date"] diff --git a/pgcommitfest/commitfest/templates/activity.html b/pgcommitfest/commitfest/templates/activity.html index 621155ee..13a9d428 100644 --- a/pgcommitfest/commitfest/templates/activity.html +++ b/pgcommitfest/commitfest/templates/activity.html @@ -16,7 +16,7 @@ {{a.date}} {{a.by}} - {{a.name}} + {{a.name}} {{a.what}} {%endfor%} diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index 471f3225..34496e5d 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -141,7 +141,7 @@ def activity(request, cfid=None, rss=None): cf = None where = "" - sql = "SELECT ph.date, auth_user.username AS by, ph.what, p.id AS patchid, p.name, (SELECT max(commitfest_id) FROM commitfest_patchoncommitfest poc WHERE poc.patch_id=p.id) AS cfid FROM commitfest_patchhistory ph INNER JOIN commitfest_patch p ON ph.patch_id=p.id INNER JOIN auth_user on auth_user.id=ph.by_id {0} ORDER BY ph.date DESC LIMIT {1}".format( + sql = "SELECT ph.date, auth_user.username AS by, ph.what, p.id AS patchid, p.name FROM commitfest_patchhistory ph INNER JOIN commitfest_patch p ON ph.patch_id=p.id INNER JOIN auth_user on auth_user.id=ph.by_id {0} ORDER BY ph.date DESC LIMIT {1}".format( where, num ) From 9f0cdd39d3aaa273a5dbd20b977f26637c8ce77a Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Mon, 10 Mar 2025 08:42:14 -0700 Subject: [PATCH 2/4] dashboard: treat only STATUS_CLOSED as closed The upcoming Parking Lot commitfest will break the assumption that lower commitfest IDs are always "in the past". --- pgcommitfest/commitfest/views.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index 34496e5d..9193ce43 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -323,7 +323,7 @@ def patchlist(request, cf, personalized=False): EXISTS ( SELECT 1 FROM commitfest_patch_authors cpa WHERE cpa.patch_id=p.id AND cpa.user_id=%(self)s ) AND ( - poc.commitfest_id < %(cid)s + cf.status = %(status_closed)s ) THEN 'Your still open patches in a closed commitfest (you should move or close these)' WHEN @@ -349,6 +349,7 @@ def patchlist(request, cf, personalized=False): cf.name AS cf_name, cf.status AS cf_status, """ + whereparams["status_closed"] = CommitFest.STATUS_CLOSED whereparams["needs_author"] = PatchOnCommitFest.STATUS_AUTHOR whereparams["needs_committer"] = PatchOnCommitFest.STATUS_COMMITTER is_committer = bool(Committer.objects.filter(user=request.user, active=True)) @@ -515,7 +516,13 @@ def commitfest(request, cfid): # Generate patch status summary. curs = connection.cursor() curs.execute( - "SELECT ps.status, ps.statusstring, count(*) FROM commitfest_patchoncommitfest poc INNER JOIN commitfest_patchstatus ps ON ps.status=poc.status WHERE commitfest_id=%(id)s GROUP BY ps.status ORDER BY ps.sortkey", + """SELECT ps.status, ps.statusstring, count(*) + FROM commitfest_patchoncommitfest poc + INNER JOIN commitfest_patchstatus ps ON ps.status=poc.status + INNER JOIN commitfest_commitfest cf ON cf.id=poc.commitfest_id + WHERE commitfest_id=%(id)s + GROUP BY ps.status + ORDER BY ps.sortkey""", { "id": cf.id, }, From b611f2159bb33d508bf5d4069947595e38349d8f Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Mon, 10 Mar 2025 09:26:04 -0700 Subject: [PATCH 3/4] Add STATUS_PARKED/STATUS_PARKING ...for the upcoming Parking Lot. --- .../migrations/0011_patch_status_parked.py | 46 +++++++++++++++++++ .../0012_commitfest_status_parking.py | 26 +++++++++++ pgcommitfest/commitfest/models.py | 6 +++ 3 files changed, 78 insertions(+) create mode 100644 pgcommitfest/commitfest/migrations/0011_patch_status_parked.py create mode 100644 pgcommitfest/commitfest/migrations/0012_commitfest_status_parking.py diff --git a/pgcommitfest/commitfest/migrations/0011_patch_status_parked.py b/pgcommitfest/commitfest/migrations/0011_patch_status_parked.py new file mode 100644 index 00000000..a66ca1c4 --- /dev/null +++ b/pgcommitfest/commitfest/migrations/0011_patch_status_parked.py @@ -0,0 +1,46 @@ +# Generated by Django 4.2.19 on 2025-03-07 23:55 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("commitfest", "0010_add_failing_since_column"), + ] + + operations = [ + migrations.AlterField( + model_name="patchoncommitfest", + name="status", + field=models.IntegerField( + choices=[ + (1, "Needs review"), + (2, "Waiting on Author"), + (3, "Ready for Committer"), + (4, "Committed"), + (5, "Moved to next CF"), + (6, "Rejected"), + (7, "Returned with feedback"), + (8, "Withdrawn"), + (9, "Moved to Parking Lot"), + ], + default=1, + ), + ), + migrations.RunSQL(""" +INSERT INTO commitfest_patchstatus (status, statusstring, sortkey) VALUES +(1,'Needs review',10), +(2,'Waiting on Author',15), +(3,'Ready for Committer',20), +(4,'Committed',25), +(5,'Moved to next CF',30), +(6,'Rejected',50), +(7,'Returned with Feedback',50), +(8,'Withdrawn', 50), +(9,'Moved to Parking Lot', 30) +ON CONFLICT (status) DO UPDATE SET statusstring=excluded.statusstring, sortkey=excluded.sortkey; +"""), + migrations.RunSQL( + "DELETE FROM commitfest_patchstatus WHERE status < 1 OR status > 9" + ), + ] diff --git a/pgcommitfest/commitfest/migrations/0012_commitfest_status_parking.py b/pgcommitfest/commitfest/migrations/0012_commitfest_status_parking.py new file mode 100644 index 00000000..9af3c813 --- /dev/null +++ b/pgcommitfest/commitfest/migrations/0012_commitfest_status_parking.py @@ -0,0 +1,26 @@ +# Generated by Django 4.2.19 on 2025-03-08 03:56 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("commitfest", "0011_patch_status_parked"), + ] + + operations = [ + migrations.AlterField( + model_name="commitfest", + name="status", + field=models.IntegerField( + choices=[ + (1, "Future"), + (2, "Open"), + (3, "In Progress"), + (4, "Closed"), + (5, "Parking"), + ], + default=1, + ), + ), + ] diff --git a/pgcommitfest/commitfest/models.py b/pgcommitfest/commitfest/models.py index fcd9edb9..6203767c 100644 --- a/pgcommitfest/commitfest/models.py +++ b/pgcommitfest/commitfest/models.py @@ -38,17 +38,20 @@ class CommitFest(models.Model): STATUS_OPEN = 2 STATUS_INPROGRESS = 3 STATUS_CLOSED = 4 + STATUS_PARKING = 5 _STATUS_CHOICES = ( (STATUS_FUTURE, "Future"), (STATUS_OPEN, "Open"), (STATUS_INPROGRESS, "In Progress"), (STATUS_CLOSED, "Closed"), + (STATUS_PARKING, "Parking"), ) _STATUS_LABELS = ( (STATUS_FUTURE, "default"), (STATUS_OPEN, "info"), (STATUS_INPROGRESS, "success"), (STATUS_CLOSED, "danger"), + (STATUS_PARKING, "info"), ) name = models.CharField(max_length=100, blank=False, null=False, unique=True) status = models.IntegerField( @@ -228,6 +231,7 @@ class PatchOnCommitFest(models.Model): STATUS_REJECTED = 6 STATUS_RETURNED = 7 STATUS_WITHDRAWN = 8 + STATUS_PARKED = 9 _STATUS_CHOICES = ( (STATUS_REVIEW, "Needs review"), (STATUS_AUTHOR, "Waiting on Author"), @@ -237,6 +241,7 @@ class PatchOnCommitFest(models.Model): (STATUS_REJECTED, "Rejected"), (STATUS_RETURNED, "Returned with feedback"), (STATUS_WITHDRAWN, "Withdrawn"), + (STATUS_PARKED, "Moved to Parking Lot"), ) _STATUS_LABELS = ( (STATUS_REVIEW, "default"), @@ -247,6 +252,7 @@ class PatchOnCommitFest(models.Model): (STATUS_REJECTED, "danger"), (STATUS_RETURNED, "danger"), (STATUS_WITHDRAWN, "danger"), + (STATUS_PARKED, "warning"), ) OPEN_STATUSES = [STATUS_REVIEW, STATUS_AUTHOR, STATUS_COMMITTER] From 6c6e1af73255d3fa41c30d2974b95fe54d6561fa Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Thu, 27 Feb 2025 14:53:41 -0800 Subject: [PATCH 4/4] WIP: add a Parking Lot The Parking Lot is a special always-open commitfest for the purpose of holding and testing draft patch submissions. Having such a holding area should make it easier for people to keep track of patchsets they're not quite ready to submit for review. Internally, the Parking Lot is assigned a static ID of zero. This is chosen because a) it does not overlap with Django's default AutoField sequence, which begins at one, and b) it requires no updates to the current URL patterns, which match nonnegative integers. The Parking Lot entry has the special status STATUS_PARKING so that it does not conflict with pre-existing coded assumptions on what "open", "future", etc. mean. STATUS_PARKING CFs are excluded from the "num_cfs" count for a patch. The new /close/parked handler is added to swap patches into the Parking Lot. Patches are then removed by moving them to the next open CF, or by closing as usual. Prior to this patch: - CFs with IDs less than the current in-progress CF could safely be assumed closed, - patches only ever moved forward through increasing CF IDs, and - the latest CF start date determined a patch's "current" CF. These assumptions all break under the current model. They have been modified: - use STATUS_CLOSED specifically when deciding whether a CF is closed - when moving a patch between CFs, allow for the possibility of an existing entry in the junction table - a patch's "current" CF is determined by its latest entry date TODO: - ensure all prior assumptions on CF ID are cleaned up - should the default filter be changed for the Parking Lot? --- media/commitfest/js/commitfest.js | 5 ++ .../migrations/0013_add_parking_lot.py | 16 ++++ pgcommitfest/commitfest/models.py | 2 +- pgcommitfest/commitfest/templates/home.html | 1 + .../commitfest/templates/patch_commands.inc | 1 + pgcommitfest/commitfest/views.py | 74 +++++++++++++++++-- pgcommitfest/settings.py | 2 + pgcommitfest/urls.py | 3 +- 8 files changed, 94 insertions(+), 10 deletions(-) create mode 100644 pgcommitfest/commitfest/migrations/0013_add_parking_lot.py diff --git a/media/commitfest/js/commitfest.js b/media/commitfest/js/commitfest.js index 77d49fc2..eb45abf8 100644 --- a/media/commitfest/js/commitfest.js +++ b/media/commitfest/js/commitfest.js @@ -18,6 +18,11 @@ function verify_next() { 'Are you sure you want to move this patch to the next commitfest?\n\nThis means the patch will be marked as closed in this commitfest, but will automatically be moved to the next one. If no further work is expected on this patch, it should be closed with "Rejected" or "Returned with Feedback" instead.\n\nSo - are you sure?', ); } +function verify_parked() { + return confirm( + 'Are you sure you want to park this patch?\n\nThis means the patch will be marked as closed in this commitfest, and automatically moved to the Parking Lot. Its status will be reset to Waiting on Author, and it will remain there until it is closed or moved to the next open commitfest.\n\nSo - are you sure?', + ); +} function findLatestThreads() { $("#attachThreadListWrap").addClass("loading"); $("#attachThreadSearchButton").addClass("disabled"); diff --git a/pgcommitfest/commitfest/migrations/0013_add_parking_lot.py b/pgcommitfest/commitfest/migrations/0013_add_parking_lot.py new file mode 100644 index 00000000..89e74bfd --- /dev/null +++ b/pgcommitfest/commitfest/migrations/0013_add_parking_lot.py @@ -0,0 +1,16 @@ +# Generated by Django 4.2.19 on 2025-03-10 16:28 + +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("commitfest", "0012_commitfest_status_parking"), + ] + + operations = [ + migrations.RunSQL(""" + INSERT INTO commitfest_commitfest (id, name, status) + VALUES (0, 'Parking Lot', 5); + """), + ] diff --git a/pgcommitfest/commitfest/models.py b/pgcommitfest/commitfest/models.py index 6203767c..389cb8e4 100644 --- a/pgcommitfest/commitfest/models.py +++ b/pgcommitfest/commitfest/models.py @@ -162,7 +162,7 @@ class Patch(models.Model, DiffableModel): } def current_commitfest(self): - return self.commitfests.order_by("-startdate").first() + return self.commitfests.order_by("-patchoncommitfest__enterdate").first() def current_patch_on_commitfest(self): cf = self.current_commitfest() diff --git a/pgcommitfest/commitfest/templates/home.html b/pgcommitfest/commitfest/templates/home.html index a3f26da0..0cb27c9a 100644 --- a/pgcommitfest/commitfest/templates/home.html +++ b/pgcommitfest/commitfest/templates/home.html @@ -24,6 +24,7 @@

Commands

List of commitfests

    +
  • Parking Lot (Drafts)
    • {%for c in commitfests%}
  • {{c}} ({{c.statusstring}}{%if c.startdate%} - {{c.periodstring}}{%endif%})
    • {%endfor%} diff --git a/pgcommitfest/commitfest/templates/patch_commands.inc b/pgcommitfest/commitfest/templates/patch_commands.inc index 8f09b18c..d9154455 100644 --- a/pgcommitfest/commitfest/templates/patch_commands.inc +++ b/pgcommitfest/commitfest/templates/patch_commands.inc @@ -22,6 +22,7 @@
  • Withdrawn
  • Returned with feedback
  • Move to next CF
    • +
  • Move to Parking Lot
  • Committed
diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index 9193ce43..ad570a76 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -46,7 +46,9 @@ def home(request): - commitfests = list(CommitFest.objects.all()) + # Skip "special" commitfest holding areas (primary keys <= 0); they're + # handled separately. + commitfests = list(CommitFest.objects.filter(pk__gt=0)) opencf = next((c for c in commitfests if c.status == CommitFest.STATUS_OPEN), None) inprogresscf = next( (c for c in commitfests if c.status == CommitFest.STATUS_INPROGRESS), None @@ -447,6 +449,7 @@ def patchlist(request, cf, personalized=False): params = { "openstatuses": PatchOnCommitFest.OPEN_STATUSES, "cid": cf.id, + "parking": CommitFest.STATUS_PARKING, } params.update(whereparams) @@ -458,7 +461,9 @@ def patchlist(request, cf, personalized=False): (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 + INNER JOIN commitfest_commitfest cf ON cf.id = pcf.commitfest_id + WHERE pcf.patch_id=p.id AND cf.status != %(parking)s) AS num_cfs, branch.needs_rebase_since, branch.failing_since, @@ -638,7 +643,7 @@ def patch(request, patchid): patch_commitfests = ( PatchOnCommitFest.objects.select_related("commitfest") .filter(patch=patch) - .order_by("-commitfest__startdate") + .order_by("-enterdate") .all() ) cf = patch_commitfests[0].commitfest @@ -1034,6 +1039,7 @@ def close(request, patchid, status): PatchOnCommitFest.STATUS_REVIEW, PatchOnCommitFest.STATUS_AUTHOR, PatchOnCommitFest.STATUS_COMMITTER, + PatchOnCommitFest.STATUS_PARKED, ): # This one can be moved pass @@ -1086,14 +1092,66 @@ def close(request, patchid, status): "/%s/%s/" % (poc.commitfest.id, poc.patch.id) ) # Create a mapping to the new commitfest that we are bouncing - # this patch to. - newpoc = PatchOnCommitFest( + # this patch to. Patches may be bounced back and forth from the parking + # lot, so we have to handle a potential previous entry for this patch. + PatchOnCommitFest.objects.update_or_create( patch=poc.patch, commitfest=newcf[0], - status=oldstatus, - enterdate=datetime.now(), + defaults=dict( + status=oldstatus, + enterdate=datetime.now(), + leavedate=None, + ), + ) + elif status == "parked": + # Parking has similar considerations to "next", but we're more lenient + # about what can be moved in. + if poc.status in ( + PatchOnCommitFest.STATUS_COMMITTED, + PatchOnCommitFest.STATUS_NEXT, + PatchOnCommitFest.STATUS_PARKED, + PatchOnCommitFest.STATUS_REJECTED, + ): + # Can't be moved! + messages.error( + request, + "A patch in status {0} cannot be moved to the parking lot.".format( + poc.statusstring + ), + ) + return HttpResponseRedirect("/%s/%s/" % (poc.commitfest.id, poc.patch.id)) + elif poc.status in ( + PatchOnCommitFest.STATUS_AUTHOR, + PatchOnCommitFest.STATUS_COMMITTER, + PatchOnCommitFest.STATUS_RETURNED, + PatchOnCommitFest.STATUS_REVIEW, + PatchOnCommitFest.STATUS_WITHDRAWN, + ): + # This one can be moved + pass + else: + messages.error(request, "Invalid existing patch status") + + poc.status = PatchOnCommitFest.STATUS_PARKED + + # Map this patch directly to the parking lot. + try: + parking_lot = CommitFest.objects.get(pk=0) + except CommitFest.DoesNotExist: + messages.error(request, "No parking lot exists!") + return HttpResponseRedirect("/%s/%s/" % (poc.commitfest.id, poc.patch.id)) + + # Patches may be bounced back and forth from the parking lot, so we have + # to handle a potential previous entry for this patch. + PatchOnCommitFest.objects.update_or_create( + patch=poc.patch, + commitfest=parking_lot, + defaults=dict( + status=PatchOnCommitFest.STATUS_AUTHOR, + enterdate=datetime.now(), + leavedate=None, + ), ) - newpoc.save() elif status == "committed": committer = get_object_or_404(Committer, user__username=request.GET["c"]) if committer != poc.patch.committer: diff --git a/pgcommitfest/settings.py b/pgcommitfest/settings.py index 9b867b71..99b1b3c0 100644 --- a/pgcommitfest/settings.py +++ b/pgcommitfest/settings.py @@ -19,6 +19,8 @@ } } +# "Static" commitfests use primary key values <= 0, so this must be an unsigned +# field type. DEFAULT_AUTO_FIELD = "django.db.models.AutoField" # Local time zone for this installation. Choices can be found here: diff --git a/pgcommitfest/urls.py b/pgcommitfest/urls.py index a67f55fc..5f8c6e61 100644 --- a/pgcommitfest/urls.py +++ b/pgcommitfest/urls.py @@ -27,7 +27,8 @@ re_path(r"^(\d+)/new/$", views.newpatch), re_path(r"^patch/(\d+)/status/(review|author|committer)/$", views.status), re_path( - r"^patch/(\d+)/close/(reject|withdrawn|feedback|committed|next)/$", views.close + r"^patch/(\d+)/close/(reject|withdrawn|feedback|committed|next|parked)/$", + views.close, ), re_path(r"^patch/(\d+)/reviewer/(become|remove)/$", views.reviewer), re_path(r"^patch/(\d+)/committer/(become|remove)/$", views.committer),