-
Notifications
You must be signed in to change notification settings - Fork 11.9k
fix: SavePreferences being called multiple times and causing issues #35904
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
Changes from all commits
656e543
8cc8566
ec7c7f5
9df1b67
5910dc5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@rocket.chat/meteor": patch | ||
--- | ||
|
||
Fixes issue when trying to save preferences in the user preferences page |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import { mockAppRoot } from '@rocket.chat/mock-providers'; | ||
import { renderHook, waitFor } from '@testing-library/react'; | ||
|
||
import { useSavePreferences } from './useSavePreferences'; | ||
import type { AccountPreferencesData } from '../../views/account/preferences/useAccountPreferencesValues'; | ||
|
||
const mockSetPreferencesEndpoint = jest.fn(); | ||
|
||
describe('useSavePreferences', () => { | ||
it('should call setPreferencesEndpoint with correct data', async () => { | ||
const dirtyFields = { language: true }; | ||
const { result } = renderHook(() => useSavePreferences({ dirtyFields, reset: jest.fn(), currentData: {} }), { | ||
wrapper: mockAppRoot().withEndpoint('POST', '/v1/users.setPreferences', mockSetPreferencesEndpoint).build(), | ||
}); | ||
|
||
const formData: AccountPreferencesData = { language: 'en' }; | ||
await waitFor(() => result.current(formData)); | ||
|
||
expect(mockSetPreferencesEndpoint).toHaveBeenCalledTimes(1); | ||
}); | ||
}); | ||
Comment on lines
+9
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be an e2e test instead. I wasn't able to make this test fail, even by copying the old logic into the new hook. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
import { useEndpoint } from '@rocket.chat/ui-contexts'; | ||
import { useMutation } from '@tanstack/react-query'; | ||
import type { UseFormReset } from 'react-hook-form'; | ||
import { useTranslation } from 'react-i18next'; | ||
|
||
import { getDirtyFields } from '../../lib/getDirtyFields'; | ||
import { dispatchToastMessage } from '../../lib/toast'; | ||
import type { AccountPreferencesData } from '../../views/account/preferences/useAccountPreferencesValues'; | ||
|
||
type useSavePreferencesProps = { | ||
dirtyFields: Partial<Record<keyof AccountPreferencesData, boolean | boolean[]>>; | ||
reset: UseFormReset<AccountPreferencesData>; | ||
currentData: AccountPreferencesData; | ||
}; | ||
|
||
export const useSavePreferences = ({ dirtyFields, currentData, reset }: useSavePreferencesProps) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would advise against refactoring the logic into a separate hook in a fix, unless it would be used in another component. Makes it hard to understand what the root cause and fix was. |
||
const setPreferencesEndpoint = useEndpoint('POST', '/v1/users.setPreferences'); | ||
const { t } = useTranslation(); | ||
|
||
const setPreferencesAction = useMutation({ | ||
mutationFn: ({ | ||
data, | ||
}: { | ||
data: AccountPreferencesData & { dontAskAgainList?: { action: string; label: string }[] } & { highlights?: string[] }; | ||
}) => setPreferencesEndpoint({ data }), | ||
onSuccess: () => { | ||
dispatchToastMessage({ type: 'success', message: t('Preferences_saved') }); | ||
}, | ||
onError: (error) => { | ||
dispatchToastMessage({ type: 'error', message: error }); | ||
MartinSchoeler marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}, | ||
onSettled: () => reset(currentData, { keepValues: true }), | ||
}); | ||
|
||
return async (formData: AccountPreferencesData) => { | ||
const { highlights, dontAskAgainList, ...data } = getDirtyFields(formData, dirtyFields); | ||
if (highlights || highlights === '') { | ||
Object.assign(data, { | ||
highlights: | ||
typeof highlights === 'string' && | ||
highlights | ||
.split(/,|\n/) | ||
.map((val) => val.trim()) | ||
.filter(Boolean), | ||
}); | ||
} | ||
|
||
if (dontAskAgainList) { | ||
const list = | ||
Array.isArray(dontAskAgainList) && dontAskAgainList.length > 0 | ||
? dontAskAgainList.map(([action, label]) => ({ action, label })) | ||
MartinSchoeler marked this conversation as resolved.
Show resolved
Hide resolved
|
||
: []; | ||
Object.assign(data, { dontAskAgainList: list }); | ||
} | ||
setPreferencesAction.mutateAsync({ data }); | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
import { ButtonGroup, Button, Box, Accordion } from '@rocket.chat/fuselage'; | ||
import { useToastMessageDispatch, useSetting, useTranslation, useEndpoint } from '@rocket.chat/ui-contexts'; | ||
import { useMutation } from '@tanstack/react-query'; | ||
import { useSetting, useTranslation } from '@rocket.chat/ui-contexts'; | ||
import { useId } from 'react'; | ||
import type { ReactElement } from 'react'; | ||
import { FormProvider, useForm } from 'react-hook-form'; | ||
|
@@ -13,14 +12,12 @@ import PreferencesMyDataSection from './PreferencesMyDataSection'; | |
import PreferencesNotificationsSection from './PreferencesNotificationsSection'; | ||
import PreferencesSoundSection from './PreferencesSoundSection'; | ||
import PreferencesUserPresenceSection from './PreferencesUserPresenceSection'; | ||
import type { AccountPreferencesData } from './useAccountPreferencesValues'; | ||
import { useAccountPreferencesValues } from './useAccountPreferencesValues'; | ||
import { Page, PageHeader, PageScrollableContentWithShadow, PageFooter } from '../../../components/Page'; | ||
import { getDirtyFields } from '../../../lib/getDirtyFields'; | ||
import { useSavePreferences } from '../../../hooks/account/useSavePreferences'; | ||
|
||
const AccountPreferencesPage = (): ReactElement => { | ||
const t = useTranslation(); | ||
const dispatchToastMessage = useToastMessageDispatch(); | ||
const dataDownloadEnabled = useSetting('UserData_EnableDownload'); | ||
const preferencesValues = useAccountPreferencesValues(); | ||
|
||
|
@@ -34,41 +31,7 @@ const AccountPreferencesPage = (): ReactElement => { | |
|
||
const currentData = watch(); | ||
|
||
const setPreferencesEndpoint = useEndpoint('POST', '/v1/users.setPreferences'); | ||
const setPreferencesAction = useMutation({ | ||
mutationFn: setPreferencesEndpoint, | ||
onSuccess: () => { | ||
dispatchToastMessage({ type: 'success', message: t('Preferences_saved') }); | ||
}, | ||
onError: (error) => { | ||
dispatchToastMessage({ type: 'error', message: error }); | ||
}, | ||
onSettled: () => reset(currentData), | ||
}); | ||
|
||
const handleSaveData = async (formData: AccountPreferencesData) => { | ||
const { highlights, dontAskAgainList, ...data } = getDirtyFields(formData, dirtyFields); | ||
if (highlights || highlights === '') { | ||
Object.assign(data, { | ||
highlights: | ||
typeof highlights === 'string' && | ||
highlights | ||
.split(/,|\n/) | ||
.map((val) => val.trim()) | ||
.filter(Boolean), | ||
}); | ||
} | ||
|
||
if (dontAskAgainList) { | ||
const list = | ||
Array.isArray(dontAskAgainList) && dontAskAgainList.length > 0 | ||
? dontAskAgainList.map(([action, label]) => ({ action, label })) | ||
: []; | ||
Object.assign(data, { dontAskAgainList: list }); | ||
} | ||
|
||
setPreferencesAction.mutateAsync({ data }); | ||
}; | ||
const handleSaveData = useSavePreferences({ dirtyFields, currentData, reset }); | ||
Comment on lines
-37
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What changed in the logic here besides moving to a hook? |
||
|
||
const preferencesFormId = useId(); | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.