Page MenuHomePhabricator

Properly integrate Maniphest with Subscribers
Closed, ResolvedPublic

Description

Maniphest has a half-complete Subscribers implementation with some old hard-coding. It should be fully modernized.


Pretty much all of the time, I care more about a comment being left than I do about an subscribers being updated. However, because adding a comment also adds a subscriber, I often get "@alincoln updated subscribers of X" notifications rather than "@alincoln commented on X". It seems inconsistent however and sometimes works then other way around.

Event Timeline

joshuaspence raised the priority of this task from to Needs Triage.
joshuaspence updated the task description. (Show Details)
joshuaspence added a project: Notifications.
joshuaspence added a subscriber: joshuaspence.

The expectation is that the "subscription" will win if it's someone other than the author (argument being: adding someone else as a CC is fairly interesting, at least some of the time), but the "comment" will win if it's the author (argument being: this is essentially never interesting).

Maniphest isn't "really" using subscriptions properly yet, so it doesn't necessarily obey this rule. Audit/Diffusion is behind the times too. Other applications should follow this rule.

  • Are you seeing other behavior?
  • Do you think the expected behavior is bad?

That wasn't very clear -- specifically:

  • If the author of the comment is also the only subscribed user added by the transaction (often, an implicit subscription caused by the comment itself), "Comment" wins.
  • Otherwise, "Subscribe" wins.

I guess I'm primarily talking about maniphest. You (@epriestley) commented on two tickets (T5604 and T5602) and I had two notifications: one update subscribers and one commented.

I'm going to repurpose this since I don't think we have a ticket for swapping to "real" subscribers yet. I think that'll fix things.

epriestley renamed this task from "Updated subscribers" notifications compete with "added a comment" notifications to Properly integrate Maniphest with Subscribers.Jul 13 2014, 4:20 PM
epriestley updated the task description. (Show Details)
epriestley edited projects, added Maniphest; removed Notifications.
epriestley edited this Maniphest Task.Jul 13 2014, 4:21 PM

T5245 probably isn't a technical blocker but we'll avoid a lot of conflicts by waiting for it since the projects and subscribers code is often adjacent.

chad triaged this task as Low priority.Jul 14 2014, 1:03 AM
epriestley added a subscriber: GMTA.Sep 1 2014, 5:46 PM

◀ Merged tasks: T6010.

epriestley raised the priority of this task from Low to Normal.

Support Impact This creates various confusing application inconsistencies.

btrahan claimed this task.Dec 4 2014, 11:06 PM

Another one I figure I should do unless you've got something... Lemme know!

Should be good to go, this just involves a messy migration.

Well, maybe not that messy. I think I got away with at least one of these pretty cleanly in the past, since you basically just copy the table into the edge table and theoretically you're about done.

qgil added a comment.Dec 8 2014, 6:34 AM

For what is worth, this problem caused confusion among users that were ignoring "updated subscribers" notifications but still could see them in their feeds. It took us a while to realize that these notifications were legit, just misnamed: https://phabricator.wikimedia.org/T76098

btrahan reopened this task as Open.Dec 11 2014, 6:32 PM

There's still some cruft in the codebase here. Mainly the transaction rendering code for the old style transactions. I have to (maybe) fix something similar for projects so if this doesn't look ridiculously bad I'll fix this bit too.

That said, for a user, all the benefits of this task should be there now. Plus probably a few bugs I'll fix as we find them. :D