feat: Add lib v2/legacy tabs in studio home #1050

yusuf-musleh · 2024-05-27T17:20:29Z

Description

This PR adds a new configuration flag that shows/hides tabs in studio home along with some new functionality around to V1 and V2 Libraries.

When the new LIBRARY_MODE flag is set to "mixed" (default in dev) it will show "Libraries" and "Legacy Libraries" tabs that correspond to v1 and v2 tabs respectively:

When the new LIBRARY_MODE flag is set to "v1 only" (default in production) or "v2 only", only one tab "Libraries" is shown and only the respective libraries are fetched when the tab is clicked.

In addition to the above changes, the URL/route now updates when clicking on the tabs, and navigating to it directly would open up that tab as well as a new placeholder page that you will be redirected to when clicking on a v2 library if the library authoring MFE is not enabled:

Supporting information

Related Tickets:

Depends on: feat: Use default pagination class for v2 lib views edx-platform#34879
Related Issue: Libraries Tab of Studio Home (v2) #1033

Testing instructions

Run this PR's branch in your local env
Make sure you have the feat: Use default pagination class for v2 lib views edx-platform#34879 running locally
Make sure you have the studio.library_authoring_mfe waffle flag enabled in the
The "Libraries" and "Legacy Libraries" tabs appear in the studio home page
When clicking on the "Libraries" shows:
1. v2 libraries are shown
2. They should be paginated (you should see "Show x out of y", to further confirm pagination is working, set the default pageSize in https://github.com/openedx/frontend-app-course-authoring/pull/1050/files#diff-8eef2c5f738aa3151262ca0ca6634eb16c533410f9286ce96c0a58061590bac4R57 to a lower number, and confirm going through multiple pages of results)
3. The route in the URL changes to /libraries
4. When refreshing the page, it should keep you on the "Libraries" tab
5. When clicking on a v2 library it takes you to the library authoring mfe for that v2 library
6. When clicking on "New Library" button it should take you to the library authoring mfe to create a new v2 library
7. When disabling the studio.library_authoring_mfe waffle flag, and then clicking on a v2 library it should take you to the placeholder page (shown in screenshot above), the url should reflect the clicked v2 library (eg: /library/lib:SampleTaxonomyOrg1:AL1)
8. The same should happen when clicking on "New Library" when studio.library_authoring_mfe waffle flag is disabled, it should redirect you to the placeholder page
When clicking on the "Legacy Libraries" tab:
1. v1 libraries are shown
2. The route in the URL changes to /libraries-v1
3. When refreshing the page it should keep you on the "Legacy Libraries" tab
4. When clicking on a v1 library it takes you to the legacy studio UI for the clicked library
5. Since the default LIBRARY_MODE is "mixed", clicking on "New Library" should still take you to the library authoring mfe to create a new v2 library
Change the LIBRARY_MODE flag to "v2 only" and restart the course-authoring container
1. Confirm that there is only 1 tab "Library" that shows up
2. When clicked it shows v2 libraries
3. The route changes to /libraries
4. When clicking on "New Library" it takes you to the library authoring mfe if enabled to create new v2 libraries otherwise it takes you to a placeholder page
Change the LIBRARY_MODE flag to "v1 only" and restart the course-authoring container
1. Confirm that there is only 1 tab "Library" that shows up
2. When clicked it shows v1 libraries
3. The route changes to /libraries-v1
4. When clicking "New Library" it takes you to the legacy UI to create a new v1 library

Private-ref: FAL-3751

openedx-webhooks · 2024-05-27T17:20:33Z

Thanks for the pull request, @yusuf-musleh! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

src/studio-home/tabs-section/index.jsx

src/studio-home/data/api.js

bradenmacdonald · 2024-05-27T17:38:41Z

@yusuf-musleh Can you check if there's a way to update the URL, so when I click on the "Libraries" tab, the URL changes from /home to /libraries or /home?tab=libraries, and I can bookmark and return to that view?

yusuf-musleh · 2024-05-28T18:31:25Z

Can you check if there's a way to update the URL, so when I click on the "Libraries" tab, the URL changes from /home to /libraries or /home?tab=libraries, and I can bookmark and return to that view?

@bradenmacdonald I've implemented this here in 17a5070. The url path now updates with /libraries or /legacy-libraries and /home when the tabs change. They also open the respective tab when being accessed with the path directly and supports the browsers back/forward keys.

I opted for paths rather than query params as react-router would always refresh the page when dealing/changing query params (I've tried a bunch of different approaches), as opposed to changing the paths when the routes point to the same component which doesn't cause a refresh.

yusuf-musleh · 2024-06-03T16:49:26Z

@bradenmacdonald I had to temporarily remove the ts/tsx related code to get the tests to run (da189c1 and 14933d2). I assume once the new frontend-build is merged this will no longer be an issue.

bradenmacdonald · 2024-06-03T16:53:23Z

@yusuf-musleh Yes, that's right.

codecov · 2024-06-04T13:03:52Z

Codecov Report

Attention: Patch coverage is 93.82716% with 5 lines in your changes missing coverage. Please review.

Project coverage is 92.59%. Comparing base (e2ed3bc) to head (09e3979).

Files	Patch %	Lines
...tudio-home/tabs-section/libraries-v2-tab/index.jsx	80.95%	3 Missing and 1 partial ⚠️
src/studio-home/tabs-section/index.jsx	96.42%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1050   +/-   ##
=======================================
  Coverage   92.59%   92.59%           
=======================================
  Files         708      710    +2     
  Lines       12614    12683   +69     
  Branches     2768     2797   +29     
=======================================
+ Hits        11680    11744   +64     
- Misses        899      903    +4     
- Partials       35       36    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rpenido · 2024-06-04T18:06:17Z

Hi @yusuf-musleh! Just to note that I think you should skip the tests from LibraryV2Placeholder.jsx (using /* istanbul ignore next */) as this file will be removed in the PR that adds the Library Home page.

yusuf-musleh · 2024-06-04T20:00:30Z

@rpenido Good point! Thanks for the pointer, I've added it.

pomegranited

Hi @yusuf-musleh , I found a few things to fix, but this is working fine in general.

Feel free to push back on anything :)

pomegranited · 2024-06-06T02:29:12Z

src/index.jsx

@@ -52,6 +53,9 @@ const App = () => {
    createRoutesFromElements(
      <Route>
        <Route path="/home" element={<StudioHome />} />
+        <Route path="/libraries" element={<StudioHome />} />
+        <Route path="/legacy-libraries" element={<StudioHome />} />


nit: could this path mention v1 instead of "legacy"? I don't have strong feelings about this, just don't like to see words like "old" or "legacy" (or "new") in URLs.

Suggested change

<Route path="/legacy-libraries" element={<StudioHome />} />

<Route path="/libraries-v1" element={<StudioHome />} />

I agree, it's cleaner that way, updated here: a0a30b7

pomegranited · 2024-06-06T02:43:23Z

src/studio-home/StudioHome.jsx

+    if (isMixedOrV2LibrariesMode(libMode)) {
+      libraryHref = libraryAuthoringMfeUrl && redirectToLibraryAuthoringMfe
+        ? `${libraryAuthoringMfeUrl}create`
+        : `${window.location.origin}/course-authoring/library/create`;


MFEs can be deployed to different paths, so we can't hard-code /course-authoring here., but we can use these helpers from frontend-platform:

import { getConfig, getPath } from '@edx/frontend-platform';

Suggested change

: `${window.location.origin}/course-authoring/library/create`;

: `${getPath(getConfig().PUBLIC_PATH)}library/create`;

Got it, updated it here: 2859741

pomegranited · 2024-06-06T02:45:54Z

src/studio-home/StudioHome.test.jsx

+        setConfig({
+          ...getConfig(),
+          LIBRARY_MODE: 'v1 only',
+        });


Looks like you've set things up for testing here, but haven't added the actual tests yet?
Or maybe these changes aren't needed with your tab-section tests below?

For this test, the library mode needs to be set to v1 only. I set it back to mixed inside beforeEach to make sure the rest of the tests have the correct mode.

Oh gotcha.. thanks for explaining!

pomegranited · 2024-06-06T02:47:55Z

src/studio-home/__mocks__/studioHomeMock.js

@@ -62,7 +62,7 @@ module.exports = {
    },
  ],
  librariesEnabled: true,


Reminder: we also need a test where librariesEnabled: false, to check the placeholder page redirect.

I believe this scenarios will be checked as part of @rpenido 's PR, since the placeholder is going to be swapped out to an actual page.

src/studio-home/tabs-section/TabsSection.test.jsx

pomegranited · 2024-06-06T03:00:48Z

src/studio-home/tabs-section/libraries-v2-tab/index.jsx

+      ? `${libraryAuthoringMfeUrl}library/${id}`
+      // Redirection to the placeholder is done in the MFE rather than
+      // through the backend i.e. redirection from cms, because this this will probably change
+      : `${window.location.origin}/course-authoring/library/${id}`


Same comment as here about generating course authoring MFE URLs.

Updated: 2859741

pomegranited · 2024-06-06T03:09:01Z

src/studio-home/tabs-section/index.jsx

+      navigate(
+        libMode === 'v1 only'
+          ? '/libraries'
+          : '/legacy-libraries',


I understand switching the label for v1 Libraries to "Legacy Libraries" if v2 libraries are enabled -- that makes sense, and it will help users transition from v1 to v2.

But I don't think we need to use a different URL as well -- that's making things too complicated.

tab === TABS_LIST.legacyLibraries should always resolve to '/legacy-libraries'

tab === TABS_LIST.libraries should resolve to /libraries .

This would simplify your useEffect above too.

That makes sense, I've changed it here: 094086e

pomegranited · 2024-06-06T06:11:35Z

src/studio-home/tabs-section/libraries-v2-tab/index.jsx

+            <CardItem
+              key={`${org}+${slug}`}
+              isLibraries
+              displayName={title}


The ContentLibrary model doesn't have a title field.. I'm not sure what should go here instead?

That's right, however the serializers have the title field.

Here is an example of what is returned in the from the API:

{ "id": "lib:SampleTaxonomyOrg1:TL1", "type": "complex", "org": "SampleTaxonomyOrg1", "slug": "TL1", "title": "Test Library 1", "description": "", "num_blocks": 0, "version": 0, "last_published": null, "allow_lti": false, "allow_public_learning": false, "allow_public_read": false, "has_unpublished_changes": false, "has_unpublished_deletes": false, "license": "" }

Oh my mistake, you're totally correct, there is a title field in the serializer, and it's the library's learning package title. But since I was naughty and neglected to use the content_libraries.api to create my libraries, they didn't have learning packages, so I had no titles to click on.

My bad!

ohh I see, gotcha!

pomegranited · 2024-06-06T06:15:45Z

src/studio-home/__mocks__/studioHomeMock.js

@@ -62,7 +62,7 @@ module.exports = {
    },
  ],
  librariesEnabled: true,
-  libraryAuthoringMfeUrl: 'http://localhost:3001',
+  libraryAuthoringMfeUrl: 'http://localhost:3001/',


The platform default for this URL does not contain the trailing slash, but usually with a fully-deployed MFE, there's a PUBLIC_PATH variable that can default to /.

But I don't know for sure that our MFE configurations are consistent enough to rely on the presence of a trailing slash. Instead of constructing Library Authoring URLs from simple string concatenation, could you create a function that handles inserting the trailing slash if needed?

Interesting, I wasn't familiar with that! That's a good idea, I've added the function here 567dcb4

yusuf-musleh · 2024-06-07T01:13:13Z

@pomegranited Thanks for the detailed review! I believe I addressed all the comments and it should be ready to go for another round when you get a chance.

One more thing to note is, once the new frontend-build PRs are merged, I need to revert some commits on this PR to bring back the typescript code I removed to get the tests running:

pomegranited

👍 Works perfectly, and the code looks great.

I tested this using my tutor dev stack and @yusuf-musleh 's spectacular testing instructions :)
I read through the code, including the temporary TS/X changes.
I checked for accessibility issues by using my keyboard alone to navigate the new tabs and pages.
Includes documentation -- updates README
User-facing strings are extracted for translation

yusuf-musleh · 2024-06-07T11:03:10Z

Thanks @pomegranited for the great review!

@xitij2000 This is ready for CC when you have a chance.

xitij2000 · 2024-06-14T06:52:48Z

@xitij2000 @yusuf-musleh Also, please be aware that you can add the create-sandbox label to a PR like this and Grove will spin up a sandbox where you can test the feature. Though in this case it would probably use the "production" settings and you would only see the v1 tab.

I generally prefer to run and test the code as well especially after a review round in case something breaks. My devstack is fixed now so I am reviewing this right now.

yusuf-musleh · 2024-06-14T06:58:48Z

@bradenmacdonald

feel free to merge it with the JS version and we can do a fast-follow to convert it to TypeScript as soon as the frontend-build PR does get merged.

Sounds good, will do.

yusuf-musleh · 2024-06-19T10:19:12Z

My devstack is fixed now so I am reviewing this right now.

@xitij2000 Just checking in to see in case I missed the review or if it wasn't submitted yet.

xitij2000

Looks good! Just feel like the image doesn't belong here.

xitij2000 · 2024-06-19T11:42:48Z

README.rst

+.. image:: ./docs/readme-images/feature-v2-and-legacy-libs.png
+


I think we can leave out the image. They get outdated quick and it's a large chunk of data that will forever be a part of this repo and increase its size. Having it in the PR should be enough.

Thanks for the review! Good point, I've removed it.

When lib mode is set to "mixed", both "Libraries" and "Legacy Libraries" tabs are show in the Studio Home. When "Libraries" is clicked, v2 libraries are fetched, when "Legacy Libraries" is clicked, v1 libraries are fetched. When lib mode is set to "v1 only" or "v2 only", only one tab "Libraries" is show and only the respective libraries are fetched when the tab is clicked.

This is to switch between different library modes.

The path updates when selecting tabs, when accessing the url with the path directly it will open its respective tab. Navigating using the browser back/forward buttons is also supported.

This commit is temporary as the current frontend build system in tests doesnt support TS syntax. That should be fixed soon, and this commit should be removed.

This is a temporary commit since there are currently no webpack loaders that support tsx files in the test running. This commit should be removed once that is fixed upstream.

openedx-webhooks · 2024-06-20T12:01:00Z

@yusuf-musleh 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label May 27, 2024

bradenmacdonald reviewed May 27, 2024

View reviewed changes

src/studio-home/tabs-section/index.jsx Outdated Show resolved Hide resolved

bradenmacdonald reviewed May 27, 2024

View reviewed changes

src/studio-home/data/api.js Outdated Show resolved Hide resolved

yusuf-musleh force-pushed the yusuf-musleh/lib-v2-tab-studio-home branch from 17a5070 to 025a85d Compare May 30, 2024 11:54

yusuf-musleh mentioned this pull request May 30, 2024

feat: Use default pagination class for v2 lib views openedx/edx-platform#34879

Merged

yusuf-musleh force-pushed the yusuf-musleh/lib-v2-tab-studio-home branch 4 times, most recently from 0439a24 to 975b8db Compare June 3, 2024 16:28

yusuf-musleh force-pushed the yusuf-musleh/lib-v2-tab-studio-home branch from 975b8db to 8b96268 Compare June 4, 2024 12:54

yusuf-musleh force-pushed the yusuf-musleh/lib-v2-tab-studio-home branch from ad85c17 to 7a8488d Compare June 5, 2024 09:44

yusuf-musleh marked this pull request as ready for review June 5, 2024 09:53

yusuf-musleh requested a review from a team as a code owner June 5, 2024 09:53

rpenido mentioned this pull request Jun 5, 2024

feat: library home page bare bones (TEMP) open-craft/frontend-app-authoring#37

Closed

pomegranited suggested changes Jun 6, 2024

View reviewed changes

yusuf-musleh force-pushed the yusuf-musleh/lib-v2-tab-studio-home branch 4 times, most recently from e08b1ce to 2e3fa43 Compare June 7, 2024 00:50

pomegranited approved these changes Jun 7, 2024

View reviewed changes

xitij2000 approved these changes Jun 19, 2024

View reviewed changes

yusuf-musleh added 20 commits June 19, 2024 17:38

feat: Add LIBRARY_MODE config variable

89d76a7

This is to switch between different library modes.

feat: Add url paths/navigation for each tab

1c22798

The path updates when selecting tabs, when accessing the url with the path directly it will open its respective tab. Navigating using the browser back/forward buttons is also supported.

feat: LibraryV2 redirect to lib mfe or placeholder

1e2a635

feat: Add pagination support for lib v2s

9d76d07

fix: Redirect to placeholder create lib in v2/mixed disabled mfe

4c99093

temp: This removes TS code to get tests to run

1a90d3e

This commit is temporary as the current frontend build system in tests doesnt support TS syntax. That should be fixed soon, and this commit should be removed.

test: Update existing tests to support changes

fb261d9

temp: Rename .tsx -> .jsx & .ts -> .js for tests

3032e93

This is a temporary commit since there are currently no webpack loaders that support tsx files in the test running. This commit should be removed once that is fixed upstream.

fix: Fix lint issues

f20c74d

test: Add tests for new functionality

3127393

refactor: Change /legacy-libraries -> /libraries-v1

d200ee8

refactor: Remove hardcoded mfe path

0c34852

feat: Add function to construct Lib Auth MFE URL

2c48e60

feat: Make URL /library-v1 when referencing legacy

f496c01

fix: Add missing part of path

efe9cf4

fix: Issue with destinationUrl

f3a4386

test: Add checks for Tab.eventKey in tests

209e746

fix: Revert card item url changes to keep simple

d99a215

chore: Remove screenshot from README

09e3979

yusuf-musleh force-pushed the yusuf-musleh/lib-v2-tab-studio-home branch from 2e3fa43 to 09e3979 Compare June 19, 2024 14:42

xitij2000 merged commit 088a01d into openedx:master Jun 20, 2024
6 checks passed

yusuf-musleh mentioned this pull request Jun 23, 2024

[FC-0059] refactor: Change lib v2 code to TS #1124

Closed

pomegranited mentioned this pull request Jun 25, 2024

refactor: Change lib v2 code to TS open-craft/frontend-app-authoring#43

Merged

xitij2000 deleted the yusuf-musleh/lib-v2-tab-studio-home branch August 28, 2024 09:26

	<Route path="/legacy-libraries" element={<StudioHome />} />
	<Route path="/libraries-v1" element={<StudioHome />} />

	: `${window.location.origin}/course-authoring/library/create`;
	: `${getPath(getConfig().PUBLIC_PATH)}library/create`;

		.. image:: ./docs/readme-images/feature-v2-and-legacy-libs.png

feat: Add lib v2/legacy tabs in studio home #1050

feat: Add lib v2/legacy tabs in studio home #1050

Conversation

yusuf-musleh commented May 27, 2024 • edited Loading

Description

Supporting information

Testing instructions

openedx-webhooks commented May 27, 2024 • edited Loading

bradenmacdonald commented May 27, 2024

yusuf-musleh commented May 28, 2024

yusuf-musleh commented Jun 3, 2024

bradenmacdonald commented Jun 3, 2024

codecov bot commented Jun 4, 2024 • edited Loading

Codecov Report

rpenido commented Jun 4, 2024

yusuf-musleh commented Jun 4, 2024

pomegranited left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pomegranited Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yusuf-musleh commented Jun 7, 2024

pomegranited left a comment • edited Loading

Choose a reason for hiding this comment

yusuf-musleh commented Jun 7, 2024

xitij2000 commented Jun 14, 2024

yusuf-musleh commented Jun 14, 2024

yusuf-musleh commented Jun 19, 2024

xitij2000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openedx-webhooks commented Jun 20, 2024

yusuf-musleh commented May 27, 2024 •

edited

Loading

openedx-webhooks commented May 27, 2024 •

edited

Loading

codecov bot commented Jun 4, 2024 •

edited

Loading

pomegranited Jun 7, 2024 •

edited

Loading

pomegranited left a comment •

edited

Loading