Upgrade Commitfest to Workflow Manager #62
Conversation
Upgrading to Workflow involves many changes. These few are basicially non-invasive and set the stage for what is to come. Introduced: workflow.html - a help page for the new idea of Workflow CommitFest.STATUS_PARKED New Workflow Model to implement Workflow rules and functionality Enforcement of the Following Constraints: - "At most one" non-Closed Commitfest Status Presence - "At most one" non-Moved POC Status Presence - Closed POC status include leavedate, otherwise it is absent Also, added a convenience Make target for restoring test environment Also, added *.inc to the .editorconfig for html fragments Workflow Highlights: Commitfests are now just Future-In Progress-Closed Yearly Close/Create at v18 Feature Freeze: Drafts v19, Bugs v18
In the interest of breaking up this major change to both UX, backend, and Workflow this patch separates out the main UX redesign choices for independent consideration. There is a new workflow display that will be polished and described once the final components have been incorporated. For now it simply places all of the action menu items on the screen; and incorporates a more coordinated and flow-oriented layout for the information in the existing key-value table. The #history-table is hidden by default but does include potentially useful information and so is expected to remain long-term. The status history field in the keyvalue-table may be moved here at some point. The elements from the #keyvalue-table that did not get incorporated into the workflow display are placed into a new workflow supplemental table. Unlike the key-value table, the new tables are horizontally compact. The #keyvalue-table is presently hidden but viewable. Conceptually, the contents of this table should never be needed in this view and at some point the table itself can be removed. For now it is left as a fallback. The existing action bar has been renamed the administrative bar. This will be visible only to staff and has minimal logic for showing and hiding UI widgets. That way, should the UI logic be faulty, it is generally still possible for staff to fix it. In this commit the contents of the bar remain unchanged, only the file name is different.
Remove all usage of close/next in the UI (just administrative actions). Remove all 'next' detection logic. The UI will provide valid options to the user to choose between and they can make an informed choice; especially since for the typical user there will only be a single choice, to the Future Commitfest or back to the Parked one. Committers can also choose to bring any patch into the In Progress or Open/Interlude. The implicit constraint 'next' applied to the system was that, practically speaking, as Patches moved they always move to a Commitfest that was started in the future: thus current_patch_on_commitfest could just use a descending start date sort to find the primary Commitfest. The transition action uses the Workflow implementation which allows for a Patch to re-enter the same Commitfest. This changes the entrydate on the POC which is now used to find the current Commitfest. The Workflow module tries to improve upon the History messages being generated. Additional thoughts forthcoming in this area. Should be simple enough to retain the status quo on these if desired, especially since I'm disliking the absence of a cfid column; which is why i added the relevant POC Commitfest name to the messages. The new code is a bit chatty in terms of feedback to the user. Though, on the flip side, it intentionally doesn't feel the need to verify that the button just clicked was intended. That needs to be tidied up based upon the guidelines I need to review.
Implement the Workflow rules checked for in the model within the GUI. This way, people only see the actions relevant to their situation. None of the URLs have changed and the only modifications to the context environment so far have been additive - thus up to here there have been only a couple of minor backend changes. Namely, enforcing the already implicit policy of only opening up at most one of each Commitfest type at any given it. And allowing for re-entrancy of a Patch to a Commitfest. The UI does enforce a "Commit or Reject" policy for Patch resolution. Staff can still use Returned with Feedback. The author is expected to Withdraw their patch if they do not plan to act on the feedback, or move it back to Parked and Waiting on Author. Or they can Withdraw and create a new patch in the future. While the Workflow is not against giving people options, the volume of both rejected and returned resolutions is too small to warrant distinguishing them. Hopefully the finished UX is intuitive. The core three actors in the Workflow - author, reviewer, committer - have their core actions in columns left-to-right (though the columns are action-type oriented, not actor oriented). The headers are the verbs for the actions in the second row. The third row is actor-oriented listing the author(s), reviewer(a), and committer along with related membership actions. The fourth column is where the new movement implementation appears, making /close/next obsolete, especially with the addition of Parked. Having run out of actor categories the empty cell in the bottom-left becomes a perfect place to display the core information from the key-value table (which has been hidden). Namely, the status of the Commitfest and Patch this POC pertains to, the version (hyperlink on the label to the Edit screen), the topic (also with a label hyperlink to Edit), the Subscribe/Unsubscribe button, and the last modified timestamp. Emails, CFBot, and Links also brought over in a key-value table; while History is also hidden but part of the final design. The hidden key-value table is being phased out. The key elements from it that need to be figured out are: - Status (is a form of History and that should get a re-work) - Created (doesn't seem actionable, especially now with Parked) - Latest Email (redundant with Email content?) ID doesn't seem to useful and can be gotten from the URL if needed. Title is already at the top of the page. All other fields are incorporated into the Workflow UI.
By creating the poc_enforce_maxoneoutcome_idx unique partial index we have ensured that every Patch has only one current commitfest defined by having a status that isn't Moved. Update Patch to find this PoC via the index and then return its commitfest property for current_commitfest (and note the method as being deprecated).
Make getting a POC while only having a patchid a single Workflow call. Patch and committer use the committer lookup and change interface. Status and close use the updatePOCStatus interface used by transition. All four leverage PoC as their main hook for patch and commitfest info.
It is desirable to be able to view the status history along with entry and leave dates. Add an edit button for authors to match up with reviewer and committer. Also, explicitly label what we are displaying in the final row.
The idea of having actions visible and then redirect for authentication is acceptable in general but the /patch view is now driven by the role of the user viewing the page that it seems easier to simply show no actions if the user is not authenticated and add a note just above the action table indicating the read-only mode. Add missing is_staff protection for the administrative actions form. Just outright removing the key-value table if you are not logged in. Don't want to edit it, want to get rid of it.
Categories are labels/titles and should use title case. Use initial capital letter on all non-joining words. In Progress is an OK label choice but Ongoing communicates the same thing with a single word just like all the other options. In the workflow the "Parked" status is being used for commitfests labelled, e.g., Draft v18, so use Drafts as the category label too. Change Open to Bugs to relfect its new usage. Once committed a large technical refactor/rename pass over the models and code to make the enums match up with the usage is planned.
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.
There was a problem hiding this comment.
Overall I think this has some good ideas. It's a big enough change in the usage of the site that we should probably discuss this also on the hackers mailinglist once there's a bit more mature proposal.
To help make this easier reviewable, and allow merging incremental improvements quicker. I think it would be helpful if you moved some of the changes to separate PRs. Things like the editorconfig/makefile changes as well as some refactoringslike patch_tr_cfbot.inc
| ("commitfest", "0010_add_failing_since_column"), | ||
| ] | ||
| operations = [ | ||
| migrations.RunSQL(""" |
There was a problem hiding this comment.
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.
| ] | ||
| operations = [ | ||
| migrations.RunSQL(""" | ||
| CREATE FUNCTION assert_poc_not_future_for_poc() |
There was a problem hiding this comment.
Instead of restricting what you can do with "Future" commitfests, why not remove them completely? (honest question, they seem kinda useless to me)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| (STATUS_AUTHOR, "Waiting on Author"), | ||
| (STATUS_COMMITTER, "Ready for Committer"), | ||
| (STATUS_COMMITTED, "Committed"), | ||
| (STATUS_NEXT, "Moved to next CF"), | ||
| (STATUS_NEXT, "Moved"), |
There was a problem hiding this comment.
Nice. This also makes sense without your other changes. It's not always moved to the next commitfest currently either, e.g. a previously closed one can years later be moved to the open commitfest again.
| bin is closed and a new one is created. If the feature freeze is for | ||
| version 18 then the new drafts bin is called "Drafts v19". | ||
| </p> | ||
| <h3>Patches on Commitfests (PoC)</h3> |
There was a problem hiding this comment.
Terrible acronym, because it conflicts with the much more commonly used "Proof of Concept". I know it's called that way in the code, I hate it there too. We should definitely not use it publicly.
In general I think "patches on commitfests" should be considered an implementation detail (and I'm not sure I even like the implementation). "patches on commitfests" is not something users should know about. Users should simply be thinking of a "patches", not a "patches on commitfests". From a users perspective a patch has these things which are internally implemented using "patches on commitfests"
- A current status
- A current commitfest
- A list of previous commitfests
There was a problem hiding this comment.
Good point. I hate Patch though...since a patch is physical file being applied to the codebase. CFBot calls it Submission - probably because of the fact its job is to apply patch files. How about we start calling the things in the Commitfests Submissions publicly in CFApp and resolve the internal naming mess later?
There was a problem hiding this comment.
I agree that Patch is an overloaded term. I'm not a huge fan of "Submission" either, it's rather vague and thus often needs the "commitfests" prefix to clarify. How about we call it a "Patch Set"/"Patchset" instead of a "Submission".
There was a problem hiding this comment.
After also considering Patch Series I’m most accepting of Patch. It contains versioned patch sets.
| state. The PoC they were moved from is updated to "Moved" to indicate this | ||
| flow-of-time action. | ||
| </p> | ||
| <h4>Is Abandoned</h4> |
There was a problem hiding this comment.
I cannot find anything about this in the code. What is the intent here?
Also, given we don't automatically move patches anymore. I think we probably need better wording here, because otherwise all patches in the active commitfest would be called abandoned if it was closed.
There was a problem hiding this comment.
I'm changing it to "Is Ignored"
I'm giving a name/label to what already exists in reality. Open patches (WoA, NR, RfC) on Closed Commitfests.
CFBot definitely ignores it and its visibility within the CFApp is significantly reduced in that state, effectively ignored by most people.
There was a problem hiding this comment.
We do/should? Continue to check for thread messages on them though…consider whether such thread activity can alter the status of the submission.
| is made aware of these files and can incorporate information pertaining to them | ||
| into its reporting. | ||
| </p> | ||
| <h3>Commitfests</h3> |
There was a problem hiding this comment.
You already sortof introduced commitfests in the workflow section. I think this needs some re-ordering/deduplicating/merging of information.
There was a problem hiding this comment.
As with patch, its intended more as a reference section for the topic. But I too don't consider it quite publication worthy yet. Just waiting to see if new/different material needs to be considered as well.
Yeah this seems to be broken. I haven't looked into it closely yet, but I think it's probably that the admin login form does not allow logins as non-staff users. |
|
Could you create a separate PR for your editorconfig changes? Then I can merge that and I don't have to press "Approve CI run" button anytime you make a change to this PR, because this is your first PR to this repo and github doesn't trust you yet. |
Will do. |
This is now mostly a referential PR that shows the end goal. First step on getting there is over in PR #65
Note: I'm now seeing just how tied-in CFBot is to what I hoped were just human-readable labels. There is definitely a subset of these changes that can go in without full surgery on the CFBot. But I'm doing surgery on CFBot right now to at least making getting some of the others in more easily. In particular, not yet pushed, but a new API endpoint to start reducing the amount of screen scraping needed.
Key Features
Bugs (and Priority) Commitfest (Yearly @ feature freeze) (Open Status)Architecture
Comments
This is probably about 95% the way done, though this is my first Python code submission ever and I haven't read up on Django yet. I remembered to do format/lint at the end of the first push (commit 11) - can rebase if needed but figured this probably goes in as a single commit anyway. Commit 12 reflects the changes to revert "Future" to being a memo commitfest (no patches, enforced by trigger) and Open to being the normal "next CF" while Bugs is removed.
The first four commits affect the desired changes with minimal new backend or middleware coding. They do add the Parked CommitFest status option, as well as constraints ensuring that a Patch has exactly one primary PatchOnCommitFest (poc) and that among the Commitfests all statuses except Close only appear once. The UX changes to the /patch viewer then leverage these to implement the relevant "Move to" actions for each non-Closed Commitfest status; via a new /transition action leveraging most of the Workflow model.
The fifth commit tries and improves upon finding the primary PoC for a patching using the unique partial index instead of a collection filter-and-sort. Not being familiar with Django this is my best guess at what to be doing here.
The sixth commit just makes the different actions, and patch view, use the Workflow model added in the first patch.
Seven through ten are a bit tack-on as I was preparing to submit this pull request; they make no substantive changes to the proposed Workflow model but clean up some details in the UX.
Oh, yeah, the Makefile and .editorconfig changes made my life easier so I tossed them in for discussion.
Also, couldn't figure out how to login as normal (password normal). Will open a separate issue once I try harder but it didn't just work like the readme said.
Related
#53 - PR. Implements Parked without a reserved commitfest id. Close
#17 - Most of the discussion was mine and this is what I was aiming for. Close
#25 - Introduces the feature-freeze annual commitfest switch-over and tweaks the existing three-part commitfest to two-parts. Update
#40 - Not addressed with the initial pull request but definitely a related concern. Reframe Issue Subject
#19 - Adding a new fixed "Feedback" is justified with the Workflow description but also made optional. Close and just use #11
#15 - The author listing cell in the new UI delegates to the Edit screen for now; try and get this in prior to reworking that element of the UI. Leave Open
David J. (David G. Johnston on the mailing lists and elsewhere)
You can also find me on the PostgreSQL Hacking Discord (#commitfest-dev)