Page MenuHomePhabricator

Add participants ModularTransactions to Conpherence
ClosedPublic

Authored by epriestley on Apr 13 2017, 10:29 PM.
Tags
None
Referenced Files
F14120937: D17685.id42557.diff
Fri, Nov 29, 10:49 AM
F14119098: D17685.id42650.diff
Fri, Nov 29, 3:15 AM
Unknown Object (File)
Mon, Nov 25, 12:03 AM
Unknown Object (File)
Thu, Nov 21, 12:50 PM
Unknown Object (File)
Wed, Nov 20, 6:49 PM
Unknown Object (File)
Mon, Nov 18, 8:38 PM
Unknown Object (File)
Sun, Nov 17, 3:07 PM
Unknown Object (File)
Sun, Nov 17, 10:08 AM
Subscribers

Details

Summary

Moves participants over to ModularTransactions, simplified a lot of the code. Fixes T12550

Test Plan

Create a new room with just myself and myself + fake accounts.
Remove a person.
Remove myself.
Edit a room, topic.
Type some messages.
???
Profit

Diff Detail

Repository
rP Phabricator
Branch
conph-xaction-2 (branched from master)
Lint
Lint Passed
Unit
Tests Skipped
Build Status
Buildable 16490
Build 21957: Run Core Tests
Build 21956: arc lint + arc unit

Unit TestsFailed

TimeTest
29 msConpherenceRoomTestCase::Unknown Unit Message ("")
EXCEPTION (RuntimeException): Undefined variable: old #0 /core/data/drydock/workingcopy-83/repo/phabricator/src/applications/transactions/storage/PhabricatorModularTransactionType.php(313): PhutilErrorHandler::handleError(8, 'Undefined varia...', '/core/data/dryd...', 313, Array) #1 /core/data/drydock/workingcopy-83/repo/phabricator/src/applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php(87): PhabricatorModularTransactionType->getAddedPHIDList()
39 msConpherenceRoomTestCase::Unknown Unit Message ("")
EXCEPTION (RuntimeException): Undefined variable: old #0 /core/data/drydock/workingcopy-83/repo/phabricator/src/applications/transactions/storage/PhabricatorModularTransactionType.php(313): PhutilErrorHandler::handleError(8, 'Undefined varia...', '/core/data/dryd...', 313, Array) #1 /core/data/drydock/workingcopy-83/repo/phabricator/src/applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php(87): PhabricatorModularTransactionType->getAddedPHIDList()
39 msConpherenceRoomTestCase::Unknown Unit Message ("")
EXCEPTION (RuntimeException): Undefined variable: old #0 /core/data/drydock/workingcopy-83/repo/phabricator/src/applications/transactions/storage/PhabricatorModularTransactionType.php(313): PhutilErrorHandler::handleError(8, 'Undefined varia...', '/core/data/dryd...', 313, Array) #1 /core/data/drydock/workingcopy-83/repo/phabricator/src/applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php(87): PhabricatorModularTransactionType->getAddedPHIDList()
43 msConpherenceRoomTestCase::Unknown Unit Message ("")
EXCEPTION (RuntimeException): Undefined variable: old #0 /core/data/drydock/workingcopy-83/repo/phabricator/src/applications/transactions/storage/PhabricatorModularTransactionType.php(313): PhutilErrorHandler::handleError(8, 'Undefined varia...', '/core/data/dryd...', 313, Array) #1 /core/data/drydock/workingcopy-83/repo/phabricator/src/applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php(87): PhabricatorModularTransactionType->getAddedPHIDList()
1 msAlmanacNamesTestCase::Unknown Unit Message ("")
30 assertions passed.
View Full Test Results (4 Failed · 332 Passed)

Event Timeline

src/applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php
41

Not sure how / where to grab a current transaction PHID

  • less broken, just a half diff
  • everything i think workypoos
chad edited the test plan for this revision. (Show Details)
src/applications/conpherence/editor/ConpherenceEditor.php
195

Can these be moved?

src/applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php
39

Don't know how to set this in Modular.

To get the current transaction PHID, you can do something like this on ModularTransactionType:

final protected function getCurrentTransactionPHID(() {
  $xaction_phid = $this->getStorage()->getPHID();
  if (!$xaction_phid) {
    throw new Exception(pht('Current transaction does not yet have a PHID yet!'));
  }
  return $xaction_phid;
}

It's possible that will throw. If it does, you can do this instead:

final protected function getCurrentTransactionPHID(() {
  $storage = $this->getStorage();
  
  $xaction_phid = $storage->getPHID();
  if (!$xaction_phid) {
    $xaction_phid = $storage->generatePHID();
    $storage->setPHID($xaction_phid);
  }
  
  return $xaction_phid;
}

That's a bit more magic but I think it's still OK and should work.


As written, I think this change breaks all the rendering of historic transactions? It also doesn't support the '=' action, which is OK for now, but if it's later exposed via EditEngine the controls there will think that = is supported.

Possibly better is:

  • Keep oldValue as a list of old participants.
  • Keep newValue as a list of new participants.
  • Put a method on ModularTransactionType that calls $this->getEditor()->getPHIDTransactionNewValue() to make the method callable from ModularTransactions and handle converting - / + / = into a list of PHIDs.
  • Use it to generate a list of PHIDs for the newValue.
  • Keep applyExternalEffects, getTitle() and validateTransactions() based around lists of PHIDs.

This should also handle cases like array('-' => array('A'), '+' => array('A')) (adding and removing the same user in one transaction) correctly, but I think the current code has less-ideal handling of these weird edge cases.

src/applications/conpherence/editor/ConpherenceEditor.php
195

Not currently. They're sort of obsoleted by EditEngine and I think I want to get rid of them completely, but probably best to just leave them here for now and we'll figure this out later.

src/applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php
38

Since we're touching this code, maybe prefer PhabricatorTime::getNow().

chad planned changes to this revision.Apr 14 2017, 4:43 PM

EXCEPTION: (Error) Call to a member function generatePHID() on null at [<phabricator>/src/applications/transactions/storage/PhabricatorModularTransactionType.php:315]

Seems like getStorage is null here.

This is getting pretty far into sketchy territory but you could do this, in PhabricatorApplicationTransactionEditor->applyExternalEffects():

    $xtype = $this->getModularTransactionType($type);
    if ($xtype) {
+     $xtype = clone $xtype;
+     $xtype->setStorage($xaction);
      return $xtype->applyExternalEffects($object, $xaction->getNewValue());
    }

Or just leave this as PHID-VOID-0000 now and I'll counter-diff you some less questionable version of this after it lands?

I think we overwrite this PHID anyway in applyFinalEffects.

Yeah my sneaky plan for a good version of this is like:

  • Set it to null here or something like that.
  • If it's null when we get to applyFinalEffects(), always set it to the last transaction PHID.

Not sure if that'd work but then we wouldn't have to do anything fancy here.

getPHIDTransactionNewValue takes a transaction, which.... still having problems getting

I'll upload what I have but Im stuck

epriestley edited reviewers, added: chad; removed: epriestley.

I have to go pick up the pup in a little bit but let me see if I can steal all your internet points first

Somewhat works? Gotta go get a pupper.

I don't really know what applyFinalEffects() is actually doing so I probably need to go look at the code a bit before I can fix that, but I think the bits other than that sorta work.

It updates the participants that they have a new unseen message

(Still working on this...)

Somewhat works? Gotta go get a pupper.

Did you retrieve pupper at least?

Yes! I found a very nice pupper at a bargain rate!

  • Now with simplified logic.
  • Restore missing cache clear.
chad added inline comments.
src/applications/transactions/storage/PhabricatorModularTransactionType.php
312–317

so clean.

This revision is now accepted and ready to land.Apr 19 2017, 8:48 PM