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.

Sign up for GitHub

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

Feature/update user-info in settings #939

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Feature/update user-info in settings #939

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6bcff5a
add PhoneInput component + validatePhoneNumber method
madsab Aug 5, 2024
9241ba5
set User-properties middlename and phonenumber to optional
madsab Aug 13, 2024
d97a83a
fix(button): prevent `loading` prop from being passed as an HTML attr…
madsab Aug 13, 2024
f200520
feat: update user-info in settings (not image)
madsab Aug 14, 2024
1f8d6cb
fix: eslint
madsab Aug 15, 2024
9bc792f
fix: missing peer dependency @types/node@* + postcss@^8.3.3
madsab Aug 15, 2024
b78041a
add owUser properties to next-auth User
madsab Aug 15, 2024
49b8c73
Merge branch 'main' into feature/dot-730-change-account-name
madsab Aug 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions apps/web/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
"@dotkomonline/ui": "workspace:*",
"@dotkomonline/utils": "workspace:*",
"@fadi-ui/react-country-flag": "^1.0.7",
"@hookform/resolvers": "^3.3.4",
madsab marked this conversation as resolved.
Show resolved Hide resolved
"@next/env": "^14.0.3",
"@portabletext/react": "^3.0.11",
"@radix-ui/colors": "^3.0.0",
Expand Down
2 changes: 1 addition & 1 deletion apps/web/src/components/organisms/Navbar/components/profile/ProfileMenu/SettingsMenuItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const SettingsMenuItem: React.FC<SettingsMenuItemProps> = ({ menuItem }) => {
)}
>
<div className={cn("mr-4 h-7 w-7")}>
<Icon icon={icon} width="w-7" />
<Icon icon={icon} width={24} />
</div>
<p className="font-medium">{title}</p>
</Link>
Expand Down
203 changes: 167 additions & 36 deletions apps/web/src/components/views/SettingsView/components/SettingsLanding.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,50 +1,181 @@
"use client"
import AvatarImgChange from "@/app/settings/components/ChangeAvatar"
import { CountryCodeSelect } from "@/app/settings/components/CountryCodeSelect"
import { TextInput, Textarea } from "@dotkomonline/ui"
import { trpc } from "@/utils/trpc/client"
import type { UserWrite } from "@dotkomonline/types"
import { Button, PhoneInput, TextInput, Textarea, isValidPhoneNumber } from "@dotkomonline/ui"
import { zodResolver } from "@hookform/resolvers/zod"
import type { NextPage } from "next"
import type { User } from "next-auth"
import { useState } from "react"
import { useForm } from "react-hook-form"
import { z } from "zod"

interface FormInputProps {
title: string
children?: JSX.Element
}
const nameSchema = z
.string()
.min(2, "Navn må være minst 2 bokstaver")
.max(50, "Navn kan maks være 50 bokstaver")
.regex(/^[a-zA-ZæøåÆØÅ\s'-]+$/, "Navn kan ikke inneholde tall eller spesialtegn bortsett fra - og '")
Comment on lines +15 to +17
Copy link
Member

@henrikhorluck henrikhorluck Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we should have a limitation on names here, first of all because regex can cause issues with names from e.g. FEIDE not being valid.

We actually don't need name-validation since we now always set the name from FEIDE, so we have access to users' legal names. Accounts without membership/usernames should probably only show their emails and we are likely fine with them not being able to change their names

Copy link
Member

Choose a reason for hiding this comment

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

Agree, I don't think we should allow self-name changing since we're also storing the names in two places. More place for error

Copy link
Member

Choose a reason for hiding this comment

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

since we're also storing the names in two places.

What do you mean by this? The single source of truth for us should be Auth0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that this can cause issues, but what about users not registering with FEIDE? Can we force name inputs on account registration?

Copy link
Member

@henrikhorluck henrikhorluck Aug 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Even social members will have their name set through FEIDE (and if not that is HS badly modifying data / some desperate actions related to ITEX). This is then only relevant for what are essentially guest-users, which I think has only been used for utmatrikulering/immball (?).

tl;dr: we will eventually not have "active" users where we do not have their name from FEIDE

any users who actively want to set their name to something else could be something we support, but that should be dealt with on an individual basis if so, and not something I would spend time implementing before it is a feature request.

.toLowerCase()
.trim()
.transform((v) =>
v
.split(" ")
.map((word) => word.charAt(0).toUpperCase() + word.slice(1))
.join(" ")
)

const FormInput: React.FC<FormInputProps> = ({ title, children }) => (
<div className="w-full border-t-[1px] border-slate-7 flex py-8 justify-between px-4">
<div className="w-1/4">{title}:</div>
<div className="flex-1 flex justify-center">{children}</div>
</div>
)
const updateUserSchema = z.object({
givenName: nameSchema,
familyName: nameSchema,
email: z.string().email("Ikke en gyldig epost"),
phone: z
.string()
.refine((v) => isValidPhoneNumber(v), "Ikke et gyldig telefonnummer")
.optional(),
allergies: z.array(z.string()).optional(),
picture: z.string().optional(),
})

const Landing: NextPage<{ user: User }> = ({ user }) => {
const [allergies, setAllergies] = useState<string>()
const updateUser = trpc.user.update.useMutation()
const {
register,
handleSubmit,
setValue,
trigger,
formState: { errors, dirtyFields, isSubmitting },
} = useForm<UserWrite>({
resolver: zodResolver(updateUserSchema),
defaultValues: {
...user,
},
})

const formatAllergies = (allergies: string) => {
setAllergies(
allergies
.toLowerCase()
.replace(/[\s,]+/g, ",")
.replace(/(^\w|,\w)/g, (match) => match.toUpperCase())
Comment on lines +59 to +60
Copy link
Member

Choose a reason for hiding this comment

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

Allergies has to be consistently either a single string-field or a list as presented to users, I don't think we should split it based on commas/spaces, users will just give input that does not turn well into a list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I'm not sure if i understand the problem? If the allergies are used as a string, then most likely all users allergies will be unique and there will be no way of using them for analytics, for example knowing how many attendees in an event are allergic to nuts.
    However, it might be a suitable solution to give the user an input-field for ONE allergy, then hitting a + to get another input-field for the next allergy.
  2. What do you mean by users will just give input that does not turn well into a list, could you give an example.? In theory, a user can always give bad inputs even if the input is a string or a list

Copy link
Member

@henrikhorluck henrikhorluck Aug 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

users allergies will be unique and there will be no way of using them for analytics, for example knowing how many attendees in an event are allergic to nuts.

You would need to perform some kind of text-search w/ synonyms. There is native support for that in e.g. postgres. Also, we don't do such kinds of analytics / statistics, this information is generally only useful on an individual level.

AFAIK allergies are dealt with on an individual level, not by performing some grouping/staking the field before ordering something for everyone

However, it might be a suitable solution to give the user an input-field for ONE allergy, then hitting a + to get another input-field for the next allergy.

Yes exactly, but from the images in the PR-description it is shown as a text-field, but stored as a list

could you give an example.?

Look at e.g. the values people have for allergies at old events like https://old.online.ntnu.no/dashboard/events/2268/, there is mix of description and extra contextual information, not just "nuts" / "peanuts" / "pork", and full sentences with weird splits.

IMO: trying to make a regex/split this from free text is a lost cause, just let it be a free text-field, and allow event organizers to sort by non-empty values just read through all of them individually. That is how it currently works, and I think most other features for this is over-engineering or complicating it without giving us any gains. A potential alternative would be that we have a specific list of allergies we can accomodate for, and not allow user-input as text, but IMO that sounds like a lot of work with

  • gathering the specific allergies
  • makes the user have to actively hunt for their allergies
  • maintaining/updating the list

Most of our attendees do not have allergies, that might be a bias/registration problem, but I would focus on other features related to monoweb / figure out concrete issues users/organizers have with it before we do anything more

)
}

const onSubmit = async (data: UserWrite) => {
const [firstName, ...rest] = data.givenName.split(" ")
const middleName = rest.join(" ") || null
const fakePopulatedData = {
...data,
givenName: firstName,
middleName: middleName,
name: `${data.givenName} ${data.familyName}`,

/* Add fake data as the session token does not currently give a full user */
phone: data.phone || null,
auth0Id: "auth0|c75f2ffa-4b07-41c2-b173-273a2b443c6d",
gender: "male" as const,
studyYear: 2,
picture: "https://example.com/image.jpg",
lastSyncedAt: new Date(),
}
await updateUser.mutateAsync(
{ data: fakePopulatedData },
{
onSuccess: (data) => {
// change to toast when toast-functionality is implemented and remove console.log and alert
console.log("Success:", data)
alert("Endringer lagret")
},
onError: (error) => {
// change to toast when toast-functionality is implemented and remove console.log and alert
console.log("Error:", error)
alert("Noe gikk galt, prøv igjen")
},
}
)
}

return (
<div className="flex w-full flex-col space-y-4">
<div className="flex flex-col items-center justify-evenly space-y-4 mb-4">
<AvatarImgChange {...user} />
</div>
<FormInput title="Navn">
<div className="w-full flex flex-wrap justify-center ">
<TextInput width="flex-1 mb-2 mx-1" placeholder="Fornavn" defaultValue={user.givenName} />
<TextInput width="flex-1 mx-1" placeholder="Etternavn" defaultValue={user.familyName} />
</div>
</FormInput>
<FormInput title="Epost">
<TextInput width="flex-1" placeholder="Epost" defaultValue={user.email} />
</FormInput>
<FormInput title="Telefon">
<div className="w-full flex space-x-2">
<CountryCodeSelect />
<TextInput width="w-full" maxLength={8} />
<div className="flex w-full flex-col space-y-4 py-2">
<form onSubmit={handleSubmit(onSubmit)}>
<div className="flex flex-col items-center justify-evenly mb-4">
<AvatarImgChange {...user} />
</div>
</FormInput>
<FormInput title="Bio">
<Textarea placeholder="Din råkule bio" />
</FormInput>
<FormInput title="Allergier">
<Textarea placeholder="Dine allergier" />
</FormInput>
<FormInputContainer title="Navn" required>
<div className="w-full flex flex-wrap justify-center h-full ">
<TextInput
width="flex-1 mb-2 mx-1"
placeholder="Fornavn"
defaultValue={user.givenName}
{...register("givenName")}
error={errors.givenName?.message}
/>
<TextInput
width="flex-1 mx-1"
placeholder="Etternavn"
defaultValue={user.familyName}
{...register("familyName")}
error={errors.familyName?.message}
/>
</div>
</FormInputContainer>
<FormInputContainer title="Epost" required>
<TextInput
width="flex-1"
placeholder="Epost"
defaultValue={user.email}
{...register("email")}
error={errors.email?.message}
/>
</FormInputContainer>
<FormInputContainer title="Telefon">
<PhoneInput
width="w-full"
placeholder="Telefonnummer"
defaultValue={user.phone || undefined}
onChange={(value: string) => {
setValue("phone", value, { shouldDirty: true })
trigger("phone")
}}
error={errors.phone?.message}
/>
</FormInputContainer>
<FormInputContainer title="Allergier">
<Textarea
spellCheck="false"
placeholder="Dine allergier"
defaultValue={user.allergies?.join(",")}
value={allergies}
{...register("allergies", {
onChange: (e) => {
formatAllergies(e.target.value)
},
setValueAs: (v: string) => (v ? v.split(",").filter((a) => a.length > 0) : []),
})}
/>
</FormInputContainer>
{Object.keys(dirtyFields).length > 0 && (
<Button type="submit" className="w-full my-2 fade-in-25" loading={isSubmitting}>
Lagre
</Button>
)}
</form>
</div>
)
}
interface FormInputContainerProps {
title: string
children?: JSX.Element
required?: boolean
}

const FormInputContainer: React.FC<FormInputContainerProps> = ({ title, children, required }) => (
<div className="w-full border-t-[1px] border-slate-7 flex pt-12 justify-between px-4">
<div className="w-1/4">
{title}:{required && "*"}
</div>
<div className="flex-1 flex min-h-24 justify-center">{children}</div>
</div>
)

export default Landing
10 changes: 9 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@
"name": "monorepo",
"version": "1.0.0",
"description": "Monoweb is the next-generation web application for Online. This is the monorepo source.",
"keywords": ["online", "ntnu", "student-association"],
"keywords": [
"online",
"ntnu",
"student-association"
],
"homepage": "https://online.ntnu.no",
"author": "Dotkom <dotkom@online.ntnu> (https://online.ntnu.no)",
"bugs": {
Expand Down Expand Up @@ -52,5 +56,9 @@
"turbo": "^1.10.16",
"turbo-ignore": "^1.10.16",
"typescript": "^5.4.5"
},
"dependencies": {
"@types/node": "^20.12.7",
"postcss": "^8.3.3"
}
}
31 changes: 24 additions & 7 deletions packages/auth/src/auth-options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,27 @@
import type { ServiceLayer } from "@dotkomonline/core"
import type { DefaultSession, DefaultUser, NextAuthOptions, User } from "next-auth"
import type { User as Auth0User } from "@dotkomonline/types"
import Auth0Provider from "next-auth/providers/auth0"

interface Auth0IdTokenClaims {
given_name: string
middle_name: string
family_name: string
nickname: string
name: string
nickname: string

picture: string
gender: string
updated_at: string
gender: "male" | "female" | "other"
email: string
email_verified: boolean
updated_at: string

studyYear: number
allergies: string[]
phone: string
lastSyncedAt: Date


iss: string
aud: string
iat: number
Expand All @@ -27,7 +37,7 @@ declare module "next-auth" {
id: string
}

interface User extends DefaultUser {
interface User extends Auth0User, DefaultUser {
id: string
name: string
email: string
Expand Down Expand Up @@ -62,9 +72,16 @@ export const getAuthOptions = ({
id: profile.sub,
name: profile.name,
email: profile.email,
image: profile.picture ?? undefined,
// givenName: profile.given_name,
// familyName: profile.family_name,
picture: profile.picture ?? undefined,
givenName: profile.given_name,
familyName: profile.family_name,
auth0Id: profile.sub,
middleName: profile.middle_name,
allergies: profile.allergies,
phone: profile.phone,
studyYear: profile.studyYear,
gender: profile.gender,
lastSyncedAt: profile.lastSyncedAt,
}),
}),
],
Expand Down
31 changes: 31 additions & 0 deletions packages/config/tailwind-preset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,37 @@ module.exports = {
borderRadius: {
md: "4px",
},
minHeight: {
1: "0.25rem",
Copy link
Member

Choose a reason for hiding this comment

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

do we need this?

2: "0.5rem",
3: "0.75rem",
4: "1rem",
5: "1.25rem",
6: "1.5rem",
7: "1.75rem",
8: "2rem",
9: "2.25rem",
10: "2.5rem",
11: "2.75rem",
12: "3rem",
14: "3.5rem",
16: "4rem",
20: "5rem",
24: "6rem",
28: "7rem",
32: "8rem",
36: "9rem",
40: "10rem",
44: "11rem",
48: "12rem",
52: "13rem",
56: "14rem",
60: "15rem",
64: "16rem",
72: "18rem",
80: "20rem",
96: "24rem",
},
typography: ({ theme }) => ({
DEFAULT: {
"--tw-prose-invert-bullets": theme("colors.amber.12"),
Expand Down
2 changes: 1 addition & 1 deletion packages/db/src/db.generated.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ export interface OwUser {
givenName: string
id: Generated<string>
lastSyncedAt: Generated<Timestamp>
middleName: string
middleName: string | null
name: string
phone: string | null
picture: string | null
Expand Down
4 changes: 2 additions & 2 deletions packages/types/src/user.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ export const UserSchema = z.object({
email: z.string().email(),
givenName: z.string(),
familyName: z.string(),
middleName: z.string(),
middleName: z.string().nullable().optional(),
gender: z.enum(["male", "female", "other"]),
name: z.string(),
phone: z.string().nullable(),
phone: z.string().nullable().optional(),
studyYear: z.number().int().min(-1).max(6),
allergies: z.array(z.string()),
picture: z.string().nullable(),
Expand Down
3 changes: 2 additions & 1 deletion packages/ui/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,14 @@
"@radix-ui/react-label": "^2.0.2",
"@radix-ui/react-navigation-menu": "^1.1.4",
"@radix-ui/react-radio-group": "^1.1.3",
"@radix-ui/react-select": "^1.2.2",
"@radix-ui/react-switch": "^1.0.3",
"@radix-ui/react-tabs": "^1.0.4",
"@radix-ui/react-select": "^1.2.2",
"clsx": "^2.0.0",
"cva": "npm:class-variance-authority@^0.7.0",
"react": "^18.2.0",
"react-dom": "^18.2.0",
"react-phone-number-input": "^3.4.5",
Copy link
Member

@henrikhorluck henrikhorluck Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
"react-phone-number-input": "^3.4.5",

Just use <input type="tel">? https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/tel

(I would actively not bother to have country-code validation)

Copy link
Member

@henrikhorluck henrikhorluck Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We can also get phone-numbers from FEIDE, though we might need another scope (and not sure if we get it for exchange students)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. If i'm a user who used FEIDE for my account creation, i would not like for the website to get my phone number and show it (this can be a toggle of course). I would rather want to be able to type it in myself, who knows, i might want to use a different phone number than the one registered at FEIDE.

  2. By having the country code, not only does it look good in design, but also solves issues with possible members (exchange students) having non-norwegian phone numbers.

I would like to keep it, but that's my opinion

Copy link
Member

@henrikhorluck henrikhorluck Aug 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

also solves issues with possible members (exchange students) having non-norwegian phone numbers.

Which problems? This dependency seems quite unnecessary, if we wanted to validate that the input data is good we would actually need to send a test-sms with a some kind of code akin to 2FA, but that costs money.

If i'm a user who used FEIDE for my account creation, i would not like for the website to get my phone number and show it (this can be a toggle of course). I would rather want to be able to type it in myself, who knows, i might want to use a different phone number than the one registered at FEIDE.

I agree you might want to register your phone number as something different from FEIDE, but my issue was with adding a whole new dependency which seems unnecessary and just gives us technical debt. I think the feature is fine, just remove the extra JS

Copy link
Member

Choose a reason for hiding this comment

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

Regardless of outcome this PR should remove the @fadi-ui/react-country-flag dependency

"tailwind-merge": "^2.0.0"
},
"devDependencies": {
Expand Down
Loading
Loading