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.5k stars 2.85k forks source link

[$250] Task – Edited subtask name not updated in chat with user B in LHN #47655

Open IuliiaHerets opened 2 months ago

IuliiaHerets commented 2 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.22-1 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): applausetester+jp_e_category_2@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Log in
  3. Open group chat
  4. Create a task assigned to yourself with user B
  5. Open task details and create a subtask assigned to another user
  6. Open subtask details and edit subtask name
  7. Observe 1:1 chat with user B in LHN

Expected Result:

Edited subtask name updated in 1:1 chat with user B in LHN

Actual Result:

Edited subtask name not updated in 1:1 chat with user B in LHN

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/db4d3761-a989-4f55-a102-7ca20697f02c

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01df2aca4c9f6dbfa6
  • Upwork Job ID: 1826727852654566628
  • Last Price Increase: 2024-08-22
Issue OwnerCurrent Issue Owner: @jayeshmangwani
melvin-bot[bot] commented 2 months ago

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

IuliiaHerets commented 2 months ago

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

IuliiaHerets commented 2 months ago

@zanyrenney z 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

tsa321 commented 2 months ago

Edited by proposal-police: This proposal was edited at 2024-08-20 02:20:04 UTC.

Proposal

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

LHN Message is not updated on dm of the user and the user the task assgineed to when the task title is edited

What is the root cause of that problem?

In the LHN, if the last action is a task action, the displayed last message is retrieved from the task's report.reportName. However, the useMemo hook for the optionItem in LHNOption does not currently depend on the task report data:

https://github.com/Expensify/App/blob/f58439cfb116abd338dae880088a26725794108e/src/components/LHNOptionsList/OptionRowLHNData.tsx#L65-L79

As a result, the displayed last message in the LHN does not update when the task report is updated.

What changes should we make to solve this problem?

We need to adjust the optionItem dependency to include the lastActionChildReport.

To address this issue:

  1. Add a Utility Function: Add a utility function in OptionListUtils:

    function getChildReportIDofLastAction(report: OnyxEntry<Report>): string | undefined {
        if (!report?.reportID) {
            return;
        }
    
        const lastAction = lastVisibleReportActions[report.reportID];
        return lastAction?.childReportID;
    }
  2. Update LHNOptionList: In LHNOptionList, add the following lines after:

    https://github.com/Expensify/App/blob/f58439cfb116abd338dae880088a26725794108e/src/components/LHNOptionsList/LHNOptionsList.tsx#L132

    const lastActionChildReportID = OptionsListUtils.getChildReportIDofLastAction(itemFullReport); 
    const lastActionChildReport = lastActionChildReportID ? reports?.[`${ONYXKEYS.COLLECTION.REPORT}${lastActionChildReportID}`] : undefined;
  3. Modify OptionRowLHNData: In OptionRowLHNData, add the lastActionChildReport property. Then, update the useMemo of optionItem to include lastActionChildReport as a dependency:

    https://github.com/Expensify/App/blob/f58439cfb116abd338dae880088a26725794108e/src/components/LHNOptionsList/OptionRowLHNData.tsx#L65-L79

These changes will ensure that the displayed last message in the LHN updates correctly when the task report is updated. But if we only want the reportName data we can only use the lastActionChildReport.reportName as dependency to be more efficient.


If we also need to update the childReportName of lastTaskAction: ```javascript function getOptimisticDMTaskNameUpdate (taskReport, taskName) { const assigneeAccountID = getTaskAssigneeAccountID(taskReport); const dmReport = ReportUtils.getChatByParticipants([assigneeAccountID, currentUserAccountID]); const reportActions = allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${dmReport.reportID}`]; const taskAction = Object.values(reportActions).find((action) => action.childReportID === taskReport.reportID); return { onyxMethod: Onyx.METHOD.MERGE, key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${dmReport.reportID}`, value: { [taskAction.reportActionID]: { childReportName: taskName, pendingFields: { ...(taskName && {reportName: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE}), }, errorFields: null, }, }, }; } ``` Include similar code in the optimistic data of edit task in: https://github.com/Expensify/App/blob/6334d36a049d01cfa9a445f3747125d79a00bc98/src/libs/actions/Task.ts#L505

What alternative solutions did you explore? (Optional)

zanyrenney commented 2 months ago

nice! looking at external label noq

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

mkzie2 commented 2 months ago

Proposal

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

Edited subtask name not updated in 1:1 chat with user B in LHN

What is the root cause of that problem?

When we get the lastMessageTextFromReport we've had the logic to get the task created message via task report

https://github.com/Expensify/App/blob/844c6bf01829b88b7392cdc330a787a6366c8cec/src/libs/SidebarUtils.ts#L384

https://github.com/Expensify/App/blob/844c6bf01829b88b7392cdc330a787a6366c8cec/src/libs/OptionsListUtils.ts#L661-L664

https://github.com/Expensify/App/blob/844c6bf01829b88b7392cdc330a787a6366c8cec/src/libs/TaskUtils.ts#L41

https://github.com/Expensify/App/blob/844c6bf01829b88b7392cdc330a787a6366c8cec/src/libs/TaskUtils.ts#L50

But the problem here is that lastMessageTextFromReport is calculated in SidebarUtils, and we call it here. The OptionRowLHNData of the DM report doesn't re-render when the task report is changed, which causes this bug.

After the EditTask API is called, only the parent report action of task report is updated then we will not see this bug for shared report.

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

The getLastMessageTextForReport function has some cases in which we will get the data from other reports. It's better if we get this in LHNOptionsList here because we've had dependency for all reports. Follow the pattern we do it here

let lastActorDetails: Partial<PersonalDetails> | null = itemFullReport?.lastActorAccountID && personalDetails?.[itemFullReport.lastActorAccountID] ? personalDetails[itemFullReport.lastActorAccountID] : null;

if (!lastActorDetails && lastReportAction) {
    const lastActorDisplayName = lastReportAction?.person?.[0]?.text;
    lastActorDetails = lastActorDisplayName
        ? {
            displayName: lastActorDisplayName,
            accountID: report.lastActorAccountID,
        }
        : null;
}

const lastMessageTextFromReport = OptionsListUtils.getLastMessageTextForReport(itemFullReport, lastActorDetails, itemPolicy);

https://github.com/Expensify/App/blob/844c6bf01829b88b7392cdc330a787a6366c8cec/src/components/LHNOptionsList/LHNOptionsList.tsx#L114

Then we can pass lastMessageTextFromReport from LHNOptionsList --> OptionRowLHNData --> SidebarUtils and we can remove the logic calculate lastMessageTextFromReport in SidebarUtils since we've passed a param to this function or we can still keep this logic and do this if lastMessageTextFromReport param is undefined.

What alternative solutions did you explore? (Optional)

NA

allgandalf commented 2 months ago

~taking over this issue (slack context here), @zanyrenney can you please assign me here 😸~

Edit, @allroundexperts , is taking this one over (slack)

allroundexperts commented 2 months ago

Thanks for the proposals everyone.

Since the backend updates the report action of the dm task, I think we should do the same in the optimistic response as well. As such, @tsa321's proposal looks good to me.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 2 months ago

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

mkzie2 commented 2 months ago

@allroundexperts I don't think it's a good idea because

  1. Maybe we will have many related reports when we change the assignee many times
  2. Currently, we're using the task report's data to display the message in LHN and do this in TaskPreview. The current problem here is OptionRowLHN isn't re-rendered when the task report is changed. If we offline and go to the DM after editing the task title, the TaskPreview doesn't have this bug because this component is re-rendered when the task report is changed. Of If we force offline and refresh the page after editing the task title, LHN of DM chat will be updated.
tsa321 commented 2 months ago

Proposal

updated @allroundexperts I have updated my proposal to address the issue of LHN data, which needs to depend on the related task report.

mkzie2 commented 2 months ago
  1. Adding the task report as a dependency can cause many unnecessary re-renders for OptionRownLHNData of all parent reports that are related to the task report which is a bad performance for OptionRownLHNData

  2. I'm the first one who has the correct RCA and the solution which is to re-render OptionRownLHNData and re-calculate SidebarUtils.getOptionData

allroundexperts commented 2 months ago

@mkzie2 I think I had different results when testing this offline. When I was offline, no matter how many times I refreshed, the LHN did not update with the correct data. Can you post a video of the data updating after refreshing when offline?

mkzie2 commented 2 months ago

@allroundexperts The video here.

https://github.com/user-attachments/assets/a5958f36-63cb-49b2-95eb-7334ad8cc616

allroundexperts commented 2 months ago

@allroundexperts The video here.

Untitled.mp4

Hm... I see now. I was 'refreshing' the page. Instead, I just moved across the screens.

allroundexperts commented 2 months ago

Given above, I'd agree that @mkzie2's proposal is more accurate. Let's go with them instead.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 2 months ago

Current assignee @cead22 is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] commented 2 months ago

📣 @mkzie2 You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

jayeshmangwani commented 2 months ago

@zanyrenney / @cead22 For this issue, could you please unassign me from C+ and assign the @allroundexperts instead?

melvin-bot[bot] commented 1 month ago

This issue has not been updated in over 15 days. @cead22, @allroundexperts, @zanyrenney, @mkzie2 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!

mkzie2 commented 3 weeks ago

@zanyrenney The PR was deployed to production. Please help to process the payment here.