Remove Bugs from Workflow, Revert to Future-Open-Ongoing

Authors and committers can move patches. Only open patches can be committed or rejcted. Closed patches can no be reverted/re-opened if committed/rejected. Only open patch can be moved. Enforce via triggers that future commitfests may not have patches.
postgres · polobo · Apr 4, 2025 · Apr 4, 2025 · Apr 4, 2025 · Apr 4, 2025
commit 310a32020cdc06e39c3aa4ff73d1810053cdf23e
diff --git a/pgcommitfest/commitfest/migrations/0013_no_patches_in_future_cfs.py b/pgcommitfest/commitfest/migrations/0013_no_patches_in_future_cfs.py
@@ -0,0 +1,57 @@
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("commitfest", "0012_add_parked_cf_status"),
+    ]
+    operations = [
+        migrations.RunSQL("""
+CREATE FUNCTION assert_poc_not_future_for_poc()
+RETURNS TRIGGER AS $$
+DECLARE
+    cfstatus int;
+BEGIN
+    SELECT status INTO cfstatus
+    FROM commitfest_commitfest
+    WHERE id = NEW.commitfest_id;
+
+    IF cfstatus = 1 THEN
+       RAISE EXCEPTION 'Patches cannot exist on future commitfests';
+    END IF;
+
+    RETURN NEW;
+END;
+$$
+LANGUAGE plpgsql;
+
+CREATE FUNCTION assert_poc_not_future_for_cf()
+RETURNS trigger AS $$
+BEGIN
+    -- Trigger checks that we only get called when status is 1
+    PERFORM 1
+    FROM commitfest_patchoncommitfest
+    WHERE commitfest_id = NEW.id
+    LIMIT 1;
+
+    IF FOUND THEN
+       RAISE EXCEPTION 'Cannot change commitfest status to 1, patches exists.';
+    END IF;
+    RETURN NEW;
+END;
+$$
+LANGUAGE plpgsql;
+
+CREATE TRIGGER assert_poc_commitfest_is_not_future
+BEFORE INSERT OR UPDATE ON commitfest_patchoncommitfest
+FOR EACH ROW
+EXECUTE FUNCTION assert_poc_not_future_for_poc();
+
+CREATE TRIGGER assert_poc_commitfest_is_not_future
+-- Newly inserted cfs can't have patches
+BEFORE UPDATE ON commitfest_commitfest
+FOR EACH ROW
+WHEN (NEW.status = 1)
+EXECUTE FUNCTION assert_poc_not_future_for_cf();
+"""),
+    ]
diff --git a/pgcommitfest/commitfest/models.py b/pgcommitfest/commitfest/models.py
@@ -42,7 +42,7 @@ class CommitFest(models.Model):
    STATUS_PARKED = 5
    _STATUS_CHOICES = (
        (STATUS_FUTURE, "Future"),
-        (STATUS_OPEN, "Bugs"),
+        (STATUS_OPEN, "Open"),
        (STATUS_INPROGRESS, "Ongoing"),
        (STATUS_CLOSED, "Closed"),
        (STATUS_PARKED, "Drafts"),
@@ -85,10 +85,6 @@ def isclosed(self):
    def isopen(self):
        return self.status == self.STATUS_OPEN

-    @property
-    def isfuture(self):
-        return self.status == self.STATUS_FUTURE
-
    @property
    def isinprogress(self):
        return self.status == self.STATUS_INPROGRESS
@@ -301,6 +297,14 @@ def is_closed(self):
    def is_open(self):
        return not self.is_closed

+    @property
+    def is_committed(self):
+        return self.status == self.STATUS_COMMITTED
+
+    @property
+    def is_committer(self):
+        return self.status == self.STATUS_COMMITTER
+
    @property
    def statusstring(self):
        return [v for k, v in self._STATUS_CHOICES if k == self.status][0]
@@ -572,11 +576,6 @@ def open_cf():
        cfs = list(CommitFest.objects.filter(status=CommitFest.STATUS_OPEN))
        return cfs[0] if len(cfs) == 1 else None

-    # At most a single Future CommitFest is allowed and this function returns it.
-    def future_cf():
-        cfs = list(CommitFest.objects.filter(status=CommitFest.STATUS_FUTURE))
-        return cfs[0] if len(cfs) == 1 else None
-
    # At most a single In Progress CommitFest is allowed and this function returns it.
    def inprogress_cf():
        cfs = list(CommitFest.objects.filter(status=CommitFest.STATUS_INPROGRESS))
@@ -694,9 +693,8 @@ def userCanTransitionPatch(poc, target_cf, user):
            raise Exception("Cannot transition to an in-progress commitfest.")

        # Prevent users from moving closed patches, or moving open ones to
-        # non-open, non-future commitfests.  The else clause should be a
-        # can't happen.
-        if poc.is_open and (target_cf.isopen or target_cf.isfuture):
+        # non-open commitfests.  The else clause should be a can't happen.
+        if poc.is_open and target_cf.isopen:
            pass
        else:
            # Default deny policy basis

diff --git a/pgcommitfest/commitfest/templates/patch_administrative.inc b/pgcommitfest/commitfest/templates/patch_administrative.inc
@@ -24,13 +24,6 @@
     <li role="presentation"><a href="close/committed/" onclick="return flagCommitted({%if patch.committer%}'{{patch.committer}}'{%elif is_committer%}'{{user.username}}'{%else%}null{%endif%})">Commit</a></li>
     <li role="presentation" class="divider"></li>
     <li role="presentation" class="dropdown-header">Move To:</li>
-     {%if not cf.isfuture and workflow.future %}
-      <li role="presentation">
-       <a
-        href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.future.id}}">
-        {{workflow.future.name}}</a>
-      </li>
-     {%endif%}
     {%if not cf.isopen and workflow.open %}
      <li role="presentation">
       <a

diff --git a/pgcommitfest/commitfest/templates/patch_workflow.inc b/pgcommitfest/commitfest/templates/patch_workflow.inc
@@ -32,51 +32,53 @@
    <!-- Resolve -->
     <td>
      {%if is_committer or is_author %}
-       {%if is_committer %}
-        <a class="btn btn-default" href="close/committed/" onclick="return flagCommitted({%if patch.committer%}'{{patch.committer}}'{%elif is_committer%}'{{user.username}}'{%else%}null{%endif%})">Commit</a>
+       {%if is_committer and poc.is_open %}
+        {%if not poc.isparked and poc.is_committer %}
+         <a class="btn btn-default" href="close/committed/" onclick="return flagCommitted({%if patch.committer%}'{{patch.committer}}'{%elif is_committer%}'{{user.username}}'{%else%}null{%endif%})">Commit</a>
+        {%endif%}
        <a class="btn btn-default" href="close/reject/" onclick="return verify_reject()">Reject</a>
       {%endif%}
       {%if is_author %}
        <a class="btn btn-default" href="close/withdrawn/" onclick="return verify_withdrawn()">Withdraw</a>
       {%endif%}
+       {%if is_committer and not poc.is_open %}
+        <a class="btn btn-default" href="status/author/">
+         {{poc.is_committed|yesno:"Revert,Re-Open"}}
+        </a>
+       {%endif%}
      {%else%}
       <span>No Actions Available</span>
      {%endif%}
     </td>
    <!-- Move -->
     <td>
-      {%if not cf.isfuture and workflow.future %}
-       {%if True %}
-        <a class="btn btn-default"
-           href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.future.id}}">
-         {{workflow.future.name}}</a>
+      {%if poc.is_open %}
+       {%if not cf.isopen and workflow.open %}
+        {%if is_author or is_committer %}
+         <a class="btn btn-default"
+            href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.open.id}}">
+          {{workflow.open.name}}</a>
+        {%endif%}
       {%endif%}
-      {%endif%}

-      {%if not cf.isopen and workflow.open %}
-       {%if is_committer %}
-        <a class="btn btn-default"
-           href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.open.id}}">
-         {{workflow.open.name}}</a>
+       {%if not cf.isinprogress and workflow.progress %}
+        {%if is_committer %}
+         <a class="btn btn-default"
+            href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.progress.id}}">
+          {{workflow.progress.name}}</a>
+        {%endif%}
       {%endif%}
-      {%endif%}

-      {%if not cf.isinprogress and workflow.progress %}
-       {%if is_committer %}
-        <a class="btn btn-default"
-           href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.progress.id}}">
-         {{workflow.progress.name}}</a>
-       {%endif%}
-      {%endif%}
-
-      {%if not cf.isparked and workflow.parked %}
-       {%if is_author %}
-        <a class="btn btn-default"
-           href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.parked.id}}">
-         {{workflow.parked.name}}</a>
+       {%if not cf.isparked and workflow.parked %}
+        {%if is_author %}
+         <a class="btn btn-default"
+            href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.parked.id}}">
+          {{workflow.parked.name}}</a>
+        {%endif%}
       {%endif%}
+      {%else%}
+       <span>Cannot move closed patches.</span>
      {%endif%}
-
     </td>
    </tr>
   </tbody>

diff --git a/pgcommitfest/commitfest/templates/workflow.html b/pgcommitfest/commitfest/templates/workflow.html
@@ -3,21 +3,25 @@
 <h2>Key Concepts</h2>
 <h3>The Commitfest Workflow</h3>
 <p>
-  The commitfest workflow is a model for patch writing management.
-  There are a could of ways to classify patches.  First, they can be introducing
-  new features or fixing existing bugs (type).  Second, they can be awaiting changes,
+  The commitfest workflow is a model for patch writing management.  The workflow
+  is designed to manage new feature patches.  Bug fixes are typically handled
+  without the overhead of creating patches in the Commitfest application.
+  Thus the worflow broadly separates patches into "drafts" and
+  "potentially committable".  The former go onto the "Drafts v19" commitfest
+  while the later are initially placed into the open commitfest, which is
+  eventually converted into the ongoing commitfest.
+  Second, they can be awaiting changes,
  awaiting guidance, awaiting review, or waiting to be committed (status).
  The workflow uses commitfests and patch status in combination to communicate
  the overall state of the patch within these two categories.
 </p>
 <p>
  Commitfests are just bins filled with patches.  Bins have a defined lifespan
  after which they are "Closed" and a new bin for the same category is created.
-  Each bin serves a role in the workflow and there are 4 active categories of bins.
+  Each bin serves a role in the workflow and there are 3 active categories of bins.
  <ul>
-   <li>Bugs - annual, all bug reports</li>
   <li>Drafts - annual, drafts for new features</li>
-   <li>Next - scheduled, proposed new features</li>
+   <li>Open - scheduled, proposed new features</li>
   <li>Ongoing - scheduled, review and commit new features</li>
  </ul>
  There are three active categories of patch status covering the four statuses.
@@ -35,23 +39,17 @@ <h3>The Commitfest Workflow</h3>
  </ul>
  (See notes below for all available statuses and their intended usage.)
 </p>
- <p>
-  The patches in the bugs bin are all of type "bug fix".  The full lifecycle of
-  a bux fix patch lives here and there is no distinction as to the nature of the
-  reviewer category.  The remaining bins exist to help deal with the large volume
-  of new feature patches.
- </p>
 <p>
  The drafts bin is where patches waiting on significant work go.  A reviewer
  patch status category here mainly means awaiting guidance, though that will
-  often lead to a final review, allowing the patch to be moved to the future bin
+  often lead to a final review, allowing the patch to be moved to the open bin
  with the committer patch status category already set.
 </p>
 <p>
-  The future and ongoing bins are the ones that strictly comprise a commitfest.
-  The future bin always exists and new features ready for review and commit go
-  here.  At scheduled points, the future bin becomes an ongoing bin and a new
-  future bin is created.
+  The open and ongoing bins are the ones that strictly comprise a commitfest.
+  The open bin always exists and new features ready for review and commit go
+  here.  At scheduled points, the open bin becomes an ongoing bin and a new
+  open bin is created (possibly by reclassifying an existing future bin).
 </p>
 <p>
  The ongoing bin only exists during an active commitfest period, during which
@@ -60,11 +58,10 @@ <h3>The Commitfest Workflow</h3>
  the bin is closed.  This is only bin that is not present at all times.
 </p>
 <p>
-  The workflow is also designed with the release cycle in mind, in particular
-  the start of the annual feature freeze period.  At this time both the drafts
-  and bugs bins are closed and new ones created.  If the feature freeze is for
-  version 18 then the new drafts bin is called "Drafts v19" and the new bugs
-  bin is named "Bugs v18".
+  The workflow is designed with the release cycle in mind, in particular
+  the start of the annual feature freeze period.  At this time the drafts
+  bin is closed and a new one is created.  If the feature freeze is for
+  version 18 then the new drafts bin is called "Drafts v19".
 </p>
 <h3>Patches on Commitfests (PoC)</h3>
 <h4>Overview</h4>
@@ -112,7 +109,7 @@ <h4>Inactive</h4>
  A Rejected patch has the same effect as Withdrawn but communicates that the
  community, not the author, made the status change.  This should only be used
  when it is when the author is unable or unwilling to withdraw the patch or park
-  it for future rework.
+  it for rework.
 </p>
 <p>
  *Returned With Feedback complements rejected in that the implication of rejected
@@ -123,9 +120,10 @@ <h4>Inactive</h4>
  leaves reject as the fallback non-commit option and makes returned a deprecated
  administrator-only option.
 </p>
+ <h3>Special PoC Situations</h3>
 <h4>Moved</h4>
 <p>
-  Moved PoCs are a side-effect of having commitfest, and volume.  Many active
+  Moved PoCs are a side-effect of having commitfests, and volume.  Many active
  patches cannot be gotten to within a commitfest.  In order to keep them visible
  they must be moved to another commitfest, creating a new PoC with the same
  state.  The PoC they were moved from is updated to "Moved" to indicate this
@@ -137,6 +135,14 @@ <h4>Is Abandoned</h4>
  but the commitfest is closed.  Conceptually, this is the same as
  withdrawn, but through inaction.
 </p>
+ <h3>Reverted Patches</h3>
+ <p>
+  Not every patch that is committed stays that way.  The Workflow doesn't have
+  any statuses for this, though the user interface does provide an action that
+  a committer can invoke.  Upon doing so the PoC status is updated to
+  author.  The history flow of commited followed by author indictes a probable
+  revert.
+ </p>
 <h3>Patches</h3>
 <h4>Overview</h4>
 <p>
@@ -183,17 +189,21 @@ <h4>Ongoing</h4>
  An Active (see Workflow above) period where no new features should be added
  and the goal is to get as many review"patches committed as possible.
 </p>
- <h4>Future</h4>
+ <h4>Open</h4>
 <p>
-  The next active (see workflow above) period.
-  Any patch not exempted to be added to the ongoing or bugs commitfests
-  can always be added here.
+  Patches ready for final review and commit, according to the author, are placed
+  in the current open commitfest.  Upon the scheduled start date it is manually
+  updated to be an ongoing commitfest, at which point no new patches should be
+  added.
 </p>
- <h4>Bugs</h4>
+ <h4>Future</h4>
 <p>
-  A commitfest for high-priority and bug fix patches.  Full patch lifecycle.
-  Created as "Bugs v18" at the start of the v18 feature freeze period (closing
-  "Bugs v17").
+  The PostgreSQL project works on a schedule release cycle.  At the beginning
+  of each cycle the planned commitfest periods are decided and communicated to
+  the community via the creation of future commitfests.  While classified as
+  future these commitfests are not permitted to associated with any patches.
+  Their classification is changed to open as each prior ongoing commitfest
+  closes.
 </p>
 <h4>Drafts</h4>
 <p>
@@ -210,7 +220,7 @@ <h4>Drafts</h4>
 </p>
 <h4>Closed</h4>
 <p>
-  Drafts, bugs and ongoing commitfests are closed (future ones
+  Drafts and ongoing commitfests are closed (open ones
  always go through ongoing) when the period they cover has passed.
  Closing a commitfest does not impact its related PoCs; though no new PoCs can
  be created for a closed commitfest.

diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py
@@ -720,7 +720,6 @@ def patch(request, patchid):
            "userprofile": getattr(request.user, "userprofile", UserProfile()),
            "workflow": {
                "open": Workflow.open_cf(),
-                "future": Workflow.future_cf(),
                "progress": Workflow.inprogress_cf(),
                "parked": Workflow.parked_cf(),
            },
-Original file line number
+Diff line change
@@ -0,0 +1,57 @@
+    from django.db import migrations
+    class Migration(migrations.Migration):
+        dependencies = [
+            ("commitfest", "0012_add_parked_cf_status"),
+        ]
+        operations = [
+            migrations.RunSQL("""
+    CREATE FUNCTION assert_poc_not_future_for_poc()
             Copy link

  
      
    
  

  
      

  
    Collaborator


      

  

  
    
      

      
            JelteF
  

      

      

      


        Apr 13, 2025


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      Instead of restricting what you can do with "Future" commitfests, why not remove them completely? (honest question, they seem kinda useless to me)
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
  


          
          
        
      
    

    



  
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
    Contributor


      

  Author


  

  
    
      

      
            polobo
  

      

      

      


        Apr 14, 2025


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      I was going to but Robert immediately made the point that having a Calendar/Schedule of our planned Commitfests has value.  That could be done statically...but it is trivial to just make it be data-driven.  The workflow would just ignore them and anything else is just presentation.  Our existing "List of commitfests" is on the rework list.
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
  


          
          
        
      
    

    



  
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
    Collaborator


      

  

  
    
      

      
            JelteF
  

      

      

      


        Apr 14, 2025


      
  •

  
    
      
        
          edited
          
        
        
      
    
    
      
  
        
      Loading


  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  
    
  


    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      I definitely agree a schedule of planned commitfests has value, but having admins manually create "Future" commitfests is a pretty crude way to do that. And it doesn't even work well in practice, e.g. there hasn't been a single "Future" commitfest for at least 3 months.
I think a much better way would be to statically document/hardcode these dates, they are the same every year. This would also allow us to have a "Start commitfest" button that automatically creates a new "Open" commitfest for the next start/end date. i.e. instead of creating entries in the database with their only purpose being displaying a schedule, we could also display the schedule without these entries existing, because we already know the dates of the next commitfests.
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
  


          
          
        
      
    

    



  
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
    Contributor


      

  Author


  

  
    
      

      
            polobo
  

      

      

      


        Apr 14, 2025


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      All good points.  Probably want to put them into an issue at this point.
For my part I’m going to include the database constraint/trigger that prohibits Future CFs from having patches associated with them to at least avoid having to deal with that possibility when we do address this in the future.  It also allows removal of the “next” action implementation which is the primary incompatibility that exists with Workflow; which itself enables Parked/Drafts.
I’ll be changing the Action drop-down menu but postpone the main table rework as it will break CFBot.
CFBot can readily use the new Workflow endpoint to find the CF IDs it needs to scrape.  Given it already handles two CFs being scraped handling three should be a reasonable extension.
I’ll add a schedule to the new workflow doc page.
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
+    RETURNS TRIGGER AS $$
+    DECLARE
+        cfstatus int;
+    BEGIN
+        SELECT status INTO cfstatus
+        FROM commitfest_commitfest
+        WHERE id = NEW.commitfest_id;
+        IF cfstatus = 1 THEN
+           RAISE EXCEPTION 'Patches cannot exist on future commitfests';
+        END IF;
+        RETURN NEW;
+    END;
+    $$
+    LANGUAGE plpgsql;
+    CREATE FUNCTION assert_poc_not_future_for_cf()
+    RETURNS trigger AS $$
+    BEGIN
+        -- Trigger checks that we only get called when status is 1
+        PERFORM 1
+        FROM commitfest_patchoncommitfest
+        WHERE commitfest_id = NEW.id
+        LIMIT 1;
+        IF FOUND THEN
+           RAISE EXCEPTION 'Cannot change commitfest status to 1, patches exists.';
+        END IF;
+        RETURN NEW;
+    END;
+    $$
+    LANGUAGE plpgsql;
+    CREATE TRIGGER assert_poc_commitfest_is_not_future
+    BEFORE INSERT OR UPDATE ON commitfest_patchoncommitfest
+    FOR EACH ROW
+    EXECUTE FUNCTION assert_poc_not_future_for_poc();
+    CREATE TRIGGER assert_poc_commitfest_is_not_future
+    -- Newly inserted cfs can't have patches
+    BEFORE UPDATE ON commitfest_commitfest
+    FOR EACH ROW
+    WHEN (NEW.status = 1)
+    EXECUTE FUNCTION assert_poc_not_future_for_cf();
+    """),
+        ]