notifications: Match on realm domain, not href. #4817
Conversation
WesleyAC
force-pushed
the
notifications-match-domain-not-href
branch
from
e2c9921
to
ba5bae3
Compare
There was a problem hiding this comment.
Great, thanks! I haven't yet traced through how much of #4309 this will fix, but this seems like an obvious improvement to make. See one small comment below.
src/notification/index.js
Outdated
| @@ -48,7 +48,7 @@ export const getAccountFromNotificationData = ( | |||
|
|
|||
| const urlMatches = []; | |||
| identities.forEach((account, i) => { | |||
| if (account.realm.href === realmUrl.href) { | |||
| if (account.realm.host === realmUrl.host) { | |||
There was a problem hiding this comment.
We usually compare .origin, not .host (e.g., as in e47a143). .origin should have everything in .host, plus the scheme, like http or https, which I'd be inclined to differentiate on. What do you think?
There was a problem hiding this comment.
Ah, yeah, I was considering that when I made this, but didn't realize .origin was a thing. I'll change that, since in theory someone could run two different zulip servers, one on http://example.com and the other on https://example.com, even if that would be... extremely chaotic :p
We had a steady stream of errors in Sentry caused by differences between the `known_urls` and `realm_uri`, where one of those had an extra `/` on the end (or some more surprising junk, such as a `/join/` link) [1]. Since we don't support running Zulip on a non-root path [2], we can simply compare the domain instead of the href. This is probably more of a band-aid than a real fix (we should figure out how the strange values get into `known_urls` and fix that), but we might as well just check the domain, since the path doesn't matter. Related: zulip#4309. [1] zulip#4309 (comment) [2] https://chat.zulip.org/#narrow/stream/3-backend/topic/support.20for.20running.20on.20non-webroot.20url.3F
WesleyAC
force-pushed
the
notifications-match-domain-not-href
branch
from
ba5bae3
to
63cc7fd
Compare
|
Revision pushed :) |
|
LGTM, thanks! Please merge at will. |
We had a steady stream of errors in Sentry caused by differences between
the
known_urlsandrealm_uri, where one of those had an extra/onthe end (or some more surprising junk, such as a
/join/link). Sincewe don't support running Zulip on a non-root path [1], we can simply
compare the domain instead of the href. This is probably more of a
band-aid than a real fix (we should figure out how the strange values
get into
known_urlsand fix that), but we might as well just check thedomain, since the path doesn't matter.
Related: #4309.
[1] https://chat.zulip.org/#narrow/stream/3-backend/topic/support.20for.20running.20on.20non-webroot.20url.3F