-
Notifications
You must be signed in to change notification settings - Fork 183
feat: Add ability to create Legacy Libraries #2551
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
feat: Add ability to create Legacy Libraries #2551
Conversation
|
What's the rationale for implementing this in the MFE when you can't view legacy libraries in the MFE? |
|
@bradenmacdonald As part of our effort to remove legacy studio pages and get rid of studio-frontend, we removed the legacy course & library listing in openedx/openedx-platform#37454, which also removed the ability to create legacy libraries. When we merged that, I made the judgement call that it was OK to remove the ability to create new legacy libraries, because:
But, I was wrong--
So, our options are (a) reinstate all of those legacy studio pages and re-install studio-frontend, or (b) just reinstate the ability to create legacy libs in the MFE, and remove it shortly after the Ulmo cutoff. |
0688d52 to
c362bc2
Compare
|
@kdmccormick Ah, that makes sense.
But I don't think v1 libraries do either? |
|
I didn't think so, but it turns out they do: openedx/openedx-platform#37500 (comment) |
93e9a8a to
a908ef2
Compare
| test('show api error', async () => { | ||
| const user = userEvent.setup(); | ||
| axiosMock.onGet(getStudioHomeApiUrl()).reply(200, studioHomeMock); | ||
| axiosMock.onPost(getContentLibraryV1CreateApiUrl()).reply(400, { | ||
| field: 'Error message', | ||
| }); | ||
| render(<CreateLegacyLibrary />); | ||
|
|
||
| const titleInput = await screen.findByRole('textbox', { name: /library name/i }); | ||
| await user.click(titleInput); | ||
| await user.type(titleInput, 'Test Library Name'); | ||
|
|
||
| const orgInput = await screen.findByRole('combobox', { name: /organization/i }); | ||
| await user.click(orgInput); | ||
| await user.type(orgInput, 'org1'); | ||
| await user.tab(); | ||
|
|
||
| const slugInput = await screen.findByRole('textbox', { name: /library id/i }); | ||
| await user.click(slugInput); | ||
| await user.type(slugInput, 'test_library_slug'); | ||
|
|
||
| fireEvent.click(await screen.findByRole('button', { name: /create/i })); | ||
| await waitFor(async () => { | ||
| expect(axiosMock.history.post.length).toBe(1); | ||
| expect(axiosMock.history.post[0].data).toBe( | ||
| '{"display_name":"Test Library Name","org":"org1","number":"test_library_slug"}', | ||
| ); | ||
| expect(mockNavigate).not.toHaveBeenCalled(); | ||
| const errorMessage = 'Request failed with status code 400{ "field": "Error message" }'; | ||
| expect(await screen.findByRole('alert')).toHaveTextContent(errorMessage); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm struggling to get this test passing, seeking help.
(For context, this PR is adding a new component--CreateLegacyLibrary-- which is essentially a duplicate of the existing CreateLibrary component, but with some tweaks so that it creates legacy libraries instead of V2 libraries. This is a stop-gap solution just to get us through Ulmo without dropping functionality. The CreateLegacyLibrary component will be deleted between Ulmo and Verawood, so I'm doing a lot of code duplication rather than factoring out common logic between the two components.)
Like the rest of this test file, this test case show api error is copied near-verbatim from CreateLibrary.test.tsx. In particular, it is checking that if the backend returns a 400 when creating a legacy library, we get this a little Axios Error message (I know, not a great UX, but this is what CreateLibrary.tsx does too).
The test is failing with:
● <CreateLegacyLibrary /> › show api error
expect(received).toBe(expected) // Object.is equality
Expected: 1
Received: 0
on:
expect(axiosMock.history.post.length).toBe(1);The issue seems to be that the test relies on await screen.findByRole('alert') in order to determine whether the error banner has appeared. This works fine in CreateLibrary, but in CreateLegacyLibrary, we have this big omnipresent warning alert which tells users that You are creating content in a deprecated format. Because the alert always exists, findByRole('alert') is returning false-positive immediately rather than blocking until the Axios alert appears.
I've tried updating it to await.screen.findByText(/axios error/i), but that doesn't seem to match the Axios error banner, for some reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test passes if we change it to
await user.click(await screen.findByRole('button', { name: /create/i }));
await waitFor(async () => {
expect(axiosMock.history.post.length).toBe(1);
expect(axiosMock.history.post[0].data).toBe(
'{"display_name":"Test Library Name","org":"org1","number":"test_library_slug"}',
);
expect(mockNavigate).not.toHaveBeenCalled();
});
await screen.findByText('Request failed with status code 400');the findByText will fail the test if it doesn't find it, verified by seeing the test fail when using
await screen.findByText('Request failed with status code 9001');for that line instead
src/library-authoring/create-legacy-library/CreateLegacyLibrary.test.tsx
Outdated
Show resolved
Hide resolved
Is there any reason to believe that anyone has ever done this, however? I would call that undocumented at best, and perhaps even unsupported. (Still in favor of this PR, just wanting context for future DEPRs etc.) |
49db107 to
e1d66e2
Compare
e1d66e2 to
cb448e7
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2551 +/- ##
=======================================
Coverage 94.73% 94.74%
=======================================
Files 1213 1218 +5
Lines 27222 27300 +78
Branches 6141 6161 +20
=======================================
+ Hits 25790 25866 +76
- Misses 1361 1363 +2
Partials 71 71 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I tested this PR and can confirm functionality works as expected. I was able to create a legacy library without any issue. The warning shows up as expected on the legacy library creation form. |
|
@brian-smith-tcril this is all set for a code quality/best practice review. |
90e03ee to
1ce1dfd
Compare
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Left some non-blocking suggestions to switch from fireEvent to userEvent in some tests.
src/library-authoring/create-legacy-library/CreateLegacyLibrary.test.tsx
Outdated
Show resolved
Hide resolved
src/library-authoring/create-legacy-library/CreateLegacyLibrary.test.tsx
Outdated
Show resolved
Hide resolved
src/library-authoring/create-legacy-library/CreateLegacyLibrary.test.tsx
Outdated
Show resolved
Hide resolved
src/library-authoring/create-legacy-library/CreateLegacyLibrary.test.tsx
Outdated
Show resolved
Hide resolved
src/library-authoring/create-legacy-library/CreateLegacyLibrary.test.tsx
Outdated
Show resolved
Hide resolved
1ce1dfd to
a63321c
Compare
It has not been cleared yet by UX/Product. Removing the warning brings us to parity with the old legacy Create Library page. We may add this back shortly once UX/Product has reviewed it.
a63321c to
8ddb6aa
Compare
|
I have temporarily removed the deprecation warning because I didn't have time to get it reviewed by Product/UX today: 8ddb6aa. That warning would have been net-new, so this PR is still valid for parity with the legacy frontend. I'll open a new PR shortly for the deprecation warning and get Product/UX's eyes on it tomorrow and see if it makes sense to land for Ulmo. |
This adds a CreateLegacyLibrary component. It functions the same as CreateLibrary, but it calls the V1 (legacy) creation REST API rather the V2 (new/beta) REST API. This reinstates, in the MFE, something that was possible using the legacy frontend until it was prematurely removed by openedx/openedx-platform#37454. We plan to re-remove this ability between Ulmo and Verawood as part of: openedx/openedx-platform#32457. So, we have intentionally avoided factoring out common logic between CreateLibrary and CreateLegacyLibrary, ensuring that the latter remains easy to remove and clean up.
This adds a CreateLegacyLibrary component. It functions the same as CreateLibrary, but it calls the V1 (legacy) creation REST API rather the V2 (new/beta) REST API. This reinstates, in the MFE, something that was possible using the legacy frontend until it was prematurely removed by openedx/openedx-platform#37454. We plan to re-remove this ability between Ulmo and Verawood as part of: openedx/openedx-platform#32457. So, we have intentionally avoided factoring out common logic between CreateLibrary and CreateLegacyLibrary, ensuring that the latter remains easy to remove and clean up.
Background / Supporting Info
As part of our effort to remove legacy studio pages and get rid of studio-frontend, we removed the legacy course & library listing in openedx/openedx-platform#37454. This removed the ability to create legacy libraries. At the time of review we deemed this to be OK, but upon further reflection, we were mistaken, for at least two different reasons:
So, we need to reinstate the ability to create legacy libraries for Ulmo. Rather than reintroduce all the legacy studio frontend code, we're opting to just reinstate the ability to create legacy libraries using frontend-app-authoring.
We will be able to remove the ability to create legacy libraries before Verawood. So, rather than factoring out common logic between CreateLibrary and CreateLegacyLibrary, this PR aims to just duplicate and modify CreateLibrary logic, such that it will be very easy to delete CreateLegacyLibrary.
Description
This adds a CreateLegacyLibrary component. It functions the same as CreateLibrary, but it calls the V1 (legacy) creation REST API rather the V2 (new/beta) REST API.
Testing instructions
Getting there:
Test happy path:
Test ID conflict:
Test v2 CreateLibrary unchanged:
Other information
Needs to merge ASAP, definitely before Ulmo cutoff
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).propTypes,defaultProps, andinjectIntlpatterns are not used in any new or modified code.src/testUtils.tsx(specificallyinitializeMocks)apiHooks.tsin this repo for examples.messages.tsfiles have adescriptionfor translators to use.../. To import from parent folders, use@src, e.g.import { initializeMocks } from '@src/testUtils';instead offrom '../../../../testUtils'