diff --git a/media/commitfest/css/commitfest.css b/media/commitfest/css/commitfest.css index 72cae0e..b6f2d61 100644 --- a/media/commitfest/css/commitfest.css +++ b/media/commitfest/css/commitfest.css @@ -91,3 +91,86 @@ div.form-group div.controls input.threadpick-input { .search-bar { display: inline-block; } + +/* Header styling */ +#workflow-transitions-header { + font-size: 20px; + font-weight: bold; + margin-top: 20px; + color: #333; + border-bottom: 2px solid #ddd; + padding-bottom: 5px; +} + +/* Table styling */ +#workflow-transitions-table { + width: 100%; + max-width: 500px; /* Updated max-width */ + border-collapse: collapse; + margin: 20px 0; /* Remove centering */ + font-size: 16px; + text-align: left; +} + +#workflow-transitions-table th, +#workflow-transitions-table td { + border: 1px solid #ddd; + padding: 8px; + width: 25%; /* Ensure all columns have the same width */ + text-align: center; /* Center-align cell labels */ +} + +#workflow-transitions-table th { + background-color: #f4f4f4; + color: #333; + font-weight: bold; + text-align: center; +} + +#workflow-transitions-table tr:nth-child(even) { + background-color: #f9f9f9; +} + +#workflow-transitions-table tr:hover { + background-color: #f1f1f1; +} + +#workflow-schedule-table { + width: 100%; + max-width: 800px; + border-collapse: collapse; + margin: 20px 0; /* Remove centering */ + font-size: 16px; + text-align: left; +} + +#workflow-schedule-table th, +#workflow-schedule-table td { + border: 1px solid #ddd; + padding: 8px; + text-align: center; /* Center-align cell labels */ +} + +#workflow-schedule-table td { + width: 15%; /* First four columns */ +} + +#workflow-schedule-table td:nth-child(5), +#workflow-schedule-table td:nth-child(6) { + width: 20%; /* Last two columns */ +} + +#workflow-schedule-table th { + background-color: #f4f4f4; + color: #333; + font-weight: bold; + text-align: center; +} + +#workflow-schedule-table tr:nth-child(even) { + background-color: #f9f9f9; +} + +#workflow-schedule-table tr:hover { + background-color: #f1f1f1; +} diff --git a/pgcommitfest/commitfest/apiv1.py b/pgcommitfest/commitfest/apiv1.py new file mode 100644 index 0000000..c7f972a --- /dev/null +++ b/pgcommitfest/commitfest/apiv1.py @@ -0,0 +1,42 @@ +from django.http import ( + HttpResponse, +) + +import json +from datetime import datetime + +from .models import ( + Workflow, +) + + +def datetime_serializer(obj): + if isinstance(obj, datetime): + return obj.strftime("%Y-%m-%dT%H:%M:%S%z") + raise TypeError("Type not serializable") + + +def apiResponse(request, payload, status=200, content_type="application/json"): + response = HttpResponse( + json.dumps(payload, default=datetime_serializer), status=status + ) + response["Content-Type"] = content_type + response["Access-Control-Allow-Origin"] = "*" + return response + + +def optional_as_json(obj): + if obj is None: + return None + return obj.json() + + +def active_commitfests(request): + payload = { + "workflow": { + "open": optional_as_json(Workflow.open_cf()), + "inprogress": optional_as_json(Workflow.inprogress_cf()), + "parked": optional_as_json(Workflow.parked_cf()), + }, + } + return apiResponse(request, payload) diff --git a/pgcommitfest/commitfest/fixtures/auth_data.json b/pgcommitfest/commitfest/fixtures/auth_data.json index 88d8c70..9a6e2f0 100644 --- a/pgcommitfest/commitfest/fixtures/auth_data.json +++ b/pgcommitfest/commitfest/fixtures/auth_data.json @@ -88,5 +88,41 @@ "groups": [], "user_permissions": [] } +}, +{ + "model": "auth.user", + "pk": 6, + "fields": { + "password": "", + "last_login": null, + "is_superuser": false, + "username": "prolific-author", + "first_name": "Prolific", + "last_name": "Author", + "email": "", + "is_staff": false, + "is_active": true, + "date_joined": "2025-01-01T00:00:00", + "groups": [], + "user_permissions": [] + } +}, +{ + "model": "auth.user", + "pk": 7, + "fields": { + "password": "", + "last_login": null, + "is_superuser": false, + "username": "prolific-reviewer", + "first_name": "Prolific", + "last_name": "Reviewer", + "email": "", + "is_staff": false, + "is_active": true, + "date_joined": "2025-01-01T00:00:00", + "groups": [], + "user_permissions": [] + } } ] diff --git a/pgcommitfest/commitfest/fixtures/commitfest_data.json b/pgcommitfest/commitfest/fixtures/commitfest_data.json index 6e5b32f..cd54202 100644 --- a/pgcommitfest/commitfest/fixtures/commitfest_data.json +++ b/pgcommitfest/commitfest/fixtures/commitfest_data.json @@ -60,6 +60,16 @@ "enddate": "2025-05-31" } }, +{ + "model": "commitfest.commitfest", + "pk": 5, + "fields": { + "name": "Drafts PG19", + "status": 5, + "startdate": "2024-09-01", + "enddate": "2025-08-31" + } +}, { "model": "commitfest.topic", "pk": 1, @@ -237,6 +247,25 @@ ] } }, +{ + "model": "commitfest.patch", + "pk": 8, + "fields": { + "name": "Test DGJ Multi-Author and Reviewer", + "topic": 3, + "wikilink": "", + "gitlink": "", + "targetversion": 1, + "committer": 4, + "created": "2025-02-01T00:00", + "modified": "2025-02-01T00:00", + "lastmail": "2025-02-01T00:00", + "authors": [6,3], + "reviewers": [7,1], + "subscribers": [], + "mailthread_set": [8] + } +}, { "model": "commitfest.patchoncommitfest", "pk": 1, @@ -325,6 +354,17 @@ "status": 1 } }, +{ + "model": "commitfest.patchoncommitfest", + "pk": 9, + "fields": { + "patch": 8, + "commitfest": 5, + "enterdate": "2025-02-01T00:00:00", + "leavedate": null, + "status": 1 + } +}, { "model": "commitfest.patchhistory", "pk": 1, @@ -632,6 +672,33 @@ "latestmsgid": "example@message-31" } }, +{ + "model": "commitfest.mailthread", + "pk": 8, + "fields": { + "messageid": "dgj-example@message-08", + "subject": "Test DGJ Multi-Author and Reviewer", + "firstmessage": "2025-02-01T00:00", + "firstauthor": "test@test.com", + "latestmessage": "2025-02-01T00:00", + "latestauthor": "test@test.com", + "latestsubject": "Test DGJ Multi-Author and Reviewer", + "latestmsgid": "dgj-example@message-08" + } +}, +{ + "model": "commitfest.mailthreadattachment", + "pk": 8, + "fields": { + "messageid": "dgj-example@message-08", + "attachmentid": 1, + "filename": "v1-0001-content.patch", + "date": "2025-02-01T00:00", + "author": "test@test.com", + "ispatch": true, + "mailthread_id": 8 + } +}, { "model": "commitfest.patchstatus", "pk": 1, diff --git a/pgcommitfest/commitfest/migrations/0011_add_status_related_constraints.py b/pgcommitfest/commitfest/migrations/0011_add_status_related_constraints.py new file mode 100644 index 0000000..17a8866 --- /dev/null +++ b/pgcommitfest/commitfest/migrations/0011_add_status_related_constraints.py @@ -0,0 +1,68 @@ +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("commitfest", "0010_add_failing_since_column"), + ] + operations = [ + migrations.RunSQL( + """ +CREATE UNIQUE INDEX cf_enforce_maxoneopen_idx +ON commitfest_commitfest (status) +WHERE status not in (1,4); +""", + reverse_sql=""" +DROP INDEX IF EXISTS cf_enforce_maxoneopen_idx; +""", + ), + migrations.RunSQL( + """ +CREATE UNIQUE INDEX poc_enforce_maxoneoutcome_idx +ON commitfest_patchoncommitfest (patch_id) +WHERE status not in (5); +""", + reverse_sql=""" +DROP INDEX IF EXISTS poc_enforce_maxoneoutcome_idx; +""", + ), + migrations.RunSQL( + """ +ALTER TABLE commitfest_patchoncommitfest +ADD CONSTRAINT status_and_leavedate_correlation +CHECK ((status IN (4,5,6,7,8)) = (leavedate IS NOT NULL)); +""", + reverse_sql=""" +ALTER TABLE commitfest_patchoncommitfest +DROP CONSTRAINT IF EXISTS status_and_leavedate_correlation; +""", + ), + migrations.RunSQL( + """ +COMMENT ON COLUMN commitfest_patchoncommitfest.leavedate IS +$$A leave date is recorded in two situations, both of which +means this particular patch-cf combination became inactive +on the corresponding date. For status 5 the patch was moved +to some other cf. For 4,6,7, and 8, this was the final cf. +$$ +""", + reverse_sql=""" +COMMENT ON COLUMN commitfest_patchoncommitfest.leavedate IS NULL; +""", + ), + migrations.RunSQL( + """ +COMMENT ON TABLE commitfest_patchoncommitfest IS +$$This is a re-entrant table: patches may become associated +with a given cf multiple times, resetting the entrydate and clearing +the leavedate each time. Non-final statuses never have a leavedate +while final statuses always do. The final status of 5 (moved) is +special in that all but one of the rows a patch has in this table +must have it as the status. +$$ +""", + reverse_sql=""" +COMMENT ON TABLE commitfest_patchoncommitfest IS NULL; +""", + ), + ] diff --git a/pgcommitfest/commitfest/migrations/0012_add_parked_cf_status.py b/pgcommitfest/commitfest/migrations/0012_add_parked_cf_status.py new file mode 100644 index 0000000..9075946 --- /dev/null +++ b/pgcommitfest/commitfest/migrations/0012_add_parked_cf_status.py @@ -0,0 +1,23 @@ +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("commitfest", "0011_add_status_related_constraints"), + ] + operations = [ + migrations.AlterField( + model_name="commitfest", + name="status", + field=models.IntegerField( + choices=[ + (1, "Future"), + (2, "Open"), + (3, "In Progress"), + (4, "Closed"), + (5, "Parked"), + ], + default=1, + ), + ) + ] diff --git a/pgcommitfest/commitfest/migrations/0013_no_patches_in_future_cfs.py b/pgcommitfest/commitfest/migrations/0013_no_patches_in_future_cfs.py new file mode 100644 index 0000000..d877ae1 --- /dev/null +++ b/pgcommitfest/commitfest/migrations/0013_no_patches_in_future_cfs.py @@ -0,0 +1,62 @@ +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 exist.'; + 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(); +""", + reverse_sql=""" +DROP TRIGGER IF EXISTS assert_poc_commitfest_is_not_future ON commitfest_commitfest; +DROP TRIGGER IF EXISTS assert_poc_commitfest_is_not_future ON commitfest_patchoncommitfest; +DROP FUNCTION IF EXISTS assert_poc_not_future_for_poc(); +DROP FUNCTION IF EXISTS assert_poc_not_future_for_cf(); +""", + ), + ] diff --git a/pgcommitfest/commitfest/models.py b/pgcommitfest/commitfest/models.py index fcd9edb..1d1d103 100644 --- a/pgcommitfest/commitfest/models.py +++ b/pgcommitfest/commitfest/models.py @@ -1,5 +1,6 @@ from django.contrib.auth.models import User from django.db import models +from django.db.models import Q from django.shortcuts import get_object_or_404 from datetime import datetime @@ -38,17 +39,20 @@ class CommitFest(models.Model): STATUS_OPEN = 2 STATUS_INPROGRESS = 3 STATUS_CLOSED = 4 + STATUS_PARKED = 5 _STATUS_CHOICES = ( (STATUS_FUTURE, "Future"), (STATUS_OPEN, "Open"), (STATUS_INPROGRESS, "In Progress"), (STATUS_CLOSED, "Closed"), + (STATUS_PARKED, "Drafts"), ) _STATUS_LABELS = ( (STATUS_FUTURE, "default"), (STATUS_OPEN, "info"), (STATUS_INPROGRESS, "success"), (STATUS_CLOSED, "danger"), + (STATUS_PARKED, "default"), ) name = models.CharField(max_length=100, blank=False, null=False, unique=True) status = models.IntegerField( @@ -63,6 +67,8 @@ def statusstring(self): @property def periodstring(self): + # Current Workflow intent is to have all Committfest be time-bounded + # but the information is just contextual so we still permit null if self.startdate and self.enddate: return "{0} - {1}".format(self.startdate, self.enddate) return "" @@ -71,10 +77,31 @@ def periodstring(self): def title(self): return "Commitfest %s" % self.name + @property + def isclosed(self): + return self.status == self.STATUS_CLOSED + @property def isopen(self): return self.status == self.STATUS_OPEN + @property + def isinprogress(self): + return self.status == self.STATUS_INPROGRESS + + @property + def isparked(self): + return self.status == self.STATUS_PARKED + + def json(self): + return { + "id": self.id, + "name": self.name, + "status": self.statusstring, + "startdate": self.startdate.isoformat(), + "enddate": self.enddate.isoformat(), + } + def __str__(self): return self.name @@ -159,11 +186,15 @@ class Patch(models.Model, DiffableModel): } def current_commitfest(self): - return self.commitfests.order_by("-startdate").first() + return self.current_patch_on_commitfest().commitfest def current_patch_on_commitfest(self): - cf = self.current_commitfest() - return get_object_or_404(PatchOnCommitFest, patch=self, commitfest=cf) + # The unique partial index poc_enforce_maxoneoutcome_idx stores the PoC + # No caching here (inside the instance) since the caller should just need + # the PoC once per request. + return get_object_or_404( + PatchOnCommitFest, Q(patch=self) & ~Q(status=PatchOnCommitFest.STATUS_NEXT) + ) # Some accessors @property @@ -273,6 +304,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 needs_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] @@ -528,3 +567,222 @@ class CfbotTask(models.Model): status = models.TextField(choices=STATUS_CHOICES, null=False) created = models.DateTimeField(auto_now_add=True) modified = models.DateTimeField(auto_now=True) + + +# Workflow provides access to the elements required to support +# the workflow this application is built for. These elements exist +# independent of what the user is presently seeing on their page. +class Workflow(models.Model): + def get_poc_for_patchid_or_404(patchid): + return get_object_or_404( + Patch.objects.select_related(), pk=patchid + ).current_patch_on_commitfest() + + # At most a single Open CommitFest is allowed and this function returns it. + 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 In Progress CommitFest is allowed and this function returns it. + def inprogress_cf(): + cfs = list(CommitFest.objects.filter(status=CommitFest.STATUS_INPROGRESS)) + return cfs[0] if len(cfs) == 1 else None + + # At most a single Parked CommitFest is allowed and this function returns it. + def parked_cf(): + cfs = list(CommitFest.objects.filter(status=CommitFest.STATUS_PARKED)) + return cfs[0] if len(cfs) == 1 else None + + # Returns whether the user is a committer in general and for this patch + # since we retrieve all committers in order to answer these questions + # provide that list as a third return value. Passing None for both user + # and patch still returns the list of committers. + def isCommitter(user, patch): + all_committers = Committer.objects.filter(active=True).order_by( + "user__last_name", "user__first_name" + ) + if not user and not patch: + return False, False, all_committers + + committer = [c for c in all_committers if c.user == user] + if len(committer) == 1: + is_committer = True + is_this_committer = committer[0] == patch.committer + else: + is_committer = is_this_committer = False + return is_committer, is_this_committer, all_committers + + def getCommitfest(cfid): + if cfid is None or cfid == "": + return None + try: + int_cfid = int(cfid) + cfs = list(CommitFest.objects.filter(id=int_cfid)) + if len(cfs) == 1: + return cfs[0] + else: + return None + except ValueError: + return None + + # Implements a re-entrant Commitfest POC creation procedure. + # Returns the new POC object. + # Creates history and notifies as a side-effect. + def createNewPOC(patch, commitfest, initial_status, by_user): + poc, created = PatchOnCommitFest.objects.update_or_create( + patch=patch, + commitfest=commitfest, + defaults=dict( + enterdate=datetime.now(), + status=initial_status, + leavedate=None, + ), + ) + poc.patch.set_modified() + poc.patch.save() + poc.save() + + PatchHistory( + patch=poc.patch, + by=by_user, + what="{} in {}".format(poc.statusstring, commitfest.name), + ).save_and_notify() + + return poc + + # The rule surrounding patches is they may only be in one active + # commitfest at a time. The transition function takes a patch + # open in one commitfest and associates it, with the same status, + # in a new commitfest; then makes it inactive in the original. + # Returns the new POC object. + # Creates history and notifies as a side-effect. + def transitionPatch(poc, target_cf, by_user): + Workflow.userCanTransitionPatch(poc, target_cf, by_user) + + existing_status = poc.status + + # History looks cleaner if we've left the existing + # commitfest entry before joining the new one. Plus, + # not allowed to change non-current commitfest status + # and once the new POC is created it becomes current. + + Workflow.updatePOCStatus(poc, PatchOnCommitFest.STATUS_NEXT, by_user) + + new_poc = Workflow.createNewPOC(poc.patch, target_cf, existing_status, by_user) + + return new_poc + + def userCanTransitionPatch(poc, target_cf, user): + # Policies not allowed to be broken by anyone. + + # Prevent changes to non-current commitfest for the patch + # Meaning, status changed to Moved before/during transitioning + # i.e., a concurrent action took place. + if poc.commitfest != poc.patch.current_commitfest(): + raise Exception("Patch commitfest is not its current commitfest.") + + # The UI should be preventing people from trying to perform no-op requests + if poc.commitfest.id == target_cf.id: + raise Exception("Cannot transition to the same commitfest.") + + # This one is arguable but facilitates treating non-open status as final + # A determined staff member can always change the status first. + if poc.is_closed: + raise Exception("Cannot transition a closed patch.") + + # We trust privileged users to make informed choices + if user.is_staff: + return + + if target_cf.isclosed: + raise Exception("Cannot transition to a closed commitfest.") + + if target_cf.isinprogress: + raise Exception("Cannot transition to an in-progress commitfest.") + + # Prevent users from moving closed patches, or moving open ones to + # 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 + raise Exception("Transition not permitted.") + + def userCanChangePOCStatus(poc, new_status, user): + # Policies not allowed to be broken by anyone. + + # Prevent changes to non-current commitfest for the patch + # Meaning, change status to Moved before/during transitioning + if poc.commitfest != poc.patch.current_commitfest(): + raise Exception("Patch commitfest is not its current commitfest.") + + # The UI should be preventing people from trying to perform no-op requests + if poc.status == new_status: + raise Exception("Cannot change to the same status.") + + # We want commits to happen from, usually, In Progress commitfests, + # or Open ones for exempt patches. We accept Future ones too just because + # they do represent a proper, if non-current, Commitfest. + if ( + poc.commitfest.id == CommitFest.STATUS_PARKED + and new_status == PatchOnCommitFest.STATUS_COMMITTED + ): + raise Exception("Cannot change status to committed in a parked commitfest.") + + # We trust privileged users to make informed choices + if user.is_staff: + return + + is_committer, is_this_committer, all_committers = Workflow.isCommitter( + user, poc.patch + ) + + # XXX Not sure if we want to tighten this up to is_this_committer + # with only the is_staff exemption + if new_status == PatchOnCommitFest.STATUS_COMMITTED and not is_committer: + raise Exception("Only a committer can set status to committed.") + + if new_status == PatchOnCommitFest.STATUS_REJECTED and not is_committer: + raise Exception("Only a committer can set status to rejected.") + + if new_status == PatchOnCommitFest.STATUS_RETURNED and not is_committer: + raise Exception("Only a committer can set status to returned.") + + if ( + new_status == PatchOnCommitFest.STATUS_WITHDRAWN + and user not in poc.patch.authors.all() + ): + raise Exception("Only the author can set status to withdrawn.") + + # Prevent users from modifying closed patches + # The else clause should be considered a can't happen + if poc.is_open: + pass + else: + raise Exception("Cannot change status of closed patch.") + + # Update the status of a PoC + # Returns True if the status was changed, False for a same-status no-op. + # Creates history and notifies as a side-effect. + def updatePOCStatus(poc, new_status, by_user): + # XXX Workflow disallows this no-op but not quite ready to enforce it. + if poc.status == new_status: + return False + + Workflow.userCanChangePOCStatus(poc, new_status, by_user) + + poc.status = new_status + poc.leavedate = datetime.now() if not poc.is_open else None + poc.patch.set_modified() + poc.patch.save() + poc.save() + PatchHistory( + patch=poc.patch, + by=by_user, + what="{} in {}".format( + poc.statusstring, + poc.commitfest.name, + ), + ).save_and_notify() + + return True diff --git a/pgcommitfest/commitfest/templates/base.html b/pgcommitfest/commitfest/templates/base.html index c70a7f7..b606d66 100644 --- a/pgcommitfest/commitfest/templates/base.html +++ b/pgcommitfest/commitfest/templates/base.html @@ -30,6 +30,9 @@ Log in {%endif%} +
  • + Workflow +
    • {%if header_activity%}
  • {{header_activity}}
    • {%endif%} diff --git a/pgcommitfest/commitfest/templates/me.html b/pgcommitfest/commitfest/templates/me.html index fd50f63..f4c801f 100644 --- a/pgcommitfest/commitfest/templates/me.html +++ b/pgcommitfest/commitfest/templates/me.html @@ -2,8 +2,15 @@ {%load commitfest %} {%block contents%} New patch - Current commitfest - Open commitfest + {%if workflow.inprogress%} + In Progress commitfest + {%endif%} + {%if workflow.open%} + Open commitfest + {%endif%} + {%if workflow.parked%} + Draft commitfest + {%endif%}
    diff --git a/pgcommitfest/commitfest/templates/patch_commands.inc b/pgcommitfest/commitfest/templates/patch_commands.inc index 8f09b18..2ec38bc 100644 --- a/pgcommitfest/commitfest/templates/patch_commands.inc +++ b/pgcommitfest/commitfest/templates/patch_commands.inc @@ -21,8 +21,29 @@
  • Rejected
  • Withdrawn
  • Returned with feedback
    • -
  • Move to next CF
  • Committed
    • + + {%if not cf.isopen and workflow.open %} +
  • + + {{workflow.open.name}} +
    • + {%endif%} + {%if not cf.isinprogress and workflow.inprogress and is_committer %} +
  • + + {{workflow.inprogress.name}} +
    • + {%endif%} + {%if not cf.isparked and workflow.parked %} +
  • + + {{workflow.parked.name}} +
    • + {%endif%}
    diff --git a/pgcommitfest/commitfest/templates/workflow-reference.html b/pgcommitfest/commitfest/templates/workflow-reference.html new file mode 100644 index 0000000..bd05522 --- /dev/null +++ b/pgcommitfest/commitfest/templates/workflow-reference.html @@ -0,0 +1,166 @@ +{%extends "base.html"%} +{%block contents%} +

    Overview

    +

    + This reference guide provides a quick reference to the key concepts within + the Commitfest Workflow. There is also an overview + of the workflow built from of these concepts. +

    +

    CFBot

    +

    + The CFBot is continuous integration (CI) infrastructure that uses threads + to retrieve patch files, builds and tests them, and posts the results to + the Patch. Only active patches are tested, and only if they are not in + a "Closed" commitfest. +

    +

    Patches

    +

    Overview

    +

    + Patches are the projects of the workflow and are linked to the mailing list + threads within which are messages containing versioned patch sets. Patches + are classified as being either active or inactive. They also record + contributors and their roles. +

    +

    Authors, Reviewers, and Committers

    +

    + Especially in open source projects, attribution for work is important. Git + commit messages include author and reviewer attributions (among others) and + inherently identify the committer. To aid the committer in properly recording + attributions in the commit message record authors and reviewers on the patch. +

    +

    + Additionally, the commitfest application uses this information to provide + user-specific reporting and notifications. +

    +

    Active

    +

    + A Patch is active if it is in one of the following states: +

    + These correspond to the three people-related fields of the patch and indicate + whose effort is presently needed. Of course, a patch may be in a state for + which no person is assigned; in which case the patch is advertising itself as + needing that kind of attention. +

    +

    Inactive

    +

    + A Patch is inactive if it is in one of the following states: +

    +

    +

    + A Committed patch is one whose files have been committed to the repository. + A Withdrawn patch is one where the author(s) have decided to no longer pursue + working on the patch and have proactively communicated that intent through + updating the patch to this state. +

    +

    + A Withdrawn patch is the desired outcome if a patch is not going to be committed. + Only an author can withdraw a patch. If the patch simply needs work it should + updated to author and placed into whichever commitfest bin is appropriate. +

    +

    + 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 rework. +

    +

    + *Returned With Feedback complements rejected in that the implication of rejected + is that the feature the patch introduces is unwanted while, here, the implementation + is simply not acceptable. The workflow takes a different approach and considers + both to be less desirable than withdraw. Considering the distinction between + author and committer making the decision to be the key difference the workflow + leaves reject as the fallback non-commit option and makes returned a deprecated + administrator-only option. +

    +

    Threads, Messages, and Patch Sets

    +

    + One or more email threads are related to a patch and found within the messages + on these threads are patch sets containing the actual files. The workflow + tracks the metadata for the files within the patch set while the CFBot + uses the metadata to determine which files to retrieve and apply for its tests. +

    +

    Commitfests

    +

    Overview

    +

    + Commitfests are just a collection of patches. The workflow described above + explains the purpose of these collections and defines which patches belong in + in which collection. One key constraint the described workflow imposes is that + among the statuses listed below at most one commitfest can be in each of them + at any given time (except for "Closed"). This allows for implementing movement + of a patch to be keyed to commitfest status type without the need for further + disambiguation. +

    +

    In Progress

    +

    + 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. +

    +

    Open

    +

    + 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 in progress commitfest, at which point no new patches should be + added. +

    +

    Future

    +

    + 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 in progress commitfest + closes. +

    +

    Drafts

    +

    + The commitfest setup as drafts is used to hold patches that are not intended + to be formally reviewed and committed. Another term is "work-in-progress" (WIP). + Within the Workflow, at the start of the PG18 feature freeze, the existing + "Draft PG18" commitfest is closed and a new "Draft PG19" commitfest is created. + This allows for a fresh start coinciding with the project release cycle. + And while commits cannot accumulate within a drafts commitfest, withdrawn and + rejected patches would and so having a truly never-closing commitfest is not + ideal. Similarly, given the volume of patches, getting rid of abandonment + is counter-productive. This workflow provides a middle-ground between + every-other-month and never patch moving requirements. +

    +

    Closed

    +

    + Drafts and in progress commitfests are closed (open ones + always go through in progress) when the period they cover has passed. + Closing a commitfest does not impact its related patches; though no new + patches can be created for a closed commitfest. +

    +

    Special Patch Situations

    +

    Moved

    +

    + Patches retain a memory of which commitfests they have been in. When relocated + from one commitfest to another the association with the old commitfest is + marked as being "Moved". This is a special status that is neither active nor + inactive. +

    +

    Is Ignored

    +

    + This check returns true if the patch is active but exists in a closed commitfest. + Conceptually, this is the same as withdrawn, but through inaction. +

    +

    History

    +

    + Textual event log for a patch. +

    +

    Administrative Actions (Admin)

    +

    + Protections put in place to guide the described workflow can be bypassed + by those with the appropriate permissions. Exercising these elevated + permissions is called an administrative action, or administratively performed. +

    +{%endblock%} diff --git a/pgcommitfest/commitfest/templates/workflow.html b/pgcommitfest/commitfest/templates/workflow.html new file mode 100644 index 0000000..9f89b61 --- /dev/null +++ b/pgcommitfest/commitfest/templates/workflow.html @@ -0,0 +1,222 @@ +{%extends "base.html"%} +{%block contents%} +

    + The first section covers the most important concepts in the Commitfest Workflow in a + fairly descriptive manner. A reference guide + is also available. The second section contains the actual schedule mentioned + in the first section. +

    +

    Key Concepts

    +

    The Commitfest Workflow

    +

    + The Commitfest Workflow (workflow) is a project management tool. It provides + swim lanes to organize projects and places those projects into a status as to + whether the author is working on it, a review is needed, or committer action + is desired. Projects are not directly placed into swim lanes however. + Instead, bins are created and bins are move about the swim lanes while + projects are placed into the bins. +

    +

    + As with many bespoke tools, the names given to things reflects + the problem domain. As this workflow operates on the domain of software + development the projects are called "Patches", bins are called "Commitfests" (CF), + and the swim lanes are just the "CF Statuses". A key productivity tool in software + development is a continuous integration (CI) service: that is named CFBot. +

    +

    + There are three active categories of patch status: +

    + And there are three preferred inactive categories of patch status for when + a patch has been resolved, either by being committed or not: + + There are a fixed set of CF Statuses for commitfests containing active patches. + + 1 The actual CF Status is labelled "Closed", and within that are both active + and inactive patches. The inactive patches are basically done once their + commitfest is closed. The active patches are not done, but the CFBot does not + spend compute time testing them thus ignoring them. Hence the swim lane name. +

    +

    + In the following table, scheduled means the commitfest itself will have its + CF status changed. Admin means a committer can perform a patch move that is + otherwise not permitted. The policy here is that new patches should not be + added to an in progress commitfest. Manual indicates the patch itself is + moved from one commitfest to another. +

    Commitfest Status Transitions

    + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
    From / ToDraftOpenIn ProgressIgnored
    DraftsN/AManualAdminScheduled
    OpenManualN/AScheduledNot Permitted
    In ProgressManualManualN/AScheduled
    IgnoredManualManualAdminN/A
    + +

    +

    Workflow Schedule

    +

    (See also the postgresql.org versioning policy.)

    +

    + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
    MonthDraftOpenIn ProgressIgnored1Releases
    JanuaryDrafts PG182025-032025-01<=2024
    February""None2025-01PG17.2
    March2Drafts PG192025-072025-03Drafts PG183, 2025-01
    April""None2025-01, 2025-03
    May""None2025-03PG17.3
    June""None2025-03
    July"2025-092025-072025-03
    August""None2025-03, 2025-07PG17.4
    September"2025-112025-092025-07PG18.04
    October""None2025-07, 2025-09
    November"2026-012025-112025-09PG18.1, Final PG13.23
    December""None2025-09, 2025-11
    +

    +

    +{%endblock%} diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index 40616ff..15a12c0 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -42,6 +42,7 @@ Patch, PatchHistory, PatchOnCommitFest, + Workflow, ) @@ -66,6 +67,26 @@ def home(request): ) +def workflow(request): + return render( + request, + "workflow.html", + { + "title": "Commitfest Workflow Overview", + }, + ) + + +def workflow_reference(request): + return render( + request, + "workflow-reference.html", + { + "title": "Commitfest Workflow Reference Guide", + }, + ) + + @login_required def me(request): cfs = list(CommitFest.objects.filter(status=CommitFest.STATUS_INPROGRESS)) @@ -128,6 +149,11 @@ def me(request): "header_activity": "Activity log", "header_activity_link": "/activity/", "userprofile": getattr(request.user, "userprofile", UserProfile()), + "workflow": { + "open": Workflow.open_cf(), + "inprogress": Workflow.inprogress_cf(), + "parked": Workflow.parked_cf(), + }, }, ) @@ -658,7 +684,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 @@ -708,6 +734,11 @@ def patch(request, patchid): {"title": cf.title, "href": "/%s/" % cf.pk}, ], "userprofile": getattr(request.user, "userprofile", UserProfile()), + "workflow": { + "open": Workflow.open_cf(), + "inprogress": Workflow.inprogress_cf(), + "parked": Workflow.parked_cf(), + }, }, ) @@ -998,6 +1029,9 @@ def status(request, patchid, status): @login_required @transaction.atomic def close(request, patchid, status): + if status == "next": + raise Exception("Can't happen, use transition/ endpoint") + patch = get_object_or_404(Patch.objects.select_related(), pk=patchid) cf = patch.current_commitfest() @@ -1034,86 +1068,6 @@ def close(request, patchid, status): poc.status = PatchOnCommitFest.STATUS_WITHDRAWN elif status == "feedback": poc.status = PatchOnCommitFest.STATUS_RETURNED - elif status == "next": - # Only some patch statuses can actually be moved. - if poc.status in ( - PatchOnCommitFest.STATUS_COMMITTED, - PatchOnCommitFest.STATUS_NEXT, - PatchOnCommitFest.STATUS_RETURNED, - PatchOnCommitFest.STATUS_REJECTED, - ): - # Can't be moved! - messages.error( - request, - "A patch in status {0} cannot be moved to next commitfest.".format( - poc.statusstring - ), - ) - return HttpResponseRedirect("/%s/%s/" % (poc.commitfest.id, poc.patch.id)) - elif poc.status in ( - PatchOnCommitFest.STATUS_REVIEW, - PatchOnCommitFest.STATUS_AUTHOR, - PatchOnCommitFest.STATUS_COMMITTER, - ): - # This one can be moved - pass - else: - messages.error(request, "Invalid existing patch status") - - oldstatus = poc.status - - poc.status = PatchOnCommitFest.STATUS_NEXT - # Figure out the commitfest to actually put it on - newcf = CommitFest.objects.filter(status=CommitFest.STATUS_OPEN) - if len(newcf) == 0: - # Ok, there is no open CF at all. Let's see if there is a - # future one. - newcf = CommitFest.objects.filter(status=CommitFest.STATUS_FUTURE) - if len(newcf) == 0: - messages.error(request, "No open and no future commitfest exists!") - return HttpResponseRedirect( - "/%s/%s/" % (poc.commitfest.id, poc.patch.id) - ) - elif len(newcf) != 1: - messages.error( - request, "No open and multiple future commitfests exist!" - ) - return HttpResponseRedirect( - "/%s/%s/" % (poc.commitfest.id, poc.patch.id) - ) - elif len(newcf) != 1: - messages.error(request, "Multiple open commitfests exists!") - return HttpResponseRedirect("/%s/%s/" % (poc.commitfest.id, poc.patch.id)) - elif newcf[0] == poc.commitfest: - # The current open CF is the same one that we are already on. - # In this case, try to see if there is a future CF we can - # move it to. - newcf = CommitFest.objects.filter(status=CommitFest.STATUS_FUTURE) - if len(newcf) == 0: - messages.error( - request, - "Cannot move patch to the same commitfest, and no future commitfests exist!", - ) - return HttpResponseRedirect( - "/%s/%s/" % (poc.commitfest.id, poc.patch.id) - ) - elif len(newcf) != 1: - messages.error( - request, - "Cannot move patch to the same commitfest, and multiple future commitfests exist!", - ) - return HttpResponseRedirect( - "/%s/%s/" % (poc.commitfest.id, poc.patch.id) - ) - # Create a mapping to the new commitfest that we are bouncing - # this patch to. - newpoc = PatchOnCommitFest( - patch=poc.patch, - commitfest=newcf[0], - status=oldstatus, - enterdate=datetime.now(), - ) - newpoc.save() elif status == "committed": committer = get_object_or_404(Committer, user__username=request.GET["c"]) if committer != poc.patch.committer: @@ -1143,6 +1097,50 @@ def close(request, patchid, status): return HttpResponseRedirect("/%s/%s/" % (poc.commitfest.id, poc.patch.id)) +@login_required +@transaction.atomic +def transition(request, patchid): + from_cf = Workflow.getCommitfest(request.GET.get("fromcfid", None)) + + if from_cf is None: + messages.error( + request, + "Unknown from commitfest id {}".format(request.GET.get("fromcfid", None)), + ) + return HttpResponseRedirect("/patch/%s/" % (patchid)) + + cur_poc = Workflow.get_poc_for_patchid_or_404(patchid) + + if from_cf != cur_poc.commitfest: + messages.error( + request, + "Transition aborted, Redirect performed. Commitfest for patch already changed.", + ) + return HttpResponseRedirect("/patch/%s/" % (cur_poc.patch.id)) + + target_cf = Workflow.getCommitfest(request.GET.get("tocfid", None)) + + if target_cf is None: + messages.error( + request, + "Unknown destination commitfest id {}".format( + request.GET.get("tocfid", None) + ), + ) + return HttpResponseRedirect("/patch/%s/" % (cur_poc.patch.id)) + + try: + new_poc = Workflow.transitionPatch(cur_poc, target_cf, request.user) + messages.info( + request, "Transitioned patch to commitfest %s" % new_poc.commitfest.name + ) + except Exception as e: + messages.error(request, "Failed to transition patch: {}".format(e)) + return HttpResponseRedirect("/patch/%s/" % (cur_poc.patch.id)) + + return HttpResponseRedirect("/patch/%s/" % (new_poc.patch.id)) + + @login_required @transaction.atomic def reviewer(request, patchid, status): diff --git a/pgcommitfest/urls.py b/pgcommitfest/urls.py index a67f55f..8d0d308 100644 --- a/pgcommitfest/urls.py +++ b/pgcommitfest/urls.py @@ -3,6 +3,7 @@ import pgcommitfest.auth import pgcommitfest.commitfest.ajax as ajax +import pgcommitfest.commitfest.apiv1 as apiv1 import pgcommitfest.commitfest.lookups as lookups import pgcommitfest.commitfest.reports as reports import pgcommitfest.commitfest.views as views @@ -15,6 +16,9 @@ urlpatterns = [ re_path(r"^$", views.home), + re_path(r"^api/v1/commitfest/active$", apiv1.active_commitfests), + re_path(r"^workflow/$", views.workflow), + re_path(r"^workflow-reference/$", views.workflow_reference), re_path(r"^me/$", views.me), re_path(r"^archive/$", views.archive), re_path(r"^activity(?P\.rss)?/", views.activity), @@ -26,9 +30,8 @@ re_path(r"^patch/(\d+)/edit/$", views.patchform), 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 - ), + re_path(r"^patch/(\d+)/close/(reject|withdrawn|feedback|committed)/$", views.close), + re_path(r"^patch/(\d+)/transition/$", views.transition), 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),