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

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

Closed
kavimuru opened this issue Jan 26, 2022 · 81 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@kavimuru
Copy link

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?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

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:

Bug5423926_Canceled_2601.mp4

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

View all open jobs on GitHub

@MelvinBot
Copy link

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

@stitesExpensify stitesExpensify added the External Added to denote the issue can be worked on by a contributor label Jan 26, 2022
@MelvinBot
Copy link

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

@MitchExpensify
Copy link
Contributor

Upwork Job

@MelvinBot
Copy link

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

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 26, 2022
@MelvinBot
Copy link

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

@railway17
Copy link
Contributor

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

@rushatgabhane
Copy link
Member

@railway17 are you able to replicate this on production?

@railway17
Copy link
Contributor

@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
Copy link
Member

@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
Copy link
Contributor

20220129120409526.mp4

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

@rushatgabhane
Copy link
Member

rushatgabhane commented Jan 29, 2022

@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
Copy link
Contributor

@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
Copy link
Member

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

@railway17
Copy link
Contributor

railway17 commented Jan 29, 2022

@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
Copy link
Contributor

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

@MelvinBot MelvinBot removed the Overdue label Feb 8, 2022
@rushatgabhane
Copy link
Member

@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
Copy link
Member

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

@marcaaron
Copy link
Contributor

I'm also confused about the bug...

Precondition: Closed an account then reopen again
Launch the app
Login with reopened account credentials
Open Direct message with account who I canceled request money.

Are we canceling the money request and then closing the account or closing an account that has active requests?

@stitesExpensify
Copy link
Contributor

stitesExpensify commented Nov 4, 2022

Cancel and then close the account, although I would guess that other cases also exist

@marcaaron
Copy link
Contributor

Here's what I am seeing so far...

The IOUPreview component tries to fetch the report in the URL or from the action but the API returns 666 "no longer exists". The other participant has no issues viewing the UI (assuming they retain access to the report, but the other user who closed their account does not).

Maybe dumb question... but can we just reshare these IOU reports when someone reopens their account?

Thinking we can just find all IOU reports where they are the accountID or managerID and re-share them. Then they will have access to the retracted report? Retracting the report seems fine is the lack of access to it that messes up the UI.

@marcaaron
Copy link
Contributor

Maybe I'm not getting how this feature works. I can't ever remember.

@marcaaron
Copy link
Contributor

Oh actually it would just be if they are the managerID I think. Since if the report has your accountID then you would be able to access it still 🤔

@marcaaron
Copy link
Contributor

cc @Julesssss @mountiny as I think you guys are probably pretty familiar with the IOU flows and can please tell me if you see any obvious issues with this approach?

Problem:

  • We unshare IOU reports when a "manager/debtor/payer" closes their account.
  • If they reopen their account the UI breaks since they are trying to access reports they can no longer access.

Solution:

  • Reshare the IOU reports if the managerID account is reopened

@marcaaron
Copy link
Contributor

actually, that solution appears to break all new IOU transactions. 😕

@marcaaron
Copy link
Contributor

Just noting that if you do not cancel the request but close and reopen you will still get a loading spinner as the payer though the requester will see that they are owed:

2022-11-04_09-10-47

Creating a new request as the user who did not close their account works

2022-11-04_09-11-00

But will lead to them seeing two report actions

2022-11-04_09-11-22

What's also weird is that the previous action "Details" says that the other user paid (but they didn't)

2022-11-04_09-17-40

And the new preview for the new transaction looks OK

2022-11-04_09-17-57

This feature seems very broken when an account gets closed - did we just not think about what would happen at all? This might be too big of an issue for me to focus on at the moment, but will wait for input from others in case I am wrong and there's an obvious solution.

@puneetlath
Copy link
Contributor

If we canceled the IOU request before unsharing the report, would that solve it?

@marcaaron
Copy link
Contributor

ah sorry, can you clarify what you think that would solve? I'm not sure what the "it" in your question refers to.

@puneetlath
Copy link
Contributor

Sorry, what I meant was that if we cancel the IOU request before closing the account, then theoretically there is nothing that needs to be reshared when the account is re-opened since the IOU no longer exists. Perhaps that is overly simplified though.

@marcaaron
Copy link
Contributor

Got it, the loading spinners will still happen even if you manually cancel the request before closing your account. We could cancel the IOU transaction automatically when someone closes their account - but they would still lose access to the IOU report itself (so the UI will keep showing the loading spinner in both scenarios).

I still can't really think of why someone would need to have an IOU report unshared from them when they close their account.

@marcaaron
Copy link
Contributor

Looks like when someone closes their account the "active" IOU report's state and status go from:

state = 1
status = 1

to

state = 0
status = 0

AFAICT iou reports should not ever have a state of 0 and are always either SUBMITTED or MANUALREIMBURSED.

It is getting rejected like @stitesExpensify found above.

Ok so, alternative idea...

  1. If the IOUReport shared has a non-zero total then rejact all the transactions on behalf of the user.
  2. Do not "reject" the report or change the state or status of the IOU report (that should now have a zero total).
  3. Re-share the IOU report with the user when they reopen their account (so we don't have to create a new one)

@marcaaron
Copy link
Contributor

Tested the above solution out and it works pretty good here are some drafts:

https://github.com/Expensify/Web-Expensify/pull/35453
https://github.com/Expensify/Auth/pull/7228

@puneetlath
Copy link
Contributor

Nice!

@melvin-bot melvin-bot bot added the Overdue label Nov 8, 2022
@marcaaron marcaaron added Weekly KSv2 and removed Daily KSv2 labels Nov 8, 2022
@melvin-bot melvin-bot bot removed the Overdue label Nov 8, 2022
@marcaaron
Copy link
Contributor

Still working on this, but it's not a daily priority for me since closing accounts is an edge case.

@JmillsExpensify
Copy link

+1. I think weekly is appropriate.

@marcaaron
Copy link
Contributor

Related Auth PR has been merged. Wrote some integration tests for the Web-E PR (still on HOLD waiting for Auth deploy).

@JmillsExpensify
Copy link

Not sure why I'm subscribed to this issue haha, but just noticed we need to create an Upwork job for the reporting bonus.

@puneetlath
Copy link
Contributor

@Tushu17 can you please apply here for the reporting bonus: https://www.upwork.com/jobs/~0102f107207d43a2df

@Tushu17
Copy link
Contributor

Tushu17 commented Nov 14, 2022

@puneetlath Ok applied, Thanks.

@puneetlath
Copy link
Contributor

Great, paid!

@puneetlath
Copy link
Contributor

Looks like we're done here!

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. Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
None yet
Development

No branches or pull requests