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

feat: Allow modifying user email from settings #282

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ansen01-svg
Copy link
Contributor

What changed?

Added email display in settings account page.

Why?

Users were unable to view their current email, or update the email.

How was this change made?

Added email edit section to /settings/account page.

@github-actions github-actions bot added frontend CSS, HTML, and JS changes backend Schema, query, and mutation changes labels Dec 15, 2024
@evadecker evadecker self-requested a review December 15, 2024 21:43
@evadecker
Copy link
Member

Hey @ansen01-svg! I'll take a closer look at this soon. I appreciate you diving in here!

@evadecker evadecker changed the title feat: added email display in account settings page. feat: Allow modifying user email from settings Dec 15, 2024
Copy link
Member

@evadecker evadecker left a comment

Choose a reason for hiding this comment

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

This is looking good so far @ansen01-svg! Would love to get some tests written for this.

@@ -42,3 +42,5 @@ node_modules

# Local files
*.local

.env.local
Copy link
Member

Choose a reason for hiding this comment

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

.env.local is already in .gitignore, so you can leave the file as-is!

users: typeof users;
userSettings: typeof userSettings;
Copy link
Member

Choose a reason for hiding this comment

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

Did the linter rearrange these? 🤔

Comment on lines +53 to +58
export const setEmail = userMutation({
args: { email: v.optional(v.string()) },
handler: async (ctx, args) => {
await ctx.db.patch(ctx.userId, { email: args.email });
},
});
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to add a verification flow for changing emails and during signup. We can address that separately from this PR, but I've created a ticket: #286

@@ -50,6 +50,13 @@ export const setName = userMutation({
},
});

export const setEmail = userMutation({
args: { email: v.optional(v.string()) },
Copy link
Member

Choose a reason for hiding this comment

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

We should check to make sure the email address is valid here, and throw an error if an empty string or invalid email are submitted.

Copy link
Member

Choose a reason for hiding this comment

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

We should also update users.test.ts with a test for this new mutation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const ParamsSchema = z.object({
email: z.string().email(),
});

Should I implement this from Convex Auth docs with zod library or implement a regex?

Copy link
Member

Choose a reason for hiding this comment

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

It would be great to add Zod so we can use it to validate other functions, too!

Here's a helpful guide: https://stack.convex.dev/typescript-zod-function-validation

We already have Convex helpers installed, but you'll need to install zod.

@@ -0,0 +1,112 @@
import {
Copy link
Member

Choose a reason for hiding this comment

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

We'll want to write a unit test for this component! I haven't written many tests for settings components yet, but now's a good time to start. You can see a test for a similar modal-style form submission component in EditQuestTimeRequiredModal and EditQuestCostsModal.

Copy link
Member

Choose a reason for hiding this comment

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

This PR adds tests for other settings components and is a great place to look! #290

}: EditEmailModalProps) => {
const updateEmail = useMutation(api.users.setEmail);
const [email, setEmail] = useState(defaultEmail);
const [error, setError] = useState<string>();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const [error, setError] = useState<string>();
const [error, setError] = useState('');

Nit!

Comment on lines +55 to +56
title="Email address"
description="This is the email we’ll use to contact you."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
title="Email address"
description="This is the email we’ll use to contact you."
title="Edit email address"
description="What email would you like to use for Namesake?"

@@ -0,0 +1 @@
export * from "./EditEmailSetting";
Copy link
Member

Choose a reason for hiding this comment

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

This file can be renamed to index.ts (no x)

label="Name"
name="name"
type="text"
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for adding this?

@evadecker evadecker added this to the Namesake v1.0 milestone Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Schema, query, and mutation changes frontend CSS, HTML, and JS changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants