Notification Bot message breaks renamed or removed streams #426

Closed
melbel opened this Issue Jan 15, 2016 · 12 comments

Projects

None yet

4 participants

@melbel
melbel commented Jan 15, 2016

When someone creates a new stream, Notification Bot writes a message to the Stream Zulip with the content

Streams
just created a new stream . Subscribe to <stream>

with the Subscribe being a button and changing to Unsubscribe afterwards if one is subscribed.

Now if that stream gets renamed or deleted later, the button still exists, and a click will actually create a new stream of the original name. IMO this is highly undesirable since it leads to loads of confusions like we had yesterday when someone created a stream with a typo, immediately asked an admin to rename it and all the sudden we had two streams when people kept klicking on the button in the original announce message.

I have not had the time to go and try understand exactly where this ends up in the backend but maybe it is possible to simply turn off auto creation of streams upon subscribe.

@timabbott
Member

Yeah I noticed this recently as well. I think there are probably two solutions:
(1) Adding some code when a stream is renamed to rerender any of these announce messages that contain the stream name button; this has some performance risks but is probably manageable.
(2) It should be possible to fix this by using the stream ID, rather than the stream name, and then we can send an event to update the rendering to match the new name in those buttons; this is probably the cleanest approach.

@melbel
melbel commented Jan 15, 2016

From a POLA standpoint, I can see issues with both solutions:

(1) would change facts. If I create stream foo, the bot says so. If that stream later gets renamed, changing the message to bar would make the announcement wrong. I did not create a stream bar, I created foo.
(2) Will probably confuse users for all but the most obvious typos. I click on subscribe to stream foo and get subscribed to bar.

Additionaly, what happens to deleted streams? (1) would not change anything - the system would still create that stream new. (2) might throw an error depending on the implementation or simply subscribe me to the deleted stream if the changes are not implemented carefully.

So, why not do as I originally suggested and simply throw an error if one tries to subscribe to a stream that does not exist in that form any more? There already is one - and probably the most common - situation where clicking that button will fail necessarily with a rather nondescript error message: when someone creates a stream that later gets made private. One can not subscribe freely any more and a new one cannot be created as well since the names would collide. Still that stream is not visible even to the admins and the original creation message also cannot be changed without exposing the stream again.

Or is the subscription code so deeply interwoven with stream creation that reacting differently simply is not feasible? Just being curious :-)

@timabbott
Member

Oh, I see, you're suggesting making the button error if the stream doesn't exist rather than the current "create a stream" which is definitely wrong; I'd actually only noticed that it was weird the button had the strong stream name and had missed that detail. I think that'd be a great incremental improvement over the current situation. It should be fairly doable to fix it; just tweak the call to list_to_streams in zerver/views/streams.py to pass autocreate=False for this codepath.

I do think the nicer version of this would be that the message (or rendering of the button) gets edited if the stream stops existing or being public to clarify that the change happened so that you're not left with this confusing button referencing a nonexistant stream in the history, but that's fixing a clearly lower-priority failure mode than the button creating a new stream with the old name.

@sharmaeklavya2
Contributor

I tried to replicate this bug but I couldn't. I created a stream called 'Juggling' from an admin account. I checked a standard account and there was a PM from the notification bot with a button 'Subscribe to juggling'.

Then I renamed the stream from the admin account to 'Bounce Juggling'. Then I subscribed the stream from the standard user. It seemed that nothing happened. I checked the subscriptions page and 'Juggling' was not there. There was a new stream 'Bounce Juggling' to which the standard user was not subscribed.

@timabbott
Member

The issue is the notifications with a "subscribe to X" button that the notification bot sends to the "zulip" stream (if it exists) would still have the stream name "Juggling", not "Bounce Juggling", and the button won't work.

@sharmaeklavya2
Contributor

The stream 'zulip' exists but there was nothing posted there. I forgot to mention in my previous comment that the subscribe button (on the PM between the standard user and the notification bot) still says 'Subscribe to juggling' and not 'Subscribe to Bounce Juggling'. The button didn't work as you said, but it didn't create a new stream of the original name as the OP says.

@timabbott
Member

Ahh, well the important issue is that the message ends up stuck with the wrong name; the other issue the OP described would be a consequence of the button being stuck with the wrong name. I'm pretty sure the right fix for this is to have the reference be to the stream by ID, not name, which won't change if the stream is later renamed.

@timabbott timabbott added this to the Likely next milestone milestone Aug 1, 2016
@timabbott
Member

I think it's possible we want to solve this problem in part by replacing this link with a "Narrow to stream x" link, since I think most of the time folks will want to narrow to a stream before subscribing to it.

@paxapy
Contributor
paxapy commented Nov 15, 2016

Hi
@timabbott you suggested to replace this subscribe button with new #stream link, but I don't understand how it will fix the issue? This link also use stream name to navigate to a stream, and if it renamed, the link will be broken, isn't it?

@timabbott
Member

I don't think so, because we can use the stream ID in data-stream-id to control what happens when you click the new #stream links. I guess I'm not sure whether we actually do that right now, but it's easy to test :).

@paxapy
Contributor
paxapy commented Nov 15, 2016

no, we don't
should I add this click handling functionality here? Or it's better to do that in separate PR?

@timabbott
Member

Maybe do it in the same PR but a separate commit within that PR?

@timabbott timabbott added a commit that closed this issue Dec 16, 2016
@paxapy @timabbott paxapy + timabbott click_handlers.js: Handle clicks on stream links using data-stream-id.
This causes Zulip to correctly handle clicks on announcements for
renamed streams.

The stream name that the user typed will still be displayed, which
isn't ideal, but is probably the best we can do without more invasive
rerendering of old messages.

Fixes #426.
28bd9d0
@timabbott timabbott closed this in 28bd9d0 Dec 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment