Skip to content

LG-2944: account reset disavow#3780

Merged
slj merged 31 commits intomasterfrom
slj-account-reset-disavow
Jun 3, 2020
Merged

LG-2944: account reset disavow#3780
slj merged 31 commits intomasterfrom
slj-account-reset-disavow

Conversation

@slj
Copy link
Copy Markdown
Contributor

@slj slj commented May 20, 2020

Why: WIP

@slj slj requested a review from jmhooper May 20, 2020 18:26
@slj
Copy link
Copy Markdown
Contributor Author

slj commented May 22, 2020

When the user has a pending account reset request, upon login they now are directed to this screen:

image

Upon clicking Cancel, the request is cancelled and user is redirected here:

image

And the user receives this email:

image

@slj
Copy link
Copy Markdown
Contributor Author

slj commented May 22, 2020

Big 👍 to @jmhooper for the pairing/education!

@slj slj marked this pull request as ready for review May 22, 2020 21:49
@Danielle-Lee
Copy link
Copy Markdown

Thank you that is very helpful. Given that information, by "reset" we mean "deletion"? @porta-antiporta

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few last small comments, but the code LGTM.

In case we didn't already, let's make sure to get sign-off on the UX/UI/copy before merging.

login_two_factor_path(otp_delivery_preference: :sms, reauthn: false),
)

# Signing in after cancelling should not show a pending request and go string to MFA
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo? "go straight to MFA" ?

require 'rails_helper'

describe AccountReset::PendingPresenter do
# I18n.locale = :en
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this commented out line?

it 'cancels the account reset request' do
subject.call

expect(account_reset_request.reload.cancelled_at).to be_within(1.second).of(Time.zone.now)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so now that .call has a default parameter of now, we can do:

let(:now) { Time.zone.now }

it 'cancels the account reset request' do
  subject.call(now: now)

  expect(account_reset_request.reload.cancelled_at.to_i).to eq(now.to_i)
end

Out of habit I tend to convert timestamps to integers like this when round-tripping from the DB, but it might work without here

slj added 3 commits May 29, 2020 13:42
**Why**: To keep codebase current
**Why**: Rubocop
**Why**: To keep files normalized
@porta-antiporta
Copy link
Copy Markdown

@Danielle-Lee yes "account reset" and "account deletion" are the same thing. I can't remember what we call it in our user facing assets.

@Danielle-Lee
Copy link
Copy Markdown

Danielle-Lee commented May 29, 2020

I rewrote the text. I think this is clearer and it's more in line with the language we use throughout the rest of the site. Open to feedback!

For screen 1 - Title should be "You requested to delete your account."
For screen 2 - In addition to the text changes, I think we only need one action button, and 'go back' should be a link instead.

https://docs.google.com/drawings/d/1fI8momFk4KyYOrxyRlwQPvATZ-btTybhqjYYs7jMV8w/edit

cc @slj @porta-antiporta @jmhooper

@slj
Copy link
Copy Markdown
Contributor Author

slj commented Jun 1, 2020

@porta-antiporta this PR does not include changes to the SMS that's sent on account deletion requests as well? For this PR, we only send an email to the user. We don't send an SMS because of the case where the user has lost their phone and thus changed numbers.

@Danielle-Lee thanks for your feedback on the verbiage. I'm still concerned with screens 2 and 3, because the user has already started the sign-in process. As I read your suggested verbiage, it makes me feel like I'm starting from scratch.

On screen 2 I'm told I can sign in on the next screen, but you actually can't. You can only click on another button to sign in.

On screen 3, the button reads "Sign in", but I already signed in (I only got to these screens by providing my username and password).

I think adding "Continue" will make it clear to the user that he/she is not starting something over.

With this in mind, I the text on screen 2 could be reduced to:

By selecting continue and cancelling this request, your account will not be deleted,
and you can continue the sign-in process.

and screen 3 could acknowledge and cancel and the button could read "Continue Sign In" or "Continue authentication"

I'm having difficulty editing the Google Draw document; hopefully these comments are clear.

@Danielle-Lee
Copy link
Copy Markdown

You can take out the second sentence on the second screen if it might be confusing to users. (" You can sign in on the next screen to access your account.").

Before I comment on the 3rd screen....Where does the user go after the 3rd screen?

@slj
Copy link
Copy Markdown
Contributor Author

slj commented Jun 1, 2020

@Danielle-Lee after the 3rd screen, the user would go to the 2FA screen:

screen_2fa

@Danielle-Lee
Copy link
Copy Markdown

@slj Thank you that's helpful. Does it have to be 3 screens? I think we could shorten this to two screens. I put text for both a 2 screen and a 3 screen flow.

2 screens
Screen 1:
[Title] You requested to delete your account.

[Body] There is a 24 hour waiting period to delete your account. In [timestamp], you will receive an email with instructions to complete the deletion.

If you cancel this request, your account will not be deleted.

[action button] Cancel Request

Screen 2:
[Title] We have cancelled your request to delete your account.

[Action button] Continue

IF we need to keep 3 screens, please use this:

Screen 1:
[Title] You requested to delete your account.

[Body] There is a 24 hour waiting period to delete your account. In [timestamp], you will receive an email with instructions to complete the deletion.

[Action button] Continue

Screen 2:
[Body] If you cancel this request, your account will not be deleted.

[Action button] Continue [hyperlink] Go Back

Screen 3:
[Title] We have cancelled your request to delete your account.

[Action button] Continue

@slj
Copy link
Copy Markdown
Contributor Author

slj commented Jun 1, 2020

@Danielle-Lee I feel we need 3 screens because we want users to be able to "undo" the cancel if they carelessly click "Cancel request" and suddenly change their minds.

@Danielle-Lee
Copy link
Copy Markdown

@slj ok that sounds good. Does the 3 screen text I listed work now?

@slj
Copy link
Copy Markdown
Contributor Author

slj commented Jun 1, 2020

@Danielle-Lee now that I think about it, I think we need to add some caution to the user in screen 2. If they cancel the request, not only will the account remain, but should the user want to cancel again, he/she will have to undergo another 24-hour waiting period.

I think this is an important disclaimer for those users who have less time remaining.

@Danielle-Lee
Copy link
Copy Markdown

@slj I think stating that again is repetitive to what's on the first screen. Is that user feedback?

@slj
Copy link
Copy Markdown
Contributor Author

slj commented Jun 1, 2020

@Danielle-Lee I see how that could seem repetitive. However, I'd like you to reconsider omitting the 24-hour wait on screen 1.

Here's the scenario: sometime earlier, the user requested an account reset and received an email that the request would take 24 hours.

Let's say the user still has 2 hours to wait when, for some reason, he/she tries to log in. The system will detect the pending reset request and inform the user that he/she has 2 hours left before the request is granted (screen 1). So, stating that there's a 24-hour wait when the user only has 2 hours left feels unnecessary to me. But, if the user carelessly or hastily clicks "Click reset", I think it would be VERY helpful to remind the user that cancelling now, after waiting 22 hours, is a big decision.

@Danielle-Lee
Copy link
Copy Markdown

@slj Thank you for the additional context. since we don't have research on this, it boils down to our own personal perspective/preference. Looping in @juliaelman for an additional opinion.

Additionally, The 24-hour wait on the first screen helps make the 'countdown timer' make more sense so we'll need to leave that in the first screen. The user has gotten an email, potentially an SMS and now this screen all saying they have a 24 hour wait period.

Julia, thoughts on this?

@slj
Copy link
Copy Markdown
Contributor Author

slj commented Jun 3, 2020

@juliaelman, I think the screens below are a good compromise based on discussions with @Danielle-Lee, but we could really use your input:

Screen Shot 2020-06-03 at 12 19 13 PM

Screen Shot 2020-06-03 at 12 19 26 PM

Screen Shot 2020-06-03 at 12 19 38 PM

@Danielle-Lee
Copy link
Copy Markdown

the content change works for me

@juliasolorzano juliasolorzano self-requested a review June 3, 2020 17:51
Copy link
Copy Markdown

@juliasolorzano juliasolorzano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

slj added 4 commits June 3, 2020 15:30
**Why**: To allow user to back out of cancellation
**Why**: controller method no longer redirects
**Why**: timesamp comparison was failing, likely due to missing to_i
@slj slj merged commit 9d3b506 into master Jun 3, 2020
@slj slj deleted the slj-account-reset-disavow branch June 3, 2020 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants