Skip to content

785 with refactors and extra bugfix #832

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 4 commits into from
Nov 22, 2020
Merged

785 with refactors and extra bugfix #832

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
92bf6ab
bugfix: model: Update topic list on update message event.
kaustubh-nair Aug 22, 2020
9978495
tests: model: Improve _handle_update_message_event test.
neiljp Nov 6, 2020
9792e00
refactor: model: Hoist marking of messages as edited in update handler.
neiljp Nov 21, 2020
0b56f30
bugfix: model: Update topic list independently of message index status.
neiljp Oct 18, 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
144 changes: 123 additions & 21 deletions tests/model/test_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -967,135 +967,237 @@ def test_notify_users_enabled(self, mocker, model, message_fixture,
model.notify_user(message_fixture)
assert notify.called == is_notify_called

@pytest.mark.parametrize('response, update_call_count, new_index', [
@pytest.mark.parametrize(['event',
'expected_times_messages_rerendered',
'expected_index',
'topic_view_enabled'], [
({ # Only subject of 1 message is updated.
'message_id': 1,
'subject': 'new subject',
'stream_id': 10,
'message_ids': [1],
}, 1, {
'messages': {
1: {
'id': 1,
'stream_id': 10,
'content': 'old content',
'subject': 'new subject'
},
2: {
'id': 2,
'stream_id': 10,
'content': 'old content',
'subject': 'old subject'
}},
'edited_messages': {1}
}),
'edited_messages': {1},
'topics': {10: []},
}, False),
({ # Subject of 2 messages is updated
'message_id': 1,
'subject': 'new subject',
'stream_id': 10,
'message_ids': [1, 2],
}, 2, {
'messages': {
1: {
'id': 1,
'stream_id': 10,
'content': 'old content',
'subject': 'new subject'
},
2: {
'id': 2,
'stream_id': 10,
'content': 'old content',
'subject': 'new subject'
}},
'edited_messages': {1}
}),
'edited_messages': {1},
'topics': {10: []},
}, False),
({ # Message content is updated
'message_id': 1,
'stream_id': 10,
'rendered_content': '<p>new content</p>',
}, 1, {
'messages': {
1: {
'id': 1,
'stream_id': 10,
'content': '<p>new content</p>',
'subject': 'old subject'
},
2: {
'id': 2,
'stream_id': 10,
'content': 'old content',
'subject': 'old subject'
}},
'edited_messages': {1}
}),
'edited_messages': {1},
'topics': {10: ['old subject']},
}, False),
({ # Both message content and subject is updated.
'message_id': 1,
'rendered_content': '<p>new content</p>',
'subject': 'new subject',
'stream_id': 10,
'message_ids': [1],
}, 2, {
}, 2, { # 2=update of subject & content
'messages': {
1: {
'id': 1,
'stream_id': 10,
'content': '<p>new content</p>',
'subject': 'new subject'
},
2: {
'id': 2,
'stream_id': 10,
'content': 'old content',
'subject': 'old subject'
}},
'edited_messages': {1}
}),
'edited_messages': {1},
'topics': {10: []},
}, False),
({ # Some new type of update which we don't handle yet.
'message_id': 1,
'foo': 'boo',
}, 0, {
'messages': {
1: {
'id': 1,
'stream_id': 10,
'content': 'old content',
'subject': 'old subject'
},
2: {
'id': 2,
'stream_id': 10,
'content': 'old content',
'subject': 'old subject'
}},
'edited_messages': {1}
}),
({ # message_id not present in index.
'edited_messages': {1},
'topics': {10: ['old subject']},
}, False),
({ # message_id not present in index, topic view closed.
'message_id': 3,
'rendered_content': '<p>new content</p>',
'subject': 'new subject',
'stream_id': 10,
'message_ids': [3],
}, 0, {
'messages': {
1: {
'id': 1,
'stream_id': 10,
'content': 'old content',
'subject': 'old subject'
},
2: {
'id': 2,
'stream_id': 10,
'content': 'old content',
'subject': 'old subject'
}},
'edited_messages': set(),
'topics': {10: []}, # This resets the cache
}, False),
({ # message_id not present in index, topic view is enabled.
'message_id': 3,
'rendered_content': '<p>new content</p>',
'subject': 'new subject',
'stream_id': 10,
'message_ids': [3],
}, 0, {
'messages': {
1: {
'id': 1,
'stream_id': 10,
'content': 'old content',
'subject': 'old subject'
},
2: {
'id': 2,
'stream_id': 10,
'content': 'old content',
'subject': 'old subject'
}},
'edited_messages': set(),
'topics': {10: ['new subject', 'old subject']},
}, True),
({ # Message content is updated and topic view is enabled.
'message_id': 1,
'rendered_content': '<p>new content</p>',
'subject': 'new subject',
'stream_id': 10,
'message_ids': [1],
}, 2, {
'messages': {
1: {
'id': 1,
'stream_id': 10,
'content': '<p>new content</p>',
'subject': 'new subject'
},
2: {
'id': 2,
'stream_id': 10,
'content': 'old content',
'subject': 'old subject'
}},
'edited_messages': set()
}),
'edited_messages': {1},
'topics': {10: ['new subject', 'old subject']},
}, True),
], ids=[
"Only subject of 1 message is updated",
"Subject of 2 messages is updated",
"Message content is updated",
"Both message content and subject is updated",
"Some new type of update which we don't handle yet",
"message_id not present in index, topic view closed",
"message_id not present in index, topic view is enabled",
"Message content is updated and topic view is enabled",
])
def test__handle_update_message_event(self, mocker, model,
response, new_index,
update_call_count):
event, expected_index,
expected_times_messages_rerendered,
topic_view_enabled):
model.index = {
'messages': {
message_id: {
'id': message_id,
'stream_id': 10,
'content': 'old content',
'subject': 'old subject',
} for message_id in [1, 2]
},
'edited_messages': set()
'edited_messages': set(),
'topics': {10: ['old subject']},
}
mocker.patch('zulipterminal.model.Model._update_rendered_view')

model._handle_update_message_event(response)
def _set_topics_to_old_and_new(event):
model.index['topics'][10] = ['new subject', 'old subject']
fetch_topics = mocker.patch(
'zulipterminal.model.Model._fetch_topics_in_streams',
side_effect=_set_topics_to_old_and_new)

(model.controller.view.left_panel.is_in_topic_view_with_stream_id.
return_value) = topic_view_enabled

model._handle_update_message_event(event)

assert model.index == expected_index

calls_to_update_messages = model._update_rendered_view.call_count
assert calls_to_update_messages == expected_times_messages_rerendered

assert model.index == new_index
assert model._update_rendered_view.call_count == update_call_count
if topic_view_enabled:
fetch_topics.assert_called_once_with([event['stream_id']])
stream_button = model.controller.view.topic_w.stream_button
(model.controller.view.left_panel.show_topic_view.
assert_called_once_with(stream_button))
model.controller.update_screen.assert_called_once_with()

@pytest.mark.parametrize('subject, narrow, new_log_len', [
('foo', [['stream', 'boo'], ['topic', 'foo']], 2),
Expand Down
52 changes: 38 additions & 14 deletions zulipterminal/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -997,25 +997,49 @@ def _handle_update_message_event(self, event: Event) -> None:
"""
Handle updated (edited) messages (changed content/subject)
"""
# Update edited message status from single message id
# NOTE: If all messages in topic have topic edited,
# they are not all marked as edited, as per server optimization
message_id = event['message_id']
# If the message is indexed
if self.index['messages'].get(message_id):
message = self.index['messages'][message_id]
indexed_message = self.index['messages'].get(message_id, None)

if indexed_message:
self.index['edited_messages'].add(message_id)

if 'rendered_content' in event:
message['content'] = event['rendered_content']
self.index['messages'][message_id] = message
self._update_rendered_view(message_id)

# 'subject' is not present in update event if
# the event didn't have a 'subject' update.
if 'subject' in event:
new_subject = event['subject']
for msg_id in event['message_ids']:
self.index['messages'][msg_id]['subject'] = new_subject
# Update the rendered content, if the message is indexed
if 'rendered_content' in event and indexed_message:
indexed_message['content'] = event['rendered_content']
self.index['messages'][message_id] = indexed_message
self._update_rendered_view(message_id)

# NOTE: This is independent of messages being indexed
# Previous assertion:
# * 'subject' is not present in update event if
# the event didn't have a 'subject' update.
if 'subject' in event:
new_subject = event['subject']
stream_id = event['stream_id']

# Update any indexed messages & re-render them
for msg_id in event['message_ids']:
indexed_msg = self.index['messages'].get(msg_id)
if indexed_msg:
indexed_msg['subject'] = new_subject
self._update_rendered_view(msg_id)

# If topic view is open, reload list else reset cache.
if stream_id in self.index['topics']:
if hasattr(self.controller, 'view'):
view = self.controller.view
if (view.left_panel.is_in_topic_view_with_stream_id(
stream_id)):
self._fetch_topics_in_streams([stream_id])
view.left_panel.show_topic_view(
view.topic_w.stream_button)
self.controller.update_screen()
else:
self.index['topics'][stream_id] = []

def _handle_reaction_event(self, event: Event) -> None:
"""
Handle change to reactions on a message
Expand Down