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

migrate to sveltekit action #1716

Merged
merged 7 commits into from
Jan 30, 2023

Conversation

longrunningprocess
Copy link
Contributor

@longrunningprocess longrunningprocess commented Jan 25, 2023

Description

This PR migrates the change password functionality toward a newer SvelteKit feature called Actions.

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.

  • ensure passwords are required
  • ensure the passwords match
  • ensure password is actually updated to a new password

@longrunningprocess longrunningprocess added the engineering Tasks which do not directly relate to a user-facing feature or fix label Jan 25, 2023
@longrunningprocess longrunningprocess self-assigned this Jan 25, 2023
@longrunningprocess longrunningprocess enabled auto-merge (squash) January 25, 2023 21:29
@github-actions
Copy link

github-actions bot commented Jan 25, 2023

Unit Test Results

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

Results for commit 92bf264.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@myieye myieye left a comment

Choose a reason for hiding this comment

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

Just a few nits mostly 😉

I'm looking at the PHP...is there actually no password validation when changing your pasword 😲? In the long run I assume this validation will land in the new API, but this is maybe the best place for it now.

next-app/src/routes/password/change/+page.server.ts Outdated Show resolved Hide resolved
next-app/src/routes/password/change/+page.server.ts Outdated Show resolved Hide resolved
next-app/src/routes/password/change/+page.server.ts Outdated Show resolved Hide resolved
next-app/src/routes/password/change/+page.server.ts Outdated Show resolved Hide resolved
next-app/src/routes/password/change/+page.svelte Outdated Show resolved Hide resolved
next-app/src/routes/password/change/+page.ts Show resolved Hide resolved
@longrunningprocess
Copy link
Contributor Author

Just a few nits mostly 😉

I'm looking at the PHP...is there actually no password validation when changing your pasword 😲? In the long run I assume this validation will land in the new API, but this is maybe the best place for it now.

it depends where you look 😆 , but yes, we should have something like this in place: https://svelte.dev/repl/2d597987426345d697c5c6729dc2cacc?version=3.55.1

@megahirt
Copy link
Collaborator

I'm looking at the PHP...is there actually no password validation when changing your password 😲?

I would suggest that password validation (making sure the user typed it the same twice) should be done client-side and doesn't belong in the API layer. I can see making a case for password strength being enforced at the API layer, but we don't currently do that in PHP.

@myieye
Copy link
Collaborator

myieye commented Jan 27, 2023

I'm looking at the PHP...is there actually no password validation when changing your password 😲?

I would suggest that password validation (making sure the user typed it the same twice) should be done client-side and doesn't belong in the API layer. I can see making a case for password strength being enforced at the API layer, but we don't currently do that in PHP.

Oh, of course you're right 🙈. We only pass 1 password to the server. Not sure what I was thinking there.

@longrunningprocess
Copy link
Contributor Author

I'm looking at the PHP...is there actually no password validation when changing your password 😲?

I would suggest that password validation (making sure the user typed it the same twice) should be done client-side and doesn't belong in the API layer. I can see making a case for password strength being enforced at the API layer, but we don't currently do that in PHP.

yep, excellent point, I'll change that.

@longrunningprocess longrunningprocess merged commit 1045df1 into develop Jan 30, 2023
@longrunningprocess longrunningprocess deleted the chore/migrate-to-sveltekit-action branch January 30, 2023 16:56
@myieye
Copy link
Collaborator

myieye commented Feb 2, 2023

🧪 See my test results on #1719.

@laineyhm
Copy link
Collaborator

🧪 See my test results on #1719.

Ditto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engineering Tasks which do not directly relate to a user-facing feature or fix
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants