notif: On Android, use org name instead of URL as notif group summary#2136
Conversation
|
Screenshots:
|
rajveermalviya
added
the
maintainer review
rajveermalviya
force-pushed
the
pr-android-notif-org-name
branch
from
130bb4b to
64c06b5
Compare
|
(Rebased to main to fix CI) |
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks! LGTM; marking for Greg's review.
| expectedTagComponent: 'stream:${message.streamId}:${message.topic}'); | ||
| }))); | ||
|
|
||
| test('stream message: different realms', () => runWithHttpClient(() => awaitFakeAsync((async) async { |
There was a problem hiding this comment.
nit: "channel message", I think?
Oh I see; there's a strong pattern of saying "stream" in this test's siblings. So "stream" is fine, I think, and we can sweep through later and change them all together (#631).
test/notifications/display_test.dart
Outdated
| ..inboxStyle.which((it) => it.isNotNull() | ||
| ..summaryText.equals(data.realmUrl.toString())) | ||
| ..inboxStyle.which( | ||
| (it) => it.isNotNull() | ||
| ..summaryText.equals(expectedSummaryText!)) |
There was a problem hiding this comment.
nit: can keep this at two lines instead of three
chrisbobbe
added
integration review
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks! Just nits below, plus the nit Chris pointed out above.
| FcmMessageWithIdentity({ | ||
| required this.realmUrl, | ||
| required this.userId, | ||
| required this.realmName, |
There was a problem hiding this comment.
nit: let's put realm name next to realm URL, since they're more closely related to each other than either is to the user ID
test/api/notifications_test.dart
Outdated
| check(parse({ ...streamJson }..remove('realm_name'))) | ||
| .realmName.isNull(); |
There was a problem hiding this comment.
nit: put this above its two siblings in this test case, because realm_name comes before channel_name or stream in the declarations
| ..realmUrl.equals(Uri.parse(baseJson['realm_url'] as String)) | ||
| ..realmUrl.equals(Uri.parse(baseJsonPreE2ee['realm_uri'] as String)) // TODO(server-9) | ||
| ..userId.equals(234) | ||
| ..realmName.equals(baseBaseJson['realm_name'] as String) |
There was a problem hiding this comment.
nit: add a similar check in the RemoveFcmMessage tests below, since those objects also gain this field
rajveermalviya
force-pushed
the
pr-android-notif-org-name
branch
from
64c06b5 to
b0c9189
Compare
|
Thanks for the reviews @gnprice and @chrisbobbe. @gnprice pushed an update, PTAL. |
Zulip Server 8 (FL 234) started sending realmName in the FCM payload: zulip/zulip@a018f2611 So, we try to use the realm name from the account first, then fallback to notification data, and then finally fallback to displaying the realm URL. Fixes: zulip#570
|
Thanks! Looks good; merging. |
gnprice
force-pushed
the
pr-android-notif-org-name
branch
from
b0c9189 to
aacf52a
Compare
This was introduced in aacf52a (zulip#2136), after I rebased that PR and immediately merged. The rebase went past 5705f83 (zulip#2129), which had enabled the `use_null_aware_elements` lint rule, so this was effectively an indirect merge conflict.
This was introduced in 7584e9f (zulip#2136), after I rebased that PR and immediately merged. The rebase went past a82d79b (zulip#2129), which had enabled the `use_null_aware_elements` lint rule, so this was effectively an indirect merge conflict.
3 participants
Zulip Server 8 (FL 234) started sending realmName in the FCM payload:
zulip/zulip@a018f2611
So, we try to use the realm name from the account first, then fallback to notification data, and then finally fallback to displaying the realm URL.
Fixes: #570