Skip muted_streams/topics in getting next unread topic. #442
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
neiljp
reviewed
Jul 24, 2019
There was a problem hiding this comment.
@amanagr Good catch on noticing this issue 👍
There's good extra work here in addition to the actual fix for this issue, and not all the elements seem necessary (or complete) for that fix. You could build extra things into this PR, but my thought would be to split this PR into up to 3 PRs:
- This fix in behavior (1: add tests for existing method, 2: refactor (maybe?), 3/2: fix behavior using existing muting datastructures directly (as elsewhere), extending existing tests
- Encapsulating muted stream/topic checks, as per the current first commit, but as per my inline comment, incorporating a transition towards the rest of the codebase using that/those methods
- Moving to use a
model_fixturethrough the code.
Thoughts?
| @@ -75,6 +76,31 @@ def msg_box(mocker, messages_successful_response): | |||
|
|
|||
| # --------------- Model Fixtures ---------------------------------------------- | |||
|
|
|||
| @pytest.fixture | |||
| def model_fixture(mocker, initial_data, user_profile): | |||
There was a problem hiding this comment.
This seems great at first glance as another standardized fixture, but while you use it in the following commit, you could illustrate its broader use and appeal in this commit by refactoring some other tests using this new fixture.
This can be used to run a Model's functions inside other tests.
Added tests.
Use model fixture to make use of is_muted_stream function.
Added tests.
amanagr
force-pushed
the
exclude_muted
branch
2 times, most recently
from
189e6a0
to
899d116
Compare
|
Heads up @amanagr, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
rht
reviewed
Jun 26, 2022
| @@ -386,6 +386,19 @@ def fetch_all_topics(self, workers: int) -> None: | |||
| if not result] | |||
| raise ServerConnectionFailure(", ".join(failures)) | |||
|
|
|||
| def is_muted_stream(self, stream_id: int) -> bool: | |||
| if stream_id in self.muted_streams: | |||
There was a problem hiding this comment.
This can be just return stream_id in self.muted_streams.
4 tasks
neiljp
added a commit
to neiljp/zulip-terminal
that referenced
this pull request
Aug 15, 2022
Logic originally developed by Aman Agrawal (amanagr) in zulip#442. Updated to take into account: - already-added (slightly different) is_muted[stream|topic] methods - migration of get_next_unread_topic into model (simplifying tests) Also ensure the current behavior is maintained for now by explicitly sorting the unread counts, which previous relied upon the initial and subsequent data being ordered. Tests added. Co-authored by: Aman Agrawal <f2016561@pilani.bits-pilani.ac.in>
4 tasks
neiljp
added a commit
to neiljp/zulip-terminal
that referenced
this pull request
Aug 16, 2022
Logic originally developed by Aman Agrawal (amanagr) in zulip#442. Updated to take into account: - already-added (slightly different) is_muted[stream|topic] methods - migration of get_next_unread_topic into model (simplifying tests) Also ensure the current behavior is maintained for now by explicitly sorting the unread counts, which previous relied upon the initial and subsequent data being ordered. Tests added. Co-authored by: Aman Agrawal <f2016561@pilani.bits-pilani.ac.in>
neiljp
added a commit
that referenced
this pull request
Aug 16, 2022
Logic originally developed by Aman Agrawal (amanagr) in #442. Updated to take into account: - already-added (slightly different) is_muted[stream|topic] methods - migration of get_next_unread_topic into model (simplifying tests) Also ensure the current behavior is maintained for now by explicitly sorting the unread counts, which previous relied upon the initial and subsequent data being ordered. Tests added. Co-authored by: Aman Agrawal <f2016561@pilani.bits-pilani.ac.in>
mounilKshah
pushed a commit
to mounilKshah/zulip-terminal
that referenced
this pull request
Oct 15, 2022
Logic originally developed by Aman Agrawal (amanagr) in zulip#442. Updated to take into account: - already-added (slightly different) is_muted[stream|topic] methods - migration of get_next_unread_topic into model (simplifying tests) Also ensure the current behavior is maintained for now by explicitly sorting the unread counts, which previous relied upon the initial and subsequent data being ordered. Tests added. Co-authored by: Aman Agrawal <f2016561@pilani.bits-pilani.ac.in>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR skips muted topics/streams with
nhotkey.