feat: add library component picker #1356

rpenido · 2024-10-02T21:19:05Z

Description

This PR implements the components

Select Library

Pick Component

Additional Information

Part of:

Add library content to a course #1337

Depends on:

feat: add library-v2 edx-platform#35636

Testing instruction

Access http://apps.local.edly.io:2001/course-authoring/component-picker?parentLocator=block-v1:edX+DemoX+Demo_Course+type@vertical+block@vertical1
block-v1:edX+DemoX+Demo_Course+type@vertical+block@vertical1 should be the usageKey of a Unit
Select a Library
Navigate through the library and enter some collections
The library should be in read-only mode, not allowing any change in the components/collections
Click on the "+ Add" button (or in the "Add to Course") and check if the component was added to the Unit provided

Private-ref: FAL-3876

openedx-webhooks · 2024-10-02T21:19:09Z

Thanks for the pull request, @rpenido!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To 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:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/2u-tnl. Tag them in a comment and let them know that your changes are ready for review.

Where 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:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

bradenmacdonald · 2024-10-02T21:29:16Z

@rpenido FYI, once we implement #1354 we will need to make it so that this "component picker" mode only shows the published version of things, not the draft.

src/library-authoring/common/context.tsx

codecov · 2024-10-04T17:56:24Z

Codecov Report

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

Project coverage is 93.09%. Comparing base (1ee3229) to head (46babe0).
Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
...brary-authoring/component-picker/SelectLibrary.tsx	90.00%	3 Missing ⚠️
...c/library-authoring/collections/CollectionInfo.tsx	93.33%	1 Missing ⚠️
...ry-authoring/collections/LibraryCollectionPage.tsx	93.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1356      +/-   ##
==========================================
+ Coverage   93.04%   93.09%   +0.05%     
==========================================
  Files        1043     1047       +4     
  Lines       19954    20128     +174     
  Branches     4155     4266     +111     
==========================================
+ Hits        18566    18738     +172     
+ Misses       1326     1325       -1     
- Partials       62       65       +3

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

rpenido · 2024-10-10T20:47:36Z

src/library-authoring/LibraryAuthoringPage.tsx

- <Routes>
- <Route
- path={TabList.home}
- element={(
- <LibraryHome tabList={TabList} handleTabChange={handleTabChange} />
- )}
- />
- <Route
- path={TabList.components}
- element={<LibraryComponents variant="full" />}
- />
- <Route
- path={TabList.collections}
- element={<LibraryCollections variant="full" />}
- />
- <Route
- path="*"
- element={<NotFoundAlert />}
- />
- </Routes>


We had to change to render based on a state instead of the route because of the ComponentPicker component

Can you elaborate? Why can't we have nested routes, with three home/components/collections routes underneath /library/foo and the same three home/components/collections routes also available underneath /library-component-picker/ ?

Hmm. You're right. We can. I didn't think this way when I refactored this.
Do you prefer we stick to the routing approach? We will just need to fix some places where we do absolute redirects to '/library/libraryId' to support this.

Well what I really want is to implement #1230 ASAP, which will simplify everything. Then the "All/Collections/Components" tabs will essentially be just another filter. But yeah, I do think it should be in the route if possible.

Though it doesn't need to be in the route for the content picker.

Just to be clear, it is still in the URL path as it was. I'm just using states from the context to render the child components instead of the Router component.

I will check how far I'm from using Routes again tomorrow.

Oh ok. Maybe that's not so bad? I'll have a look later.

I think it would be better to fix the Route issue with #1230. I moved more things to the context here, which will help the refactor a bit.

This PR is "functional".
We will probably want to change the endpoint to add the content, but is it possible to test it? It is missing the Modal tough.
I will post a more detailed update about the task status tomorrow.

This change seems to have broken the Back/Forward button in the browser. Before, you could change tabs and then press BACK and it would return to the previous tab, but now it's not working.

rpenido · 2024-10-10T20:48:20Z

src/library-authoring/LibraryLayout.tsx

- <Routes>
- <Route
- path="collection/:collectionId"
- element={<LibraryCollectionPage />}
- />
- <Route
- path="*"
- element={<LibraryAuthoringPage />}
- />
- </Routes>


We had to change to render based on a state instead of the route because of the ComponentPicker component

We can keep this as is, don't need to remove the routes. Using match in line 15 to get collectionId and passing it to LibraryProvider should be enough.

Great! Fixed here: 1ce80cb

rpenido · 2024-10-15T20:35:21Z

@bradenmacdonald Conflicts resolved!

bradenmacdonald · 2024-10-15T21:01:43Z

@rpenido This is minor, but there's no spacing around the "Next" button and it's touching the edge of the browser window.

bradenmacdonald · 2024-10-15T21:02:56Z

@rpenido Also, the "+ Add Collection" button should not appear in this read-only mode.

bradenmacdonald · 2024-10-15T21:21:05Z

@rpenido When we implement #1385 , we're going to have to change a bunch of things with this modal (to support multi-select, and not to add to the course right away - instead just add to a list and pass that list to the Problem Bank). But this is totally fine for now - just making you aware it will need some changes.

bradenmacdonald

Phew! This is a huge PR. Lots of nice refactors in here. Just a few small issues. If you want to address any of my feedback in a follow-up PR or skip it entirely, let me know.

bradenmacdonald · 2024-10-15T21:26:22Z

src/library-authoring/LibraryAuthoringPage.tsx

- <Routes>
- <Route
- path={TabList.home}
- element={(
- <LibraryHome tabList={TabList} handleTabChange={handleTabChange} />
- )}
- />
- <Route
- path={TabList.components}
- element={<LibraryComponents variant="full" />}
- />
- <Route
- path={TabList.collections}
- element={<LibraryCollections variant="full" />}
- />
- <Route
- path="*"
- element={<NotFoundAlert />}
- />
- </Routes>


This change seems to have broken the Back/Forward button in the browser. Before, you could change tabs and then press BACK and it would return to the previous tab, but now it's not working.

bradenmacdonald · 2024-10-15T21:32:27Z

src/library-authoring/component-picker/ComponentPicker.tsx

+ throw new Error('parentLocator is required');
+ }
+
+ // URLSearchParams decodes '+' to ' ', so we need to convert it back


This shouldn't be needed. The problem was that the ID wasn't properly encoded in the first place. The + should have been replaced with %2B

The Unit Outline doesn't use the encoded key in the URL, so I opted not to use it here, either.

It will also work if we receive the URL encoded.
Let me know if this is an issue!

Yes, but there it's in the path part of the URL. Here it's in a query string, and it's definitely wrong to assume that a + in a query string value will be interpreted as +

It's fine to leave this in; as you said it's not doing any harm. But we should be encoding the ID when we pass it into the query string.

src/library-authoring/common/context.tsx

pomegranited · 2024-10-16T04:59:54Z

src/library-authoring/component-picker/SelectLibrary.scss

@@ -0,0 +1,3 @@
+.library-list {
+ min-height: 620px;


Can this style be removed? When the picker is inset in an iframe, this makes the page longer than it needs to be.

Yes. This was to make the list have the same height while showing less than 5 results.
Removed here: 72f7b85

rpenido · 2024-10-16T14:17:21Z

This change seems to have broken the Back/Forward button in the browser. Before, you could change tabs and then press BACK and it would return to the previous tab, but now it's not working.

This should fix it: a31836e
But I think we should use nested routes in the future.

rpenido · 2024-10-16T14:18:46Z

Phew! This is a huge PR.
Sorry for that @bradenmacdonald ! 😬

I think I addressed the issues. Let me know if I missed anything!

bradenmacdonald · 2024-10-16T15:54:24Z

@rpenido What about the spacing around the "Next" button and the need to hide the " + Add Collection" button?

Edit: I see they're both fixed now :)

bradenmacdonald

Looking good now, thanks for this nice work! Let me know when I can merge.

rpenido · 2024-10-16T15:59:12Z

Looking good now, thanks for this nice work! Let me know when I can merge.

Waiting for the CI/CD. If something fails, I will fix it later today,

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

bradenmacdonald reviewed Oct 2, 2024

View reviewed changes

src/library-authoring/common/context.tsx Outdated Show resolved Hide resolved

rpenido force-pushed the rpenido/fal-3876-add-library-content-to-a-course branch 4 times, most recently from adb7247 to 785a830 Compare October 4, 2024 00:00

rpenido mentioned this pull request Oct 8, 2024

feat: manage collections in component sidebar [FC-0062] #1373

Merged

1 task

rpenido force-pushed the rpenido/fal-3876-add-library-content-to-a-course branch 2 times, most recently from 5b8042d to 5c2842a Compare October 8, 2024 20:28

rpenido commented Oct 10, 2024

View reviewed changes

rpenido force-pushed the rpenido/fal-3876-add-library-content-to-a-course branch 2 times, most recently from 9babe85 to 7e61a2c Compare October 10, 2024 22:30

feat: add library component picker

cdfe3d6

rpenido force-pushed the rpenido/fal-3876-add-library-content-to-a-course branch from 7e61a2c to cdfe3d6 Compare October 10, 2024 22:45

rpenido added 2 commits October 11, 2024 11:47

feat: add api call

df9b518

test: add tests

d8f3d7a

rpenido force-pushed the rpenido/fal-3876-add-library-content-to-a-course branch from 46d7a0d to d8f3d7a Compare October 11, 2024 16:37

test: more tests

7ca8f22

rpenido force-pushed the rpenido/fal-3876-add-library-content-to-a-course branch 2 times, most recently from 0f0a5d6 to 7ca8f22 Compare October 11, 2024 19:12

rpenido added 5 commits October 11, 2024 16:40

test: try to fix flaky test

45b2f01

test: improve coverage

08a7e48

fix: some navigation fixes

be2765d

fix: more fixes

34f76e6

test: more coverage

de1be9e

rpenido force-pushed the rpenido/fal-3876-add-library-content-to-a-course branch from a6ec33d to de1be9e Compare October 11, 2024 21:42

rpenido force-pushed the rpenido/fal-3876-add-library-content-to-a-course branch 2 times, most recently from ef416c9 to edf787f Compare October 15, 2024 20:02

fix: issues from merge

3a4497b

rpenido force-pushed the rpenido/fal-3876-add-library-content-to-a-course branch from edf787f to 3a4497b Compare October 15, 2024 20:14

fix: check readOnly on ManageCollections

8431b67

bradenmacdonald reviewed Oct 15, 2024

View reviewed changes

pomegranited reviewed Oct 16, 2024

View reviewed changes

rpenido added 3 commits October 16, 2024 10:57

fix: remove min-height style

72f7b85

fix: add jsdocs

ded2f07

fix: set activeKey with navigation

a31836e

rpenido added 4 commits October 16, 2024 11:27

test: fix tests

fdc54e7

test: fix test

f57efb4

test: fix test

9e82795

fix: add padding

78d25fb

fix: remove add component when readOnly

5cc64cb

bradenmacdonald approved these changes Oct 16, 2024

View reviewed changes

bradenmacdonald added 2 commits October 16, 2024 09:17

chore: remove unused import

9709133

test: revert change that broke flaky test

46babe0

bradenmacdonald merged commit b81f611 into openedx:master Oct 16, 2024
8 checks passed

bradenmacdonald deleted the rpenido/fal-3876-add-library-content-to-a-course branch October 16, 2024 17:18

navinkarkera mentioned this pull request Oct 17, 2024

feat: direct link to single block in library [FC-0062] #1392

Open

ChrisChV mentioned this pull request Oct 22, 2024

feat: Show published components on content picker #1420

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add library component picker #1356

feat: add library component picker #1356

rpenido commented Oct 2, 2024 •

edited

Loading

openedx-webhooks commented Oct 2, 2024 •

edited

Loading

bradenmacdonald commented Oct 2, 2024

codecov bot commented Oct 4, 2024 •

edited

Loading

rpenido Oct 10, 2024

bradenmacdonald Oct 10, 2024

rpenido Oct 10, 2024 •

edited

Loading

bradenmacdonald Oct 10, 2024

bradenmacdonald Oct 10, 2024

rpenido Oct 10, 2024 •

edited

Loading

bradenmacdonald Oct 11, 2024

rpenido Oct 11, 2024

bradenmacdonald Oct 15, 2024

rpenido Oct 10, 2024 •

edited

Loading

navinkarkera Oct 14, 2024

rpenido Oct 14, 2024

rpenido commented Oct 15, 2024

bradenmacdonald commented Oct 15, 2024

bradenmacdonald commented Oct 15, 2024 •

edited

Loading

bradenmacdonald commented Oct 15, 2024

bradenmacdonald left a comment

bradenmacdonald Oct 15, 2024

bradenmacdonald Oct 15, 2024

rpenido Oct 16, 2024

bradenmacdonald Oct 16, 2024

bradenmacdonald Oct 16, 2024

pomegranited Oct 16, 2024

rpenido Oct 16, 2024

rpenido commented Oct 16, 2024

rpenido commented Oct 16, 2024

bradenmacdonald commented Oct 16, 2024 •

edited

Loading

bradenmacdonald left a comment

rpenido commented Oct 16, 2024

feat: add library component picker #1356

feat: add library component picker #1356

Conversation

rpenido commented Oct 2, 2024 • edited Loading

Description

Select Library

Pick Component

Additional Information

Testing instruction

openedx-webhooks commented Oct 2, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

bradenmacdonald commented Oct 2, 2024

codecov bot commented Oct 4, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido Oct 10, 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

rpenido Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido commented Oct 15, 2024

bradenmacdonald commented Oct 15, 2024

bradenmacdonald commented Oct 15, 2024 • edited Loading

bradenmacdonald commented Oct 15, 2024

bradenmacdonald 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

rpenido commented Oct 16, 2024

rpenido commented Oct 16, 2024

bradenmacdonald commented Oct 16, 2024 • edited Loading

bradenmacdonald left a comment

Choose a reason for hiding this comment

rpenido commented Oct 16, 2024

rpenido commented Oct 2, 2024 •

edited

Loading

openedx-webhooks commented Oct 2, 2024 •

edited

Loading

codecov bot commented Oct 4, 2024 •

edited

Loading

rpenido Oct 10, 2024 •

edited

Loading

rpenido Oct 10, 2024 •

edited

Loading

rpenido Oct 10, 2024 •

edited

Loading

bradenmacdonald commented Oct 15, 2024 •

edited

Loading

bradenmacdonald commented Oct 16, 2024 •

edited

Loading