Page MenuHomePhabricator

Make it easier to work through a queue of stuff
Closed, ResolvedPublic


Users will need to review "all the audits" at once and can find it cumbersome during all that navigation. Figure out a way to have folks "work through a queue" where after they take an action on item N the page will load to item N + 1.

Original Ask

We're doing obligatory auditing in our team for all of our commits, and working hard to get people to actually pay attention to the phabricator front page for commits to audit. One of the complaints I have been getting is the clumsiness of auditing many commits. Having to open the commit, scroll to the form (or press Z), select accept commit then press submit, and repeat 10 times when someone commited a patch series quickly becomes cumbersome.

I would like a button that will choose accept commit from the combobox and press submit at the same time. If possible, it should also proceed to the next commit in the audit list, but I can see that this is a significantly more complex feature, so for a start just getting that quick-accept button would help a lot in convincing new reluctant team members to actually keep up.

I have looked at the source code, attempting to add a button to AphrontFormSubmitControl that performs some javascript that does this, but I'm not too familiar with neither PHP nor phabricator source code, so I have not gotten far.

Related Objects

Event Timeline

metellius raised the priority of this task from to Needs Triage.
metellius updated the task description. (Show Details)
metellius added a project: Audit.
metellius added a subscriber: metellius.
timor added a subscriber: timor.Aug 7 2014, 2:00 PM

Maybe one solution would be to move the comment box to the top.

That wouldn't really change much, it might even be annoying at times since you typically read through the commit all the way to the bottom, and typically that is the point in time where you want to accept the commit.

I realize I mentioned having to scroll as one of the issue, but the biggest issue is the lack of convenient workflows for accepting many commits.

chad triaged this task as Wishlist priority.Aug 7 2014, 4:28 PM
chad added a subscriber: chad.Aug 7 2014, 4:41 PM

I don't want to comment on this specifically, just that as a product designer I hesitate when deciding between making things super easy (less quality) or some friction (pause and reset mindset). Friction isn't a bad thing, especially when you need more information or more thorough review. Because Phabricator is a tool for completing quality work, making things too easy seems not as important over ensuring some quality.

How many commits does the average person review in a week? Are there people who essentially just live in Audit? This type of "next" interface could be essentially added anywhere in Phabricator (next Needs Triage task, next Diff, etc).

In T5815#8, @chad wrote:

I don't want to comment on this specifically, just that as a product designer I hesitate when deciding between making things super easy (less quality) or some friction (pause and reset mindset). Friction isn't a bad thing, especially when you need more information or more thorough review. Because Phabricator is a tool for completing quality work, making things too easy seems not as important over ensuring some quality.

I see your point, but there are other concerns as well. In our company our style is to do many small commits, and we just don't have the manpower to do a review for every change. Auditing has become our last resort to ensuring all of our code gets looked at twice before shipping.

Looking at our git log, we have around 15-20 commits a day, with a team of 6 that's around 3 audits a day. The majority of these commits change less than 15 lines of code, and with a team somewhat skeptical to new tools with a sprinkle of RSI on the top, I hope you can understand why I'm trying to cut down the amount of clicks needed for a basic audit. Peer pressure should ensure people spend time on the audits, not intentionally cumbersome interfaces.

Bugzilla has this feature, and we use it during frequent bug scrub meetings for efficiently walking through a certain list of bugs in order to keep them up to date.

btrahan added a subscriber: btrahan.Aug 7 2014, 7:59 PM

I'm confused about how you can not have the manpower for review before commit (Differential) but do have the man power to review after commit (Audit)...? These are theoretically the exact same time commitment, though in theory audit will ultimately be more time consuming since issues are harder to fix after they've been committed, possibly deployed to production, etc but I digress.

The only way I can understand this is if in the audit case the review isn't being done to the same quality as it would be in the differential case. As @chad said, this is supposed to be a tool for quality, thoughtful work, not a tool to process a bunch of crap as quickly as possible.

I'd say ~3 audits per day, assuming you actually review it (e.g. you won't just be hitting "accept" as fast as possible), if all 3 just so happened to be accept folks can suffer through the extra 6 seconds or so total, and otherwise they need to "raise concern" or whatever anyway. These 6 seconds extra across the three audits should be a minute amount of time compared to reading the code, even if each change is a mere 15 lines long.

Long story short, I think any of these "get through this in one click" buttons are a bad product idea, and workflows that would benefit from them should be re-jiggered.

That said, "go to next" in a series of things seems much more reasonable to me. I'm going to morph this ticket to that effect.

btrahan renamed this task from A quick-accept button for accepting a commit and proceeding to the next one to Make it easier to work through a queue of stuff.Aug 7 2014, 8:01 PM
btrahan updated the task description. (Show Details)
btrahan added projects: Differential, Maniphest.

Realistically, one guy handling a series of up to 10 commits will not be a rare occurance; if you start auditing a commit series it doesn't really make sense to just quit in the middle.

I still do think reviews involves more commit logistics and terminal work, and it's often awkward to split a longer continuous work into reviewable chunks (for example with the porting I am involved with right now which has going for months). But I would like to add that I of course agree that reviews produce better commits.

Either way, I'm not hoping to spawn a heated discussion here, and I am very glad to hear that the "go to next" feature is of interest.

One last alternative for the accept-shortcut: the comment editor already has <Ctrl-enter> as a shortcut for pressing submit. Would binding something like <Ctrl-shift-enter> to "accept and submit" be unthinkable?

chad added a comment.Aug 7 2014, 8:43 PM

I don't think we'd be interested in that for the upstream. Maintaining a local patch defaulting the action to "Accept Commit" seems like the shortest path for you right now.

Not directly related, but we also had a "Comment and Close" shortcut button in Maniphest previously (similar to GitHub) but eventually removed it because it was a continual source of mistakes and confusion. You can fish around related objects of T4657 / D9221 for discussion. I think adding this to audit (or Differential) is a harder sell than Maniphest was, and it didn't work out there.

Narrowly what some people have told me is is that the flow with the back button(s) to get the the "next" audit is annoying. I think if I've read it right that's what Chad's "work through a queue" description is trying to address.

angie added a project: Restricted Project.Jun 5 2015, 6:49 PM
muteor added a subscriber: muteor.Jul 7 2015, 1:01 PM

For me having a way to do "action" then next would be perfect. So I could accept or raise concern and move onto the next audit without having to go back. What I imagine is an extra submit button like [Submit & Next] [Submit].

This would save me a load of time and clicking through 20+ tabs.

OCram added a subscriber: OCram.Sep 4 2015, 5:22 AM
angie moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Sep 9 2015, 10:28 PM
angie moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.

This is blocked by T8783 (building Nuance to initial release quality) and T5307 (taking actions with search results).

T8783 is on our natural roadmap but is a large amount of work (at least a solid week of real development).

T5307 isn't huge, but isn't trivial, either.

The result would be a new query result action, Import Items into Nuance... or similar. This would bring them into some kind of internal Nuance queue with application actions.

Definition of application actions probably depends partly on T9132. This is prioritized elsewhere, but also a large amount of work.

fanis added a subscriber: fanis.Nov 26 2015, 3:34 PM
aj removed a subscriber: aj.Nov 15 2016, 11:28 PM
epriestley moved this task from Backlog to Far Future on the Audit board.Jan 10 2017, 4:43 PM
hskiba added a subscriber: hskiba.Feb 1 2017, 8:50 AM
epriestley closed this task as Resolved.May 21 2017, 1:59 PM
epriestley claimed this task.

I'm going to close this; there are two possible followups -- each of which is better focused on a more specific set of problems -- but I'm not merging it into either one since I think there's a lot of general ambiguity about problems and use cases here. If you're interested in this, feel free to follow either or both.

  • T12740 describes building tools to make bulk edits which require human decision making easier. In this case: you are looking at the content of each object carefully, making a complex decision based on human expertise, and then taking a particular action based on that decision. For example: triaging a large number of new tasks and giving then appropriate tags and owners, asking clarifying questions, merging/closing duplicates, etc. This is generally the direction which we (the upstream) have been pulling this task in.
  • T10005 describes bringing the "Bulk Edit" tool to other applications, for applying the same change to a large number of objects. In this case: you're not making any real decisions, or are only making snap decisions based on name/metadata, then taking the same action for large numbers of results. For example: approving every user in queue except Jack, who is a jerk. I think this is generally the direction that most of the reports which were merged here are actually headed.

T12740 has some additional discussion about the weakness of current "audit" use cases we've collected, specifically. If you have a stronger audit use case which satisfies the points there (or can make a stronger argument for current use case), feel free to follow up there.