Rebrand email pushers - Githubissues

jryans commented 3 years ago

Element (at least on web) sets a brand value on the email pusher at registration time. This means there are many users with the value set to Riot, which looks more and more odd as time passes...

We should perform a check at startup to migrate the brand value if it is set to Riot.

dbkr commented 3 years ago

Why are we displaying it at all though? It seems like it should be an internal identifier (and indeed probably ought to have been vector, but never mind).

jryans commented 3 years ago

Why are we displaying it at all though?

Well, it does not appear in Element UI, but it is passed to the Synapse email notification template for mails like "You have a message on Riot from Bob".

@dbkr, are you thinking we should solve the branded email notification template issue a different way?

anoadragon453 commented 3 years ago

Clients would need to use GET /_matrix/client/r0/pushers and POST /_matrix/client/r0/pushers/set to retrieve and update pusher information from the homeserver.

t3chguy commented 3 years ago

Feels a bit redundant to do the migration in three code bases for something which is unspecced and Element+Synapse specific which could be done in one fell swoop by the backend.

Even if only Web set them, the user might have ceased using Web for only the mobile apps or differently yet to completely non Element clients.

anoadragon453 commented 3 years ago

@t3chguy mentioned helpfully that this issue cannot be entirely solved by clients alone, as pushers will be left over if somebody does not upgrade Riot->Element Web, or switches from Element Web to Element mobile/Fluffychat etc. The client will not get a chance to review the set of pushers for its user if the user has stopped using that client.

From the server side, we of course don't want to push out a database migration to all homeservers that rebrands a single Matrix client.

The best solution I can currently see (which requires work from both sides) is to just stop using the brand field in the data dict of pushers altogether, as

It is not specced, and as a result only Element Web uses it
If it's not set, Synapse will just use a value from its configuration file instead, which is "Matrix" and will make sense in the email body no matter the client
It doesn't make sense for a user who may be using various different clients across their devices to get an email saying "You've received unread notifications on Client XYZ!"
The rest of the email body/subject content is set by the homeserver.

So my proposal is to stop Synapse from using this value, and additionally ask clients to stop setting it so that older Synapse versions will just use the homeserver's configured branding instead.

neilisfragile commented 3 years ago

This seems like a pragmatic solution, my main question is why we have the brand parameter at all?

@nadonomy You might be interested in this from a UX perspective, but I think removing reliance on the client supplied brand parameter is the right way forward for now, and in future if we want to revisit then it should factor in all clients (and be spec'd!)

nadonomy commented 3 years ago

If I'm understanding @anoadragon453 @neilisfragile your comments correctly then effectively all emails would be branded as matrix? Which email address and email name would they come from?

I'm not sure that's tractable as it's unreasonable to expect new users to understand the distinction between the product and the protocol this early in the journey. Even in the simplest case we'd have users complaining their verification hasn't arrived, e.g. "I can't see my Element verification email"... because it's come from Matrix.

anoadragon453 commented 3 years ago

@nadonomy Apologies, before now I was under the impression that this brand value only affected a portion of the body in unread message notification emails, while the rest of the email was only controllable by the homeserver. Given this understanding, it made sense to drop brand as any app that tries to change things would immediately look out of place with the rest of the homeserver-branded email.

For the sake of terminology, the name of the variable that's inserted into all of these templates is called app_name. The default app_name is "Matrix", and this can be changed in the homeserver's config file using the email.app_name option. This can be overridden by clients when they register a pusher via setting the brand key in a pusher's data dict.

After taking a proper look through things (the code surrounding all of this is quite old making the flow fairly difficult to follow) I've found that app_name can be used across the From: line, the subject line and the entire body of the email.

Every other email sent out by Synapse is configured by the server however. This creates a disparity between clients being able to brand some aspects of one of the emails Synapse sends out, but not all.

Regarding client-based branding though for a second, @t3chguy brought up a good point. What if the user is using Element on desktop and FluffyChat on mobile? Does it make sense for "you have 7 unread messages!" emails to then be branded as FluffyChat when they reach a user that's also using Element on another device? Since these emails are generated by the homeserver, rather than a specific client, they should probably be branded by unifying terminology.

On the other hand, password reset and registration emails are all generated as a direct result of client actions. Thus you should probably see "FluffyChat" in the subject line of an email when you're registering an account via FluffyChat.

So given the above, we probably want to keep brand, but we'll probably want to rework where it's used. This still leaves us with the problem of supporting clients that need to change this value for existing users when they rebrand. However, if brand is only used during emails that result as a direct action by a client, then if a user stops using Client X and has transitioned to Client Y, even if they've failed to log out of Client X, they should no longer receive emails with the old branding.

This gets a little more complicated if an app is rebranded in place (without requiring users to make a new device, and thus a new pusher). In that case apps will probably want to, once on upgrade, query its current pusher and replace the branding in it).

Finally, if we actually clean up the brand situation, it would be nice to talk about it somewhere in the spec so all clients are aware and will take advantage of it. Currently the data dict is specced, but it has been left (intentionally) very vague. It may be nice to pull out brand and put it in its own top-level key.

Alternatively, maybe this information should be somewhere other than pushers altogether.

nadonomy commented 3 years ago

@anoadragon453 Thanks for the in depth reply! I'm off tomorrow but have on the list to follow up on this more next week.

I'm slightly concerned about the tail wagging the dog so to speak re the current implementation driving user experience, especially in critical user journeys around the first touch experience (email verification, receiving a 3PID invite, etc).

From my side I can prep some example email designs to help us understand what we want better, based on my understanding of your reply, and then we can jump on a call from there? Perhaps with @t3chguy joining too advocating for clients?

anoadragon453 commented 3 years ago

That sounds good to me @nadonomy, thanks!

jryans commented 3 years ago

As we have made a server-side change for matrix.org users, this seems a bit lower priority than before, so have updated labels.

element-hq / element-meta

Rebrand email pushers #591