"... needs review, but there are no reviewers" should only be triggered when there are no mailing lists, too or at least should be configurable for projects where sending the change to a list is the common request for review. Happy to provide a patch when guided appropriately :)
Description
Description
| Status | Assigned | Task | ||
|---|---|---|---|---|
| Open | None | T5786 Separate Differential reviewer warnings from the reviewers field | ||
| Open | None | T4631 Allow Differential to raise warnings on the server side via Conduit | ||
| Open | None | T6030 Allow CustomFields to specify additional options (like "required") configurable in `<application>.fields` | ||
| Wontfix | joshuaspence | T5011 Convert builtin fields in Maniphest to CustomField | ||
| Resolved | btrahan | T5245 Migrate Maniphest Projects to use edge infrastructure |
Event Timeline
Comment Actions
Oh, oops, the task title text is actually the text of the web UI warning, so maybe I'm thinking of the wrong thing.
There are two prompts:
- From the CLI, if you have no "Reviewers", you get a prompt like: "You haven't specified any reviewers. Continue anyway? [y/N]". Conceivably, you might like to disable this if the author has CC'd a mailing list.
- From the web UI, if you have no "Reviewers", you get the warning mentioned in the task title ("needs review, but there are no reviewers").
If you want to get rid of the former prompt (or replace it with a more tailored prompt which mentions mailing lists), the best way forward is to move all the prompting to the server side (T4631) so the server sends back a list of warnings and arc requires the user to confirm them.
For the latter, there isn't a super clear way forward. This behavior is part of the "Reviewers" field:
The "Reviewers" field can't be disabled because Differential needs it in order to function. Some possible approaches:
- You can hard-code a different behavior in that field. Easy, but local fork.
- We can let you disable "Reviewers" and you can create an entirely new field with similar behavior. Yuck and yuck.
- We can separate the warning into a different "ReviewersWarningField", so it can be disabled separately. Then you can implement a new custom field which provides a tailored warning. I think this is probably the best approach unless the local fork isn't a big deal? I'm a little hesitant about splitting fields which are relatively cohesive into a lot of small bits, but this doesn't seem too bad and we'd get a total of ~4-5 new fields if this was carried to an extreme, not 100.
If you want to pursue that:
- Add a new DifferentialField, like "DifferentialNoReviewersWarningField", which just implements the warning and has no edit/storage behavior.
- Move the warning logic there.
Then, on your install:
- Disable the field.
- Enable a new field you write which has more tailored warning text.
(We could also try to make mailing lists be "real" reviewers, but that seems bad/complicated/messy for a large number of reasons.)
Comment Actions
After T4631, we could put the CLI prompt in this class too, and your alternate implementation could provide a more tailored CLI warning.
Comment Actions
There may be a better pathway forward for issuing the actual prompt after T10329, depending on how that pans out.