Account self-deletion #1225
Account self-deletion #1225
Conversation
Logging out uses old action/reducer pairs, can use slices for deletion exclusively once updated
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1225 +/- ##
=========================================
Coverage ? 36.70%
=========================================
Files ? 451
Lines ? 19750
Branches ? 6466
=========================================
Hits ? 7250
Misses ? 11855
Partials ? 645 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
…eletion [VRT] Update baselines for feature/account-self-deletion
| <PageFragment fragmentId="account_deletion_final_warning" /> | ||
| <p>Please select the reason for deleting your account:</p> | ||
| <DeletionReasonForm className="ms-2" deletionReason={deletionReason} setDeletionReason={setDeletionReason} /> | ||
| <FormGroup check className="mt-4 ps-0"> |
There was a problem hiding this comment.
This check is causing the checkmark to be displayed outside of the checkbox. It looks very odd.
Is there a specific reason to include it that I'm missing? The only place we use it outside of this page is in RegistrationAgeCheck.tsx, but that's for a radio on which it doesn't seem to have any effect.
| <FormGroup check className="mt-4 ps-0"> | |
| <FormGroup className="mt-4 ps-0"> |
There was a problem hiding this comment.
Hm, this looks like a Firefox bug; something about the .tick class used in the checkbox being inline-block. I'm not convinced it needs the inline part anyway, as it's positioned by a flex container. I've made the tick display: block (and lengthened the short line by 1px... it was unbalanced 😣); this seems to fix the Firefox issue and I can't see any regressions elsewhere on any browser.
The check prop is used on all the checkbox examples on the RS docs, hence why I was a little cautious to remove it.
There was a problem hiding this comment.
Hrm, I'm still not convinced that the check prop is necessary. It seems to just be used to add the form-check class to the component, the styling from which doesn't actually make a difference here.
That said, I agree it's working now and I can't find any other issues with checkboxes from that change, so let's stick with it.
src/scss/cs/button.scss
Outdated
| strong a, a strong { | ||
| color: $dark-pink-300 !important; |
There was a problem hiding this comment.
Since you've styled .btn.btn-inline-link essentially identically to a strong a now anyway, should we combine both of the css expressions in this file to the same line too? I think it's quite likely that any future changes would want to be applied to both now, since they're mostly the same thing.
| strong a, a strong { | |
| color: $dark-pink-300 !important; | |
| strong a, a strong, .btn.btn-inline-link { | |
| color: $dark-pink-300 !important; |
There was a problem hiding this comment.
Yep, good idea. There's also the content links on the line above this. Not quite sure the link ones belong in button.scss, but imo consistency is more important and combining them as you suggest guarantees that.
|
|
||
| return <Container className="pb-5"> | ||
| <TitleAndBreadcrumb currentPageTitle={siteSpecific("Account Deletion", "Account deletion")} className="mb-4" /> | ||
| {!user ? <p>You must be logged in to delete your account.</p> : |
There was a problem hiding this comment.
Since this only checks for a logged-in user, it allows for an account other than the one being deleted to access this page. They won't then be able to actually delete the account because it's checked in the backend upon submitting the form (so it's not a security issue), but I could forsee this being confusing.
I'm particularly thinking about a case such as where they have multiple accounts and want to delete Account 1 and then log in with Account 2 because it is their new main account and THEN they click the link in the email. The page will show up as normal, but the deletion won't work.
I think that the page should either block a user from accessing it if they have a token which is invalid/expired for their account (so probably requiring an API interaction on page load) OR at least a confirmation of which account you're deleting somewhere on the page. Maybe both?
There was a problem hiding this comment.
We certainly shouldn't have an endpoint for checking if a token is valid, this would be a fairly major security concern... but we could use a separate token for validity and deletion, for example. We discussed this in the standup, and the conclusion was that given the token is only active for 24 hours and we believe the number of people who actively use two accounts is small, it isn't worth the extra effort to build anything now. There is ample opportunity throughout the process for them to contact us if something doesn't work so they should know to email us if this scenario occurs and deletion fails. It's a great thought though, and something to keep in mind if we see any tickets about it!
There was a problem hiding this comment.
Fair enough. I suppose that edge-case situation where this gets confusing isn't worse than our current situation of needing to manually process every delete request individually so there's no need to add extra complication (and I suppose the "HELLO <NAME>" in the nav-bar is still visible and should give some indication of which account you're logged into too, which should also help).
…eletion [VRT] Update baselines for feature/account-self-deletion
Adds a new modal
AccountSelfDeletionModaland new pagesAccountDeletion(at/deleteaccount) andAccountDeletionSuccess(/deleteaccount/success) to manage the deletion flow.Logging out still uses the old action/reducers rather than slices, and since new endpoints should be using slices, this unfortunately requires mixing the two. The slices in
accountApi.tssupply the new interaction with the API, and logging out is managed through the changes in/state/actions/index.tsx. We cannot use the existing logout functionality as this redirects to/(seeuserConsistencyChecker), so we make a parallel a/r pair. This allows us to create our own redirect.