recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Don't subscribe bots implicitly when they act on objects, or when they are mentioned

Summary:
See PHI1098. When users comment on objects, they are automatically subscribed. And when `@alice` mentions `@bailey` on a task, that usually subscribes `@bailey`.

These rules make less sense if the user is a bot. There's generally no reason for a bot to automatically subscribe to objects it acts on (it's not going to read email and follow up later), and it can always subscribe itself pretty easily if it wants (since everything is `*.edit` now and supports subscribe transactions).

Also, don't subscribe bots when they're mentioned for similar reasons. If users really want to subscribe bots, they can do so explicitly.

These rules seem slightly like "bad implicit magic" since it's not immediately obvious why `@abc` subscribes that user but `@xyz` may not, but some of these rules are fairly complicated already (e.g., `@xyz` doesn't subscribe them if they unsubscribed or are implicitly subscribed) and this feels like it gets the right/desired result almost-always.

Test Plan:
On a fresh task:

- Mentioned a bot in a comment with `@bot`.
- Before patch: bot got CC'd.
- After patch: no CC.
- Called `maniphest.edit` via the API to add a comment as a bot.
- Before patch: bot got CC'd.
- After patch: no CC.

Reviewers: amckinley

Reviewed By: amckinley

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

epriestley 04f9e72c a6e17fb7

+53 -14
+53 -14
src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
··· 1816 1816 $users = mpull($users, null, 'getPHID'); 1817 1817 1818 1818 foreach ($phids as $key => $phid) { 1819 - // Do not subscribe mentioned users 1820 - // who do not have VIEW Permissions 1821 - if ($object instanceof PhabricatorPolicyInterface 1822 - && !PhabricatorPolicyFilter::hasCapability( 1823 - $users[$phid], 1824 - $object, 1825 - PhabricatorPolicyCapability::CAN_VIEW) 1826 - ) { 1819 + $user = idx($users, $phid); 1820 + 1821 + // Don't subscribe invalid users. 1822 + if (!$user) { 1823 + unset($phids[$key]); 1824 + continue; 1825 + } 1826 + 1827 + // Don't subscribe bots that get mentioned. If users truly intend 1828 + // to subscribe them, they can add them explicitly, but it's generally 1829 + // not useful to subscribe bots to objects. 1830 + if ($user->getIsSystemAgent()) { 1827 1831 unset($phids[$key]); 1828 - } else { 1829 - if ($object->isAutomaticallySubscribed($phid)) { 1832 + continue; 1833 + } 1834 + 1835 + // Do not subscribe mentioned users who do not have permission to see 1836 + // the object. 1837 + if ($object instanceof PhabricatorPolicyInterface) { 1838 + $can_view = PhabricatorPolicyFilter::hasCapability( 1839 + $user, 1840 + $object, 1841 + PhabricatorPolicyCapability::CAN_VIEW); 1842 + if (!$can_view) { 1830 1843 unset($phids[$key]); 1844 + continue; 1831 1845 } 1832 1846 } 1847 + 1848 + // Don't subscribe users who are already automatically subscribed. 1849 + if ($object->isAutomaticallySubscribed($phid)) { 1850 + unset($phids[$key]); 1851 + continue; 1852 + } 1833 1853 } 1854 + 1834 1855 $phids = array_values($phids); 1835 1856 } 1836 - // No else here to properly return null should we unset all subscriber 1857 + 1837 1858 if (!$phids) { 1838 1859 return null; 1839 1860 } 1840 1861 1841 - $xaction = newv(get_class(head($xactions)), array()); 1842 - $xaction->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS); 1843 - $xaction->setNewValue(array('+' => $phids)); 1862 + $xaction = $object->getApplicationTransactionTemplate() 1863 + ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS) 1864 + ->setNewValue(array('+' => $phids)); 1844 1865 1845 1866 return $xaction; 1846 1867 } ··· 2874 2895 // don't implicitly CC them. 2875 2896 return $xactions; 2876 2897 } 2898 + } 2899 + 2900 + $actor = $this->getActor(); 2901 + 2902 + $user = id(new PhabricatorPeopleQuery()) 2903 + ->setViewer($actor) 2904 + ->withPHIDs(array($actor_phid)) 2905 + ->executeOne(); 2906 + if (!$user) { 2907 + return $xactions; 2908 + } 2909 + 2910 + // When a bot acts (usually via the API), don't automatically subscribe 2911 + // them as a side effect. They can always subscribe explicitly if they 2912 + // want, and bot subscriptions normally just clutter things up since bots 2913 + // usually do not read email. 2914 + if ($user->getIsSystemAgent()) { 2915 + return $xactions; 2877 2916 } 2878 2917 2879 2918 $xaction = newv(get_class(head($xactions)), array());