Can we do builtins with EditEngine?

I guess Calendar? does?

Not currently!

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

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

too many APMs

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

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.

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.

uh

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

Yeah, pretty much.

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.

@epriestley any final issues?

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.

Filed as T12654: Conpherence allows you to create a room you can't view.