model: Improve reporting upon moving/splitting topic. #1178

srdeotarse · 2022-03-24T13:31:35Z

What does this PR do?
This PR improves reporting upon moving a message by editing it's topic.

Partial fix for #1172

Tested?

Manually
Existing tests (adapted, if necessary)
New tests added (for any new behavior)
Passed linting & tests (each commit)

Commit flow

Notes & Questions

Interactions

Visual changes

model.py.-.zulip-terminal.WSL_.Ubuntu.-.Visual.Studio.Code.2022-03-23.21-44-36.mp4

neiljp

@srdeotarse Thanks for looking at this, including giving the tests a go 👍

This doesn't appear to pass tests (CI), which is not what you have in the PR description.

Since the re-indexing occurs outside the current method where the message occurs, it's possible we may need to do that elsewhere.

neiljp · 2022-03-26T23:08:03Z

zulipterminal/model.py

            if old_topic != new_topic:
-                self.controller.report_success("You changed a message's topic.")
+                self.controller.report_success(
+                    f"You changed {no_of_messages} message's topic from #{stream_name} > {old_topic} to #{stream_name} > {new_topic}."


I'm not sure if we have a standard way to do this using report_success, but this may be clearer if we can provide a highlight style to group the stream+topic in each case.

This may require changes to existing code, so that could be a follow-up.

After fixing the format for reporting message upon moving topics, we can look into highlighting as well.

zulipterminal/model.py

srdeotarse · 2022-03-31T18:23:59Z

Rebased with current main branch and updated mypy. Still showing error below @neiljp -

Successfully installed autoflake-1.4 autopep8-1.6.0 flake8-4.0.1 isort-5.10.1 jedi-0.18.1 lxml-stubs-0.4.0 mypy-0.942 parso-0.8.3 pudb-2022.1.1 pycodestyle-2.8.0 pyflakes-2.4.0 pytz-2022.1 snakeviz-2.1.1 types-docutils-0.18.0 types-pygments-2.9.17 types-setuptools-57.4.11 zulip-term-0.6.0+git
(zt_venv) srdeotarse@LAPTOP-VC2MC73M:~/zulip-terminal$ make check
================================================================================
Import order (isort)
--------------------------------------------------------------------------------
All files have imports sorted according to rules.
================================================================================
Type consistency (mypy)
--------------------------------------------------------------------------------
Running mypy for `zulipterminal`.
zulipterminal/model.py:584: error: Invalid index type "Optional[int]" for "Dict[int, Set[int]]"; expected type "int"
Found 1 error in 1 file (checked 37 source files)
Running mypy for `tests`.
Success: no issues found in 15 source files
================================================================================
LINTING FAILED
make: *** [makefile:22: lint] Error 1

neiljp · 2022-04-02T06:18:48Z

@srdeotarse Was there a reason you closed/reopened this? Generally there's no need to do so.

neiljp · 2022-04-02T06:20:54Z

zulipterminal/model.py

-                self.controller.report_success("You changed a message's topic.")
+                if recent_moved_msgs == "one":
+                    self.controller.report_success(
+                        f"You changed {recent_moved_msgs} message's topic from #{stream_name} > {old_topic} to #{stream_name} > {new_topic}."


This is identical except for the apostrophe placement? If so, it'll be simpler to integrate the difference into the variable, and avoid the need for a conditional here.

This was what I referred to by simplifying the code in the next review.

tests/model/test_model.py

neiljp · 2022-04-02T06:25:25Z

tests/model/test_model.py

@@ -888,24 +907,54 @@ def test_update_private_message(
                    "topic": "party",
                },
                "lets_party",
+                "party",
+                "stream",
+                "change_all",
                True,
            ),
        ],


Given the longer test here now, it would be useful to also add inline test ids.

See my followup comment in the next review.

OK, so you have the case around each set of test parameters; for the ids see other tests which use a named parameter to case (ie. pytest.param), ie. id=some_description).

You might try running this test as it is with -v and using -k to filter this test only, then see what happens when you add an id. What you see there is useful, in addition to it looking a little like a comment for that test parametrization.

srdeotarse · 2022-04-02T06:27:51Z

@neiljp I was trying to rebase with main to solve mypy error for Optional[int] and accidentally deleted my local branch.

neiljp

@srdeotarse Thanks for the update 👍

Other than the comments inline, I think we could simplify the code for readability, though it is functional as it is :)

One aspect this brings up is the short time given until the message disappears in the footer. For a simple solution, we could tune this manually with an optional parameter to pass through, which might be warranted as a preparatory commit before the one you have.

I've thought previously we could try to handle this automatically based on word length and reading speed, with some minimum, but that may need more tuning than a simple prep commit.

neiljp · 2022-04-03T22:42:48Z

tests/model/test_model.py

@@ -834,78 +834,130 @@ def test_update_private_message(
        ],
    )
    @pytest.mark.parametrize(
-        "req, old_topic, footer_updated",
+        "test_id, req, old_topic, new_topic, stream_name, propagate_mode, footer_updated",


Regarding test ids, I was referring to using pytest.param, which we've been importing as case. This makes description of each parametrized element easier.

zulipterminal/model.py

tests/model/test_model.py

neiljp

@srdeotarse I've mainly focused on the tests here, since once they're well-defined then refactoring the code to give the same behavior should be enforced - and good test-writing technique is a useful skill.

The first commit is good to have separate, but as per my inline comment I'd suggest you keep it very simple to simply allow passing a custom duration in the main commit, with a default of the current 3 as now. The version with a formula is worthy of it's own PR since the implementation is worthy of a separate discussion and this can be merged in the meantime.

tests/model/test_model.py

neiljp · 2022-04-06T17:26:35Z

tests/model/test_model.py

@@ -888,24 +907,54 @@ def test_update_private_message(
                    "topic": "party",
                },
                "lets_party",
+                "party",
+                "stream",
+                "change_all",
                True,
            ),
        ],


OK, so you have the case around each set of test parameters; for the ids see other tests which use a named parameter to case (ie. pytest.param), ie. id=some_description).

You might try running this test as it is with -v and using -k to filter this test only, then see what happens when you add an id. What you see there is useful, in addition to it looking a little like a comment for that test parametrization.

neiljp · 2022-04-06T17:27:04Z

tests/model/test_model.py

        self.client.update_message.assert_called_once_with(req)
        assert result == return_value
        self.display_error_if_present.assert_called_once_with(response, self.controller)
        report_success = model.controller.report_success
        if result and footer_updated:
-            report_success.assert_called_once_with("You changed a message's topic.")
+            report_success.assert_called_once_with(expected_report_success)


I hope you can see how much cleaner this is 👍
The simpler test logic is, the fewer bugs can be in the tests themselves - other than also making them easier to read.
You didn't write this original test, but even so imagine that you looked for this test first, and changed it to specify new behavior first, then updated the code to make it pass (see TDD - test driven development - if you've not come across it). We're wanting to make a specification for the behavior, not write test code that parallels the behavior.

neiljp · 2022-04-06T17:39:43Z

zulipterminal/core.py

+        self.view.set_footer_text(
+            text, "task:success", float(len(text.split()) * 6 / 13)
+        )  # Average reading speed = 130 wpm


I'd like to see more discussion and explanation of this logic when we do implement this, and we should more broadly apply it to the other report_* methods too. This appears to work for now, but here I'd prefer to go for simpler optional pass-through of a duration to this method for now, which we can iterate on later - ie. an additional named parameter with a default value.

We're discussing this in the stream now 👍

This is useful to ensure there is enough time for long/complex text to be read before the footer resets.

Tests updated.

This slipped out of being included in #1178. Test updated.

neiljp · 2022-04-10T01:07:00Z

@srdeotarse Thanks for the contribution 👍 I made a few minor textual adjustments which you might be able to identify manually, or find using git diff or git range-diff, I but merged otherwise as-is 🎉

srdeotarse force-pushed the issue-1172-improve-reporting-moving/spliting-topic branch from 4d493b5 to a616169 Compare March 24, 2022 13:37

zulipbot added the size: L [Automatic label added by zulipbot] label Mar 24, 2022

srdeotarse force-pushed the issue-1172-improve-reporting-moving/spliting-topic branch from a616169 to a448fab Compare March 24, 2022 13:44

neiljp reviewed Mar 26, 2022

View reviewed changes

neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Mar 26, 2022

srdeotarse force-pushed the issue-1172-improve-reporting-moving/spliting-topic branch from a448fab to f16fbaf Compare March 30, 2022 10:27

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Mar 30, 2022

srdeotarse force-pushed the issue-1172-improve-reporting-moving/spliting-topic branch from f16fbaf to 9d1610d Compare March 30, 2022 10:47

srdeotarse closed this Mar 31, 2022

srdeotarse deleted the issue-1172-improve-reporting-moving/spliting-topic branch March 31, 2022 17:26

srdeotarse restored the issue-1172-improve-reporting-moving/spliting-topic branch March 31, 2022 17:29

srdeotarse deleted the issue-1172-improve-reporting-moving/spliting-topic branch March 31, 2022 17:34

srdeotarse reopened this Mar 31, 2022

srdeotarse closed this Mar 31, 2022

srdeotarse restored the issue-1172-improve-reporting-moving/spliting-topic branch March 31, 2022 18:18

srdeotarse reopened this Mar 31, 2022

srdeotarse force-pushed the issue-1172-improve-reporting-moving/spliting-topic branch 2 times, most recently from 4b5095e to 94a3afe Compare April 2, 2022 06:13

zulipbot added size: L [Automatic label added by zulipbot] and removed size: XL [Automatic label added by zulipbot] labels Apr 2, 2022

neiljp reviewed Apr 2, 2022

View reviewed changes

tests/model/test_model.py Outdated Show resolved Hide resolved

neiljp reviewed Apr 2, 2022

View reviewed changes

tests/model/test_model.py Outdated Show resolved Hide resolved

neiljp reviewed Apr 2, 2022

View reviewed changes

srdeotarse force-pushed the issue-1172-improve-reporting-moving/spliting-topic branch from 94a3afe to 5137a37 Compare April 2, 2022 07:11

srdeotarse changed the title ~~model: Improve reporting upon moving/spliting topic.~~ model: Improve reporting upon moving/splitting topic. Apr 3, 2022

neiljp reviewed Apr 3, 2022

View reviewed changes

srdeotarse force-pushed the issue-1172-improve-reporting-moving/spliting-topic branch from 5137a37 to 42863b0 Compare April 4, 2022 13:04

neiljp added enhancement New feature or request area: UI General user interface update area: message rendering labels Apr 6, 2022

neiljp reviewed Apr 6, 2022

View reviewed changes

srdeotarse force-pushed the issue-1172-improve-reporting-moving/spliting-topic branch from 42863b0 to 705c14b Compare April 7, 2022 11:40

zulipbot added size: S [Automatic label added by zulipbot] size: L [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] size: S [Automatic label added by zulipbot] labels Apr 7, 2022

srdeotarse added 2 commits April 7, 2022 22:27

core: Add duration argument to report_* with default value of 3.

c475d18

This is useful to ensure there is enough time for long/complex text to be read before the footer resets.

model: Improve reporting upon moving/splitting topic within stream.

c456528

Tests updated.

neiljp force-pushed the issue-1172-improve-reporting-moving/spliting-topic branch from 11a63fe to a4a882c Compare April 9, 2022 23:50

neiljp added this to the Next Release milestone Apr 9, 2022

neiljp added PR ready to be merged PR has been reviewed & is ready to be merged and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Apr 9, 2022

tests: model: Add change_later test case for test_update_stream_message.

7196234

neiljp force-pushed the issue-1172-improve-reporting-moving/spliting-topic branch from a4a882c to 7196234 Compare April 10, 2022 00:03

neiljp merged commit 8db80e8 into zulip:main Apr 10, 2022

neiljp added a commit that referenced this pull request Apr 10, 2022

model: Extend duration of success message in update_stream_message.

212e4ac

This slipped out of being included in #1178. Test updated.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

model: Improve reporting upon moving/splitting topic. #1178

model: Improve reporting upon moving/splitting topic. #1178

srdeotarse commented Mar 24, 2022 •

edited

Loading

neiljp left a comment

neiljp Mar 26, 2022

srdeotarse Mar 30, 2022

srdeotarse commented Mar 31, 2022

neiljp commented Apr 2, 2022

neiljp Apr 2, 2022

neiljp Apr 3, 2022

neiljp Apr 2, 2022

neiljp Apr 3, 2022

neiljp Apr 6, 2022

srdeotarse commented Apr 2, 2022

neiljp left a comment

neiljp Apr 3, 2022

neiljp left a comment

neiljp Apr 6, 2022

neiljp Apr 6, 2022

neiljp Apr 6, 2022

neiljp Apr 10, 2022

neiljp commented Apr 10, 2022

model: Improve reporting upon moving/splitting topic. #1178

model: Improve reporting upon moving/splitting topic. #1178

Conversation

srdeotarse commented Mar 24, 2022 • edited Loading

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

srdeotarse commented Mar 31, 2022

neiljp commented Apr 2, 2022

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

srdeotarse commented Apr 2, 2022

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

neiljp commented Apr 10, 2022

srdeotarse commented Mar 24, 2022 •

edited

Loading