notifications: Correctly stringify pmUsers to fix navigation to group…#3922
notifications: Correctly stringify pmUsers to fix navigation to group…#3922gnprice merged 3 commits intozulip:masterfrom
Conversation
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks @chrisbobbe for spotting this! Will be glad to get this bug fixed. 🙂
Looking again at the output, I'd like to take one further step to tighten up the format; see below.
| @Test | ||
| fun `GroupPm#getPmUsersString gives the correct string`() { | ||
| expect.that(Recipient.GroupPm(pmUsers = setOf(123, 234, 345)).getPmUsersString()) | ||
| .isEqualTo("123, 234, 345") |
There was a problem hiding this comment.
Let's eliminate the spaces too.
If this already is enough that the JS code understands it, that's good :) -- but the format used elsewhere is purely comma-separated, so may as well make it consistent.
| fun `GroupPm#getPmUsersString correctly reorders user ids`() { | ||
| expect.that(Recipient.GroupPm(pmUsers = setOf(234, 123, 345)).getPmUsersString()) | ||
| .isEqualTo("123, 234, 345") | ||
| } |
There was a problem hiding this comment.
This test can be made a bit stronger by testing that e.g. 123 sorts after 23 -- i.e. the sorting is of the numbers, and not a lexicographic sort of their ASCII representations.
chrisbobbe
force-pushed
the
pr-fix-group-pm-notif-nav
branch
from
03b9f22 to
8d869d5
Compare
|
OK, I've pushed these changes. :) |
chrisbobbe
force-pushed
the
pr-fix-group-pm-notif-nav
branch
from
8d869d5 to
92bee0e
Compare
|
I'm resolving merge conflicts and clarifying a commit message for this now. ("Incorrectly stringified" is vague; I'm tracking down what exactly the output was, which I should have done in the first place.) |
chrisbobbe
force-pushed
the
pr-fix-group-pm-notif-nav
branch
2 times, most recently
from
9093d0d to
8153f84
Compare
gnprice
force-pushed
the
pr-fix-group-pm-notif-nav
branch
from
8153f84 to
4dc39d1
Compare
… PMs. Navigation to a group PM on pressing a notification was broken because pmUsers was incorrectly stringified in GroupPm.getPmUsersString. E.g., for a group PM among user IDs 13313, 13434, and 13657, it would stringify to (newline added for readability): "GroupPm(pmUsers=[13313, 13434, 13657]), GroupPm(pmUsers=[13313, 13434, 13657]), GroupPm(pmUsers=[13313, 13434, 13657])" It should instead stringify to "13313, 13434, 13657". (Later in this series of commits, we remove the space.) Fix and add a test.
To be reverted in the next commit. In the previous commit, we changed the return value of GroupPm.getPmUsersString in our Kotlin code from garbage separated by ', ' to numbers separated by ', '. This commit aims to prove that ', '-separated numbers will be handled correctly, at least as far as our tests can tell. But we really want it to be ','-separated (no space), which we do in the next commit.
', ' 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 each element of that array was converted to a number. Also, replace the confusing + syntax, as in +idStrs[i], with parseInt.
gnprice
force-pushed
the
pr-fix-group-pm-notif-nav
branch
from
4dc39d1 to
766dcdd
Compare
|
Merged! Thanks again. Just made small tweaks to the commit messages. Evidently I dropped this PR back in March, oops. >_> Looks like it probably got lost in the chaos of all suddenly switching to work from home, early in the pandemic. I ran across the bug today, it looked familiar, and I found this PR again. |
2 participants
… PMs.
Navigation to a group PM on pressing a notification was broken
because pmUsers was incorrectly stringified in
GroupPm.getPmUsersString. Fix, and add a test for it.