refactor password checks to client-side #1719

longrunningprocess · 2023-01-30T18:25:36Z

Description

This is a follow-through on PR feedback from #1716

Checklist

I have labeled my PR with: bug, feature, engineering, security fix or testing
I have performed a self-review of my own code
I have reviewed the title & description of this PR which I will use as the squashed PR commit message
I have enabled auto-merge (optional)

QA testing

Testers, use the following instructions on our staging environment. Post your findings as a comment and include any meaningful screenshots, etc.

While trying to change your password, ensure you cannot submit the form unless you've provided a password
While trying to change your password, ensure you cannot submit the form unless you've provided a confirmation password
While trying to change your password, ensure you cannot submit the form unless you've provided a confirmation password that matches the new password
While trying to change your password, ensure you can submit the form and your password has actually changed.

github-actions · 2023-01-30T18:27:37Z

Unit Test Results

362 tests 362 ✔️ 16s ⏱️
  37 suites     0 💤
    1 files     0 ❌

Results for commit 891e151.

♻️ This comment has been updated with latest results.

next-app/src/routes/password/change/+page.svelte

next-app/src/lib/debounce.ts

myieye

LGTM 👍

myieye · 2023-02-02T07:50:57Z

next-app/src/routes/password/change/+page.svelte

@@ -16,6 +17,15 @@
 	$: if (form?.failed) {
 		$error = Error(form.failed)
 	}
+
+	$: new_password_confirm && debounce(() => verify(new_password_confirm, new_password), 400)


On trying this out, I find the 400ms delay a bit quirky. I know the number isn't random - at least that's what I'm guessing. I think the main problem is that it puts the error message out of sync with whether the button is enabled or disabled - the button doesn't have a delay. Considering the comparison check doesn't cost us anything, I think it make sense to remove the delay.

I also don't like getting error messages as I type the confirmation password 🤔. What do you think about always leaving the Submit button enabled? Can we only call verify() on blur and submit instead?

yeah, based on our recent conversation, I'll just get rid of the bells and whistles here and stick with an error once the user submits (and leave the button enabled the whole time)

myieye · 2023-02-02T08:35:44Z

next-app/src/routes/password/change/+page.svelte

+	$: disabled = ! submittable
+
+	function verify(password_confirm: string, password: string) {
+		password_confirm !== password ? $error = Error('Passwords are not the same') : dismiss()


Instead of an error bubble, I think it would ultimately be nicer to have the error directly under the relevant field. This is a special case, where it could ultimately be either field that's "wrong", but I think it would still be fine to just put it under the confirmation field.

agreed, when it comes to forms, not utilizing the app-wide error solution would be better for the user.

myieye · 2023-02-02T08:51:15Z

🧪 QA testing

Started some more conversations around the UX ☝️
❌ I can submit an invalid form using Enter

There doesn't seem to be a form toolkit in the standard svelte libs. Is that right? I feel like all the things I've brought up, hinge on that. Perhaps we should consider adding something like svelte-forms?

Being able to submit invalid forms with Enter is the only "bug" I found. So, if you want, you can just fix that, move this along and then we look at the UX in a later PR.

longrunningprocess · 2023-02-02T15:38:21Z

There doesn't seem to be a form toolkit in the standard svelte libs. Is that right? I feel like all the things I've brought up, hinge on that. Perhaps we should consider adding something like svelte-forms?

correct, there is not a built-in for that but as you found already, there are some nice solutions out there already. I can consider that. I'll double-check daisyUI as well, he may already have something in place for it.

Being able to submit invalid forms with Enter is the only "bug" I found. So, if you want, you can just fix that, move this along and then we look at the UX in a later PR.

I need to look closer, I would've expected a standard form with a single button to automatically get the "submit" behavior even on enter but maybe I'm remembering something wrong there. Thanks for checking that though.

myieye · 2023-02-07T10:24:58Z

Being able to submit invalid forms with Enter is the only "bug" I found. So, if you want, you can just fix that, move this along and then we look at the UX in a later PR.

I need to look closer, I would've expected a standard form with a single button to automatically get the "submit" behavior even on enter but maybe I'm remembering something wrong there. Thanks for checking that though.

@longrunningprocess To prevent submitting, you're currently disabling the button. I guess Svelte[Kit] doesn't read the disabled property to determine whether the user is allowed to submit the form. So, I think what we want to do is:

Always leave the submit button enabled
Make use of use:enhance. Something like this:

use:enhance={({ cancel }) => { 
	if (passwordsDontMatch) {
		showError();
		cancel();
	}
}}

laineyhm · 2023-02-23T04:45:17Z

While trying to change your password, ensure you cannot submit the form unless you've provided a password

✅

While trying to change your password, ensure you cannot submit the form unless you've provided a confirmation password

✅

While trying to change your password, ensure you cannot submit the form unless you've provided a confirmation password that matches the new password

✅

While trying to change your password, ensure you can submit the form and your password has actually changed.

✅

billy clark added 2 commits January 30, 2023 13:18

needed to extend Button impl

d479184

incorporate PR feedback

64fa758

longrunningprocess added the engineering Tasks which do not directly relate to a user-facing feature or fix label Jan 30, 2023

longrunningprocess self-assigned this Jan 30, 2023

hahn-kev reviewed Jan 31, 2023

View reviewed changes

next-app/src/routes/password/change/+page.svelte Outdated Show resolved Hide resolved

next-app/src/routes/password/change/+page.svelte Show resolved Hide resolved

incorporated PR feedback

6b7cca3

longrunningprocess enabled auto-merge (squash) January 31, 2023 19:23

myieye reviewed Feb 1, 2023

View reviewed changes

next-app/src/lib/debounce.ts Show resolved Hide resolved

corrected spacing

c6e1d3a

myieye approved these changes Feb 1, 2023

View reviewed changes

correcteded spacing :-D

891e151

longrunningprocess merged commit 499c314 into develop Feb 1, 2023

longrunningprocess deleted the chore/migrate-to-sveltekit-action-pr-feedback branch February 1, 2023 15:22

myieye reviewed Feb 2, 2023

View reviewed changes

myieye mentioned this pull request Feb 2, 2023

migrate to sveltekit action #1716

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor password checks to client-side #1719

refactor password checks to client-side #1719

longrunningprocess commented Jan 30, 2023 •

edited by myieye

Loading

github-actions bot commented Jan 30, 2023 •

edited

Loading

myieye left a comment

myieye Feb 2, 2023 •

edited

Loading

myieye Feb 2, 2023 •

edited

Loading

longrunningprocess Feb 2, 2023

myieye Feb 2, 2023

longrunningprocess Feb 2, 2023

myieye commented Feb 2, 2023 •

edited

Loading

longrunningprocess commented Feb 2, 2023 •

edited

Loading

myieye commented Feb 7, 2023

laineyhm commented Feb 23, 2023

refactor password checks to client-side #1719

refactor password checks to client-side #1719

Conversation

longrunningprocess commented Jan 30, 2023 • edited by myieye Loading

Description

Checklist

QA testing

github-actions bot commented Jan 30, 2023 • edited Loading

Unit Test Results

myieye left a comment

Choose a reason for hiding this comment

myieye Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

myieye Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

longrunningprocess Feb 2, 2023

Choose a reason for hiding this comment

myieye Feb 2, 2023

Choose a reason for hiding this comment

longrunningprocess Feb 2, 2023

Choose a reason for hiding this comment

myieye commented Feb 2, 2023 • edited Loading

🧪 QA testing

longrunningprocess commented Feb 2, 2023 • edited Loading

myieye commented Feb 7, 2023

laineyhm commented Feb 23, 2023

While trying to change your password, ensure you cannot submit the form unless you've provided a password

While trying to change your password, ensure you cannot submit the form unless you've provided a confirmation password

While trying to change your password, ensure you cannot submit the form unless you've provided a confirmation password that matches the new password

While trying to change your password, ensure you can submit the form and your password has actually changed.

longrunningprocess commented Jan 30, 2023 •

edited by myieye

Loading

github-actions bot commented Jan 30, 2023 •

edited

Loading

myieye Feb 2, 2023 •

edited

Loading

myieye Feb 2, 2023 •

edited

Loading

myieye commented Feb 2, 2023 •

edited

Loading

longrunningprocess commented Feb 2, 2023 •

edited

Loading