[$1000] On "Split Bill" page, the display name doesn't sync on other devices after renaming it

kbecciv commented 1 year 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!

Action Performed:

Log into Account A
Split a bill with multiple accounts, including Account B
While on the split bill page, rename the display name on Account B

Expected Result:

Account B's display name on device A should synchronously update just after it has been renamed on account B's device

Actual Result:

Renaming doesn't synchronously update on device A.

Workaround:

Unknown

Platforms:

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

[ ] Android / native
[ ] Android / Chrome
[ ] iOS / native
[ ] iOS / Safari
[x] MacOS / Chrome / Safari
[ ] MacOS / Desktop

Version Number: 1.3.22.0

Reproducible in staging?: yes

Reproducible in production?: yes

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

Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/2b2aa9a4-0e90-4033-8f19-1d262717036b

https://github.com/Expensify/App/assets/93399543/a4bd99e5-81ad-45e7-a3c2-6dbd80db080b

Expensify/Expensify Issue URL:

Issue reported by: @Nathan-Mulugeta

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1685512857055539

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01182a1297b99a7cd4
Upwork Job ID: 1664733315940458496
Last Price Increase: 2023-07-10

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @maddylewis (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[x] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[x] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[x] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

namhihi237 commented 1 year ago

Proposal

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

Account B's display name on device A should synchronously update just after it has been renamed on account B's device

What is the root cause of that problem?

The problem here is that when we initialize commponent MoneyRequestConfirmationList this.state.participants is initialized in constructor with value calculated from this.getParticipantsWithAmount(props.participants). Therefore, if props.participants changes, this.state.participants is not updated accordingly.

https://github.com/Expensify/App/blob/d8b1d84c2f4a583ac2a863899c11bb5baff04ade/src/components/MoneyRequestConfirmationList.js#L107-L115

And similarly in the MoneyRequestModal component, the selectedOptionsis initialized . If props.report or props.personalDetails changes after the MoneyRequestModal component is initialized, selectedOptions will not change accordingly. https://github.com/Expensify/App/blob/8acc63040d8257859f988ff7b9804fea3acffc15/src/pages/iou/MoneyRequestModal.js#L107-L111

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

With MoneyRequestConfirmationList we will add.


componentDidUpdate(prevProps) {
if (prevProps.participants !== this.props.participants) {
    const formattedParticipants = _.map(this.getParticipantsWithAmount(this.props.participants), (participant) => ({
        ...participant,
        selected: true,
    }));

    this.setState({ participants: formattedParticipants });
}
}

2. With `MoneyRequestModal`:

useEffect(() => { setSelectedOptions( ReportUtils.isPolicyExpenseChat(props.report) ? OptionsListUtils.getPolicyExpenseReportOptions(props.report) : OptionsListUtils.getParticipantsOptions(props.report, props.personalDetails), ); }, [props.report, props.personalDetails]);


Result:

https://github.com/Expensify/App/assets/39086067/4430913b-23a7-4a53-ad7b-116bbc3e4884

### What alternative solutions did you explore? (Optional)
N/A

honnamkuan commented 1 year ago

Proposal

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

When user A choose to split bill using LHN + button > Enter Amount > Choose User B > Next > reaching the confirmation page. If User B changes his/her user profile display name at this moment, the new name does not get reflected in user A's split bill confirmation page.

What is the root cause of that problem?

This issue happens because MoneyRequestConfirmationList.js is displaying the split bill confirmation page using participants in state. The state get populated from the selectedOptions state in MoneyRequestModal.js component but the difference is it get formatted with amount within MoneyRequestConfirmationList.js.

When user B updates his profile display name, the names already stored within selectedOptions does not get updated on Onyx personalDetails key updates, and since there is no prop changes, there are also no updates being done in MoneyRequestConfirmationList.js, hence the old user B display name still get shown.

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

Add an useEffect in MoneyRequestModal.js to update the display text field within selectedOptions list object whenever there is an update of props.personalDetails

e.g.

    useEffect(() => {
        const newParticipants = _.chain(selectedOptions)
                .map((p) => {
                    const newParticipant = _.clone(p);
                    const matchedPersonalDetail = lodashGet(props.personalDetails, newParticipant.alternateText);
                    if (matchedPersonalDetail) {
                        newParticipant.text = matchedPersonalDetail.displayName;
                    }
                    return newParticipant;
                })
                .value();
            setSelectedOptions(newParticipants);
    }, [props.personalDetails]);

Add a componentDidUpdate lifecycle function in MoneyRequestConfirmationList.js, so that whenever props.participants changes, they will get reformatted with amount and updated into the component state for rendering.

e.g.

    componentDidUpdate(prevProps) {
        if (_.isEqual(prevProps.participants, this.props.participants)) {
            return;
        }
        const formattedParticipants = _.map(this.getParticipantsWithAmount(this.props.participants), (participant) => ({
            ...participant,
            selected: true,
        }));
        this.setState({participants: formattedParticipants});
    }

By applying the changes above, MoneyRequestConfirmationList.js will get updated with new participant display names when there is a pusher event to personalDetails.

What alternative solutions did you explore? (Optional)

N/A

Test Results

https://github.com/Expensify/App/assets/5896391/8875fa04-8ebd-4fe8-9b85-b6c765b1c545

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Current assignee @maddylewis is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

honnamkuan commented 1 year ago

@abdulrahuman5196 Just would like to highlight that my proposal https://github.com/Expensify/App/issues/20010#issuecomment-1573465372 fixes the issue regardless if user choose to split bill from FAB + button, or from a group chat.

whereas the proposal by namhihi237 would only works when user split bill from group chat. In fact it causes a regression when split bill is done from FAB (which is what's demonstrated in the bug report video), as there will be no props.report.participants when bill splitting is not done in a report, that will results in the participants list being updated to empty list when any of the participant update their name.

Appreciate your review.

bernhardoj commented 1 year ago

We have a refactor PR here that will handle the name/avatar changes in confirmation page. Here is the specific changes that handle that. It's not final yet though, so I guess we can hold this one.

MonilBhavsar commented 1 year ago

Thanks! Let's put it on HOLD as the code coincides and might be fixed. Putting it on HOLD on issue https://github.com/Expensify/App/issues/17662

maddylewis commented 1 year ago

still holding for - https://github.com/Expensify/App/issues/17662

MonilBhavsar commented 1 year ago

still on hold

s77rt commented 1 year ago

No longer on hold. https://github.com/Expensify/App/issues/17662 is resolved.

MonilBhavsar commented 1 year ago

Taking it off hold

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

MonilBhavsar commented 1 year ago

@namhihi237 and @honnamkuan do you want to update your proposal we have refactored the code. @abdulrahuman5196 could you help to review the proposals here, thanks!

honnamkuan commented 1 year ago

@MonilBhavsar thanks for the ping. I just done some testing, seems like this issue is no longer reproducible after the refactoring changes.

namhihi237 commented 1 year ago

Thanks, Let me check and feedback.

Nathan-Mulugeta commented 1 year ago

Hey @honnamkuan I just tested it right now, it is still reproducible, I will attach a video down below. It would help if you would share a screen recording with us to identify if there are any missed steps.

https://github.com/Expensify/App/assets/111440031/fe2527d8-372c-4885-9c1f-d4920874d260

bernhardoj commented 1 year ago

The issue is fixed when you split bill with a user that you have a chat with.

https://github.com/Expensify/App/assets/50919443/84ed53c2-bbe4-431d-a886-c90c5444a834

The video (by @Nathan-Mulugeta) shows a split bill with a new user that you never chat before. If you never have a chat with a user, you don't have their personal detail data. Because we don't have the personal detail data, I think it's impossible to get the name (or anything) updates.

maddylewis commented 1 year ago

thanks for that breakdown @bernhardoj! to confirm, was the issue fixed via https://github.com/Expensify/App/issues/17662, or do we still need to address something here?

bernhardoj commented 1 year ago

Technically, yes. But let's have a confirmation from @MonilBhavsar about this https://github.com/Expensify/App/issues/20010#issuecomment-1623052679

abdulrahuman5196 commented 1 year ago

@MonilBhavsar

@bernhardoj 's response here is correct https://github.com/Expensify/App/issues/20010#issuecomment-1623052679

This issue is now happening only for user who you have never chatted with. What the expectation here? Should the display name change? AFAIK the answer is no, correct me if wrong.

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

MonilBhavsar commented 1 year ago

This doesn't seem like issue to me https://github.com/Expensify/App/issues/20010#issuecomment-1621778418 If there is not chat between users, we won't send pusher events for name update and it won't get updated. Not an issue IMO. cc @Beamanator @puneetlath buddy check please 🙏

Beamanator commented 1 year ago

Agreed @MonilBhavsar 👍

MonilBhavsar commented 1 year ago

Yoo, thanks Alex 🙌

MonilBhavsar commented 1 year ago

@maddylewis, we can close this issue now. It will be great to add regression testing steps for this flow. Reported issue was fixed by https://github.com/Expensify/App/pull/17964/ So, we need to pay reporting bonus to @Nathan-Mulugeta

maddylewis commented 1 year ago

sorry for the delay here! tomorrow morning i will pay the $250 reporting bonus to @Nathan-Mulugeta and then close the issue!

MonilBhavsar commented 1 year ago

@maddylewis is processing payment

maddylewis commented 1 year ago

offer sent to @Nathan-Mulugeta

Nathan-Mulugeta commented 1 year ago

Just accepted offer @maddylewis

maddylewis commented 1 year ago

paid!

Expensify / App