Skip to content

[BD-14] feat: implement V2 libraries usage for library content block - #30615

Merged
connorhaugh merged 1 commit into
openedx:masterfrom
raccoongang:update-library-content-block-to-use-v2-libs
Aug 16, 2022
Merged

[BD-14] feat: implement V2 libraries usage for library content block#30615
connorhaugh merged 1 commit into
openedx:masterfrom
raccoongang:update-library-content-block-to-use-v2-libs

Conversation

@dyudyunov

@dyudyunov dyudyunov commented Jun 17, 2022

Copy link
Copy Markdown
Contributor

This will close Milestone 4.0b

  • V2 libraries are available for selection in the Random Block edit modal;
  • selected V2 library blocks are copied to the modulestore and saved as children of the Random Block;
  • V2 library version validation works the same as for the V1 libraries (with a possibility to update the block with the latest version).
  • filtering by problem type can't be done for V2 the same as for V1 because the v2 library problems are not divided by types. Selecting a problem type filter in the edit mode using the V2 library will not have any effect;
  • unit tests added/updated.

Note: I didn’t add any additional permission checks for getting libraries, only reused current functionality (as we’ve discussed earlier). If the permission checks need any updates - it will be done as a separate task (to avoid blocking several tasks dependent on this one)

@openedx-webhooks

openedx-webhooks commented Jun 17, 2022

Copy link
Copy Markdown

Thanks for the pull request, @dyudyunov!

When this pull request is ready, tag your edX technical lead.

@openedx-webhooks openedx-webhooks added the blended PR is managed through 2U's blended developmnt program label Jun 17, 2022
@dyudyunov
dyudyunov force-pushed the update-library-content-block-to-use-v2-libs branch 3 times, most recently from ad22fc3 to cce72f9 Compare June 27, 2022 14:48
@dyudyunov

Copy link
Copy Markdown
Contributor Author

Those failing tests are not related to the changes in my PR and they are floating - I've got them failed around 50% of the times I tried to run them locally

@dyudyunov
dyudyunov force-pushed the update-library-content-block-to-use-v2-libs branch from cce72f9 to e534499 Compare July 14, 2022 07:12
@natabene

Copy link
Copy Markdown
Contributor

@dyudyunov When do you anticipate this will be ready for a review?

@dyudyunov

Copy link
Copy Markdown
Contributor Author

hi @natabene
It's currently in our internal testing and I'll remove the WIP status as soon as it passes it. I hope that will be done this week

@dyudyunov
dyudyunov force-pushed the update-library-content-block-to-use-v2-libs branch from c68bda2 to 2647def Compare August 1, 2022 11:51
YT: https://youtrack.raccoongang.com/issue/EDX_BLND_CLI-87

- V2 libraries are available for selection in the Random Block edit modal;
- selected V2 library blocks are copied to the modulestore and saved as children of the Random Block;
- V2 library version validation works the same as for the V1 libraries (with possibility to update block with the latest version);
- filtering by problem type can't be done for V2 the same as for V1 because the v2 library problems are not divided by types;
- the problem type field is hidden for v2 libraries in the edit mode;
- unit tests added/updated.
@dyudyunov
dyudyunov force-pushed the update-library-content-block-to-use-v2-libs branch from 2647def to bd00331 Compare August 1, 2022 12:19
@dyudyunov dyudyunov changed the title ⚠️ [WIP][BD-14] feat: implement V2 libraries usage for library content block [BD-14] feat: implement V2 libraries usage for library content block Aug 1, 2022
@dyudyunov

Copy link
Copy Markdown
Contributor Author

@natabene now it's ready for the review 🎉

@connorhaugh
connorhaugh self-requested a review August 4, 2022 13:01
Comment thread xmodule/library_content_module.py
Covers getting results for either library key as a string or LibraryLocatorV2.

NOTE:
We don't publish library updates so the library version will always be 0.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't publish library updates so the library version will always be 0.

Is this going to change, or are libraries going to remain unversioned?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Libraries versioning works, but this isn't checked because the test goal here is to prove that the version could be received either for the id as a string or as a LibraryLocatorV2.
Publishing a library is a part of the content libraries functionality, not the library-content/library-sourced xblock so I don't test it here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, this language is not wholly correct then. I knew libraries were versioned, I didn't know about the xblock level. Maybe We are not testing the library version here, so it will be 0 is more informative and less confusing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok
@connorhaugh don't you mind if I ask @UvgenGen to update this in his future PRs? I propose that because his work is based on this branch and I don't want him for rebasing several feature branches because of comment change )

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that is ok!

@connorhaugh

Copy link
Copy Markdown
Contributor

How long do we plan to use both V1 and V2 libraries?

@dyudyunov

Copy link
Copy Markdown
Contributor Author

How long do we plan to use both V1 and V2 libraries?

I have no info about that 🙂

@connorhaugh
connorhaugh merged commit dcf1cc1 into openedx:master Aug 16, 2022
@openedx-webhooks

Copy link
Copy Markdown

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

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blended PR is managed through 2U's blended developmnt program

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants