Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.89k stars 2.94k forks source link

[$250] When creating a group chat, some members appear as Hidden. #51934

Open m-natarajan opened 1 month ago

m-natarajan commented 1 month ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: Reproducible in staging?: Needs reproduction Reproducible in production?: Needs reproduction If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @dukenv0307 Slack conversation (hyperlinked to channel name): ts_external_expensify_bugs

Action Performed:

  1. Login with new account
  2. Click on start chat button
  3. Add some people to a group chat
  4. Click on the report header

Expected Result:

All members are displayed under display name or email

Actual Result:

Some people are shown as Hidden

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence https://github.com/user-attachments/assets/702eb5b9-1899-4155-bfec-6ac05023ba5c

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021853509471517999703
  • Upwork Job ID: 1853509471517999703
  • Last Price Increase: 2024-11-04
  • Automatic offers:
    • dukenv0307 | Contributor | 104738208
    • daledah | Contributor | 105178037
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @sakluger (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

MelvinBot commented 1 month ago

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

daledah commented 1 month ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

When creating a group chat, some members appear as hidden.

What is the root cause of that problem?

Many times when we call the searchForReports API to get personalDetail information, we are missing displayName field

bu-1

and when there is no displayName it will show as a Hidden via the getDisplayNameOrDefault function here

https://github.com/Expensify/App/blob/aa6401de4a3c02adce2cb898eba8c58e1c92b62d/src/pages/ReportParticipantsPage.tsx#L136

What changes do you think we should make in order to solve the problem?

We should add the default value is details?.login to the function getDisplayNameOrDefault to cover case missing displayName

       text: formatPhoneNumber(PersonalDetailsUtils.getDisplayNameOrDefault(details, details?.login))

https://github.com/Expensify/App/blob/aa6401de4a3c02adce2cb898eba8c58e1c92b62d/src/pages/ReportParticipantsPage.tsx#L136

We can do the same with other places

What alternative solutions did you explore? (Optional)

NA

daledah commented 1 month ago

I still can reproduce this bug

https://github.com/user-attachments/assets/deb93e1d-82a7-4c8a-a024-96412e4c3616

dukenv0307 commented 1 month ago

I report this bug so can I take it as C+? Thanks @sakluger

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~021853509471517999703

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @allroundexperts (External)

melvin-bot[bot] commented 1 month ago

πŸ“£ @dukenv0307 πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

sakluger commented 1 month ago

Hey @dukenv0307 - I've assigned you as C+. Thanks for helping with this one!

dukenv0307 commented 1 month ago

@daledah Bug:

https://github.com/user-attachments/assets/99010c85-2b21-4345-befa-f1becbb49b2f

daledah commented 1 month ago

@dukenv0307 We also need to add default value to displayName in ReportParticipantDetails component here

https://github.com/Expensify/App/blob/e2987bdedbbb47ee9cc1404f0c2e6557edc3d786/src/pages/ReportParticipantDetailsPage.tsx#L57

https://github.com/user-attachments/assets/c5d31611-09e4-482c-a74e-aa3920ce1fba

dukenv0307 commented 1 month ago

I'm not sure we should use login as a fallback value in all places.

BTW, the display name will be present in the following API

image

Should we fix it on BE or update some places on FE?

πŸŽ€πŸ‘€πŸŽ€ C+ reviewed

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @thienlnam, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

thienlnam commented 1 month ago

It looks intentional that the API doesn't always return a displayName if the user is not known - so we can probably fall back on the login since that should always be present

daledah commented 1 month ago

so we can probably fall back on the login since that should always be present

@thienlnam please assign to me, already reviewed C+ here

thienlnam commented 1 month ago

@dukenv0307 Did you approve the proposal as well or was that just to get an internal engineer assigned?

dukenv0307 commented 1 month ago

@thienlnam If we decide to fall back on the login, we should do that in all places, ideally in this function. But it's quite unsafe (Hidden is shown only when login is undefined). What do you think?

melvin-bot[bot] commented 4 weeks ago

@sakluger, @thienlnam, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

sakluger commented 4 weeks ago

@thienlnam any thoughts on @dukenv0307's question?

thienlnam commented 4 weeks ago

Hmm, I think that's okay - but cc @puneetlath for confirmation

sakluger commented 3 weeks ago

@puneetlath I assigned you temporarily - could you check Jack's question here? Feel free to unassign afterwards.

puneetlath commented 3 weeks ago

Interesting. I think that we should fall back on the login if there is no display name. Since users won't always have display names set.

But also it seems weird to me that we're not returning the display name from the BE. Is there one set for the user in question?

daledah commented 3 weeks ago

@puneetlath I tried many times but still can't figure out the logic that BE returns missing displayName

melvin-bot[bot] commented 3 weeks ago

@sakluger @thienlnam @dukenv0307 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 3 weeks ago

@sakluger, @thienlnam, @dukenv0307 Whoops! This issue is 2 days overdue. Let's get this updated quick!

sakluger commented 3 weeks ago

@thienlnam is there a display name set in the BE for the user in question on this GH issue?

thienlnam commented 3 weeks ago

Login is always present. Display name is not always present because there are certain conditions that need to be met for it to be returned (BE code). So it still seems fine to fall back to login if the displayName does not exist

I'm not quite following this though

Hidden is shown only when login is undefined

Isn't that expected that if you don't have a login for someone they will show as 'Hidden'?

dukenv0307 commented 3 weeks ago

@thienlnam

Login is always present.

We have the flag shouldFallbackToHidden with default value is true in

https://github.com/Expensify/App/blob/aa6401de4a3c02adce2cb898eba8c58e1c92b62d/src/libs/PersonalDetailsUtils.ts#L52

Login is always present.

As login is always present, if we decide to fall back to login, shouldFallbackToHidden and hiddenTranslation would be useless in this function. Please correct me if I misunderstand something.

I think we can go with @daledah's proposal to fix the problem in getDisplayNameOrDefault, but I need to confirm with you to avoid the regression

thienlnam commented 3 weeks ago

Ah I see thanks for clarifying. That's probably not desired in these cases since then we won't show Hidden at all anymore.

So actually the fix here seems like we should be returning the displayName if you are the person that added these users, otherwise for the other users that haven't chatted you would still see hidden since they haven't interacted in the chat yet. Once the users chat in the group chat, then their details should be visible to the rest of the members

Does that make sense? I'm not entirely sure how hidden members should work within the context of a group chat cc @puneetlath

sakluger commented 2 weeks ago

@puneetlath adding you back so you can help address expected behavior from the last comment. (Again, feel free to unassign once you answer).

melvin-bot[bot] commented 2 weeks ago

@puneetlath, @sakluger, @thienlnam, @dukenv0307 Whoops! This issue is 2 days overdue. Let's get this updated quick!

sakluger commented 2 weeks ago

I'm going to bring this back to the original Slack thread to discuss expected behavior.

puneetlath commented 2 weeks ago

Hmm, I think there's some confusion here. It doesn't seem like there's actually a bug. The login will not always be present if you don't know the user. Display name will always be present, if its been set, but not login.

sakluger commented 2 weeks ago

Backing up to the original issue...The issue was that we were hiding display name while showing the same user's email. As far as I know, there shouldn't be a scenario where the email is shown and the display name is hidden.

Here's a screenshot from the reproduction video:

image
puneetlath commented 2 weeks ago

Ah yes, I agree with that. There's no scenario that I'm aware of where that should be the case. If we are sending the email, we should be sending the display name too. And if the user doesn't have one, we should be using their email as their display name.

daledah commented 1 week ago

@thienlnam as expected here and C+ reviewed here, could you please assign me?

melvin-bot[bot] commented 1 week ago

@sakluger, @thienlnam, @dukenv0307 Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 1 week ago

@sakluger @thienlnam @dukenv0307 this issue is now 4 weeks old, please consider:

Thanks!

sakluger commented 1 week ago

@dukenv0307 and @thienlnam - given the clarified expected behavior, how should we proceed?

thienlnam commented 1 week ago

I don't think that's correct, as from my understanding that would be removing showing 'Hidden' at all

dukenv0307 commented 1 week ago

@thienlnam

And if the user doesn't have one, we should be using their email as their display name.

If so, we can update getDisplayNameOrDefault function to fallback to login. We should keep Hidden to be safe.

https://github.com/Expensify/App/blob/3cc88f5a83a979c5e588e01065083a3d714e69da/src/libs/PersonalDetailsUtils.ts#L78

thienlnam commented 1 week ago

Cool cool, the rest can be figured out in PR - just want to make sure we're not removing 'Hidden'

melvin-bot[bot] commented 1 week ago

πŸ“£ @daledah πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

sakluger commented 6 days ago

@thienlnam what is the scenario where the email is shown and the display name is hidden? There should be no scenario like that.

thienlnam commented 5 days ago

I don't think there's a case for that - my comment is more regarding if we don't have their email we should still be showing 'Hidden'

dukenv0307 commented 4 days ago

@thienlnam @sakluger @daledah I found the inconsistency when searching users, SearchForReports API sometime returns detail within login but sometimes not

Member: dukenv0307+6@gmail.com

SearchForReports returns personalDetailsList without login

https://github.com/user-attachments/assets/0c5dc8ff-5020-41ad-a9a2-00b2d357a1c3

SearchForReports returns personalDetailsList within login

https://github.com/user-attachments/assets/56f43d22-af1e-481b-8e2d-493b4ddf226b

thienlnam commented 2 days ago

cc @youssef-lr since it looks like you added that - is it the case that we only set the login if it matches the searched email exactly? Checking that context from here

dukenv0307 commented 2 hours ago

@youssef-lr Can you please take a look at this issue? Thanks