Skip to content

Commit

Permalink
notif: Separate ids in pm_users for group PMs with ',' instead of ', '.
Browse files Browse the repository at this point in the history
', ' would have been handled correctly, but seemingly by accident;
in getNarrowFromNotificationData, pm_users was split on ',' to give
['1', ' 2', ' 3'] (note the spaces), then + was used on elements of
that array, as in +idStrs[i], to convert to a number.

Also, replace the confusing + syntax with parseInt.
  • Loading branch information
Chris Bobbe committed Feb 21, 2020
1 parent 299d6fc commit 92bee0e
Show file tree
Hide file tree
Showing 4 changed files with 9 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ sealed class Recipient {
* pmUsers: the user IDs of all users in the conversation.
*/
data class GroupPm(val pmUsers: Set<Int>) : Recipient() {
fun getPmUsersString() = pmUsers.sorted().joinToString { it.toString() }
fun getPmUsersString() = pmUsers.sorted().joinToString(separator=",") { it.toString() }
}

/** A stream message. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,13 @@ class FcmMessageTest : FcmMessageTestBase() {
class RecipientTest : FcmMessageTestBase() {
@Test
fun `GroupPm#getPmUsersString gives the correct string`() {
expect.that(Recipient.GroupPm(pmUsers = setOf(123, 234, 345)).getPmUsersString())
.isEqualTo("123, 234, 345")
expect.that(Recipient.GroupPm(pmUsers = setOf(123,234,345)).getPmUsersString())
.isEqualTo("123,234,345")
}
@Test
fun `GroupPm#getPmUsersString correctly reorders user ids`() {
expect.that(Recipient.GroupPm(pmUsers = setOf(234, 123, 23, 345)).getPmUsersString())
.isEqualTo("23, 123, 234, 345")
expect.that(Recipient.GroupPm(pmUsers = setOf(234,123,23,345)).getPmUsersString())
.isEqualTo("23,123,234,345")
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/notification/__tests__/notification-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ describe('getNarrowFromNotificationData', () => {
test('on notification for a group message returns a group narrow', () => {
const notification = {
recipient_type: 'private',
pm_users: '1, 2, 4',
pm_users: '1,2,4',
};
const usersById = new Map([
[1, { email: '[email protected]' }],
Expand All @@ -47,7 +47,7 @@ describe('getNarrowFromNotificationData', () => {
test('do not throw when users are not found; return null', () => {
const notification = {
recipient_type: 'private',
pm_users: '1, 2, 4',
pm_users: '1,2,4',
};
const usersById = new Map();

Expand Down
3 changes: 2 additions & 1 deletion src/notification/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,8 @@ export const getNarrowFromNotificationData = (
const emails = [];
const idStrs = data.pm_users.split(',');
for (let i = 0; i < idStrs.length; ++i) {
const user = usersById.get(+idStrs[i]);
const id = parseInt(idStrs[i], 10);
const user = usersById.get(id);
if (!user) {
return null;
}
Expand Down

0 comments on commit 92bee0e

Please sign in to comment.