feat: Migrate pages and resources to React Query - #3222
Conversation
- Migrates the main pages of pages and resources - Migrates the Settings modal of: ORA, Progress, Teams and Wiki
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @ChrisChV! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3222 +/- ##
==========================================
+ Coverage 95.93% 96.02% +0.09%
==========================================
Files 1397 1393 -4
Lines 33622 33589 -33
Branches 7701 7711 +10
==========================================
- Hits 32256 32255 -1
+ Misses 1323 1291 -32
Partials 43 43 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
Thanks for taking this on!
| courseApps: sortedCourseApps || [], | ||
| courseAppsStatus, |
There was a problem hiding this comment.
Are these used only on the "Pages & Resources" page? If so, I think they should be in their own CourseAppsContext rather than in the general CourseAuthoringContext.
We don't want to load the course apps on every page.
| // courseApps is the array reference held by the React Query cache; sort a copy | ||
| // so we don't mutate it during render (StrictMode double-renders can otherwise | ||
| // produce inconsistent results between the two passes). | ||
| const sortedCourseApps = courseApps ? | ||
| [...courseApps].sort((firstEl, secondEl) => ( | ||
| COURSE_APPS_ORDER.indexOf(firstEl.id) - COURSE_APPS_ORDER.indexOf(secondEl.id) | ||
| )) : | ||
| courseApps; |
There was a problem hiding this comment.
Do we need to have both courseApps and sortedCourseApps?
It would be simpler if you move this sorting into the getCourseApps API function directly, so it's always sorted everywhere. There's no harm in always sorting the array. And then you can sort it in place without needing to make a copy of it.
And in modern JS you can just write courseApps.toSorted(...) instead of [...courseApps].sort(...) although either is fine.
| failureReason: courseAppsError, | ||
| } = useCourseApps(courseId); | ||
|
|
||
| let courseAppsStatus: RequestStatusType = RequestStatus.SUCCESSFUL; |
There was a problem hiding this comment.
Can we just use the react query status property of the query instead of a custom RequestStatus enum here?
| /** | ||
| * Get's advanced setting for a course. | ||
| */ | ||
| export async function getCourseAdvancedSettings( | ||
| courseId: string, | ||
| settings: string[], | ||
| ): Promise<any> { | ||
| const { data } = await getAuthenticatedHttpClient() | ||
| .get(`${getCourseAdvancedSettingsApiUrl()}/${courseId}`, { | ||
| params: { | ||
| filter_fields: settings.map(snakeCase).join(','), | ||
| }, | ||
| }); | ||
|
|
||
| return camelCaseObject(data); | ||
| } | ||
|
|
||
| /** | ||
| * Get's advanced setting for a course. | ||
| */ | ||
| export async function updateCourseAdvancedSettings( | ||
| courseId: string, | ||
| setting: string, | ||
| value: any, | ||
| ): Promise<Record<string, any>> { | ||
| const { data } = await getAuthenticatedHttpClient() | ||
| .patch(`${getCourseAdvancedSettingsApiUrl()}/${courseId}`, { [snakeCase(setting)]: { value } }); | ||
|
|
||
| return camelCaseObject(data); | ||
| } |
There was a problem hiding this comment.
I see you've removed the existing implementations of getCourseAdvancedSettings and updateCourseAdvancedSettings from src/pages-and-resources/data/api.js which is great, but there's still duplicate implementations of them both in src/advanced-settings/data/api.ts so please combine those with your new API implementation too.
| */ | ||
| export const useCourseAdvancedSettings = (courseId: string, settings: string[]) => ( | ||
| useQuery({ | ||
| queryKey: ['courseSettings', courseId], |
There was a problem hiding this comment.
This is the same queryKey as useCourseSettings above, which is going to cause bugs.
Also, the settings filter must be in the query key, or we'll have even more bugs.
| queryKey: ['courseSettings', courseId], | |
| queryKey: ['courseAdvancedSettings', courseId, settings], |
| const { | ||
| data: courseApps, | ||
| isPending: courseAppsIsPending, | ||
| failureReason: courseAppsError, |
There was a problem hiding this comment.
It's better to use error instead of failureReason, because failureReason resets to null when a retry begins, and it will temporarily show a success with courseApps being undefined, whereas error will be preserved until the new fetch succeeds.
| // oxlint-disable-next-line @typescript-eslint/await-thenable - this dispatch() IS returning a promise. | ||
| success = await dispatch(updateAppStatus(courseId, appInfo.id, values.enabled)); | ||
| if (!appInfo) { | ||
| return; |
There was a problem hiding this comment.
Shouldn't we display an error in this case?
| }, 2000); | ||
| } | ||
| }, [resetStatusRequestStatus]); | ||
| }, [updateSettingsMutation]); |
There was a problem hiding this comment.
| }, [updateSettingsMutation]); | |
| }, [updateSettingsMutation.status]); |
The mutation object can change for various reasons; it's better to just depend on the status if that's what's relevant to the effect.
| updateSettingsMutation, | ||
| deleteSettingsMutation, |
There was a problem hiding this comment.
| updateSettingsMutation, | |
| deleteSettingsMutation, | |
| updateSettingsMutation.status, | |
| deleteSettingsMutation.status, |
| setTimeout(() => { | ||
| dispatch(updateResetStatus({ status: '' })); | ||
| updateSettingsMutation.reset(); | ||
| }, 2000); |
There was a problem hiding this comment.
The effect should have a cleanup function that clears the timer.
4 participants
Description
courseAppsandXpertSettingsmodels.Supporting information
Testing instructions
Content>Pages & ResourcesLearning Assistant
LEARNING_ASSISTANT_AVAILABLE = Trueto yourcms/envs/private.pyLive
ORA
Proctoring
ENABLE_PROCTORED_EXAMS = Trueto yourcms/envs/private.pyProgress
Teams
teams.enable_teams_appactivated for Everyone.Wiki
Xpert
summaryhook.summaryhook_summaries_configurationactivated for Everyone.Best Practices Checklist
We're trying to move away from some deprecated patterns in this codebase. Please
check if your PR meets these recommendations before asking for a review:
.ts,.tsx).propTypesanddefaultPropsin any new or modified code.src/testUtils.tsx(specificallyinitializeMocks)apiHooks.tsin this repo for examples.messages.tsfiles have adescriptionfor translators to use.../in import paths. To import from parent folders, use@src, e.g.import { initializeMocks } from '@src/testUtils';instead offrom '../../../../testUtils'