From 394250397ec4375c41be06eb034688e922e9e8aa Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 12 Aug 2014 09:27:26 -0700
Subject: [PATCH] Improve "unblock task" feed stories

Summary:
Fixes T5184. Fixes T5008. Three issues with stories/notifications about changing the status of tasks which block other tasks:

**Bad Feed Stories**

  - Problem: Feed story rendering was confusing (T5184).
  - Solution: fix it to provide context.

**Too Many Feed Stories**

  - Problem: Feed gets a story for the original task's close ("a closed x"), and a story for each blocked task ("a closed x, a task blocking y").
  - "Solution": Punt. These are redundant in the full feed but not in filtered feeds. Right solution is display-time aggregation. No users have really complained about this.

**Too Many Notifications**

  - Problem: Users subscribed to both tasks get notified about the clsoe, and also about the unblocked task. These notifications are redundant.
  - "Solution": Punt. This is easy to fix by silencing notifications for the sub-editor, but I'm worried it would be confusing. Users haven't complained. Display-time aggregation might be a better fix.

Test Plan: {F189463}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T5008, T5184

Differential Revision: https://secure.phabricator.com/D10235
---
 .../editor/ManiphestTransactionEditor.php     |  5 ---
 .../storage/ManiphestTransaction.php          | 36 ++++++++++++++++---
 2 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php
index 115d8e7b50..85f458aacf 100644
--- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php
+++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php
@@ -373,11 +373,6 @@ final class ManiphestTransactionEditor
             ->setOldValue(array($object->getPHID() => $old))
             ->setNewValue(array($object->getPHID() => $new));
 
-          // TODO: We should avoid notifiying users about these indirect
-          // changes if they are getting a notification about the current
-          // change, so you don't get a pile of extra notifications if you are
-          // subscribed to this task.
-
           id(new ManiphestTransactionEditor())
             ->setActor($this->getActor())
             ->setActingAsPHID($this->getActingAsPHID())
diff --git a/src/applications/maniphest/storage/ManiphestTransaction.php b/src/applications/maniphest/storage/ManiphestTransaction.php
index ac0ada0178..3d5716c656 100644
--- a/src/applications/maniphest/storage/ManiphestTransaction.php
+++ b/src/applications/maniphest/storage/ManiphestTransaction.php
@@ -632,14 +632,40 @@ final class ManiphestTransaction
         }
 
       case self::TYPE_UNBLOCK:
+        $blocker_phid = key($new);
+        $old_status = head($old);
+        $new_status = head($new);
 
-        // TODO: We should probably not show these in feed; they're highly
-        // redundant. For now, just use the normal titles. Right now, we can't
-        // publish something to noficiations without also publishing it to feed.
-        // Fix that, then stop these from rendering in feed only.
+        $old_closed = ManiphestTaskStatus::isClosedStatus($old_status);
+        $new_closed = ManiphestTaskStatus::isClosedStatus($new_status);
 
-        break;
+        $old_name = ManiphestTaskStatus::getTaskStatusName($old_status);
+        $new_name = ManiphestTaskStatus::getTaskStatusName($new_status);
 
+        if ($old_closed && !$new_closed) {
+          return pht(
+            '%s reopened %s, a task blocking %s, as "%s".',
+            $this->renderHandleLink($author_phid),
+            $this->renderHandleLink($blocker_phid),
+            $this->renderHandleLink($object_phid),
+            $new_name);
+        } else if (!$old_closed && $new_closed) {
+          return pht(
+            '%s closed %s, a task blocking %s, as "%s".',
+            $this->renderHandleLink($author_phid),
+            $this->renderHandleLink($blocker_phid),
+            $this->renderHandleLink($object_phid),
+            $new_name);
+        } else {
+          return pht(
+            '%s changed the status of %s, a task blocking %s, '.
+            'from "%s" to "%s".',
+            $this->renderHandleLink($author_phid),
+            $this->renderHandleLink($blocker_phid),
+            $this->renderHandleLink($object_phid),
+            $old_name,
+            $new_name);
+        }
 
       case self::TYPE_OWNER:
         if ($author_phid == $new) {