search

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.34k stars 2.77k forks source link

[HOLD for payment 2023-07-05] [$1000] Deleting thread replies showing thread title for a while #19352

Closed kavimuru closed 1 year ago

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

  1. Open a chat
  2. Send a message to the user
  3. Clicked on "Reply in thread" and reply 2,3 on the thread
  4. Edit Thread title/message
  5. Now delete the last thread reply one by one

Expected Result:

Deleting thread replies message should not displayed thread title for a while

Actual Result:

Deleting thread replies showing thread title for a while

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

Version Number: 1.3.16.5 Reproducible in staging?: y Reproducible in production?: y 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/43996225/4c858c64-9518-44d3-b27f-f6e21c02c741

https://github.com/Expensify/App/assets/43996225/ea3ed220-04c9-49a5-bb47-c09ccb9d1c31

Expensify/Expensify Issue URL: Issue reported by: @ayazhussain79 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1684358266982189

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0134fe88a3ecfbf83e
  • Upwork Job ID: 1661107080114233344
  • Last Price Increase: 2023-06-06
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @miljakljajic (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)

allroundexperts commented 1 year ago

Proposal

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

Deleting thread replies showing thread title for a while

What is the root cause of that problem?

The issue only happens, when you edit the thread title and then try to delete the last message.

The root cause of this issue is that when editing thread titles, we're editing the thread report itself. This causes an incomplete action (which does not have a timestamp and other properties and has message only) to be created in the thread report.

Screenshot 2023-05-21 at 3 30 27 PM

After this, when the last message is deleted in a thread report, the new last message is calculated using getLastVisibleAction function which returns an incorrect value here because one of the action is incomplete ie it does not have a timestamp.

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

We can replace this with:

// parent report can be fetched from onyx storage using props.parentReportID prop
 report={props.parentReport}

However, doing this will cause the following to show up in the thread:

Screenshot 2023-05-21 at 4 23 32 PM

To avoid this, we can add a new prop called displayThreadReplies in ReportActionItem component (such that thread replies are not shown if the prop is set as false) and set its value to false here.

If above is not an option, then we can also pass the parent report id and parent report action to the editReportComment function here IF thread title is being edited. This will ensure that edits are being done to the parent report instead of creating an incomplete action in the thread report. The editReportComment function is being called here. We can check here if the report action is a thread parent. If it is, then we can pass parent params to the editReportComment function.

Result

https://github.com/Expensify/App/assets/30054992/6768cef3-965e-4be8-8e45-7cead585cd39

What alternative solutions did you explore? (Optional)

In the editReportComment, we first have to check if the given originalReportAction is isThreadParent. We also have to get the parentReportId and parentReportActionId ie:

    const isThreadParent = ReportUtils.isThreadParent(originalReportAction);
    const {parentReportID: threadParentReportId, parentReportActionID: threadParentReportActionId} = allReports[reportID];

If the originalReportAction is a thread parent, then in all the subsequent optimistic actions and api calls, we have to use parentReportId instead of the reportID and parentReportActionId instead of the reportActionId. Also, we have to update the thread report name, if the message being edited is thread parent. ie:

if (isThreadParent) {
        optimisticData.push({
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
            value: {
                displayName: textForNewComment,
            },
        });
    }
miljakljajic commented 1 year ago

I am OOO so I will re-assign this

miljakljajic commented 1 year ago

Apologies for the delay in reviewing, I've been at EC3.

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @abekkala (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)

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Current assignee @abekkala 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 - @0xmiroslav (External)

melvin-bot[bot] commented 1 year ago

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

strepanier03 commented 1 year ago

@techievivek and @0xmiroslav - Can you both check these other GH (https://github.com/Expensify/App/issues/19352 and https://github.com/Expensify/App/issues/19348) and see if you agree that all three GHs have the same root cause?

If they have the same root cause then should we merge these three into one GH?

@allroundexperts has posted a proposal that solves all three in one.

Context here for Vivek.

abekkala commented 1 year ago

@techievivek and @0xmiroslav just a friendly bump in regards to Sheena's message above!

Can you address that please?

techievivek commented 1 year ago

Sorry for the delay in reviewing(I was ooo in the last few days), the root cause for both issues appears to be similar and happens only when the thread title gets updated( if we look at the above proposal @allroundexperts has mentioned that this happens only when we first update the thread report title and then make changes to the thread messages). And the fix for both issues also seems to be similar so I suggest we merge this with the other issue.

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? 💸

techievivek commented 1 year ago

Not overdue. @strepanier03 @abekkala I have shared my thoughts above in the comment, and I think we should just merge all the issue into one.

melvin-bot[bot] commented 1 year ago

@abekkala @techievivek @0xmiroslav 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!

abekkala commented 1 year ago

Yeah, let's get these merged. @strepanier03 you mentioned

should we merge these three into one GH?

But only listed two: this one (https://github.com/Expensify/App/issues/19352) and then yours #https://github.com/Expensify/App/issues/19348

Is there another one? Should we just use the first one created for moving forward?

abdulrahuman5196 commented 1 year ago

@abekkala I think we can use https://github.com/Expensify/App/issues/19348 (I think its the first one). We also completed C+ review there - https://github.com/Expensify/App/issues/19348

abekkala commented 1 year ago

Yes, that one is the first one so we can use that one moving forward.

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? 💸

Beamanator commented 1 year ago

I hired @allroundexperts in https://github.com/Expensify/App/issues/19348, so I think we can go ahead and close this one out

melvin-bot[bot] commented 1 year ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.33-4 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-07-05. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

melvin-bot[bot] commented 1 year ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed: