- Notifications
You must be signed in to change notification settings - Fork12
Add a Parking Lot (Drafts)#53
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm probably missing something obvious, but why was this join added? Thecf doesn't actually seem to be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's used inpatchlist()here, during personalization. (I'm not a huge fan of the long-distance coupling, but...)
Uh oh!
There was an error while loading.Please reload this page.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This seems incorrect? I.e. it won't remove anything afaict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah, I meant to add an explanation to the commit but didn't. This is fully cargo-culted from the last change made tocommitfest_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
(Maybe@mhagander knows why this should or should not be done this time around?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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 acommitfest ID.
The upcoming Drafts commitfest will break the assumption that lowercommitfest IDs are always "in the past".
...for the upcoming Drafts feature.
Drafts is a special always-open commitfest for the purpose of holdingand testing draft patch submissions. Having such a holding area shouldmake it easier for people to keep track of patchsets they're not quiteready to submit for review.Internally, Drafts is assigned a static ID of zero. This is chosenbecause a) it does not overlap with Django's default AutoField sequence,which begins at one, and b) it requires no updates to the current URLpatterns, which match nonnegative integers.The Drafts entry has the special status STATUS_DRAFT so that it does notconflict with pre-existing coded assumptions on what "open", "future",etc. mean. STATUS_DRAFT CFs are excluded from the "num_cfs" count for apatch.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 byclosing 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 beenmodified:- 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 dateTODO:- ensure all prior assumptions on CF ID are cleaned up
The Drafts CF can theoretically accumulate a large number of closeditems. Add a filter that encompasses all open statuses so that Draftswill be able to use it as a default.
This hides abandoned draft patches unless the user specifically asks tosee them.TODO: Does the behavior of the "Clear" button on the filter UI elementneed to change, or does it actually work the way a user would want forDrafts?
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. |
Uh oh!
There was an error while loading.Please reload this page.
[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