feat: refactor program dashboard to use react query#827
feat: refactor program dashboard to use react query#827asharma12-sonata wants to merge 63 commits intoopenedx:masterfrom
Conversation
our github actions should point to our release branch
chore: updating the workflows
…extra repositories (openedx#752)
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Maxwell Frank <92897870+MaxFrank13@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…#783) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…edx#784) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
… npm ci triggered by CI
f11b964 to
2164115
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #827 +/- ##
==========================================
- Coverage 98.19% 98.19% -0.01%
==========================================
Files 145 150 +5
Lines 1388 1493 +105
Branches 298 322 +24
==========================================
+ Hits 1363 1466 +103
- Misses 25 27 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| }); | ||
|
|
||
| it('calls logError if the API request fails', async () => { | ||
| it('renders fallback UI when the API request fails', async() => { |
There was a problem hiding this comment.
@asharma12-sonata @MaxFrank13 I might be missing something in this change, but if this unit tests handles the scenario of when the API request fails, shouldn't we expect an error message on the screen, not the ExploreProgramsCTA?
There was a problem hiding this comment.
Agreed! If we look at the PR I created, there is error handling as well as a test that checks for the Alert on failure. We should try to match this using React Query.
With React Query we are expected to add error handling to the queryFn. See the docs here. It looks like the Alert is still set up in the component.
This test is only passing because we are setting a new Error('Nettwork failed') to the error field of the returned data from useProgramsListData. On it's own this is valid because the useQuery hook does return the error (source), however, our component is only using isError which is a boolean and not the actual error object (source).
So we should modify the test to return a boolean in the mock for the isError field rather than an Error object as the error field. Then the test should be modified to test the error behavior (i.e. check that the Alert is rendered)
There was a problem hiding this comment.
Yes, but as Jason called out, that test is verifying that the CTA shows up on error. This contradicts what the component actually does which is render the failure alert when the API request fails.
So the test needs to be modified to reflect what the component actually does. Part of the issue is with the mocking (as I called out in the previous comment)
There was a problem hiding this comment.
Added test which renders alert and no program card when api fails.
| (useProgramsListData as jest.Mock).mockReturnValue({ | ||
| data: [], | ||
| loading: false, | ||
| error: mockError, |
There was a problem hiding this comment.
| error: mockError, | |
| isError: true, |
There was a problem hiding this comment.
Is the update only in your local environment? I don't see the change reflected in this PR
There was a problem hiding this comment.
There is a queryHooks.ts file in data/hooks (here). Should this code live alongside the other React Query hooks?
There was a problem hiding this comment.
Clarifying context:
We have a queryHooks.ts file that lives in the data directory that is at the root of the application. This data directory can be used by any component in the app. Since it has a queryHooks file, this would imply that it is the home for React Query hooks (such as the useInitializeLearnerHome hook) that are used throughout the app.
The question I posed in the PR comment was: should we move the hook you made into this file, rather then keeping it in the api file as we did with the previous pre-React Query app? This would mean moving both the hook and it's test
There was a problem hiding this comment.
I’m not entirely sure about this, but if hooks are required to live in the root hooks folder, I can move it there. Please confirm.
There was a problem hiding this comment.
Unless you had an idea when putting them in ProgramDashboard/data/api.ts then it's probably best they live in the data/queryHooks file. There isn't a "right" or "wrong" here. It's more about making a decision of whether or not to uphold a pattern
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| // Set up a successful mock API response by default | ||
| (useProgramsListData as jest.Mock).mockResolvedValue(mockApiData); |
There was a problem hiding this comment.
This mock should match the others
| (useProgramsListData as jest.Mock).mockResolvedValue(mockApiData); | |
| (useProgramsListData as jest.Mock).mockResolvedValue({ | |
| data: mockApiData, | |
| loading: false, | |
| isError: false, | |
| }); |
| (useProgramsListData as jest.Mock).mockReturnValue({ | ||
| data: mockApiData, | ||
| loading: false, | ||
| error: null, |
There was a problem hiding this comment.
Do we need this mock if we have the beforeEach() mocking it out before every test?
There was a problem hiding this comment.
Removed and changes pushed.
MaxFrank13
left a comment
There was a problem hiding this comment.
Looks good. I had a few questions and suggestions. Also, can we add more screenshots and description similar to the original PR?
| it('calls logError if the API request fails', () => { | ||
| const mockError = new Error('Network failed'); | ||
|
|
||
| (useProgramsListData as jest.Mock).mockReturnValue({ | ||
| data: undefined, | ||
| isLoading: false, | ||
| isError: true, | ||
| error: mockError, | ||
| }); | ||
|
|
||
| const mockContactUrl = 'mock-contact-url'; | ||
| getConfig.mockReturnValue({ | ||
| CONTACT_URL: mockContactUrl, | ||
| }); | ||
|
|
||
| renderComponent(); | ||
|
|
||
| expect(screen.getByRole('link', { name: mockContactUrl })).toBeInTheDocument(); | ||
| expect(screen.queryAllByTestId('program-list-card')).toHaveLength(0); | ||
| expect(logError).toHaveBeenCalledWith(mockError); |
There was a problem hiding this comment.
I realize that you have added this new test rather then modifying the one before it. This is fine but we should either edit/remove the test before this.
| (useProgramsListData as jest.Mock).mockReturnValue({ | ||
| data: [], | ||
| isLoading: false, | ||
| isError: false, |
There was a problem hiding this comment.
If the API request fails, that would mean this would return isError: true, right?
There was a problem hiding this comment.
correct, I think once isError: true is added, then the test should fail because the expect is incorrectly assuming the ExploreProgramsCta will render. I think this test is really just adding more confusion and should be deleted.
There was a problem hiding this comment.
Tests in this file are verifying the useProgramsListData query hook rather then verifying the API helper function. Any tests for the hook should go in queryHooks.test.ts alongside the other query code.
Additionally, there is an api.ts file that lives in data/services/lms/api. This holds all of the API calls to the LMS. There is an api.test.ts that lives alongside this file. It has mocks set up to test API calls. Since we need a test for the ProgramDashboard's api.ts file, it may be easier to move the code to data/services/lms/api. Alternatively, you could just write the test here for the API call and use the LMS API test code as a reference.
Either way, we should make sure the test for fetchProgramsListData is isolated.
There was a problem hiding this comment.
updated and changes pushed
MaxFrank13
left a comment
There was a problem hiding this comment.
Had some additional comments. Also, please include screenshots of Programs being rendered to the dashboard. Also, we should enhance the description to match what we had in the old PR
|
|
||
| describe('fetchProgramsListData', () => { | ||
| it('uses the expected URL to call the endpoint', async () => { | ||
| await fetchProgramsListData(); | ||
|
|
||
| expect(getAuthenticatedHttpClient).toHaveBeenCalled(); | ||
| expect(mockGet).toHaveBeenCalledWith( | ||
| `${mockLMSBaseUrl}/api/dashboard/v0/programs/`, | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
@MaxFrank13 I'm guessing that since this is an api call and not a hook, this should be moved to the containers/ProgramDashboard/data/api.test.ts file, right?
There was a problem hiding this comment.
Yes my understanding is that this test should live in api.test.ts and the test that is in that file should live in queryHooks.test.ts
… for banner image which is updated in api endpoint

This PR includes changes from
#751
This PR adds the programs dashboard in accordance with the above proposal. This is a conversion of the legacy programs dashboard that lives in edx-platform. This PR converts the legacy frontend into a React based frontend that lives under its own route. The route is conditionally rendered based on a new ENABLE_PROGRAM_DASHBOARD environment variable, not to be confused with the ENABLE_PROGRAMS variable, which only handles the rendering of the "Programs" tab. This is done so that operators can choose to either use the legacy frontend or the new React-based frontend.
In order to create a new route in this MFE, the App.jsx file had to be refactored. The LearnerDashboardHeader and FooterSlot were moved out of App.jsx and into index.jsx. This aligns with the way other MFEs are setup. The h1 tag for the app was also moved to the LearnerDashboardHeader so that it would appear on all routes. The Header logic has also been refactored so that the correct tab is highlighted based on the pathname of the page.
All other changes are related to the Program Dashboard itself. The dashboard uses the /api/dashboard/v0/programs/ endpoint to retrieve enrollment data.
Directory structure
Other changes are related to using react query for program dashboard.
Programs Dashboard