-
-
Notifications
You must be signed in to change notification settings - Fork 268
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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
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 |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
if realm_enable_guest_user_indicator and is_guest: | ||
self.suffix_style = "current_user" | ||
self.suffix_text = "(guest)" | ||
self.update_widget() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
3026f12
to
e29b0f1
Compare
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). |
@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:
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. |
There was a problem hiding this 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/model.py
Outdated
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 |
There was a problem hiding this comment.
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".
@@ -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), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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?
e29b0f1
to
3ad97a8
Compare
@neiljp this i have updated the pr with the latest changes:
all these felt like similar changes only so once you approve my current approach will start writing tests for the same . |
a7fef19
to
7f5e64b
Compare
7f5e64b
to
efa4597
Compare
@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? |
@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 However, I suspect you will have gitlint errors locally too. See the docs, but eg try something like |
@neiljp you missed this i guess if the changes are good ill write tests and we can close this |
@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. |
There was a problem hiding this 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.
zulipterminal/ui_tools/messages.py
Outdated
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"] | ||
), |
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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
""" | ||
Returns True if the realm_enable_guest_user_indicator | ||
setting is enabled and the user is a guest | ||
""" |
There was a problem hiding this comment.
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.
zulipterminal/model.py
Outdated
setting is enabled and the user is a guest | ||
""" | ||
user = self.get_user_info(user_id) | ||
assert user is not None |
There was a problem hiding this comment.
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.
efa4597
to
aadb367
Compare
There was a problem hiding this 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.
zulipterminal/model.py
Outdated
@@ -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: |
There was a problem hiding this comment.
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' : '', |
There was a problem hiding this comment.
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
zulipterminal/ui_tools/views.py
Outdated
@@ -657,6 +656,7 @@ class RightColumnView(urwid.Frame): | |||
""" | |||
|
|||
def __init__(self, view: Any) -> None: | |||
self.model = view.model |
There was a problem hiding this comment.
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.
zulipterminal/ui_tools/messages.py
Outdated
@@ -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 | |||
|
There was a problem hiding this comment.
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
zulipterminal/ui_tools/messages.py
Outdated
@@ -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"]) |
There was a problem hiding this comment.
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
zulipterminal/ui_tools/messages.py
Outdated
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) |
There was a problem hiding this comment.
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
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.
aadb367
to
0fa8dd0
Compare
There was a problem hiding this 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)) Theui_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: |
There was a problem hiding this comment.
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 tois_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_
orhas_
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' : '', |
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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?
if is_guest: | ||
self.suffix_style = "current_user" | ||
self.suffix_text = "(guest)" | ||
self.update_widget() |
There was a problem hiding this comment.
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.
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 |
There was a problem hiding this comment.
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)
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)
External discussion & connections
Add guest user suffix #T1444 #T1460
How did you test this?
Self-review checklist for each commit
Visual changes