Add handling for User update events #1249

mounilKshah · 2022-08-21T11:50:14Z

What does this PR do?
The PR is for handling any changes made with respect to the users' meta data ie, meta data of the current user and that of other users.

Fixes #988

Tested?

Manually
Existing tests (adapted, if necessary)
New tests added (for any new behavior)
Passed linting & tests (each commit)

Commit flow

Only one commit currently: Updates data structures in the codebase as per the events received.

Notes & Questions

Yet to imbibe UI changes for the user events.

neiljp

@mounilKshah Thanks for looking at this - this looks functional, at least in some ways 👍

While not strictly necessary, it'd be clearer to have the api_type changes in a separate commit since it documents the API first (you could even link to it in the commit text). I would also consider separate commits for first updating realm_users and then the active user for reasons below.

As per my inline comments I think you can simplify the update logic you already have, which may mean the additional function is unnecessary. It is called in every branch of the conditional, and the function itself seems like it could become 3 lines of code.

Currently your update of realm_users benefits from the regular calling of get_all_users and subsequently the user list update, and the former propagating that to other user datastructures that are either loosely (eg. by get_user_info()) or more tightly (by eg. user_dict) coupled. However, the logged-in user info updates are not currently updated from the initial_data by other code, so you'll need to add this manually, which I suspect won't be as simple as a dict update.

So, if you split this into 3 commits and address the inline comments, and add/adjust a test for the handler method, I think at least the first 2 commits could be quickly mergeable if you focus on those first. If the first two commits are clean and you re-push then I could re-review while you consider the third more. If the 3rd is also ready that would be fine too of course - just emphasizing that we could merge incrementally, which would help decrease turnaround time :)

zulipterminal/api_types.py

neiljp · 2022-08-21T18:35:34Z

zulipterminal/api_types.py

+class Person(TypedDict):
+    user_id: int
+    full_name: str
+    timezone: str
+    display_email: str
+    email: str
+    role: int
+    avatar_url: str
+    avatar_version: int
+    profile_data: Dict[str, str]


This does provide a set of names which the person object may contain, so supports type checking at some level.

You seem to be missing some possible elements of this, though? If some aren't used right now then we can comment them out, with a note to indicate that we don't handle them. That will cause mypy to alert us if we use fields that aren't explicitly listed.
(I referred to https://zulip.com/api/get-events#realm_user-update)

I know the API refers to it as person, but I think we should use a more specific name for the type here given the module namespace, maybe even RealmUserEventPerson to indicate it's only used there.

For now I would be happy with the current format with the additional elements, perhaps with blank lines between each group of fields. These type additions would be fine in a separate early commit before you use them.

However, it also doesn't represent the actual API, where eg. if we get full_name then nothing other than the user_id common to all cases should be present? So we should be able to encode that in the typing here. Closer to merging we could consider adjusting the separate commit to improve the typing as I mention above, or even after this PR.

neiljp · 2022-08-21T18:36:05Z

zulipterminal/model.py

@@ -8,6 +8,7 @@
 from concurrent.futures import Future, ThreadPoolExecutor, wait
 from copy import deepcopy
 from datetime import datetime
+from pprint import pprint


Yeah, it was to print the events containing realm user update. As this is still in draft stage, I thought of keeping it for reference for anybody testing this.

If you think it's useful, it's good to leave such things in a separate followup commit, since then cleaning up should be easier and you can label the commit appropriately.

zulipterminal/model.py

zulipbot · 2022-08-28T02:08:02Z

Hello @mounilKshah, it seems like you have referenced #988 in your pull request description, but you have not referenced them in your commit message description(s). Referencing an issue in a commit message automatically closes the corresponding issue when the commit is merged, which makes the issue tracker easier to manage.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #988..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

To learn how to write a great commit message, please refer to our guide.

neiljp

@mounilKshah This is nicely split and tidier, and I appreciate you pushing the changes, but as per inline notes the api types don't seem to match the docs, and I think there was confusion over how to handle what I meant by only 'update' ops.

The above and a test for the new function in the 2nd commit are necessary to get those merged ahead of the 3rd commti.

For the part related to the third commit, this changes initial_data but doesn't go further as per comments in my last review. Look for where the various 'active' user data that could get updated by these events is first extracted from initial_data, and think how we might go from there.

zulipterminal/api_types.py

neiljp · 2022-08-28T04:54:25Z

zulipterminal/api_types.py

@@ -167,6 +167,35 @@ class ReactionEvent(TypedDict):
    message_id: int


+class RealmUserEventPerson(TypedDict):


The layout looks better here and I can see you've extended them, but they don't seem to match the API still?
The API link from the last review has more fields than you have here, and I'm not seeing display_email or profile_data? Did you seem them elsewhere? Do the docs need updating?

display_email and profile_data are actually present in the commit. Did you mean to say delivery_email instead of display_email?

I don't see display_email in the API, or profile_data, but they were in the commit.

neiljp · 2022-08-28T04:58:06Z

zulipterminal/model.py

@@ -8,6 +8,7 @@
 from concurrent.futures import Future, ThreadPoolExecutor, wait
 from copy import deepcopy
 from datetime import datetime
+from pprint import pprint


If you think it's useful, it's good to leave such things in a separate followup commit, since then cleaning up should be easier and you can label the commit appropriately.

zulipterminal/model.py

This commit contains types for RealmUserEvent and RealmUserEventPerson added to api_types.

neiljp · 2022-09-24T16:03:04Z

zulipterminal/api_types.py

+
+    role: int
+
+    email: str


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?

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.

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

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

neiljp

@mounilKshah Tests look good, left a few thoughts to make them clearer 👍

API spec isn't critical, but unsure if there's something which has confused your aligning these values with those in the docs.

neiljp · 2022-09-24T16:20:51Z

tests/model/test_model.py

+        "person, changed_details",
+        [
+            (
+                {"user_id": 11, "full_name": "New Full Name"},


The user_id is common in all cases, so let's specify and build it in the test function.

Rather than setting the variable in the test function, you can alternatively list it as a default argument value, as long as it's a non-mutable value.

neiljp · 2022-09-24T16:22:37Z

tests/model/test_model.py

+                "delivery_email",
+            ),
+        ],
+        ids=[


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.

neiljp · 2022-09-24T16:28:37Z

tests/model/test_model.py

+        event = {"type": "realm_user", "op": "update", "id": 1000, "person": person}
+        model._handle_realm_user_event(event)
+        assert (
+            initial_data["realm_users"][1][changed_details]


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?

Okay, I'll add a comment for this 👍

This commit contains Model._handle_realm_user_event() which updates the meta data of the respective user in Model.initia_data["realm_users"]. It also contains test for this function in test_model.

This commit updates Model.initial_data when the realm_user: update event occurs for the current user.

zulipbot · 2022-09-25T06:54:23Z

Heads up @mounilKshah, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

neiljp · 2022-09-25T06:59:08Z

@mounilKshah Thanks for the updates here! I just merged the first two commits of this manually with some minor adjustments to avoid the need to add the "email" field 🎉
(last commit was 01b3853)

I think we'll likely need to do quite a lot of refactoring before other updates of this form, so the user updates aren't as critical right now.

zulipbot added size: L [Automatic label added by zulipbot] area: event handling How events from the server are responded to GSoC Possible GSoC project component missing feature: user A missing feature for all users, present in another Zulip client labels Aug 21, 2022

neiljp reviewed Aug 21, 2022

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback PR needs buddy review labels Aug 21, 2022

mounilKshah force-pushed the user_update_events branch from 22a865f to 9f75b0f Compare August 28, 2022 02:07

zulipbot added size: M [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Aug 28, 2022

mounilKshah added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Aug 28, 2022

mounilKshah marked this pull request as ready for review August 28, 2022 02:09

neiljp reviewed Aug 28, 2022

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Aug 28, 2022

mounilKshah force-pushed the user_update_events branch from 9f75b0f to 90a6c66 Compare August 28, 2022 12:38

mounilKshah force-pushed the user_update_events branch from 90a6c66 to 715fc79 Compare September 12, 2022 17:09

api_types: Add RealmUserEvent and RealmUserEventPerson.

669a834

This commit contains types for RealmUserEvent and RealmUserEventPerson added to api_types.

mounilKshah force-pushed the user_update_events branch from 715fc79 to 58effe5 Compare September 24, 2022 09:10

zulipbot added size: L [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Sep 24, 2022

mounilKshah added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Sep 24, 2022

neiljp reviewed Sep 24, 2022

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed PR needs buddy review labels Sep 24, 2022

mounilKshah added 2 commits September 25, 2022 09:57

model/test_model: Add function to handle realm_user event update.

6cfeae4

This commit contains Model._handle_realm_user_event() which updates the meta data of the respective user in Model.initia_data["realm_users"]. It also contains test for this function in test_model.

model: Update data of current user for realm_user update events.

359dd01

This commit updates Model.initial_data when the realm_user: update event occurs for the current user.

mounilKshah force-pushed the user_update_events branch from 58effe5 to 359dd01 Compare September 25, 2022 04:28

zulipbot added the has conflicts label Sep 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add handling for User update events #1249

Add handling for User update events #1249

mounilKshah commented Aug 21, 2022

neiljp left a comment

neiljp Aug 21, 2022

neiljp Aug 21, 2022

mounilKshah Aug 27, 2022

neiljp Aug 28, 2022

zulipbot commented Aug 28, 2022

neiljp left a comment

neiljp Aug 28, 2022

mounilKshah Aug 28, 2022

neiljp Aug 28, 2022

neiljp Aug 28, 2022

neiljp Sep 24, 2022

mounilKshah Sep 24, 2022 •

edited

Loading

neiljp Sep 25, 2022

neiljp Sep 25, 2022

neiljp left a comment

neiljp Sep 24, 2022

neiljp Sep 24, 2022

neiljp Sep 24, 2022

mounilKshah Sep 24, 2022

zulipbot commented Sep 25, 2022

neiljp commented Sep 25, 2022

		@@ -167,6 +167,35 @@ class ReactionEvent(TypedDict):
		message_id: int


		class RealmUserEventPerson(TypedDict):

Add handling for User update events #1249

Are you sure you want to change the base?

Add handling for User update events #1249

Conversation

mounilKshah commented Aug 21, 2022

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zulipbot commented Aug 28, 2022

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mounilKshah Sep 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zulipbot commented Sep 25, 2022

neiljp commented Sep 25, 2022

mounilKshah Sep 24, 2022 •

edited

Loading