bugfix/migration: Support API migration for muted topics #744

preetmishra · 2020-07-28T14:28:01Z

This reflects the expected response change for muted_topics, from [stream_name, topic] to [stream_name, topic, date_muted] in Zulip version 3.0, Feature level 1 (see zulip/zulip@9340cd1).

Tests amended.

preetmishra · 2020-07-28T14:43:19Z

Logged #745 to track related improvements.

neiljp

@preetmishra Thanks for taking this up - other than the bugfix, this has the potential to allow extra details be listed about the muting 👍

While I'm not averse to introducing another data-structure in the Model, if it's necessary I would prefer it be marked private (leading underscore) and access wrapped in a method - particularly since it's use seems to be one particular check that is essentially being duplicated in multiple locations.

A nice extension to this would be supporting events to update these data-structures from the server, and of course potentially a UI to interact locally (you have some of these in #745 👍).

preetmishra · 2020-07-28T19:42:24Z

@neiljp Thanks for the review. I have updated the PR as per our discussion in the stream. This is larger than what we started with, but I believe the refactoring is worthwhile.

I have also added the proposed extension in #745.

zulipterminal/model.py

neiljp

@preetmishra Thanks for continuing to work on this; the new adjustments to allow testing against multiple versions may be the element making this more difficult to test.

The last commit is actually also a bugfix, right?

zulipterminal/model.py

zulipterminal/ui_tools/buttons.py

tests/ui/test_utils.py

neiljp · 2020-08-01T01:08:44Z

tests/ui/test_utils.py

+        model.is_in_muted_topics = mocker.Mock(return_value=(
+            [model.stream_dict[msg['stream_id']]['name'],
+             msg.get('subject', '')] in muted_topics
+        ))


This seems strange since it's basically reimplementing the function?

tests/model/test_model.py

zulipterminal/model.py

neiljp · 2020-08-01T01:36:08Z

zulipterminal/model.py

+        # NOTE: The expected response has been upgraded from
+        # [stream_name, topic] to [stream_name, topic, date_muted] in
+        # 3.0, Feature level 1.


It'd be useful to standardize our notes in the code for this; we could add that to the development section - this would make it easier to check for changes at different server versions and feature levels :)

preetmishra · 2020-08-01T03:56:14Z

@neiljp The first two commits make modification in the existing code to facilitate the migration. The last commit is the migration commit (should we call it a bugfix?). Note that the first two commits do not introduce any 'new' feature.

preetmishra · 2020-08-01T11:44:47Z

@neiljp Thanks for the review. In the latest update, I have simplified the parameterize blocks to avoid unnecessary mocking wherever feasible, made muted_topics protected with an underscore, added _muted_topics_dict along with an assert, and addressed other in-line suggestions (excluding the standard comment that we are discussing in zulip-terminal > Comments for server versions).

preetmishra · 2020-08-01T14:21:10Z

(Updated to rebase onto upstream/master.)

This decouples stream muting from is_muted_topic to allow independent muted_topics lookups. Additionally, this adds a docstring to clarify the method further. The method had been here for a while without any use-case. Besides, the current behaviour can easily be achieved by using is_muted_topic with is_muted_stream at any point. Test amended.

preetmishra · 2020-08-06T18:18:02Z

Updated to resolve conflicts.

neiljp

@preetmishra It's good to see this simplification and needed update (bugfix!) - other than also simplifying the code, it should allow simplifying the tests too :)

tests/core/test_core.py

neiljp · 2020-08-09T01:34:34Z

tests/ui/test_ui_tools.py

    ], ids=[
+        # Assuming 'Django', 'topic1' is muted via muted_topics.


Is this information actually used in this test now? This may not belong in this commit and the test name isn't that clear, but isn't this test focused on ensuring mark_muted is called if topics are muted in the model?

Other tests may also be simplified in a similar way after this, potentially.

Yes, it is needed here to understand why the values are set to True/False for is_muted_topic_return_value. Though, I have added an additional refactor commit to simplify it.

The is_muted util test is simplified in this commit itself. We might want to discuss test_classify_unread_counts - how would we like it to be handled? Maybe as a separate issue.

zulipterminal/helper.py

tests/model/test_model.py

zulipterminal/model.py

neiljp · 2020-08-09T02:28:57Z

tests/conftest.py

+        'server_feature_level:None',
+    ]
+)
+def muted_topics_dict(request):


This is the parametrization of muted_topics in the Model only - we should likely have one for initial data from server, and a test for initial data -> model data, which we don't have? (that was in the previous iteration?)

I didn't have that test in the previous iteration. Thanks for pointing that out.

I have added the test in the latest update with a local parametrize block in Model.py. It wouldn't be needed in conftest as we'd only test against processed_muted_topics everywhere else.

I'm actually thinking we might move the server response fixtures into a separate fixture file specific to the model in a similar way. What do you think?

The ones that are already in conftest.py or also the ones that are used in test_model.py?

In any case, I believe it is reasonable.

Tests amended with simplified parametrize blocks wherever feasible.

Test amended.

This reflects the expected server response change for muted_topics, from [stream_name, topic] to [stream_name, topic, date_muted] in Zulip version 3.0, Feature level 1 (see zulip/zulip@9340cd1). Test updated along with the addition of processed_muted_topics() fixture in conftest.py and added for muted topic's local conversion.

This simplifies the test parametrization and its body.

preetmishra · 2020-08-11T17:44:38Z

@neiljp Thanks for the review. This is ready with improved assertion against server response, improved tests, an additional refactor commit and other in-line suggestions that you mentioned.

zulipbot · 2020-08-13T17:32:46Z

Heads up @preetmishra, 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 upstream/master branch and resolve your pull request's merge conflicts accordingly.

neiljp · 2020-08-13T17:34:08Z

@preetmishra Thanks for this bugfix and compatibility change (as well as the refactors) 👍 As per PMs I've merged the first four 🎉

I think we can save the last commit for a separate set of changes to review separately. Other than unblocking the other PR, this also could be followed by:

a server-response fixture refactor (after discussion)
feature-level comments (we briefly discussed already)

zulipbot added the size: M [Automatic label added by zulipbot] label Jul 28, 2020

preetmishra changed the title ~~buttons/helper/model/utils: Support API migration for muted topics.~~ migration: Support API migration for muted topics Jul 28, 2020

preetmishra added the PR needs review PR requires feedback to proceed label Jul 28, 2020

preetmishra mentioned this pull request Jul 28, 2020

[WIP] Unify case sensitive topic names #733

Open

neiljp reviewed Jul 28, 2020

View reviewed changes

preetmishra force-pushed the migration-muted-topics branch from e8f08f3 to cf22b40 Compare July 28, 2020 19:30

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Jul 28, 2020

preetmishra force-pushed the migration-muted-topics branch from cf22b40 to b5feeb7 Compare July 28, 2020 19:53

sumanthvrao reviewed Jul 29, 2020

View reviewed changes

zulipterminal/model.py Outdated Show resolved Hide resolved

neiljp reviewed Aug 1, 2020

View reviewed changes

neiljp removed the PR needs review PR requires feedback to proceed label Aug 1, 2020

preetmishra force-pushed the migration-muted-topics branch from b5feeb7 to 38d90a7 Compare August 1, 2020 11:37

preetmishra added the PR needs review PR requires feedback to proceed label Aug 1, 2020

preetmishra force-pushed the migration-muted-topics branch from 38d90a7 to 5aace3b Compare August 1, 2020 14:16

preetmishra force-pushed the migration-muted-topics branch from 5aace3b to 95e5310 Compare August 6, 2020 18:17

neiljp added the PR blocks other PR label Aug 8, 2020

neiljp reviewed Aug 9, 2020

View reviewed changes

neiljp removed the PR needs review PR requires feedback to proceed label Aug 9, 2020

preetmishra changed the title ~~migration: Support API migration for muted topics~~ [WIP] migration: Support API migration for muted topics Aug 9, 2020

preetmishra added 3 commits August 11, 2020 22:49

refactor: buttons/helper/utils: Use is_muted_topic for lookups.

9d889ea

Tests amended with simplified parametrize blocks wherever feasible.

model: Restrict muted_topics access using a protected access modifier.

433fa40

Test amended.

refactor: tests: ui_tools: Simplify test_init_calls_mark_muted().

9989b91

This simplifies the test parametrization and its body.

preetmishra force-pushed the migration-muted-topics branch from 95e5310 to 9989b91 Compare August 11, 2020 17:28

preetmishra changed the title ~~[WIP] migration: Support API migration for muted topics~~ migration: Support API migration for muted topics Aug 11, 2020

preetmishra added the PR needs review PR requires feedback to proceed label Aug 11, 2020

zulipbot added the has conflicts label Aug 13, 2020

neiljp closed this Aug 13, 2020

neiljp added this to the Next Release milestone Aug 13, 2020

neiljp changed the title ~~migration: Support API migration for muted topics~~ bugfix/migration: Support API migration for muted topics Aug 13, 2020

preetmishra deleted the migration-muted-topics branch August 14, 2020 05:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix/migration: Support API migration for muted topics #744

bugfix/migration: Support API migration for muted topics #744

preetmishra commented Jul 28, 2020 •

edited

Loading

preetmishra commented Jul 28, 2020

neiljp left a comment

preetmishra commented Jul 28, 2020

neiljp left a comment

neiljp Aug 1, 2020

neiljp Aug 1, 2020

preetmishra commented Aug 1, 2020

preetmishra commented Aug 1, 2020

preetmishra commented Aug 1, 2020

preetmishra commented Aug 6, 2020

neiljp left a comment

neiljp Aug 9, 2020

preetmishra Aug 11, 2020

neiljp Aug 9, 2020

preetmishra Aug 11, 2020

neiljp Aug 13, 2020

preetmishra Aug 13, 2020

preetmishra Aug 13, 2020

preetmishra commented Aug 11, 2020

zulipbot commented Aug 13, 2020

neiljp commented Aug 13, 2020

		], ids=[
		# Assuming 'Django', 'topic1' is muted via muted_topics.

bugfix/migration: Support API migration for muted topics #744

bugfix/migration: Support API migration for muted topics #744

Conversation

preetmishra commented Jul 28, 2020 • edited Loading

preetmishra commented Jul 28, 2020

neiljp left a comment

Choose a reason for hiding this comment

preetmishra commented Jul 28, 2020

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

preetmishra commented Aug 1, 2020

preetmishra commented Aug 1, 2020

preetmishra commented Aug 1, 2020

preetmishra commented Aug 6, 2020

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

preetmishra commented Aug 11, 2020

zulipbot commented Aug 13, 2020

neiljp commented Aug 13, 2020

preetmishra commented Jul 28, 2020 •

edited

Loading