Convert from names to stream IDs to identify streams #3918

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.

Sign up for GitHub

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

Jump to bottom

Closed

gnprice opened this issue Feb 20, 2020 · 6 comments · Fixed by #5268

Labels

Member

gnprice commented Feb 20, 2020

This is a meta-issue which covers a large number of independent tasks. Filing it in order to have a place to point to refer to the overall effort.

This is parallel to #3764, but for streams. Just like users can change their email addresses, streams can have their names changed, and so the permanent, numeric stream IDs are a better way to refer to a stream.

Most of the discussion in #3764 applies to this as well.

Tasks for specific instances of this include #3244, and #2760 which we fixed last summer.

gnprice added the api migrations label

gnprice mentioned this issue

Fix last connectFlowFixMe; and delete it! #3914

Merged

gnprice mentioned this issue

Renaming a stream makes it appear as 'unsubscribed' #2782

Closed

gnprice added a commit to gnprice/zulip-mobile that referenced this issue


          messageActionSheet tests: Clean up some stream names + IDs.

8fc6317

The Subscription object has both a stream name and a stream ID, and
so to keep the test data well-formed these should be kept aligned in
referring to the same stream.  Similarly the Message object.

Right now to make the test pass it's enough to set the stream name,
because we don't use the stream ID; but that's a case of zulip#3918 and
should change, after which the mismatch will be a live cause of test
failures.

We could do this by passing a stream object to `eg.makeSubscription`
and to `eg.streamMessage`.  But the most natural one to use is
`eg.stream`; and the example data already refers to `eg.stream` by
default in all the natural places, among them `eg.streamMessage()`
and `eg.subscription`, so simply use that.

gnprice mentioned this issue

Add sender_id and stream_id properties to Outbox #3998

Closed

gnprice added a commit to gnprice/zulip-mobile that referenced this issue


          messageActionSheet tests: Clean up some stream names + IDs.

8106d5d

The Subscription object has both a stream name and a stream ID, and
so to keep the test data well-formed these should be kept aligned in
referring to the same stream.  Similarly the Message object.

Right now to make the test pass it's enough to set the stream name,
because we don't use the stream ID; but that's a case of zulip#3918 and
should change, after which the mismatch will be a live cause of test
failures.

We could do this by passing a stream object to `eg.makeSubscription`
and to `eg.streamMessage`.  But the most natural one to use is
`eg.stream`; and the example data already refers to `eg.stream` by
default in all the natural places, among them `eg.streamMessage()`
and `eg.subscription`, so simply use that.

chrisbobbe pushed a commit to gnprice/zulip-mobile that referenced this issue


          messageActionSheet tests: Clean up some stream names + IDs.

2201bd8

The Subscription object has both a stream name and a stream ID, and
so to keep the test data well-formed these should be kept aligned in
referring to the same stream.  Similarly the Message object.

Right now to make the test pass it's enough to set the stream name,
because we don't use the stream ID; but that's a case of zulip#3918 and
should change, after which the mismatch will be a live cause of test
failures.

We could do this by passing a stream object to `eg.makeSubscription`
and to `eg.streamMessage`.  But the most natural one to use is
`eg.stream`; and the example data already refers to `eg.stream` by
default in all the natural places, among them `eg.streamMessage()`
and `eg.subscription`, so simply use that.

gnprice mentioned this issue

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

Closed

chrisbobbe mentioned this issue

internal_link: allow streamID without stream name #4413

Merged

AkashDhiman added a commit to AkashDhiman/zulip-mobile that referenced this issue


          ActionSheet [nfc]: Modify code to use stream_id instead of stream name.

a5c30c0

This commit removes stream name as a parameter to functions related to
`TopicActionSheet` and instead introduces stream_id in its place. Stream
ID is a better parameter since it always remains constant and many of
the functions present in action sheet were already dependent on
stream_id.

Related: zulip#3918

AkashDhiman added a commit to AkashDhiman/zulip-mobile that referenced this issue


          ActionSheet [nfc]: Modify code to use stream_id instead of stream name.

ae3e22e

This commit removes stream name as a parameter to functions related to
`TopicActionSheet` and instead introduces stream_id in its place. Stream
ID is a better parameter since it always remains constant and many of
the functions present in action sheet were already dependent on
stream_id.

Related: zulip#3918

AkashDhiman mentioned this issue

Add mark as read option in TopicActionSheet and Modify TopicActionSheet to use stream_id. #4635

Merged

AkashDhiman added a commit to AkashDhiman/zulip-mobile that referenced this issue


          topicActions [nfc]: Update deleteMessagesForTopic to use streamId.

2afa453

This also involved changing the call to `deleteMessagesForTopic`
appropriately.
The change is done to facilitate migration to `stream_id` instead of
stream name for identifying streams.

Related: zulip#3918

AkashDhiman added a commit to AkashDhiman/zulip-mobile that referenced this issue


          ActionSheet [nfc]: Modify code to use stream_id instead of stream name.

c01db96

This commit removes stream name as a parameter to functions related to
`TopicActionSheet` and instead introduces streamId in its place. Stream
ID is a better parameter since it always remains constant.

Related: zulip#3918

AkashDhiman added a commit to AkashDhiman/zulip-mobile that referenced this issue


          topicActions [nfc]: Update deleteMessagesForTopic to use streamId.

53e8a9c

This also involved changing the call to `deleteMessagesForTopic`
appropriately.
The change is done to facilitate migration to `stream_id` instead of
stream name for identifying streams.

Related: zulip#3918

AkashDhiman added a commit to AkashDhiman/zulip-mobile that referenced this issue


          ActionSheet [nfc]: Modify code to use stream_id instead of stream name.

6d9505f

This commit removes stream name as a parameter to functions related to
`TopicActionSheet` and instead introduces streamId in its place. Stream
ID is a better parameter since it always remains constant.

Related: zulip#3918

AkashDhiman added a commit to AkashDhiman/zulip-mobile that referenced this issue


          topicActions [nfc]: Update deleteMessagesForTopic to use streamId.

1acb3e8

This also involved changing the call to `deleteMessagesForTopic`
appropriately.
The change is done to facilitate migration to `stream_id` instead of
stream name for identifying streams.

Related: zulip#3918

AkashDhiman added a commit to AkashDhiman/zulip-mobile that referenced this issue


          ActionSheet [nfc]: Modify code to use stream_id instead of stream name.

e630dd0

This commit removes stream name as a parameter to functions related to
`TopicActionSheet` and instead introduces streamId in its place. Stream
ID is a better parameter since it always remains constant.

Related: zulip#3918

Contributor

chrisbobbe commented May 13, 2021

Tasks for specific instances of this include #3244, and #2760 which we fixed last summer.

Do I understand correctly that #3244 is the last remaining instance of this? #4635 has a commit that's marked as fixing that.

AkashDhiman added a commit to AkashDhiman/zulip-mobile that referenced this issue


          topicActions [nfc]: Update deleteMessagesForTopic to use streamId.

c56372b

This also involved changing the call to `deleteMessagesForTopic`
appropriately.
The change is done to facilitate migration to `stream_id` instead of
stream name for identifying streams.

Related: zulip#3918

AkashDhiman added a commit to AkashDhiman/zulip-mobile that referenced this issue


          ActionSheet [nfc]: Modify code to use stream_id instead of stream name.

2e0b7c9

This commit removes stream name as a parameter to functions related to
`TopicActionSheet` and instead introduces streamId in its place. Stream
ID is a better parameter since it always remains constant.

Related: zulip#3918

AkashDhiman added a commit to AkashDhiman/zulip-mobile that referenced this issue


          topicActions [nfc]: Use streamId in deleteMessagesForTopic.

833efb8

This also involved changing the call to `deleteMessagesForTopic`
appropriately.

The change is done to facilitate migration to `stream_id` instead of
stream name for identifying streams.

Related: zulip#3918

AkashDhiman added a commit to AkashDhiman/zulip-mobile that referenced this issue


          ActionSheet [nfc]: Make use of stream_id instead of stream name.

44788c2

This commit removes stream name as a parameter to functions related
to `TopicActionSheet` and instead introduces streamId in its place.
Stream ID is a better parameter since it always remains constant.

Related: zulip#3918

AkashDhiman added a commit to AkashDhiman/zulip-mobile that referenced this issue


          topicActions [nfc]: Use streamId in deleteMessagesForTopic.

0206d89

This also involves changing the call to `deleteMessagesForTopic`
appropriately.

The change is done as a step to migrate to using `stream_id`
instead of stream name for identifying streams.

Related: zulip#3918

AkashDhiman added a commit to AkashDhiman/zulip-mobile that referenced this issue


          topicActions [nfc]: Use streamId in deleteMessagesForTopic.

1a6f56c

This also involves changing the call to `deleteMessagesForTopic`
appropriately.

The change is done as a step to migrate to using `stream_id`
instead of stream name for identifying streams.

Related: zulip#3918

AkashDhiman added a commit to AkashDhiman/zulip-mobile that referenced this issue


          topicActions [nfc]: Use streamId in deleteMessagesForTopic.

This also involves changing the call to `deleteMessagesForTopic`
appropriately.

The change is done as a step to migrate to using `stream_id`
instead of stream name for identifying streams.

Related: zulip#3918

gnprice pushed a commit to AkashDhiman/zulip-mobile that referenced this issue


          topicActions [nfc]: Use streamId in deleteMessagesForTopic.

ad3c65d

This also involves changing the call to `deleteMessagesForTopic`
appropriately.

The change is done as a step to migrate to using `stream_id`
instead of stream name for identifying streams.

Related: zulip#3918

Member Author

gnprice commented Sep 14, 2021

Tasks for specific instances of this include #3244, and #2760 which we fixed last summer.

Do I understand correctly that #3244 is the last remaining instance of this? #4635 has a commit that's marked as fixing that.

No, unfortunately -- those were just the instances that had specific tasks at the time.

#4333 is the biggest remaining instance of this -- we identify narrows by stream name rather than stream ID. It connects to other instances which may not have specific tasks in our tracker.

#3998 will help, giving us stream IDs on Outbox values. That will let various code that consumes those -- especially all the code that consumes Message | Outbox -- stop using names and use the IDs instead.

Member Author

gnprice commented Sep 14, 2021

The victory condition for this issue is basically: if you try removing the name property from the definitions of the Stream and Subscription types, Flow should find only a handful of errors (i.e. places we're still using the property), all of which should be purely legitimate uses. (Which basically means just showing the name to the user in the UI.)

One quick approximation to that is to grep for streamName -- I've tried to shift to that name for most locals and function parameters that refer to a stream name. If you try that now, it finds 131 matches (or 103 with -w). The bulk of those are uses that ought to be using stream IDs. I believe most of them at this point are part of #4333 -- they're using the stream name to identify a stream or topic narrow.

So I think at this point the sequence for resolving this issue will be:

Finish Add sender_id and stream_id properties to Outbox #3998. With Add stream_id to Outbox values #5000 merged, the next step there is to wait for a few weeks after a release with Add stream_id to Outbox values #5000 is out; then we can merge a migration that finishes Add sender_id and stream_id properties to Outbox #3998.
- Then do any easy followups on that.
Then do Identify narrows with user IDs and stream IDs, not emails and names #4333. This may be fairly complex. We did the PMs side of this in the series of PRs leading up to narrow: Stop storing emails! #4382.
Then mop up any remaining instances of this issue. I think at that point these will be small, if there are any.

gnprice mentioned this issue

outbox: Make stream_id required on StreamOutbox #5056

Merged

gnprice mentioned this issue

stream IDs: StreamItem and TopicItem props and callbacks #5069

Merged

gnprice mentioned this issue

stream IDs: sharing, links, tests #5183

Merged

gnprice mentioned this issue

message_actions: Add move message option #5189

Open

gnprice mentioned this issue

narrow: Store stream IDs instead of names! #5205

Merged

Member Author

gnprice commented Feb 1, 2022

Then mop up any remaining instances of this issue. I think at that point these will be small, if there are any.

OK, with #5205 merged, this is now indeed mostly done!

I went and looked through the codebase for remaining places where we're using the name of a stream. This meant searching the code for the patterns streamName, stream.name, and 3918, and also making the name property on Stream and Subscription be write-only/reads-forbidden and seeing where Flow complained.

The results were:

In notifications, up until getNarrowFromNotificationData. This is Include stream ID in push notifications for stream messages zulip#18067, for which a fix was just merged the other day. Servers starting with 5.0 will send the stream ID as well as the name.
api.subscriptionAdd, api.subscriptionRemove, api.createStream. This is Switch webapp to subscribe/unsubscribe from streams by ID zulip#10744, currently open.
The muted-topics model. This is also open in the server: chat discussion.
api.sendMessage. This got stream-ID support in server 2.0, so it's already in all the server versions we support.
showStreamInHomeNarrow, which will be simple to fix.
Several references that don't ultimately actually use the stream name, notably around TopicItem.
And then a number of places where the stream name is actually appropriate to use, because it's for showing it to the user in the UI. (The main/unreads screen; the stream-list screens; recipient headers in the message list; the app bar on the message-list and lightbox screens; autocompleting #-mentions; a few others.)

For the places that need server support, I'll want to make sure that

there's a server issue open, if it's not already fixed in main;
there's a comment with either TODO(server-x.y) or a reference to the server issue;
where we can't yet rely on server support, we at least rely on stream IDs internally and convert from/to names at the boundary with the server -- so in particular we store muted topics by stream ID.

Then when those are the only remaining uses of stream names that should be IDs, I think it may be time to declare victory on this issue. We'll get to the TODO(server-x.y) comments when the time comes, and we can track the still-open server issues in their own way.

gnprice added a commit to gnprice/zulip-mobile that referenced this issue


          mute: Maintain muted-topics state by stream ID, not name

ca0f299

This represents one of the last significant pieces left for zulip#3918.

gnprice mentioned this issue

mute: Maintain muted-topics state by stream ID, not name #5223

Merged

gnprice added a commit that referenced this issue


          mute: Maintain muted-topics state by stream ID, not name

b159cea

This represents one of the last significant pieces left for #3918.

gnprice added a commit to gnprice/zulip-mobile that referenced this issue


          api [nfc]: Add TODO comments for stream IDs in subscribe/unsubscribe

10543a8

Our goal in this series isn't yet to resolve the issue zulip#3918, but to
get to where it's easy to *audit* our code for the remaining things we
need to do to complete it.

There are two kinds of searches one wants to do in such an audit:

 (a) Searching for *known* instances of the issue, in order to find
     remaining items to work on and eventually to confirm there are
     none left.  For this, we want simple systematic patterns to
     search for.

 (b) Searching for yet-*unknown* instances of the issue.  This is
     inherently fuzzier; one searches for patterns like /stream.name/
     or /streamName/ or even /stream.*name/i to try to find code that
     refers to stream names, which produce a lot of matches including
     some legitimate uses and some that aren't even about stream names.

     For this, the main thing we can do is make it as simple as
     possible when reading such search results to confirm that a given
     spot is a known instance of the issue, in order to focus on those
     that might be unknown.

For the sake of (a), we add TODO-3918 comments at each instance
that's actionable on its own, and references to zulip#3918 where there's
another TODO comment that the fix blocks on.

For the sake of (b), when a given instance of the issue pops up in a
number of scattered places (like the definition and each call site of
a given function that takes a stream name), we add references to zulip#3918
at each of those places even when we only need one TODO comment.

This commit adds such comments for two related instances of the issue,
and the next few commits will add them for others.

gnprice added a commit to gnprice/zulip-mobile that referenced this issue


          stream [nfc]: Add TODO-3918 to all remaining uses of stream names

ef26b84

Or in some cases, there's an existing TODO-server comment, and we
just add a reference to the issue.  (These are places where there's
nothing to be done about the issue before simply dropping support
for older servers, so the proper thing for the TODO itself to refer
to is the server version.)

I believe this covers all remaining instances of zulip#3918.  That means
that every place we refer to a stream name should either:
 * be a legitimate use of stream names;
 * have a TODO-3918 comment; or
 * have a different kind of TODO comment (as it happens, always a
   TODO-server comment), and a mention of zulip#3918.

As a result, a search like `git grep TODO..3918` will find all the
remaining places we need to act in order to close issue zulip#3918,
according to the plan I described here:
  zulip#3918 (comment)

The broader search `git grep -w 3918` will additionally find all the
places that are (known) instances of the issue but are blocked purely
on server upgrades.

gnprice mentioned this issue

Fix some more stream names to be IDs, and mark the rest #5246

Merged

gnprice added a commit that referenced this issue


          api [nfc]: Add TODO comments for stream IDs in subscribe/unsubscribe

6c261ba

Our goal in this series isn't yet to resolve the issue #3918, but to
get to where it's easy to *audit* our code for the remaining things we
need to do to complete it.

There are two kinds of searches one wants to do in such an audit:

 (a) Searching for *known* instances of the issue, in order to find
     remaining items to work on and eventually to confirm there are
     none left.  For this, we want simple systematic patterns to
     search for.

 (b) Searching for yet-*unknown* instances of the issue.  This is
     inherently fuzzier; one searches for patterns like /stream.name/
     or /streamName/ or even /stream.*name/i to try to find code that
     refers to stream names, which produce a lot of matches including
     some legitimate uses and some that aren't even about stream names.

     For this, the main thing we can do is make it as simple as
     possible when reading such search results to confirm that a given
     spot is a known instance of the issue, in order to focus on those
     that might be unknown.

For the sake of (a), we add TODO-3918 comments at each instance
that's actionable on its own, and references to #3918 where there's
another TODO comment that the fix blocks on.

For the sake of (b), when a given instance of the issue pops up in a
number of scattered places (like the definition and each call site of
a given function that takes a stream name), we add references to #3918
at each of those places even when we only need one TODO comment.

This commit adds such comments for two related instances of the issue,
and the next few commits will add them for others.

gnprice added a commit that referenced this issue


          stream [nfc]: Add TODO-3918 to all remaining uses of stream names

bb1d3ab

Or in some cases, there's an existing TODO-server comment, and we
just add a reference to the issue.  (These are places where there's
nothing to be done about the issue before simply dropping support
for older servers, so the proper thing for the TODO itself to refer
to is the server version.)

I believe this covers all remaining instances of #3918.  That means
that every place we refer to a stream name should either:
 * be a legitimate use of stream names;
 * have a TODO-3918 comment; or
 * have a different kind of TODO comment (as it happens, always a
   TODO-server comment), and a mention of #3918.

As a result, a search like `git grep TODO..3918` will find all the
remaining places we need to act in order to close issue #3918,
according to the plan I described here:
  #3918 (comment)

The broader search `git grep -w 3918` will additionally find all the
places that are (known) instances of the issue but are blocked purely
on server upgrades.

gnprice mentioned this issue

Fix all remaining stream names to be IDs, modulo server support #5268

Merged

gnprice closed this as completed in #5268

Member Author

gnprice commented May 12, 2022

The muted-topics model. This is also open in the server: chat discussion.

This got filed as a server issue: zulip/zulip#21015.

There's now a draft API change and backend implementation: zulip/zulip#21251.

Member Author

gnprice commented May 12, 2022

Here's also a quote from my description of #5268 which closed this issue. It's an update of my comment above from a month before it was closed, explaining the status of the followup work and how it's tracked separately from this issue:

As forecast at #3918 (comment) , there remain some places where we use stream names that should be IDs, but these are limited to interacting with the server in places where it doesn't support IDs. For those, moreover:

We nevertheless make sure to convert promptly to stream IDs at the edge (e.g., maintaining the muted-topics model in terms of stream IDs). This means that while we're still exposed to bugs if a stream rename races with another interaction we have with the server about that stream, we at least don't hit a bug simply because a stream was renamed while the app was open -- in other words, the race window is much narrower.

Some of them were fixed in the server a while ago, and have comments like TODO(server-2.0):. We'll take care of those sometime soon in a followup to api: Start relying more on non-ancient servers #5100, as we concretely drop support for ancient server versions we already don't support.

Some (well, one -- notifications) were fixed in the server recently, and have comments like TODO(server-5.0):. We'll take care of those circa late 2023, when we drop support for all server releases that exist today. In the meantime, we make sure to take advantage of stream IDs when the server is new enough to provide them.

Some aren't fixed yet: Switch webapp to subscribe/unsubscribe from streams by ID zulip#10744. These have TODO(server-future): comments.

gnprice mentioned this issue

Adapt topic-mute model to new user_topic API #5380

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment