Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 10, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c475d18
core: Add duration argument to report_* with default value of 3.
srdeotarse Apr 7, 2022
c456528
model: Improve reporting upon moving/splitting topic within stream.
srdeotarse Apr 7, 2022
7196234
tests: model: Add change_later test case for test_update_stream_message.
srdeotarse Apr 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 42 additions & 19 deletions tests/model/test_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,78 +834,101 @@ def test_update_private_message(
],
)
@pytest.mark.parametrize(
"req, old_topic, footer_updated",
"req, old_topic, expected_report_success",
[
(
case(
{
"message_id": 1,
"propagate_mode": "change_one",
"content": "hi!",
"topic": "Some topic",
},
"Some topic",
False,
None, # None as footer is not updated.
),
(
case(
{
"message_id": 1,
"propagate_mode": "change_one",
"topic": "Topic change",
},
"Old topic",
True,
"You changed one message's topic from #stream > Old topic to #stream > Topic change.",
),
(
{"message_id": 1, "propagate_mode": "change_all", "topic": "Old topic"},
case(
{
"message_id": 1,
"propagate_mode": "change_all",
"topic": "Old topic",
},
"Old topic",
False,
None, # None as footer is not updated.
),
(
case(
{
"message_id": 1,
"propagate_mode": "change_later",
"content": ":smile:",
"topic": "terminal",
},
"terminal",
False,
None, # None as footer is not updated.
),
(
case(
{
"message_id": 1,
"propagate_mode": "change_later",
"content": ":smile:",
"topic": "new_terminal",
},
"old_terminal",
"You changed some messages' topic from #stream > old_terminal to #stream > new_terminal.",
),
case(
{
"message_id": 1,
"propagate_mode": "change_one",
"content": "Hey!",
"topic": "grett",
},
"greet",
True,
"You changed one message's topic from #stream > greet to #stream > grett.",
),
(
case(
{
"message_id": 1,
"propagate_mode": "change_all",
"content": "Lets party!",
"topic": "party",
},
"lets_party",
True,
"You changed all messages' topic from #stream > lets_party to #stream > party.",
),
],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my followup comment in the next review.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)
def test_update_stream_message(
self, mocker, model, response, return_value, req, old_topic, footer_updated
self,
mocker,
model,
response,
return_value,
req,
old_topic,
expected_report_success,
old_stream_name="stream",
):
self.client.update_message = mocker.Mock(return_value=response)
model.index["messages"][req["message_id"]]["subject"] = old_topic

model.index["messages"][req["message_id"]][
"display_recipient"
] = old_stream_name
result = model.update_stream_message(**req)

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.")
if result and expected_report_success is not None:
report_success.assert_called_once_with(expected_report_success)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else:
report_success.assert_not_called()

Expand Down
12 changes: 6 additions & 6 deletions zulipterminal/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,23 +437,23 @@ def show_typing_notification(self) -> None:
self.is_typing_notification_in_progress = False
self.view.set_footer_text()

def report_error(self, text: str) -> None:
def report_error(self, text: str, duration: int = 3) -> None:
"""
Helper to show an error message in footer
"""
self.view.set_footer_text(text, "task:error", 3)
self.view.set_footer_text(text, "task:error", duration)

def report_success(self, text: str) -> None:
def report_success(self, text: str, duration: int = 3) -> None:
"""
Helper to show a success message in footer
"""
self.view.set_footer_text(text, "task:success", 3)
self.view.set_footer_text(text, "task:success", duration)

def report_warning(self, text: str) -> None:
def report_warning(self, text: str, duration: int = 3) -> None:
"""
Helper to show a warning message in footer
"""
self.view.set_footer_text(text, "task:warning", 3)
self.view.set_footer_text(text, "task:warning", duration)

def search_messages(self, text: str) -> None:
# Search for a text in messages
Expand Down
18 changes: 16 additions & 2 deletions zulipterminal/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,10 +576,24 @@ def update_stream_message(
response = self.client.update_message(request)
display_error_if_present(response, self.controller)
if response["result"] == "success":
old_topic = self.index["messages"][message_id].get("subject", None)
message = self.index["messages"][message_id]
stream_name = message.get("display_recipient", None)
old_topic = message.get("subject", None)
new_topic = request["topic"]

recent_moved_msgs = ""
if old_topic != new_topic:
self.controller.report_success("You changed a message's topic.")
if propagate_mode == "change_one":
recent_moved_msgs = "one"
one_message = f"You changed {recent_moved_msgs} message's topic from #{stream_name} > {old_topic} to #{stream_name} > {new_topic}."
self.controller.report_success(one_message)
else:
if propagate_mode == "change_all":
recent_moved_msgs = "all"
if propagate_mode == "change_later":
recent_moved_msgs = "some"
some_all_messages = f"You changed {recent_moved_msgs} messages' topic from #{stream_name} > {old_topic} to #{stream_name} > {new_topic}."
self.controller.report_success(some_all_messages)

return response["result"] == "success"

Expand Down