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

[WAITING ON #40059][$250] Conversation - Deleted message reappears when reopening the conversation #39521

Closed
6 tasks done
lanitochka17 opened this issue Apr 3, 2024 · 29 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause

Comments

@lanitochka17
Copy link

lanitochka17 commented Apr 3, 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: 1.4.59
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
Issue reported by: Applause - Internal Team

Action Performed:

  1. Open the https://staging.new.expensify.com/
    website.
  2. Tap on the chat icon.
  3. Open any conversation.
  4. Long press on any sent message.
  5. Tap on "Delete Comment"
  6. Tap on "Delete"
  7. Tap on the arrow on the top left corner.
  8. Tap on the same conversation again.
  9. Check if the deleted message is still displayed or was succesfully deleted

Expected Result:

Deleted message shouldn´t reappear after closing and reopening the conversation

Actual Result:

Deleted message reappears on the chat after closing it and reopening it

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6434653_1711994994698.Delete.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~011225f8750b0176de
  • Upwork Job ID: 1777801993580064768
  • Last Price Increase: 2024-05-07
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 3, 2024
Copy link

melvin-bot bot commented Apr 3, 2024

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

@shahinyan11
Copy link

shahinyan11 commented Apr 3, 2024

Proposal

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

Conversation - Deleted message reappears when reopening the conversation

What is the root cause of that problem?

It is regression after this changes.

With the current implementation it will only delete the report action if it is a parent action of some thread ( with other words the report action has child report ). Otherwise here we remove DeleteComment from persistedRequests and prevent sending request to the server.

It would be better to know in what cases we should prevent a request from being sent to the server before proposing a solution. But at this stage I will still offer two different options.

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

looking at isDeletedParentAction, I understand that we only want to delete the report action which is parent, but we haven't calculated that if the report activity doesn't have a child report, it won't be a parent. So that we should delete the report action if it is a parent or just has not child report

  1. Define new variable hasThreadMessage in deleteReportComment function
const hasThreadMessage = !!reportAction.childReportID`
  1. Update this condition to !hasThreadMessage || isDeletedParentAction

What alternative solutions did you explore? (Optional)

Remove WRITE_COMMANDS.DELETE_COMMENT from this array or transfer it in above array

Copy link

melvin-bot bot commented Apr 8, 2024

@johncschuster Huh... This is 4 days overdue. Who can take care of this?

@johncschuster johncschuster added the External Added to denote the issue can be worked on by a contributor label Apr 9, 2024
Copy link

melvin-bot bot commented Apr 9, 2024

Job added to Upwork: https://www.upwork.com/jobs/~011225f8750b0176de

@melvin-bot melvin-bot bot changed the title Conversation - Deleted message reappears when reopening the conversation [$250] Conversation - Deleted message reappears when reopening the conversation Apr 9, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 9, 2024
Copy link

melvin-bot bot commented Apr 9, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Apr 9, 2024
@Nghondzweni
Copy link

Contributor details
Your Expensify account email: trnghondzweni@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01d6ad2a62c32371cb

Copy link

melvin-bot bot commented Apr 10, 2024

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@nkdengineer
Copy link
Contributor

nkdengineer commented Apr 10, 2024

Proposal

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

Deleted message reappears on the chat after closing it and reopening it

What is the root cause of that problem?

In here, we always include the current command in the list of conflicting commands, if the report action is not a parent report action.

The intention here was that if the report action is an optimistic one, and we delete the report action, it will clear all the commands related to that report action. The "optimistic" report action was never created so we don't have to create the report action, neither do we need to delete that report action. All we need to do in that case is to clear all the commands (listed here) and also clear the report action in Onyx.

But the problem here is that the report action in our case is not optimistic, it's an already-created report action. We're still omitting the DELETE_COMMENT command for this report action, leading to it not being deleted.

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

We can update this to

shouldIncludeCurrentRequest: !isDeletedParentAction && reportAction.isOptimisticAction,

So it will not include the current delete command in the "conflicting commands" list if the report action is not optimistic. This means an already-created report action will always be deleted properly in the back-end.

What alternative solutions did you explore? (Optional)

Do not include WRITE_COMMANDS.DELETE_COMMENT here if the report action is not an optimistic one.

@melvin-bot melvin-bot bot added the Overdue label Apr 12, 2024
@johncschuster
Copy link
Contributor

@ishpaul777 can you review the above proposals?

@melvin-bot melvin-bot bot removed the Overdue label Apr 12, 2024
@ishpaul777
Copy link
Contributor

ishpaul777 commented Apr 14, 2024

I could not reproduce the issue, but i have a feeling that this PR should handle it #40059, i am subscribed to the PR will retest once its merged

Screen.Recording.2024-04-15.at.12.26.14.AM.mov

Copy link

melvin-bot bot commented Apr 16, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Apr 16, 2024
@johncschuster johncschuster changed the title [$250] Conversation - Deleted message reappears when reopening the conversation [WAITING ON #40059][$250] Conversation - Deleted message reappears when reopening the conversation Apr 16, 2024
@johncschuster
Copy link
Contributor

Thanks for the update, @ishpaul777! @MelvinBot, this isn't overdue. We're waiting on #40059 to get merged.

@melvin-bot melvin-bot bot removed the Overdue label Apr 16, 2024
Copy link

melvin-bot bot commented Apr 17, 2024

@johncschuster @ishpaul777 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!

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@melvin-bot melvin-bot bot added the Overdue label Apr 19, 2024
@ishpaul777
Copy link
Contributor

Seems like another PR might fixed it #40180, I am not able to reproduce as well (i was not even before lol) @johncschuster Can you please retest and close if the issue is fixed.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Apr 19, 2024
Copy link

melvin-bot bot commented Apr 23, 2024

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

Copy link

melvin-bot bot commented Apr 23, 2024

@johncschuster, @ishpaul777 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@ishpaul777
Copy link
Contributor

gentle bump @johncschuster

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Apr 23, 2024
@johncschuster
Copy link
Contributor

Sorry I missed this, @ishpaul777! I'll ask for a retest on this.

@melvin-bot melvin-bot bot removed the Overdue label Apr 26, 2024
@johncschuster johncschuster added the retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause label Apr 26, 2024
@melvin-bot melvin-bot bot added the Overdue label Apr 29, 2024
Copy link

melvin-bot bot commented Apr 30, 2024

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

Copy link

melvin-bot bot commented Apr 30, 2024

@johncschuster, @ishpaul777 Whoops! This issue is 2 days overdue. Let's get this updated quick!

@ishpaul777
Copy link
Contributor

waiting for a retest from QA team

@melvin-bot melvin-bot bot removed the Overdue label Apr 30, 2024
@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

Copy link

melvin-bot bot commented May 1, 2024

@johncschuster @ishpaul777 this issue is now 4 weeks old, please consider:

  • Finding a contributor to fix the bug
  • Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

@melvin-bot melvin-bot bot added the Overdue label May 3, 2024
@ishpaul777
Copy link
Contributor

i think we can close this @johncschuster 😄

@melvin-bot melvin-bot bot removed the Overdue label May 4, 2024
@mvtglobally
Copy link

Issue not reproducible during KI retests. (Third week) Good to close?

@melvin-bot melvin-bot bot added the Overdue label May 6, 2024
Copy link

melvin-bot bot commented May 7, 2024

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

@ishpaul777
Copy link
Contributor

gentle bump @johncschuster : )

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels May 7, 2024
@ishpaul777
Copy link
Contributor

@melvin-bot melvin-bot bot removed the Overdue label May 9, 2024
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. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause
Projects
No open projects
Archived in project
Development

No branches or pull requests

7 participants