Skip to content
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

Preparation for deprecating reactions' user metadata #1556

Merged
merged 9 commits into from
Dec 10, 2024
Merged

Preparation for deprecating reactions' user metadata #1556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4765327
refactor: model/views: Simplify functions handling message reactions.
Niloth-p Nov 17, 2024
d9cd5e5
api_types/model: Add support for `user_id` in reaction events.
Niloth-p Nov 17, 2024
e037dc4
test_model: Align reactions schema with Get Message API endpoint.
Niloth-p Nov 17, 2024
f7e56c9
bugfix: model: Remove the reaction corresponding to the reaction event.
Niloth-p Nov 17, 2024
911f8ee
model: Convert reaction events to message reactions schema.
Niloth-p Nov 17, 2024
35d5bee
model: Add get_user_id_from_reaction for messages and reaction events.
Niloth-p Nov 18, 2024
6e2f98c
test_model: Add user_id test cases for reaction factories.
Niloth-p Nov 17, 2024
198e8f3
test_model: Add varied test cases for testing reaction event handling.
Niloth-p Nov 18, 2024
005dabb
messages/views: Prepare for the deprecation of user metadata.
Niloth-p Nov 18, 2024
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
299 changes: 260 additions & 39 deletions tests/model/test_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,6 +790,75 @@ def test_has_user_reacted_to_message(

assert has_reacted == expected_has_user_reacted

@pytest.mark.parametrize(
"user_data, expected_user_id",
[
case({"user_id": 456}, 456, id="reaction_event_user_id"),
case(
{"user": {"user_id": 123}},
123,
id="reaction_event_user",
),
case(
{"user_id": 101},
101,
id="reaction_user_id",
),
case(
{"user": {"id": 202}},
202,
id="reaction_user",
),
],
)
def test_get_user_id_from_reaction_success(
self,
model: Model,
user_data: Dict[str, Any],
expected_user_id: int,
):
reaction = {
"type": "reaction",
"op": "add",
"reaction_type": "unicode_emoji",
"emoji_code": "1f44d",
"emoji_name": "thumbs_up",
"message_id": "24757",
**user_data,
}
assert model.get_user_id_from_reaction(reaction) == expected_user_id

@pytest.mark.parametrize(
"user_data",
[
case({}, id="reaction_event_missing_user_id_user"),
case(
{"user": {"id": "not_an_int"}},
id="invalid_user",
),
case(
{"user_id": "not_an_int"},
id="invalid_user_id",
),
],
)
def test_get_user_id_from_reaction_failure(
self,
model: Model,
user_data: Dict[str, Any],
):
reaction = {
"type": "reaction",
"op": "add",
"reaction_type": "unicode_emoji",
"emoji_code": "1f44d",
"emoji_name": "thumbs_up",
"message_id": "24757",
**user_data,
}
with pytest.raises((AssertionError, AttributeError)):
model.get_user_id_from_reaction(reaction)

@pytest.mark.parametrize("recipient_user_ids", [[5140], [5140, 5179]])
@pytest.mark.parametrize("status", ["start", "stop"])
def test_send_typing_status_by_user_ids(
Expand Down Expand Up @@ -2793,21 +2862,25 @@ def test__update_rendered_view_change_narrow(

@pytest.fixture
def reaction_event_factory(self):
def _factory(*, op: str, message_id: int):
return {
"emoji_code": "1f44d",
def _factory(*, op: str, message_id: int, user=None, user_id=None):
base_event = {
"id": 2,
"user": {
"email": "[email protected]",
"user_id": 5140,
"full_name": "Foo Boo",
},
"emoji_code": "1f44d",
"reaction_type": "unicode_emoji",
"message_id": message_id,
"emoji_name": "thumbs_up",
"type": "reaction",
"op": op,
}
if user is not None:
base_event["user"] = {
"email": "[email protected]",
"user_id": 5140,
"full_name": "Foo Boo",
}
if user_id is not None:
base_event["user_id"] = user_id
return base_event

return _factory

Expand All @@ -2816,28 +2889,45 @@ def reaction_event_index_factory(self):
"""
Generate index for reaction tests based on minimal specification

Input is a list of pairs, of a message-id and a list of reaction tuples
Input:
- msgs: A list of tuples where each tuple contains:
- A message ID
- A list of reaction tuples, where each reaction includes:
- user_id
- reaction_type
- emoji_code
- emoji_name
- schema: Defines the fields present in the reaction
(e.g., "with_user", "with_user_id", "with_both")

NOTE: reactions as None indicate not indexed, [] indicates no reaction
"""
MsgsType = List[Tuple[int, Optional[List[Tuple[int, str, str, str]]]]]

def _factory(msgs: MsgsType):
def _factory(msgs: MsgsType, schema="with_user"):
def build_reaction(user_id, type, code, name):
reaction = {
"reaction_type": type,
"emoji_code": code,
"emoji_name": name,
}
if schema in {"with_user", "with_both"}:
reaction["user"] = {
"email": f"User email #{user_id}",
"full_name": f"User #{user_id}",
"id": user_id,
}
if schema in {"with_user_id", "with_both"}:
reaction["user_id"] = user_id
return reaction

return {
"messages": {
message_id: {
"id": message_id,
"content": f"message content {message_id}",
"reactions": [
{
"user": {
"email": f"User email #{user_id}",
"full_name": f"User #{user_id}",
"user_id": user_id,
},
"reaction_type": type,
"emoji_code": code,
"emoji_name": name,
}
build_reaction(user_id, type, code, name)
for user_id, type, code, name in reactions
],
}
Expand All @@ -2849,25 +2939,43 @@ def _factory(msgs: MsgsType):
return _factory

@pytest.mark.parametrize("op", ["add", "remove"])
@pytest.mark.parametrize(
"reaction_event_schema", ["with_user", "with_user_id", "with_both"]
)
@pytest.mark.parametrize(
"reaction_schema", ["with_user", "with_user_id", "with_both"]
)
def test__handle_reaction_event_not_in_index(
self,
mocker,
model,
reaction_event_factory,
reaction_event_index_factory,
op,
reaction_event_schema,
reaction_schema,
unindexed_message_id=1,
):
reaction_event = reaction_event_factory(
op=op,
message_id=unindexed_message_id,
)
common_args = {
"op": op,
"message_id": unindexed_message_id,
}
if reaction_event_schema == "with_user":
reaction_event = reaction_event_factory(**common_args, user=True)
elif reaction_event_schema == "with_user_id":
reaction_event = reaction_event_factory(**common_args, user_id=5140)
else:
reaction_event = reaction_event_factory(
**common_args, user=True, user_id=5140
)

model.index = reaction_event_index_factory(
[
(unindexed_message_id, None), # explicitly exclude
(2, [(1, "unicode_emoji", "1232", "thumbs_up")]),
(3, []),
]
],
reaction_schema,
)
model._update_rendered_view = mocker.Mock()
previous_index = deepcopy(model.index)
Expand All @@ -2879,10 +2987,116 @@ def test__handle_reaction_event_not_in_index(
assert not model._update_rendered_view.called

@pytest.mark.parametrize(
"op, expected_number_after",
"reaction_event_schema", ["with_user", "with_user_id", "with_both"]
)
@pytest.mark.parametrize(
"reaction_schema", ["with_user", "with_user_id", "with_both"]
)
@pytest.mark.parametrize(
"msgs, op, expected_number_after",
[
("add", 2),
("remove", 1), # Removed emoji doesn't match, so length remains 1
case(
[
(2, [(5140, "unicode_emoji", "2764", "heart")]),
(1, []),
],
"add",
2,
id="single_reaction_add",
),
case(
[
(2, [(5140, "unicode_emoji", "1f44d", "thumbs_up")]),
(1, []),
],
"remove",
0,
id="single_reaction_remove",
),
case(
[
(
2,
[
(3478, "unicode_emoji", "1f44d", "thumbs_up"),
(3479, "unicode_emoji", "1f44d", "thumbs_up"),
],
),
],
"add",
3,
id="same_emoji_different_users_add",
),
case(
[
(
2,
[
(3478, "unicode_emoji", "1f44d", "thumbs_up"),
(5140, "unicode_emoji", "1f44d", "thumbs_up"),
],
),
],
"remove",
1,
id="same_emoji_different_users_remove",
),
case(
[
(
2,
[
(5140, "zulip_extra_emoji", "zulip", "zulip"),
(3478, "unicode_emoji", "2764", "heart"),
],
),
],
"add",
3,
id="different_emoji_different_users_add",
),
case(
[
(
2,
[
(5140, "unicode_emoji", "1f44d", "thumbs_up"),
(3478, "unicode_emoji", "2764", "heart"),
],
),
],
"remove",
1,
id="different_emoji_different_users_remove",
),
case(
[
(
2,
[
(5140, "zulip_extra_emoji", "zulip", "zulip"),
(5140, "unicode_emoji", "2764", "heart"),
],
),
],
"add",
3,
id="different_emoji_same_user_add",
),
case(
[
(
2,
[
(5140, "unicode_emoji", "1f44d", "thumbs_up"),
(5140, "unicode_emoji", "5678", "heart"),
],
),
],
"remove",
1,
id="different_emoji_same_user_remove",
),
],
)
def test__handle_reaction_event_for_msg_in_index(
Expand All @@ -2892,19 +3106,26 @@ def test__handle_reaction_event_for_msg_in_index(
reaction_event_factory,
reaction_event_index_factory,
op,
reaction_event_schema,
reaction_schema,
msgs,
expected_number_after,
event_message_id=1,
event_message_id=2,
):
reaction_event = reaction_event_factory(
op=op,
message_id=event_message_id,
)
model.index = reaction_event_index_factory(
[
(1, [(1, "unicode_emoji", "1232", "thumbs_up")]),
(2, []),
]
)
common_args = {
"op": op,
"message_id": event_message_id,
}
if reaction_event_schema == "with_user":
reaction_event = reaction_event_factory(**common_args, user=True)
elif reaction_event_schema == "with_user_id":
reaction_event = reaction_event_factory(**common_args, user_id=5140)
else:
reaction_event = reaction_event_factory(
**common_args, user=True, user_id=5140
)

model.index = reaction_event_index_factory(msgs, reaction_schema)
model._update_rendered_view = mocker.Mock()

model._handle_reaction_event(reaction_event)
Expand Down
Loading
Loading