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

stream-settings: Remove redundant actually_filter_streams function call. #9089

Closed
wants to merge 1 commit into from

Conversation

shubham-padia
Copy link
Member

Fixes #9033.
After adding a newly created stream to the top of the stream list,
call to actually_filter_streams in stream_events.mark_subscribed
rerendered the filter_table and the stream list was refreshed.
Actually_filter streams was introduced to rerender the subscriber
list but stream_edit.rerender_subscribers_list takes care of it rn.

actually_filter_streams was added in 20af49b to solve #6797.
rerender_subscribers_list was added in 9090d6b to solve #6955.

Although no visual change was noticed, this change may break things if actually_filter_streams was also used here for any other purpose, cc'ing @brockwhittaker for that.

Fixes zulip#9033.
After adding a newly created stream to the top of the stream list,
call to actually_filter_streams in stream_events.mark_subscribed
rerendered the filter_table and the stream list was refreshed.
Actually_filter streams was introduced to rerender the subscriber
list but stream_edit.rerender_subscribers_list takes care of it rn.
@zulipbot
Copy link
Member

Hello @zulip/server-streams members, this pull request was labeled with the "area: stream settings" label, so you may want to check it out!

@timabbott
Copy link
Sponsor Member

@YJDave can you help with testing/reviewing this?

@YJDave
Copy link
Collaborator

YJDave commented Apr 14, 2018

Sure, I tested it locally seems fine. But I will debug this more today further in actually_filter_streams function, as we did many changes recently.

@timabbott
Copy link
Sponsor Member

I don't think this is correct -- actually_filter_streams is for if you've typed something in the "Filter streams" list at the top of the "manage streams" UI; I'd worry about this introducing bugs where a stream that matches a filter doesn't appear or something.

@YJDave
Copy link
Collaborator

YJDave commented Apr 21, 2018

Yes, I played with this locally, works fine with filter. We can merge. 👍

@timabbott
Copy link
Sponsor Member

Regardless of whether that specific bug is there, can you explain why this works and won't create some other problem?

@YJDave
Copy link
Collaborator

YJDave commented Apr 29, 2018

Ok, so actually_filter_streams function filter the list of streams present in left hand side bar in stream settings. If there is no query passed to filter, it will sort all streams.
Now on stream creation event this function is called automatically and as there is no query entered by user it will sort all streams in list.
If we remove this function call from stream creation, it won't throw error or return unexpected result on filter.

@timabbott
Copy link
Sponsor Member

OK, after some careful testing, I think this is correct, but I was confused by the original description that referenced rerender_subscribers_list.

Merged, thanks @shubham-padia!

One change I made: We use formal English in commit messages so that they're understandable to people everywhere. So I replaced the abbreviation "rn" (I assume for "right now") with "already" in the commit message.

@timabbott timabbott closed this Apr 30, 2018
@shubham-padia shubham-padia deleted the create_stream branch September 8, 2018 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants