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

Identify narrows with user IDs and stream IDs, not emails and names #4333

Closed
gnprice opened this issue Dec 10, 2020 · 4 comments · Fixed by #5205
Closed

Identify narrows with user IDs and stream IDs, not emails and names #4333

gnprice opened this issue Dec 10, 2020 · 4 comments · Fixed by #5205
Assignees

Comments

@gnprice
Copy link
Member

gnprice commented Dec 10, 2020

This is a major component of both #3764 and #3918. Currently we describe PM narrows (both 1:1 and group) by emails of the people involved, and stream and topic narrows by the stream name (and in the latter case the topic.) We should instead use the numeric user IDs and stream IDs, for all the reasons described in #3764 and #3918.

This has been a stubborn issue to resolve because there are so many places in the code that we create and consume Narrow objects, and so many different places the data for them comes from, and all of those need to migrate. I've had several draft branches for doing this, starting with some in 2020-02. The work I'm currently doing on #4035, of which #4332 just now is the latest, will bring it closer by centralizing the fiddly bits in the PM case, and in particular making it so that we always have user IDs on hand at the points where we construct PM narrows.

One symptom of this issue is described at #3230. (Though probably a rare one; found internally, and has no user reports.)

@Ayushdubey86
Copy link

I would like to work on this!

@gnprice
Copy link
Member Author

gnprice commented Dec 14, 2020

Hi @Ayushdubey86 ! This issue is a pretty complex one, and I'm already working on it. To find good issues to take on, take a look at our "help wanted" label. See also our contribution guidelines, and say hello in #mobile on chat.zulip.org.

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.
@timabbott
Copy link
Member

Just adding a note that we'll want to open a zulip/zulip issue for doing the cleanup discussed here:
https://chat.zulip.org/#narrow/stream/3-backend/topic/stream.20administrators/near/1124311 (noting we need to wait 6 months before doing it) once we're no longer using the name field in subscription/update events.

@chrisbobbe
Copy link
Contributor

Thanks, Tim!

once we're no longer using the name field in subscription/update events.

I was mistaken about this; we actually don't use it, so the name field can be removed right away, as Greg points out in the CZO discussion.

chrisbobbe pushed a commit that referenced this issue Feb 1, 2022
In the migration tests, it is kind of messy that we have to update
a previous migration's test.  That happens because we're running
all the migrations to completion.

As the comment at the "whole sequence" test says about a related
point: this is probably not a great design for continuing to add
more migrations in this sequence.  But pretty soon we're going to
cap it off and write future migrations in a new framework, which
will come with its own way of writing tests.  So for the moment
just live with it.

Fixes-partly: #4333
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants