Page MenuHomePhabricator

Move user approval to modular transactions
ClosedPublic

Authored by amckinley on Dec 12 2018, 11:50 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 22, 1:44 PM
Unknown Object (File)
Sun, Dec 22, 7:12 AM
Unknown Object (File)
Fri, Dec 20, 9:45 PM
Unknown Object (File)
Tue, Dec 17, 8:19 PM
Unknown Object (File)
Tue, Dec 17, 10:27 AM
Unknown Object (File)
Mon, Dec 16, 11:42 PM
Unknown Object (File)
Wed, Dec 11, 12:22 AM
Unknown Object (File)
Wed, Dec 4, 11:25 PM
Subscribers
Tokens
"Like" token, awarded by avivey.

Details

Summary

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.

Test Plan

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

Diff Detail

Repository
rP Phabricator
Branch
user-approve (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 21318
Build 29008: Run Core Tests
Build 29007: arc lint + arc unit

Event Timeline

src/applications/people/storage/PhabricatorUser.php
1243 ↗(On Diff #47465)

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

src/applications/people/xaction/PhabricatorUserApproveTransaction.php
23–26

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?

src/applications/people/xaction/PhabricatorUserApproveTransaction.php
23–26

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.

src/applications/people/xaction/PhabricatorUserApproveTransaction.php
23–26

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.

Looks great to me except for the permissions juggling, try this inline?

src/applications/people/xaction/PhabricatorUserApproveTransaction.php
87

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.

Override requireCapabilities to correctly check permissions.

This revision is now accepted and ready to land.Dec 13 2018, 12:07 AM

Move logging in PhabricatorUserDisableTransaction.

This revision was automatically updated to reflect the committed changes.