Rewrite EchoNotificationFormatter
Closed, ResolvedPublic

Description

Kunal says the notification formatter is inflexible and should be rewritten. It relies too much on messages to format things, it doesn't allow for flexibility in what its "payload" is, and there are a lot of little things that make it difficult to write correct code.

For example, you have to use $this->language if it's set, or $wgLang is not, but there's no getter wrapping this for you. If you do this wrong, then when a French-speaking user does something that causes a German-speaking user to be notified, (part of) the notification will be in French instead of in German.

Also, the $wgEchoNotifications description format is difficult to understand, and the formatting code depends on this global state.

Catrope created this task.Aug 3 2015, 11:47 PM
Catrope updated the task description. (Show Details)
Catrope raised the priority of this task from to Needs Triage.
Catrope added a project: Notifications.
Catrope added subscribers: Catrope, Legoktm.
Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptAug 3 2015, 11:47 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

See also T56486: Echo flyout takes too long to open: this might be (partially) caused by the formatter being slow.

DannyH triaged this task as Normal priority.Aug 4 2015, 10:41 PM

Extensions currently integrating with Echo:

I'm going to start with cleaning up the icon code since it's relatively isolated (e.g. T60726: Echo: refactor duplicate icon handling code into getNotificationIconUrl()).

DannyH raised the priority of this task from Normal to High.Aug 11 2015, 5:07 PM
DannyH added a subscriber: DannyH.

Change 232632 had a related patch set uploaded (by Legoktm):
[WIP] Clean up and refactor formatting system

https://gerrit.wikimedia.org/r/232632

The architecture I came up with is:

  • EchoEventFormatter (more accurate name tbd) - returns info about how to format the EchoEvent:
    • getIconURL
    • getHeaderText
    • getBodyText
    • getPrimaryLink
    • getSecondaryLinks
  • Individual formatters for each output type (e.g. EchoSpecialNotificationsFormatter, EchoPlainTextEmailFormatter) that are given a EchoEventFormatter and do their magic.

The main todos I have left (aside from writing the rest of the code):

  • determining where checking for rev del should go.
  • In a lot of places, we have different text for emails and flyout notifications, do we want to continue keeping them different? (IMO no)

The architecture I came up with is:

  • EchoEventFormatter (more accurate name tbd) - returns info about how to format the EchoEvent:

EchoEventPresentationModel? I think it fits the presentation model pattern.

DannyH removed a subscriber: DannyH.Oct 5 2015, 10:44 PM

Change 232632 merged by jenkins-bot:
Clean up and refactor formatting system

https://gerrit.wikimedia.org/r/232632

Restricted Application added a project: Growth-Team. · View Herald TranscriptSun, Nov 18, 12:52 PM