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.

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

ui: Add guest user suffix #1460

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

AnkurPrabhu
Copy link
Collaborator

@AnkurPrabhu AnkurPrabhu commented Jan 12, 2024

This commit is for adding guest user suffix in the UI if the user is a guest user and if realm_enable_guest_user_indicator setting is True.

What does this PR do, and why?

This PR Implements #1444
The approach here is to update the function user_name_from_id to return the user's name with the suffix (guest) if the condition satisfies and calling the function in places where we display user's name.
The approach for Right column view is different from others we just pass the required arguments (realm_enable_guest_user_indicator and is_guest) to UserButton and add suffix to the users name depending upon the arguments

Outstanding aspect(s)

  • wanted to know if my approach is right
  • other scenarios where the display change needs to be done
  • will fix the test cases once approach is approved

External discussion & connections

How did you test this?

  • Manually - Behavioral changes
  • Manually - Visual changes
  • Adapting existing automated tests
  • Adding automated tests for new behavior (or missing tests)
  • Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

  • It is a minimal coherent idea
  • It has a commit summary following the documented style (title & body)
  • It has a commit summary describing the motivation and reasoning for the change
  • It individually passes linting and tests
  • It contains test additions for any new behavior
  • It flows clearly from a previous branch commit, and/or prepares for the next commit

Visual changes

Screenshot 2024-01-13 at 2 15 00 AM Screenshot 2024-01-13 at 2 15 07 AM Screenshot 2024-01-13 at 2 15 24 AM Screenshot 2024-01-13 at 2 15 39 AM Screenshot 2024-01-13 at 2 20 13 AM

@zulipbot zulipbot added the size: L [Automatic label added by zulipbot] label Jan 12, 2024
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@AnkurPrabhu Thanks for working on this :) 👍

The UI you have in your initial screenshots look like a good starting point 👍 The main concern would be to have consistent styling for the guest suffix, and certainly a space from the name. Specifically, we need it to be distinct from the user name, so it is clear that it is not part of the user name! A new style may be warranted for this purpose, since keeping it distinct will change with theme (see the themes main file and individual themes).

I've noted inline some general points and also regarding keeping the model and UI logic separate from each other.

While it seems like this is a relatively small change, particularly when including tests you will likely find it clearer to split this into smaller commits and address the changes to the model and each UI area separately. That makes it easier to merge good parts earlier, if the commits are relatively independent and can be reordered, or set up with simpler commits first. The user list changes look like they may be more complex, for example.

We may need to decide in a followup or in this PR what to do if text wraps. I think the server had a similar issue with the user list at one point - eg. do we prioritize the user name or the guest marker, or both? This also applies if the active user is a guest!

zulipterminal/model.py Outdated Show resolved Hide resolved
Comment on lines 1268 to 1278
user = self.get_user_info(user_id)
if not user:
raise RuntimeError("Invalid user ID.")

return self.user_dict[user_email]["full_name"]
role = user["role"]
name = user["full_name"]
is_guest = role == 600
guest_user_indicator = self.initial_data["realm_enable_guest_user_indicator"]
if guest_user_indicator and is_guest:
return name + "(guest)"
else:
return name
Copy link
Collaborator

Choose a reason for hiding this comment

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

While this update centralizes the change to enable each location to output the 'same' extra suffix, this limits the code from only accessing the user name.

We could add a supplemental helper function in the model for constructing the two parts as needed, but where the suffix goes is not relevant to the model - it should occur elsewhere, since the presentation is down to the UI code.

More generally, perhaps later, we may want to extract some elements from the get_user_info method, since that generates a translation upon every call, for all the user data, not just the role. That's fine for the user popup and occasional calls, but this is likely going to be used very often!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could add a supplemental helper function in the model for constructing the two parts as needed, but where the suffix goes is not relevant to the model - it should occur elsewhere, since the presentation is down to the UI code.

can you explain this as in which two parts ? and what exactly do you mean by where the suffix goes is not relevant to the model.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mainly meant the name and the suffix as the two parts.

In terms of 'where the suffix goes', the styling and placement of the suffix is not something that the model should be handling - that is an issue for the UI. The model should only be concerned with providing an answer to "should I show a guest suffix for this user".

zulipterminal/ui_tools/buttons.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/views.py Outdated Show resolved Hide resolved
Comment on lines 293 to 294
if realm_enable_guest_user_indicator and is_guest:
self.suffix_style = "current_user"
self.suffix_text = "(guest)"
self.update_widget()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This gets overridden by the '(you)' indicator; does the web app do that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the web app does not contain '(you)' for the current user but on hover it overrides the guest suffix take a look at the screenshot attached, Ill put this down the code so that '(you)' gets overridden if you want me to add guest for the active user as well.
Screenshot 2024-01-16 at 7 42 13 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, this will be part of the challenge with how to render this in the user list. I'm not sure there's an obvious solution given our platform constraints, but isolating these changes to their own commit will make this easier to examine separately.

@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback area: UI General user interface update version parity: 8 labels Jan 15, 2024
@AnkurPrabhu
Copy link
Collaborator Author

Hey @neiljp i have tried a new approach in this one pls let me know your thoughts also as you told to split it in different prs this pr will be related to adding the guest suffix in the messages box (changes related to header and recipients).
For displaying the guest suffix in the right column that will be a separate pr but the changes in the button have been added here as I wanted to know your thoughts.

@neiljp
Copy link
Collaborator

neiljp commented Jan 24, 2024

@AnkurPrabhu Thanks for following up. Unfortunate I didn't fetch the previous version, so it's difficult for me to identify precisely what you've changed in this version. I've left some notes inline, as well as responses to earlier comments.

Please note:

  • I was mainly referring to multiple commits; these can be with the same PR if they work together
  • some suffix work can certainly be left to another PR, but the foundational work likely wants to be in the first PR
  • I know you left the button changes in, but you could still keep these in the PR, but in a different commit :)

For the next step, I'd suggest splitting your work here into multiple commits in this PR, and thinking how you might write tests for each new code. Firstly I'd advise adding a model method like I mentioned in a response to an earlier comment, and then adapting the rest of your code to use that method.

Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@AnkurPrabhu I left my main comments in another comment, which refers to these comments too :)

zulipterminal/ui_tools/messages.py Outdated Show resolved Hide resolved
zulipterminal/model.py Outdated Show resolved Hide resolved
Comment on lines 1268 to 1278
user = self.get_user_info(user_id)
if not user:
raise RuntimeError("Invalid user ID.")

return self.user_dict[user_email]["full_name"]
role = user["role"]
name = user["full_name"]
is_guest = role == 600
guest_user_indicator = self.initial_data["realm_enable_guest_user_indicator"]
if guest_user_indicator and is_guest:
return name + "(guest)"
else:
return name
Copy link
Collaborator

Choose a reason for hiding this comment

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

I mainly meant the name and the suffix as the two parts.

In terms of 'where the suffix goes', the styling and placement of the suffix is not something that the model should be handling - that is an issue for the UI. The model should only be concerned with providing an answer to "should I show a guest suffix for this user".

zulipterminal/ui_tools/views.py Outdated Show resolved Hide resolved
@@ -68,6 +68,7 @@
'task:error' : (Color.WHITE, Color.DARK_RED),
'task:warning' : (Color.WHITE, Color.BROWN),
'ui_code' : (Color.BLACK, Color.WHITE),
'guest_suffix' : (Color.WHITE, Color.BLACK),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is one place where a style can be added to a theme, but note there are others - it will need adding in each place.

For example, look for one of the other style names throughout the code.

See previous style-adding commits in the git log for examples - we tend to add a style in all places in a single commit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes i saw there are a lot of files, I just wanted to show this to know if I am on the right track or not, will add it in all the files

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that the alignment needs adjusting manually in most of these files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have done it in all the files right ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This covers all the files, but the alignment is not quite right.

Did you check what the styles look like?
The one in zt_dark is the same as regular text right now, I think?

zulipterminal/ui_tools/utils.py Outdated Show resolved Hide resolved
@AnkurPrabhu
Copy link
Collaborator Author

@neiljp this i have updated the pr with the latest changes:

  1. have added style to all the current themes
  2. currently showing guest user suffix at
  • message header
  • message recipients
  • right column (button)
  • message info view
  • message stream members view
  • user info view
    let me know if I am missing the suffix at any place

all these felt like similar changes only so once you approve my current approach will start writing tests for the same .
also i was thinking of making a constant in the model like guest_suffix= "( guest)" so that we can use this everywhere instead if writing it as a string everywhere... @neiljp thoughts ?

@AnkurPrabhu AnkurPrabhu force-pushed the Add-guest-user-suffix branch 2 times, most recently from a7fef19 to 7f5e64b Compare January 29, 2024 15:08
@zulipbot zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Jan 29, 2024
@AnkurPrabhu
Copy link
Collaborator Author

AnkurPrabhu commented Jan 29, 2024

@neiljp i have fixed all the existing tests and have split the commits as suggested, can you tell me how to fix this gitlint one which is failing?

@neiljp
Copy link
Collaborator

neiljp commented Jan 29, 2024

@AnkurPrabhu The test error appears to be due to your branch not including the setup code for the gitlint in CI. This should resolve if you fetch the latest main branch and rebase this branch against the updated main which has the appropriate commits to install gitlint in CI.

However, I suspect you will have gitlint errors locally too. See the docs, but eg try something like gitlint --commits HEAD~3.. (or with main.., once you're up to date with main).

@AnkurPrabhu
Copy link
Collaborator Author

@neiljp this i have updated the pr with the latest changes:

  1. have added style to all the current themes
  2. currently showing guest user suffix at
  • message header
  • message recipients
  • right column (button)
  • message info view
  • message stream members view
  • user info view
    let me know if I am missing the suffix at any place

all these felt like similar changes only so once you approve my current approach will start writing tests for the same . also i was thinking of making a constant in the model like guest_suffix= "( guest)" so that we can use this everywhere instead if writing it as a string everywhere... @neiljp thoughts ?

@neiljp you missed this i guess if the changes are good ill write tests and we can close this

@neiljp
Copy link
Collaborator

neiljp commented Feb 3, 2024

@AnkurPrabhu Those locations look good; if we miss one then it can be added later.

I agree that extracting a common constant would be good, but note again that this is not something that would belong in the model, for the same reasons as before.

Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@AnkurPrabhu I just left some feedback in addition to answering your questions.

The new model method is in the right direction, and the split commits are an improvement 👍

However, please do split the UI changes into separate ones for each UI part that you update.

Comment on lines 683 to 689
different = { # How this message differs from the previous one
"recipients": recipient_header is not None,
"author": message["this"]["author"] != message["last"]["author"],
"suffix": (
message["last"]["suffix"] is not None
and message["this"]["suffix"] != message["last"]["suffix"]
),
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's unclear to me if we need to include the 'different' part here, and so even the 'suffix' elements in the earlier dicts. Can we only add it later?

@@ -68,6 +68,7 @@
'task:error' : (Color.WHITE, Color.DARK_RED),
'task:warning' : (Color.WHITE, Color.BROWN),
'ui_code' : (Color.BLACK, Color.WHITE),
'guest_suffix' : (Color.WHITE, Color.BLACK),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that the alignment needs adjusting manually in most of these files.

zulipterminal/model.py Outdated Show resolved Hide resolved
Comment on lines 2076 to 2079
"""
Returns True if the realm_enable_guest_user_indicator
setting is enabled and the user is a guest
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

A docstring is for users of the function, not a comment as to how it works.

If you feel there is a need for a description, beyond the name of the function, then please adjust the docstring - but it should not describe the implementation.

setting is enabled and the user is a guest
"""
user = self.get_user_info(user_id)
assert user is not None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on existing model code, let's raise a RuntimeError here.

We should move towards better exceptions for these cases in future though.

zulipterminal/model.py Show resolved Hide resolved
@AnkurPrabhu
Copy link
Collaborator Author

@neiljp I have split it based on ui.
I have not split the message info view as i thought they are part of the message itself.
I Have fixed all the existing tests.

@neiljp can you guide me on what are the next steps or changes i should work on

@mounilKshah 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 Feb 24, 2024
Copy link
Collaborator

@mounilKshah mounilKshah left a comment

Choose a reason for hiding this comment

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

Thank you for updating the PR @AnkurPrabhu
This works fine on my system. I think the commit titles and messages for all commits can be modified in accordance with the Zulip commit standards.

I had a few suggestions and have mentioned them in the review.

@@ -2094,3 +2094,12 @@ def poll_for_events(self) -> None:
self.controller.raise_exception_in_main_thread(
sys.exc_info(), critical=False
)

def show_guest_suffix(self, user_id: int) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is a general boolean function which determines whether the user_id passed belongs to a guest user or not, should we rename it to is_guest_user?

@@ -78,6 +78,7 @@
'task:error' : 'standout',
'task:warning' : 'standout',
'ui_code' : 'bold',
'guest_suffix' : '',
Copy link
Collaborator

Choose a reason for hiding this comment

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

The commit title and message for this commit can be made more informative about the contents of this commit. Maybe something like config/themes: Add guest_suffix styling to themes

@@ -657,6 +656,7 @@ class RightColumnView(urwid.Frame):
"""

def __init__(self, view: Any) -> None:
self.model = view.model
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this line is required.

@@ -64,6 +64,7 @@ def __init__(self, message: Message, model: "Model", last_message: Any) -> None:
self.topic_links: Dict[str, Tuple[str, int, bool]] = dict()
self.time_mentions: List[Tuple[str, str]] = list()
self.last_message = last_message

Copy link
Collaborator

Choose a reason for hiding this comment

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

You may remove the extra line breaks here and elsewhere in the next iteration

@@ -91,16 +92,24 @@ def __init__(self, message: Message, model: "Model", last_message: Any) -> None:
if self._is_private_message_to_self():
recipient = self.message["display_recipient"][0]
self.recipients_names = recipient["full_name"]
self.recipients_names += (
" (guest)"
if self.model.show_guest_suffix(user_id=recipient["id"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since there is only one argument passed in the function, I think it is okay to not use the user_id assignment, instead simply pass the ID

Comment on lines 103 to 111
self.recipients_names = ""
recipients = []
for recipient in self.message["display_recipient"]:
if recipient["email"] != self.model.user_email:
recipient_name = self.recipients_names = recipient["full_name"]
if self.model.show_guest_suffix(user_id=recipient["id"]):
recipient_name += " (guest)"
recipients.append(recipient_name)
self.recipients_names = ", ".join(recipients)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This works fine but you can also maintain the original python comprehension and only add the suffix to the recipient_name with a conditional, similar to how it's done above in the private message conditional block

@mounilKshah mounilKshah added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Feb 24, 2024
This commit is for adding a function is_guest_user() in the model
which will validate if a user is a guest user and
realm_enable_guest_user_indicator setting is True.
This commit is for adding the style for guest suffix for all the themes
currently present
This commit is for adding guest user suffix in the right column of the
UI. The approach here is to update the username by passing
the value returned by model.is_guest_user() to the UserButton
and depending upon the argument passed we show the suffix.
This commit is for adding guest user suffix in the UI if the user is
a guest user and if realm_enable_guest_user_indicator setting is True.
The approach here is to update the username by adding the guest
suffix  if model.is_guest_user() returns True.
@zulipbot zulipbot added size: L [Automatic label added by zulipbot] and removed size: XL [Automatic label added by zulipbot] labels Feb 26, 2024
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@AnkurPrabhu Thanks for making the changes 👍
(as noted in the stream, I started this yesterday, but had issues on my reviewing computer)

I've yet to test this on a live server with guest users - have you done so?

I've left some followup comments where you or I missed something.

In addition to the inline comments:

  • Somewhere we discussed adding the (guest) string as a constant? (ui: Add guest user suffix #1460 (comment)) The ui_mappings.py file seems like a reasonable place for this; it can be in it's own commit, and then we can squash it with the first UI commit that gets merged.
  • It's good to see the user list changes in a separate commit, but could you separate the changes to each other UI part separately too? I expect they should be independent of each other, and that would make it much easier to see the changes associated with each, test them separately, reorder them, or work on them later - and the commit title/body can vary for each to explain the feature you are adding in that specific commit.
  • Each commit title should be able to be relatively standalone, and describe what it does itself, not the PR overall; this might be just due to the current commits not being updated, but that also helps with review - it explains each commit as part of the set.

The user column looks like it is going to be the more complex part, so that commit might be better at the end. This may need to be a symbol, as for text like elsewhere it needs more space, and so might be dependent upon a dynamic larger column width if the window is wide enough, which someone looked into working on a while back.

@@ -2094,3 +2094,12 @@ def poll_for_events(self) -> None:
self.controller.raise_exception_in_main_thread(
sys.exc_info(), critical=False
)

def is_guest_user(self, user_id: int) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is a general boolean function which determines whether the user_id passed belongs to a guest user or not, should we rename it to is_guest_user?

If this were strictly the case, I would agree with you @mounilKshah, but this is a combination of something like is_guest_user plus what the organization setting is.

The previous show_guest_suffix was an improvement in this way, but it's worth noting:

  • show_guest_suffix(user_id) reads like an action for that user
  • an is_ or has_ prefix (as per Mounil's suggestion) is generally good for a boolean function, but wouldn't work here. Maybe something based on this comment? (ui: Add guest user suffix #1460 (comment))
  • while the issue does indicate a suffix, the server setting describes it as an indicator, and we don't need to have it as a suffix in the UI (it could be a prefix or symbol, or another way of marking a user as a guest)

While getting a name 'just right' is not critical, it really helps make code more readable! It also makes docstrings less necessary. We/you/I can set the name later if you'd prefer :)

@@ -78,6 +78,7 @@
'task:error' : 'standout',
'task:warning' : 'standout',
'ui_code' : 'bold',
'guest_suffix' : '',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you test this in monochrome mode?

@@ -68,6 +68,7 @@
'task:error' : (Color.WHITE, Color.DARK_RED),
'task:warning' : (Color.WHITE, Color.BROWN),
'ui_code' : (Color.BLACK, Color.WHITE),
'guest_suffix' : (Color.WHITE, Color.BLACK),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This covers all the files, but the alignment is not quite right.

Did you check what the styles look like?
The one in zt_dark is the same as regular text right now, I think?

Comment on lines +296 to +299
if is_guest:
self.suffix_style = "current_user"
self.suffix_text = "(guest)"
self.update_widget()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I previously missed a problem with the approach you're using here: it's possible for users to have unread direct messages from guests, and the unread count is shown in the suffix.

We originally allowed (you) in the suffix since the unread count associated with messages to yourself would be zero (at least before the prospect of marking messages unread, which we don't currently handle).

That's what I meant by the conversation around #1460 (comment): we need to determine how to handle showing an extra piece of data in buttons, or at least those for user buttons.

Comment on lines +2099 to +2105
if self.initial_data.get("realm_enable_guest_user_indicator", None):
user = self.get_user_info(user_id)
if not user:
raise RuntimeError("Invalid user ID.")
role = user["role"]
return role == 600
return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

This logic is much simpler than you had before 👍

Adding a test for this new function (in this same commit) would help define the function better.

Other than splitting out role aspects from the get_user_info, there is one other part of this that we can slightly optimize, that shouldn't need checking in such a detailed way each time we call this. Do you see what it is?
(This change would also potentially simplify your test, and help with handling events related to it)

@neiljp neiljp removed the PR needs review PR requires feedback to proceed label Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: UI General user interface update PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback size: L [Automatic label added by zulipbot] version parity: 8
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants