Feature: Add user info view #848

Ezio-Sarthak · 2020-12-10T16:10:06Z

What does this PR do?!

Adds a basic interpolable user info view from web app.
Custom fields omitted for now.
Press i in the users list to see user info.

Commit flow

Create UI <=> API mappings for a realm user.
Adds a method to return tidied user info, so that it becomes displayable.
Support backward compatibility of API user response.
Adds hotkey i to trigger the popup.
Structures the user info popup to be shown.
Adds UI elements for toggling the popup.
Adds keypress events to trigger the popup.

Fixes #511

Screenshots/GIF

neiljp

@Ezio-Sarthak This looks like a good start 👍

I like that you've got the separate commits, though we may want to squash some together to represent the changes that should only be visible at the same time.

I do like u on a message to some extent, I'm just wary of using keys for actions and users getting used to them, only for us to run out of keys to use later :) I think a u from message info, or i from the user list might be good triggers for the popup, until we can agree about u from the message list.

I often find myself wondering whether someone may be awake, so I look forward to the next iteration :)

Pipfile

neiljp · 2020-12-13T00:45:05Z

zulipterminal/config/keys.py

+    ('USER_INFO', {
+        'keys': ['u'],
+        'help_text': 'View user description',
+        'key_category': 'msg_actions',


I think we could try u as a key for this, though previously from messages we've aimed to have keys from the message-info popup rather than directly - this keeps simple keys free for later use. Any change to keys.py should be in the same commit as one to the keys list in README.md.

I think it would definitely be helpful to hook the same popup functionality into the user list, probably via i in that case - in parallel to the same key being applied to message and streams.

I totally feel you :) Just like the edit history view in the message info, I'll add the key for the user info view (i) so that we're clear with the keys.
Yeah, I'll hook the README and keys commit together :)

neiljp · 2020-12-13T00:47:44Z

zulipterminal/ui_tools/views.py

+        user_details = []
+        for key, value in display_data.items():
+            if(key == 'Name'):
+                continue
+            user_details.append((key,value))


This looks like it could be a lot simpler as a comprehension.

Yeah, absolutely. I'll make it cleaner and simpler :)

zulipterminal/core.py

neiljp · 2020-12-13T01:28:42Z

zulipterminal/core.py

+            display_data['Local time'] = datetime.now(pytz.timezone(str(res_data['timezone']))).strftime("%H:%M")
+        except:
+            display_data['Local time'] = "Unknown timezone"
+        presence = self.client.get_user_presence(str(res_data['email']))


We should have this information already? Do we need to fetch it? What does the webapp do?

Yeah, I think so (not completely sure though).
By the GET request client.get_user_by_id(user_id), I got the following dict:

{ 'result': 'success', 'msg': '', 'user': { 'email': '[email protected]', 'user_id': 16790, 'avatar_version': 2, 'is_admin': False, 'is_owner': False, 'is_guest': False, 'is_bot': False, 'full_name': 'Matt Hall', 'timezone': 'America/Los_Angeles', 'is_active': True, 'date_joined': '2020-07-24T21:53:55.770840+00:00', 'avatar_url': '/user_avatars/2/848f00cf86435f7a12c7f6dafb476e47a7ee5665.png?x=x&version=2' } }

Seems like we have to fetch the timezone of a user in any case. Please drop any suggestions if you have :)
UPDATE: The logged-in user's time zone is stored in page_params js global variable in webapp.

Right, well the user data should be available in the model from what I can tell. It would benefit from refactoring, but the initial data is in Model.initial_data['realm_users'].

Thanks @neiljp for reminding me of the users realm, I've updated the PR accordingly :)

zulipterminal/core.py

zulipterminal/ui_tools/boxes.py

Pipfile

zulipterminal/ui_tools/views.py

Ezio-Sarthak · 2020-12-14T03:36:15Z

@neiljp Thanks for the review!
I too feel the same regarding the local time delay 😄
I'll update and do all the changes mentioned, and certainly make the PR more cleaner one :)

Ezio-Sarthak · 2020-12-15T18:02:16Z

PR updated :)

neiljp · 2021-01-07T01:11:56Z

@Ezio-Sarthak This is one of many in the queue to review, but you could check over your commit titles.

Ezio-Sarthak · 2021-01-07T06:38:11Z

Commit titles improved :)

neiljp

@Ezio-Sarthak This looks tidier than I remember 👍

We're essentially making new data available here, rather than just plumbing through to existing prepared data, so don't be too concerned this is taking a bit longer than other PRs. In particular the project has ended up with a lot of duplicate test data in the past which became quite challenging to update, so I'm keen to keep this as clean as we can, within reason :)

neiljp · 2021-04-21T02:19:06Z

zulipterminal/model.py

+            tz = pytz.timezone(user_info['timezone'])
+            time = datetime.datetime.now(tz).replace(tzinfo=None).timestamp()


Reading the pytz docs, it seems that passing a timezone when constructing a datetime is not advised, and I suspect that's what is happening with now(tz)?

Right. I'll consider this and your other comment mentioning now() declaration in UI.

neiljp · 2021-04-21T05:26:56Z

zulipterminal/ui_tools/views.py

+        if data['timezone']:
+            display_data['Timezone'] = data['timezone']
+        if data['local_time']:
+            display_data['Local time'] = data['local_time'][11:]


My timezone output from this has an underscore instead of a space; I'm not sure if there's a built-in conversion for this?
I do see that this is also the case in the webapp too though!

A possible point to consider (or in the future in a next iteration) is that while the "local time" is going to be very similar between the model+UI, now changes rather continuously, so it might be more meaningful to have the user info timezone be something to use, and the local time generated here.

Agreed. I understand the timezone strings might be unaltered due to internal calculations, and the server would expect to let the client-side handle it. This should be straightforward with replace in our case. Could you elaborate (either here or in stream) exactly what similarity you observed regarding the buggy underscore behavior with webapp ?:sweat_smile:

neiljp · 2021-05-11T17:20:47Z

zulipterminal/model.py

@@ -774,6 +778,7 @@ def get_all_users(self) -> List[Dict[str, Any]]:
                'user_id': bot['user_id'],
                'status': 'inactive',
            }
+            self.cross_realm_bots_by_id[user['user_id']] = user


Would it be simpler to also add an entry to all_users_by_id for cross-realm-bots here too?

neiljp · 2021-05-11T17:25:28Z

zulipterminal/model.py

+        self.all_users_by_id: Dict[int, Dict[str, Any]] = {}
+        self.cross_realm_bots_by_id: Dict[int, Dict[str, Any]] = {}


These are intended to be internal, so let's indicate that with a leading underscore.

At risk of expanding this PR further, you could clarify the Any by adding new types to api_types - you may be able to use a style similar to the Event Union to simplify between regular users and bots?

I'd expect a blank line before these lines.

Re the 2nd point, I like this idea. Though, I think the API responses for realm_users and cross_realm_bots are mostly the same, as seen in the register API response.

That said, I wonder if we could keep it a single TypedDict for now?

neiljp · 2021-05-11T17:28:56Z

zulipterminal/model.py

+        data = self.all_users_by_id.get(user_id, None)
+        if not data:
+            # If the user is a cross realm bot (e.g., Welcome Bot)
+            data = self.cross_realm_bots_by_id.get(user_id, None)


This would be simplified by combining the regular and cross-realm bots - unless there's a reason we need the latter separate?

Yes. I think I could remove this commit now that I could access all users via a single get command. What do you think?

zulipterminal/model.py

neiljp · 2021-05-14T19:18:07Z

zulipterminal/ui_tools/views.py

+            display_data['Local time'] = data['local_time'][11:]
+
+        if data['is_bot']:
+            display_data['Role'] = BOT_TYPE_BY_ID[data['bot_type']]


To be forward-compatible, we likely want to make this into a .get with some kind of 'unknown' text?

Possibly the same for USER_ROLE in terms of the default value, as eg. 'moderator' would not be identified right now - which I've tested since I now am one :)

Yeah, that makes sense. For USER_ROLE, I wish to propose that as I'm returning a custom-made role param from get_tidied_user_info, I could forcefully pass is_moderator (with False as default, or specified value) to it, and then handle it easily. I suppose it would make sense with regards to backward compatibility?

Noting in my above proposal, if is_moderator is false, it wouldn't affect the default Member value as well, due to the internal ordering in role :)

neiljp · 2021-05-14T19:31:02Z

tests/ui_tools/test_popups.py

+        mocker.patch.object(self.controller, 'maximum_popup_dimensions',
+                            return_value=(64, 64))
+        mocker.patch(VIEWS + '.urwid.SimpleFocusListWalker', return_value=[])
+        self.user_data = dict(


This is essentially a fixture for the output of get_tidied_user_info?

I understand for getting this going that a dict internal to the test is easier, but it would be good if we could unify a "sample output" from that method, ie. relate it to our current standard test users in some way. If we change the output from the model method then we need to change the internals of this test otherwise - or eg. handling adding is_moderator.

tests/ui_tools/test_popups.py

zulipterminal/api_types.py

neiljp

@Ezio-Sarthak I'm keen to merge this, so after having merged (split) the first commit with a slight adjustment as we discussed, I've left a few comments and I'd like to get this in ASAP.

After reviewing the data we get from the server today, I think we're not quite handling the role correctly, and there's also an older-style bot_owner field to consider.

I think we need to integrate the user-id code better, but it's in now for use in other PRs.

zulipterminal/model.py

tests/model/test_model.py

This commit adds two constant dicts (BOT_TYPE_BY_ID, ROLE_BY_ID) to translate bot/role ids into descriptive text, as well as connect old (`is_*`) and new role specifications.

This commit adds a model method to tidy and return required user information given user id. The returned information is in the form of a new TypedDict 'TidiedUserInfo'. Tests added for individual properties and sample human user. Fixture `tidied_user_info_response` added in `conftest.py`.

This commit handles the API response for older servers. The endpoints are prioritized according to the `RealmUser` TypedDict in `api_types.py`. API response changes (old -> new): (Server versions noted in api_types.py) * bot_owner (email: str) -> bot_owner_id (user_id: int) * is_* (e.g., is_admin: bool) -> role (role: int) Tests added.

As a v0, user info would be seen by pressing `i` in the users' list. This would allow the unification of the keypress event for `i` in all three views. It could be integrated to also trigger from message info view as a follow up, potentially with a different hotkey. docs/hotkeys.md regenerated.

This commit adds the class responsible to show user information. * It inherits the generalized PopUpView class. * Fetches tidied data to be displayed from model through static method `_fetch_user_data()`. Tests added (UserInfoView).

* Member function show_user_info() to trigger the popup. * New theme area added: `area:user`. This could generally be used in the future for user-related areas.

This commit wraps up the user-info feature by defining keypress events to toggle user info view. Tests added. Fixes zulip#511.

neiljp · 2021-07-10T23:04:50Z

@Ezio-Sarthak Thanks for the fixture work; I squashed those fixup commits from yesterday and also added a tiny test to use that new fixture early on, making a few tiny commit text changes, resulting in what's on the PR now - which I'm merging now! 🎉

We should really test those new model attributes we added, though I was ~ok with merging that commit early, there's nothing validating them right now.

Ezio-Sarthak changed the title ~~Feature: Add user info view~~ [WIP] Feature: Add user info view Dec 11, 2020

Ezio-Sarthak changed the title ~~[WIP] Feature: Add user info view~~ Feature: Add user info view Dec 11, 2020

Ezio-Sarthak mentioned this pull request Dec 11, 2020

Feature: Enable view of user info #511

Closed

neiljp reviewed Dec 13, 2020

View reviewed changes

Ezio-Sarthak force-pushed the add-user-info-view branch 10 times, most recently from 0f6733a to bf698ef Compare December 18, 2020 08:25

neiljp added the PR needs review PR requires feedback to proceed label Dec 21, 2020

Ezio-Sarthak force-pushed the add-user-info-view branch from bf698ef to aa8a559 Compare December 23, 2020 09:59

zulipbot added the size: XL [Automatic label added by zulipbot] label Dec 23, 2020

Ezio-Sarthak force-pushed the add-user-info-view branch 2 times, most recently from 2b5d88a to cab7b28 Compare December 24, 2020 10:01

Ezio-Sarthak requested a review from neiljp December 29, 2020 06:48

Ezio-Sarthak force-pushed the add-user-info-view branch 3 times, most recently from df964b1 to af34576 Compare January 6, 2021 15:55

Ezio-Sarthak force-pushed the add-user-info-view branch 3 times, most recently from cb1a099 to 3e1741e Compare January 7, 2021 06:28

Ezio-Sarthak force-pushed the add-user-info-view branch 2 times, most recently from e0ee338 to c39a7b2 Compare April 28, 2021 11:17

Ezio-Sarthak force-pushed the add-user-info-view branch from c39a7b2 to 21319e8 Compare May 13, 2021 16:08

neiljp reviewed May 14, 2021

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 May 14, 2021

Ezio-Sarthak force-pushed the add-user-info-view branch from 21319e8 to a9dc63f Compare June 13, 2021 16:44

Ezio-Sarthak commented Jun 13, 2021

View reviewed changes

zulipterminal/api_types.py Outdated Show resolved Hide resolved

Ezio-Sarthak 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 Jun 13, 2021

Ezio-Sarthak force-pushed the add-user-info-view branch from a9dc63f to b43fc1e Compare July 6, 2021 04:34

neiljp reviewed Jul 8, 2021

View reviewed changes

zulipterminal/model.py Outdated Show resolved Hide resolved

zulipterminal/model.py Outdated Show resolved Hide resolved

zulipterminal/model.py Show resolved Hide resolved

tests/model/test_model.py Outdated Show resolved Hide resolved

tests/model/test_model.py Show resolved Hide resolved

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 Jul 8, 2021

Ezio-Sarthak force-pushed the add-user-info-view branch 2 times, most recently from 6da0b68 to 6dab928 Compare July 9, 2021 10:06

Ezio-Sarthak 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 Jul 9, 2021

Ezio-Sarthak force-pushed the add-user-info-view branch from 6dab928 to 5a039dd Compare July 10, 2021 17:15

Ezio-Sarthak and others added 7 commits July 10, 2021 15:03

ui_mappings: Create UI <=> API mappings of a realm user.

6644e65

This commit adds two constant dicts (BOT_TYPE_BY_ID, ROLE_BY_ID) to translate bot/role ids into descriptive text, as well as connect old (`is_*`) and new role specifications.

views: Structure UI in UserInfoView class.

ee85756

This commit adds the class responsible to show user information. * It inherits the generalized PopUpView class. * Fetches tidied data to be displayed from model through static method `_fetch_user_data()`. Tests added (UserInfoView).

core/themes: Add UI elements to user info view on toggling.

bcc2ee2

* Member function show_user_info() to trigger the popup. * New theme area added: `area:user`. This could generally be used in the future for user-related areas.

buttons: Add USER_INFO keypress events to trigger user info popup.

92dbb8c

This commit wraps up the user-info feature by defining keypress events to toggle user info view. Tests added. Fixes zulip#511.

neiljp force-pushed the add-user-info-view branch from 5a039dd to 92dbb8c Compare July 10, 2021 22:55

neiljp merged commit 05a468f into zulip:main Jul 10, 2021

neiljp mentioned this pull request Mar 20, 2022

Added feature: Show user info by pressing 'i' #519

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Add user info view #848

Feature: Add user info view #848

Ezio-Sarthak commented Dec 10, 2020 •

edited

Loading

neiljp left a comment

neiljp Dec 13, 2020

Ezio-Sarthak Dec 14, 2020

neiljp Dec 13, 2020

Ezio-Sarthak Dec 14, 2020

neiljp Dec 13, 2020

Ezio-Sarthak Dec 14, 2020 •

edited

Loading

neiljp Dec 16, 2020

Ezio-Sarthak Dec 16, 2020

Ezio-Sarthak commented Dec 14, 2020 •

edited

Loading

Ezio-Sarthak commented Dec 15, 2020

neiljp commented Jan 7, 2021

Ezio-Sarthak commented Jan 7, 2021

neiljp left a comment

neiljp Apr 21, 2021

Ezio-Sarthak May 26, 2021

neiljp Apr 21, 2021

Ezio-Sarthak May 26, 2021

neiljp May 11, 2021

neiljp May 11, 2021

Ezio-Sarthak May 26, 2021 •

edited

Loading

neiljp May 11, 2021

Ezio-Sarthak May 26, 2021

neiljp May 14, 2021

Ezio-Sarthak May 26, 2021 •

edited

Loading

neiljp May 14, 2021

neiljp left a comment

neiljp commented Jul 10, 2021

		tz = pytz.timezone(user_info['timezone'])
		time = datetime.datetime.now(tz).replace(tzinfo=None).timestamp()

		self.all_users_by_id: Dict[int, Dict[str, Any]] = {}
		self.cross_realm_bots_by_id: Dict[int, Dict[str, Any]] = {}

Feature: Add user info view #848

Feature: Add user info view #848

Conversation

Ezio-Sarthak commented Dec 10, 2020 • edited Loading

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

Ezio-Sarthak Dec 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ezio-Sarthak commented Dec 14, 2020 • edited Loading

Ezio-Sarthak commented Dec 15, 2020

neiljp commented Jan 7, 2021

Ezio-Sarthak commented Jan 7, 2021

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

Choose a reason for hiding this comment

Ezio-Sarthak May 26, 2021 • edited Loading

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

Ezio-Sarthak May 26, 2021 • edited Loading

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

neiljp commented Jul 10, 2021

Ezio-Sarthak commented Dec 10, 2020 •

edited

Loading

Ezio-Sarthak Dec 14, 2020 •

edited

Loading

Ezio-Sarthak commented Dec 14, 2020 •

edited

Loading

Ezio-Sarthak May 26, 2021 •

edited

Loading

Ezio-Sarthak May 26, 2021 •

edited

Loading