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

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

Open
3 of 6 tasks
IuliiaHerets opened this issue Aug 19, 2024 · 23 comments
Open
3 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Aug 19, 2024

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:

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6576273_1724088874978.Subt_edit.mp4

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
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 19, 2024
Copy link

melvin-bot bot commented Aug 19, 2024

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
Copy link
Author

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

@IuliiaHerets
Copy link
Author

@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
Copy link
Contributor

tsa321 commented Aug 20, 2024

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:

}, [
fullReport,
lastReportActionTransaction,
reportActions,
personalDetails,
preferredLocale,
policy,
parentReportAction,
iouReportReportActions,
transaction,
transactionViolations,
receiptTransactions,
invoiceReceiverPolicy,
shouldDisplayReportViolations,
]);

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:

    const itemIouReportReportActions = iouReportIDOfLastAction ? reportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${iouReportIDOfLastAction}`] : undefined;

    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:

    }, [
    fullReport,
    lastReportActionTransaction,
    reportActions,
    personalDetails,
    preferredLocale,
    policy,
    parentReportAction,
    iouReportReportActions,
    transaction,
    transactionViolations,
    receiptTransactions,
    invoiceReceiverPolicy,
    shouldDisplayReportViolations,
    ]);

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:
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:

const optimisticData: OnyxUpdate[] = [

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Aug 21, 2024
@zanyrenney
Copy link
Contributor

nice! looking at external label noq

@melvin-bot melvin-bot bot removed the Overdue label Aug 22, 2024
@zanyrenney zanyrenney added External Added to denote the issue can be worked on by a contributor Overdue labels Aug 22, 2024
@melvin-bot melvin-bot bot changed the title Task – Edited subtask name not updated in chat with user B in LHN [$250] Task – Edited subtask name not updated in chat with user B in LHN Aug 22, 2024
Copy link

melvin-bot bot commented Aug 22, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 22, 2024
Copy link

melvin-bot bot commented Aug 22, 2024

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

@mkzie2
Copy link
Contributor

mkzie2 commented Aug 23, 2024

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

const lastMessageTextFromReport = OptionsListUtils.getLastMessageTextForReport(report, lastActorDetails, policy);

} else if (ReportActionUtils.isCreatedTaskReportAction(lastReportAction)) {
lastMessageTextFromReport = TaskUtils.getTaskCreatedMessage(lastReportAction);
} else if (lastReportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.SUBMITTED) {
lastMessageTextFromReport = ReportUtils.getIOUSubmittedMessage(lastReportAction);

const taskReport = ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${taskReportID}`] ?? {};

const taskTitle = getTaskTitle(taskReportID, reportAction?.childReportName);

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

  • visibleReportActionItems[report.reportID] is lastReportAction here because we've sorted the display report action

  • report is itemFullReport

  • policy is itemPolicy

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);

const renderItem = useCallback(

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
Copy link
Contributor

allgandalf commented Aug 25, 2024

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

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

@melvin-bot melvin-bot bot added the Overdue label Aug 25, 2024
@allroundexperts
Copy link
Contributor

allroundexperts commented Aug 25, 2024

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

Copy link

melvin-bot bot commented Aug 25, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Aug 25, 2024
@mkzie2
Copy link
Contributor

mkzie2 commented Aug 26, 2024

@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
Copy link
Contributor

tsa321 commented Aug 26, 2024

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
Copy link
Contributor

mkzie2 commented Aug 26, 2024

  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
Copy link
Contributor

@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
Copy link
Contributor

mkzie2 commented Aug 26, 2024

@allroundexperts The video here.

Untitled.mp4

@allroundexperts
Copy link
Contributor

@allroundexperts The video here.

Untitled.mp4

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

@allroundexperts
Copy link
Contributor

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

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Aug 27, 2024

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

@melvin-bot melvin-bot bot added Overdue and removed Help Wanted Apply this label when an issue is open to proposals by contributors labels Aug 28, 2024
Copy link

melvin-bot bot commented Aug 28, 2024

📣 @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
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label Aug 28, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 29, 2024
@melvin-bot melvin-bot bot removed the Weekly KSv2 label Sep 23, 2024
Copy link

melvin-bot bot commented Sep 23, 2024

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!

@melvin-bot melvin-bot bot added the Monthly KSv2 label Sep 23, 2024
@mkzie2
Copy link
Contributor

mkzie2 commented Oct 1, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

8 participants