Skip to content

Commit 310a320

Browse files
polobopolobo
committed
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.
1 parent e6dd8ac commit 310a320

File tree

6 files changed

+141
-82
lines changed

6 files changed

+141
-82
lines changed

‎pgcommitfest/commitfest/migrations/0013_no_patches_in_future_cfs.py

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
from django.db import migrations
2+
3+
4+
class Migration(migrations.Migration):
5+
dependencies = [
6+
("commitfest", "0012_add_parked_cf_status"),
7+
]
8+
operations = [
9+
migrations.RunSQL("""
10+
CREATE FUNCTION assert_poc_not_future_for_poc()
11+
RETURNS TRIGGER AS $$
12+
DECLARE
13+
cfstatus int;
14+
BEGIN
15+
SELECT status INTO cfstatus
16+
FROM commitfest_commitfest
17+
WHERE id = NEW.commitfest_id;
18+
19+
IF cfstatus = 1 THEN
20+
RAISE EXCEPTION 'Patches cannot exist on future commitfests';
21+
END IF;
22+
23+
RETURN NEW;
24+
END;
25+
$$
26+
LANGUAGE plpgsql;
27+
28+
CREATE FUNCTION assert_poc_not_future_for_cf()
29+
RETURNS trigger AS $$
30+
BEGIN
31+
-- Trigger checks that we only get called when status is 1
32+
PERFORM 1
33+
FROM commitfest_patchoncommitfest
34+
WHERE commitfest_id = NEW.id
35+
LIMIT 1;
36+
37+
IF FOUND THEN
38+
RAISE EXCEPTION 'Cannot change commitfest status to 1, patches exists.';
39+
END IF;
40+
RETURN NEW;
41+
END;
42+
$$
43+
LANGUAGE plpgsql;
44+
45+
CREATE TRIGGER assert_poc_commitfest_is_not_future
46+
BEFORE INSERT OR UPDATE ON commitfest_patchoncommitfest
47+
FOR EACH ROW
48+
EXECUTE FUNCTION assert_poc_not_future_for_poc();
49+
50+
CREATE TRIGGER assert_poc_commitfest_is_not_future
51+
-- Newly inserted cfs can't have patches
52+
BEFORE UPDATE ON commitfest_commitfest
53+
FOR EACH ROW
54+
WHEN (NEW.status = 1)
55+
EXECUTE FUNCTION assert_poc_not_future_for_cf();
56+
"""),
57+
]

‎pgcommitfest/commitfest/models.py

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ class CommitFest(models.Model):
4242
STATUS_PARKED = 5
4343
_STATUS_CHOICES = (
4444
(STATUS_FUTURE, "Future"),
45-
(STATUS_OPEN, "Bugs"),
45+
(STATUS_OPEN, "Open"),
4646
(STATUS_INPROGRESS, "Ongoing"),
4747
(STATUS_CLOSED, "Closed"),
4848
(STATUS_PARKED, "Drafts"),
@@ -85,10 +85,6 @@ def isclosed(self):
8585
def isopen(self):
8686
return self.status == self.STATUS_OPEN
8787

88-
@property
89-
def isfuture(self):
90-
return self.status == self.STATUS_FUTURE
91-
9288
@property
9389
def isinprogress(self):
9490
return self.status == self.STATUS_INPROGRESS
@@ -301,6 +297,14 @@ def is_closed(self):
301297
def is_open(self):
302298
return not self.is_closed
303299

300+
@property
301+
def is_committed(self):
302+
return self.status == self.STATUS_COMMITTED
303+
304+
@property
305+
def is_committer(self):
306+
return self.status == self.STATUS_COMMITTER
307+
304308
@property
305309
def statusstring(self):
306310
return [v for k, v in self._STATUS_CHOICES if k == self.status][0]
@@ -572,11 +576,6 @@ def open_cf():
572576
cfs = list(CommitFest.objects.filter(status=CommitFest.STATUS_OPEN))
573577
return cfs[0] if len(cfs) == 1 else None
574578

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

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

‎pgcommitfest/commitfest/templates/patch_administrative.inc

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,6 @@
2424
<li role="presentation"><a href="close/committed/" onclick="return flagCommitted({%if patch.committer%}'{{patch.committer}}'{%elif is_committer%}'{{user.username}}'{%else%}null{%endif%})">Commit</a></li>
2525
<li role="presentation" class="divider"></li>
2626
<li role="presentation" class="dropdown-header">Move To:</li>
27-
{%if not cf.isfuture and workflow.future %}
28-
<li role="presentation">
29-
<a
30-
href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.future.id}}">
31-
{{workflow.future.name}}</a>
32-
</li>
33-
{%endif%}
3427
{%if not cf.isopen and workflow.open %}
3528
<li role="presentation">
3629
<a

‎pgcommitfest/commitfest/templates/patch_workflow.inc

Lines changed: 30 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -32,51 +32,53 @@
3232
<!-- Resolve -->
3333
<td>
3434
{%if is_committer or is_author %}
35-
{%if is_committer %}
36-
<a class="btn btn-default" href="close/committed/" onclick="return flagCommitted({%if patch.committer%}'{{patch.committer}}'{%elif is_committer%}'{{user.username}}'{%else%}null{%endif%})">Commit</a>
35+
{%if is_committer and poc.is_open %}
36+
{%if not poc.isparked and poc.is_committer %}
37+
<a class="btn btn-default" href="close/committed/" onclick="return flagCommitted({%if patch.committer%}'{{patch.committer}}'{%elif is_committer%}'{{user.username}}'{%else%}null{%endif%})">Commit</a>
38+
{%endif%}
3739
<a class="btn btn-default" href="close/reject/" onclick="return verify_reject()">Reject</a>
3840
{%endif%}
3941
{%if is_author %}
4042
<a class="btn btn-default" href="close/withdrawn/" onclick="return verify_withdrawn()">Withdraw</a>
4143
{%endif%}
44+
{%if is_committer and not poc.is_open %}
45+
<a class="btn btn-default" href="status/author/">
46+
{{poc.is_committed|yesno:"Revert,Re-Open"}}
47+
</a>
48+
{%endif%}
4249
{%else%}
4350
<span>No Actions Available</span>
4451
{%endif%}
4552
</td>
4653
<!-- Move -->
4754
<td>
48-
{%if not cf.isfuture and workflow.future %}
49-
{%if True %}
50-
<a class="btn btn-default"
51-
href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.future.id}}">
52-
{{workflow.future.name}}</a>
55+
{%if poc.is_open %}
56+
{%if not cf.isopen and workflow.open %}
57+
{%if is_author or is_committer %}
58+
<a class="btn btn-default"
59+
href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.open.id}}">
60+
{{workflow.open.name}}</a>
61+
{%endif%}
5362
{%endif%}
54-
{%endif%}
5563

56-
{%if not cf.isopen and workflow.open %}
57-
{%if is_committer %}
58-
<a class="btn btn-default"
59-
href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.open.id}}">
60-
{{workflow.open.name}}</a>
64+
{%if not cf.isinprogress and workflow.progress %}
65+
{%if is_committer %}
66+
<a class="btn btn-default"
67+
href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.progress.id}}">
68+
{{workflow.progress.name}}</a>
69+
{%endif%}
6170
{%endif%}
62-
{%endif%}
6371

64-
{%if not cf.isinprogress and workflow.progress %}
65-
{%if is_committer %}
66-
<a class="btn btn-default"
67-
href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.progress.id}}">
68-
{{workflow.progress.name}}</a>
69-
{%endif%}
70-
{%endif%}
71-
72-
{%if not cf.isparked and workflow.parked %}
73-
{%if is_author %}
74-
<a class="btn btn-default"
75-
href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.parked.id}}">
76-
{{workflow.parked.name}}</a>
72+
{%if not cf.isparked and workflow.parked %}
73+
{%if is_author %}
74+
<a class="btn btn-default"
75+
href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.parked.id}}">
76+
{{workflow.parked.name}}</a>
77+
{%endif%}
7778
{%endif%}
79+
{%else%}
80+
<span>Cannot move closed patches.</span>
7881
{%endif%}
79-
8082
</td>
8183
</tr>
8284
</tbody>

‎pgcommitfest/commitfest/templates/workflow.html

Lines changed: 43 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -3,21 +3,25 @@
33
<h2>Key Concepts</h2>
44
<h3>The Commitfest Workflow</h3>
55
<p>
6-
The commitfest workflow is a model for patch writing management.
7-
There are a could of ways to classify patches. First, they can be introducing
8-
new features or fixing existing bugs (type). Second, they can be awaiting changes,
6+
The commitfest workflow is a model for patch writing management. The workflow
7+
is designed to manage new feature patches. Bug fixes are typically handled
8+
without the overhead of creating patches in the Commitfest application.
9+
Thus the worflow broadly separates patches into "drafts" and
10+
"potentially committable". The former go onto the "Drafts v19" commitfest
11+
while the later are initially placed into the open commitfest, which is
12+
eventually converted into the ongoing commitfest.
13+
Second, they can be awaiting changes,
914
awaiting guidance, awaiting review, or waiting to be committed (status).
1015
The workflow uses commitfests and patch status in combination to communicate
1116
the overall state of the patch within these two categories.
1217
</p>
1318
<p>
1419
Commitfests are just bins filled with patches. Bins have a defined lifespan
1520
after which they are "Closed" and a new bin for the same category is created.
16-
Each bin serves a role in the workflow and there are 4 active categories of bins.
21+
Each bin serves a role in the workflow and there are 3 active categories of bins.
1722
<ul>
18-
<li>Bugs - annual, all bug reports</li>
1923
<li>Drafts - annual, drafts for new features</li>
20-
<li>Next - scheduled, proposed new features</li>
24+
<li>Open - scheduled, proposed new features</li>
2125
<li>Ongoing - scheduled, review and commit new features</li>
2226
</ul>
2327
There are three active categories of patch status covering the four statuses.
@@ -35,23 +39,17 @@ <h3>The Commitfest Workflow</h3>
3539
</ul>
3640
(See notes below for all available statuses and their intended usage.)
3741
</p>
38-
<p>
39-
The patches in the bugs bin are all of type "bug fix". The full lifecycle of
40-
a bux fix patch lives here and there is no distinction as to the nature of the
41-
reviewer category. The remaining bins exist to help deal with the large volume
42-
of new feature patches.
43-
</p>
4442
<p>
4543
The drafts bin is where patches waiting on significant work go. A reviewer
4644
patch status category here mainly means awaiting guidance, though that will
47-
often lead to a final review, allowing the patch to be moved to the future bin
45+
often lead to a final review, allowing the patch to be moved to the open bin
4846
with the committer patch status category already set.
4947
</p>
5048
<p>
51-
The future and ongoing bins are the ones that strictly comprise a commitfest.
52-
The future bin always exists and new features ready for review and commit go
53-
here. At scheduled points, the future bin becomes an ongoing bin and a new
54-
future bin is created.
49+
The open and ongoing bins are the ones that strictly comprise a commitfest.
50+
The open bin always exists and new features ready for review and commit go
51+
here. At scheduled points, the open bin becomes an ongoing bin and a new
52+
open bin is created (possibly by reclassifying an existing future bin).
5553
</p>
5654
<p>
5755
The ongoing bin only exists during an active commitfest period, during which
@@ -60,11 +58,10 @@ <h3>The Commitfest Workflow</h3>
6058
the bin is closed. This is only bin that is not present at all times.
6159
</p>
6260
<p>
63-
The workflow is also designed with the release cycle in mind, in particular
64-
the start of the annual feature freeze period. At this time both the drafts
65-
and bugs bins are closed and new ones created. If the feature freeze is for
66-
version 18 then the new drafts bin is called "Drafts v19" and the new bugs
67-
bin is named "Bugs v18".
61+
The workflow is designed with the release cycle in mind, in particular
62+
the start of the annual feature freeze period. At this time the drafts
63+
bin is closed and a new one is created. If the feature freeze is for
64+
version 18 then the new drafts bin is called "Drafts v19".
6865
</p>
6966
<h3>Patches on Commitfests (PoC)</h3>
7067
<h4>Overview</h4>
@@ -112,7 +109,7 @@ <h4>Inactive</h4>
112109
A Rejected patch has the same effect as Withdrawn but communicates that the
113110
community, not the author, made the status change. This should only be used
114111
when it is when the author is unable or unwilling to withdraw the patch or park
115-
it for future rework.
112+
it for rework.
116113
</p>
117114
<p>
118115
*Returned With Feedback complements rejected in that the implication of rejected
@@ -123,9 +120,10 @@ <h4>Inactive</h4>
123120
leaves reject as the fallback non-commit option and makes returned a deprecated
124121
administrator-only option.
125122
</p>
123+
<h3>Special PoC Situations</h3>
126124
<h4>Moved</h4>
127125
<p>
128-
Moved PoCs are a side-effect of having commitfest, and volume. Many active
126+
Moved PoCs are a side-effect of having commitfests, and volume. Many active
129127
patches cannot be gotten to within a commitfest. In order to keep them visible
130128
they must be moved to another commitfest, creating a new PoC with the same
131129
state. The PoC they were moved from is updated to "Moved" to indicate this
@@ -137,6 +135,14 @@ <h4>Is Abandoned</h4>
137135
but the commitfest is closed. Conceptually, this is the same as
138136
withdrawn, but through inaction.
139137
</p>
138+
<h3>Reverted Patches</h3>
139+
<p>
140+
Not every patch that is committed stays that way. The Workflow doesn't have
141+
any statuses for this, though the user interface does provide an action that
142+
a committer can invoke. Upon doing so the PoC status is updated to
143+
author. The history flow of commited followed by author indictes a probable
144+
revert.
145+
</p>
140146
<h3>Patches</h3>
141147
<h4>Overview</h4>
142148
<p>
@@ -183,17 +189,21 @@ <h4>Ongoing</h4>
183189
An Active (see Workflow above) period where no new features should be added
184190
and the goal is to get as many review"patches committed as possible.
185191
</p>
186-
<h4>Future</h4>
192+
<h4>Open</h4>
187193
<p>
188-
The next active (see workflow above) period.
189-
Any patch not exempted to be added to the ongoing or bugs commitfests
190-
can always be added here.
194+
Patches ready for final review and commit, according to the author, are placed
195+
in the current open commitfest. Upon the scheduled start date it is manually
196+
updated to be an ongoing commitfest, at which point no new patches should be
197+
added.
191198
</p>
192-
<h4>Bugs</h4>
199+
<h4>Future</h4>
193200
<p>
194-
A commitfest for high-priority and bug fix patches. Full patch lifecycle.
195-
Created as "Bugs v18" at the start of the v18 feature freeze period (closing
196-
"Bugs v17").
201+
The PostgreSQL project works on a schedule release cycle. At the beginning
202+
of each cycle the planned commitfest periods are decided and communicated to
203+
the community via the creation of future commitfests. While classified as
204+
future these commitfests are not permitted to associated with any patches.
205+
Their classification is changed to open as each prior ongoing commitfest
206+
closes.
197207
</p>
198208
<h4>Drafts</h4>
199209
<p>
@@ -210,7 +220,7 @@ <h4>Drafts</h4>
210220
</p>
211221
<h4>Closed</h4>
212222
<p>
213-
Drafts, bugs and ongoing commitfests are closed (future ones
223+
Drafts and ongoing commitfests are closed (open ones
214224
always go through ongoing) when the period they cover has passed.
215225
Closing a commitfest does not impact its related PoCs; though no new PoCs can
216226
be created for a closed commitfest.

‎pgcommitfest/commitfest/views.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -720,7 +720,6 @@ def patch(request, patchid):
720720
"userprofile": getattr(request.user, "userprofile", UserProfile()),
721721
"workflow": {
722722
"open": Workflow.open_cf(),
723-
"future": Workflow.future_cf(),
724723
"progress": Workflow.inprogress_cf(),
725724
"parked": Workflow.parked_cf(),
726725
},

0 commit comments

Comments
 (0)