From a3a7ecd7e58680bc717b08c326d2bb7fc760ae68 Mon Sep 17 00:00:00 2001 From: "David G. Johnston" Date: Mon, 14 Apr 2025 08:39:50 -0700 Subject: [PATCH 1/2] Add workflow information page to base.html nav bar Introductory material to understand how the workflow operates and its terminology and concepts in the software development domain compared to generic project managed. Includes a schedule. Also add a reference guide. --- media/commitfest/css/commitfest.css | 83 +++++++ pgcommitfest/commitfest/templates/base.html | 3 + .../templates/workflow-reference.html | 166 +++++++++++++ .../commitfest/templates/workflow.html | 222 ++++++++++++++++++ pgcommitfest/commitfest/views.py | 20 ++ pgcommitfest/urls.py | 2 + 6 files changed, 496 insertions(+) create mode 100644 pgcommitfest/commitfest/templates/workflow-reference.html create mode 100644 pgcommitfest/commitfest/templates/workflow.html diff --git a/media/commitfest/css/commitfest.css b/media/commitfest/css/commitfest.css index 72cae0e..b6f2d61 100644 --- a/media/commitfest/css/commitfest.css +++ b/media/commitfest/css/commitfest.css @@ -91,3 +91,86 @@ div.form-group div.controls input.threadpick-input { .search-bar { display: inline-block; } + +/* Header styling */ +#workflow-transitions-header { + font-size: 20px; + font-weight: bold; + margin-top: 20px; + color: #333; + border-bottom: 2px solid #ddd; + padding-bottom: 5px; +} + +/* Table styling */ +#workflow-transitions-table { + width: 100%; + max-width: 500px; /* Updated max-width */ + border-collapse: collapse; + margin: 20px 0; /* Remove centering */ + font-size: 16px; + text-align: left; +} + +#workflow-transitions-table th, +#workflow-transitions-table td { + border: 1px solid #ddd; + padding: 8px; + width: 25%; /* Ensure all columns have the same width */ + text-align: center; /* Center-align cell labels */ +} + +#workflow-transitions-table th { + background-color: #f4f4f4; + color: #333; + font-weight: bold; + text-align: center; +} + +#workflow-transitions-table tr:nth-child(even) { + background-color: #f9f9f9; +} + +#workflow-transitions-table tr:hover { + background-color: #f1f1f1; +} + +#workflow-schedule-table { + width: 100%; + max-width: 800px; + border-collapse: collapse; + margin: 20px 0; /* Remove centering */ + font-size: 16px; + text-align: left; +} + +#workflow-schedule-table th, +#workflow-schedule-table td { + border: 1px solid #ddd; + padding: 8px; + text-align: center; /* Center-align cell labels */ +} + +#workflow-schedule-table td { + width: 15%; /* First four columns */ +} + +#workflow-schedule-table td:nth-child(5), +#workflow-schedule-table td:nth-child(6) { + width: 20%; /* Last two columns */ +} + +#workflow-schedule-table th { + background-color: #f4f4f4; + color: #333; + font-weight: bold; + text-align: center; +} + +#workflow-schedule-table tr:nth-child(even) { + background-color: #f9f9f9; +} + +#workflow-schedule-table tr:hover { + background-color: #f1f1f1; +} diff --git a/pgcommitfest/commitfest/templates/base.html b/pgcommitfest/commitfest/templates/base.html index c70a7f7..b606d66 100644 --- a/pgcommitfest/commitfest/templates/base.html +++ b/pgcommitfest/commitfest/templates/base.html @@ -30,6 +30,9 @@ Log in {%endif%} +
  • + Workflow +
    • {%if header_activity%}
  • {{header_activity}}
    • {%endif%} diff --git a/pgcommitfest/commitfest/templates/workflow-reference.html b/pgcommitfest/commitfest/templates/workflow-reference.html new file mode 100644 index 0000000..bd05522 --- /dev/null +++ b/pgcommitfest/commitfest/templates/workflow-reference.html @@ -0,0 +1,166 @@ +{%extends "base.html"%} +{%block contents%} +

    Overview

    +

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

    +

    CFBot

    +

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

    +

    Patches

    +

    Overview

    +

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

    +

    Authors, Reviewers, and Committers

    +

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

    +

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

    +

    Active

    +

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

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

    +

    Inactive

    +

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

    +

    +

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

    +

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

    +

    + A Rejected patch has the same effect as Withdrawn but communicates that the + community, not the author, made the status change. This should only be used + when it is when the author is unable or unwilling to withdraw the patch or park + it for rework. +

    +

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

    +

    Threads, Messages, and Patch Sets

    +

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

    +

    Commitfests

    +

    Overview

    +

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

    +

    In Progress

    +

    + An Active (see Workflow above) period where no new features should be added + and the goal is to get as many review"patches committed as possible. +

    +

    Open

    +

    + Patches ready for final review and commit, according to the author, are placed + in the current open commitfest. Upon the scheduled start date it is manually + updated to be an in progress commitfest, at which point no new patches should be + added. +

    +

    Future

    +

    + The PostgreSQL project works on a schedule release cycle. At the beginning + of each cycle the planned commitfest periods are decided and communicated to + the community via the creation of future commitfests. While classified as + future these commitfests are not permitted to associated with any patches. + Their classification is changed to open as each prior in progress commitfest + closes. +

    +

    Drafts

    +

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

    +

    Closed

    +

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

    +

    Special Patch Situations

    +

    Moved

    +

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

    +

    Is Ignored

    +

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

    +

    History

    +

    + Textual event log for a patch. +

    +

    Administrative Actions (Admin)

    +

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

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

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

    +

    Key Concepts

    +

    The Commitfest Workflow

    +

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

    +

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

    +

    + There are three active categories of patch status: +

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

    +

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

    Commitfest Status Transitions

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

    +

    Workflow Schedule

    +

    (See also the postgresql.org versioning policy.)

    +

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

    +

    +{%endblock%} diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index 40616ff..1611f37 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -66,6 +66,26 @@ def home(request): ) +def workflow(request): + return render( + request, + "workflow.html", + { + "title": "Commitfest Workflow Overview", + }, + ) + + +def workflow_reference(request): + return render( + request, + "workflow-reference.html", + { + "title": "Commitfest Workflow Reference Guide", + }, + ) + + @login_required def me(request): cfs = list(CommitFest.objects.filter(status=CommitFest.STATUS_INPROGRESS)) diff --git a/pgcommitfest/urls.py b/pgcommitfest/urls.py index a67f55f..381a523 100644 --- a/pgcommitfest/urls.py +++ b/pgcommitfest/urls.py @@ -15,6 +15,8 @@ urlpatterns = [ re_path(r"^$", views.home), + re_path(r"^workflow/$", views.workflow), + re_path(r"^workflow-reference/$", views.workflow_reference), re_path(r"^me/$", views.me), re_path(r"^archive/$", views.archive), re_path(r"^activity(?P\.rss)?/", views.activity), From 33cf2086511b19e43833531617f64e9b713a79d9 Mon Sep 17 00:00:00 2001 From: "David G. Johnston" Date: Mon, 14 Apr 2025 14:24:01 -0700 Subject: [PATCH 2/2] Introduce Workflow, Drafts, and API The Commitfest Workflow is a formalization and extension of the existing commitfest-only protocol. There is now some in-app documentation and the idea of moving patches to the "next CF" has begun being deprecated. Instead the UI provides the user valid choices on where to transition patches, which, with the introduction of the Drafts commitfest, is basically one or the other. Committers gain the ability to transition patches into In Progress if desired. Patches within In Progress commitfests can be moved into either Open or Drafts. Begin enforcement of a maximum of one commifest in each of Parked, Open, and In Progress. The workflow defines the drafts commitfest as lasting for one year matching up with the major release process. Namely, once the commifest leading up to feature freeze is In Progress a new Drafts commitfest should be created. With the workflow changes it is necessary for CFBot to be made aware of the specific commitfest ids that are active. Provide a JSON API for this purpose to begin weaning CFBot off scraping html. The workflow in-app documentation includes a month-based schedule showing which commifests are active, and their names, in each month (among other related details). Begin enforcing the existing rule that future commitfests cannot have patches via triggers. Future work on the administrative and UX aspects of the CFApp likely will do away with future commitfests altogether. Thoughts on the topic are welcome in the GitHub issues. --- pgcommitfest/commitfest/apiv1.py | 42 +++ .../commitfest/fixtures/auth_data.json | 36 +++ .../commitfest/fixtures/commitfest_data.json | 67 +++++ .../0011_add_status_related_constraints.py | 68 +++++ .../migrations/0012_add_parked_cf_status.py | 23 ++ .../0013_no_patches_in_future_cfs.py | 62 ++++ pgcommitfest/commitfest/models.py | 264 +++++++++++++++++- pgcommitfest/commitfest/templates/me.html | 11 +- .../commitfest/templates/patch_commands.inc | 23 +- pgcommitfest/commitfest/views.py | 140 ++++------ pgcommitfest/urls.py | 7 +- 11 files changed, 653 insertions(+), 90 deletions(-) create mode 100644 pgcommitfest/commitfest/apiv1.py 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/migrations/0013_no_patches_in_future_cfs.py diff --git a/pgcommitfest/commitfest/apiv1.py b/pgcommitfest/commitfest/apiv1.py new file mode 100644 index 0000000..c7f972a --- /dev/null +++ b/pgcommitfest/commitfest/apiv1.py @@ -0,0 +1,42 @@ +from django.http import ( + HttpResponse, +) + +import json +from datetime import datetime + +from .models import ( + Workflow, +) + + +def datetime_serializer(obj): + if isinstance(obj, datetime): + return obj.strftime("%Y-%m-%dT%H:%M:%S%z") + raise TypeError("Type not serializable") + + +def apiResponse(request, payload, status=200, content_type="application/json"): + response = HttpResponse( + json.dumps(payload, default=datetime_serializer), status=status + ) + response["Content-Type"] = content_type + response["Access-Control-Allow-Origin"] = "*" + return response + + +def optional_as_json(obj): + if obj is None: + return None + return obj.json() + + +def active_commitfests(request): + payload = { + "workflow": { + "open": optional_as_json(Workflow.open_cf()), + "inprogress": optional_as_json(Workflow.inprogress_cf()), + "parked": optional_as_json(Workflow.parked_cf()), + }, + } + return apiResponse(request, payload) diff --git a/pgcommitfest/commitfest/fixtures/auth_data.json b/pgcommitfest/commitfest/fixtures/auth_data.json index 88d8c70..9a6e2f0 100644 --- a/pgcommitfest/commitfest/fixtures/auth_data.json +++ b/pgcommitfest/commitfest/fixtures/auth_data.json @@ -88,5 +88,41 @@ "groups": [], "user_permissions": [] } +}, +{ + "model": "auth.user", + "pk": 6, + "fields": { + "password": "", + "last_login": null, + "is_superuser": false, + "username": "prolific-author", + "first_name": "Prolific", + "last_name": "Author", + "email": "", + "is_staff": false, + "is_active": true, + "date_joined": "2025-01-01T00:00:00", + "groups": [], + "user_permissions": [] + } +}, +{ + "model": "auth.user", + "pk": 7, + "fields": { + "password": "", + "last_login": null, + "is_superuser": false, + "username": "prolific-reviewer", + "first_name": "Prolific", + "last_name": "Reviewer", + "email": "", + "is_staff": false, + "is_active": true, + "date_joined": "2025-01-01T00:00:00", + "groups": [], + "user_permissions": [] + } } ] diff --git a/pgcommitfest/commitfest/fixtures/commitfest_data.json b/pgcommitfest/commitfest/fixtures/commitfest_data.json index 6e5b32f..cd54202 100644 --- a/pgcommitfest/commitfest/fixtures/commitfest_data.json +++ b/pgcommitfest/commitfest/fixtures/commitfest_data.json @@ -60,6 +60,16 @@ "enddate": "2025-05-31" } }, +{ + "model": "commitfest.commitfest", + "pk": 5, + "fields": { + "name": "Drafts PG19", + "status": 5, + "startdate": "2024-09-01", + "enddate": "2025-08-31" + } +}, { "model": "commitfest.topic", "pk": 1, @@ -237,6 +247,25 @@ ] } }, +{ + "model": "commitfest.patch", + "pk": 8, + "fields": { + "name": "Test DGJ Multi-Author and Reviewer", + "topic": 3, + "wikilink": "", + "gitlink": "", + "targetversion": 1, + "committer": 4, + "created": "2025-02-01T00:00", + "modified": "2025-02-01T00:00", + "lastmail": "2025-02-01T00:00", + "authors": [6,3], + "reviewers": [7,1], + "subscribers": [], + "mailthread_set": [8] + } +}, { "model": "commitfest.patchoncommitfest", "pk": 1, @@ -325,6 +354,17 @@ "status": 1 } }, +{ + "model": "commitfest.patchoncommitfest", + "pk": 9, + "fields": { + "patch": 8, + "commitfest": 5, + "enterdate": "2025-02-01T00:00:00", + "leavedate": null, + "status": 1 + } +}, { "model": "commitfest.patchhistory", "pk": 1, @@ -632,6 +672,33 @@ "latestmsgid": "example@message-31" } }, +{ + "model": "commitfest.mailthread", + "pk": 8, + "fields": { + "messageid": "dgj-example@message-08", + "subject": "Test DGJ Multi-Author and Reviewer", + "firstmessage": "2025-02-01T00:00", + "firstauthor": "test@test.com", + "latestmessage": "2025-02-01T00:00", + "latestauthor": "test@test.com", + "latestsubject": "Test DGJ Multi-Author and Reviewer", + "latestmsgid": "dgj-example@message-08" + } +}, +{ + "model": "commitfest.mailthreadattachment", + "pk": 8, + "fields": { + "messageid": "dgj-example@message-08", + "attachmentid": 1, + "filename": "v1-0001-content.patch", + "date": "2025-02-01T00:00", + "author": "test@test.com", + "ispatch": true, + "mailthread_id": 8 + } +}, { "model": "commitfest.patchstatus", "pk": 1, diff --git a/pgcommitfest/commitfest/migrations/0011_add_status_related_constraints.py b/pgcommitfest/commitfest/migrations/0011_add_status_related_constraints.py new file mode 100644 index 0000000..17a8866 --- /dev/null +++ b/pgcommitfest/commitfest/migrations/0011_add_status_related_constraints.py @@ -0,0 +1,68 @@ +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("commitfest", "0010_add_failing_since_column"), + ] + operations = [ + migrations.RunSQL( + """ +CREATE UNIQUE INDEX cf_enforce_maxoneopen_idx +ON commitfest_commitfest (status) +WHERE status not in (1,4); +""", + reverse_sql=""" +DROP INDEX IF EXISTS cf_enforce_maxoneopen_idx; +""", + ), + migrations.RunSQL( + """ +CREATE UNIQUE INDEX poc_enforce_maxoneoutcome_idx +ON commitfest_patchoncommitfest (patch_id) +WHERE status not in (5); +""", + reverse_sql=""" +DROP INDEX IF EXISTS poc_enforce_maxoneoutcome_idx; +""", + ), + migrations.RunSQL( + """ +ALTER TABLE commitfest_patchoncommitfest +ADD CONSTRAINT status_and_leavedate_correlation +CHECK ((status IN (4,5,6,7,8)) = (leavedate IS NOT NULL)); +""", + reverse_sql=""" +ALTER TABLE commitfest_patchoncommitfest +DROP CONSTRAINT IF EXISTS status_and_leavedate_correlation; +""", + ), + migrations.RunSQL( + """ +COMMENT ON COLUMN commitfest_patchoncommitfest.leavedate IS +$$A leave date is recorded in two situations, both of which +means this particular patch-cf combination became inactive +on the corresponding date. For status 5 the patch was moved +to some other cf. For 4,6,7, and 8, this was the final cf. +$$ +""", + reverse_sql=""" +COMMENT ON COLUMN commitfest_patchoncommitfest.leavedate IS NULL; +""", + ), + migrations.RunSQL( + """ +COMMENT ON TABLE commitfest_patchoncommitfest IS +$$This is a re-entrant table: patches may become associated +with a given cf multiple times, resetting the entrydate and clearing +the leavedate each time. Non-final statuses never have a leavedate +while final statuses always do. The final status of 5 (moved) is +special in that all but one of the rows a patch has in this table +must have it as the status. +$$ +""", + reverse_sql=""" +COMMENT ON TABLE commitfest_patchoncommitfest IS NULL; +""", + ), + ] diff --git a/pgcommitfest/commitfest/migrations/0012_add_parked_cf_status.py b/pgcommitfest/commitfest/migrations/0012_add_parked_cf_status.py new file mode 100644 index 0000000..9075946 --- /dev/null +++ b/pgcommitfest/commitfest/migrations/0012_add_parked_cf_status.py @@ -0,0 +1,23 @@ +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("commitfest", "0011_add_status_related_constraints"), + ] + operations = [ + migrations.AlterField( + model_name="commitfest", + name="status", + field=models.IntegerField( + choices=[ + (1, "Future"), + (2, "Open"), + (3, "In Progress"), + (4, "Closed"), + (5, "Parked"), + ], + default=1, + ), + ) + ] diff --git a/pgcommitfest/commitfest/migrations/0013_no_patches_in_future_cfs.py b/pgcommitfest/commitfest/migrations/0013_no_patches_in_future_cfs.py new file mode 100644 index 0000000..d877ae1 --- /dev/null +++ b/pgcommitfest/commitfest/migrations/0013_no_patches_in_future_cfs.py @@ -0,0 +1,62 @@ +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("commitfest", "0012_add_parked_cf_status"), + ] + operations = [ + migrations.RunSQL( + """ +CREATE FUNCTION assert_poc_not_future_for_poc() +RETURNS TRIGGER AS $$ +DECLARE + cfstatus int; +BEGIN + SELECT status INTO cfstatus + FROM commitfest_commitfest + WHERE id = NEW.commitfest_id; + IF cfstatus = 1 THEN + RAISE EXCEPTION 'Patches cannot exist on future commitfests'; + END IF; + RETURN NEW; +END; +$$ +LANGUAGE plpgsql; + +CREATE FUNCTION assert_poc_not_future_for_cf() +RETURNS trigger AS $$ +BEGIN + -- Trigger checks that we only get called when status is 1 + PERFORM 1 + FROM commitfest_patchoncommitfest + WHERE commitfest_id = NEW.id + LIMIT 1; + IF FOUND THEN + RAISE EXCEPTION 'Cannot change commitfest status to 1, patches exist.'; + END IF; + RETURN NEW; +END; +$$ +LANGUAGE plpgsql; + +CREATE TRIGGER assert_poc_commitfest_is_not_future +BEFORE INSERT OR UPDATE ON commitfest_patchoncommitfest +FOR EACH ROW +EXECUTE FUNCTION assert_poc_not_future_for_poc(); + +CREATE TRIGGER assert_poc_commitfest_is_not_future +-- Newly inserted cfs can't have patches +BEFORE UPDATE ON commitfest_commitfest +FOR EACH ROW +WHEN (NEW.status = 1) +EXECUTE FUNCTION assert_poc_not_future_for_cf(); +""", + reverse_sql=""" +DROP TRIGGER IF EXISTS assert_poc_commitfest_is_not_future ON commitfest_commitfest; +DROP TRIGGER IF EXISTS assert_poc_commitfest_is_not_future ON commitfest_patchoncommitfest; +DROP FUNCTION IF EXISTS assert_poc_not_future_for_poc(); +DROP FUNCTION IF EXISTS assert_poc_not_future_for_cf(); +""", + ), + ] diff --git a/pgcommitfest/commitfest/models.py b/pgcommitfest/commitfest/models.py index fcd9edb..1d1d103 100644 --- a/pgcommitfest/commitfest/models.py +++ b/pgcommitfest/commitfest/models.py @@ -1,5 +1,6 @@ from django.contrib.auth.models import User from django.db import models +from django.db.models import Q from django.shortcuts import get_object_or_404 from datetime import datetime @@ -38,17 +39,20 @@ class CommitFest(models.Model): STATUS_OPEN = 2 STATUS_INPROGRESS = 3 STATUS_CLOSED = 4 + STATUS_PARKED = 5 _STATUS_CHOICES = ( (STATUS_FUTURE, "Future"), (STATUS_OPEN, "Open"), (STATUS_INPROGRESS, "In Progress"), (STATUS_CLOSED, "Closed"), + (STATUS_PARKED, "Drafts"), ) _STATUS_LABELS = ( (STATUS_FUTURE, "default"), (STATUS_OPEN, "info"), (STATUS_INPROGRESS, "success"), (STATUS_CLOSED, "danger"), + (STATUS_PARKED, "default"), ) name = models.CharField(max_length=100, blank=False, null=False, unique=True) status = models.IntegerField( @@ -63,6 +67,8 @@ def statusstring(self): @property def periodstring(self): + # Current Workflow intent is to have all Committfest be time-bounded + # but the information is just contextual so we still permit null if self.startdate and self.enddate: return "{0} - {1}".format(self.startdate, self.enddate) return "" @@ -71,10 +77,31 @@ def periodstring(self): def title(self): return "Commitfest %s" % self.name + @property + def isclosed(self): + return self.status == self.STATUS_CLOSED + @property def isopen(self): return self.status == self.STATUS_OPEN + @property + def isinprogress(self): + return self.status == self.STATUS_INPROGRESS + + @property + def isparked(self): + return self.status == self.STATUS_PARKED + + def json(self): + return { + "id": self.id, + "name": self.name, + "status": self.statusstring, + "startdate": self.startdate.isoformat(), + "enddate": self.enddate.isoformat(), + } + def __str__(self): return self.name @@ -159,11 +186,15 @@ class Patch(models.Model, DiffableModel): } def current_commitfest(self): - return self.commitfests.order_by("-startdate").first() + return self.current_patch_on_commitfest().commitfest def current_patch_on_commitfest(self): - cf = self.current_commitfest() - return get_object_or_404(PatchOnCommitFest, patch=self, commitfest=cf) + # The unique partial index poc_enforce_maxoneoutcome_idx stores the PoC + # No caching here (inside the instance) since the caller should just need + # the PoC once per request. + return get_object_or_404( + PatchOnCommitFest, Q(patch=self) & ~Q(status=PatchOnCommitFest.STATUS_NEXT) + ) # Some accessors @property @@ -273,6 +304,14 @@ def is_closed(self): def is_open(self): return not self.is_closed + @property + def is_committed(self): + return self.status == self.STATUS_COMMITTED + + @property + def needs_committer(self): + return self.status == self.STATUS_COMMITTER + @property def statusstring(self): return [v for k, v in self._STATUS_CHOICES if k == self.status][0] @@ -528,3 +567,222 @@ class CfbotTask(models.Model): status = models.TextField(choices=STATUS_CHOICES, null=False) created = models.DateTimeField(auto_now_add=True) modified = models.DateTimeField(auto_now=True) + + +# Workflow provides access to the elements required to support +# the workflow this application is built for. These elements exist +# independent of what the user is presently seeing on their page. +class Workflow(models.Model): + def get_poc_for_patchid_or_404(patchid): + return get_object_or_404( + Patch.objects.select_related(), pk=patchid + ).current_patch_on_commitfest() + + # At most a single Open CommitFest is allowed and this function returns it. + def open_cf(): + cfs = list(CommitFest.objects.filter(status=CommitFest.STATUS_OPEN)) + return cfs[0] if len(cfs) == 1 else None + + # At most a single In Progress CommitFest is allowed and this function returns it. + def inprogress_cf(): + cfs = list(CommitFest.objects.filter(status=CommitFest.STATUS_INPROGRESS)) + return cfs[0] if len(cfs) == 1 else None + + # At most a single Parked CommitFest is allowed and this function returns it. + def parked_cf(): + cfs = list(CommitFest.objects.filter(status=CommitFest.STATUS_PARKED)) + return cfs[0] if len(cfs) == 1 else None + + # Returns whether the user is a committer in general and for this patch + # since we retrieve all committers in order to answer these questions + # provide that list as a third return value. Passing None for both user + # and patch still returns the list of committers. + def isCommitter(user, patch): + all_committers = Committer.objects.filter(active=True).order_by( + "user__last_name", "user__first_name" + ) + if not user and not patch: + return False, False, all_committers + + committer = [c for c in all_committers if c.user == user] + if len(committer) == 1: + is_committer = True + is_this_committer = committer[0] == patch.committer + else: + is_committer = is_this_committer = False + return is_committer, is_this_committer, all_committers + + def getCommitfest(cfid): + if cfid is None or cfid == "": + return None + try: + int_cfid = int(cfid) + cfs = list(CommitFest.objects.filter(id=int_cfid)) + if len(cfs) == 1: + return cfs[0] + else: + return None + except ValueError: + return None + + # Implements a re-entrant Commitfest POC creation procedure. + # Returns the new POC object. + # Creates history and notifies as a side-effect. + def createNewPOC(patch, commitfest, initial_status, by_user): + poc, created = PatchOnCommitFest.objects.update_or_create( + patch=patch, + commitfest=commitfest, + defaults=dict( + enterdate=datetime.now(), + status=initial_status, + leavedate=None, + ), + ) + poc.patch.set_modified() + poc.patch.save() + poc.save() + + PatchHistory( + patch=poc.patch, + by=by_user, + what="{} in {}".format(poc.statusstring, commitfest.name), + ).save_and_notify() + + return poc + + # The rule surrounding patches is they may only be in one active + # commitfest at a time. The transition function takes a patch + # open in one commitfest and associates it, with the same status, + # in a new commitfest; then makes it inactive in the original. + # Returns the new POC object. + # Creates history and notifies as a side-effect. + def transitionPatch(poc, target_cf, by_user): + Workflow.userCanTransitionPatch(poc, target_cf, by_user) + + existing_status = poc.status + + # History looks cleaner if we've left the existing + # commitfest entry before joining the new one. Plus, + # not allowed to change non-current commitfest status + # and once the new POC is created it becomes current. + + Workflow.updatePOCStatus(poc, PatchOnCommitFest.STATUS_NEXT, by_user) + + new_poc = Workflow.createNewPOC(poc.patch, target_cf, existing_status, by_user) + + return new_poc + + def userCanTransitionPatch(poc, target_cf, user): + # Policies not allowed to be broken by anyone. + + # Prevent changes to non-current commitfest for the patch + # Meaning, status changed to Moved before/during transitioning + # i.e., a concurrent action took place. + if poc.commitfest != poc.patch.current_commitfest(): + raise Exception("Patch commitfest is not its current commitfest.") + + # The UI should be preventing people from trying to perform no-op requests + if poc.commitfest.id == target_cf.id: + raise Exception("Cannot transition to the same commitfest.") + + # This one is arguable but facilitates treating non-open status as final + # A determined staff member can always change the status first. + if poc.is_closed: + raise Exception("Cannot transition a closed patch.") + + # We trust privileged users to make informed choices + if user.is_staff: + return + + if target_cf.isclosed: + raise Exception("Cannot transition to a closed commitfest.") + + if target_cf.isinprogress: + raise Exception("Cannot transition to an in-progress commitfest.") + + # Prevent users from moving closed patches, or moving open ones to + # non-open commitfests. The else clause should be a can't happen. + if poc.is_open and target_cf.isopen: + pass + else: + # Default deny policy basis + raise Exception("Transition not permitted.") + + def userCanChangePOCStatus(poc, new_status, user): + # Policies not allowed to be broken by anyone. + + # Prevent changes to non-current commitfest for the patch + # Meaning, change status to Moved before/during transitioning + if poc.commitfest != poc.patch.current_commitfest(): + raise Exception("Patch commitfest is not its current commitfest.") + + # The UI should be preventing people from trying to perform no-op requests + if poc.status == new_status: + raise Exception("Cannot change to the same status.") + + # We want commits to happen from, usually, In Progress commitfests, + # or Open ones for exempt patches. We accept Future ones too just because + # they do represent a proper, if non-current, Commitfest. + if ( + poc.commitfest.id == CommitFest.STATUS_PARKED + and new_status == PatchOnCommitFest.STATUS_COMMITTED + ): + raise Exception("Cannot change status to committed in a parked commitfest.") + + # We trust privileged users to make informed choices + if user.is_staff: + return + + is_committer, is_this_committer, all_committers = Workflow.isCommitter( + user, poc.patch + ) + + # XXX Not sure if we want to tighten this up to is_this_committer + # with only the is_staff exemption + if new_status == PatchOnCommitFest.STATUS_COMMITTED and not is_committer: + raise Exception("Only a committer can set status to committed.") + + if new_status == PatchOnCommitFest.STATUS_REJECTED and not is_committer: + raise Exception("Only a committer can set status to rejected.") + + if new_status == PatchOnCommitFest.STATUS_RETURNED and not is_committer: + raise Exception("Only a committer can set status to returned.") + + if ( + new_status == PatchOnCommitFest.STATUS_WITHDRAWN + and user not in poc.patch.authors.all() + ): + raise Exception("Only the author can set status to withdrawn.") + + # Prevent users from modifying closed patches + # The else clause should be considered a can't happen + if poc.is_open: + pass + else: + raise Exception("Cannot change status of closed patch.") + + # Update the status of a PoC + # Returns True if the status was changed, False for a same-status no-op. + # Creates history and notifies as a side-effect. + def updatePOCStatus(poc, new_status, by_user): + # XXX Workflow disallows this no-op but not quite ready to enforce it. + if poc.status == new_status: + return False + + Workflow.userCanChangePOCStatus(poc, new_status, by_user) + + poc.status = new_status + poc.leavedate = datetime.now() if not poc.is_open else None + poc.patch.set_modified() + poc.patch.save() + poc.save() + PatchHistory( + patch=poc.patch, + by=by_user, + what="{} in {}".format( + poc.statusstring, + poc.commitfest.name, + ), + ).save_and_notify() + + return True diff --git a/pgcommitfest/commitfest/templates/me.html b/pgcommitfest/commitfest/templates/me.html index fd50f63..f4c801f 100644 --- a/pgcommitfest/commitfest/templates/me.html +++ b/pgcommitfest/commitfest/templates/me.html @@ -2,8 +2,15 @@ {%load commitfest %} {%block contents%} New patch - Current commitfest - Open commitfest + {%if workflow.inprogress%} + In Progress commitfest + {%endif%} + {%if workflow.open%} + Open commitfest + {%endif%} + {%if workflow.parked%} + Draft commitfest + {%endif%}
    diff --git a/pgcommitfest/commitfest/templates/patch_commands.inc b/pgcommitfest/commitfest/templates/patch_commands.inc index 8f09b18..2ec38bc 100644 --- a/pgcommitfest/commitfest/templates/patch_commands.inc +++ b/pgcommitfest/commitfest/templates/patch_commands.inc @@ -21,8 +21,29 @@
  • Rejected
  • Withdrawn
  • Returned with feedback
    • -
  • Move to next CF
  • Committed
    • + + {%if not cf.isopen and workflow.open %} +
  • + + {{workflow.open.name}} +
    • + {%endif%} + {%if not cf.isinprogress and workflow.inprogress and is_committer %} +
  • + + {{workflow.inprogress.name}} +
    • + {%endif%} + {%if not cf.isparked and workflow.parked %} +
  • + + {{workflow.parked.name}} +
    • + {%endif%}
    diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index 1611f37..15a12c0 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -42,6 +42,7 @@ Patch, PatchHistory, PatchOnCommitFest, + Workflow, ) @@ -148,6 +149,11 @@ def me(request): "header_activity": "Activity log", "header_activity_link": "/activity/", "userprofile": getattr(request.user, "userprofile", UserProfile()), + "workflow": { + "open": Workflow.open_cf(), + "inprogress": Workflow.inprogress_cf(), + "parked": Workflow.parked_cf(), + }, }, ) @@ -678,7 +684,7 @@ def patch(request, patchid): patch_commitfests = ( PatchOnCommitFest.objects.select_related("commitfest") .filter(patch=patch) - .order_by("-commitfest__startdate") + .order_by("-enterdate") .all() ) cf = patch_commitfests[0].commitfest @@ -728,6 +734,11 @@ def patch(request, patchid): {"title": cf.title, "href": "/%s/" % cf.pk}, ], "userprofile": getattr(request.user, "userprofile", UserProfile()), + "workflow": { + "open": Workflow.open_cf(), + "inprogress": Workflow.inprogress_cf(), + "parked": Workflow.parked_cf(), + }, }, ) @@ -1018,6 +1029,9 @@ def status(request, patchid, status): @login_required @transaction.atomic def close(request, patchid, status): + if status == "next": + raise Exception("Can't happen, use transition/ endpoint") + patch = get_object_or_404(Patch.objects.select_related(), pk=patchid) cf = patch.current_commitfest() @@ -1054,86 +1068,6 @@ def close(request, patchid, status): poc.status = PatchOnCommitFest.STATUS_WITHDRAWN elif status == "feedback": poc.status = PatchOnCommitFest.STATUS_RETURNED - elif status == "next": - # Only some patch statuses can actually be moved. - if poc.status in ( - PatchOnCommitFest.STATUS_COMMITTED, - PatchOnCommitFest.STATUS_NEXT, - PatchOnCommitFest.STATUS_RETURNED, - PatchOnCommitFest.STATUS_REJECTED, - ): - # Can't be moved! - messages.error( - request, - "A patch in status {0} cannot be moved to next commitfest.".format( - poc.statusstring - ), - ) - return HttpResponseRedirect("/%s/%s/" % (poc.commitfest.id, poc.patch.id)) - elif poc.status in ( - PatchOnCommitFest.STATUS_REVIEW, - PatchOnCommitFest.STATUS_AUTHOR, - PatchOnCommitFest.STATUS_COMMITTER, - ): - # This one can be moved - pass - else: - messages.error(request, "Invalid existing patch status") - - oldstatus = poc.status - - poc.status = PatchOnCommitFest.STATUS_NEXT - # Figure out the commitfest to actually put it on - newcf = CommitFest.objects.filter(status=CommitFest.STATUS_OPEN) - if len(newcf) == 0: - # Ok, there is no open CF at all. Let's see if there is a - # future one. - newcf = CommitFest.objects.filter(status=CommitFest.STATUS_FUTURE) - if len(newcf) == 0: - messages.error(request, "No open and no future commitfest exists!") - return HttpResponseRedirect( - "/%s/%s/" % (poc.commitfest.id, poc.patch.id) - ) - elif len(newcf) != 1: - messages.error( - request, "No open and multiple future commitfests exist!" - ) - return HttpResponseRedirect( - "/%s/%s/" % (poc.commitfest.id, poc.patch.id) - ) - elif len(newcf) != 1: - messages.error(request, "Multiple open commitfests exists!") - return HttpResponseRedirect("/%s/%s/" % (poc.commitfest.id, poc.patch.id)) - elif newcf[0] == poc.commitfest: - # The current open CF is the same one that we are already on. - # In this case, try to see if there is a future CF we can - # move it to. - newcf = CommitFest.objects.filter(status=CommitFest.STATUS_FUTURE) - if len(newcf) == 0: - messages.error( - request, - "Cannot move patch to the same commitfest, and no future commitfests exist!", - ) - return HttpResponseRedirect( - "/%s/%s/" % (poc.commitfest.id, poc.patch.id) - ) - elif len(newcf) != 1: - messages.error( - request, - "Cannot move patch to the same commitfest, and multiple future commitfests exist!", - ) - return HttpResponseRedirect( - "/%s/%s/" % (poc.commitfest.id, poc.patch.id) - ) - # Create a mapping to the new commitfest that we are bouncing - # this patch to. - newpoc = PatchOnCommitFest( - patch=poc.patch, - commitfest=newcf[0], - status=oldstatus, - enterdate=datetime.now(), - ) - newpoc.save() elif status == "committed": committer = get_object_or_404(Committer, user__username=request.GET["c"]) if committer != poc.patch.committer: @@ -1163,6 +1097,50 @@ def close(request, patchid, status): return HttpResponseRedirect("/%s/%s/" % (poc.commitfest.id, poc.patch.id)) +@login_required +@transaction.atomic +def transition(request, patchid): + from_cf = Workflow.getCommitfest(request.GET.get("fromcfid", None)) + + if from_cf is None: + messages.error( + request, + "Unknown from commitfest id {}".format(request.GET.get("fromcfid", None)), + ) + return HttpResponseRedirect("/patch/%s/" % (patchid)) + + cur_poc = Workflow.get_poc_for_patchid_or_404(patchid) + + if from_cf != cur_poc.commitfest: + messages.error( + request, + "Transition aborted, Redirect performed. Commitfest for patch already changed.", + ) + return HttpResponseRedirect("/patch/%s/" % (cur_poc.patch.id)) + + target_cf = Workflow.getCommitfest(request.GET.get("tocfid", None)) + + if target_cf is None: + messages.error( + request, + "Unknown destination commitfest id {}".format( + request.GET.get("tocfid", None) + ), + ) + return HttpResponseRedirect("/patch/%s/" % (cur_poc.patch.id)) + + try: + new_poc = Workflow.transitionPatch(cur_poc, target_cf, request.user) + messages.info( + request, "Transitioned patch to commitfest %s" % new_poc.commitfest.name + ) + except Exception as e: + messages.error(request, "Failed to transition patch: {}".format(e)) + return HttpResponseRedirect("/patch/%s/" % (cur_poc.patch.id)) + + return HttpResponseRedirect("/patch/%s/" % (new_poc.patch.id)) + + @login_required @transaction.atomic def reviewer(request, patchid, status): diff --git a/pgcommitfest/urls.py b/pgcommitfest/urls.py index 381a523..8d0d308 100644 --- a/pgcommitfest/urls.py +++ b/pgcommitfest/urls.py @@ -3,6 +3,7 @@ import pgcommitfest.auth import pgcommitfest.commitfest.ajax as ajax +import pgcommitfest.commitfest.apiv1 as apiv1 import pgcommitfest.commitfest.lookups as lookups import pgcommitfest.commitfest.reports as reports import pgcommitfest.commitfest.views as views @@ -15,6 +16,7 @@ urlpatterns = [ re_path(r"^$", views.home), + re_path(r"^api/v1/commitfest/active$", apiv1.active_commitfests), re_path(r"^workflow/$", views.workflow), re_path(r"^workflow-reference/$", views.workflow_reference), re_path(r"^me/$", views.me), @@ -28,9 +30,8 @@ re_path(r"^patch/(\d+)/edit/$", views.patchform), re_path(r"^(\d+)/new/$", views.newpatch), re_path(r"^patch/(\d+)/status/(review|author|committer)/$", views.status), - re_path( - r"^patch/(\d+)/close/(reject|withdrawn|feedback|committed|next)/$", views.close - ), + re_path(r"^patch/(\d+)/close/(reject|withdrawn|feedback|committed)/$", views.close), + re_path(r"^patch/(\d+)/transition/$", views.transition), re_path(r"^patch/(\d+)/reviewer/(become|remove)/$", views.reviewer), re_path(r"^patch/(\d+)/committer/(become|remove)/$", views.committer), re_path(r"^patch/(\d+)/(un)?subscribe/$", views.subscribe),