From 3e0391c574f2d489c1a5ce42bfc998956ae298e1 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 10 May 2019 11:41:43 -0700
Subject: [PATCH] Don't mark subscribes/mentions as "Lock Overridden"

Summary:
See PHI1209. When a task is in "Hard Lock" mode, it's still possible to apply some changes to it. Notably:

  - You can subscribe/unsubscribe.
  - You can mention it on another object.
  - You can add a relationship from some other object to it (e.g., select it as a "Parent Task" for some other task).

Currently, these types of edits will show a "Lock Overridden" timeline emblem icon. However, they should not: you didn't override a lock to make these changes, they just bypass locks.

For now, special case these cases (self subscribe/unsubscribe + inverse edge edits) so they don't get the little icon, since I think this list is exhaustive today.

Some day we should modularize this, but we'd need code like this anyway (since TYPE_SUBSCRIBE is not modular yet), and this seems unlikely to cause problems even if it's a bit rough.

Test Plan:
  - Hard-locked a task.
    - Subscribed/unsubscribed, mentioned, relationship'd it as a non-author. No timeline emblems.
  - Soft-locked a task.
    - Subscribed/unsubscribed, mentioned, relationship'd it, no timeline emblems.
    - Clicked "Edit", answered "yes" to the override prompt, edited it. Got a timeline emblem.
  - Added some comments and stuff to a normal non-locked task, no emblems.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20513
---
 ...habricatorApplicationTransactionEditor.php | 54 ++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
index f622e0a21d..b383d0605c 100644
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -1164,7 +1164,10 @@ abstract class PhabricatorApplicationTransactionEditor
 
       foreach ($xactions as $xaction) {
         if ($was_locked) {
-          $xaction->setIsLockOverrideTransaction(true);
+          $is_override = $this->isLockOverrideTransaction($xaction);
+          if ($is_override) {
+            $xaction->setIsLockOverrideTransaction(true);
+          }
         }
 
         $xaction->setObjectPHID($object->getPHID());
@@ -5204,6 +5207,55 @@ abstract class PhabricatorApplicationTransactionEditor
     return $xaction->getBodyForMail();
   }
 
+  private function isLockOverrideTransaction(
+    PhabricatorApplicationTransaction $xaction) {
+
+    // See PHI1209. When an object is locked, certain types of transactions
+    // can still be applied without requiring a policy check, like subscribing
+    // or unsubscribing. We don't want these transactions to show the "Lock
+    // Override" icon in the transaction timeline.
+
+    // We could test if a transaction did no direct policy checks, but it may
+    // have done additional policy checks during validation, so this is not a
+    // reliable test (and could cause false negatives, where edits which did
+    // override a lock are not marked properly).
+
+    // For now, do this in a narrow way and just check against a hard-coded
+    // list of non-override transaction situations. Some day, this should
+    // likely be modularized.
+
+
+    // Inverse edge edits don't interact with locks.
+    if ($this->getIsInverseEdgeEditor()) {
+      return false;
+    }
+
+    // For now, all edits other than subscribes always override locks.
+    $type = $xaction->getTransactionType();
+    if ($type !== PhabricatorTransactions::TYPE_SUBSCRIBERS) {
+      return true;
+    }
+
+    // Subscribes override locks if they affect any users other than the
+    // acting user.
+
+    $acting_phid = $this->getActingAsPHID();
+
+    $old = array_fuse($xaction->getOldValue());
+    $new = array_fuse($xaction->getNewValue());
+    $add = array_diff_key($new, $old);
+    $rem = array_diff_key($old, $new);
+
+    $all = $add + $rem;
+    foreach ($all as $phid) {
+      if ($phid !== $acting_phid) {
+        return true;
+      }
+    }
+
+    return false;
+  }
+
 
 /* -(  Extensions  )--------------------------------------------------------- */