Page MenuHomePhabricator

Modernize PhortuneAccount with EditEngine/Modular Transactions
ClosedPublic

Authored by chad on Mar 30 2017, 5:52 PM.
Tags
None
Referenced Files
F14099964: D17585.id.diff
Tue, Nov 26, 1:04 PM
F14094734: D17585.diff
Mon, Nov 25, 5:00 PM
Unknown Object (File)
Sat, Nov 23, 4:41 AM
Unknown Object (File)
Fri, Nov 22, 1:39 AM
Unknown Object (File)
Sun, Nov 17, 10:37 PM
Unknown Object (File)
Wed, Nov 13, 11:07 PM
Unknown Object (File)
Sun, Nov 10, 12:17 PM
Unknown Object (File)
Wed, Nov 6, 7:26 PM
Subscribers

Details

Summary

This updates the backend of PhortuneAccount to use EditEngine and Modular Transactions and updates language to "account manager" for clarity of role.

Test Plan
  • Wiped phortune_account table
  • Visit Phortune, see new account automatically created.
  • Edit name and managers
  • Try to set no name or remove myself as a manager, get error messages
  • Visit /phortune/ and create another new account

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley added inline comments.
src/applications/phortune/editor/PhortuneAccountEditEngine.php
90–91

Probably fine to drop "member" stuff, there shouldn't be any backward compatibility issues.

src/applications/phortune/editor/PhortuneAccountEditor.php
20

You should be able to get rid of this (ModularTransactions should figure it out automatically.)

60–71

Does this prevent you from creating an account without yourself as a manager?

src/applications/phortune/storage/PhortuneAccountTransaction.php
18–27

I don't want to add new mailtags -- they make T10448 harder and I don't think we get anything out of them in this case.

src/applications/phortune/xaction/PhortuneAccountNameTransaction.php
38–39

$errors should just be a list, without keys ('name', 'length').

One issue we might run into with named keys like this is that it's possible that two different fields could both raise length errors. If they did, they might overwrite one another.

This revision is now accepted and ready to land.Mar 30 2017, 7:23 PM

So I don't need mailtags to send mail? Specifically I think we should send mail if people add new managers, change contact info, yada yada, will that just work?

I don't think mail needs to have any mail tags. If it does, we could remove that requirement to simplify the eventual implementation of T10448. We sent plenty of mail with no tags, though (password reset, email verify, etc).

chad marked 4 inline comments as done.Mar 30 2017, 8:04 PM
chad added inline comments.
src/applications/phortune/xaction/PhortuneAccountNameTransaction.php
38–39

It prints out the error twice, I'll take another look.

chad marked an inline comment as done.Mar 30 2017, 8:30 PM
chad added inline comments.
src/applications/phortune/editor/PhortuneAccountEditor.php
60–71

Uh no, also can't seem to find a good fix for that.

$old = $xaction->getOldValue(); doesn't seem to return anything, ever.

ok I found a better workaround I think. Tested creation with 0 members, adding 1, adding 1 + removing 1.

chad requested review of this revision.Mar 30 2017, 8:33 PM
chad edited edge metadata.

This ended up in my "Waiting on Other Reviewers" bucket, which looks like yet another bug.

src/applications/phortune/storage/PhortuneAccount.php
98

Probably fine to call this just getURI(), that's the behavior of other getURI() methods.

This revision is now accepted and ready to land.Apr 11 2017, 6:34 PM
This revision was automatically updated to reflect the committed changes.