Change/drop/reconcile some miscellaneous edit behaviors in Maniphest

Summary:
Ref T9132. Open to discussion here since it's mostly product stuff, but here's my gut on this:

- Change Maniphest behavior to stop assigning tasks if they're unassigned when closed. I think this behavior often doesn't make much sense. We'll probably separately track "who closed this" in T4434 eventually.
- Only add the actor as a subscriber if they comment, like in other applications. Previously, we added them as a subscriber for other types of changes (like priority and status changes). This is more consistent, but open to retaining the old behavior or some compromise between the two.
- Retain the "when changing owner, subscribe the old owner" behavior.

Test Plan:
- Added a comment, got CC'd.
- Changed owners, saw old owner get CC'd.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9132

Differential Revision: https://secure.phabricator.com/D14670

epriestley 10 years ago f1744ac6 f9e84d1a

+75 -5

3 changed files

expand all

src

applications

maniphest

editor

ManiphestEditEngine.php

ManiphestTransactionEditor.php

transactions

editor

PhabricatorApplicationTransactionEditor.php

-4

src/applications/maniphest/editor/ManiphestEditEngine.php

··· 60 60 61 61 $priority_map = ManiphestTaskPriority::getTaskPriorityMap(); 62 62 63 - // TODO: Restore these or toss them: 64 - // - When closing an unassigned task, assign the closing user. 65 - // - Make sure implicit CCs on actions are working reasonably. 66 - 67 63 if ($object->isClosed()) { 68 64 $priority_label = null; 69 65 $owner_label = null;

+74

src/applications/maniphest/editor/ManiphestTransactionEditor.php

··· 745 745 return $errors; 746 746 } 747 747 748 + protected function expandTransactions( 749 + PhabricatorLiskDAO $object, 750 + array $xactions) { 751 + $results = parent::expandTransactions($object, $xactions); 752 + 753 + $is_unassigned = ($object->getOwnerPHID() === null); 754 + 755 + $any_assign = false; 756 + foreach ($xactions as $xaction) { 757 + if ($xaction->getTransactionType() == ManiphestTransaction::TYPE_OWNER) { 758 + $any_assign = true; 759 + break; 760 + } 761 + } 762 + 763 + $is_open = !$object->isClosed(); 764 + 765 + $new_status = null; 766 + foreach ($xactions as $xaction) { 767 + switch ($xaction->getTransactionType()) { 768 + case ManiphestTransaction::TYPE_STATUS: 769 + $new_status = $xaction->getNewValue(); 770 + break; 771 + } 772 + } 773 + 774 + if ($new_status === null) { 775 + $is_closing = false; 776 + } else { 777 + $is_closing = ManiphestTaskStatus::isClosedStatus($new_status); 778 + } 779 + 780 + // If the task is not assigned, not being assigned, currently open, and 781 + // being closed, try to assign the actor as the owner. 782 + if ($is_unassigned && !$any_assign && $is_open && $is_closing) { 783 + // Don't assign the actor if they aren't a real user. 784 + $actor = $this->getActor(); 785 + $actor_phid = $actor->getPHID(); 786 + if ($actor_phid) { 787 + $results[] = id(new ManiphestTransaction()) 788 + ->setTransactionType(ManiphestTransaction::TYPE_OWNER) 789 + ->setNewValue($actor_phid); 790 + } 791 + } 792 + 793 + return $results; 794 + } 795 + 796 + protected function expandTransaction( 797 + PhabricatorLiskDAO $object, 798 + PhabricatorApplicationTransaction $xaction) { 799 + 800 + $results = parent::expandTransaction($object, $xaction); 801 + 802 + switch ($xaction->getTransactionType()) { 803 + case ManiphestTransaction::TYPE_OWNER: 804 + // When a task is reassigned, move the old owner to the subscriber 805 + // list so they're still in the loop. 806 + $owner_phid = $object->getOwnerPHID(); 807 + if ($owner_phid) { 808 + $results[] = id(new ManiphestTransaction()) 809 + ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS) 810 + ->setIgnoreOnNoEffect(true) 811 + ->setNewValue( 812 + array( 813 + '+' => array($owner_phid => $owner_phid), 814 + )); 815 + } 816 + break; 817 + } 818 + 819 + return $results; 820 + } 821 + 748 822 749 823 }

+1 -1

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

··· 1369 1369 * resigning from a revision in Differential implies removing yourself as 1370 1370 * a reviewer. 1371 1371 */ 1372 - private function expandTransactions( 1372 + protected function expandTransactions( 1373 1373 PhabricatorLiskDAO $object, 1374 1374 array $xactions) { 1375 1375

Configure Feed

Configure Feed