Skip to content

Add handling for User update events #1249

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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
43 changes: 43 additions & 0 deletions tests/model/test_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,7 @@ def test_register_initial_desired_events(self, mocker, initial_data):
"update_display_settings",
"user_settings",
"realm_emoji",
"realm_user",
]
fetch_event_types = [
"realm",
Expand Down Expand Up @@ -3346,6 +3347,48 @@ def test__handle_subscription_event_subscribers_one_user_multiple_streams(
new_subscribers = model.stream_dict[stream_id]["subscribers"]
assert new_subscribers == expected_subscribers

@pytest.mark.parametrize(
"person, changed_details",
[
(
{"full_name": "New Full Name"},
"full_name",
),
({"timezone": "New Timezone"}, "timezone"),
({"is_billing_admin": False}, "is_billing_admin"),
({"role": 10}, "role"),
(
{"avatar_url": "new_avatar_url", "avatar_version": 21},
"avatar_url",
),
({"new_email": "new_display@email.com"}, "email"),
(
{"delivery_email": "new_delivery@email.com"},
"delivery_email",
),
],
ids=[
Copy link
Collaborator

Choose a reason for hiding this comment

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

There a lot of test cases & ids, so it's simpler to keep them inline - then later we wouldn't have to check where in each list to add/remove/edit them, or to move them around.

"full_name",
"timezone",
"billing_admin_role",
"role",
"avatar",
"display_email",
"delivery_email",
],
)
def test__handle_realm_user_event(
self, person, changed_details, model, initial_data
):
# For testing purposes, initial_data["realm_users"][1] from the initial_data fixture is used here.
person["user_id"] = 11
event = {"type": "realm_user", "op": "update", "id": 1000, "person": person}
model._handle_realm_user_event(event)
assert (
initial_data["realm_users"][1][changed_details]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm guessing the 1 is the index into initial_data that corresponds to user_id=11?

If that's the case, it would be more reliable to make that explicit in the test code, eg. if the initial data gets reordered at a later stage, this would make this test fail even though it's functionally correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I'll add a comment for this 👍

== event["person"][changed_details]
)

@pytest.mark.parametrize("value", [True, False])
def test__handle_user_settings_event(self, mocker, model, value):
setting = "send_private_typing_notifications"
Expand Down
32 changes: 32 additions & 0 deletions zulipterminal/api_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,37 @@ class ReactionEvent(TypedDict):
message_id: int


class RealmUserEventPerson(TypedDict, total=False):
user_id: int

full_name: str

avatar_url: str
avatar_source: str
avatar_url_medium: str
avatar_version: int

# NOTE: This field will be removed in future as it is redundant with the user_id
# email: str
timezone: str

role: int

email: str
Copy link
Collaborator

Choose a reason for hiding this comment

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

In comparing the fields that can be updated, this one stands out as different from the remaining one field possible (custom_profile_field), unless I'm missing another use of email?

Copy link
Collaborator Author

@mounilKshah mounilKshah Sep 24, 2022

Choose a reason for hiding this comment

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

The email field does not exist under RealmUserEventPerson, rather we get new_email from the API. However, in model.initial_data["realm_users"], we use email and not new_email. Hence, in the updated code (under Model._handle_realm_user_events(), where we now have a conditional for replacing the new_email key with the email key, mypy returns an error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see your point. Can we avoid that by moving the conditional into the loop where we've found the user-id?

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, let me just read the code again, I may just merge these first two commits roughly as they are.


new_email: str

delivery_email: str

is_billing_admin: bool


class RealmUserEvent(TypedDict):
type: Literal["realm_user"]
op: Literal["update"]
person: RealmUserEventPerson


class SubscriptionEvent(TypedDict):
type: Literal["subscription"]
op: str
Expand Down Expand Up @@ -249,4 +280,5 @@ class UpdateGlobalNotificationsEvent(TypedDict):
UpdateRealmEmojiEvent,
UpdateUserSettingsEvent,
UpdateGlobalNotificationsEvent,
RealmUserEvent,
]
22 changes: 22 additions & 0 deletions zulipterminal/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ def __init__(self, controller: Any) -> None:
("update_display_settings", self._handle_update_display_settings_event),
("user_settings", self._handle_user_settings_event),
("realm_emoji", self._handle_update_emoji_event),
("realm_user", self._handle_realm_user_event),
]
)

Expand Down Expand Up @@ -1830,6 +1831,27 @@ def _handle_update_display_settings_event(self, event: Event) -> None:
view.message_view.log[msg_pos] = msg_w_list[0]
self.controller.update_screen()

def _handle_realm_user_event(self, event: Event) -> None:
"""
Handle change to user(s) metadata (Eg: full_name, timezone, etc.)
"""
assert event["type"] == "realm_user"
if event["op"] == "update":
# realm_users has 'email' attribute and not 'new_email'
if "new_email" in event["person"]:
event["person"]["email"] = event["person"].pop("new_email")
updated_details = event["person"]
# Role is not present under self.initial_data,
# but exists only under self.initial_data["realm_users"]
if "role" not in event["person"]:
# check if the event contains details of current user or some other user in the org
if updated_details["user_id"] == self.user_id:
self.initial_data.update(updated_details)
for realm_user in self.initial_data["realm_users"]:
if realm_user["user_id"] == updated_details["user_id"]:
realm_user.update(updated_details)
break

def _register_desired_events(self, *, fetch_data: bool = False) -> str:
fetch_types = None if not fetch_data else self.initial_data_to_fetch
event_types = list(self.event_actions)
Expand Down