Skip to content

Handle read events from server #535

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 3 commits into
base: main
Choose a base branch
from
Open

Handle read events from server #535

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
e7621d5
helper: Store unread message data in index.
kaustubh-nair Mar 23, 2020
c2a1380
helper: Modify set_counts to use index['unread_msgs'].
kaustubh-nair Apr 3, 2020
0169add
model: Update index['unread_msgs'] when appending messages.
kaustubh-nair Apr 7, 2020
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
37 changes: 32 additions & 5 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,8 @@ def empty_index():
stream_msg_template['id']: stream_msg_template,
pm_template['id']: pm_template,
group_pm_template['id']: group_pm_template,
})
}),
'unread_msgs': defaultdict(dict),
})


Expand Down Expand Up @@ -732,10 +733,10 @@ def stream_dict(streams_fixture):
@pytest.fixture
def classified_unread_counts():
"""
Unread counts return by
helper.classify_unread_counts function.
Tuple of unread counts and unread_msg data
returned by helper.classify_unread_counts function.
"""
return {
return ({
'all_msg': 12,
'all_pms': 8,
'unread_topics': {
Expand All @@ -754,7 +755,33 @@ def classified_unread_counts():
1000: 3,
99: 1
}
}
}, {
1: {'type': 'private', 'sender_id': 1, 'flags': []},
2: {'type': 'private', 'sender_id': 1, 'flags': []},
3: {'type': 'private', 'sender_id': 2, 'flags': []},
4: {'type': 'stream', 'display_recipient': 'Some general stream',
'stream_id': 1000, 'subject': 'Some general unread topic',
'sender_ids': frozenset({1, 2}), 'flags': []},
5: {'type': 'stream', 'display_recipient': 'Some general stream',
'stream_id': 1000, 'subject': 'Some general unread topic',
'sender_ids': frozenset({1, 2}), 'flags': []},
6: {'type': 'stream', 'display_recipient': 'Some general stream',
'stream_id': 1000, 'subject': 'Some general unread topic',
'sender_ids': frozenset({1, 2}), 'flags': []},
7: {'type': 'stream', 'display_recipient': 'Secret stream',
'stream_id': 99, 'subject': 'Some private unread topic',
'sender_ids': frozenset({1, 2}), 'flags': []},
11: {'type': 'private', 'display_recipient':
frozenset({11, 12, 1001}), 'flags': []},
12: {'type': 'private', 'display_recipient':
frozenset({11, 12, 1001}), 'flags': []},
13: {'type': 'private', 'display_recipient':
frozenset({11, 12, 1001}), 'flags': []},
101: {'type': 'private', 'display_recipient':
frozenset({11, 12, 13, 1001}), 'flags': []},
102: {'type': 'private', 'display_recipient':
frozenset({11, 12, 13, 1001}), 'flags': []},
})

# --------------- UI Fixtures -----------------------------------------

Expand Down
5 changes: 3 additions & 2 deletions tests/helper/test_helper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,9 @@ def test_classify_unread_counts(mocker, initial_data, stream_dict,
model.initial_data = initial_data
model.muted_topics = muted_topics
model.muted_streams = muted_streams
assert classify_unread_counts(model) == dict(classified_unread_counts,
**vary_in_unreads)
assert classify_unread_counts(model) == (dict(classified_unread_counts[0],
**vary_in_unreads),
classified_unread_counts[1])


@pytest.mark.parametrize('color', [
Expand Down
63 changes: 45 additions & 18 deletions tests/model/test_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def model(self, mocker, initial_data, user_profile):
# NOTE: PATCH WHERE USED NOT WHERE DEFINED
self.classify_unread_counts = mocker.patch(
'zulipterminal.model.classify_unread_counts',
return_value=[])
return_value=([], {}))
self.client.get_profile.return_value = user_profile
model = Model(self.controller)
return model
Expand Down Expand Up @@ -81,7 +81,7 @@ def test_init_InvalidAPIKey_response(self, mocker, initial_data):
return_value=({}, set(), [], []))
self.classify_unread_counts = mocker.patch(
'zulipterminal.model.classify_unread_counts',
return_value=[])
return_value=([], {}))

with pytest.raises(ServerConnectionFailure) as e:
model = Model(self.controller)
Expand All @@ -103,7 +103,7 @@ def test_init_ZulipError_exception(self, mocker, initial_data,
return_value=({}, set(), [], []))
self.classify_unread_counts = mocker.patch(
'zulipterminal.model.classify_unread_counts',
return_value=[])
return_value=([], {}))

with pytest.raises(ServerConnectionFailure) as e:
model = Model(self.controller)
Expand Down Expand Up @@ -440,7 +440,7 @@ def test_success_get_messages(self, mocker, messages_successful_response,
return_value=({}, set(), [], []))
self.classify_unread_counts = mocker.patch(
'zulipterminal.model.classify_unread_counts',
return_value=[])
return_value=([], {}))

# Setup mocks before calling get_messages
self.client.get_messages.return_value = messages_successful_response
Expand Down Expand Up @@ -480,7 +480,7 @@ def test_get_message_false_first_anchor(
return_value=({}, set(), [], []))
self.classify_unread_counts = mocker.patch(
'zulipterminal.model.classify_unread_counts',
return_value=[])
return_value=([], {}))

# Setup mocks before calling get_messages
messages_successful_response['anchor'] = 0
Expand Down Expand Up @@ -513,7 +513,7 @@ def test_fail_get_messages(self, mocker, error_response,
return_value=({}, set(), [], []))
self.classify_unread_counts = mocker.patch(
'zulipterminal.model.classify_unread_counts',
return_value=[])
return_value=([], {}))

# Setup mock before calling get_messages
# FIXME This has no influence on the result
Expand Down Expand Up @@ -593,7 +593,7 @@ def test__update_initial_data_raises_exception(self, mocker, initial_data):
return_value=({}, set(), [], []))
self.classify_unread_counts = mocker.patch(
'zulipterminal.model.classify_unread_counts',
return_value=[])
return_value=([], {}))

# Setup mocks before calling get_messages
self.client.register.return_value = initial_data
Expand Down Expand Up @@ -627,7 +627,7 @@ def test_get_all_users(self, mocker, initial_data, user_list, user_dict,
return_value=({}, set(), [], []))
self.classify_unread_counts = mocker.patch(
'zulipterminal.model.classify_unread_counts',
return_value=[])
return_value=([], {}))
model = Model(self.controller)
assert model.user_dict == user_dict
assert model.users == user_list
Expand All @@ -650,7 +650,7 @@ def test__handle_message_event_with_Falsey_log(self, mocker,
model.found_newest = True
mocker.patch('zulipterminal.model.Model._update_topic_index')
index_msg = mocker.patch('zulipterminal.model.index_messages',
return_value={})
return_value=initial_index)
model.msg_list = mocker.Mock(log=[])
create_msg_box_list = mocker.patch('zulipterminal.model.'
'create_msg_box_list',
Expand All @@ -671,7 +671,7 @@ def test__handle_message_event_with_valid_log(self, mocker,
model.found_newest = True
mocker.patch('zulipterminal.model.Model._update_topic_index')
index_msg = mocker.patch('zulipterminal.model.index_messages',
return_value={})
return_value=initial_index)
model.msg_list = mocker.Mock(log=[mocker.Mock()])
create_msg_box_list = mocker.patch('zulipterminal.model.'
'create_msg_box_list',
Expand All @@ -689,12 +689,37 @@ def test__handle_message_event_with_valid_log(self, mocker,
assert_called_once_with(model, [message_fixture['id']],
last_message=expected_last_msg))

@pytest.mark.parameter('unread_msgs', [
({'type': 'stream', 'stream_id': 5140,
'subject': 'Test', 'display_recipient': 'PTEST'}),
({'type': 'private', 'sender_id': '5140'}),
({'type': 'private', 'display_recipient':
[{
'id': 5179,
'is_mirror_dummy': False,
'full_name': 'Boo Boo',
'short_name': 'boo',
'email': '[email protected]',
}, {
'short_name': 'foo',
'id': 5140,
'is_mirror_dummy': False,
'full_name': 'Foo Foo',
'email': '[email protected]',
}, {
'short_name': 'bar',
'id': 5180,
'is_mirror_dummy': False,
'full_name': 'Bar Bar',
'email': '[email protected]',
}]}),
])
def test__handle_message_event_with_flags(self, mocker,
model, message_fixture):
model.found_newest = True
mocker.patch('zulipterminal.model.Model._update_topic_index')
index_msg = mocker.patch('zulipterminal.model.index_messages',
return_value={})
return_value=initial_index)
model.msg_list = mocker.Mock()
create_msg_box_list = mocker.patch('zulipterminal.model.'
'create_msg_box_list',
Expand All @@ -720,8 +745,9 @@ def test__handle_message_event_with_flags(self, mocker,

@pytest.mark.parametrize('response, narrow, recipients, log', [
({'type': 'stream', 'stream_id': 1, 'subject': 'FOO',
'id': 1}, [], frozenset(), ['msg_w']),
({'type': 'private', 'id': 1},
'id': 1, 'display_recipient': 'a'}, [], frozenset(), ['msg_w']),
({'type': 'private', 'id': 1, 'sender_id': 1,
'display_recipient': []},
[['is', 'private']], frozenset(), ['msg_w']),
({'type': 'stream', 'id': 1, 'stream_id': 1, 'subject': 'FOO',
'display_recipient': 'a'},
Expand All @@ -734,14 +760,15 @@ def test__handle_message_event_with_flags(self, mocker,
'display_recipient': 'a'},
[['stream', 'c'], ['topic', 'b']],
frozenset(), []),
({'type': 'private', 'id': 1,
({'type': 'private', 'id': 1, 'sender_id': 5827,
'display_recipient': [{'id': 5827}, {'id': 5}]},
[['pm_with', '[email protected]']],
frozenset({5827, 5}), ['msg_w']),
({'type': 'private', 'id': 1},
({'type': 'private', 'id': 1, 'sender_id': 5827,
'display_recipient': []},
[['is', 'search']],
frozenset(), []),
({'type': 'private', 'id': 1,
({'type': 'private', 'id': 1, 'sender_id': 5827,
'display_recipient': [{'id': 5827}, {'id': 3212}]},
[['pm_with', '[email protected]']],
frozenset({5827, 5}), []),
Expand All @@ -759,7 +786,7 @@ def test__handle_message_event(self, mocker, user_profile, response,
model.found_newest = True
mocker.patch('zulipterminal.model.Model._update_topic_index')
index_msg = mocker.patch('zulipterminal.model.index_messages',
return_value={})
return_value=initial_index)
create_msg_box_list = mocker.patch('zulipterminal.model.'
'create_msg_box_list',
return_value=["msg_w"])
Expand Down Expand Up @@ -1297,7 +1324,7 @@ def test_update_read_status(self, mocker, model, event_op,
== flags_before)

if event_op == 'add':
set_count.assert_called_once_with(list(changed_ids),
set_count.assert_called_once_with(list(event_message_ids),
self.controller, -1)
elif event_op == 'remove':
set_count.assert_not_called()
Expand Down
48 changes: 37 additions & 11 deletions zulipterminal/helper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
'search': Set[int], # {message_id, ...}
# Downloaded message data
'messages': Dict[int, Message], # message_id: Message
'unread_msgs': Dict[int, Dict[str, Any]], # message_id: Dict
})

initial_index = Index(
Expand All @@ -54,6 +55,7 @@
topics=defaultdict(list),
search=set(),
messages=defaultdict(dict),
unread_msgs=defaultdict(dict),
)


Expand Down Expand Up @@ -96,9 +98,8 @@ def _set_count_in_model(new_count: int, changed_messages: List[Message],
if message['type'] == 'stream':
key = (message['stream_id'], message['subject'])
unreads = unread_counts['unread_topics']
# self-pm has only one display_recipient
# 1-1 pms have 2 display_recipient
elif len(message['display_recipient']) <= 2:
# 1-1 pms and self-pms
elif message['type'] == 'private' and 'sender_id' in message:
key = message['sender_id']
unreads = unread_counts['unread_pms'] # type: ignore
else: # If it's a group pm
Expand Down Expand Up @@ -136,13 +137,16 @@ def _set_count_in_view(controller: Any, new_count: int,
all_pm = controller.view.pm_button
all_mentioned = controller.view.mentioned_button
for message in changed_messages:
user_id = message['sender_id']

# If we sent this message, don't increase the count
if user_id == controller.model.user_id:
continue

msg_type = message['type']
# FIXME no user_id for streams?
if msg_type != 'stream':
user_id = message['sender_id']

# If we sent this message, don't increase the count
if user_id == controller.model.user_id:
continue
Copy link
Member Author

@kaustubh-nair kaustubh-nair Apr 2, 2020

Choose a reason for hiding this comment

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

This is kind of hacky - we don't have sender_id for stream messages in initial_data['unread_msgs']
Though on close inspection this piece of code doesn't seem to be doing anything?
Refer to conversation on czo https://chat.zulip.org/#narrow/stream/206-zulip-terminal/topic/Handling.20read.20events/near/845434


add_to_counts = True
if 'mentioned' in message['flags']:
unread_counts['all_mentions'] += new_count
Expand Down Expand Up @@ -188,7 +192,7 @@ def set_count(id_list: List[int], controller: Any, new_count: int) -> None:
# This method applies new_count for 'new message' (1) or 'read' (-1)
# (we could ensure this in a different way by a different type)
assert new_count == 1 or new_count == -1
messages = controller.model.index['messages']
messages = controller.model.index['unread_msgs']
unread_counts = controller.model.unread_counts # type: UnreadCounts
changed_messages = [messages[id] for id in id_list]
_set_count_in_model(new_count, changed_messages, unread_counts)
Expand Down Expand Up @@ -386,9 +390,11 @@ def index_messages(messages: List[Message],
return index


def classify_unread_counts(model: Any) -> UnreadCounts:
def classify_unread_counts(model: Any) -> Tuple[UnreadCounts,
Dict[int, Dict[str, Any]]]:
# TODO: support group pms
unread_msg_counts = model.initial_data['unread_msgs']
unread_msgs = {} # type: Dict[int, Dict[str, Any]]

unread_counts = UnreadCounts(
all_msg=0,
Expand All @@ -405,13 +411,25 @@ def classify_unread_counts(model: Any) -> UnreadCounts:

for pm in unread_msg_counts['pms']:
count = len(pm['unread_message_ids'])
pm_data = {'type': 'private', 'sender_id': pm['sender_id'],
'flags': []}
unread_msgs.update(dict(zip(pm['unread_message_ids'],
[pm_data] * count)))
unread_counts['unread_pms'][pm['sender_id']] = count
unread_counts['all_msg'] += count
unread_counts['all_pms'] += count

for stream in unread_msg_counts['streams']:
count = len(stream['unread_message_ids'])
stream_id = stream['stream_id']
stream_name = model.stream_dict[stream_id]['name']
sender_ids = stream['sender_ids']
sender_ids = frozenset(sender_ids)
stream_data = {'type': 'stream', 'stream_id': stream_id, 'subject':
stream['topic'], 'display_recipient': stream_name,
'sender_ids': sender_ids, 'flags': []}
unread_msgs.update(dict(zip(stream['unread_message_ids'],
[stream_data] * count)))
if [model.stream_dict[stream_id]['name'],
stream['topic']] in model.muted_topics:
continue
Expand All @@ -428,11 +446,19 @@ def classify_unread_counts(model: Any) -> UnreadCounts:
count = len(group_pm['unread_message_ids'])
user_ids = group_pm['user_ids_string'].split(',')
user_ids = frozenset(map(int, user_ids))
huddle_data = {'type': 'private', 'display_recipient': user_ids,
'flags': []}
unread_msgs.update(dict(zip(group_pm['unread_message_ids'],
[huddle_data] * count)))
unread_counts['unread_huddles'][user_ids] = count
unread_counts['all_msg'] += count
unread_counts['all_pms'] += count

return unread_counts
for mentioned_message_id in unread_msg_counts['mentions']:
if(unread_msgs.get(mentioned_message_id)):
unread_msgs[mentioned_message_id].update({'flags': ['mentioned']})

return unread_counts, unread_msgs


def match_user(user: Any, text: str) -> bool:
Expand Down
Loading