Add a Parking Lot #53
Conversation
Simplify the Activity query now that patch links no longer need a commitfest ID.
The upcoming Parking Lot commitfest will break the assumption that lower commitfest IDs are always "in the past".
...for the upcoming Parking Lot.
The Parking Lot is a special always-open commitfest for the purpose of holding and testing draft patch submissions. Having such a holding area should make it easier for people to keep track of patchsets they're not quite ready to submit for review. Internally, the Parking Lot is assigned a static ID of zero. This is chosen because a) it does not overlap with Django's default AutoField sequence, which begins at one, and b) it requires no updates to the current URL patterns, which match nonnegative integers. The Parking Lot entry has the special status STATUS_PARKING so that it does not conflict with pre-existing coded assumptions on what "open", "future", etc. mean. STATUS_PARKING CFs are excluded from the "num_cfs" count for a patch. The new /close/parked handler is added to swap patches into the Parking Lot. Patches are then removed by moving them to the next open CF, or by closing as usual. Prior to this patch: - CFs with IDs less than the current in-progress CF could safely be assumed closed, - patches only ever moved forward through increasing CF IDs, and - the latest CF start date determined a patch's "current" CF. These assumptions all break under the current model. They have been modified: - use STATUS_CLOSED specifically when deciding whether a CF is closed - when moving a patch between CFs, allow for the possibility of an existing entry in the junction table - a patch's "current" CF is determined by its latest entry date TODO: - ensure all prior assumptions on CF ID are cleaned up - should the default filter be changed for the Parking Lot?
| """SELECT ps.status, ps.statusstring, count(*) | ||
| FROM commitfest_patchoncommitfest poc | ||
| INNER JOIN commitfest_patchstatus ps ON ps.status=poc.status | ||
| INNER JOIN commitfest_commitfest cf ON cf.id=poc.commitfest_id |
There was a problem hiding this comment.
I'm probably missing something obvious, but why was this join added? The cf doesn't actually seem to be used.
There was a problem hiding this comment.
It's used in patchlist() here, during personalization. (I'm not a huge fan of the long-distance coupling, but...)
| _STATUS_CHOICES = ( | ||
| (STATUS_FUTURE, "Future"), | ||
| (STATUS_OPEN, "Open"), | ||
| (STATUS_INPROGRESS, "In Progress"), | ||
| (STATUS_CLOSED, "Closed"), | ||
| (STATUS_PARKING, "Parking"), |
There was a problem hiding this comment.
nit: Seems good to change the name to "Parking Lot"
| ON CONFLICT (status) DO UPDATE SET statusstring=excluded.statusstring, sortkey=excluded.sortkey; | ||
| """), | ||
| migrations.RunSQL( | ||
| "DELETE FROM commitfest_patchstatus WHERE status < 1 OR status > 9" |
There was a problem hiding this comment.
This seems incorrect? I.e. it won't remove anything afaict.
There was a problem hiding this comment.
Ah, I meant to add an explanation to the commit but didn't. This is fully cargo-culted from the last change made to commitfest_patchstatus: 535af6e
If this table doesn't need that much pruning in practice, that's fine and I can remove it. But I can't take a look at the current state of the prod database to really know for sure 😁
There was a problem hiding this comment.
(Maybe @mhagander knows why this should or should not be done this time around?)
The Parking Lot is a special always-open commitfest for the purpose of holding and testing draft patch submissions. Having such a holding area should make it easier for people to keep track of patchsets they're not quite ready to submit for review. This is one possible implementation of #17.
Internally, the Parking Lot is assigned a static ID of zero. This is chosen because a) it does not overlap with Django's default
AutoFieldsequence, which begins at one, and b) it requires no updates to the current URL patterns, which match nonnegative integers.The Parking Lot entry has the special status
STATUS_PARKINGso that it does not conflict with pre-existing coded assumptions on what "open", "future", etc. mean.STATUS_PARKINGCFs are excluded from thenum_cfscount for a patch.The new
/close/parkedhandler is added to swap patches into the Parking Lot. Patches are then removed by moving them to the next open CF, or by closing as usual.Prior to this patch:
These assumptions all break under the current model. They have been modified:
STATUS_CLOSEDspecifically when deciding whether a CF is closedRoadmap
TODO