fix: SavePreferences being called multiple times and causing issues #35904
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 5910dc5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 36 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
![kodus-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #35904 +/- ##
===========================================
- Coverage 61.26% 61.26% -0.01%
===========================================
Files 3164 3164
Lines 74757 74757
Branches 16689 16689
===========================================
- Hits 45801 45800 -1
- Misses 25850 25852 +2
+ Partials 3106 3105 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
| 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 }); |
There was a problem hiding this comment.
What changed in the logic here besides moving to a hook?
| currentData: AccountPreferencesData; | ||
| }; | ||
|
|
||
| export const useSavePreferences = ({ dirtyFields, currentData, reset }: useSavePreferencesProps) => { |
There was a problem hiding this comment.
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.
| 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); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
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.
The preferences are being saved multiple times when you press save, this can cause some issues, like when selecting a new language, the page tries to reset the forms while the promise is being processed, causing the language to be saved as the previous value
Issue(s)
CORE-1106
Steps to test or reproduce
This issue happened more consistently with the
PortugueseandBrazilan PortugueselanguagesFurther comments
This pull request addresses an issue where the
SavePreferencesfunction was being called multiple times, leading to potential problems. The changes include the introduction of a new React hook,useSavePreferences, which is designed to handle the saving of user account preferences. This hook usesuseMutationfrom@tanstack/react-queryto interact with the/v1/users.setPreferencesAPI endpoint, processes specific fields likehighlightsanddontAskAgainListbased on dirty fields, and provides user feedback through toast messages.Additionally, a new test suite for the
useSavePreferenceshook has been added using React Testing Library and Jest to ensure that the hook correctly triggers theusers.setPreferencesAPI endpoint when invoked. The preference saving logic in theAccountPreferencesPagecomponent has been refactored to use this new custom hook, simplifying the component and improving code organization by centralizing the saving mechanism.