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

Security - Close Account - No growl is received when closing an account #10224

Closed
kbecciv opened this issue Aug 3, 2022 · 22 comments
Closed
Assignees
Labels
Engineering Improvement Item broken or needs improvement. Reviewing Has a PR in review Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Aug 3, 2022

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. Access staging.new.expensify.com OR open the desktop mac app
  2. Sign into a public domain account
  3. Click on the Profile icon > Security > Close account
  4. Input the correct information in the fields and proceed with the "close account" button in red.

Expected Result:

The user expects to be navigated to the Sign in page and receive a growl so that they may now their actions were successful.

Actual Result:

The user is redirected correctly but no growl was received.

Workaround:

Unknown

Platform:

Where is this issue occurring?

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

Version Number: 1.1.87.8

Reproducible in staging?: Yes

Reproducible in production?: No

Email or phone of affected tester (no customers): any

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5673568_PROD.mp4
Bug5673568_No_growl.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Aug 3, 2022
@OSBotify
Copy link
Contributor

OSBotify commented Aug 3, 2022

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@melvin-bot
Copy link

melvin-bot bot commented Aug 3, 2022

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

@MonilBhavsar
Copy link
Contributor

@Beamanator looks like it is coming from #9635
I think it would be better to display growl in this case that action is successful. What do you think?

@MonilBhavsar
Copy link
Contributor

cc @Julesssss as it appears like Alex is OOO today and this is hourly issue

@parasharrajat
Copy link
Member

There was a confirmation modal.

@Beamanator
Copy link
Contributor

I was ooo most of yesterday, back today! I'm not sure this growl was necessary, i'll take a look at the design doc where this refactor happened to see if this detail was missed or if it's expected we don't show the growl

@Beamanator
Copy link
Contributor

Ok the confirmation Growl wasn't specifically discussed in the doc, but I don't think the Growl is necessary. We automatically redirect the user to the sign in screen when they close their account, that should be good enough. I would say we just need to update the regression test steps to not look for the growl anymore

@Julesssss
Copy link
Contributor

There was a confirmation modal.

Yeah, there was. IIRC the 'type your account details' was a replacement for this step though.

I also kinda think the growl should exist though. Regardless, it shouldn't be a deployblocker.

@Julesssss Julesssss added Daily KSv2 Hourly KSv2 and removed Hourly KSv2 DeployBlockerCash This issue or pull request should block deployment Daily KSv2 labels Aug 3, 2022
@Beamanator
Copy link
Contributor

Beamanator commented Aug 3, 2022

Yeah, there was. IIRC the 'type your account details' was a replacement for this step though.

I think you're right about this one. In the PR you can see that the only modal I changed was the error modal, not any confirm modal

I also kinda think the growl should exist though. Regardless, it shouldn't be a deployblocker.

Agree it shouldn't be a deploy blocker, the growl could make sense too - I think we are mainly trying to avoid growls now'days because they're only showing for a limited time and .. some other reasons I forget? But if you close your account, you get logged out and it doesn't really matter if you don't see the growl right?

@Julesssss
Copy link
Contributor

I view this as an exception to the no growl rule as closing an account is an incredibly rare and important event. Also, when testing I did notice that there wasn't any feedback and it felt a bit odd -- almost like there had been an error.

@Beamanator
Copy link
Contributor

Beamanator commented Aug 3, 2022

I'm ok with either way - growl or no growl. I don't find it confusing that "close account" takes you back to the sign in page w/out a growl but I wouldn't be opposed to adding one back there

@MonilBhavsar MonilBhavsar added the Improvement Item broken or needs improvement. label Aug 3, 2022
@MonilBhavsar
Copy link
Contributor

Agree 👍 this seems like a "no growl" exception and we should add one to indicate that action succeeded.

@Julesssss Julesssss added Weekly KSv2 and removed Daily KSv2 labels Aug 3, 2022
@melvin-bot melvin-bot bot added the Overdue label Aug 15, 2022
@MonilBhavsar
Copy link
Contributor

Engaged in N07, will update soon

@melvin-bot melvin-bot bot removed the Overdue label Aug 24, 2022
@melvin-bot melvin-bot bot added the Overdue label Sep 2, 2022
@MonilBhavsar
Copy link
Contributor

Will get back next week!

@melvin-bot melvin-bot bot removed the Overdue label Sep 8, 2022
@melvin-bot melvin-bot bot added the Overdue label Sep 19, 2022
@MonilBhavsar
Copy link
Contributor

Working on it now!

@melvin-bot melvin-bot bot removed the Overdue label Sep 21, 2022
@MonilBhavsar
Copy link
Contributor

I had a look at the User_Delete flow and the design doc. I thought of displaying a growl after we make a Write request here but realized we can't be optimistic that account will be deleted every time as we many checks here before deleting the account. So, I think the only way to add a growl is to make it use makeRequestWithSideEffects instead of Write. I'm not sure if we really want to change request type just to display growl.
@Beamanator @Julesssss thoughts? Should I bring this in slack, or :donothing:?

@Beamanator
Copy link
Contributor

I think this could be a good convo to bring to slack, since:

  1. As you mentioned, we try not to use makeRequestWithSideEffects when possible
  2. Some people are adamant about not using Growls in our product, so maybe there's some other kind of message / notification we can display to the user

@MonilBhavsar
Copy link
Contributor

@melvin-bot melvin-bot bot added the Overdue label Oct 3, 2022
@MonilBhavsar
Copy link
Contributor

MonilBhavsar commented Oct 7, 2022

So when we push onyx update to clients, we can pass a message in ACCOUNT CLOSE_ACCOUNT Onyx key and display it to user.

Here I tried displaying it on Login page.

Screenshot 2022-10-07 at 3 30 48 PM

@Expensify/design Please review and share your thoughts.
Context: When user closes their account and it is successful, they're redirected to login page. We want to display a message to tell them that their account is closed.
As in the screenshot above, I think we can display a message on the login page, Or make a new page with a message and a button to redirect to signin page.

@melvin-bot melvin-bot bot removed the Overdue label Oct 7, 2022
@Beamanator
Copy link
Contributor

FYI we have an onyx key closeAccount so maybe we can use that instead of ACCOUNT?

@MonilBhavsar
Copy link
Contributor

Yeah, that will be better 👍

@michelle-thompson
Copy link
Contributor

I think the above screenshot looks good 👍

@MonilBhavsar MonilBhavsar added the Reviewing Has a PR in review label Oct 10, 2022
@melvin-bot melvin-bot bot closed this as completed Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Improvement Item broken or needs improvement. Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

7 participants