Page MenuHomePhabricator
Create Task
Maniphest T4225

Allow Herald rules to apply the "exist" and "do not exist" conditions to the "Reviewers" field
Closed, ResolvedPublic
Actions

Description

I'm trying to design some Herald rules to support a workflow for when we start actively using Phabricator. I want to be able to support a couple behaviors:

(1. I'd like for my engineers to be able to store "work in progress" differential revisions that aren't yet ready for review (rather than on their laptops when can be eaten by a bear, struck by lightning, or sold to support crippling drug habits). I think that arc diff --plan-changes can work for this, but I think a new status of "Work in Progress" would be apropos (e.g., arc diff --wip)). Dupes T2543

  1. I'd like for my engineers to not worry too much about who is going to review their code, so I'd like them to be able to leave the reviewers field empty when populating their revision. Then, based on herald rules that apply some heuristics (which repository, which directory within the repository) will automatically select some reviewers. (Of course, if they nominate a reviewer, the herald rules should do nothing. However, I don't want to pester potential reviewers until the review exits the "work in progress" status. I see a couple problems a) It's not clear how to create a Hearald rule that matches the empty set. b) Herald doesn't currently have a capability to test against the status of a revision.

(3. It might be nice to have a "pester" button on a differential revision; this would be used by an engineer that they'd like feedback from a particular reviewer that is listed on a revision.) Moved to T4226

Revisions and Commits

rP Phabricator
D7757rP6b1ec35cf377 Allow Herald rules to check for revisions with no reviewers

Event Timeline

wotte created this task.Dec 11 2013, 2:37 PM
wotte raised the priority of this task from to Needs Triage.
wotte updated the task description. (Show Details)
wotte added projects: Herald, Differential.
wotte updated the task description. (Show Details)
wotte added a subscriber: wotte.
epriestley added a subscriber: epriestley.Dec 11 2013, 3:05 PM

T2543 meanders around but eventually discusses a "Draft" or "In Preparation" phase, which I think would be analogous to your "Work in Progress" phase in (1). This likely remains blocked until T2222.

This phase would not trigger Herald rules, so that should solve the "no triggers until phase exit" part of (2). For the other part of (2), we should allow you to apply the "exist" and "do not exist" conditions to the "Reviewers" field, so you can write [Reviewers] [do not exist] to detect no reviewers (maybe we can find a slightly more natural phrasing).

I'm very hesitant to build (3) into the product. We went down the "nagging" route a bit at Facebook and it seemed to create more annoyance than results. I think this would imply a lot of weird social context, too. I can file something explicit to discuss this in greater detail.

wotte updated the task description. (Show Details)Dec 11 2013, 7:23 PM
wotte updated the task description. (Show Details)Dec 11 2013, 7:26 PM
wotte renamed this task from Add herald rule for Differential revisions based on status to Allow Herald rules to apply the "exist" and "do not exist" conditions to the "Reviewers" field.Dec 11 2013, 7:31 PM
wotte added a comment.Dec 11 2013, 7:34 PM

I've edited the issue based on your feedback. I think T2543 would address 1. If the outcome if T2543 would either a) not match against Herald rules, or b) a new Herald rule was added to match against the status of a revision, T2543 would also solve most of 2.

I think matching against the empty set for Reviewers would be useful, so I've changed this issue to address that.

I've opened a separate issue T4226 to discuss pester/nag, which I think can be a useful tool.

epriestley edited this Maniphest Task.Dec 11 2013, 8:42 PM
epriestley added a comment.Dec 11 2013, 8:48 PM

(T2443 may also be of interest, vaguely.)

epriestley edited this Maniphest Task.Dec 11 2013, 10:46 PM
epriestley closed this task as Resolved.Dec 11 2013, 10:46 PM

Closed by commit rP6b1ec35cf377.

epriestley added a comment.Dec 11 2013, 10:48 PM

(2b) should be fixed at HEAD, let us know if you run into any issues with it. T2543 should happen sooner or later, but T2222 is a bit of work and doesn't have a concrete place on the roadmap yet.

Herald added a subscriber: eadler. · View Herald TranscriptSep 15 2017, 5:26 PM
Phabricator · Built by Phacility