feat: Library Content Block Editor #411

connorhaugh · 2023-10-17T20:09:08Z

This PR adds a new editor for the library_content block.

Initial Editor with no libraries selected:

Editor with a V2 Library selected:

Editor with a V1 Library selected:

Editor with random library blocks mode selection:

Testing Instructions:
This edx-platform PR is required: openedx/edx-platform#33977
Waffle flag needed: new_core_editors.use_new_library_content_editor

Create a number of libraries with xblocks (both v2 and v1 libraries are fine).
In course authoring, add a "Library Content" block to your unit.
Click edit to edit the block. This will bring you to the Library Content Block Editor.
Library Selection
a. Selecting a library should show one of the two screens above depending on if the library is v1 or v2.
Library Settings
a. Mode: User can select 'Show a number of components to display at random from the library.' or 'Show the selected blocks of the library to all users.'
If user chooses the first option, they should see a number input for the number of blocks to show the learner (for both v1 and v2 libraries).
If user chooses the second option, they should see a table with all the blocks in the selected library.
b. Show Reset
Block Selection: With the 'selected blocks' mode choice, the user can see a table of all the blocks in the library. Users can check the box to include the block as one of the components to display to learners. The table can be sorted by block name and type.
When saving and reopening the editor, the attributes described in steps 4-6 should show.

src/editors/containers/EditorContainer/index.jsx

connorhaugh

Just some early feedback (I realize this is not fully ready for review yet.)

src/editors/containers/LibraryContentEditor/data/urls.js

src/editors/containers/LibraryContentEditor/hooks.js

src/editors/data/services/cms/api.js

connorhaugh · 2023-11-30T13:16:06Z

src/editors/containers/LibraryContentEditor/data/api.js

+  fetchLibraryProperty: ({ studioEndpointUrl, libraryId }) => get(
+    urls.libraryProperty({ studioEndpointUrl, libraryId }),
+  ),
+  fetchLibraryContent: ({ studioEndpointUrl, libraryId }) => get(


Hate to drop a confounding variable at this point in the process (so maybe it should be a fast follow to this PR). I think this will end up making the code much more simple and fast, but might be frustrating now:

In reading this, I realize I may have not gotten a particular point across. I attempted to explain that library content blocks copy content from the library into the course. This value is stored as the "children" of the library content block in the course. As this copy is now in the course, we can edit it, and the library remains unchanged.

Therefore, we don't want to be fetching the blocks from the library. Instead, we already have them stored in the library content block as the "children" values.

Does that make sense?

connorhaugh

What does the lifecycle of the library api methods look like? Do we only initialize them for library editors (I hope that is a yes). Just couldn't see that cleanly in the code.

src/editors/containers/LibraryContentEditor/BlocksSelector.jsx

src/editors/containers/LibraryContentEditor/data/urls.js

src/editors/containers/LibraryContentEditor/utils.js

rayzhou-bit · 2023-12-04T18:40:38Z

What does the lifecycle of the library api methods look like? Do we only initialize them for library editors (I hope that is a yes). Just couldn't see that cleanly in the code.

You should be able to see these in the useEffect calls in the hooks.js file.

When editor is initialized, lists of v1 and v2 libraries are fetched.
When a library is selected (including when opening an editor with an already selected library), the blocks of the library is fetched.

rayzhou-bit · 2023-12-07T22:18:25Z

Oof I left the description half finished. I'll update that in a bit and I'll make sure to add that screenshot.

rayzhou-bit · 2023-12-08T01:44:14Z

And can you confirm that V1 library users will still be able to specify a number of blocks to randomly display?

Yes, the Enter the number of components to display to each student. Set it to -1 to display all components. field is still there for v1 libraries.

connorhaugh · 2023-12-11T15:25:33Z

src/editors/data/constants/requests.js

+  fetchV2Libraries: 'fetchV2Libraries',
+  fetchV2LibraryMetadata: 'fetchV2LibraryMetadata',
+  fetchV2LibraryContent: 'fetchV2LibraryContent',
+  fetchBlockContent: 'fetchBlockContent',


It looks like the api calls to fetchV2LibraryContent fetchV2LibraryContent is not made or used atm, right?

fetchV2LibraryContent is used but fetchV2LibraryMetadata is not. This was originally for fetching v1 library metadata (before I renamed some things to v2). V1 library metadata is needed to get v1 library version. I'll need to put this back in.

We acttually need none of:

fetchV2LibraryContent
fetchV2LibraryContent

as we shouldn't be getting the content from the library.

fetchV2LibraryContent points to the /api/libraries/v2/${libraryId}/blocks/ endpoint. We want this for library blocks right? Do I just need to rename this to fetchV2LibraryBlocks or is there something else I'm missing?

I'm realizing there is more to this than I may have let on. from a UI perspective.
There are three-ish cases we need to consider here:

A user clicks edit on a library block which already has a source library_id then either:

they view their selected blocks in the library.

they click on a new library.

A user clicks edit on a library block which does not already have a souce library_id.

In theory, in the first case, we need to show the library content block's children (not the library's) to be available for selection.

the last two cases are a conundrum. We know that child blocks of a library and child blocks of a library content block have different ids. Therefore, it is fruitless to attempt to do block selection in those cases. How can we fix this? I think we should do this as a fast-follow, but definitely this is a blocker to rollout.

Just checking to see if I understand this correctly... Is the library content block's children the candidates of what might be an older version of the selected library?

connorhaugh · 2023-12-11T19:26:32Z

Hey -- it looks great but a couple of bugs from manual testing:

State preservation

Create A V2 lib
Associate it with a lib content block
Set it to "Show the selected blocks of the library to all users." and select 1 block
Save & exit
Expected behavior Returning to the editor shows "Show the selected blocks of the library to all users." and the block is selected
Actual behavior Returning to the editor shows "Show a number of components to display at random from the library."

Saving Errors

Create a v1 lib
Associate it with a library content block.
set the max_count to be anything
Save
Expected behavior Save is successful and returns you to the unit page.
Actual behavior you get a full page error saying Cannot read properties of undefined (reading 'toString')

Follow on bug (not a blocker for you or dealt with in this PR just have to confirm it works when we merge) candidates aren't interpreted in the "view" page.

jesperhodge · 2023-12-13T21:54:01Z

src/editors/containers/LibraryContentEditor/index.test.jsx

+    const { getByTestId } = renderComponent({ ...props, blockFinished: false });
+    expect(getByTestId('librarycontenteditor-loadingspinner')).toBeTruthy();
+  });
+  it('Renders as expected once loaded', () => {


Also on this component level, we need to consider what should happen if the loading has finished but the block fails. What should be rendered? Should child components be rendered if there is no block to render? Should we throw an error that NewRelic will pick up? - If we do still render child components, will child components throw unwanted errors?

I would include a test for it with the desired behavior, and probably the desired behavior warrants some thinking about what should happen.

That's my first-glance take, maybe you already dealt with everything in the hook. It wouldn't hurt having an additional test here anyway, so we can see if a future change inside the component breaks something.

I think we want whatever is the most simple here in terms of behavior @rayzhou-bit , but +1 to having a unit test here.

jesperhodge · 2023-12-13T22:15:19Z

src/editors/containers/LibraryContentEditor/hooks.test.js

+        library_key: 'v1id',
+      },
+    ];
+    it('should fetch v2 and v1 libraries when block finishes loading', () => {


A blank line between it statements is somehow just essential for me. At some point I just start seeing a wall of text otherwise.

jesperhodge · 2023-12-13T22:18:00Z

src/editors/containers/LibraryContentEditor/data/requests.js

@@ -0,0 +1,97 @@
+import { StrictDict } from '../../../utils';


A lot here seems not covered by tests. Probably warrants a requests.test.js file.

I think the previous logic was "These are a wrapper/middleware so the unit testing would essentially be "did any of the inputs change" which doesn't improve confidence.

rayzhou-bit · 2023-12-19T20:59:07Z

State preservation
Expected behavior Returning to the editor shows "Show the selected blocks of the library to all users." and the block is selected

I'm not getting a manual in the api call.

…nt-block-editor

kdmccormick · 2024-06-18T20:37:30Z

We will be implementing static reference without a wrapper block. For example, instead of a course library_content block wrapping a library problem block, we will have a course problem block with a pointer to the upstream_block. Randomized content will still need a wrapper block, which may be the library_content block, or may be a new block.

Thanks for all your work on this, folks. I'll pass this PR along to the team working on the new library-reference frontends so that they have the opportunity to reuse some of this work.

connorhaugh and others added 12 commits October 17, 2023 20:08

feat: initial editor layout + ui

27b2c6f

feat: wip

1a3bc9b

feat: wip2

668c184

feat: wip3

d8a45fd

feat: polish

3f1887c

test: add reducers and selectors tests

d66c7e9

fix: proptypes

3734e01

test: add blockselector test

2b263ee

test: add three more JS tests

ffd2e63

fix: add tests for all but hooks.js

65eee08

fix: initialize

2674607

fix: library api

e74c90f

connorhaugh changed the title ~~feat: initial editor layout + ui~~ feat: Library Content Block Editor Nov 13, 2023

connorhaugh commented Nov 13, 2023

View reviewed changes

src/editors/containers/EditorContainer/index.jsx Show resolved Hide resolved

connorhaugh marked this pull request as ready for review November 13, 2023 20:12

connorhaugh commented Nov 14, 2023

View reviewed changes

rayzhou-bit added 9 commits November 15, 2023 01:04

feat: api tweaks mostly with index.js

3d8d5b0

feat: libraryselector.js api update

5d2f783

feat: blockselector.js rewrite wip

a9989da

feat: general changes to api usage

56c4c39

feat: table saves candidates

2b92174

feat: load candidate works

21bd883

feat: v1 library selection

ab3e541

fix: loading issue and polish

fe65556

fix: saving blocks

45b9f40

connorhaugh commented Nov 30, 2023

View reviewed changes

rayzhou-bit added 2 commits November 30, 2023 18:15

feat: sort library dropdown alphabetically

1339322

feat: lint and some fixes

c92c35f

connorhaugh commented Dec 4, 2023

View reviewed changes

src/editors/containers/LibraryContentEditor/BlocksSelector.jsx Show resolved Hide resolved

src/editors/containers/LibraryContentEditor/data/urls.js Outdated Show resolved Hide resolved

src/editors/containers/LibraryContentEditor/utils.js Show resolved Hide resolved

connorhaugh commented Dec 11, 2023

View reviewed changes

jesperhodge reviewed Dec 13, 2023

View reviewed changes

rayzhou-bit added 2 commits December 19, 2023 17:38

feat: remove fetchV2LibraryMetadata

6265f5a

feat: v1 library api update

3a689a4

rayzhou-bit added 8 commits December 19, 2023 21:11

feat: fix

0d51711

feat: lint

b09ed11

feat: library version should be string

32c4fb3

feat: failure tests

208070a

feat: merge conflict

02f6ac3

feat: LCB children

3ea40d8

feat: new fetch children api

fd6d530

feat: test and lint

1c783d7

rayzhou-bit mentioned this pull request Jan 3, 2024

feat: include children info in xblock api openedx/edx-platform#33977

Merged

rayzhou-bit and others added 5 commits January 5, 2024 20:52

feat: use usage id and fetch v1 library block

0bfa336

feat: regex fix and v1 library version

8553ac6

feat: lint

9db63b4

feat: v1 api fix and candidate saving

0c68a92

feat: test and lint

01f6296

rayzhou-bit requested a review from kdmccormick January 22, 2024 18:37

kdmccormick mentioned this pull request Feb 6, 2024

[Tagging] Enable Libraries v2 on the Tagging Sandbox openedx/modular-learning#166

Closed

Merge remote-tracking branch 'upstream/main' into feat--library-conte…

d94280c

…nt-block-editor

kdmccormick closed this Jun 18, 2024

kdmccormick deleted the feat--library-content-block-editor branch June 18, 2024 20:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Library Content Block Editor #411

feat: Library Content Block Editor #411

connorhaugh commented Oct 17, 2023 •

edited by rayzhou-bit

Loading

connorhaugh left a comment

connorhaugh Nov 30, 2023

connorhaugh left a comment •

edited

Loading

rayzhou-bit commented Dec 4, 2023

rayzhou-bit commented Dec 7, 2023

rayzhou-bit commented Dec 8, 2023

connorhaugh Dec 11, 2023 •

edited

Loading

rayzhou-bit Dec 12, 2023

connorhaugh Dec 18, 2023

rayzhou-bit Dec 19, 2023

connorhaugh Dec 20, 2023

rayzhou-bit Dec 20, 2023

connorhaugh commented Dec 11, 2023

jesperhodge Dec 13, 2023

connorhaugh Dec 18, 2023

jesperhodge Dec 13, 2023

jesperhodge Dec 13, 2023

connorhaugh Dec 18, 2023

rayzhou-bit commented Dec 19, 2023

kdmccormick commented Jun 18, 2024

		@@ -0,0 +1,97 @@
		import { StrictDict } from '../../../utils';

feat: Library Content Block Editor #411

feat: Library Content Block Editor #411

Conversation

connorhaugh commented Oct 17, 2023 • edited by rayzhou-bit Loading

connorhaugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorhaugh left a comment • edited Loading

Choose a reason for hiding this comment

rayzhou-bit commented Dec 4, 2023

rayzhou-bit commented Dec 7, 2023

rayzhou-bit commented Dec 8, 2023

connorhaugh Dec 11, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorhaugh commented Dec 11, 2023

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

rayzhou-bit commented Dec 19, 2023

kdmccormick commented Jun 18, 2024

connorhaugh commented Oct 17, 2023 •

edited by rayzhou-bit

Loading

connorhaugh left a comment •

edited

Loading

connorhaugh Dec 11, 2023 •

edited

Loading