Page MenuHomePhabricator

Move user approval to modular transactions

Authored by amckinley on Dec 12 2018, 11:50 PM.



See This particular use case doesn't seem very compelling, but moving this logic out of PhabricatorUserEditor is a win anyway.

Test Plan

Registered a new user, approved/unapproved them conduit, approved from the UI.

Diff Detail

rP Phabricator
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

amckinley created this revision.Dec 12 2018, 11:50 PM
amckinley requested review of this revision.Dec 12 2018, 11:52 PM
amckinley added inline comments.Dec 12 2018, 11:52 PM
1243 ↗(On Diff #47465)

This is obviously wrong, but it makes everything else work.

amckinley added inline comments.Dec 12 2018, 11:53 PM

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?

epriestley added inline comments.Dec 12 2018, 11:55 PM

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.

epriestley added inline comments.Dec 13 2018, 12:00 AM

Okay, so sequence is:

  • Open transaction.
  • Internal effects.
  • save()
  • External effects.
  • Final effects.
  • Save transaction.
  • didCommitTransactions()

Normally, save() generates a PHID, so I think the long-ago rationale was:

  • Update the object itself.
  • Do a single save() with all the changes.
  • Now stuff that needs a PHID (edges, etc) can do its thing.

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.

epriestley requested changes to this revision.Dec 13 2018, 12:02 AM

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.

This revision now requires changes to proceed.Dec 13 2018, 12:02 AM

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.

amckinley updated this revision to Diff 47466.Dec 13 2018, 12:06 AM

Override requireCapabilities to correctly check permissions.

epriestley accepted this revision.Dec 13 2018, 12:07 AM
This revision is now accepted and ready to land.Dec 13 2018, 12:07 AM
amckinley updated this revision to Diff 47467.Dec 13 2018, 12:08 AM

Move logging in PhabricatorUserDisableTransaction.

Harbormaster completed remote builds in B21318: Diff 47467.
This revision was automatically updated to reflect the committed changes.