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

Adding user profile fields #25

Merged
merged 12 commits into from
Mar 19, 2024
Merged

Adding user profile fields #25

merged 12 commits into from
Mar 19, 2024

Conversation

braydonwang
Copy link
Contributor

@braydonwang braydonwang commented Feb 16, 2024

Notion Ticket Link

Add User Profile Fields

Implementation Description

  • Added additional fields to user schema
  • Created custom scalar Date type with its resolver

Steps to Test

  1. Nothing at the moment, will need to wait for api endpoints

What should reviewers focus on?

Screenshots (optional)

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • I have run the appropriate linter(s)
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

Linting & Formatting

Mac

docker exec -it CWC-backend /bin/bash -c "yarn fix"
docker exec -it CWC-frontend /bin/bash -c "yarn fix"

Windows

docker exec -it CWC-backend bash -c "yarn fix"
docker exec -it CWC-frontend bash -c "yarn fix"

Copy link
Contributor

@shencynthia shencynthia left a comment

Choose a reason for hiding this comment

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

Just some small changes for consistency and also a nit: use imperative tense for commits + PR (e.g. Add rather than Adding).

Otherwise, looks good!!

locationType: {
type: String,
required: true,
enum: ["Remote", "Hybrid", "In Person"],
Copy link
Contributor

@shencynthia shencynthia Feb 22, 2024

Choose a reason for hiding this comment

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

For the enum in userType you have "InPerson" and here its "In Person".

backend/types.ts Outdated
@@ -1,5 +1,7 @@
export type Role = "Volunteer" | "Staff" | "Admin";

export type LocationType = "Remote" | "Hybrid" | "In Person";
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here, "InPerson" v.s "In Person"

Copy link
Contributor

@shencynthia shencynthia left a comment

Choose a reason for hiding this comment

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

LGTM! 👍👍👍

role: Role!
locationType: LocationType
headshot: String!
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason birthday and locationType are nullable? Ooh actually... nevermind!

I think startDate, birthday, locationType, and headshot should all be nullable (so no ! beside their types) because we don't provide this information to the query on the user's initial signup - the only information we pass when we register a user is their first name & last name, their email, and their password.

The decision between passing all this info to the query at once vs. in multiple steps is probably more of a PM question for later... but for now we should probably keep them nullable so the Sign Up page doesn't break :O

headshot: {
type: String,
required: true,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as before here: these probably shouldn't be required - the current signup functionality will break :(

@owen-sellner owen-sellner merged commit d1f0cee into main Mar 19, 2024
1 check failed
@owen-sellner owen-sellner deleted the braydon/add-user-fields branch March 19, 2024 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants