Page MenuHomePhabricator

Consider flags to disable project features (can tag, can have members, can be reviewer...)
Closed, WontfixPublic

Description

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.

Event Timeline

joshuaspence raised the priority of this task from to Needs Triage.
joshuaspence updated the task description. (Show Details)
joshuaspence added a project: Projects.
joshuaspence added a subscriber: joshuaspence.

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:

  • Project can be tagged on objects.
  • Project can be a reviewer.
  • Project can be an auditor.
  • Project can be a subscriber.
  • Project can have members.

...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.

Yeah that all sounds reasonable.

epriestley renamed this task from Limit visibility of ACL projects to Consider flags to disable project features (can tag, can have members, can be reviewer...) .Jul 13 2014, 4:23 PM
epriestley triaged this task as Low priority.
eadler added a subscriber: eadler.Apr 26 2015, 6:27 AM
revi added a subscriber: revi.Jun 28 2015, 6:47 AM
epriestley closed this task as Wontfix.Jan 20 2016, 3:05 PM
epriestley claimed this task.

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:

  • Since this was filed, we added the "browse" interaction to typeaheads, so it's a bit easier to find the right projects.
  • (This might also have been filed before we showed icons, which also help?)
  • T7858 discusses expanding the browse view with additional information to address the "I have no idea what I'm looking for" case.
  • In the specific case of auditor/subscriber/reviewer, we could treat projects with no members as though they were disabled projects (show them only if there is just one exact match), but I'd like to see more specific confusion first.

@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.

chad added a subscriber: chad.Feb 14 2016, 8:54 PM

What about having a "Remove Reviewer" Herald Rule?

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'.

chad added a comment.Feb 14 2016, 9:06 PM

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.

chad added a comment.Feb 14 2016, 9:08 PM

Oh yeah it's reasonable to find another solution for you.

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:

  • FreeBSD might have more inexperienced/random contributors and more people using using the web UI, for whatever reason.
  • The web UI lets you typeahead stuff, so they just mash whatever they can or use to find stuff that seems plausible.
  • So their goal is just "I want someone to accept my stuff", and they don't know how to select reviewers, and the web UI makes it relatively easy to add a lot of unrelated random people.

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.