Skip to content

Add moved label for topic/stream changes #1331

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add moved label for topic/stream changes #1331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6d48472
api_types: Add edit_history to Message.
Mar 30, 2023
b212f6f
helper: Add a function analyse_edit_history.
Mar 30, 2023
39cc8cb
messages: Add condition for moved label
Mar 30, 2023
47b451d
model: update _handle_update_messages_event.
Mar 30, 2023
1a940dc
test_helper: Added tests for analyse_edit_history.
Apr 14, 2023
9c54b8e
test_messages: Add and modify test for message labels.
Mar 30, 2023
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
1 change: 1 addition & 0 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1073,6 +1073,7 @@ def empty_index(
stream_msg_ids_by_stream_id=defaultdict(set, {}),
topic_msg_ids=defaultdict(dict, {}),
edited_messages=set(),
moved_messages=set(),
topics=defaultdict(list),
search=set(),
messages=defaultdict(
Expand Down
128 changes: 128 additions & 0 deletions tests/helper/test_helper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from zulipterminal.config.keys import primary_key_for_command
from zulipterminal.helper import (
Index,
analyse_edit_history,
canonicalize_color,
classify_unread_counts,
display_error_if_present,
Expand Down Expand Up @@ -112,6 +113,133 @@ def test_index_messages_narrow_user_multiple(
assert index_messages(messages, model, model.index) == index_user_multiple


@pytest.mark.parametrize(
("content_changed, stream_changed, old_topic, current_topic, expected_result"),
[
case(
False,
True,
None,
None,
{"edited_messages": {12345}, "moved_messages": set()},
id="only_stream_changed",
),
case(
True,
False,
None,
None,
{"edited_messages": {12345}, "moved_messages": set()},
id="only_content_changed",
),
case(
True,
True,
"Topic B",
"Topic C",
{"edited_messages": {12345}, "moved_messages": set()},
id="both_stream_and_content_changed",
),
case(
True,
False,
"Topic B",
"Topic C",
{"edited_messages": {12345}, "moved_messages": set()},
id="both_content_and_topic_changed",
),
case(
False,
False,
"Topic D",
"Topic E",
{"edited_messages": set(), "moved_messages": {12345}},
id="actual_topic_change",
),
case(
False,
True,
"Topic F",
"Topic G",
{"edited_messages": {12345}, "moved_messages": set()},
id="topic_and_stream_changed",
),
case(
False,
False,
"✔ Topic I",
"✔ Topic J",
{"edited_messages": set(), "moved_messages": {12345}},
id="topic_changed_from_resolved_to resolved_but_different_topic",
),
case(
False,
False,
"Topic K",
"Topic L",
{"edited_messages": set(), "moved_messages": {12345}},
id="topic_changed_when_both_are_unresolved",
),
case(
False,
False,
"✔ Topic M",
"Topic M",
{"edited_messages": set(), "moved_messages": set()},
id="only_resolved_topic_to_unresolved_topic",
),
case(
False,
False,
"Topic M",
"✔ Topic M",
{"edited_messages": set(), "moved_messages": set()},
id="only_unresolved_topic_to_resolved_topic",
),
case(
False,
False,
"✔ Topic N",
"Topic M",
{"edited_messages": set(), "moved_messages": {12345}},
id="resolved_topic_to_changed_unresolved_topic",
),
case(
False,
False,
"Topic M",
"✔ Topic N",
{"edited_messages": set(), "moved_messages": {12345}},
id="unresolved_topic_to_changed_resolved_topic",
),
],
)
def test_analyse_edit_history(
content_changed: bool,
stream_changed: bool,
current_topic: str,
old_topic: str,
expected_result: Dict[str, Any],
initial_index: Index,
) -> None:
msg_id = 12345
expected_index = dict(
initial_index,
edited_messages=expected_result["edited_messages"],
moved_messages=expected_result["moved_messages"],
)
analyse_edit_history(
msg_id,
initial_index,
content_changed,
stream_changed,
current_topic,
old_topic,
)

assert initial_index == expected_index


@pytest.mark.parametrize(
"edited_msgs",
[
Expand Down
17 changes: 13 additions & 4 deletions tests/model/test_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2150,7 +2150,8 @@ def test_notify_users_hides_PM_content_based_on_user_setting(
"topic_msg_ids": {
10: {"new subject": {1}, "old subject": {2}},
},
"edited_messages": {1},
"edited_messages": set(),
"moved_messages": {1},
"topics": {10: []},
},
False,
Expand Down Expand Up @@ -2186,7 +2187,8 @@ def test_notify_users_hides_PM_content_based_on_user_setting(
"topic_msg_ids": {
10: {"new subject": {1, 2}, "old subject": set()},
},
"edited_messages": {1},
"edited_messages": set(),
"moved_messages": {1},
"topics": {10: []},
},
False,
Expand Down Expand Up @@ -2292,6 +2294,7 @@ def test_notify_users_hides_PM_content_based_on_user_setting(
10: {"new subject": set(), "old subject": {1, 2}},
},
"edited_messages": {1},
"moved_messages": set(),
"topics": {10: ["new subject", "old subject"]},
},
False,
Expand Down Expand Up @@ -2329,7 +2332,8 @@ def test_notify_users_hides_PM_content_based_on_user_setting(
"topic_msg_ids": {
10: {"new subject": {1}, "old subject": {2}},
},
"edited_messages": {1},
"edited_messages": set(),
"moved_messages": {1},
"topics": {10: []},
},
False,
Expand Down Expand Up @@ -2363,6 +2367,7 @@ def test_notify_users_hides_PM_content_based_on_user_setting(
10: {"new subject": set(), "old subject": {1, 2}},
},
"edited_messages": {1},
"moved_messages": set(),
"topics": {10: ["new subject", "old subject"]},
},
False,
Expand Down Expand Up @@ -2401,6 +2406,7 @@ def test_notify_users_hides_PM_content_based_on_user_setting(
10: {"new subject": {3}, "old subject": {1, 2}},
},
"edited_messages": set(),
"moved_messages": set(),
"topics": {10: []}, # This resets the cache
},
False,
Expand Down Expand Up @@ -2439,6 +2445,7 @@ def test_notify_users_hides_PM_content_based_on_user_setting(
10: {"new subject": {3}, "old subject": {1, 2}},
},
"edited_messages": set(),
"moved_messages": set(),
"topics": {10: ["new subject", "old subject"]},
},
True,
Expand Down Expand Up @@ -2476,7 +2483,8 @@ def test_notify_users_hides_PM_content_based_on_user_setting(
"topic_msg_ids": {
10: {"new subject": {1}, "old subject": {2}},
},
"edited_messages": {1},
"edited_messages": set(),
"moved_messages": {1},
"topics": {10: ["new subject", "old subject"]},
},
True,
Expand Down Expand Up @@ -2512,6 +2520,7 @@ def test__handle_update_message_event(
10: {"new subject": set(), "old subject": {1, 2}},
},
"edited_messages": set(),
"moved_messages": set(),
"topics": {10: ["new subject", "old subject"]},
}
mocker.patch(MODEL + "._update_rendered_view")
Expand Down
44 changes: 39 additions & 5 deletions tests/ui_tools/test_messages.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1135,18 +1135,52 @@ def test_main_view_compact_output(
assert len(view_components) == 1
assert isinstance(view_components[0], Padding)

def test_main_view_generates_EDITED_label(
self, mocker, messages_successful_response
@pytest.mark.parametrize(
[
"message_edited",
"message_moved",
"expected_left_padding",
"expected_label_text",
],
[
case(True, False, 7, "EDITED", id="message_indexed_to_edited_messages"),
case(False, True, 6, "MOVED", id="message_indexed_to_moved_messages"),
case(
False,
False,
Comment on lines +1146 to +1150
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no True, True case here. Your code doesn't seem to limit this from occurring (considering indexing and events), and I'm not sure if you expect it to?

If we do care about moved and edited being disjoint sets, then somewhere the code needs amending. However, the logic here doesn't currently care - but we need to define that's the case.

0,
"EDITED",
id="message_neither_index_to_edited_messages_or_moved_messages",
Comment on lines +1152 to +1153
Copy link
Collaborator

Choose a reason for hiding this comment

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

If neither, then why is this text "EDITED"?

),
],
)
def test_main_view_generates_EDITED_or_MOVED_label(
self,
mocker,
messages_successful_response,
message_edited,
message_moved,
expected_left_padding,
expected_label_text,
):
messages = messages_successful_response["messages"]
for message in messages:
self.model.index["edited_messages"].add(message["id"])
msg_id = message["id"]
message_index = {
"edited_messages": {msg_id} if message_edited else set(),
"moved_messages": {msg_id} if message_moved else set(),
}
self.model.index = dict(
self.model.index,
edited_messages=message_index["edited_messages"],
moved_messages=message_index["moved_messages"],
)
msg_box = MessageBox(message, self.model, message)
view_components = msg_box.main_view()

label = view_components[0].original_widget.contents[0]
assert label[0].text == "EDITED"
assert label[1][1] == 7
assert label[0].text == expected_label_text
assert label[1][1] == expected_left_padding

@pytest.mark.parametrize(
"to_vary_in_last_message, update_required",
Expand Down
1 change: 1 addition & 0 deletions zulipterminal/api_types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ class Message(TypedDict, total=False):
# NOTE: `subject_links` in Zulip 2.1; deprecated from Zulip 3.0 / ZFL 1
subject_links: List[str]
is_me_message: bool
edit_history: List[Dict[str, Any]]
reactions: List[Dict[str, Any]]
submessages: List[Dict[str, Any]]
flags: List[MessageFlag]
Expand Down
69 changes: 66 additions & 3 deletions zulipterminal/helper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,12 @@
import requests
from typing_extensions import Literal, ParamSpec, TypedDict

from zulipterminal.api_types import Composition, EmojiType, Message
from zulipterminal.api_types import (
RESOLVED_TOPIC_PREFIX,
Composition,
EmojiType,
Message,
)
from zulipterminal.config.keys import primary_key_for_command
from zulipterminal.config.regexes import (
REGEX_COLOR_3_DIGIT,
Expand Down Expand Up @@ -110,6 +115,7 @@ class Index(TypedDict):
topic_msg_ids: Dict[int, Dict[str, Set[int]]]
# Extra cached information
edited_messages: Set[int] # {message_id, ...}
moved_messages: Set[int]
topics: Dict[int, List[str]] # {topic names, ...}
search: Set[int] # {message_id, ...}
# Downloaded message data by message id
Expand All @@ -126,6 +132,7 @@ class Index(TypedDict):
stream_msg_ids_by_stream_id=defaultdict(set),
topic_msg_ids=defaultdict(dict),
edited_messages=set(),
moved_messages=set(),
topics=defaultdict(list),
search=set(),
# mypy bug: https://github.com/python/mypy/issues/7217
Expand Down Expand Up @@ -305,6 +312,44 @@ def set_count(id_list: List[int], controller: Any, new_count: int) -> None:
controller.update_screen()


def analyse_edit_history(
msg_id: int,
index: Index,
Comment on lines +316 to +317
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a lot of duplicate lines in this method, index[something].add(msg_id), which suggests the design may be simplified.

Rather than passing in these parameters so that we can modify the Index, can we return eg. a string for each case? That means we decouple this from the index, which should simplify the test, in addition to knowing that it doesn't depend on the previous edited/moved state - or anything else in the index, which we can't say right now.

content_changed: bool,
stream_changed: bool,
current_topic: Any = None,
old_topic: Any = None,
Comment on lines +320 to +321
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please avoid Any if possible.

I also don't see that you need a default parameter here - you're always passing the current and old topic values, even if they are None?

I also see that both current and old topic must be None or str together, or else the middle conditional breaks. In other similar cases we have an assert at the top of the function to ensure that 'coordinated' values are used correctly by callers. The alternative is to define the parameter as a data structure, where the entire parameter can then be None or multiple values.

) -> None:
resolve_change = False
resolved_prefix = RESOLVED_TOPIC_PREFIX + " "
Comment on lines +323 to +324
Copy link
Collaborator

Choose a reason for hiding this comment

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

These both only apply to the middle section, so move them there?

if content_changed or stream_changed:
index["edited_messages"].add(msg_id)
elif old_topic:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This also depends upon current_topic being non-None, which as it stands is not guaranteed. In other similar cases we have an assert at the top of the function to ensure that 'coordinated' values are used correctly. The alternative is to define the parameter as a data structure.

old_topic_resolved = old_topic.startswith(resolved_prefix)
current_topic_resolved = current_topic.startswith(resolved_prefix)
if not current_topic_resolved:
if old_topic_resolved:
if old_topic[2:] != current_topic:
Copy link
Collaborator

Choose a reason for hiding this comment

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

There seems to be a symmetry in this logic, but made less clear since when comparing topics you check the slice within a conditional. It could be that this symmetry is not sufficient to allow the code to be simplified completely, but I'd be interested to see how far it might go if you assigned eg. current_name and old_name for the pieces without the prefix and compared those instead?

index["moved_messages"].add(msg_id)
if old_topic[2:] == current_topic:
resolve_change = True
if not old_topic_resolved and not current_topic_resolved:
index["moved_messages"].add(msg_id)
else:
if old_topic_resolved and old_topic[2:] != current_topic[2:]:
index["moved_messages"].add(msg_id)
if not old_topic_resolved:
if current_topic[2:] != old_topic:
index["moved_messages"].add(msg_id)
if current_topic[2:] == old_topic:
resolve_change = True

else:
index["edited_messages"].add(msg_id)
Comment on lines +347 to +348
Copy link
Collaborator

Choose a reason for hiding this comment

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

What part of the input parameter space does this cover? Should this be an error branch instead?

if msg_id not in index["moved_messages"] and not resolve_change:
index["edited_messages"].add(msg_id)
Comment on lines +349 to +350
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this intended to apply only to the middle conditional branch above? The other branches both add to edited_messages and don't affect moved_messages. Or is this intended to reflect some prior state in moved_messages?

While I understand you translated this function from the JS code, I don't have the context to know how much you changed. I don't want to unnecessarily change the structure of a function, but equally if it can be simplified, it seems good to do so :)



def index_messages(messages: List[Message], model: Any, index: Index) -> Index:
"""
STRUCTURE OF INDEX
Expand Down Expand Up @@ -433,8 +478,26 @@ def index_messages(messages: List[Message], model: Any, index: Index) -> Index:
narrow = model.narrow
for msg in messages:
if "edit_history" in msg:
index["edited_messages"].add(msg["id"])

stream_changed = False
content_changed = False
current_topic = None
old_topic = None
for edit_history_event in msg["edit_history"]:
if "prev_content" in edit_history_event:
content_changed = True
if "prev_stream" in edit_history_event:
stream_changed = True
if "prev_topic" in edit_history_event:
current_topic = edit_history_event["topic"]
old_topic = edit_history_event["prev_topic"]
Comment on lines +485 to +492
Copy link
Collaborator

Choose a reason for hiding this comment

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

This piece of code is not tested. I don't have an issue with setting the flags on looping through the history, but the current/old_topic data depends on the order of cycling through the history, so could be set to different values multiple times in the history. The test for analyse_edit_history isn't (yet!) in this same commit for me to easily understand if it would matter; it may also be clearer if we can simplify the logic of that function.

While ideally the index method - and so this code - could be tested, I'd feel more comfortable still without a test, if the logic of the passed-in current/old values was clearer, for example if the precise current/old values don't matter in the analyse function. Otherwise I'd question why we don't need to track/analyse all the previous topic changes.

Comment on lines +491 to +492
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that topic and prev_topic were both only added in Zulip 5.0, so we'll need a workaround for this, and any test of this part may need to depend upon the feature level - though note that prev_stream works fine since it just skips it if it is not present.

analyse_edit_history(
msg["id"],
index,
content_changed,
stream_changed,
current_topic,
old_topic,
)
index["messages"][msg["id"]] = msg
if not narrow:
index["all_msg_ids"].add(msg["id"])
Expand Down
Loading