Page MenuHomePhabricator

Move user renames to modular transactions
ClosedPublic

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

Details

Summary

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

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
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
src/applications/people/controller/PhabricatorPeopleRenameController.php
89

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.
src/applications/people/controller/PhabricatorPeopleRenameController.php
83–84

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?

src/applications/people/xaction/PhabricatorUserApproveTransaction.php
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
src/applications/people/xaction/PhabricatorUserUsernameTransaction.php
63–65

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
src/applications/people/controller/PhabricatorPeopleRenameController.php
89

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
src/applications/people/xaction/PhabricatorUserApproveTransaction.php
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.