From aa4f9e0490e23d68278f32267b4f8b214acbac3a Mon Sep 17 00:00:00 2001 From: "David G. Johnston" Date: Fri, 4 Apr 2025 07:49:01 -0700 Subject: [PATCH 01/12] Staging Commit for Workflow Upgrading to Workflow involves many changes. These few are basicially non-invasive and set the stage for what is to come. Introduced: workflow.html - a help page for the new idea of Workflow CommitFest.STATUS_PARKED New Workflow Model to implement Workflow rules and functionality Enforcement of the Following Constraints: - "At most one" non-Closed Commitfest Status Presence - "At most one" non-Moved POC Status Presence - Closed POC status include leavedate, otherwise it is absent Also, added a convenience Make target for restoring test environment Also, added *.inc to the .editorconfig for html fragments Workflow Highlights: Commitfests are now just Future-In Progress-Closed Yearly Close/Create at v18 Feature Freeze: Drafts v19, Bugs v18 --- .editorconfig | 2 +- Makefile | 7 + .../commitfest/fixtures/auth_data.json | 36 +++ .../commitfest/fixtures/commitfest_data.json | 40 +++ .../0011_add_status_related_constraints.py | 45 +++ .../migrations/0012_add_parked_cf_status.py | 22 ++ pgcommitfest/commitfest/models.py | 268 ++++++++++++++++++ pgcommitfest/commitfest/templates/base.html | 3 + .../commitfest/templates/workflow.html | 234 +++++++++++++++ pgcommitfest/commitfest/views.py | 10 + pgcommitfest/urls.py | 1 + 11 files changed, 667 insertions(+), 1 deletion(-) create mode 100644 pgcommitfest/commitfest/migrations/0011_add_status_related_constraints.py create mode 100644 pgcommitfest/commitfest/migrations/0012_add_parked_cf_status.py create mode 100644 pgcommitfest/commitfest/templates/workflow.html diff --git a/.editorconfig b/.editorconfig index b1eff40a..e3256545 100644 --- a/.editorconfig +++ b/.editorconfig @@ -12,6 +12,6 @@ trim_trailing_whitespace = true indent_style = space indent_size = 4 -[*.html] +[*.{html,inc}] indent_style = space indent_size = 1 diff --git a/Makefile b/Makefile index c0343821..284c7a1b 100644 --- a/Makefile +++ b/Makefile @@ -16,3 +16,10 @@ lint-fix-unsafe: npx @biomejs/biome check --fix --unsafe fix: format lint-fix-unsafe + +init-dev: + dropdb --if-exists pgcommitfest + createdb pgcommitfest + ./manage.py migrate + ./manage.py loaddata auth_data.json + ./manage.py loaddata commitfest_data.json diff --git a/pgcommitfest/commitfest/fixtures/auth_data.json b/pgcommitfest/commitfest/fixtures/auth_data.json index 88d8c708..9a6e2f03 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 6e5b32ff..9317624a 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 v18", + "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": null, + "authors": [6,3], + "reviewers": [7,1], + "subscribers": [], + "mailthread_set": [] + } +}, { "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, 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 00000000..d6dae769 --- /dev/null +++ b/pgcommitfest/commitfest/migrations/0011_add_status_related_constraints.py @@ -0,0 +1,45 @@ +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 (4); +"""), + + migrations.RunSQL(""" +CREATE UNIQUE INDEX poc_enforce_maxoneoutcome_idx +ON commitfest_patchoncommitfest (patch_id) +WHERE status not in (5); +"""), + + migrations.RunSQL(""" +ALTER TABLE commitfest_patchoncommitfest +ADD CONSTRAINT status_and_leavedate_correlation +CHECK ((status IN (4,5,6,7,8)) = (leavedate IS NOT NULL)); +"""), + + 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. +$$ +"""), + + 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. +$$ +"""), + ] 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 00000000..3e5c49ec --- /dev/null +++ b/pgcommitfest/commitfest/migrations/0012_add_parked_cf_status.py @@ -0,0 +1,22 @@ +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/models.py b/pgcommitfest/commitfest/models.py index fcd9edb9..08cba6e8 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_PARKED = 5 _STATUS_CHOICES = ( (STATUS_FUTURE, "Future"), (STATUS_OPEN, "Open"), (STATUS_INPROGRESS, "In Progress"), (STATUS_CLOSED, "Closed"), + (STATUS_PARKED, "Parked"), ) _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 +66,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 +76,26 @@ 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 isfuture(self): + return self.status == self.STATUS_FUTURE + + @property + def isinprogress(self): + return self.status == self.STATUS_INPROGRESS + + @property + def isparked(self): + return self.status == self.STATUS_PARKED + def __str__(self): return self.name @@ -528,3 +549,250 @@ 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): + # 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 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)) + 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, change status to Moved before/during transitioning + if poc.commitfest != poc.patch.current_commitfest(): + raise Exception("PoC commitfest is not patch's 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, non-future commitfests. The else clause should be a + # can't happen. + if (poc.is_open and (target_cf.isopen or target_cf.isfuture)): + 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("PoC commitfest is not patch's 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 = 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 not user 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 + + # Sets the value of the committer for the patch + # Returns True if the committer was changed, False for a same-committer no-op. + # Creates history and notifies as a side-effect. + def setCommitter(poc, committer, by_user): + if (poc.patch.committer == committer): + return False + + prevcommitter = poc.patch.committer + poc.patch.committer = committer + poc.patch.set_modified() + poc.patch.save() + poc.save() + + if committer is None: + msg = "Removed {} as committer in {}".format(prevcommitter.fullname, poc.commitfest.name) + elif prevcommitter is None: + msg = "Set {} as committer in {}".format(poc.patch.committer.fullname, poc.commitfest.name) + else: + msg = "Changed to {} as committer in {}".format(poc.patch.committer.fullname, poc.commitfest.name) + + PatchHistory( + patch=poc.patch, by=by_user, + what=msg, + ).save_and_notify(prevcommitter=prevcommitter) + + return True diff --git a/pgcommitfest/commitfest/templates/base.html b/pgcommitfest/commitfest/templates/base.html index c70a7f77..b606d666 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/workflow.html b/pgcommitfest/commitfest/templates/workflow.html new file mode 100644 index 00000000..326d0bb5 --- /dev/null +++ b/pgcommitfest/commitfest/templates/workflow.html @@ -0,0 +1,234 @@ +{%extends "base.html"%} +{%block contents%} +

    Key Concepts

    +

    The Commitfest Workflow

    +

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

    +

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

    +There are three active categories of patch status covering the four statuses. + +And there are three preferred categories of patch status for when a patch has +been resolved, either by being committed or not. + +(See notes below for all available statuses and their intended usage.) +

    +

    + The patches in the open 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. +

    +

    + The parked 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 + with the committer patch status category already set. +

    +

    + The future and in progress 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 in progress bin and a new + future bin is created. +

    +

    + The in progress bin only exists during an active commitfest period, during which + no new patches should be added and the community reviewers and committers are + encouraged to focus on reviews and commits. At the end of the scheduled period + the bin is closed. This is only bin that is not present at all times. +

    +

    + 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 parked + and open bins are closed and new ones created. If the feature freeze is for + version 18 then the new parked bin is called "Drafts v19" and the new open + bin is named "Bugs v18". +

    +

    Patches on Commitfests (PoC)

    +

    Overview

    +

    + PoCs are the central concept of the commitfest workflow. When you are viewing + a Patch you are always implicitly acting within its primary PoC. The key + property of a PoC is its state, the possible values of which are: active, + inactive, moved, and abandoned. Each is described in detail below. +

    +

    Active

    +

    + A Patch is active if, in its primary PoC, 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, in its primary PoC, 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 PoC 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 future 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. +

    +

    Moved

    +

    + Moved PoCs are a side-effect of having commitfest, 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 + flow-of-time action. +

    +

    Is Abandoned

    +

    + This check returns true if the PoC state is in the set of active states + but the commitfest is closed. Conceptually, this is the same as + withdrawn, but through inaction. +

    +

    Patches

    +

    Overview

    +

    + Patches are the basic unit of work in the workflow. They are moved around + between various commitfest bins during their lifetime to reflect how they + interact with the focus of the community during each time period. + This movement, combined with the fact that a patch may only have a single + status, means that only one of the patch's PoCs can have a state that isn't + "Moved". While a patch's status is derived from its primary PoC, everything + else is attached to the patch itself. +

    +

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

    +

    Threads

    +

    + The actual content of a patch does not existing within the commitfest application + (more-or-less); instead, the patch file submission and review process is all done + on the -hackers mailing list. Threads are the means by which the commitfest + is made aware of these files and can incorporate information pertaining to them + into its reporting. +

    +

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

    +

    Future

    +

    + The next active (see workflow above) period. + Any patch not exempted to be added to the in progress or open commitfests + can always be added here. +

    +

    Open

    +

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

    +

    Parked

    +

    + The commitfest setup as parked 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 v18 feature freeze, the existing + "Draft v18" commitfest is closed and a new "Draft v19" commitfest is created. + This allows for a fresh start coinciding with the project release cycle. + And while commits cannot accumulate within a parked 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

    +

    + Parked, open and in progress commitfests are closed (future ones + always go through in progress) 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. +

    +

    History

    +

    + Textual event log for a patch. +

    +

    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 PoCs are tested. +

    +

    Administrative Actions

    +

    + 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/views.py b/pgcommitfest/commitfest/views.py index 40616ff8..7f89ed2e 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -66,6 +66,16 @@ def home(request): ) +def workflow(request): + return render( + request, + "workflow.html", + { + "title": "Workflow Overview", + }, + ) + + @login_required def me(request): cfs = list(CommitFest.objects.filter(status=CommitFest.STATUS_INPROGRESS)) diff --git a/pgcommitfest/urls.py b/pgcommitfest/urls.py index a67f55fc..96af678e 100644 --- a/pgcommitfest/urls.py +++ b/pgcommitfest/urls.py @@ -15,6 +15,7 @@ urlpatterns = [ re_path(r"^$", views.home), + re_path(r"^workflow/$", views.workflow), re_path(r"^me/$", views.me), re_path(r"^archive/$", views.archive), re_path(r"^activity(?P\.rss)?/", views.activity), From 87ab07c5b671eae6fd4c1ef02f4c8c6760660a4d Mon Sep 17 00:00:00 2001 From: "David G. Johnston" Date: Fri, 4 Apr 2025 11:32:24 -0700 Subject: [PATCH 02/12] Patch UX Enhancement Refactor In the interest of breaking up this major change to both UX, backend, and Workflow this patch separates out the main UX redesign choices for independent consideration. There is a new workflow display that will be polished and described once the final components have been incorporated. For now it simply places all of the action menu items on the screen; and incorporates a more coordinated and flow-oriented layout for the information in the existing key-value table. The #history-table is hidden by default but does include potentially useful information and so is expected to remain long-term. The status history field in the keyvalue-table may be moved here at some point. The elements from the #keyvalue-table that did not get incorporated into the workflow display are placed into a new workflow supplemental table. Unlike the key-value table, the new tables are horizontally compact. The #keyvalue-table is presently hidden but viewable. Conceptually, the contents of this table should never be needed in this view and at some point the table itself can be removed. For now it is left as a fallback. The existing action bar has been renamed the administrative bar. This will be visible only to staff and has minimal logic for showing and hiding UI widgets. That way, should the UI logic be faulty, it is generally still possible for staff to fix it. In this commit the contents of the bar remain unchanged, only the file name is different. --- media/commitfest/css/commitfest.css | 20 ++ pgcommitfest/commitfest/templates/patch.html | 247 ++++-------------- ..._commands.inc => patch_administrative.inc} | 0 .../templates/patch_table_keyvalue.inc | 195 ++++++++++++++ .../commitfest/templates/patch_tr_cfbot.inc | 53 ++++ .../commitfest/templates/patch_tr_email.inc | 56 ++++ .../commitfest/templates/patch_workflow.inc | 77 ++++++ pgcommitfest/commitfest/views.py | 9 +- 8 files changed, 459 insertions(+), 198 deletions(-) rename pgcommitfest/commitfest/templates/{patch_commands.inc => patch_administrative.inc} (100%) create mode 100644 pgcommitfest/commitfest/templates/patch_table_keyvalue.inc create mode 100644 pgcommitfest/commitfest/templates/patch_tr_cfbot.inc create mode 100644 pgcommitfest/commitfest/templates/patch_tr_email.inc create mode 100644 pgcommitfest/commitfest/templates/patch_workflow.inc diff --git a/media/commitfest/css/commitfest.css b/media/commitfest/css/commitfest.css index 72cae0e9..985491fb 100644 --- a/media/commitfest/css/commitfest.css +++ b/media/commitfest/css/commitfest.css @@ -91,3 +91,23 @@ div.form-group div.controls input.threadpick-input { .search-bar { display: inline-block; } + +.workflow table.table { + width: auto; +} + +#keyvalue-table { + display: none; +} + +#keyvalue-table.collapse.in { + display: table; +} + +#history-table { + display: none; +} + +#history-table.collapse.in { + display: table; +} diff --git a/pgcommitfest/commitfest/templates/patch.html b/pgcommitfest/commitfest/templates/patch.html index c2e73460..f80dc9f9 100644 --- a/pgcommitfest/commitfest/templates/patch.html +++ b/pgcommitfest/commitfest/templates/patch.html @@ -1,206 +1,59 @@ {%extends "base.html"%} {%load commitfest%} {%block contents%} - {%include "patch_commands.inc"%} - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - {%if patch.wikilink%} - Wiki{%endif%}{%if patch.gitlink%} - Git - {%endif%} - - - - - - - - - - -
    ID{{patch.id}}
    Title{{patch.name}}
    CI (CFBot) - {%if not cfbot_branch %} - Not processed - {%elif cfbot_branch.needs_rebase_since %} - - Needs rebase! - Needs rebase {% cfsince cfbot_branch.needs_rebase_since %}. {%if cfbot_branch.failing_since and cfbot_branch.failing_since != cfbot_branch.needs_rebase_since %}Failing {% cfsince cfbot_branch.failing_since %}. {%endif%}
    Additional links previous successfully applied patch (outdated):
    - - - Summary - {%else%} - - - Summary - {%for c in cfbot_tasks %} - {%if c.status == 'COMPLETED'%} - - {%elif c.status == 'CREATED' or c.status == 'SCHEDULED' %} - - {%elif c.status == 'EXECUTING' %} - - {%else %} - - {%endif%} - {%endfor%} - {%endif%} - {%if cfbot_branch %} - - {%endif%} - -
    Stats (from CFBot) - {%if cfbot_branch and cfbot_branch.commit_id %} - {%if cfbot_branch.version %} - Patch version: {{ cfbot_branch.version }}, - {%endif%} - Patch count: {{ cfbot_branch.patch_count }}, - First patch: +{{ cfbot_branch.first_additions }}−{{ cfbot_branch.first_deletions }}, - All patches: +{{ cfbot_branch.all_additions }}−{{ cfbot_branch.all_deletions }} - {%else%} - Unknown - {%endif%} -
    Topic{{patch.topic}}
    Created{{patch.created}}
    Last modified{{patch.modified}} ({% cfwhen patch.modified %})
    Latest email{%if patch.lastmail%}{{patch.lastmail}} ({% cfwhen patch.lastmail %}){%endif%}
    Status{%for c in patch_commitfests %} -
    {{c.commitfest}}: {{c.statusstring}}
    - {%endfor%} -
    Target version{%if patch.targetversion%}{{patch.targetversion}}{%endif%}
    Authors{{patch.authors_string}}
    Reviewers{{patch.reviewers_string}}{{is_reviewer|yesno:"Remove from reviewers,Become reviewer"}}
    Committer{%if patch.committer%}{{patch.committer.fullname}}{%endif%} - {%if is_committer%}{{is_this_committer|yesno:"Unclaim as committer,Claim as committer"}}{%endif%} -
    Links
    Emails - {%if user.is_authenticated%} -
    - {%else%} -
    - {%endif%} -
    - {%for t in patch.mailthread_set.all%} -
    {{t.subject}}
    -
    - First at {{t.firstmessage}} by {{t.firstauthor|hidemail}}
    - Latest at {{t.latestmessage}} by {{t.latestauthor|hidemail}}
    - {%for ta in t.mailthreadattachment_set.all%} - {%if forloop.first%} - Latest attachment ({{ta.filename}}) at {{ta.date}} from {{ta.author|hidemail}} -
    - {%endif%} -     Attachment ({{ta.filename}}) at {{ta.date}} from {{ta.author|hidemail}} (Patch: {{ta.ispatch|yesno:"Yes,No,Pending check"}})
    - {%if forloop.last%}
    {%endif%} - {%endfor%} -
    - {%for a in t.mailthreadannotation_set.all%} - {%if forloop.first%} -

    Annotations

    - - - - - - - - - - - {%endif%} - - - - - - - {%if forloop.last%} - -
    WhenWhoMailAnnotation
    {{a.date}}{{a.user_string}}From {{a.mailauthor}}
    at {{a.maildate}}    		{{a.annotationtext}}
    - {%endif%} - {%endfor%} - {%if user.is_authenticated%}{%endif%} -
    -
    - {%endfor%} -
    -
    History -
    - - - - - - - - - - {%for h in patch.history %} - - - - - - {%endfor%} - -
    WhenWhoWhat
    {{h.date}}{{h.by_string}}{{h.what}}
    -
    - {%if user.is_authenticated%} - {{is_subscribed|yesno:"Unsubscribe from patch update emails,Subscribe to patch update emails"}} - {%endif%} -
    + {%include "patch_administrative.inc"%} + {%include "patch_workflow.inc"%} + +
    + + + {%include "patch_tr_email.inc"%} + {%include "patch_tr_cfbot.inc"%} + + + + + +
    Links + {%if patch.wikilink%} + Wiki + {%endif%} + {%if patch.gitlink%} + Git + {%endif%} + Edit +
    +
    -
    - {%include "patch_commands.inc"%} +
    + +
    +
    +

    History

    + + + + + + + + + + {%for h in patch.history %} + + + + + + {%endfor%} + +
    WhenWhoWhat
    {{h.date}}{{h.by_string}}{{h.what}}
    +
    + +{%include "patch_table_keyvalue.inc"%} + {%comment%}commit dialog{%endcomment%} diff --git a/pgcommitfest/commitfest/templates/patch_workflow.inc b/pgcommitfest/commitfest/templates/patch_workflow.inc index 60d543c5..eca9bd1b 100644 --- a/pgcommitfest/commitfest/templates/patch_workflow.inc +++ b/pgcommitfest/commitfest/templates/patch_workflow.inc @@ -3,10 +3,10 @@ - - - - + + + + @@ -78,12 +78,23 @@ + + + + + + + + @@ -38,7 +49,9 @@
    - + {%if user.is_authenticated %} + + {%endif%}
    @@ -62,8 +75,9 @@

    History

    Assign ToAnnotateResolveMove ToAssign ToAnnotateResolveMove To
    Author(s)Reviewer(s)CommitterStatus
    {%for author in authors %}
    {{author.first_name}} {{author.last_name}}
    {%endfor%} + {%if is_author or is_committer %} + Edit Authors + {%endif%}     		From be4975391da612af38961783d32dba93dd176524 Mon Sep 17 00:00:00 2001 From: "David G. Johnston" Date: Sun, 6 Apr 2025 09:07:12 -0700 Subject: [PATCH 08/12] Protect actions behind user.is_authenticated The idea of having actions visible and then redirect for authentication is acceptable in general but the /patch view is now driven by the role of the user viewing the page that it seems easier to simply show no actions if the user is not authenticated and add a note just above the action table indicating the read-only mode. Add missing is_staff protection for the administrative actions form. Just outright removing the key-value table if you are not logged in. Don't want to edit it, want to get rid of it. --- pgcommitfest/commitfest/templates/patch.html | 22 +- .../commitfest/templates/patch_workflow.inc | 256 +++++++++--------- 2 files changed, 153 insertions(+), 125 deletions(-) diff --git a/pgcommitfest/commitfest/templates/patch.html b/pgcommitfest/commitfest/templates/patch.html index edeb3229..4f79cc9a 100644 --- a/pgcommitfest/commitfest/templates/patch.html +++ b/pgcommitfest/commitfest/templates/patch.html @@ -1,7 +1,16 @@ {%extends "base.html"%} {%load commitfest%} {%block contents%} - {%include "patch_administrative.inc"%} + {%if user.is_staff %} + {%include "patch_administrative.inc"%} + {%endif%} + + {%if not user.is_authenticated %} +
    + Read-Only mode. Log in to interact with this patch. +
    + {%endif%} + {%include "patch_workflow.inc"%}
    @@ -18,7 +27,9 @@ {%if patch.gitlink%} Git {%endif%} - Edit + {%if user.is_authenticated %} + Edit + {%endif%}
    - + {%if user.is_authenticated %} {%include "patch_table_keyvalue.inc"%} + {%endif%} {%comment%}commit dialog{%endcomment%}
    - {%if user.is_authenticated%}{%endif%} - Topic{%if user.is_authenticated%}{%endif%}: + Topic: {{ patch.topic }} + {%if user.is_authenticated %} + + {%endif%}
    Last Modified: {{patch.modified}} ({% cfwhen patch.modified %}) From a166d502a52aae0ff22041e4f3dbab50ae094dcb Mon Sep 17 00:00:00 2001 From: "David G. Johnston" Date: Sun, 6 Apr 2025 18:12:19 -0700 Subject: [PATCH 10/12] Rework labelling. More succinct, labels match actions, not statuses Categories are labels/titles and should use title case. Use initial capital letter on all non-joining words. In Progress is an OK label choice but Ongoing communicates the same thing with a single word just like all the other options. In the workflow the "Parked" status is being used for commitfests labelled, e.g., Draft v18, so use Drafts as the category label too. Change Open to Bugs to relfect its new usage. Once committed a large technical refactor/rename pass over the models and code to make the enums match up with the usage is planned. --- pgcommitfest/commitfest/models.py | 12 +++--- .../templates/patch_administrative.inc | 20 +++++----- .../commitfest/templates/patch_workflow.inc | 4 +- .../commitfest/templates/workflow.html | 40 +++++++++---------- 4 files changed, 38 insertions(+), 38 deletions(-) diff --git a/pgcommitfest/commitfest/models.py b/pgcommitfest/commitfest/models.py index a674d978..ddd00d0f 100644 --- a/pgcommitfest/commitfest/models.py +++ b/pgcommitfest/commitfest/models.py @@ -42,10 +42,10 @@ class CommitFest(models.Model): STATUS_PARKED = 5 _STATUS_CHOICES = ( (STATUS_FUTURE, "Future"), - (STATUS_OPEN, "Open"), - (STATUS_INPROGRESS, "In Progress"), + (STATUS_OPEN, "Bugs"), + (STATUS_INPROGRESS, "Ongoing"), (STATUS_CLOSED, "Closed"), - (STATUS_PARKED, "Parked"), + (STATUS_PARKED, "Drafts"), ) _STATUS_LABELS = ( (STATUS_FUTURE, "default"), @@ -255,13 +255,13 @@ class PatchOnCommitFest(models.Model): STATUS_RETURNED = 7 STATUS_WITHDRAWN = 8 _STATUS_CHOICES = ( - (STATUS_REVIEW, "Needs review"), + (STATUS_REVIEW, "Needs Review"), (STATUS_AUTHOR, "Waiting on Author"), (STATUS_COMMITTER, "Ready for Committer"), (STATUS_COMMITTED, "Committed"), - (STATUS_NEXT, "Moved to next CF"), + (STATUS_NEXT, "Moved"), (STATUS_REJECTED, "Rejected"), - (STATUS_RETURNED, "Returned with feedback"), + (STATUS_RETURNED, "Returned with Feedback"), (STATUS_WITHDRAWN, "Withdrawn"), ) _STATUS_LABELS = ( diff --git a/pgcommitfest/commitfest/templates/patch_administrative.inc b/pgcommitfest/commitfest/templates/patch_administrative.inc index 072bddb8..b48ba2f3 100644 --- a/pgcommitfest/commitfest/templates/patch_administrative.inc +++ b/pgcommitfest/commitfest/templates/patch_administrative.inc @@ -12,18 +12,18 @@
    Change Status
    {%endfor%} {%if user.is_authenticated %} - + {%endif%} {%if patch.committer%}
    {{patch.committer.fullname}}
    {%endif%} - {%if is_committer%}{%endif%} + {%if is_committer%}{%endif%} diff --git a/pgcommitfest/commitfest/templates/workflow.html b/pgcommitfest/commitfest/templates/workflow.html index 326d0bb5..618b43af 100644 --- a/pgcommitfest/commitfest/templates/workflow.html +++ b/pgcommitfest/commitfest/templates/workflow.html @@ -15,10 +15,10 @@

    The Commitfest Workflow

    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.
      -
    • Open - annual, all bug reports
      • -
    • Parked - annual, drafts for new features
      • -
    • Future - scheduled, proposed new features
      • -
    • In Progress - scheduled, review and commit new features
      • +
    • Bugs - annual, all bug reports
      • +
    • Drafts - annual, drafts for new features
      • +
    • Next - scheduled, proposed new features
      • +
    • Ongoing - scheduled, review and commit new features
    There are three active categories of patch status covering the four statuses.
      @@ -36,34 +36,34 @@

      The Commitfest Workflow

      (See notes below for all available statuses and their intended usage.)

      - The patches in the open bin are all of type "bug fix". The full lifecycle of + 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.

      - The parked bin is where patches waiting on significant work go. A reviewer + 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 with the committer patch status category already set.

      - The future and in progress bins are the ones that strictly comprise a commitfest. + 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 in progress bin and a new + here. At scheduled points, the future bin becomes an ongoing bin and a new future bin is created.

      - The in progress bin only exists during an active commitfest period, during which + The ongoing bin only exists during an active commitfest period, during which no new patches should be added and the community reviewers and committers are encouraged to focus on reviews and commits. At the end of the scheduled period the bin is closed. This is only bin that is not present at all times.

      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 parked - and open bins are closed and new ones created. If the feature freeze is for - version 18 then the new parked bin is called "Drafts v19" and the new open + 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".

      Patches on Commitfests (PoC)

      @@ -178,7 +178,7 @@

      Overview

      of a patch to be keyed to commitfest status type without the need for further disambiguation.

      -

      In Progress

      +

      Ongoing

      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. @@ -186,23 +186,23 @@

      In Progress

      Future

      The next active (see workflow above) period. - Any patch not exempted to be added to the in progress or open commitfests + Any patch not exempted to be added to the ongoing or bugs commitfests can always be added here.

      -

      Open

      +

      Bugs

      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").

      -

      Parked

      +

      Drafts

      - The commitfest setup as parked is used to hold patches that are not intended + 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 v18 feature freeze, the existing "Draft v18" commitfest is closed and a new "Draft v19" commitfest is created. This allows for a fresh start coinciding with the project release cycle. - And while commits cannot accumulate within a parked commitfest, withdrawn and + 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 @@ -210,8 +210,8 @@

      Parked

      Closed

      - Parked, open and in progress commitfests are closed (future ones - always go through in progress) when the period they cover has passed. + Drafts, bugs and ongoing commitfests are closed (future 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.

      From e6dd8ac67331ae12f314935a3037f963b72f170f Mon Sep 17 00:00:00 2001 From: "David G. Johnston" Date: Sun, 6 Apr 2025 18:44:56 -0700 Subject: [PATCH 11/12] Perform format and lint run. --- .../0011_add_status_related_constraints.py | 5 +- .../migrations/0012_add_parked_cf_status.py | 1 + pgcommitfest/commitfest/models.py | 69 +-- pgcommitfest/commitfest/templates/patch.html | 2 +- .../templates/patch_administrative.inc | 8 +- .../templates/patch_table_keyvalue.inc | 362 +++++++------- .../commitfest/templates/patch_tr_cfbot.inc | 2 +- .../commitfest/templates/patch_workflow.inc | 202 ++++---- .../commitfest/templates/workflow.html | 462 +++++++++--------- pgcommitfest/commitfest/views.py | 41 +- pgcommitfest/urls.py | 4 +- 11 files changed, 589 insertions(+), 569 deletions(-) diff --git a/pgcommitfest/commitfest/migrations/0011_add_status_related_constraints.py b/pgcommitfest/commitfest/migrations/0011_add_status_related_constraints.py index d6dae769..5291d190 100644 --- a/pgcommitfest/commitfest/migrations/0011_add_status_related_constraints.py +++ b/pgcommitfest/commitfest/migrations/0011_add_status_related_constraints.py @@ -1,5 +1,6 @@ from django.db import migrations + class Migration(migrations.Migration): dependencies = [ ("commitfest", "0010_add_failing_since_column"), @@ -10,19 +11,16 @@ class Migration(migrations.Migration): ON commitfest_commitfest (status) WHERE status not in (4); """), - migrations.RunSQL(""" CREATE UNIQUE INDEX poc_enforce_maxoneoutcome_idx ON commitfest_patchoncommitfest (patch_id) WHERE status not in (5); """), - migrations.RunSQL(""" ALTER TABLE commitfest_patchoncommitfest ADD CONSTRAINT status_and_leavedate_correlation CHECK ((status IN (4,5,6,7,8)) = (leavedate IS NOT NULL)); """), - migrations.RunSQL(""" COMMENT ON COLUMN commitfest_patchoncommitfest.leavedate IS $$A leave date is recorded in two situations, both of which @@ -31,7 +29,6 @@ class Migration(migrations.Migration): to some other cf. For 4,6,7, and 8, this was the final cf. $$ """), - migrations.RunSQL(""" COMMENT ON TABLE commitfest_patchoncommitfest IS $$This is a re-entrant table: patches may become associated diff --git a/pgcommitfest/commitfest/migrations/0012_add_parked_cf_status.py b/pgcommitfest/commitfest/migrations/0012_add_parked_cf_status.py index 3e5c49ec..90759466 100644 --- a/pgcommitfest/commitfest/migrations/0012_add_parked_cf_status.py +++ b/pgcommitfest/commitfest/migrations/0012_add_parked_cf_status.py @@ -1,5 +1,6 @@ from django.db import migrations, models + class Migration(migrations.Migration): dependencies = [ ("commitfest", "0011_add_status_related_constraints"), diff --git a/pgcommitfest/commitfest/models.py b/pgcommitfest/commitfest/models.py index ddd00d0f..e3dd279a 100644 --- a/pgcommitfest/commitfest/models.py +++ b/pgcommitfest/commitfest/models.py @@ -189,7 +189,9 @@ def current_patch_on_commitfest(self): # 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)) + return get_object_or_404( + PatchOnCommitFest, Q(patch=self) & ~Q(status=PatchOnCommitFest.STATUS_NEXT) + ) # Some accessors @property @@ -560,9 +562,10 @@ class CfbotTask(models.Model): # 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() + 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(): @@ -603,9 +606,8 @@ def isCommitter(user, patch): is_committer = is_this_committer = False return is_committer, is_this_committer, all_committers - def getCommitfest(cfid): - if (cfid is None or cfid == ""): + if cfid is None or cfid == "": return None try: int_cfid = int(cfid) @@ -624,7 +626,7 @@ def createNewPOC(patch, commitfest, initial_status, by_user): poc, created = PatchOnCommitFest.objects.update_or_create( patch=patch, commitfest=commitfest, - defaults = dict( + defaults=dict( enterdate=datetime.now(), status=initial_status, leavedate=None, @@ -635,10 +637,9 @@ def createNewPOC(patch, commitfest, initial_status, by_user): poc.save() PatchHistory( - patch=poc.patch, by=by_user, - what="{} in {}".format( - poc.statusstring, - commitfest.name) + patch=poc.patch, + by=by_user, + what="{} in {}".format(poc.statusstring, commitfest.name), ).save_and_notify() return poc @@ -659,16 +660,9 @@ def transitionPatch(poc, target_cf, by_user): # 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) + Workflow.updatePOCStatus(poc, PatchOnCommitFest.STATUS_NEXT, by_user) - new_poc = Workflow.createNewPOC( - poc.patch, - target_cf, - existing_status, - by_user) + new_poc = Workflow.createNewPOC(poc.patch, target_cf, existing_status, by_user) return new_poc @@ -702,7 +696,7 @@ def userCanTransitionPatch(poc, target_cf, user): # 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)): + if poc.is_open and (target_cf.isopen or target_cf.isfuture): pass else: # Default deny policy basis @@ -723,7 +717,10 @@ def userCanChangePOCStatus(poc, new_status, user): # 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: + 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 @@ -743,7 +740,10 @@ def userCanChangePOCStatus(poc, new_status, user): 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 not user in poc.patch.authors.all(): + 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 @@ -753,13 +753,12 @@ def userCanChangePOCStatus(poc, new_status, user): 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): + if poc.status == new_status: return False Workflow.userCanChangePOCStatus(poc, new_status, by_user) @@ -770,11 +769,12 @@ def updatePOCStatus(poc, new_status, by_user): poc.patch.save() poc.save() PatchHistory( - patch=poc.patch, by=by_user, + patch=poc.patch, + by=by_user, what="{} in {}".format( poc.statusstring, poc.commitfest.name, - ), + ), ).save_and_notify() return True @@ -783,7 +783,7 @@ def updatePOCStatus(poc, new_status, by_user): # Returns True if the committer was changed, False for a same-committer no-op. # Creates history and notifies as a side-effect. def setCommitter(poc, committer, by_user): - if (poc.patch.committer == committer): + if poc.patch.committer == committer: return False prevcommitter = poc.patch.committer @@ -793,14 +793,21 @@ def setCommitter(poc, committer, by_user): poc.save() if committer is None: - msg = "Removed {} as committer in {}".format(prevcommitter.fullname, poc.commitfest.name) + msg = "Removed {} as committer in {}".format( + prevcommitter.fullname, poc.commitfest.name + ) elif prevcommitter is None: - msg = "Set {} as committer in {}".format(poc.patch.committer.fullname, poc.commitfest.name) + msg = "Set {} as committer in {}".format( + poc.patch.committer.fullname, poc.commitfest.name + ) else: - msg = "Changed to {} as committer in {}".format(poc.patch.committer.fullname, poc.commitfest.name) + msg = "Changed to {} as committer in {}".format( + poc.patch.committer.fullname, poc.commitfest.name + ) PatchHistory( - patch=poc.patch, by=by_user, + patch=poc.patch, + by=by_user, what=msg, ).save_and_notify(prevcommitter=prevcommitter) diff --git a/pgcommitfest/commitfest/templates/patch.html b/pgcommitfest/commitfest/templates/patch.html index 4f79cc9a..7c2d78d1 100644 --- a/pgcommitfest/commitfest/templates/patch.html +++ b/pgcommitfest/commitfest/templates/patch.html @@ -76,7 +76,7 @@

      History

    {%if user.is_authenticated %} -{%include "patch_table_keyvalue.inc"%} + {%include "patch_table_keyvalue.inc"%} {%endif%} {%comment%}commit dialog{%endcomment%} diff --git a/pgcommitfest/commitfest/templates/patch_administrative.inc b/pgcommitfest/commitfest/templates/patch_administrative.inc index b48ba2f3..adcc0594 100644 --- a/pgcommitfest/commitfest/templates/patch_administrative.inc +++ b/pgcommitfest/commitfest/templates/patch_administrative.inc @@ -28,28 +28,28 @@
  • - {{workflow.future.name}} + {{workflow.future.name}}
    • {%endif%} {%if not cf.isopen and workflow.open %}
  • - {{workflow.open.name}} + {{workflow.open.name}}
    • {%endif%} {%if not cf.isinprogress and workflow.progress %}
  • - {{workflow.progress.name}} + {{workflow.progress.name}}
    • {%endif%} {%if not cf.isparked and workflow.parked %}
  • - {{workflow.parked.name}} + {{workflow.parked.name}}
    • {%endif%} diff --git a/pgcommitfest/commitfest/templates/patch_table_keyvalue.inc b/pgcommitfest/commitfest/templates/patch_table_keyvalue.inc index 8999e62f..27acdab7 100644 --- a/pgcommitfest/commitfest/templates/patch_table_keyvalue.inc +++ b/pgcommitfest/commitfest/templates/patch_table_keyvalue.inc @@ -1,195 +1,195 @@ {%load commitfest%} - - - - - - - - - - - - - +
    ID{{patch.id}}
    Title{{patch.name}}
    CI (CFBot) - {%if not cfbot_branch %} - Not processed - {%elif cfbot_branch.needs_rebase_since %} - - Needs rebase! - Needs rebase {% cfsince cfbot_branch.needs_rebase_since %}. {%if cfbot_branch.failing_since and cfbot_branch.failing_since != cfbot_branch.needs_rebase_since %}Failing {% cfsince cfbot_branch.failing_since %}. {%endif%}
    Additional links previous successfully applied patch (outdated):
    - - - Summary - {%else%} - - - Summary - {%for c in cfbot_tasks %} - {%if c.status == 'COMPLETED'%} - - {%elif c.status == 'CREATED' or c.status == 'SCHEDULED' %} - - {%elif c.status == 'EXECUTING' %} - - {%else %} - - {%endif%} - {%endfor%} - {%endif%} - {%if cfbot_branch %} - - {%endif%} - -
    + + + + - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - {%if patch.wikilink%} - Wiki{%endif%}{%if patch.gitlink%} - Git - {%endif%} + + - + + + + + + + + + + + + + + + + + + + + + + + + - - - - -
    ID{{patch.id}}
    Stats (from CFBot) - {%if cfbot_branch and cfbot_branch.commit_id %} - {%if cfbot_branch.version %} - Patch version: {{ cfbot_branch.version }}, - {%endif%} - Patch count: {{ cfbot_branch.patch_count }}, - First patch: +{{ cfbot_branch.first_additions }}−{{ cfbot_branch.first_deletions }}, - All patches: +{{ cfbot_branch.all_additions }}−{{ cfbot_branch.all_deletions }} - {%else%} - Unknown - {%endif%} -
    Topic{{patch.topic}}
    Created{{patch.created}}
    Last modified{{patch.modified}} ({% cfwhen patch.modified %})
    Latest email{%if patch.lastmail%}{{patch.lastmail}} ({% cfwhen patch.lastmail %}){%endif%}
    Status{%for c in patch_commitfests %} -
    {{c.commitfest}}: {{c.statusstring}}
    - {%endfor%} -
    Target version{%if patch.targetversion%}{{patch.targetversion}}{%endif%}
    Authors{{patch.authors_string}}
    Reviewers{{patch.reviewers_string}}{{is_reviewer|yesno:"Remove from reviewers,Become reviewer"}}
    Committer{%if patch.committer%}{{patch.committer.fullname}}{%endif%} - {%if is_committer%}{{is_this_committer|yesno:"Unclaim as committer,Claim as committer"}}{%endif%} -
    LinksTitle{{patch.name}}
    EmailsCI (CFBot) - {%if user.is_authenticated%} -
    + {%if not cfbot_branch %} + Not processed + {%elif cfbot_branch.needs_rebase_since %} + + Needs rebase! + Needs rebase {% cfsince cfbot_branch.needs_rebase_since %}. {%if cfbot_branch.failing_since and cfbot_branch.failing_since != cfbot_branch.needs_rebase_since %}Failing {% cfsince cfbot_branch.failing_since %}. {%endif%}
    Additional links previous successfully applied patch (outdated):
    + + + Summary {%else%} -
    - {%endif%} -
    - {%for t in patch.mailthread_set.all%} -
    {{t.subject}}
    -
    - First at {{t.firstmessage}} by {{t.firstauthor|hidemail}}
    - Latest at {{t.latestmessage}} by {{t.latestauthor|hidemail}}
    - {%for ta in t.mailthreadattachment_set.all%} - {%if forloop.first%} - Latest attachment ({{ta.filename}}) at {{ta.date}} from {{ta.author|hidemail}} -
    - {%endif%} -     Attachment ({{ta.filename}}) at {{ta.date}} from {{ta.author|hidemail}} (Patch: {{ta.ispatch|yesno:"Yes,No,Pending check"}})
    - {%if forloop.last%}
    {%endif%} - {%endfor%} -
    - {%for a in t.mailthreadannotation_set.all%} - {%if forloop.first%} -

    Annotations

    - - - - - - - - - - - {%endif%} - - - - - - - {%if forloop.last%} - -
    WhenWhoMailAnnotation
    {{a.date}}{{a.user_string}}From {{a.mailauthor}}
    at {{a.maildate}}    		{{a.annotationtext}}
    - {%endif%} - {%endfor%} - {%if user.is_authenticated%}{%endif%} -
    -
    + + + Summary + {%for c in cfbot_tasks %} + {%if c.status == 'COMPLETED'%} + + {%elif c.status == 'CREATED' or c.status == 'SCHEDULED' %} + + {%elif c.status == 'EXECUTING' %} + + {%else %} + + {%endif%} {%endfor%} -
    + {%endif%} + {%if cfbot_branch %} + + {%endif%} + +
    Stats (from CFBot) + {%if cfbot_branch and cfbot_branch.commit_id %} + {%if cfbot_branch.version %} + Patch version: {{ cfbot_branch.version }}, + {%endif%} + Patch count: {{ cfbot_branch.patch_count }}, + First patch: +{{ cfbot_branch.first_additions }}−{{ cfbot_branch.first_deletions }}, + All patches: +{{ cfbot_branch.all_additions }}−{{ cfbot_branch.all_deletions }} + {%else%} + Unknown + {%endif%} +
    Topic{{patch.topic}}
    Created{{patch.created}}
    Last modified{{patch.modified}} ({% cfwhen patch.modified %})
    Latest email{%if patch.lastmail%}{{patch.lastmail}} ({% cfwhen patch.lastmail %}){%endif%}
    Status{%for c in patch_commitfests %} +
    {{c.commitfest}}: {{c.statusstring}}
    + {%endfor%}
    History -
    - - - - - - - - - - {%for h in patch.history %} + + + + + + + + + + + + + + + + + + {%if patch.wikilink%} + Wiki{%endif%}{%if patch.gitlink%} + Git + {%endif%} + + + + -
    WhenWhoWhat
    Target version{%if patch.targetversion%}{{patch.targetversion}}{%endif%}
    Authors{{patch.authors_string}}
    Reviewers{{patch.reviewers_string}}{{is_reviewer|yesno:"Remove from reviewers,Become reviewer"}}
    Committer{%if patch.committer%}{{patch.committer.fullname}}{%endif%} + {%if is_committer%}{{is_this_committer|yesno:"Unclaim as committer,Claim as committer"}}{%endif%} +
    Links
    Emails + {%if user.is_authenticated%} +
    + {%else%} +
    + {%endif%} +
    + {%for t in patch.mailthread_set.all%} +
    {{t.subject}}
    +
    + First at {{t.firstmessage}} by {{t.firstauthor|hidemail}}
    + Latest at {{t.latestmessage}} by {{t.latestauthor|hidemail}}
    + {%for ta in t.mailthreadattachment_set.all%} + {%if forloop.first%} + Latest attachment ({{ta.filename}}) at {{ta.date}} from {{ta.author|hidemail}} +
    + {%endif%} +     Attachment ({{ta.filename}}) at {{ta.date}} from {{ta.author|hidemail}} (Patch: {{ta.ispatch|yesno:"Yes,No,Pending check"}})
    + {%if forloop.last%}
    {%endif%} + {%endfor%} +
    + {%for a in t.mailthreadannotation_set.all%} + {%if forloop.first%} +

    Annotations

    + + + + + + + + + + + {%endif%} - - - + + + + + {%if forloop.last%} + +
    WhenWhoMailAnnotation
    {{h.date}}{{h.by_string}}{{h.what}}{{a.date}}{{a.user_string}}From {{a.mailauthor}}
    at {{a.maildate}}    		{{a.annotationtext}}
    + {%endif%} {%endfor%} -
    -
    - {%if user.is_authenticated%} - {{is_subscribed|yesno:"Unsubscribe from patch update emails,Subscribe to patch update emails"}} - {%endif%} -
    + {%if user.is_authenticated%}{%endif%} + + + {%endfor%} + + + + + History + +
    + + + + + + + + + + {%for h in patch.history %} + + + + + + {%endfor%} + +
    WhenWhoWhat
    {{h.date}}{{h.by_string}}{{h.what}}
    +
    + {%if user.is_authenticated%} + {{is_subscribed|yesno:"Unsubscribe from patch update emails,Subscribe to patch update emails"}} + {%endif%} + + + + diff --git a/pgcommitfest/commitfest/templates/patch_tr_cfbot.inc b/pgcommitfest/commitfest/templates/patch_tr_cfbot.inc index 8bd496b2..af43a350 100644 --- a/pgcommitfest/commitfest/templates/patch_tr_cfbot.inc +++ b/pgcommitfest/commitfest/templates/patch_tr_cfbot.inc @@ -1,6 +1,6 @@ {%load commitfest%} -CFBot Status + CFBot Status {%if not cfbot_branch %} Not processed diff --git a/pgcommitfest/commitfest/templates/patch_workflow.inc b/pgcommitfest/commitfest/templates/patch_workflow.inc index de907b10..29d1ba6f 100644 --- a/pgcommitfest/commitfest/templates/patch_workflow.inc +++ b/pgcommitfest/commitfest/templates/patch_workflow.inc @@ -13,71 +13,71 @@ - - {%if poc.status == poc.STATUS_AUTHOR %} - Reviewer - {%endif%} - {%if poc.status == poc.STATUS_REVIEW or poc.status == poc.STATUS_COMMITTER %} - Author - {%endif%} - {%if poc.status == poc.STATUS_REVIEW %} - Committer - {%endif%} - + + {%if poc.status == poc.STATUS_AUTHOR %} + Reviewer + {%endif%} + {%if poc.status == poc.STATUS_REVIEW or poc.status == poc.STATUS_COMMITTER %} + Author + {%endif%} + {%if poc.status == poc.STATUS_REVIEW %} + Committer + {%endif%} + - - Comment - Review - + + Comment + Review + - - {%if is_committer or is_author %} - {%if is_committer %} - Commit - Reject - {%endif%} - {%if is_author %} - Withdraw + + {%if is_committer or is_author %} + {%if is_committer %} + Commit + Reject + {%endif%} + {%if is_author %} + Withdraw + {%endif%} + {%else%} + No Actions Available {%endif%} - {%else%} - No Actions Available - {%endif%} - + - - {%if not cf.isfuture and workflow.future %} - {%if True %} - - {{workflow.future.name}} + + {%if not cf.isfuture and workflow.future %} + {%if True %} + + {{workflow.future.name}} + {%endif%} {%endif%} - {%endif%} - {%if not cf.isopen and workflow.open %} - {%if is_committer %} - - {{workflow.open.name}} + {%if not cf.isopen and workflow.open %} + {%if is_committer %} + + {{workflow.open.name}} + {%endif%} {%endif%} - {%endif%} - {%if not cf.isinprogress and workflow.progress %} - {%if is_committer %} - - {{workflow.progress.name}} + {%if not cf.isinprogress and workflow.progress %} + {%if is_committer %} + + {{workflow.progress.name}} + {%endif%} {%endif%} - {%endif%} - {%if not cf.isparked and workflow.parked %} - {%if is_author %} - - {{workflow.parked.name}} + {%if not cf.isparked and workflow.parked %} + {%if is_author %} + + {{workflow.parked.name}} + {%endif%} {%endif%} - {%endif%} - + {%endif%} @@ -92,57 +92,57 @@ - - {%for author in authors %} -
    {{author.first_name}} {{author.last_name}}
    - {%endfor%} - {%if is_author or is_committer %} - Edit Authors - {%endif%} - - - - {%for reviewer in reviewers %} -
    {{reviewer.first_name}} {{reviewer.last_name}}
    - {%endfor%} - {%if user.is_authenticated %} -
    {{is_reviewer|yesno:"Remove Self,Become Reviewer"}}
    - {%endif%} - - - - {%if patch.committer%}
    {{patch.committer.fullname}}
    {%endif%} - {%if is_committer%}
    {{is_this_committer|yesno:"Unclaim,Claim for Self"}}
    {%endif%} - - - -
    - {{poc.commitfest.statusstring}} - {{poc.statusstring}} - - Version: - - {%if patch.targetversion%}{{patch.targetversion}}{%else%}N/A{%endif%} - {%if user.is_authenticated %} - - {%endif%} - {%if user.is_authenticated %} - Updates: {{is_subscribed|yesno:"Unsubscribe,Subscribe"}} + + {%for author in authors %} +
    {{author.first_name}} {{author.last_name}}
    + {%endfor%} + {%if is_author or is_committer %} + Edit Authors {%endif%} -
    -
    - - Topic: - - {{ patch.topic }} + + + + {%for reviewer in reviewers %} +
    {{reviewer.first_name}} {{reviewer.last_name}}
    + {%endfor%} {%if user.is_authenticated %} - +
    {{is_reviewer|yesno:"Remove Self,Become Reviewer"}}
    {%endif%} -
    -
    - Last Modified: {{patch.modified}} ({% cfwhen patch.modified %}) -
    - + + + + {%if patch.committer%}
    {{patch.committer.fullname}}
    {%endif%} + {%if is_committer%}
    {{is_this_committer|yesno:"Unclaim,Claim for Self"}}
    {%endif%} + + + +
    + {{poc.commitfest.statusstring}} + {{poc.statusstring}} + + Version: + + {%if patch.targetversion%}{{patch.targetversion}}{%else%}N/A{%endif%} + {%if user.is_authenticated %} + + {%endif%} + {%if user.is_authenticated %} + Updates: {{is_subscribed|yesno:"Unsubscribe,Subscribe"}} + {%endif%} +
    +
    + + Topic: + + {{ patch.topic }} + {%if user.is_authenticated %} + + {%endif%} +
    +
    + Last Modified: {{patch.modified}} ({% cfwhen patch.modified %}) +
    + diff --git a/pgcommitfest/commitfest/templates/workflow.html b/pgcommitfest/commitfest/templates/workflow.html index 618b43af..7335e310 100644 --- a/pgcommitfest/commitfest/templates/workflow.html +++ b/pgcommitfest/commitfest/templates/workflow.html @@ -1,234 +1,234 @@ {%extends "base.html"%} {%block contents%} -

    Key Concepts

    -

    The Commitfest Workflow

    -

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

    -

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

      -
    • Bugs - annual, all bug reports
      • -
    • Drafts - annual, drafts for new features
      • -
    • Next - scheduled, proposed new features
      • -
    • Ongoing - scheduled, review and commit new features
      • -
    -There are three active categories of patch status covering the four statuses. -
      -
    • Author - the author needs to make changes
      • -
    • Reviewer - the patch needs guidance or a review
      • -
    • Committer - the patch is ready to be committed
      • -
    -And there are three preferred categories of patch status for when a patch has -been resolved, either by being committed or not. -
      -
    • Committed - a committer has applied the patches to the git repo
      • -
    • Withdrawn - the author has withdrawn the patch from consideration
      • -
    • Rejected - a committer has decided that the patch should not be applied
      • -
    -(See notes below for all available statuses and their intended usage.) -

    -

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

    -

    - 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 - with the committer patch status category already set. -

    -

    - 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 ongoing bin only exists during an active commitfest period, during which - no new patches should be added and the community reviewers and committers are - encouraged to focus on reviews and commits. At the end of the scheduled period - the bin is closed. This is only bin that is not present at all times. -

    -

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

    -

    Patches on Commitfests (PoC)

    -

    Overview

    -

    - PoCs are the central concept of the commitfest workflow. When you are viewing - a Patch you are always implicitly acting within its primary PoC. The key - property of a PoC is its state, the possible values of which are: active, - inactive, moved, and abandoned. Each is described in detail below. -

    -

    Active

    -

    - A Patch is active if, in its primary PoC, it is in one of the following states: -

      -
    • Waiting on Author (Author)
      • -
    • Review Needed (Reviewer)
      • -
    • Ready for Committer (Committer)
      • -
    - 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, in its primary PoC, it is in one of the following states: -

      -
    • Committed
      • -
    • Withdrawn
      • -
    • Rejected
      • -
    • Returned With Feedback*
      • -
    -

    -

    - 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 PoC 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 future 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. -

    -

    Moved

    -

    - Moved PoCs are a side-effect of having commitfest, 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 - flow-of-time action. -

    -

    Is Abandoned

    -

    - This check returns true if the PoC state is in the set of active states - but the commitfest is closed. Conceptually, this is the same as - withdrawn, but through inaction. -

    -

    Patches

    -

    Overview

    -

    - Patches are the basic unit of work in the workflow. They are moved around - between various commitfest bins during their lifetime to reflect how they - interact with the focus of the community during each time period. - This movement, combined with the fact that a patch may only have a single - status, means that only one of the patch's PoCs can have a state that isn't - "Moved". While a patch's status is derived from its primary PoC, everything - else is attached to the patch itself. -

    -

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

    -

    Threads

    -

    - The actual content of a patch does not existing within the commitfest application - (more-or-less); instead, the patch file submission and review process is all done - on the -hackers mailing list. Threads are the means by which the commitfest - is made aware of these files and can incorporate information pertaining to them - into its reporting. -

    -

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

    -

    Ongoing

    -

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

    -

    Future

    -

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

    -

    Bugs

    -

    - 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"). -

    -

    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 v18 feature freeze, the existing - "Draft v18" commitfest is closed and a new "Draft v19" 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, bugs and ongoing commitfests are closed (future 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. -

    -

    History

    -

    - Textual event log for a patch. -

    -

    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 PoCs are tested. -

    -

    Administrative Actions

    -

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

    +

    Key Concepts

    +

    The Commitfest Workflow

    +

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

    +

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

      +
    • Bugs - annual, all bug reports
      • +
    • Drafts - annual, drafts for new features
      • +
    • Next - scheduled, proposed new features
      • +
    • Ongoing - scheduled, review and commit new features
      • +
    + There are three active categories of patch status covering the four statuses. +
      +
    • Author - the author needs to make changes
      • +
    • Reviewer - the patch needs guidance or a review
      • +
    • Committer - the patch is ready to be committed
      • +
    + And there are three preferred categories of patch status for when a patch has + been resolved, either by being committed or not. +
      +
    • Committed - a committer has applied the patches to the git repo
      • +
    • Withdrawn - the author has withdrawn the patch from consideration
      • +
    • Rejected - a committer has decided that the patch should not be applied
      • +
    + (See notes below for all available statuses and their intended usage.) +

    +

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

    +

    + 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 + with the committer patch status category already set. +

    +

    + 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 ongoing bin only exists during an active commitfest period, during which + no new patches should be added and the community reviewers and committers are + encouraged to focus on reviews and commits. At the end of the scheduled period + the bin is closed. This is only bin that is not present at all times. +

    +

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

    +

    Patches on Commitfests (PoC)

    +

    Overview

    +

    + PoCs are the central concept of the commitfest workflow. When you are viewing + a Patch you are always implicitly acting within its primary PoC. The key + property of a PoC is its state, the possible values of which are: active, + inactive, moved, and abandoned. Each is described in detail below. +

    +

    Active

    +

    + A Patch is active if, in its primary PoC, it is in one of the following states: +

      +
    • Waiting on Author (Author)
      • +
    • Review Needed (Reviewer)
      • +
    • Ready for Committer (Committer)
      • +
    + 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, in its primary PoC, it is in one of the following states: +

      +
    • Committed
      • +
    • Withdrawn
      • +
    • Rejected
      • +
    • Returned With Feedback*
      • +
    +

    +

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

    +

    Moved

    +

    + Moved PoCs are a side-effect of having commitfest, 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 + flow-of-time action. +

    +

    Is Abandoned

    +

    + This check returns true if the PoC state is in the set of active states + but the commitfest is closed. Conceptually, this is the same as + withdrawn, but through inaction. +

    +

    Patches

    +

    Overview

    +

    + Patches are the basic unit of work in the workflow. They are moved around + between various commitfest bins during their lifetime to reflect how they + interact with the focus of the community during each time period. + This movement, combined with the fact that a patch may only have a single + status, means that only one of the patch's PoCs can have a state that isn't + "Moved". While a patch's status is derived from its primary PoC, everything + else is attached to the patch itself. +

    +

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

    +

    Threads

    +

    + The actual content of a patch does not existing within the commitfest application + (more-or-less); instead, the patch file submission and review process is all done + on the -hackers mailing list. Threads are the means by which the commitfest + is made aware of these files and can incorporate information pertaining to them + into its reporting. +

    +

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

    +

    Ongoing

    +

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

    +

    Future

    +

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

    +

    Bugs

    +

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

    +

    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 v18 feature freeze, the existing + "Draft v18" commitfest is closed and a new "Draft v19" 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, bugs and ongoing commitfests are closed (future 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. +

    +

    History

    +

    + Textual event log for a patch. +

    +

    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 PoCs are tested. +

    +

    Administrative Actions

    +

    + 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/views.py b/pgcommitfest/commitfest/views.py index f5af7154..7a041bf7 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -680,12 +680,16 @@ def patch(request, patchid): # XXX: this creates a session, so find a smarter way. Probably handle # it in the callback and just ask the user then? if request.user.is_authenticated: - is_committer, is_this_committer, all_committers = Workflow.isCommitter(request.user, patch) + is_committer, is_this_committer, all_committers = Workflow.isCommitter( + request.user, patch + ) is_author = request.user in patch.authors.all() is_reviewer = request.user in patch.reviewers.all() is_subscribed = patch.subscribers.filter(id=request.user.id).exists() else: - is_committer, is_this_committer, all_committers = Workflow.isCommitter(None, None) + is_committer, is_this_committer, all_committers = Workflow.isCommitter( + None, None + ) is_author = False is_reviewer = False is_subscribed = False @@ -719,7 +723,7 @@ def patch(request, patchid): "future": Workflow.future_cf(), "progress": Workflow.inprogress_cf(), "parked": Workflow.parked_cf(), - } + }, }, ) @@ -989,7 +993,7 @@ def status(request, patchid, status): poc = Workflow.get_poc_for_patchid_or_404(patchid) try: - if (Workflow.updatePOCStatus(poc, newstatus, request.user)): + if Workflow.updatePOCStatus(poc, newstatus, request.user): messages.info(request, "Updated status to %s" % poc.statusstring) except Exception as e: messages.error(request, "Failed to update status of patch: {}".format(e)) @@ -1003,7 +1007,7 @@ def close(request, patchid, status): poc = Workflow.get_poc_for_patchid_or_404(patchid) committer = None - # Inactive statuses only, except moved (next), which is handled by /transition + # Inactive statuses only, except moved (next), which is handled by /transition if status == "reject": newstatus = PatchOnCommitFest.STATUS_REJECTED elif status == "withdrawn": @@ -1017,10 +1021,10 @@ def close(request, patchid, status): raise Exception("Can't happen") try: - if (committer): + if committer: Workflow.setCommitter(poc, committer, request.user) messages.info(request, "Committed by %s" % committer.user) - if (Workflow.updatePOCStatus(poc, newstatus, request.user)): + if Workflow.updatePOCStatus(poc, newstatus, request.user): messages.info(request, "Closed. Updated status to %s" % poc.statusstring) except Exception as e: messages.error(request, "Failed to close patch: {}".format(e)) @@ -1034,24 +1038,37 @@ 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))) + 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.") + 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))) + 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) + 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)) @@ -1107,7 +1124,7 @@ def committer(request, patchid, status): # We could ignore a no-op become...but we expect the application to # prevent such things on this particular interface. - if (not Workflow.setCommitter(poc, new_committer, request.user)): + if not Workflow.setCommitter(poc, new_committer, request.user): raise Exception("Not expecting a no-op: toggling committer") return HttpResponseRedirect("../../") diff --git a/pgcommitfest/urls.py b/pgcommitfest/urls.py index 6d92ed96..b81f2c11 100644 --- a/pgcommitfest/urls.py +++ b/pgcommitfest/urls.py @@ -27,9 +27,7 @@ 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)/$", 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), From 310a32020cdc06e39c3aa4ff73d1810053cdf23e Mon Sep 17 00:00:00 2001 From: "David G. Johnston" Date: Mon, 7 Apr 2025 13:35:43 -0700 Subject: [PATCH 12/12] 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. --- .../0013_no_patches_in_future_cfs.py | 57 ++++++++++++++ pgcommitfest/commitfest/models.py | 24 +++--- .../templates/patch_administrative.inc | 7 -- .../commitfest/templates/patch_workflow.inc | 58 +++++++------- .../commitfest/templates/workflow.html | 76 +++++++++++-------- pgcommitfest/commitfest/views.py | 1 - 6 files changed, 141 insertions(+), 82 deletions(-) create mode 100644 pgcommitfest/commitfest/migrations/0013_no_patches_in_future_cfs.py 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 00000000..75ed886f --- /dev/null +++ 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 index e3dd279a..8548f61c 100644 --- 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 index adcc0594..ca3ff0a0 100644 --- a/pgcommitfest/commitfest/templates/patch_administrative.inc +++ b/pgcommitfest/commitfest/templates/patch_administrative.inc @@ -24,13 +24,6 @@
  • Commit
    • - {%if not cf.isfuture and workflow.future %} -
  • - - {{workflow.future.name}} -
    • - {%endif%} {%if not cf.isopen and workflow.open %}
  • {%if is_committer or is_author %} - {%if is_committer %} - Commit + {%if is_committer and poc.is_open %} + {%if not poc.isparked and poc.is_committer %} + Commit + {%endif%} Reject {%endif%} {%if is_author %} Withdraw {%endif%} + {%if is_committer and not poc.is_open %} + + {{poc.is_committed|yesno:"Revert,Re-Open"}} + + {%endif%} {%else%} No Actions Available {%endif%} - {%if not cf.isfuture and workflow.future %} - {%if True %} - - {{workflow.future.name}} + {%if poc.is_open %} + {%if not cf.isopen and workflow.open %} + {%if is_author or is_committer %} + + {{workflow.open.name}} + {%endif%} {%endif%} - {%endif%} - {%if not cf.isopen and workflow.open %} - {%if is_committer %} - - {{workflow.open.name}} + {%if not cf.isinprogress and workflow.progress %} + {%if is_committer %} + + {{workflow.progress.name}} + {%endif%} {%endif%} - {%endif%} - {%if not cf.isinprogress and workflow.progress %} - {%if is_committer %} - - {{workflow.progress.name}} - {%endif%} - {%endif%} - - {%if not cf.isparked and workflow.parked %} - {%if is_author %} - - {{workflow.parked.name}} + {%if not cf.isparked and workflow.parked %} + {%if is_author %} + + {{workflow.parked.name}} + {%endif%} {%endif%} + {%else%} + Cannot move closed patches. {%endif%} - diff --git a/pgcommitfest/commitfest/templates/workflow.html b/pgcommitfest/commitfest/templates/workflow.html index 7335e310..eff223f3 100644 --- a/pgcommitfest/commitfest/templates/workflow.html +++ b/pgcommitfest/commitfest/templates/workflow.html @@ -3,9 +3,14 @@

    Key Concepts

    The Commitfest Workflow

    - 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. @@ -13,11 +18,10 @@

    The Commitfest Workflow

    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.

      -
    • Bugs - annual, all bug reports
    • Drafts - annual, drafts for new features
      • -
    • Next - scheduled, proposed new features
      • +
    • Open - scheduled, proposed new features
    • Ongoing - scheduled, review and commit new features
    There are three active categories of patch status covering the four statuses. @@ -35,23 +39,17 @@

    The Commitfest Workflow

    (See notes below for all available statuses and their intended usage.)

    -

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

    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.

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

    The ongoing bin only exists during an active commitfest period, during which @@ -60,11 +58,10 @@

    The Commitfest Workflow

    the bin is closed. This is only bin that is not present at all times.

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

    Patches on Commitfests (PoC)

    Overview

    @@ -112,7 +109,7 @@

    Inactive

    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.

    *Returned With Feedback complements rejected in that the implication of rejected @@ -123,9 +120,10 @@

    Inactive

    leaves reject as the fallback non-commit option and makes returned a deprecated administrator-only option.

    +

    Special PoC Situations

    Moved

    - 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 @@

    Is Abandoned

    but the commitfest is closed. Conceptually, this is the same as withdrawn, but through inaction.

    +

    Reverted Patches

    +

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

    Patches

    Overview

    @@ -183,17 +189,21 @@

    Ongoing

    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.

    -

    Future

    +

    Open

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

    -

    Bugs

    +

    Future

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

    Drafts

    @@ -210,7 +220,7 @@

    Drafts

    Closed

    - 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 index 7a041bf7..3799a496 100644 --- 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(), },