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

Move user information to auth0 #1023

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

Conversation

jotjern
Copy link
Member

@jotjern jotjern commented Sep 24, 2024

This pull request does a few things to make it possible to move user data:

  • Remove all columns except auth0_id from ow_user, and rename auth0_id to just id.
  • Change all foreign keys on user_id to point to the new id
  • Rewrite user repository to get data from auth0 instead of database
  • Remove auth0 repository and auth0 synchronisation service
  • Implement profile settings page to allow users to change their data
  • Fix the dashboard user page to allow administrators to update user data

Copy link

linear bot commented Sep 24, 2024

Copy link
Member

@junlarsen junlarsen left a comment

Choose a reason for hiding this comment

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

Didn't we agree on keeping the user table? @henrikhorluck

Make it so the user table is a single column that has the auth0 subject in it? With the current changes it's very possible to lose references because there is no foreign key relations

@jotjern jotjern changed the title Remove user table and move information to auth0 Move user information to auth0 Sep 24, 2024
onSubmit: (data) => {
const result = UserWriteSchema.parse(data)

if (result.address === "") {
result.address = undefined
Copy link
Member

Choose a reason for hiding this comment

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

}),
phone: createTextInput({
label: "Telefon",
placeholder: "12345678",
Copy link
Member

Choose a reason for hiding this comment

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

Dette eksempelet er ikke gyldig, og merk at for internasjonale må man ha med +xx

}),
lastName: createTextInput({
label: "Etternavn",
placeholder: "Ola",
Copy link
Member

Choose a reason for hiding this comment

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

Nordmann?

data: [
{ label: "Mann", value: "male" },
{ label: "Kvinne", value: "female" },
{ label: "Annet", value: "other" },
Copy link
Member

Choose a reason for hiding this comment

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

Kan vi ha med "Ønsker ikke svare"?

label: "Allergier",
placeholder: "Melk, nøtter, gluten",
}),
rfid: createTextInput({
Copy link
Member

Choose a reason for hiding this comment

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

Jeg tror ikke dette er noe vi burde be brukere skrive selv, er litt jobb å dra frem NTNU-kortet, og vi bruker det ikke til noe atm

@@ -22,7 +22,7 @@ export default function UserDetailsPage() {
<Box p="md">
<Group>
<CloseButton onClick={() => router.back()} />
<Title>{user.name}</Title>
<Title>{user.firstName || user.lastName ? `${user.firstName} ${user.lastName}` : user.email}</Title>
Copy link
Member

Choose a reason for hiding this comment

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

Flytt denne logikken til brukeren?

await core.auth0SynchronizationService.populateUserWithFakeData(token.sub, token.email) // Remove when we have real data
const user = await core.auth0SynchronizationService.ensureUserLocalDbIsSynced(token.sub, new Date())
const user: User | null = await core.userService.getById(token.sub)
await core.userService.registerId(token.sub)
Copy link
Member

Choose a reason for hiding this comment

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

Kan vi ikke bare kun ha getById som registrer selv? API-minimalisering

getAll(limit: number, page: number): Promise<User[]>
update(id: UserId, data: Partial<UserWrite>): Promise<User>
searchForUser(query: string, limit: number, page: number): Promise<User[]>
createDummyUser(data: UserWrite, password: string): Promise<User>
Copy link
Member

Choose a reason for hiding this comment

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

Dette virker 100% test-relatert og burde være i et annet API

import type { NotificationPermissionsRepository } from "./notification-permissions-repository"
import type { PrivacyPermissionsRepository } from "./privacy-permissions-repository"
import type { UserRepository } from "./user-repository"

export interface UserService {
getById(id: UserId): Promise<User | null>
getAll(limit: number): Promise<User[]>
getAll(limit: number, offset: number): Promise<User[]>
searchForUser(query: string, limit: number, offset: number): Promise<User[]>
Copy link
Member

Choose a reason for hiding this comment

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

Denne burde lenke til auth0 dokumentasjon over støttet syntax

@@ -254,7 +245,7 @@ export interface PrivacyPermissions {
phoneVisible: Generated<boolean>;
profileVisible: Generated<boolean>;
updatedAt: Generated<Timestamp>;
userId: string;
userId: string | null;
Copy link
Member

Choose a reason for hiding this comment

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

Hvorfor er alle disse satt som | null? Er det ment som on delete set null? Gir mer mening å slette hele privacy-objektet ved sletting/anonymisering.

Merk at vi i ow4 aldri sletter brukere—vi bare anonymiserer.

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.

3 participants