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

Delete comment - Deleted comment is not synchronized between other clients #3524

Closed
dklymenk opened this issue Jun 10, 2021 · 13 comments · Fixed by #3734
Closed

Delete comment - Deleted comment is not synchronized between other clients #3524

dklymenk opened this issue Jun 10, 2021 · 13 comments · Fixed by #3734
Assignees

Comments

@dklymenk
Copy link
Contributor

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Expected Result:

Deleting a comment should delete it on other participants' screens too

Actual Result:

Deleting a comment doesn't cause any visible update on any other client with chat open.

Action Performed:

  1. Login into account A on 2 devices. (or tabs/browsers that don't share local storage)
  2. Login into account B on another devices or incognito window.
  3. Send a message from account A to account B.
  4. You will see it on both second screen for account A and screen for account B.
  5. Delete the message you've just sent as account A.
  6. The message will still be on both second screen for account A and screen for account B.

Workaround:

Page refresh doesn't help. Only workaround is deleting the local storage entry for the chat or wiping it completely, forcing it to reload entire chat history.

Platform:

Where is this issue occurring?

Web
iOS
Android
Desktop App
Mobile Web

Version Number: 1.0.65-0
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

new_issue_cut.mp4

Expensify/Expensify Issue URL:

View all open jobs on Upwork

@dklymenk dklymenk added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jun 10, 2021
@MelvinBot
Copy link

Triggered auto assignment to @puneetlath (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jun 10, 2021
@dklymenk
Copy link
Contributor Author

Sadly, I do not have a proposal for this issue, since I believe it is caused by a response from BE.

Here are my findings that lead me to believe it's a BE issue.

The main reason the message is not updated is because sequenceNumber passed to updateReportActionMessage function is always equal to -1. This causes the function to create new reportAction with key -1 instead of updating text for deleted message. Please note that this is not the case for edited messages where sequenceNumber is correct. Issue occurs only when edited text message is empty (aka deleted message).

To make sure the -1 comes from BE, I checked the WS messages with dev tools. Here is an example message with an invalid sequenceNumber:
DeepinScreenshot_select-area_20210610231859

I hope it helps.

@puneetlath
Copy link
Contributor

Nice find @dklymenk! I was able to reproduce this as well.

@puneetlath puneetlath removed their assignment Jun 11, 2021
@MelvinBot
Copy link

Triggered auto assignment to @Dal-Papa (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@puneetlath puneetlath added the Daily KSv2 label Jun 11, 2021
@Dal-Papa Dal-Papa added External Added to denote the issue can be worked on by a contributor Weekly KSv2 labels Jun 14, 2021
@Dal-Papa Dal-Papa removed the Daily KSv2 label Jun 14, 2021
@MelvinBot
Copy link

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

@MelvinBot MelvinBot added Daily KSv2 and removed Weekly KSv2 labels Jun 14, 2021
@dklymenk
Copy link
Contributor Author

dklymenk commented Jun 14, 2021

Is BE repo available for upwork contractors? Why the External label?

@Dal-Papa
Copy link

@dklymenk : I think this could be decoupled between some backend features missing and then hooking it into the frontend. I will circle back once I have discussed with other engineers.

@puneetlath
Copy link
Contributor

Removing the external label for now. @Dal-Papa feel free to re-add it when you're ready.

@puneetlath puneetlath removed their assignment Jun 18, 2021
@puneetlath puneetlath removed the External Added to denote the issue can be worked on by a contributor label Jun 18, 2021
@Dal-Papa
Copy link

Sounds like we're gonna need to have some Pusher events fired off when deletion/edition happens so I'm going to create an internal GH first and keep that one open for when that's done and we can start listening to said event.

@dklymenk
Copy link
Contributor Author

There is already a pusher event and it works for edition flawlessly. The issue is only with deletion and the fact that BE returns -1 instead of the actual sequenceNumber. Please my first comment #3524 (comment)

@dklymenk
Copy link
Contributor Author

@Dal-Papa the way I see it, if BE starts returning proper sequenceNumber values, the issue on FE side will be automatically fixed.

@dklymenk
Copy link
Contributor Author

I'm sorry, but was the issue on FE all along? Or were some changes required on BE too?

@Dal-Papa
Copy link

I'm sorry, but was the issue on FE all along? Or were some changes required on BE too?

No changes were made on the BE, it was simply the FE not sending the sequenceNumber to the BE when deleting a message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants