I think that projects which are created solely for the purposes of access control (e.g. Blessed Reviewers and Blessed Committers) should have a limited visibility within the UI.
As an example, I don't think users should be able to assign tickets to these projects. T390 is probably related.
Does cause any material harm?
What if I create a task like "Add @alincoln to Blessed Reviewers"?
I think it's potentially reasonable to have a flag which prevents a project from being used in a "Projects:" field, but at the least this will sometimes be confusing ("why don't all the projects show up?", "do you have 'don't show up' checked?", "oops, yeah"). This used to happen a lot with disabled users and bots before we made them always show up, just not until you have no other options. I'd like to have a stronger justification for this than I've currently seen (e.g., we haven't seen any project abuse on this install -- are you seeing it locally)?
Being a member of Blessed Reviewers doesnt imply that you can add new members. We have an "Access" project for this purpose.
We haven't seen any abuse, no. I guess it just makes it easier to users to know what project to assign to. Especially if we had projects like "Some Project", " Users That Can Commit to Some Project " and "Users Who Can Review Code for Some Project".
Out of curiosity, would setting the visibility of a project to "No-one" have the same effect?
Being a member of Blessed Reviewers doesnt imply that you can add new members.
Oh, I just mean that it's conceivable that tasks could actually be related to these kinds of projects. For example, they might be administrative tasks. The scenario I'm imagining is something like, say, we have a new hire, and whoever is onboarding them wants them to have commit access, but does not have permission to add members to the project. So they assign me a task "Add @alincoln to Blessed Reviewers" to get commit access for the new hire. It seems like it would be reasonable to tag this task with the Blessed Reviewers project.
Out of curiosity, would setting the visibility of a project to "No-one" have the same effect?
Mostly, but it would make it more difficult to understand who needs to review your revision when Herald triggers a project review, because you'd see "Restricted Project" instead of "Blessed Reviewers". So, at least in this case, it would probably be a net negative for usability. There might be other cases where it's appropriate (e.g., if a project is really an internal policy group, and does not get triggered as a reviewer). Blessed Committers wouldn't have any issues, for example, although I think it's nice to be able to check who has commit access.
I suppose that differentials has a stronger case than maniphests. As in, there are some projects which don't make any sense to be specified as code reviewers.
Yeah, we could have a whole list of options:
...etc., but I only want to add those if there's a real material justification for them, since otherwise it's a lot of complexity for not-necessarily-a-lot-of-benefit. I generally worry that adding these rules will make the interactions harder to understand too, and that mitigating the confusion will further add complexity. If users don't actually have much trouble using projects appropriately in practice, we could skip all of this mess.
This has been open for about 18 months without anyone coming up with any really concrete use cases to motivate any of these flags, so I'm just going to close it out until such a motivation develops and this becomes non-hypothetical. I haven't seen any misuse here personally in the upstream, and we haven't heard too much feedback about this particularly, as opposed to some general difficulty finding the right projects in typeaheads, particularly for newer users.
Some possible related work:
@epriestley The FreeBSD install is seeing a lot of confusion around this. We have some groups used for policy ("all committers", "all people with access to this repo") which are getting added to reviews. This causes the main dashboard to be useless and likely causes people to filter from phabricator.
That would be helpful and solve the entire problem if emails were completed post-transaction (i.e., one didn't get an email for being a reviewer and then another for being removed). I'd love to add a Herald rule that says 'if any of these projects are added, remove them'.
Overall my concern is that this is the kind of feature that causes more problems than issues it resolves. It makes Phabricator much harder to build, to test, to document. It makes it harder for users to configure and understand, and it causes product confusion to the end user. Pretty much any workaround is going to be preferable, unfortunately.
I agree. This feature is suboptimal from a complexity and product point of view. Personally, I'd love to see much more powerful Herald system than specific features added to solve smaller use cases. I wasn't trying to ask you to implement this feature so much as I was trying to say that the underlying problem is real and ought to be solved.
Do you know why users are adding "All Committers" as "Reviewers"? We haven't seen that on this install, but maybe your flow is different somehow?
That is, if you ask one of the users who did this why they did it, what do they say? What were they hoping to do?
For example, here's a possible sequence of events which might be leading to this:
If so, one possible approach might be giving most users a form without a Reviewers field using EditEngine, and having Herald or some Herald+Humans system assign appropriate reviewers, similar to how "New Bug Report" on this install is tailored for new users and doesn't let them assign things.
That won't work until Differential gets updated for EditEngine, but might be a solution to the problem that doesn't need weird one-off flags on Projects, if the reasoning above is roughly the process users are actually using to arrive at the destination of "Reviewers: everyone ever".
I suspect its a mixture of history and expectations since Phabricator is used solely as a review system and not as a primary communication mechanism.
From a quick straw poll of people that have done this recently, it seems that most people want "someone" to review their patches but don't realize that casting such a wide net harms their cause. Ideally we can create smaller, more focused, groups that people add too. We started on this ("python ports reviewers").
We may try the EditEngine approach too, I'll bring it up internally.