Allow configuring the Riot URL used in notification emails #1811

Merged
merged 1 commit into from Jan 18, 2017

Projects

None yet

4 participants

@aperezdc
Contributor
aperezdc commented Jan 13, 2017 edited

⚠️ Do not merge yet: ⚠️

I am posting this for initial feedback, in the meanwhile I will be testing this in my HS, and remove this warning afterwards.

Our HS has been running for the whole weekend with this patch applied without any issue — and it's nice to have the notification mails with the links pointing to our self-hosted Riot instance 😄


The URLs used for notification emails were hardcoded to use either matrix.to or vector.im; but for self-hosted setups where Riot is also self-hosted it may be desirable to allow configuring an alternative Riot URL.

Fixes #1809.

@aperezdc aperezdc Allow configuring the Riot URL used in notification emails
The URLs used for notification emails were hardcoded to use either matrix.to
or vector.im; but for self-hosted setups where Riot is also self-hosted it
may be desirable to allow configuring an alternative Riot URL.

Fixes #1809.

Signed-off-by: Adrian Perez de Castro <aperez@igalia.com>
a3e4a19
@matrixbot

Can one of the admins verify this patch?

@matrixbot

Can one of the admins verify this patch?

@matrixbot

Can one of the admins verify this patch?

@erikjohnston
Member

@matrixbot ok to test

@dbkr
Member
dbkr commented Jan 18, 2017

A better way to do this would be to have the riot base URL passed in the pusher data dict when the pusher is set up, then you could use any riot URL on any HS. However, I'm OK with this as a quick-fix. @erikjohnston wdyt?

@erikjohnston erikjohnston was assigned by dbkr Jan 18, 2017
@erikjohnston
Member

A better way to do this would be to have the riot base URL passed in the pusher data dict when the pusher is set up, then you could use any riot URL on any HS. However, I'm OK with this as a quick-fix. @erikjohnston wdyt?

Are email pushers not shared between different devices? Can you not set one up on your phone?

@aperezdc
Contributor

My understanding about what @erikjohnston comments is that, if an user never logged in via Riot Web, and only via Riot Android (or iOS), the user could add an email and set-up mail notifications from their mobile client, and there wouldn't be any pusher with an URL configured. I have no idea if that's bad or not...

Also, another doubt: what it two clients want to set their own URL? For example the mobile clients could want to include a custom URL handler (once the mx://... URL scheme thingy is defined), and Riot Web may want to have an actual HTTP(S) URL instead. Would there be more than an URL associated with the pusher? If yes, how would that translate into notification emails? If there's only two, one of them being an mx:// URL, the mail could have two links: “Open in the Riot application” and “Open in a web browser”. What about multiple URLs added by different instances of Riot Web?

I think the above questions probably need people from the team to reflec and come up with a better solution — for the moment it would be great to have this PR merged (which fortunately makes just a small change) as a workaround.

@dbkr
Member
dbkr commented Jan 18, 2017

This is the same problem as branding (ie. if you use Riot and another client, should your notification mails be Riot branded or not?). The decision on this is that the branding depends on where you set up the email notifs, so it would depend on that. The device you use is a red herring since you're setting up email notifications which you might be reading on any device, not just the one you're setting them up on, so they'll always have to give URLs that will work anywhere.

But yes, I'm OK with this as a quick fix. Merging unless anyone objects?

@erikjohnston
Member

Fine by me

@dbkr dbkr merged commit 97d3918 into matrix-org:develop Jan 18, 2017

3 of 4 checks passed

Sytest Dendron (Merged PR) Build finished.
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Merged PR) Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@aperezdc aperezdc deleted the aperezdc:unhardcode-riot-urls branch Jan 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment