zulip-terminal
zulip-terminal copied to clipboard
Add id to users with same name
What does this PR do? On the Zulip terminal, users with same name weren't discernable because they were considered the same users. This PR compares users based on their user id and then displays the user id beside their user name if they have the same user name.
Fixes #1151
Tested?
- [x] Manually
- [x] Existing tests (adapted, if necessary)
- [x] New tests added (for any new behavior)
- [x] Passed linting & tests (each commit)
Commit flow first commit -> comparing users based on user_id instead of name so that a separate header can be displayed
second commit -> displaying user_id beside all the users who have same name as more than one user in the list of all users. Adding a new style class so that the appended user_id doesn't look like a part of the username
third commit -> changing label from names
to msg_sender
in all colour themes
Notes & Questions This makes users with same name have different headers by appending the id to the author name and shows it as a visually separate addition.
Interactions
- Waiting on PR review
Visual changes
Before:
After:
@Sushmey I just merged the last commit you had and merged that first with slight adjustments, since it does the independent rename of name->msg_sender.
Some things to note:
- this change was quite separate, so having it first in the sequence of commits would mean it'd have been closer to the original code branch (now main), so easier to partially merge first if it's a small preparation for others and fits in the same PR
- since it was the last commit, I adjusted the UI code to work without the changes - not much effort this time :)
- I updated the commit title & description:
- in ZT we include the file names as an 'area' if they can be made to fit
- generally it's good to use the present tense, so 'Rename' here vs 'Renaming'
- this particular commit doesn't fix 1151 :)
- it's useful to put the 'why' in the commit description
I'll take a look at the other commits.
@Sushmey Ah, I'm also finding the tests are failing locally, so that may have been a migration issue on my part.
I also meant to say that the last commit doesn't have test changes and clearly changes behavior, which is one reason I'd like to see a new test for previous commit, so that the code change can match a test change.
@neiljp Thank you so much for the review! 🤗
I've made changes based on your comments.
I added a separate test for duplicate usernames but different author ids to account for the new behaviour and updated a test case for the same. I tried including it in the same test but it doesn't seem possible without having to change every test case parameter. Also, added a mock response as false for the is_user_name_duplicate function so that it doesn't fail for the reasons mentioned earlier, and since duplicate usernames aren't what we are testing in that test anyway.