Page MenuHomePhabricator
Create Task
Maniphest T10822

When vary-subjects is enabled, a task is merged, and subscribers are modifed the subject is misleading
Closed, InvalidPublic
Actions

Description

This was observed on this very instance. My email notification settings for Maniphest are:

pasted_file (138×626 px, 16 KB)
pasted_file (275×597 px, 44 KB)

However I've observed a few times that I've received notification emails for "changed subscribers" like this one a few hours ago:

From: epriestley (Evan Priestley) <noreply@phabricator.com>
Date: Fri, Apr 15, 2016 at 5:14 AM
Subject: [Maniphest] [Changed Subscribers] T4103: Implement "Role Profiles" to provide search, homepage and application defaults
To: <REDACTED>


epriestley added a subscriber: nicorac.
epriestley merged a task: T10821: Add default settings for newly created accounts.

TASK DETAIL
  https://secure.phabricator.com/T4103

EMAIL PREFERENCES
  https://secure.phabricator.com/settings/panel/emailpreferences/

To: epriestley
Cc: nicorac, isfs, ofbeaton, jasonfsmitty, revi, r0bbie, dans, richardvanvelzen, ProfFan, JOY, ablekh, carwyn, jithinvmohan, featherless, arune, edibiase, kymera.travis, gou1, gerx03, cspeckmim, srijan, jayvdb, nickz, aHa, eadler, maiki, joshuaspence, mirrom, Mnkras, djc, metellius, trsystran, MZMcBride, devurandom, johnny-bit, scfc, timor, jbrown, aklapper, bigo, mikerbrewster, m.capobianco, chad, lpriestley, ite-klass, noisy, shadowhand, avivey, wotte, cburroughs, chasemp, philippe.jadin, swisspol, btrahan, hsnim, michaeloa, asherkin, epriestley, vandana9, allan.laal, hach-que

Related Objects

Event Timeline

swisspol created this task.EditedApr 15 2016, 3:15 PM
swisspol updated the task description. (Show Details)

It looks like the changed subscribers results from a merged task, so maybe this case is not handled?

I'd certainly like to be notified when a task is merged, but then the notification should say that, not that subscribers were changed.

eadler closed this task as Invalid.EditedApr 15 2016, 3:32 PM
eadler added a subscriber: eadler.

You're getting this cause a task was merged, not because of the changed subscribers. The logic roughly looks like if (get mail for any reason) { send entire email; }. If you look at the full headers you should see a list of 'mail-tags'. If you're notifications are set to email for any one of those tags you'll get the email.

eadler added a comment.Apr 15 2016, 3:33 PM

T9412 might be helpful in understanding this in the future.

swisspol added a comment.Apr 15 2016, 3:57 PM

I really don't think this task should be closed. This is a user experience bug, independently of any underlying good reason there might be in the implementation.

The user explicitly demand not to be notified of subscriber changes on tasks. Maniphest still sends emails with [Changed Subscribers] in their title and body. That's is the very definition of a bug.

swisspol added a comment.Apr 15 2016, 3:59 PM

You can either solve this by not sending such emails when a task is merged (but they you don't notified of merged tasks which is bad), or by having a dedicated subject / body for merged task notifications, which then:

  1. solves this bug
  2. improves the UX since you now get a notification email that actually reflects the action that happened
eadler renamed this task from Receiving "Changed Subscribers" notification emails even if disabled in account preferences to When vary-subjects is enabled, a task is merged, and subscribers are modifed the subject is misleading.Apr 15 2016, 4:09 PM
eadler reopened this task as Open.
eadler added projects: Mail, Maniphest.

The issue may be more general than the new title, but this is more actionable (I think).
Note that I don't work for Phacility.

epriestley added a subscriber: epriestley.Apr 15 2016, 4:14 PM

When a group of transactions makes several changes, we guess at which change is most likely to be interesting and include that change as the action in the subject line.

We could list all changes instead, but this would lead to very long subjects like [Changed Projects, Changed Subscribers, Commented, Changed Priority] which I think would generally be less useful than showing a single action. The common case is that this collapses [Accepted, Commented] to [Accepted], which I believe is more concise and more useful for most users.

We could list no change action instead, and installs can configure this by adjusting metamta.vary-subjects, or you can configure it yourself in Settings → Email Format.

In choosing which action to show, we currently use a set of heuristics to guess at which action is most likely to be interesting. We won't always get this right. If we don't take individual user settings into account, we'll always exhibit "the very definition of a bug" for some users. That is:

  • User A doesn't want mail changes of type X.
  • User B doesn't want mail changes of type Y.
  • User C updates a task, applying changes of type X and Y.

If we pick a single subject for mail sent to both A and B, it will necessarily exhibit this "bug" for one of them.

We could vary the subject for each user individually: rank the actions heuristically, and then remove any actions which the user did not want to receive mail for, choosing the first action in the remaining list.

This won't work if metamta.one-mail-per-recipient is disabled, because we must send a single mail body to all recipients. So we must have a mode where we do not do this.

We could refine this behavior in this subset of cases where we can send individual emails.

We could also adjust the heuristic to rank Changed Subscribers below Merged in importance. I think they currently both have default priorities and the ranking is arbitrary (neither action is considered "stronger"). However, this would create the inverse bug: users who did not want merge email but did want subscriber change email would see [Merged].

As a third option, we could do what you suggest and completely special case merges, replacing the entire email pathway with a special one dedicated to this one action in one application.

Unfortunately, we have finite resources, both in our ability to develop software features and our ability to manage product complexity. We are likely to refine the heuristic (see T8952 for adjacent refinements) because it impacts many other cases, but this won't actually resolve the issue you've experienced, and won't make the software less buggy by your definition.

We will not pursue the other changes. This is a trivial nuisance and the suggestion that we have a dedicated pathway for it is absurd. I think closing this task as invalid is correct.

epriestley closed this task as Invalid.Apr 15 2016, 4:14 PM
epriestley added a comment.Apr 15 2016, 4:26 PM

(@eadler's retitle of this bug is fine and I agree that [Merged] should be a stronger action than [Changed Subscribers], I'll make a note about this on T8952.)

epriestley mentioned this in T8952: Hide "Herald added subscribers..." stories from feed completely.Apr 15 2016, 4:27 PM
swisspol added a comment.Apr 15 2016, 4:41 PM

Thanks for looking into this @epriestley!

The heuristics you described make total sense. It might just be a matter of relative priority between the two.

PS: Note that when I mentioned a "dedicated subject / body for merged task notifications", I wasn't implying a custom code path just for this, which obviously doesn't make sense. Not being familiar with the that code in Phabricator, I was assuming you had some sort of a set of email templates, and for some reason there wasn't one for the "merged task" case, so it was falling back to the "subscribers changed" one.

Phabricator · Built by Phacility