WIP: add a Drafts commitfest

Drafts 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, Drafts 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 Drafts entry has the special status STATUS_DRAFT so that it does not conflict with pre-existing coded assumptions on what "open", "future", etc. mean. STATUS_DRAFT CFs are excluded from the "num_cfs" count for a patch. The new /close/draft handler is added to swap patches into Drafts. 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
postgres · jchampio · Mar 10, 2025 · Mar 10, 2025 · Mar 10, 2025 · Feb 27, 2025
commit 9bc4ea195ba715f2d24d46ad40e7114479a6cfa2
diff --git 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_draft() {
+    return confirm(
+        'Are you sure you want to move this patch to Drafts?\n\nThis means the patch will be marked as closed in this commitfest. Its status will be reset to Waiting on Author, and it will remain in Drafts 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_drafts_cf.py b/pgcommitfest/commitfest/migrations/0013_add_drafts_cf.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_draft"),
+    ]
+
+    operations = [
+        migrations.RunSQL("""
+            INSERT INTO commitfest_commitfest (id, name, status)
+                                       VALUES (0, 'Drafts', 5);
+        """),
+    ]
diff --git 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
@@ -24,6 +24,7 @@ <h3>Commands</h3>
  </form>
  <h3>List of commitfests</h3>
  <ul>
+   <li><a href="/0/">Drafts</a></li>
   {%for c in commitfests%}
    <li><a href="/{{c.id}}/">{{c}}</a> ({{c.statusstring}}{%if c.startdate%} - {{c.periodstring}}{%endif%})</li>
   {%endfor%}

diff --git a/pgcommitfest/commitfest/templates/patch_commands.inc b/pgcommitfest/commitfest/templates/patch_commands.inc
@@ -22,6 +22,7 @@
      <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/?cfid={{cf.id}}" onclick="return verify_next()">Move to next CF</a></li>
+     <li role="presentation"><a href="close/draft/" onclick="return verify_draft()">Move to Drafts</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>

diff --git 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
@@ -474,6 +476,7 @@ def patchlist(request, cf, personalized=False):
     params = {
         "openstatuses": PatchOnCommitFest.OPEN_STATUSES,
         "cid": cf.id,
+        "draft": CommitFest.STATUS_DRAFT,
     }
     params.update(whereparams)
 
@@ -485,7 +488,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 != %(draft)s) AS num_cfs,
 
 branch.needs_rebase_since,
 branch.failing_since,
@@ -665,7 +670,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
@@ -1061,6 +1066,7 @@ def close(request, patchid, status):
             PatchOnCommitFest.STATUS_REVIEW,
             PatchOnCommitFest.STATUS_AUTHOR,
             PatchOnCommitFest.STATUS_COMMITTER,
+            PatchOnCommitFest.STATUS_DRAFT,
         ):
             # This one can be moved
             pass
@@ -1113,14 +1119,70 @@ 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 draft,
+        # 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 == "draft":
+        # The Drafts CF 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_DRAFT,
+            PatchOnCommitFest.STATUS_NEXT,
+            PatchOnCommitFest.STATUS_REJECTED,
+        ):
+            # Can't be moved!
+            messages.error(
+                request,
+                "A patch in status {0} cannot be moved to Drafts.".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_DRAFT
+
+        # Map this patch directly to the Drafts CF.
+        try:
+            drafts = CommitFest.objects.get(pk=0)
+        except CommitFest.DoesNotExist:
+            messages.error(request, "No draft commitfest exists!")
+            return HttpResponseRedirect("/%s/%s/" % (poc.commitfest.id, poc.patch.id))
+
+        if poc.commitfest == drafts:
+            messages.error(request, "Patch is already in Drafts.")
+            return HttpResponseRedirect("/%s/%s/" % (poc.commitfest.id, poc.patch.id))
+
+        # Patches may be bounced back and forth from draft, so we have
+        # to handle a potential previous entry for this patch.
+        PatchOnCommitFest.objects.update_or_create(
+            patch=poc.patch,
+            commitfest=drafts,
+            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
@@ -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
@@ -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|draft)/$",
+        views.close,
     ),
     re_path(r"^patch/(\d+)/reviewer/(become|remove)/$", views.reviewer),
     re_path(r"^patch/(\d+)/committer/(become|remove)/$", views.committer),