Move user renames to modular transactions
ClosedPublic
Actions

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

Details

Reviewers

epriestley

Commits

rPd23cc4b862aa: Move user renames to modular transactions

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

Branch

master

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 21338
Build 29038: Run Core Tests
Build 29037: arc lint + arc unit

Event Timeline

amckinley created this revision.Dec 14 2018, 12:18 AM

Harbormaster completed remote builds in B21338: Diff 47487.Dec 14 2018, 12:22 AM

amckinley requested review of this revision.Dec 14 2018, 12:22 AM

Herald added a subscriber: Korvin. · View Herald TranscriptDec 14 2018, 12:22 AM

amckinley added inline comments.Dec 14 2018, 12:23 AM

src/applications/people/controller/PhabricatorPeopleRenameController.php
87	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
81	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	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
62–64	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
87	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.