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.54k stars 2.88k forks source link

[$250] mWeb - Group - New users are not displayed in group name #46502

Closed lanitochka17 closed 1 month ago

lanitochka17 commented 3 months 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: 9.0.14 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): https://expensify.testrail.io/index.php?/tests/view/4784615 Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/home
  2. Log in a new gmail account
  3. Tap fab - start chat
  4. Enter a new user with no expensify account and tap add to group
  5. Tap group name.

Expected Result:

New users must be displayed in group name

Actual Result:

New users are not displayed in group name Reproducible on these devises: Samsung galaxy Ao2s/12 - Chrome. Redmi note 10s/Android 13, Samsung Galaxy m2/Android 12

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/f4d9936d-2285-4465-8c94-ba8263c5d5af

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01511ab0ad63b9289d
  • Upwork Job ID: 1818320616796150714
  • Last Price Increase: 2024-07-30
  • Automatic offers:
    • hoangzinh | Reviewer | 103362702
    • nkdengineer | Contributor | 103362703
Issue OwnerCurrent Issue Owner: @hoangzinh
melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @lschurr (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.

lanitochka17 commented 3 months ago

We think that this bug might be related to #vip-vsp

lanitochka17 commented 3 months ago

@lschurr FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

lanitochka17 commented 3 months ago

We think that this bug might be related to #vip-vsp

cretadn22 commented 3 months ago

Proposal

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

Group - New users are not displayed in group name

What is the root cause of that problem?

In this case, we use ReportUtils.getGroupChatName function to get group name

https://github.com/Expensify/App/blob/5d26dbe390d1642748a481e194e41bdbf2d95cab/src/pages/NewChatConfirmPage.tsx#L64

And in ReportUtils.getGroupChatName function, we use getDisplayNameForParticipant function

https://github.com/Expensify/App/blob/6bc174f949d8c0a41327809c4dcbfa5dc261fe13/src/libs/ReportUtils.ts#L2017

The getDisplayNameForParticipant function return empty string, because the App can't find accountID of new user in personalDetail

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

We need to add a fallback value when the App can't find accountID of new user in personalDetail. Keep in mind that the new user's email is stored in newGroupDraft.participants

In getGroupChatName function, we can replace participantAccountIDs param into participant param. This participant parameter will contain both the accountID and the login field, allowing us to use participant.login as a default value.

Additional, we maybe need to apply the same solution in here

https://github.com/Expensify/App/blob/5d26dbe390d1642748a481e194e41bdbf2d95cab/src/pages/GroupChatNameEditPage.tsx#L50

Result:

Ảnh chụp Màn hình 2024-07-30 lúc 21 38 58

What alternative solutions did you explore? (Optional)

We can introduce new param in getDisplayNameForParticipant function called participant. And then if the App can't find accountID of new user in personalDetail, we will use participant.login as a default value

cretadn22 commented 3 months ago

I have implemented my solution locally and included the results as evidence in my proposal

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

nkdengineer commented 3 months ago

Proposal

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

New users are not displayed in group name Reproducible on these devises: Samsung galaxy Ao2s/12 - Chrome. Redmi note 10s/Android 13, Samsung Galaxy m2/Android 12

What is the root cause of that problem?

If we select a new user on offline mode or select a new user before SearchForReports API is complete, the personal details of this user don't exist in Onyx. So getDisplayNameForParticipant returns an empty string here and then the new user doesn't display in the group chat name.

https://github.com/Expensify/App/blob/d2fdb27359dd524c419192c03c5bbbf8c121dcde/src/libs/ReportUtils.ts#L2016-L2018

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

getDisplayNameForParticipant function is used in many places of the app so we should handle to fallback the login in getGroupChatName function as a special case instead of handling it in getDisplayNameForParticipant function.

function getGroupChatName(draftParticipants?: SelectedParticipant[], shouldApplyLimit = false, report?: OnyxEntry<Report>): string | undefined {
let participants = draftParticipants?.map((participant) => participant.accountID) ?? Object.keys(report?.participants ?? {}).map(Number);

https://github.com/Expensify/App/blob/d2fdb27359dd524c419192c03c5bbbf8c121dcde/src/libs/ReportUtils.ts#L2002

getDisplayNameForParticipant(participant, isMultipleParticipantReport) || draftParticipants?.[index]?.login

https://github.com/Expensify/App/blob/d2fdb27359dd524c419192c03c5bbbf8c121dcde/src/libs/ReportUtils.ts#L2017

const groupName = newGroupDraft?.reportName ? newGroupDraft?.reportName : ReportUtils.getGroupChatName(newGroupDraft?.participants);

https://github.com/Expensify/App/blob/d2fdb27359dd524c419192c03c5bbbf8c121dcde/src/pages/NewChatConfirmPage.tsx#L64

const existingReportName = useMemo(
    () => (report ? ReportUtils.getGroupChatName(undefined, false, report) : ReportUtils.getGroupChatName(groupChatDraft?.participants)),
    [groupChatDraft?.participants, report],
);

https://github.com/Expensify/App/blob/d2fdb27359dd524c419192c03c5bbbf8c121dcde/src/pages/GroupChatNameEditPage.tsx#L49-L52

What alternative solutions did you explore? (Optional)

nkdengineer commented 3 months ago

Updated proposal

hoangzinh commented 3 months ago

Thanks for proposals, everyone. @nkdengineer's proposal looks good to me

Link to proposal https://github.com/Expensify/App/issues/46502#issuecomment-2258799397

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 3 months ago

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

cretadn22 commented 3 months ago

@hoangzinh

His solution is safer and similar with other places

This also be mentioned in my proposal

cretadn22 commented 3 months ago

His solution is safer and similar with other places

@hoangzinh What are the differences between my proposal and @nkdengineer's proposal? I notice that @nkdengineer's proposal is more detailed, but the main idea is the same.

hoangzinh commented 3 months ago

Ah @cretadn22 I didn't mean @nkdengineer's proposal has more details than yours. But:

Btw, as a new contributor, your proposal looks good.

cretadn22 commented 3 months ago

In getDisplayNameForParticipant function, we can replace participantAccountIDs param into participant param. This participant parameter will contain both the accountID and the login field, allowing us to use participant.login as a default value.

Ahhh, This is my mistake, I mean getGroupChatName function

cretadn22 commented 3 months ago

The rest is correct to getGroupChatName, I only make mistake about function name

we can replace participantAccountIDs param into participant param. This participant parameter will contain both the accountID and the login field, allowing us to use participant.login as a default value.

cretadn22 commented 3 months ago
Ảnh chụp Màn hình 2024-08-01 lúc 18 56 19

This is my local implementation for the proposal. I have made code changed and thoroughly tested it before posting proposal, and I’ve included an image of the result in my proposal.

hoangzinh commented 3 months ago

Yeah, but according to Contributing guides, I can only select 1 proposal that's earliest provided, best proposed solution + RCA. Let's wait for the internal engineer to review it.

stitesExpensify commented 3 months ago

This is a tough one, but I agree that @nkdengineer should get the job for the reasons Vinh mentioned here

melvin-bot[bot] commented 3 months ago

📣 @hoangzinh 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 3 months ago

📣 @nkdengineer 🎉 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 📖

melvin-bot[bot] commented 2 months ago

This issue has not been updated in over 15 days. @hoangzinh, @lschurr, @stitesExpensify, @nkdengineer eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

hoangzinh commented 2 months ago

Not overdue. The PR has been merged to Production since last week. Ready for payment. I will complete BZ checklist soon

hoangzinh commented 2 months ago

BugZero Checklist:

nkdengineer commented 1 month ago

@lschurr This is ready for payment for a while, could you help to apply the Daily label? TIA

lschurr commented 1 month ago

Hi folks! Sorry, I was OOO last week. Handling payments now.

lschurr commented 1 month ago

Payment summary: