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.49k stars 2.85k forks source link

[HOLD for payment 2023-06-29] [$1000] Delete popup is stuck and screen blinks for few seconds when user tries to delete a message and user gets removed from workspace at that instance #17846

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 the app on any device and login with user A
  2. Open the app on any other device and login with user B (ensure that user A is present in one of the workspace to which user B is admin)
  3. On user A device, open workspace to which user B is admin
  4. On user A device, right click or long press on any of the self message and click on delete message to open delete confirmation popup
  5. From user B device, remove user A from that workspace
  6. Observe on user A device, when user B removes user A from workspace, chat blinks by showing not access page, chat page and again not access page and also the delete popup is still open

Expected Result:

App should also close all the popups open on user A device when user B remove user A from workspace. App should also not blink the #announce chat for user A when user B removes user A from workspace

Actual Result:

App keeps the delete popup open even though user A is no longer able to able that chat. App also blinks the chat few times before displaying 'You don't have access' screen

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.4 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://user-images.githubusercontent.com/43996225/233814365-a544b62f-e828-4ca2-afa5-fe2d01c44548.mp4

https://user-images.githubusercontent.com/43996225/233814372-d03315b4-61d8-4f05-b278-1bb3f9463f98.mp4

Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1682176575452809

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~019ed03865c0010a59
  • Upwork Job ID: 1651681139754299392
  • Last Price Increase: 2023-06-09
MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

alexpensify commented 1 year ago

I'll try to test soon.

MelvinBot commented 1 year ago

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

alexpensify commented 1 year ago

This one is still on my radar to test.

alexpensify commented 1 year ago

I'm able to replicate

2023-04-26_15-08-22

2023-04-26_15-08-40

MelvinBot commented 1 year ago

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

alexpensify commented 1 year ago

@bondydaa - I think we take this to the room to identify how it should function but let me know if you have ideas then we assign it to External. Thanks!

bondydaa commented 1 year ago

Hmm I don't see why engineering label was added here, this should be fine for a contributor to work on so adding external label. Me asking in the room for ideas is the same as posting the external label no?

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~019ed03865c0010a59

MelvinBot commented 1 year ago

Current assignee @alexpensify is eligible for the External assigner, not assigning anyone new.

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

Current assignee @bondydaa is eligible for the External assigner, not assigning anyone new.

manish-tapadar-18 commented 1 year ago

@bondydaa If you are using websocket or any real time mechanism then easily you can solve this problem. When someone remove a user from workspace we can easily send a real time packet and close the modal or popup.

MelvinBot commented 1 year ago

📣 @manish-tapadar-18! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Screen Shot 2022-11-16 at 4 42 54 PM

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
bondydaa commented 1 year ago

Yep we are using web-socket via a service called Pusher which is how the report is able to get updated and show "not available" anymore. I think we just need to also dismiss the modal component somehow which I didn't dig into.

If you haven't already checkout https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md for how we typically expect proposals to come in (https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md#propose-a-solution-for-the-job)

chafikchaban commented 1 year ago

Proposal

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

ActionContext menu delete modal is not closed when user gets removed from workspace. ( same goes for the context menu which IMO should also be closed in this case )

What is the root cause of that problem?

the ReportActionContextMenu is not aware of the changes, thus it keeps its states ( open context menu or delete modal )

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

when a report update is received, in the case of a null report, invoke hideDeleteModal and hideContextMenu from ReportActionContextMenu in Report.js#L688

results:

https://user-images.githubusercontent.com/30408070/235005953-003766c6-27bb-4a11-bc24-d4154363055d.mov

https://user-images.githubusercontent.com/30408070/235005975-e4efb24c-9308-4c89-95af-419129e4c97c.mov

What alternative solutions did you explore? (Optional)

no better solution i can think of

MelvinBot commented 1 year ago

📣 @chafikchaban! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Screen Shot 2022-11-16 at 4 42 54 PM

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
chafikchaban commented 1 year ago

Contributor details Your Expensify account email: chafikchaaben@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01568ab50c600ff939

MelvinBot commented 1 year ago

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

hellohublot commented 1 year ago

Proposal

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

Delete popup is stuck and screen blinks for few seconds when user tries to delete a message and user gets removed from workspace at that instance

What is the root cause of that problem?

PopoverReportActionContextMenu is a global component, it will not be automatically hidden when the ReportActionItem is deleted

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

When I tried to switch the Report by the browser's back button, I found that the [PopoverReportActionContextMenu.PopoverWithMeasuredContent, PopoverReportActionContextMenu.ConfirmModal] was not hidden either

So I think we should show and hide ReportActionContextMenu at the same place, so I think we can judge ReportActionContextMenu.isActiveReportAction(this.props.action.reportActionID) in ReportActionItem.useEffect(() => { return () => { } }, []), and execute ReportActionContextMenu.hideContextMenu and ReportActionContextMenu.hideDeleteModal

I think we should also call hideContextMenu in NavigationRoot.parseAndLogRoute, but this is optional

bondydaa commented 1 year ago

Thanks for the proposals!

@chafikchaban I think with your proposal the context menu / delete modal would clear any time a report was deleted, not just the report I was trying to edit, right? Like if I have reportID 1 open, go to delete a message then I'm removed from reportID 2 by a workspace admin even though reportID 1 was open for me once onyx got the update for the reportID 2 we'd call to close the context menu?

@hellohublot I think your proposal is on the right track, though we are in the process of updating all of our components to use react hooks so we wouldn't want to add a new lifecycle method.

The PR to convert ReportActionItem is here https://github.com/Expensify/App/pull/16635/, I'm wondering if maybe we hold this issue until that is merged to avoid a potentially nasty conflict, cc @jasperhuangg @marcaaron thoughts?

@0xmiroslav what do you think?

bondydaa commented 1 year ago

bump @0xmiroslav @jasperhuangg @marcaaron thoughts on whether we should hold this until the ReportActionItem refactor is done https://github.com/Expensify/App/pull/16635? Not that it's going to fix this bug but since we will most likely need to tap into some of the new hooks it's probably better to wait til that is done.

jasperhuangg commented 1 year ago

@bondydaa Thanks for the bump. I was holding off on completing the refactor since it wasn't a high enough priority item, but it sounds to me like this is cause to get it done now. I'll push it forward today

marcaaron commented 1 year ago

Seems like we are pretty close to merging that other one so let's get it over the finish line then wrap this up!

bondydaa commented 1 year ago

okay going to throw this on hold then until that PR is merged then.

0xmiros commented 1 year ago

Refactoring to functional component PR is still in review. Once merged, I will post here to remove hold. However, there are no satisfactory proposals yet. Existing 2 proposals don't address both issues (stuck, blink) stated in issue title & description.

hellohublot commented 1 year ago

@bondydaa @0xmiroslav Thanks, I guess my proposal was able to solve the delete box stucks, so I continued to investigate how to solve screen blinks

After deleting a person from a room, the interface display sequence: ServerOnyx.update({ report_xxx: null }) -> FullPageNotFoundView -> fetchReportIfNeeded -> ReportActionsSkeletonView -> FullPageNotFoundView

So I tried to add if (routeReportID === prevProps.report.reportID && !onyxReportID) { return } in ReportScreen.componentDidUpdate, but this will not work, because we ignore not only when it is deleted by the server, but also when using deeplink, The onyxReportID will be set to empty for a short time

so I think the following video may not have a perfect solution. If it can be solved, it must have been solved long ago, or we can separate it into a separate issue, because it is not related to delete box already

https://user-images.githubusercontent.com/20135674/235612710-a89640ba-9bc4-4618-be17-9d872c341a3b.mov

alexpensify commented 1 year ago

Is there any feedback for @0xmiroslav following their latest tests? Thanks!

alexpensify commented 1 year ago

Bumping this one again since we need proposal feedback. Thanks!

0xmiros commented 1 year ago

As this is on hold, I will evaluate @hellohublot's last comment after refactor PR is merged. It just got ready for final review.

alexpensify commented 1 year ago

Whoops, sorry I missed that this is on hold but thank you for the update.

alexpensify commented 1 year ago

I don't think the PR has been merged yet.

alexpensify commented 1 year ago

The PR we are waiting on is still under review.

alexpensify commented 1 year ago

Still on hold

alexpensify commented 1 year ago

Still on hold due to this PR - https://github.com/Expensify/App/pull/18809

@bondydaa this appears to be taking longer. Should we move this one to a weekly?

alexpensify commented 1 year ago

@dylanexpensify - This one is on hold, assigning a BZ team member to update it when the HOLD is over. Thanks!

bondydaa commented 1 year ago

ya sure 👍

dylanexpensify commented 1 year ago

Any update here, @bondydaa? 🙇‍♂️

melvin-bot[bot] commented 1 year ago

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

dylanexpensify commented 1 year ago

I'm heading OOO tomorrow for about a week so adding another BZ member while I'm away to help keep the train moving! 🚂 Thanks @bfitzexpensify!!

bfitzexpensify commented 1 year ago

https://github.com/Expensify/App/pull/18809 has been deployed to staging — I'll message again once it hits prod, and then @0xmiroslav you can review @hellohublot's proposal

alexpensify commented 1 year ago

@bfitzexpensify - I appreciate your help here! I'm back online and removing your assignment.

alexpensify commented 1 year ago

https://github.com/Expensify/App/pull/18809 hit production! @bondydaa are we clear to remove the hold?

0xmiros commented 1 year ago

yes, let's remove hold!

melvin-bot[bot] commented 1 year ago

⚠️ This issue has had its price increased by 4x or more. Please review the issue and ensure the price is correct.

melvin-bot[bot] commented 1 year ago

Upwork job price has been updated to $1000

bondydaa commented 1 year ago

okay off hold.

the ReportActionItem component is now using hooks and has the useEffect hook already https://github.com/Expensify/App/blob/77c9d55879be3d4fb86118af6a2909881a05fe3d/src/pages/home/report/ReportActionItem.js#L112-L121

so my hunch is we might need to tap into that if possible here but I would suggest any previous proposals take a look at the updated component before resubmitting https://github.com/Expensify/App/pull/18809/

alexpensify commented 1 year ago

Waiting for proposals

alexpensify commented 1 year ago

Waiting for proposals