Page MenuHomePhabricator

Improve workflow when users who do not have permission to see a revision are added as reviewers
Open, NormalPublic

Description

See PHI1987. When a user who can not see a revision is added to that revision as a reviewer, there's no workflow feedback about this. This can lead to revisions sitting unreviewed indefinitely because no reviewer can actually see them.

Previously, see T4411, which discusses some similar concerns with subscribers.

The desired workflow here isn't unambiguous, but in all cases it has this general shape:

  • The user making the edit (adding the subscribers or reviewers) is prompted with some sort of warning, error, or choice about how to proceed.
  • Possibly, they are given an option to share the object with the users who don't have permission to see it.
  • When a user related to an object can't see it, the UI clearly shows that they can't see it.

The various avenues here have some product tradeoffs, but they are primarily shaped by technical concerns. In particular:

UI: This one is fairly unambiguous, and "Mentions" in remarkup already have a disabled state for users who can't see the object.

  • Reviewers who can't see the revision should be indicated clearly.
  • Subscribers who can't see the object should be indicated clearly.

Typeaheads: This starts to become trickier. If you use the web UI to typeahead a reviewer or subscriber, it would be nice to warn you that they can't see the object. However, this might have some significant performance implications and will naively show the wrong result in many cases if you have also adjusted the "Visible To" policy. Mention typeaheads could do this more safely, but currently don't (at least, not in all cases: Support issues don't show the right state).

  • Mention typeaheads should show viewer state unless performance is hugely prohibitive.
  • Other typeaheads should probably (?) show viewer state.

Submitting Web Edits (Forms): When you submit an edit via the web UI (for normal edit forms), and are trying to add new users who can't see the object, you should minimally be prompted to confirm that you want to apply this kind of edit ("such-and-such can't see this object, add them as subscribers/reviewers anyway?").

That is, these options should be available:

  • Cancel (go back and revise the edit)
  • Submit (add users who can't currently see the object).

Possibly, they should be given a choice to punch through policies, too:

  • Expose (add users who can't currently see the object, and also let them see the object).

Access to the "Expose" action may be somewhat contentious in some cases, but it reduces approximately to taking a screenshot of the page and emailing it, so it's not really a new capability. Still, it may be appropriate to restrict.

Submitting Web Edits (Comments/Actions): When you submit comments that mention users who can't see the object, you should get a similar prompt. Subscriber/reviewer actions should otherwise work the same way.

Submitting Edits (CLI): Ideally, "arc diff" should give you an equivalent prompt if you select users who don't have view permission.

Other Edits: Other edits are possible, e.g. via Herald, the API, writing directly to the database, etc. In these cases, I suspect they should generally just go through. Herald could get an "Add subscribers and expose" action or similar if necessary. The API could get a separate "expose" action.

Policy Changes: When any of these workflows cause policy changes which hide the object from some related users who could previously see it, similar prompts would also be desirable. There are some limits to how much we can do here (if you remove Alice from a group, it doesn't make sense to prompt you that she'll lose access to 9 million tasks) but this should be tractable in the most common cases.


I generally favor maintaining the "Exposure" / "Shared With..." list as a concept separate from subscribers/reviewers/etc. This likely simplifies a lot of behavior and makes how the mechanism works easier to understand and implement.

This implies that subscribers/reviewers without view access are a regular state, which seems reasonable. In particular, if you remove Alice as a member of a project which grants her access to objects, the expected behavior is generally that she loses access to those objects. She shouldn't get to retain access just because she subscribed to some, or was added as a reviewer, or whatever else. But it's also impractical (and would be wildly noisy/disruptive, and difficult to undo in the event of a mistake) to go remove all those relationships.

So that leaves us with:

  • An object having related users who don't have permission to see that object is an expected state after certain edits (like removing a user as a project member, or revoking their access to a Space).
  • Objects should clearly show when related users don't have access to them.
  • Edits which will add relationships to users who can't see the object should, to the greatest degree possible, warn the user.

And then possibly:

  • Objects may maintain a separate "Exposure" list which pierces policies (but probably not Spaces).
  • Prompts about adding users who don't have view permission may offer to add them to the exposure list.

Event Timeline

epriestley triaged this task as Normal priority.Thu, Feb 4, 10:15 PM
epriestley created this task.
  • Context objects don't make it into timeline rendering engines.
  • Context objects don't make it into comment previews.
  • When rendering a "no view permission" hovercard, it would be nice to annotate it with an explicit "The user can't see this object" piece of context information.

See also T13068, which suggests rendering mentions in a special style when the user has muted the object.

This is ripe for implementation technically now, but I have some reservations reservation is that it exposes "Mute" status to other users, and this is currently a private action. We could perhaps imagine some drama arising from users muting things that other users think are very important, etc., and interpreting the action as "Snub".

Since "Mute" has (last time I checked) a non-zero but very-low use rate and no users have actually raised this as an issue (although they might not be aware of it, since there's no way to figure out that someone has muted something right now) I'm inclined not to pursue any change here. If evidence arises that "Mute" leads to a lot of confusion I'd be open to reconsidering, but I might also just remove "Mute" since I think it's a fairly low-value feature and only worth retaining if it generally doesn't cause any confusion.