See https://discourse.phabricator-community.org/t/how-to-approve-user-via-conduit-api/2189. This particular use case doesn't seem very compelling, but moving this logic out of PhabricatorUserEditor is a win anyway.
- rPaba99459238f: Move user approval to modular transactions
Registered a new user, approved/unapproved them conduit, approved from the UI.
In PhabricatorUserDisableTransaction, this code was inside applyInternalEffects, but it feels more like an external effect to me. Or does putting it inside applyInternalEffects also wrap the whole thing in a transaction, which we still want?
Yeah, I think it's "internal" since it's on the same database, uh, maybe. The names aren't great nowadays (and perhaps never were?), more like "apply inside-transaction changes" and "apply outside-transaction changes", except the transaction is held through both anyway I'm pretty sure? I'm not sure there's actually any distinction at all anymore. I don't think it matters in any realistic scenario. We should possibly just merge them. Let me see if I can find any reason not to, offhand.
Okay, so sequence is:
Normally, save() generates a PHID, so I think the long-ago rationale was:
But this is muddy because we can generate a PHID at any time, and sometimes do.
Another case is that save() may occasionally fail with a duplicate key collision that we couldn't (or didn't) detect beforehand. It makes sense to be sure we actually saved the PHID before we start writing rows into other databases.
So I guess this distinction is probably worth preserving, but all the rules are very muddy. This is probably an "external" effect from the point of view of "it relies on the user PHID" so I think your implementation is more correct.
Also note that this is a slight behavior change, because it is now possible to "unapprove" an already-approved user (primarily by using Conduit). The effects of this change are left as an exercise for the reader, but PhabricatorPeopleDisableController, for example, won't let you disable a user that has already been approved.
Looks great to me except for the permissions juggling, try this inline?
You should be able to implement getRequiredCapabilities() (like PhabricatorUserDisableTransaction) to get rid of the need for CAN_EDIT.
Since validateTransactions() checks admin, it's okay to remove the required capabilities check. That will let you get rid of the "obviously wrong" change above.
You could also move the log in Disable if you want. I'm not sure anyone's going to approve or disable a user while creating them (and you can't create via the API today anyway) but I think this implementation allows it and the other one doesn't necessarily.
The effects of this change are left as an exercise for the reader, but PhabricatorPeopleDisableController, for example, won't let you disable a user that has already been approved.
You can still disable them from their profile page (the "Disable" workflow), just not through the "Disapprove" workflow (from the "Approve" queue page). This is kind of a gotcha but separate "disapproved" and "disabled" states that have different meanings (or, worse, don't have different meanings) seems like more of a gotcha.