Skip to content
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: support static reference in LibraryContentBlock #33511

Closed
wants to merge 128 commits into from

Conversation

connorhaugh
Copy link
Contributor

@connorhaugh connorhaugh commented Oct 16, 2023

Requires:

Description

This PR adds the ability for course authors to reference specific pieces of content from a content library in their course using the library xblock. It does so for v1 and v2 xblocks.

It does so by implementing the changes described in the ADR which describes the new desired schema for the block

Testing instructions

To test this PR:

  1. Create a v1 and a v2 content library, "v1 lib" and "v2 lib." Make sure each has some problems in it.

  2. In a unit in a course, create 2 library blocks. associate v1 lib with the first block, and v2 lib with the second.

  3. Test Randomized content behavior: with shuffle = true and max_count = 1, save and publish the blocks.
    expected: you should see 1 block at random per user per block. Try again with max_count = 2, and then max_count = -1. You should then see 2 and all blocks respectively.

  4. Test Specific content reference behavior. With shuffle = false and manual = true, select one piece of content from the v1 and v2 libs. Republish. This content should then be displayed to the user.

kdmccormick and others added 13 commits September 14, 2023 17:52
Rather than implementing V2-library and static-library-reference support
in a new block, we will be enhancing the existing `LibraryContentBlock`
in-place.

Relevant ADR PR: #33231
Co-Authored-By: Connor Haugh <chaugh@2u.com>
Co-Authored-By: Eugene Dyudyunov <evgen.dyudyunov@raccoongang.com>
...per comment in cms/djangoapps/contentstore/tasks.py.

This fixes an exception that was raised in the background whenever
the update_children_task was started:

     Traceback (most recent call last):
       File "/openedx/venv/lib/python3.8/site-packages/celery/utils/dispatch/signal.py", line 276, in send
         response = receiver(signal=self, sender=sender, **named)
       File "/openedx/venv/lib/python3.8/site-packages/user_tasks/signals.py", line 215, in start_user_task
         sender.status.start()
       File "/openedx/venv/lib/python3.8/site-packages/user_tasks/tasks.py", line 84, in status
         name = self.generate_name(arguments_dict)
       File "/openedx/edx-platform/openedx/core/djangoapps/content_libraries/tasks.py", line 274, in generate_name
         key = arguments_dict['dest_block_key']
     KeyError: 'dest_block_key'
@kdmccormick kdmccormick changed the title Feat library content block reference Support static reference in LibraryContentBlock Oct 17, 2023
@kdmccormick kdmccormick changed the title Support static reference in LibraryContentBlock feat: support static reference in LibraryContentBlock Oct 17, 2023
@kdmccormick kdmccormick marked this pull request as draft October 17, 2023 15:11
Just like ./common/ and ./openedx/, the unit tests in ./xmodule/
validate behavior in both LMS and CMS. In order to fully test
./xmodule/, we need to run its tests in both contexts.

Also in this commit:

* refactor: Rename the shards to be clear whether they're
            running under LMS or CMS.
* docs: correct comments regarding conditions under which
        codejail's test_cant_do_something_forbidden is skipped.

* test: update a unit test which was using the now-deleted
        library_sourced block to use library_content block instead.
Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

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

@connorhaugh Just an FYI on some recent changes I've pushed.

@@ -264,6 +242,108 @@ def _publish_event(self, event_name, result, **kwargs):
self.runtime.publish(self, f"edx.librarycontentblock.content.{event_name}", event_data)
self._last_event_result_count = len(result) # pylint: disable=attribute-defined-outside-init

def available_children(self) -> list[tuple[str, str]]:
Copy link
Member

Choose a reason for hiding this comment

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

I exposed this as a public method so that unit tests can use it without duplicating the same logic.

The underlying implementation is still a static method so that make_selection can use it.

)

@classmethod
def _available_children(
Copy link
Member

Choose a reason for hiding this comment

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

This was previously "_get_valid_children". I renamed it "_available_children" because it struck me as more self-explanatory -- these are the children which are available for selection.

Comment on lines 266 to 270
children = [(child.block_type, child.block_id) for child in library_children]
if manual:
return [candidate for candidate in candidates if candidate in children]
else:
return children
Copy link
Member

Choose a reason for hiding this comment

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

This is one of the recent "bugs" I fixed, if you'd call it that.

Before, when manual==True, we would return the whole candidates list, regardless of whether all of those candidates appeared as block children.

Now, when manual=True, we return the candidates which are also children. That way, if (for some reason) child is removed but the candidate entry is not, we don't get ItemNotFound errors from modulestore. I'm not sure when that issue would happen, but it seems like a reasonable safeguard to have in place.

@kdmccormick kdmccormick removed the create-sandbox open-craft-grove should create a sandbox environment from this PR label Jan 8, 2024
@kdmccormick kdmccormick added the Content Libraries This PR is related to v2 Content Libraries work label May 15, 2024
@kdmccormick
Copy link
Member

Closing, as we will be implementing static reference on a per-component basis rather than though the LibraryContentBlock.

@kdmccormick kdmccormick deleted the feat--Library-Content-Block-Reference branch May 15, 2024 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content Libraries This PR is related to v2 Content Libraries work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants