Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pin down the main forms of identifying a PM thread, and their conversion helpers #4035

Closed
gnprice opened this issue Apr 11, 2020 · 2 comments · Fixed by #4356
Closed

Pin down the main forms of identifying a PM thread, and their conversion helpers #4035

gnprice opened this issue Apr 11, 2020 · 2 comments · Fixed by #4356
Assignees
Labels
a-data-sync Zulip's event system, event queues, staleness/liveness P1 high-priority

Comments

@gnprice
Copy link
Member

gnprice commented Apr 11, 2020

There are a bunch of different ways we identify a PM conversation -- all of them variations on listing the people in the conversation:

  • Include self, or not.
    • Or include self only for the self-PM thread.
    • Or include self for group PMs (i.e. >= 3 people total), and for the self-PM thread (i.e. 1 person total), but not for non-self 1:1 threads (i.e. exactly 2 people total.)
  • Use user IDs, or emails.
  • Sort the items, usually.
    • AFAIK they always come sorted, either numerically for user IDs (even when stringified, like 2,11) or lexicographically for emails. But there may be exceptions in a corner I haven't looked closely into.
  • Use arrays, or comma-joined strings.

Much of this complexity comes to us from the Zulip server API -- this is an unfortunately unnecessarily-complex area of the API.

Then some of it is I think self-inflicted within the mobile codebase; partly from cases of #3764, and partly in other ways. And even what comes originally from the API, we could do a lot to handle more cleanly:

  • name things to clearly disambiguate
  • write comments on the nuances
  • where we can and it helps, convert things at the edge into a sane internal format, on the principle of the crunchy shell.

The spot in the code where this is most concentrated is src/utils/recipient.js... but that file is more the incomplete solution than it is the problem -- what's needed is partly to document, rename, and clean up some of those helpers, but also to deal with other places in the code where we're interacting with this tangled API surface, e.g. converting from one form to another, and doing so by hand without using that file's helpers and with no explicit reference to the different pieces of API involved.


This came up again just today at #3535 (comment) : it's basically the remaining obstacle to us completing that PR, fixing #3133, so that the PMs tab stops feeling broken for people who heavily use PMs.

It's been a recurring drag on development in certain areas for a long time, too, so it's really past time we took care of it.

I have a couple of draft branches with a lot of comments in them, from when I studied some of this a month or two ago. So, assigning to myself to at least kick things off by cleaning up and sending some of those changes.

@gnprice
Copy link
Member Author

gnprice commented Apr 15, 2020

I've just pushed some commits that make progress on this:
35be10b on display_recipient
a53cd60 and 6847b0a on unread_msgs

@gnprice
Copy link
Member Author

gnprice commented Dec 16, 2020

In addition to the commits mentioned above, some more of this work landed a couple of weeks later in #4040 (and its successor #4078.)

Then since the big changes in #4335 last week, this is pretty nearly complete, so I've just gone and taken a look to see what's left. I think it's just

  • the normalizeRecipients function, which does something rather fuzzy, but which fortunately we have very few remaining uses of;
  • a spot in unreadSelectors.js where we convert from our usual form of key for a PM thread (coming from a Narrow value) to the form needed in the unread_msgs data, and do so with a few lines of ad-hoc code with some dubious error handling.

Both of those will be easier to cleanly resolve when we have user IDs in our narrows -- #4333, which I'm in the middle of a series of PRs for (of which the latest is #4342.) In fact for one of them I already prepared a fix the other day that's in one of my draft branches for that work.

I've now gone and made a fix for the other, and I've moved both fixes toward the early part of the remaining drafts. They'll come shortly after we get user IDs available in our Narrow values, which I plan to send next after #4342 is merged. Then this will be complete!

gnprice added a commit to gnprice/zulip-mobile that referenced this issue Dec 28, 2020
The hairy logic in normalizeRecipients was one of the last few cases
of the confusion around PM recipients we're seeking to remove for zulip#4035.

It also represents one of the many places we're in the process of
converting from using emails to using numeric user IDs.

The case where this could potentially change behavior is where we
have several messages that are in the same PM conversation, but the
`display_recipient` property on different messages disagrees about
the email of some participant (e.g. because that user changed their
email between when we learned about one message and when we learned
about the other.)  The old code would fail to notice the messages
were in the same conversation, and so when showing them
consecutively in an interleaved narrow (like all-messages or a
search result), we'd show a new recipient bar between them.

With this fix, we correctly identify the messages as belonging to
the same conversation.
gnprice added a commit to gnprice/zulip-mobile that referenced this issue Jan 6, 2021
This completes a major objective of the long string of refactoring
that appeared in the series of PRs zulip#4330, zulip#4332, zulip#4335, zulip#4339, zulip#4342,
then zulip#4346, zulip#4356, zulip#4361, zulip#4364, and most recently zulip#4368.  After this
change, the portion of zulip#4333 that's about PMs, emails, and user IDs --
aka the portion of zulip#3764 that's about narrows -- is complete.

Still open from zulip#4333 is to convert stream and topic narrows from
stream names to stream IDs.  I'm hopeful that that will be simpler:
(a) unlike the situation for PMs, there's just one stream mentioned
at a time, so there's no question of sorting, and (b) there isn't
the "include self or not" complication that's bedeviled much of our
code related to PMs.  In other words, stream and topic narrows
don't suffer from zulip#4035.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-data-sync Zulip's event system, event queues, staleness/liveness P1 high-priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant