Page MenuHomePhabricator

Send code reviews to non-Phabricator users
Closed, WontfixPublic


We've got an existing 500 dev organization. People are slowly signing up. The process is, "hey, I want to send you a code review in Differential. Have you signed up yet?" If the answer is no, you have to wait for them to sign up and get their username.

It would be nice to be able to send a code review to someone who has not yet signed up. Probably by email address. That would just send them an invite.


Event Timeline

This is desirable, but a bit involved, particularly in the cluster.

T1205 is the general entry point for "grey users", who are users we know to exist through some discovered artifact (e.g., an email address, or an account in another connected external system). These users sort of exist today and are used to some degree by inbound email handling, but haven't been fleshed out much beyond that.

One general concern is that typoing an email address (e.g., instead of shouldn't invite random external users without involving administrators, but we could mandate be set and addresses match before inviting users, or otherwise add configuration to provide control over this.

This is moderately more involved in the cluster because it's possible that a user may click an invite link to an instance, be shipped to admin for registration, register with a different address, and then either be unable to login to their instance or fail to associate with their existing "grey" account after login. These situations are remediable but probably very confusing. There are a couple of ways we could handle this, but I don't immediately see a particularly clean approach that keeps admin/instance concerns highly separated.

Overall, this is something we do want to pursue but it will take more than an afternoon to build. Let me think about how we can handle the particulars of the invite flow in the cluster for a bit and see if I can come up with something reasonably clean. The state of the world in T1205 probably needs some consolidation, too.

I'm not familiar with @oliver.fisher's case or the numerous situations that Phabricator would need to account for, but this problem sounded relate-able to ones we've come across in the past.

In most cases the problem unfolds to having the cluster delegate authentication to a site-specific LDAP/AD. We've looked at the possibility of running a trusted service local to the site which the cluster would connect to in order to delegate (or depending on connection concerns, periodically update the cluster). This would require having some form of "domains" specified in the cluster which primarily defines which authentication is delegated to. This also means an even more complex setup that requires maintenance (and support!) across different organizations (what happens when cluster updates and authenticators are required to update as well?). Phabricator may want to solve the problem differently and I'm interested to see how this progresses.

I think that @oliver.fisher's use-case - this ticket and T9297 has more to do with initial on-boarding to Phacility/Phabricator, and bulk-creation of accounts.

If I understand correctly, if Oliver could send a list of 500 email addresses and account names, and have Phacility just create all these accounts created (And send a "reset password" email), instead of having each user register on their own, then they wouldn't be hitting this need.

(Regardless, this feature has merit on it's own).

chad renamed this task from Send code reviews to non-Phabricator users. to Send code reviews to non-Phabricator users.Sep 1 2015, 3:01 AM

You can just type 500 email addresses into this box (and, at least locally, it works fine, although admittedly I haven't done it in production personally):

Screen Shot 2015-08-31 at 8.02.13 PM.png (1×1 px, 126 KB)

...but I think the issue here is not wanting to invite everyone all at once in a giant bulk action, since there are a lot of advantages to being able to onboard teams organically if you haven't already adopted Phabricator.

Yep, @epriestley has nailed the scenario exactly. I'd like use of Phabricator to spread organically (hopefully virally) through the org. That makes user commitment to it much stronger - and it gives me a measure of how enthusiastic people are about the tools and whether we should commit to it.

Ignoring the actual invite workflow for now, the major technical issue with supporting this is handling the eventual merging of accounts.

If we knew that each email address was unique and would eventually correspond to zero or one account, we could just allocate a real User record, put a flag on the object to note that it's a placeholder, allocate a real User PHID, and everything else would be straightforward. We'd use the user PHID everywhere and eventually it would become a real user record, and in the meantime the amount of code we'd need to adjust to handle it would be small (mostly rendering code).

However, there is no such guarantee. Users can invite and, and both addresses may be the same user and eventually claimed by the same account. No matter what we did when we learned about these addresses, we definitely wrote two different PHIDs into a lot of places in the database to track their activity, and those PHIDs are now substantially equivalent. This is largely the same as Alice signing up with alicea and aanderson and later deciding she wants to merge the two accounts.

In particular, when a user searches for revisions where alice is, say, a reviewer, they now really want revisions where alice or either of the two other email addresses are reviewers. This seems very difficult to implement comprehensively. We have an enormous amount of code which assumes PHIDs are scalars, and code like this would no longer work:

if ($alice->getPHID() === $reviewer->getPHID()) {
  // ...

Instead, it would need to be rewritten as something like:

if ($alice->ownsPHID($reviewer->getPHID())) {
  // ...

While this is technically possible, it would be hugely involved and highly error prone (the "wrong" code still works most of the time, and all the time on pathways that are regularly used or tested). We would also need to rewrite lots of database queries, and anything where we group by a user (e.g., Maniphest "Group by Assignee") would now be grouping by membership in lists. In some cases (where we rely on GROUP BY in the database), we'd no longer be able to use keys if we execute this query naively.

An alternative is to try to rewrite all the PHIDs in the database as soon as Alice signs up, replacing the external account PHIDs with the new user PHID. If these PHIDs are otherwise fairly normal PHIDs and can appear anywhere that user PHIDs might, this feels similarly impractical and hugely error prone, and also destroys data if implemented naively. I discuss this briefly in T1205, but with a different set of assumptions (that we don't really link accounts and this is just a convenience: probably fine in the inbound mail case, but not really OK in the reviewer case).

Another consideration is that if merges are possible, merging can create otherwise impossible states. For example, you can't normally be an author and reviewer of a revision, but if you author a revision, add an email address as a reviewer, and then merge that address into your account, you now are. These situations are rare, but potentially serious, and especially difficult to reason about or handle correctly.

Generally, I don't see a reasonable way to resolve this "account merge" problem cleanly in the general case where we let grey accounts behave in a user-like way.

To step back, the driving cases here in general are:

  1. Organic growth in an organization by letting users invite other users (here, and mentioned in T1205).
  2. Inbound mail from non-users (some discussion in T1205).
  3. Many future use cases in Nuance (T8783).

The stuff in case (3) doesn't really overlap here. Nuance has separate external identities may link them, but does not (and should not) merge them: if we import a tweet, we shouldn't respond by email even if we know how to link someone's Twitter account to their email address (at least, we shouldn't do this by default, and should't do it automatically or destructively -- letting a human operator choose to do this would be reasonable). These sources may be related, but they are not equivalent.

The stuff in case (2) probably doesn't matter much either. I think it's mostly a mixture of stuff that should eventually move to Nuance (like a bugs@ address where everyone in the org sends bugs) and moot cases like automated tools dumping stuff into Maniphest, where merging and identity aren't important and the originator is never going to be a real user.

So we're largely just left with (1). I think we can potentially attack that differently and just skip all the hard merging problems.

Specifically, suppose we:

  • Allow access to the invite tool to be set to "All Users".
  • Tailor the workflow to better accommodate user-to-user invites (e.g., allow multiple invites, tweak copy a bit -- there are some abuse cases here we need to be a little bit careful about).
  • Provide a default "Invites" dashboard panel with an easy way to invite other users.
  • Provide an "Invite users to this object..." action on relevant objects (revisions + tasks + commits?). We'd record the objects a user was invited to.

So existing users would see:

  • "Invite" button on home page.
  • Status of their pending invites (see who has accepted and who hasn't responded).
  • "Invite" button on relevant objects.

When users accepted an invite, they'd see:

  • List of objects they'd been invited to look at on their home page, and who wanted them to look at the stuff.
  • List of users who invited them or whatever if this feels reasonable.

This isn't quite as clean as letting you type Reviewers: or type an email address directly into "Subscribers:" but is better in some ways: it's explicit, it's observable, the inviter can provide a heartfelt message, the invitee can arrive to a tailored list of stuff relevant to them. And it's far more technically feasible, since it doesn't require us to ever merge anything.

The explicitness and context we can provide because the actions are more structured feel good to me. The technical feasibility also feels good to me.

Not being able to type email addresses into web UI typeaheads feels less-good, but we could actually make that work fairly naturally after T9132. Not being able to type email addresses into the CLI in arc diff also doesn't feel great, but we could consider exposing an "Invite:" field there which accepted addresses, or treating addresses as invites, although both feel a little awkward.

Conceivably, we could special case this to make it work, flag these invitees as implicit reviewers, and add them when they sign up. This is messy but doesn't seem completely unreasonable. We could auto-CC other invitees on signup too. I'm less excited about pursuing this (in particular, it makes some of the logic in cases like T731 even more complex, since we must now track potential-reviewers-who-are-being-invited: for example, we would not want to warn you that you didn't specify any reviewers, even though the revision will technically not have any reviewers yet), but it feels tractable and relatively practical in a way that letting these users join the general population and then trying to replace a bunch of PHIDs later does not.

Overall, these tradeoffs feel fairly reasonable to me, and I think they give us sustainable and maintainable (albeit complex) ways to move forward if the things we're trading away prove to be costly.

Some other miscellaneous thoughts:

  • Users might invite another user to an object the recipient can not see. We could probably handle this by showing "alice invited you to look at 6 more objects you can't see yet" on the home page and letting users figure this out.
  • Established users may eventually want to turn off "invite mode" if everyone they care about has already signed up. If nothing else, we could give them a preference.
  • We could track whose invite you accepted and make you eternally beholden to their whims.
  • This still leaves the instance/admin case unresolved in the Phacility cluster, but reduces the problem to an existing problem which I'm not particularly worried about -- this challenge in attacking this is mostly in making the solution robust and not making a huge mess of proprietary junk in the open source upstream in doing so, not in any real technical or product difficulty.
eadler added a project: Restricted Project.Jan 11 2016, 9:36 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jan 11 2016, 9:42 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Feb 18 2016, 6:32 PM
eadler removed a project: Restricted Project.Feb 18 2016, 6:35 PM
epriestley claimed this task.

The driving use case for this fell through, and I no longer plan to pursue it.