Page MenuHomePhabricator

Move user renames to modular transactions

Authored by amckinley on Dec 14 2018, 12:18 AM.



Cleaning up more super-old code from PhabricatorUserEditor. Also fix user logging in approve transactions. I'm not sure how it worked at all previously.

Test Plan

Created new users, renamed them, checked DB for sanity. Entered invalid names, duplicate names, and empty names, got appropriate error messages.

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 14 2018, 12:18 AM
amckinley requested review of this revision.Dec 14 2018, 12:22 AM
amckinley added inline comments.Dec 14 2018, 12:23 AM

This just kinda worked, but I figured it out by looking around at similar callers, so let me know if this old/bad/etc.

epriestley accepted this revision.Dec 14 2018, 12:27 AM
epriestley added inline comments.

I think this does the wrong thing if you do this:

  • Edit alice.
  • Try to change her name to bailey.
  • (Another user already has that name!)
  • Submit the form.
  • Expect: error, plus form value is bailey.
  • Actual?: error, plus your edit was reverted.

Or maybe more simply just type in an invalid username like @@@ BONGLORD @@@. Your creative work is discarded, I think?

24–27 ↗(On Diff #47487)

I'd expect $this->newUserLog() to work? Why doesn't it?

This revision is now accepted and ready to land.Dec 14 2018, 12:27 AM
epriestley added inline comments.Dec 14 2018, 12:30 AM

You moved this over faithfully, but the standard behavior for transactions is just to ignore no-op transactions, since they get dropped automatically and it makes some general transaction handling stuff (and some API stuff) easier (e.g., your "synchronize all the usernames" script can just make all the edits it wants to in order to get things into the right state, bulk edits won't fail mysteriously, etc). For consistency/simplicity, you could reasonably just put this near the top of the loop with if ($old === $new) { continue; } if it tickles your fancy.

epriestley added inline comments.Dec 14 2018, 12:32 AM

This is good / modern. It's sort of "less modern" than real EditEngine but we don't have a Grand Abstraction For All Weird Forms and I don't think there's one anywhere on the horizon.

amckinley updated this revision to Diff 47488.Dec 14 2018, 12:46 AM

Requested changes.

amckinley added inline comments.Dec 14 2018, 12:47 AM
24–27 ↗(On Diff #47487)

It never crossed my mind that the newUserLog implementation would be in PhabricatorUserTransactionType, but TIL.

This revision was automatically updated to reflect the committed changes.