Skip to content

Upgrade Commitfest to Workflow Manager #62

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft
2 changes: 1 addition & 1 deletion .editorconfig
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,6 @@ trim_trailing_whitespace = true
indent_style = space
indent_size = 4

[*.html]
[*.{html,inc}]
indent_style = space
indent_size = 1
7 changes: 7 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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
20 changes: 20 additions & 0 deletions media/commitfest/css/commitfest.css
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
36 changes: 36 additions & 0 deletions pgcommitfest/commitfest/fixtures/auth_data.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": []
}
}
]
40 changes: 40 additions & 0 deletions pgcommitfest/commitfest/fixtures/commitfest_data.json
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
from django.db import migrations


class Migration(migrations.Migration):
dependencies = [
("commitfest", "0010_add_failing_since_column"),
]
operations = [
migrations.RunSQL("""
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: would be nice to include reverse_sql too in all these RunSQL migrations, so that the migration can be reverted. Mostly useful for development and testing.

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.
$$
"""),
]
23 changes: 23 additions & 0 deletions pgcommitfest/commitfest/migrations/0012_add_parked_cf_status.py
Original file line number Diff line number Diff line change
@@ -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,
),
)
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
from django.db import migrations


class Migration(migrations.Migration):
dependencies = [
("commitfest", "0012_add_parked_cf_status"),
]
operations = [
migrations.RunSQL("""
CREATE FUNCTION assert_poc_not_future_for_poc()
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@JelteF JelteF Apr 14, 2025

Choose a reason for hiding this comment

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

I definitely agree a schedule of planned commitfests has value, but having admins manually create "Future" commitfests is a pretty crude way to do that. And it doesn't even work well in practice, e.g. there hasn't been a single "Future" commitfest for at least 3 months.

I think a much better way would be to statically document/hardcode these dates, they are the same every year. This would also allow us to have a "Start commitfest" button that automatically creates a new "Open" commitfest for the next start/end date. i.e. instead of creating entries in the database with their only purpose being displaying a schedule, we could also display the schedule without these entries existing, because we already know the dates of the next commitfests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All good points. Probably want to put them into an issue at this point.

For my part I’m going to include the database constraint/trigger that prohibits Future CFs from having patches associated with them to at least avoid having to deal with that possibility when we do address this in the future. It also allows removal of the “next” action implementation which is the primary incompatibility that exists with Workflow; which itself enables Parked/Drafts.

I’ll be changing the Action drop-down menu but postpone the main table rework as it will break CFBot.

CFBot can readily use the new Workflow endpoint to find the CF IDs it needs to scrape. Given it already handles two CFs being scraped handling three should be a reasonable extension.

I’ll add a schedule to the new workflow doc page.

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();
"""),
]
Loading