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

SGeetansh · 2020-03-04T17:58:26Z

Fixes #511
This pull request implements a feature of displaying the information of the user in focus. All the information that is visible in the web app is present.
It doesn't pass the tests right now. Please suggest any required changes.

sumanthvrao

@SGeetansh Thanks for working on this. I gave a quick review of some aspects I came across.

Haven't looked into the the core show_user_info method thoroughly yet, but based on some manual testing the data which is being show looks good!

I know it's still a draft PR, but you could try splitting it into commits so reviewing becomes quicker as well. :)

sumanthvrao · 2020-03-05T01:50:39Z

requirements.txt

@@ -13,5 +13,5 @@ emoji==0.5.0
 urwid_readline==0.10
 beautifulsoup4==4.6.0
 lxml==4.2.3
-
+pytz==2019.3


@SGeetansh In order to upgrade a dependency / introduce a new one, requirements.txt is not the only place you need to include this. You should also introduce it in Pipfile and setup.py besides this. This is so that, running pip3 install -e .[dev] automatically updates the local environment and adds these packages.

For more details run git log --grep='requirements' and see how previous upgrades have been written.

sumanthvrao · 2020-03-06T01:20:37Z

zulipterminal/config/keys.py

@@ -200,6 +200,11 @@
        'help_text': 'View stream description',
        'key_category': 'stream_list',
    }),
+    ('USER_INFO', {


It would also benefit to have a note for this in README. (Maybe these two changes can be extracted into a separate commit)

sumanthvrao · 2020-03-06T01:21:42Z

zulipterminal/model.py

@@ -79,14 +79,14 @@ def __init__(self, controller: Any) -> None:
        self.stream_id = -1
        self.recipients = frozenset()  # type: FrozenSet[Any]
        self.index = initial_index
-
+    


Some extra spaces ;)

sumanthvrao · 2020-03-06T01:24:19Z

zulipterminal/ui_tools/buttons.py

@@ -213,6 +215,11 @@ def _narrow_with_compose(self, button: Any) -> None:
        self._view.body.focus.original_widget.set_focus('footer')
        self._view.write_box.private_box_view(self)

+    def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
+        if is_command_key('USER_INFO', key):
+            self.model.controller.show_user_info(self.user_id)


alternatively, can we could just use self.controller.show_user_info here , and avoid the line on top? :)

sumanthvrao · 2020-03-06T01:34:54Z

zulipterminal/core.py

+    def show_user_info(self, user_id:int) -> None:
+        user_id = user_id
+        url = 'users/' + str(user_id)
+        result = self.client.call_endpoint(


It's always good to check if the result was a success or not here. We do that in many places so you can have a look.

@sumanthvrao
I checked the other places. They just check if the response message was 'success' or not, with a return statement at the end of the function (return response['result'] == success). Do you want me to just write that only? Sorry, if I am wrong, but can you please elaborate on this? I didn't put a try-except here because I think it isn't required.

Yup, that's the correct way to do the validation. No need of a try-catch block.

The pytz library is used for timezone calculations. In our case it will be used to fetch the local time of a user given his/her local timezone. The standard time library can't be used.

sumanthvrao

@SGeetansh Thanks for sticking with this 👍

I feel the commit order and structure can be changed.By this, I mean, in the first commit you introduced some changes which you edited in the third commit again. Instead you could introduce the entire change and just avoid linking it for now. The linking can be done in the follow-up commits.

An example commit order:

The requirements update commit for pytz
UserInfoView commit (and the keypress). Since we don't call this anywhere, tests will still pass by just introducing this commit.
show_user_info in controller commit. Introduce the entire method here :)
lastly, Add the keypress to UserButton which helps link the changes made in previous commits.

So you see, the earlier commits build up to the later one, but only the last one does the actual linking.

One more point was that I saw a lot of diffs wrt to white spaces. Remember to remove them while you do a rebase.

Are you planning to add tests? (Again, a commit can have the code changes and the tests in it) :)

Let me know if you have further doubts.

sumanthvrao · 2020-03-11T01:23:43Z

zulipterminal/config/keys.py

@@ -200,6 +200,11 @@
        'help_text': 'View stream description',
        'key_category': 'stream_list',
    }),
+    ('USER_INFO', {
+        'keys': {'i'},


You could extract the README and keys into a sepearte commit as well. As it stands, this commit has a lot of file changes.

sumanthvrao · 2020-03-11T01:24:08Z

requirements.txt

@@ -13,5 +13,4 @@ emoji==0.5.0
 urwid_readline==0.10
 beautifulsoup4==4.6.0
 lxml==4.2.3
-
-zipp==1.0.0  # To support Python 3.5
+zipp==1.0.0  # To support Python 3.5


Unwanted diff? :)

sumanthvrao · 2020-03-11T01:31:40Z

zulipterminal/ui_tools/views.py

@@ -1,5 +1,6 @@
 import threading
 import time
+import math


Isn't import math used in your third commit. If so, let's import it only in the commit where it is used.

sumanthvrao · 2020-03-11T01:55:02Z

zulipterminal/core.py

+
+        show_userinfo_view = UserInfoView(self, display_data)
+        self.show_pop_up(show_userinfo_view, "{}".format(res_data['full_name']))
+        return response['result'] == 'success'


Where do you validate whether the return value was true/false?

Can you point me to an instance where this is done? I don't know what to do. And do I really need to do that because there would always be a response since if a user already exists, it has to have his info on the server, am I right?

@SGeetansh I meant you could do this at the beginning of this function after you make the API call. Only if the response result is a success, you can parse and store the parameters as you have done.

In case the API call fails, no errors will be thrown if you do this validation :)

sumanthvrao · 2020-03-11T01:56:51Z

zulipterminal/ui_tools/views.py

@@ -850,13 +850,26 @@ def keypress(self, size: Tuple[int, int], key: str) -> str:


 class UserInfoView(urwid.ListBox):
-    def __init__(self, controller, display_data) -> None:
+    def __init__(self, controller, display_data:dict) -> None:


Maybe type-hint more?

zulipbot · 2020-04-17T00:45:22Z

Heads up @SGeetansh, we just merged some commits that conflict with the changes your 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/master branch and resolve your pull request's merge conflicts accordingly.

neiljp · 2020-10-20T20:53:18Z

@SGeetansh I hope you had a good Summer - just checking in if you planned to update this PR?

If you do plan to update this:

Some timezone code has been added, so the dependency commit shouldn't be necessary now
The popup code has been streamlined, which may well make this easier - see PopUpView and child classes
Just having the available information from the existing presences data would be good in a working commit, then following up with custom profile fields in a later one - the titles need sourcing from somewhere as I think the values you're using are hard-coded from chat.zulip.org?

neiljp · 2022-03-20T05:53:26Z

Closing, as this was replaced via #848.

@SGeetansh Thanks for filing the issue and taking a shot here 👍

zulipbot added the size: XL [Automatic label added by zulipbot] label Mar 4, 2020

sumanthvrao reviewed Mar 6, 2020

View reviewed changes

SGeetansh added 2 commits March 8, 2020 18:03

Added the pop up for displaying user information

8e067b9

requirement: Added pytz 2019.3 for displaying local time in user info

2ec1f0a

The pytz library is used for timezone calculations. In our case it will be used to fetch the local time of a user given his/her local timezone. The standard time library can't be used.

SGeetansh force-pushed the UserInfoView branch from 0565508 to 9319514 Compare March 8, 2020 13:02

Fixes zulip#511: Press 'i' to show user info

00db9c0

SGeetansh force-pushed the UserInfoView branch from 9319514 to 00db9c0 Compare March 8, 2020 13:08

SGeetansh requested a review from sumanthvrao March 8, 2020 13:09

neiljp added the PR needs review PR requires feedback to proceed label Mar 9, 2020

sumanthvrao reviewed Mar 11, 2020

View reviewed changes

neiljp removed the PR needs review PR requires feedback to proceed label Apr 9, 2020

zulipbot added the has conflicts label Apr 17, 2020

Ezio-Sarthak mentioned this pull request Dec 10, 2020

Feature: Add user info view #848

Merged

Base automatically changed from master to main January 30, 2021 20:30

neiljp force-pushed the main branch from f9f483a to 793e73d Compare December 15, 2021 21:30

neiljp closed this Mar 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SGeetansh commented Mar 4, 2020 •

edited

Loading

sumanthvrao left a comment

sumanthvrao Mar 5, 2020

sumanthvrao Mar 6, 2020

sumanthvrao Mar 6, 2020

sumanthvrao Mar 6, 2020

sumanthvrao Mar 6, 2020

SGeetansh Mar 7, 2020 •

edited

Loading

sumanthvrao Mar 8, 2020

sumanthvrao left a comment •

edited

Loading

sumanthvrao Mar 11, 2020

sumanthvrao Mar 11, 2020

sumanthvrao Mar 11, 2020

sumanthvrao Mar 11, 2020

SGeetansh Mar 16, 2020 •

edited

Loading

sumanthvrao Mar 20, 2020 •

edited

Loading

sumanthvrao Mar 11, 2020

zulipbot commented Apr 17, 2020

neiljp commented Oct 20, 2020

neiljp commented Mar 20, 2022

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

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

Conversation

SGeetansh commented Mar 4, 2020 • edited Loading

sumanthvrao 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

SGeetansh Mar 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sumanthvrao left a comment • 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

Choose a reason for hiding this comment

SGeetansh Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

sumanthvrao Mar 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zulipbot commented Apr 17, 2020

neiljp commented Oct 20, 2020

neiljp commented Mar 20, 2022

SGeetansh commented Mar 4, 2020 •

edited

Loading

SGeetansh Mar 7, 2020 •

edited

Loading

sumanthvrao left a comment •

edited

Loading

SGeetansh Mar 16, 2020 •

edited

Loading

sumanthvrao Mar 20, 2020 •

edited

Loading