zulip-terminal icon indicating copy to clipboard operation
zulip-terminal copied to clipboard

ui: Add guest user suffix

Open AnkurPrabhu opened this issue 1 year ago • 8 comments

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

  • [x] Discussed in #zulip-terminal in Add guest user suffix #T1444 #T1460
  • [ ] Fully fixes #
  • [x] Partially fixes issue #1444
  • [ ] Builds upon previous unmerged work in PR #
  • [ ] Is a follow-up to work in PR #
  • [ ] Requires merge of PR #
  • [ ] Merge will enable work on #

How did you test this?

  • [x] Manually - Behavioral changes
  • [x] 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)
  • [x] 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

AnkurPrabhu avatar Jan 12 '24 20:01 AnkurPrabhu

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.

AnkurPrabhu avatar Jan 22 '24 19:01 AnkurPrabhu

@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.

neiljp avatar Jan 24 '24 07:01 neiljp

@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 avatar Jan 26 '24 10:01 AnkurPrabhu

@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 avatar Jan 29 '24 19:01 AnkurPrabhu

@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).

neiljp avatar Jan 29 '24 23:01 neiljp

@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

AnkurPrabhu avatar Jan 30 '24 13:01 AnkurPrabhu

@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.

neiljp avatar Feb 03 '24 01:02 neiljp

@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

AnkurPrabhu avatar Feb 24 '24 05:02 AnkurPrabhu