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

IOU - All the cancelled request preview shows loading spinner for reopened accounts - reported by @Tushu17 #7420

Closed kavimuru closed 1 year ago

kavimuru commented 2 years 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:

Precondition: Closed an account then reopen again

  1. Launch the app
  2. Login with reopened account credentials
  3. Open Direct message with account who I canceled request money.

Expected Result:

All canceled request money should appear without any issues

Actual Result:

All canceled request money shows spinner.

Workaround:

Unknown

Platform:

Where is this issue occurring?

Version Number: 1.1.33-2 Reproducible in staging?: Yes Reproducible in production?: Yes Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/151190208-15448c72-dbbc-4a8a-8925-f78b3d760c7b.mp4

Expensify/Expensify Issue URL: Issue reported by: Slack conversation:

View all open jobs on GitHub

MelvinBot commented 2 years ago

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

MelvinBot commented 2 years ago

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

MitchExpensify commented 2 years ago

Upwork Job

MelvinBot commented 2 years ago

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

MelvinBot commented 2 years ago

Current assignee @stitesExpensify is eligible for the Exported assigner, not assigning anyone new.

railway17 commented 2 years ago

Could you attach reproduce video again? I can't replicate on my side. I tested the latest main branch.

rushatgabhane commented 2 years ago

@railway17 are you able to replicate this on production?

railway17 commented 2 years ago

@rushatgabhane I didn't test in production. But is it possible to test in product with my personal account?

I've contributed to 1 issue in Expensify, but not sure I can test in prod or not.... In the issue replication, I am not clear what reopened account is.

Just tried the below steps: login, request, cancel, logout, re-login, open direct chat. But can't replicate. My replication flow is correct?

rushatgabhane commented 2 years ago

@railway17 Thanks for the details! For reopening the account, you first need to close it.

To close: settings -> security -> close account. To reopen: just sign in to the same account

railway17 commented 2 years ago

https://user-images.githubusercontent.com/29966461/151646629-762047e7-bd76-4af8-9763-76702acf3aa4.mp4

@rushatgabhane I can't see any option to close the account.

rushatgabhane commented 2 years ago

@railway17 can you try it in production (new.expensify.com) I'm not sure why you can't see close account page as it isn't hidden in any beta.

railway17 commented 2 years ago

@rushatgabhane Ok, I can see and replicate it in production. Looks like latest main is not same with current prod. I can't see Close Account in en.js Let me try to find which commit is current prod

rushatgabhane commented 2 years ago

@railway17 main has close account feature. you might need to sync your fork if you're testing on it 😄

railway17 commented 2 years ago

@rushatgabhane I have another question. Let's assume that this issue is fixed and there was $500 requested money before closing the account. When login reopened account, this amount should be kept?

The current action result is that old money is reset to 0 when login and requesting another money. But Expected result doesn't mention about keeping old total requested money

stitesExpensify commented 2 years ago

FYI @railway17 @rushatgabhane is OOO at the moment, i'm sure he'll respond when he's back

rushatgabhane commented 2 years ago

@railway17 woah, sorry for being so late to respond.

Let's assume that this issue is fixed and there was $500 requested money before closing the account. When login reopened account, this amount should be kept?

No, the amount shouldn't be kept because all expense data is deleted by expensify (as per the email received on deletion).

rushatgabhane commented 2 years ago

@stitesExpensify I think this is an issue that should be fixed on the backend.

rushatgabhane commented 2 years ago

I think this is an issue that should be fixed on the backend.

@stitesExpensify gentle bump ^^

stitesExpensify commented 2 years ago

Sorry for the late response. Just to make sure I'm following @rushatgabhane, are you suggesting that we should just delete all pending money requests at the time an account is closed?

rushatgabhane commented 2 years ago

@stitesExpensify really sorry for the late reply, I missed this one.

My assumption: when an account is closed, all IOU requests (cancelled or not cancelled) are deleted on the server side. Which explains why the IOU preview returns 404 for a reopened account.

This image backs my assumption.

image

Only the report (chat) history is preserved for a deleted account.

Solution

One solution could be to remove IOU previews from all reports of the deleted account, which ofc should be done on the backend. I hope I'm making sense.


https://user-images.githubusercontent.com/29673073/154583699-52fe267d-1151-4020-af8f-ea980ff8d2bf.mp4

Tushu17 commented 2 years ago

I think #7435 and this is the same issue so Ig we should close #7435. Quick question:- #7435 was reported in slack before this issue but took time to get logged so is this eligible for reporting bonus (https://expensify.slack.com/archives/C01GTK53T8Q/p1642285631499100) cc: @stitesExpensify

stitesExpensify commented 2 years ago

Hmm @Tushu17 is this the same issue? In your case it looks like you can still see the IOU, whereas here it is an endless spinner. Is it just because you hadn't refreshed maybe?

Tushu17 commented 2 years ago

@stitesExpensify Yeah it's same but there is a little difference about loading spinner. To repro loading spinner case, You need to delete your account and have to reopen it. But when the other user request money and deletes his account loading spinner doesn't show up. the similar thing between both is that in both you get error message when trying to see the details and I guess solving one will also solve the other.

stitesExpensify commented 2 years ago

Gotcha. In that case yes, I think that you would qualify for the reporting bonus

stitesExpensify commented 2 years ago

I also agree with @rushatgabhane that this is an internal issue, so I'm removing the help wanted and external labels

MitchExpensify commented 2 years ago

Going to remove my assignment in that case too @stitesExpensify

stitesExpensify commented 2 years ago

No update due to OOO

stitesExpensify commented 2 years ago

Started a slack conversation about this issue, apparently the requests should not be deleted when you delete the account so this is actually a different bug than we originally thought. Looking into it more this week

stitesExpensify commented 2 years ago

Okay so when you close an account we retract all of your processing reports here and then when we get to auth we delete all open, non-processing reports here. To me this seems like the expected behavior. Is that not the case @iwiznia ?

iwiznia commented 2 years ago

Well, it is the expected behavior for oldDot, but causes problems in newDot, so maybe we need to change it, because reports in newDot are never in closed state and processing is a totally valid "final" state for a report.

stitesExpensify commented 2 years ago

I think if you delete your account with a money request still pending then it makes total sense that we would retract that request. Am I missing a different case?

iwiznia commented 2 years ago

If we do that, what happens to the workspace chat and all the things pointing to the now non existent reports? Also, what happens for credit card requests that will never be deleted and will never be approved (ie: their final state would be processing)?

stitesExpensify commented 2 years ago

I was thinking that we should make a new report action that shows the request is "cancelled" and then we can show it in the UI.

what happens for credit card requests that will never be deleted and will never be approved

I don't know what that is tbh. What is a credit card request and how is it different from a normal "Request money"?

iwiznia commented 2 years ago

A credit card request is a transaction done with the expensify credit card. Since those don't need to be paid and for now there are no approvals, the state of the report when they are done with them will still be processing.

stitesExpensify commented 2 years ago

Gotcha. So should we just not retract/close any processing IOU reports then?

iwiznia commented 2 years ago

I don't know.

stitesExpensify commented 2 years ago

No update

michaelhaxhiu commented 2 years ago

Fixing some labels are the GH history indicates this is an Internal issue.

melvin-bot[bot] commented 2 years ago

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

melvin-bot[bot] commented 2 years ago

Current assignee @stitesExpensify is eligible for the Exported assigner, not assigning anyone new.

michaelhaxhiu commented 2 years ago

Ugh... sorry I removed that Exported label by mistake and re-added it. We should keep the exported label so it's clear that we can pay the reporting bonus to @Tushu17 if it's fixed.

parasharrajat commented 2 years ago

Unassigned as this is internal.

stitesExpensify commented 2 years ago

Looking into this today

stitesExpensify commented 2 years ago

I got distracted with this last week. Will be looking at it tomorrow

melvin-bot[bot] commented 2 years ago

A Contributor Manager will be assigned to issue payment via Upwork if we deploy an associated Pull Request to production. Per Contributing.md.

stitesExpensify commented 2 years ago

I made a test PR that doesn't retract IOU reports that are processing, and that fixed the issue but IDK if that's really the best solution. Have been working on offline first so this is not a priority for me at the moment.

stitesExpensify commented 2 years ago

Still not a priority at the moment

stitesExpensify commented 2 years ago

I still haven't worked on this. Making it a monthly since it's an edge case involving closing and re-opening an account

stitesExpensify commented 2 years ago

No update, focused on n7

stitesExpensify commented 2 years ago

No update, focused on n7