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

[HOLD for payment on July 7] Web - Chat - Message not deleted when selecting delete option from the context menu #3767

Closed
kavimuru opened this issue Jun 25, 2021 · 13 comments · Fixed by #3803
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@kavimuru
Copy link

kavimuru commented Jun 25, 2021

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

Upwork link: https://www.upwork.com/jobs/~01ea79ee49be020d89


Action Performed:

  1. Open e.cash in Web
  2. Navigate to a conversation
  3. Right click on any own message
  4. Choose Delete Comment option

Expected Result:

The comment is deleted when you click tab "Delete comment"

Actual Result:

The delete function does not work and message not deleted.

Workaround:

Unknown

Platform:

Where is this issue occurring?

Web ✔️
iOS
Android
Desktop App
Mobile Web

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

Bug5128072_Gravar__12.mp4

Expensify/Expensify Issue URL:

View all open jobs on Upwork

@MelvinBot
Copy link

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

@NikkiWines
Copy link
Contributor

I'm not able to reproduce this on web on 1.0.74-0 - @kavimuru would you mind retesting this to confirm?

Screen.Recording.2021-06-25.at.4.51.15.PM.mov

@isagoico
Copy link

@NikkiWines I thought the same and noticed this is only reproducible when using the right click menu. The hover menu works as expected.

@NikkiWines
Copy link
Contributor

@isagoico nice catch! I didn't read those reproduction steps closely enough it seems. Can confirm there is an issue when using the right-click menu

Screen.Recording.2021-06-28.at.10.37.16.AM.mov

Adding the External label since this would be a good one for someone from UpWork to work on

@NikkiWines NikkiWines added Weekly KSv2 Daily KSv2 External Added to denote the issue can be worked on by a contributor and removed Daily KSv2 labels Jun 28, 2021
@MelvinBot
Copy link

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

@MelvinBot MelvinBot added Daily KSv2 and removed Weekly KSv2 labels Jun 28, 2021
@NikkiWines NikkiWines removed their assignment Jun 28, 2021
@parasharrajat
Copy link
Member

parasharrajat commented Jun 28, 2021

Proposal

So this one is interesting. Actually it is caused by one of my changes here #3515 but those changes are needed. As ReportActionContextMenu is now closed on clicking outside, so it cause the delete Modal to be closed immediately if click anywhere. Though it seems that we are clicking on the button but before that event is processed, the Wrapping ReportActionContextMenu will be closed. Due to the reason that ConfirmModal is contained in ReportActionContextMenu, it results in both being closed. And no delete action will be fired.

To fix this we too options.

  1. We prevent click event from being propagated to the ReportActionContextMenu close event.Thus it will work fine.
  2. we take out the ConfirmModal for delete action outside the ReportActionContextMenu In reportActionItem, now we close the ReportActionContextMenu when ConfirmModal is opened and there won't be any issues. (Preferred).

@roryabraham
Copy link
Contributor

So just to clarify my understanding of this issue:

  1. The ReportActionContextMenu needs to be closed on outside click.
  2. If you click on the ConfirmModal that appears for the delete action, you will have clicked outside the ReportActionContextMenu
  3. Thus, the ReportActionContextMenu is closed. Since the ConfirmModal is a child of the ReportActionContextMenu, the ConfirmModal is also closed.
  4. (the only part I'm fuzzy on) For some reason, no delete action is fired when the ConfirmModal is closed.

And your proposals for potential solutions are:

  1. Prevent the click in the ConfirmModal being bubbled to its parent, the ReportActionContextMenu. Thus, prevent the outside click from closing the ReportActionContextMenu.
  2. Lift the ConfirmModal higher up in the component tree such that it's not a child of the ReportActionContextMenu. Open only open it once the ReportActionContextMenu is closed.

Can you confirm that, @parasharrajat ?

If so, I agree that 2 is better - it's probably a bad practice to allow two non-full-screen modals to be open at a time, or to nest modals in general. It also might be more efficient to have the single modal higher up in the component tree that can be reused and recycled for many different ReportActionContextMenu components.

@parasharrajat
Copy link
Member

parasharrajat commented Jun 28, 2021

Yeah, I confirm @roryabraham. We should lift up the ConfirmModal to the parent component and maybe we can make it singleton for all actions.

@roryabraham
Copy link
Contributor

Cool, I don't think this needs to be a deploy blocker, but @parasharrajat do you have the bandwidth to work on this?

@parasharrajat
Copy link
Member

Yup, I have partial solution ready.

@Jag96
Copy link
Contributor

Jag96 commented Jun 29, 2021

Posted to Upwork and invited @parasharrajat to the job!

@Jag96 Jag96 changed the title Web - Chat - Message not deleted when selecting delete option from the context menu [HOLD for payment on July 7] Web - Chat - Message not deleted when selecting delete option from the context menu Jul 1, 2021
@Jag96
Copy link
Contributor

Jag96 commented Jul 1, 2021

Reopening to keep track of payment date

@Jag96 Jag96 reopened this Jul 1, 2021
@Jag96 Jag96 added the Reviewing Has a PR in review label Jul 1, 2021
@Jag96
Copy link
Contributor

Jag96 commented Jul 7, 2021

Paid!

@Jag96 Jag96 closed this as completed Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants