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

Add id to users with same name #1255

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

Conversation

Sushmey
Copy link
Collaborator

@Sushmey Sushmey commented Sep 15, 2022

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?

  • Manually
  • Existing tests (adapted, if necessary)
  • New tests added (for any new behavior)
  • 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:
Screenshot 2022-09-13 at 7 44 25 PM

After:
Screenshot 2022-10-21 at 11 48 24 AM

@zulipbot zulipbot added size: S [Automatic label added by zulipbot] size: M [Automatic label added by zulipbot] and removed size: S [Automatic label added by zulipbot] labels Sep 15, 2022
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.

I ran your code manually and it worked fine for me. I tried going through the files to give an in-depth review, but it is difficult to review due to the current commit structure. It would be great if you could squash the commits using git rebase.
Also, upon running pytest on your PR, there were errors. All those can be looked into deeper once the commit structure is streamlined.

zulipterminal/model.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/boxes.py Outdated Show resolved Hide resolved
@zulipbot zulipbot added size: L [Automatic label added by zulipbot] size: M [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] size: L [Automatic label added by zulipbot] labels Sep 24, 2022
@zulipbot zulipbot added size: L [Automatic label added by zulipbot] size: M [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] size: L [Automatic label added by zulipbot] labels Sep 25, 2022
@Sushmey Sushmey force-pushed the sushmey-dev-branch branch 4 times, most recently from c49b6a6 to 32f3598 Compare October 2, 2022 03:23
@Sushmey Sushmey force-pushed the sushmey-dev-branch branch 2 times, most recently from 6d0751e to 41c83cb Compare October 10, 2022 08:07
@zulipbot zulipbot added size: L [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Oct 10, 2022
@zulipbot zulipbot added size: XS [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Oct 12, 2022
@zulipbot zulipbot added size: L [Automatic label added by zulipbot] and removed size: XS [Automatic label added by zulipbot] labels Oct 12, 2022
@neiljp
Copy link
Collaborator

neiljp commented Mar 22, 2023

@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 neiljp force-pushed the sushmey-dev-branch branch 2 times, most recently from 8b85a73 to b3344a8 Compare March 22, 2023 20:22
@neiljp neiljp added bug Something isn't working area: message rendering labels Mar 22, 2023
@zulipbot zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Mar 23, 2023
@zulipbot zulipbot added size: L [Automatic label added by zulipbot] and removed size: XL [Automatic label added by zulipbot] labels Mar 23, 2023
@Sushmey Sushmey force-pushed the sushmey-dev-branch branch 2 times, most recently from 616238c to a6e9263 Compare April 25, 2023 08:07
@Sushmey Sushmey force-pushed the sushmey-dev-branch branch 3 times, most recently from 7dc3a92 to 8be2fcd Compare February 7, 2024 10:13
Users with same name had their adjacent messages displayed under the same
header and no visually discerning feature was available for users with same
name. This solves that.

Fixes zulip#1151.
@zulipbot zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Feb 9, 2024
Added a function that checks if username is duplicate.

Added tests for the same.
Add a style class to user id in header to differentiate from the username.

Users with same usernames have their user id appended to it.

Doing it in the same header test needs changing of all the parameters.

Hence adding a new test function specifically for that case.
@Sushmey
Copy link
Collaborator Author

Sushmey commented Feb 13, 2024

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

@neiljp neiljp 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 Mar 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: message rendering bug Something isn't working PR needs review PR requires feedback to proceed size: XL [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Two users with the the same name appear as the same person
4 participants