Add a Parking Lot (Drafts) #53
Conversation
| """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...)
| 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?)
There was a problem hiding this comment.
@JelteF Is it possible to take a look at prod just to confirm that this code is unnecessary? I'm happy to remove it.
Simplify the Activity query now that patch links no longer need a commitfest ID.
The upcoming Drafts commitfest will break the assumption that lower commitfest IDs are always "in the past".
...for the upcoming Drafts feature.
Drafts 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, Drafts 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 Drafts entry has the special status STATUS_DRAFT so that it does not conflict with pre-existing coded assumptions on what "open", "future", etc. mean. STATUS_DRAFT CFs are excluded from the "num_cfs" count for a patch. The new /close/draft handler is added to swap patches into Drafts. 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
The Drafts CF can theoretically accumulate a large number of closed items. Add a filter that encompasses all open statuses so that Drafts will be able to use it as a default.
This hides abandoned draft patches unless the user specifically asks to see them. TODO: Does the behavior of the "Clear" button on the filter UI element need to change, or does it actually work the way a user would want for Drafts?
|
The most recent force-push changes the name to Drafts and adds a default "Open" filter to the special CF, to try to keep it from being overwhelmed by abandoned entries. |
[update 5/25: the name of the feature has been changed to Drafts]
Drafts 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, Drafts 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 Drafts entry has the special status
STATUS_DRAFTso that it does not conflict with pre-existing coded assumptions on what "open", "future", etc. mean.STATUS_DRAFTCFs are excluded from thenum_cfscount for a patch.The new
/close/drafthandler is added to swap patches into Drafts. 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
Openselector to the filter bar UI.Openselector by default.TODO