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

chore: Migrate updateProfile to ts #3607

Merged
merged 10 commits into from
Jan 2, 2025
Merged

chore: Migrate updateProfile to ts #3607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f30e40a
chore: Migrate updateProfile to ts
suejung-sentry Dec 23, 2024
ecac085
Merge remote-tracking branch 'origin/main' into sshin/2861
suejung-sentry Jan 2, 2025
2eecb38
fix tests
suejung-sentry Jan 2, 2025
7cbf5b2
missed some fields
suejung-sentry Jan 2, 2025
a72cd15
fix tests
suejung-sentry Jan 2, 2025
8d47ccd
cleanup
suejung-sentry Jan 2, 2025
419651b
Merge remote-tracking branch 'origin/main' into sshin/2861
suejung-sentry Jan 2, 2025
8cba744
make schema validation stricter
suejung-sentry Jan 2, 2025
3aa56b4
Merge remote-tracking branch 'origin/main' into sshin/2861
suejung-sentry Jan 2, 2025
db6d0b6
fix tests
suejung-sentry Jan 2, 2025
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
33 changes: 25 additions & 8 deletions src/pages/AccountSettings/tabs/Admin/DetailsSection/DetailsSection.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ beforeEach(() => {
server.resetHandlers()
})

afterEach(() => {
vi.clearAllMocks()
})

afterAll(() => {
server.close()
})
Expand All @@ -43,7 +47,7 @@ describe('DetailsSection', () => {
const mutate = vi.fn()
const addNotification = vi.fn()

useAddNotification.mockReturnValue(addNotification)
vi.mocked(useAddNotification).mockReturnValue(addNotification)
server.use(
graphql.mutation('UpdateProfile', (info) => {
mutate(info.variables)
Expand All @@ -52,15 +56,23 @@ describe('DetailsSection', () => {
data: {
updateProfile: {
me: {
username: 'donald duck',
email: info.variables.input.email
? info.variables.input.email
: '[email protected]',
name: info.variables.input.name
? info.variables.input.name
: 'donald duck',
avatarUrl: 'http://127.0.0.1/avatar-url',
onboardingCompleted: false,
onboardingCompleted: true,
privateAccess: null,
businessEmail: null,
user: {
name: info.variables.input.name
? info.variables.input.name
: 'donald duck',
username: 'donald duck',
avatarUrl: 'http://127.0.0.1/avatar-url',
avatar: 'http://127.0.0.1/avatar-url',
student: false,
studentCreatedAt: null,
studentUpdatedAt: null,
},
},
},
},
Expand Down Expand Up @@ -218,10 +230,15 @@ describe('DetailsSection', () => {
)
})
})

describe('when mutation is not successful', () => {
it('adds an error notification', async () => {
const { user, addNotification } = setup()
server.use(
graphql.mutation('UpdateProfile', () => {
return HttpResponse.error()
})
)

render(<DetailsSection name="donald duck" email="[email protected]" />, {
wrapper,
})
Expand Down
13 changes: 12 additions & 1 deletion src/pages/AccountSettings/tabs/Profile/NameEmailCard/NameEmailCard.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,18 @@ describe('NameEmailCard', () => {
updateProfile: {
me: {
email: json.variables.input.email || '',
user: { name: json.variables.input.name || '' },
privateAccess: null,
onboardingCompleted: true,
businessEmail: null,
user: {
name: json.variables.input.name || '',
username: 'test',
avatarUrl: 'http://127.0.0.1/avatar-url',
avatar: 'http://127.0.0.1/avatar-url',
student: false,
studentCreatedAt: null,
studentUpdatedAt: null,
},
},
},
},
Expand Down
61 changes: 50 additions & 11 deletions src/services/user/useUpdateProfile.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ const user = {
name: 'terry',
avatarUrl: 'http://127.0.0.1/avatar-url',
onboardingCompleted: false,
privateAccess: true,
businessEmail: null,
student: false,
studentCreatedAt: null,
studentUpdatedAt: null,
}

const queryClient = new QueryClient({
Expand Down Expand Up @@ -50,9 +55,19 @@ describe('useUpdateProfile', () => {
server.use(
graphql.mutation('UpdateProfile', (info) => {
const newUser = {
...user,
name: info.variables.input.name,
email: info.variables.input.email,
Copy link
Contributor Author

@suejung-sentry suejung-sentry Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

the existing mocks didn't match the current shape of response so updated

onboardingCompleted: user.onboardingCompleted,
privateAccess: user.privateAccess,
businessEmail: user.businessEmail,
user: {
name: info.variables.input.name,
username: user.username,
avatarUrl: user.avatarUrl,
avatar: user.avatarUrl,
student: user.student,
studentCreatedAt: user.studentCreatedAt,
studentUpdatedAt: user.studentUpdatedAt,
},
}

return HttpResponse.json({
Expand Down Expand Up @@ -88,11 +103,19 @@ describe('useUpdateProfile', () => {

await waitFor(() =>
expect(result.current.data).toStrictEqual({
avatarUrl: 'http://127.0.0.1/avatar-url',
email: '[email protected]',
name: 'new name',
privateAccess: true,
onboardingCompleted: false,
username: 'TerrySmithDC',
businessEmail: null,
user: {
name: 'new name',
username: 'TerrySmithDC',
avatarUrl: 'http://127.0.0.1/avatar-url',
avatar: 'http://127.0.0.1/avatar-url',
student: false,
studentCreatedAt: null,
studentUpdatedAt: null,
},
})
)
})
Expand All @@ -108,11 +131,19 @@ describe('useUpdateProfile', () => {

await waitFor(() =>
expect(result.current.data).toStrictEqual({
avatarUrl: 'http://127.0.0.1/avatar-url',
email: '[email protected]',
name: 'new name',
privateAccess: true,
onboardingCompleted: false,
username: 'TerrySmithDC',
businessEmail: null,
user: {
name: 'new name',
username: 'TerrySmithDC',
avatarUrl: 'http://127.0.0.1/avatar-url',
avatar: 'http://127.0.0.1/avatar-url',
student: false,
studentCreatedAt: null,
studentUpdatedAt: null,
},
})
)
})
Expand Down Expand Up @@ -153,11 +184,19 @@ describe('useUpdateProfile', () => {

await waitFor(() =>
expect(result.current.data).toStrictEqual({
avatarUrl: 'http://127.0.0.1/avatar-url',
email: '[email protected]',
name: 'new name',
privateAccess: true,
onboardingCompleted: false,
username: 'TerrySmithDC',
businessEmail: null,
user: {
name: 'new name',
username: 'TerrySmithDC',
avatarUrl: 'http://127.0.0.1/avatar-url',
avatar: 'http://127.0.0.1/avatar-url',
student: false,
studentCreatedAt: null,
studentUpdatedAt: null,
},
})
)
})
Expand Down
77 changes: 73 additions & 4 deletions src/services/user/useUpdateProfile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,68 @@
import { useMutation, useQueryClient } from '@tanstack/react-query'
import { z } from 'zod'

import config from 'config'

import Api from 'shared/api'
import { NetworkErrorObject, rejectNetworkError } from 'shared/api/helpers'

import { GoalsSchema, TypeProjectsSchema } from './useUser'

const CurrentUserFragment = z.object({
Copy link
Contributor

Choose a reason for hiding this comment

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

can we export this from useUser and just use one?

Looks like some of the values there are non nullish which match the api (ex: onboardingCompleted)

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 get what you mean, the duplication is not great. Unfortunately I feel like unless we share the graphQL query/fragment across both hooks too, we should keep the response zod schema validations independent so the given hook's paired query + response validation zod doesn't diverge. For example, termsAgreement is part of the useUser query but omitted here (perhaps that's its own oversight) so can't share a zod schema validation that includes that field.

Re: non-nullish - yeah, on that one I left it loose for the client to handle these values being undefined (and less trust in the api). I made it a bit stricter now to match that in useUser, and updated the test mocks too.

email: z.string().nullable(),
privateAccess: z.boolean().nullable(),
onboardingCompleted: z.boolean(),
businessEmail: z.string().nullable(),
user: z.object({
name: z.string().nullable(),
username: z.string(),
avatarUrl: z.string(),
avatar: z.string(),
student: z.boolean(),
studentCreatedAt: z.string().nullable(),
studentUpdatedAt: z.string().nullable(),
}),
trackingMetadata: z
.object({
service: z.string(),
ownerid: z.number(),
serviceId: z.string(),
plan: z.string().nullable(),
staff: z.boolean().nullable(),
hasYaml: z.boolean(),
bot: z.string().nullable(),
delinquent: z.boolean().nullable(),
didTrial: z.boolean().nullable(),
planProvider: z.string().nullable(),
planUserCount: z.number().nullable(),
createdAt: z.string().nullable(),
updatedAt: z.string().nullable(),
profile: z
.object({
createdAt: z.string(),
otherGoal: z.string().nullable(),
typeProjects: TypeProjectsSchema,
goals: GoalsSchema,
})
.nullable(),
})
.nullish(),
})

const UpdateProfileResponseSchema = z.object({
updateProfile: z
.object({
me: CurrentUserFragment.nullish(),
error: z
.discriminatedUnion('__typename', [
z.object({
__typename: z.literal('ValidationError'),
}),
])
.nullish(),
})
.nullish(),
})

const currentUserFragment = `
fragment CurrentUserFragment on Me {
Expand All @@ -26,7 +86,6 @@ fragment CurrentUserFragment on Me {
plan
staff
hasYaml
service
bot
delinquent
didTrial
Expand Down Expand Up @@ -72,10 +131,20 @@ export function useUpdateProfile({ provider }: { provider: string }) {
email,
},
},
}).then((res) => res?.data?.updateProfile?.me)
}).then((res) => {
const parsedData = UpdateProfileResponseSchema.safeParse(res.data)
if (!parsedData.success) {
return rejectNetworkError({
status: 404,
data: {},
dev: 'useUpdateProfile - 404 failed to parse',
} satisfies NetworkErrorObject)
}
return parsedData.data.updateProfile?.me
})
},
onSuccess: (user) => {
queryClient.setQueryData(['currentUser', provider], () => user)
onSuccess: (currentUser) => {
queryClient.setQueryData(['currentUser', provider], () => currentUser)

if (config.IS_SELF_HOSTED) {
queryClient.invalidateQueries(['SelfHostedCurrentUser'])
Expand Down
4 changes: 2 additions & 2 deletions src/services/user/useUser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { z } from 'zod'
import Api from 'shared/api'
import { NetworkErrorObject } from 'shared/api/helpers'

const TypeProjectsSchema = z.array(
export const TypeProjectsSchema = z.array(
z.union([
z.literal('PERSONAL'),
z.literal('YOUR_ORG'),
Expand All @@ -14,7 +14,7 @@ const TypeProjectsSchema = z.array(
])
)

const GoalsSchema = z.array(
export const GoalsSchema = z.array(
z.union([
z.literal('STARTING_WITH_TESTS'),
z.literal('IMPROVE_COVERAGE'),
Expand Down
Loading