mirror of
https://we.phorge.it/source/phorge.git
synced 2024-12-24 06:20:56 +01:00
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
This commit is contained in:
parent
94389fcd9f
commit
394250397e
2 changed files with 31 additions and 10 deletions
|
@ -373,11 +373,6 @@ final class ManiphestTransactionEditor
|
||||||
->setOldValue(array($object->getPHID() => $old))
|
->setOldValue(array($object->getPHID() => $old))
|
||||||
->setNewValue(array($object->getPHID() => $new));
|
->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())
|
id(new ManiphestTransactionEditor())
|
||||||
->setActor($this->getActor())
|
->setActor($this->getActor())
|
||||||
->setActingAsPHID($this->getActingAsPHID())
|
->setActingAsPHID($this->getActingAsPHID())
|
||||||
|
|
|
@ -632,14 +632,40 @@ final class ManiphestTransaction
|
||||||
}
|
}
|
||||||
|
|
||||||
case self::TYPE_UNBLOCK:
|
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
|
$old_closed = ManiphestTaskStatus::isClosedStatus($old_status);
|
||||||
// redundant. For now, just use the normal titles. Right now, we can't
|
$new_closed = ManiphestTaskStatus::isClosedStatus($new_status);
|
||||||
// publish something to noficiations without also publishing it to feed.
|
|
||||||
// Fix that, then stop these from rendering in feed only.
|
|
||||||
|
|
||||||
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:
|
case self::TYPE_OWNER:
|
||||||
if ($author_phid == $new) {
|
if ($author_phid == $new) {
|
||||||
|
|
Loading…
Reference in a new issue