Page MenuHomePhabricator

Move user renames to modular transactions
ClosedPublic

Authored by amckinley on Dec 14 2018, 12:18 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 21, 1:45 PM
Unknown Object (File)
Jan 22 2024, 4:05 AM
Unknown Object (File)
Jan 5 2024, 5:28 PM
Unknown Object (File)
Dec 29 2023, 9:32 AM
Unknown Object (File)
Oct 30 2023, 11:33 AM
Unknown Object (File)
Oct 30 2023, 6:48 AM
Unknown Object (File)
Oct 24 2023, 2:40 AM
Unknown Object (File)
Jul 22 2023, 9:15 AM
Subscribers

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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 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
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.

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.

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.