Page MenuHomePhabricator

Update Conpherence to use EditEngine
ClosedPublic

Authored by chad on Oct 6 2016, 12:25 AM.

Details

Summary

Moves Conpherence to use EditEngine. This removes the "First Message" field, but I think that's ok until we have direct messaging of some sort, then maybe have built-ins cover that case.

Test Plan
  • Visit /new/ and /edit/ for creating new rooms.
  • Edit a room in full conpherence
  • Edit a room in durable column
  • grep for METADATA calls

Diff Detail

Repository
rP Phabricator
Branch
conph-edit-engine (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 14031
Build 18202: Run Core Tests
Build 18201: arc lint + arc unit

Event Timeline

chad retitled this revision from to [wip] Conpherence EditEngine.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added a reviewer: epriestley.

Can we do builtins with EditEngine?

chad edited edge metadata.
  • Remove ConpherenceNewRoomController
chad retitled this revision from [wip] Conpherence EditEngine to Update Conpherence to use EditEngine.Oct 16 2016, 6:51 PM
chad updated this object.
chad edited the test plan for this revision. (Show Details)

TODO: Fix unit tests.

@epriestley I'm hung up on:

Invalid 'new' value for PHID transaction. Value should contain only keys '+' (add PHIDs), '-' (remove PHIDs) and '=' (set PHIDS).

It seems EditEngine is passing in a PHID array for Users without using '=' ? So my transactions aren't validating? If I fix that issue, then adding and removing single members (and the unit tests) fail. So I should .... ?

  • remove ConpherenceThread db stuff
  • only show participants on room create (mirrors old behavior)

Going to cheat and just use old behavior, that we only allow participant editing when the room is being created. Otherwise use main UI.

epriestley edited edge metadata.

Try setUseEdgeTransactions(true) on the PhabricatorUsersEditField for members to un-cheat, I think, maybe? But I think the cheat behavior is good/reasonable, I don't expect to get a tokenizer with 200 people in it when I change the topic of General Chat.

(This code looks fine to me, but I didn't run through it locally -- if this is still WIP kick it back to me with whatever's still causing issues?)

src/applications/conpherence/editor/ConpherenceEditEngine.php
12

Until things are more stable, maybe do this:

public function isEngineConfigurable() {
  return false;
}

...to prevent people from defining custom forms for now, since we may break them later (or never let you customize them if we take over the whole workflow with private/public specializations).

This revision is now accepted and ready to land.Oct 17 2016, 10:45 PM
epriestley rescinded a token.
epriestley awarded a token.

too many APMs

This should be reviewable, but going to take another pass at testing it locally before landing.

chad edited edge metadata.
  • rebase

Closer, but getting hung up on a policy check somewhere when I make a new room now.

Users with the "Can Edit" capability:
Participants in this room can take this action.

chad planned changes to this revision.Oct 18 2016, 3:49 AM

yeah I can't figure this out.

Ah, the policy stuff might be a little messy. The issue is probably just with the "Room Members" policy -- I'd guess other policies work?

The ConpherenceThreadMembersPolicyRule needs to be modified to be able to simulate policies after edits, like the PhabricatorProjectMembersPolicyRule can. That is, the chain of events should be:

  • You create a room with "Visible To: Room Members" and "Initial Users: yourself".
  • The policy code checks if you could see the room if both edits happened.
  • Since you could (you'd be a member) it allows it.

The "if both edits happened" part probably isn't working, so it's just testing if you could see the room without considering that you'll soon be a member.

To fix this, you need to:

  • Pass a membership hint with passTransactionHintToRule() in ConpherenceThreadEditor->adjustObjectForPolicyChecks() (this tells the rule who the members will be in the future).
  • Check the membership hint in ConpherenceThreadMembersPolicyRule->willApplyRules().
  • A model for this is in PhabricatorProjectTransactionEditor / PhabricatorProjectMembersPolicyRule.

This is also tricky to get right; I can shoot you a diff for it and you can put this change on top of that one if you don't have much luck.

Other policies don't work actually. I feel like something else here was missed because I remember being able to make a new room when I started this diff.

Ah, alright -- I'll take a look.

At least, if I force everything to all users, I still get a view policy failure. I think something in edit engine is happening, participants are never getting written.

I think ApplicationTransactionEditor->requireCapabilities() -- and, generally, a bunch of other Conpherence editing -- needs a rewrite to handle this. The Projects application has a vaguely similar issue, but I think we haven't hit all the problems it creates yet there because the defaults are less restrictive. We'll hit them eventually. This also dovetails with the task somewhere about the "Stacked Actions" dropdown letting you apply transactions you don't otherwise necessarily have permission to apply (e.g., change task priority).

Roughly, what's happening is:

  • Object is created with default permissions (usually "Can Edit: Room Members").
  • Object is saved as a side effect of applyInitialEffects(), so members can be written. This is fairly wrong and shouldn't really be happening.
  • Policies are checked in a bad way inside requireCapabilities(). This check also happens very late.
  • Since the object was already saved, this skips the bad short circuit around policies in ConpherenceThread->hasAutomaticCapability().
  • None of this pathway uses adjustObjectForPolicyChecks(), so these checks are often incorrect anyway.

Instead, this should look more like this:

  • Remove initial effects.
  • Use adjustObjectForPolicyChecks() to compute and hint membership.
  • requireCapabilities() should change to return a list of capabilities (or extended capabilities? Or new capability-descriptions?) only. This will impact all applications which override this method. Fortunately, these aren't super common and we can do this in phases.
  • requireCapabilities() should default to CAN_EDIT for all transactions. This will impact all applications. This should be supported by Modular Transactions.
  • requireCapabilities() should probably be a static check (not dependent on which object is being edited) which occurs earlier, although there are some cases where we probably can't
  • Evaluation of requireCapabilities() should occur against policy-adjusted objects.
  • requireCapabilities() should probably be respected directly by stacked actions, since the changes will otherwise create weird interactions (e.g., show you actions you can't actually take). This gets more complicated with cases like "Move on Workboard" and T6502, or "Move Phriction Document" and the need to be able to edit the target document.
  • requireCapabilities() can be used to weaken policy checks in Conduit. In particular, I believe you can't add comments to objects you don't have edit permission on via the API today, even though you could via the web UI. This is inconsistent and can be remedied after these other changes.

I can't really come up with any reasonable quick fixes or workarounds here that don't dig us deeper into this hole.

so what should I do here... sounds like wait for better infrastructure?

This revision is now accepted and ready to land.Nov 21 2016, 5:52 PM
chad planned changes to this revision.Feb 2 2017, 3:23 AM

I think this doesn't fully work, just making sure I don't land it by mistake.

This revision is now accepted and ready to land.Apr 26 2017, 4:55 PM

FWIW, I still can't seem to get this firing. Not important but seems like I'm just missing something silly. Participants never get passed to ConpherenceThreadParticipantsTransaction, presumably because we setUseEdgeTransactions, which we want the list as... thoughts?

What's not working, specifically? From poking at it locally it seems like it at least sorta works? (e..g, I could update room titles and such)

Creating new rooms never attaches participants.

This is a bit tricky, try:

diff --git a/src/applications/conpherence/editor/ConpherenceEditEngine.php b/src/applications/conpherence/editor/ConpherenceEditEngine.php
index e600cac61e..91cd8fd081 100644
--- a/src/applications/conpherence/editor/ConpherenceEditEngine.php
+++ b/src/applications/conpherence/editor/ConpherenceEditEngine.php
@@ -70,8 +70,10 @@ final class ConpherenceEditEngine
 
     if ($this->getIsCreate()) {
       $participant_phids = array($viewer->getPHID());
+      $initial_phids = array();
     } else {
       $participant_phids = $object->getParticipantPHIDs();
+      $initial_phids = $participant_phids;
     }
 
     // Only show participants on create or conduit, not edit
@@ -100,6 +102,7 @@ final class ConpherenceEditEngine
       id(new PhabricatorUsersEditField())
         ->setKey('participants')
         ->setValue($participant_phids)
+        ->setInitialValue($initial_phids)
         ->setIsConduitOnly($conduit_only)
         ->setAliases(array('users', 'members', 'participants', 'userPHID'))
         ->setDescription(pht('Room participants.'))

When you setValue(array('alincoln')) and then submit the form as an edge transaction, we compare the submitted value to the initial value and compute the difference (to prevent race situations where two user edit projects or subscribers at the same time). In this case, both of them are array('alincoln') so the code sees that there was "no change" and just does nothing.

If you setValue(array('alincoln')) but setInitialValue(array()), it will override the default behavior and let it see "alincoln" as a new token by default.

that all works yay. just an edge case now you can create a room you can't see... unsure if need to resolve.

  • works, but let's you create rooms you can't see
chad requested review of this revision.Apr 26 2017, 5:55 PM
chad edited edge metadata.

You probably want to once over this again before I land.

I think we should prevent you from creating a room you can't see, let me see if I can figure out what's going on there.

src/applications/conpherence/application/PhabricatorConpherenceApplication.php
47–50

Can we get rid of this (just point whatever uses new/ at edit/)?

src/applications/conpherence/controller/ConpherenceUpdateController.php
28 ↗(On Diff #42790)

I think we're going to kill this controller soon, but we could use a default + throw if we don't.

src/applications/conpherence/editor/ConpherenceEditEngine.php
108

If you want to offer all these variants, consider a more complete set:

user
users
userPHID
userPHIDs
member
members
memberPHID
memberPHIDs
pariticipant
participants
participantPHID
participantPHIDs

...or just ditch 'em? Not sure there's much value.

I think you can delete this block in ConpherenceThread->hasAutomaticCapability(), which is effectively skipping/disabling the checks:

// this bad boy isn't even created yet so go nuts $user
if (!$this->getID()) {
  return true;
}

However, then we need to implement adjustObjectForPolicyChecks() to accommodate participant changes.

Implemented naively, this will also prevent you from leaving rooms.

Let me just file something and counter-diff you when I have a chance, I think it's fine to leave this particular trap in place since users are unlikely to delete themselves from a private room during creation and be confused that they aren't room members.

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