[LX-2365] allows XBlock API users to optionally use LabXchange block types

[pomegranited]

Description

Adds an optional parameter to the blockstore runtime REST APIs to request LabXchange's block type overrides so that the custom XBlock implementations can be tested with a subset of users.

This PR adds functionality, and so should not affect other users of these APIs.

Testing instructions

Set up devstack

Note: I tried setting up blockstore + library-authoring MFE in an Ocim sandbox, but was unable to get them to work together. So testing will have to be done on a devstack.

If you have a labxchange devstack, you can use the libraries created by LX, you don't have to do these steps.

1. Install the library authoring MFE on your devstack.
   ⚠️ There's a typo in step 2 -- the gradebook MFE has been renamed to frontend-app-gradebook:

   DEFAULT_SERVICES ?= \
   credentials+discovery+ecommerce+edx_notes_api+forum+frontend-app-publisher+frontend-app-learning+frontend-app-gradebook+lms+studio+frontend-app-library-authoring
   

2. The MFE instructions include setting up blockstore, which you'll also need to have running.
3. Install the latest master of labxchange-xblocks into your lms and studio virtualenvs:

   # make lms-shell, or make studio-shell
   pip install -e git+https://github.com/open-craft/labxchange-xblocks.git@master#egg=labxchange-xblocks
   

4. Sign in to Studio and visit the Libraries link, and ensure this takes you to the library authoring MFE.
5. Create a new Library.
6. Add Problem, Video, HTML, and Poll blocks to the library.

Running automated tests

Manually run the new/fixed tests, which are skipped in CI:

1. In the blockstore dir, run make testserver to launch the test container.
   This can be run alongside the main blockstore easyserver.
2. In the Studio shell, run:

   EDXAPP_RUN_BLOCKSTORE_TESTS=yes ENABLE_ELASTICSEARCH_FOR_TESTS=yes pytest --ds=cms.envs.test  openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py

Test APIs

If you are running the labxchange devstack:

1. Visit the Library link
2. Locate a Video block, a Question block, and a Text block, and note their IDs.

Otherwise:

1. Sign in to Studio and visit the Libraries link.
2. Note the IDs for the Video, Problem, HTML, and Poll blocks you created above.

Test the metadata API for each block ID:

1. Visit http://localhost:18000/api/xblock/v2/xblocks/<block_id>/
   Note the returned block_type should be one of video, problem, html, or poll.
2. Visit http://localhost:18000/api/xblock/v2/xblocks/<block_id>/?lx_block_types=1
   Note that the returned block_type is now one of lx_video, lx_question, lx_html, or poll (unchanged)

Test the handler URL:

1. Visit http://localhost:18000/api/xblock/v2/xblocks/<block_id>/handler_url/student_view_user_state?lx_block_types=1
   Note that the returned handler_url also has the lx_block_types=1 parameter appended.
2. Visit http://localhost:18000/api/xblock/v2/xblocks/<block_id>/handler_url/student_view_user_state
   Note the returned handler_url does not have any parameters appended.

Note: If you follow the handler_url link for a block, it will only work if the returned block type contains that handler. E.g., student_view_user_state will work when problem is overridden to lx_question since the QuestionBlock has that handler. But the ProblemBlock doesn't, so it will return a 500. The VideoBlock has student_view_user_state in both the Open edX and LabXchange versions of the block.

Deadline

None

Author Notes & Concerns

1. This change is temporary, and will be reverted by a future PR from the LabXchange group: LX-2450

2. This PR also adds HTML templates to the common/test dir so that the tests in test_content_libraries.py will succeed.

   The content library tests are not run as part of the normal CI, and were failing when I ran them in my devstack. As noted under "Running automated tests", these tests must be run using the CMS test settings in order to create content libraries and blocks. Unlike the lms.env.test MAKO_TEMPLATE_DIRS_BASE setting, cms.env.test MAKO_TEMPLATE_DIRS_BASE does not include lms/templates in its list of template directories, and so the content library tests were unable to render problems and videos inside the CMS test context.

   Simply adding lms/templates to the list of cms test mako template dirs fixed this issue, but unfortunately broke a number of unrelated tests in the process. So instead, I cpoied the minimum set of templates from lms/templates to common/test, which allowed the CI to succeed and the content libraries tests to pass.

We'll try to fix this properly as part of the BD-14 Blockstore Libraries project.

[openedx-webhooks]

Thanks for the pull request, @pomegranited! I've created OSPR-6255 to keep track of it in JIRA, where we prioritize reviews. 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.

[natabene]

@pomegranited Thank you for your contribution. Please let me know once this is ready for our review.

[samuelallan72]

@pomegranited I'm not sure about this api - it's not clear how the from and to values relate (as in the order). Both from seeing an example querystring and the docstring here. Also then we need to define in the docs the behaviour when the two lists are not the same length.

?from_block_types=old1,old2,old3&to_block_types=new1,new2,new3

Idk it seems very brittle. Two separate arguments being two parts of the same operation. Need to be the same length and same order, clients and the server need to do fancy things with zip to create or parse. It's not clear what is happening when reading it.

Maybe we could use something like this instead?:

?block_override=old1:new1&block_override=old2:new2

The code would need to look something like:

requested_overrides = request_args.getlist('block_overrides', [])

overrides = {}
for item in requested_overrides:
    # split on `:` (or whatever deliminator we want)
    # verify there are exactly two non-empty entries in the split list
    # (ignore or return 400 on issues?)
    # overrides[old] = new

return overrides

CC @symbolist

[pomegranited]

Yeah, there's pros and cons to both, and no clear convention on what to do in this case. The edx REST API conventions want comma-separated strings for list parameters, but they don't say anything about maps.

I liked this way because it was easy to generate from a dict (since python3 orders keys/values on insertion), and easy to reassemble using zip.

We'd have the same issue with having to check parameter format and empty-strings if we use a separator like : too, so I'm inclined to keep this format as it was what was requested in the ticket. Will change it if the upstream reviewers object.

[pomegranited]

From @bradenmacdonald 's comment:

RE the query string API, my personal preference is strongly for ?block_override=old1:new1&block_override=old2:new2 but don't take that as the final word on the subject.

I'm ok with this approach.. who's the best person at Open edX to ask now that Nimisha's gone? CC @ormsbee

[pomegranited]

NVM -- we're classifying this as a temporary change, so aren't going to spend more time debating how the query parameters are passed in.

[pomegranited]

NVM again -- for security reasons, we've hardcoded the block override types, and are using a simple boolean argument instead: ?lx_block_types=1.

[natabene]

@pomegranited Is this ready?

[edx-status-bot]

Your PR has finished running tests. There were no failures.

[pomegranited]

Hi @natabene yep it's ready for review now. We still need to review internally, but it would be good to know if we can use a core committer review for this? i.e. @Agrendalath or @bradenmacdonald

[bradenmacdonald]

@pomegranited @swalladge Is this temporary code or do you intend this to be a long-term feature of the platform? Because I'm struggling to think of a justification or even another use case for this feature other than the current circumstances that you described. Generally, even two different "video" XBlocks are going to have wildly different fields and data formats and not be interchangeable in this way.

Perhaps if this feature allowed rolling out multiple versions of the same XBlock so that one can progressively test it with more users, it would be more generally useful. But I'm not sure that's even possible with python. What are your thoughts?

RE the query string API, my personal preference is strongly for ?block_override=old1:new1&block_override=old2:new2 but don't take that as the final word on the subject.

[samuelallan72]

@pomegranited 👍 leaving my thumbsup here, since I tested as part of opencraft/client/LabXchange/labxchange-dev!1578.

Note that the design for the api parameters haven't been settled on yet.

• I tested this: followed test instructions on opencraft/client/LabXchange/labxchange-dev!1578
• I read through the code and considered the security, stability and performance implications of the changes.
• I tested that the UI can be used with a keyboard only (tab order, keyboard controls).
• Includes tests for bugfixes and/or features added.
• Includes documentation
• Includes fixtures that create objects required for manual testing.

[samuelallan72]

@bradenmacdonald

Is this temporary code or do you intend this to be a long-term feature of the platform?

I think @symbolist has the most context on the longer term plans for this.

[symbolist]

@bradenmacdonald I only have the current migration in mind. Agree hard to think of other cases where it would be useful. 🤔 We can also remove the code later.

[pomegranited]

@bradenmacdonald @symbolist

Perhaps if this feature allowed rolling out multiple versions of the same XBlock so that one can progressively test it with more users, it would be more generally useful. But I'm not sure that's even possible with python. What are your thoughts?

Pip doesn't support installing multiple versions of the same repo, but someone could add new versions to the XBlock entry points. E.g. with the Poll XBlock, mapping 'survey-2.0 = poll:SurveyBlock2. But I don't know how widely useful this would be.

And I can't think of a way to make this code less intrusive, unfortunately.

Should I mark these commits as temp: , and ensure we've got a removal plan?

[bradenmacdonald]

@pomegranited

Should I mark these commits as temp: , and ensure we've got a removal plan?

I think so yes. And in that case, I don't think it's worth spending much time on what format the query string is in, if it's just temporary. Anything that works is fine.

[pomegranited]

Thanks @bradenmacdonald -- I've updated the PR description to note that this is a temporary change, and I'll update my commit messages to temp: when I do the final squash/rebase.

Let me know if you need anything else in order to do this review?

[ormsbee]

Are there security/integrity concerns here? Can I get the answers for something by asking the API to map problem to html or some other block type that just spits out the whole text of children instead of processing it?

[pomegranited]

@ormsbee CC @symbolist @bradenmacdonald

Are there security/integrity concerns here? Can I get the answers for something by asking the API to map problem to html or some other block type that just spits out the whole text of children instead of processing it?

Hmm.. the answer is yes/maybe. 😦

The only way around this issue I can think of is if we make the type mappings configurable by Admins only (e.g. in a Site configuration or otherwise), and change the API parameter to a boolean like use_type_override=1.

Can you think of any other ways of (safely) achieving the goals here? We're working on a new runtime, but that's a ways off.

[bradenmacdonald]

I'm not really clear on what the HTML templates are doing in this PR, could you please clarify that?

Otherwise I've reviewed the code and as long as this is temporary it's good with me 👍🏻

• I did tested this - relying on @ChrisChV's test as first reviewer
• I read through the code
• I checked for accessibility issues: n/a
• Includes documentation

[pomegranited]

Thanks @bradenmacdonald !

I'm not really clear on what the HTML templates are doing in this PR, could you please clarify that?

The explanation is a little involved, so I've updated the "Author Notes & Concerns" to address your question. Does that make sense?

Let me know if you're still ok with this, and I'll rebase and squash these commits down to a temp: commit.

[bradenmacdonald]

@pomegranited I see, thanks for the clear explanation.

It's always been a pain point that those tests don't run on CI. The work to move blockstore into edx-platform should resolve that once and for all, and then we can always run the tests.

If you intend to merge those .html template changes temporarily, then revert them along with the rest of this PR after it's done, fine with me. Please proceed with merging :)

But if you were thinking of leaving those .html files in the codebase after this, I am a little hesitant; I want the tests passing but I'm worried those template files will be unmaintained and will confuse others as they did me. We should really either (A) do the fix you originally tried and fix the unexpected other test failures, or (B) move the templates to a common place, not duplicate them, or (C) ... not sure, but there's probably another option. In any case, I think that fix belongs in a dedicated PR and is more associate with the "pull blockstore into edx-platform" work

[pomegranited]

Thanks @bradenmacdonald !

It's always been a pain point that those tests don't run on CI. The work to move blockstore into edx-platform should resolve that once and for all, and then we can always run the tests.

Yep -- I'm working on that right now with SE-5256.

If you intend to merge those .html template changes temporarily, then revert them along with the rest of this PR after it's done, fine with me. Please proceed with merging :)

I split this into 2 temp commits -- one for the templates, and one for this feature, since the template changes will be reverted soon as part of BD-14, but this feature will need to stay in a little longer.

I can't merge edx-platform PRs -- could you schedule the merge with upstream? Thank you!

CC @ormsbee @symbolist @natabene

[pomegranited]

@bradenmacdonald Ooh actually looks like Monday AM EST is the designated time for merging outside OSPRs now (see (private) Slack message), so if we could get this done this morning, that would be excellent. CC @ormsbee @natabene

[symbolist]

@pomegranited Thanks!

I think it is a holiday in the US on Monday 17th January? If yes, we may want to wait until tomorrow for the merge.

[bradenmacdonald]

@pomegranited @symbolist I found out that today is an important day for some online programs so there is a feature freeze for the day (Jan 18). I can merge this for you tomorrow (Jan 19) during east coast working hours.

[pomegranited]

Lovely, thank you for keeping on top of this @bradenmacdonald :)

[bradenmacdonald]

Hmm, @symbolist or @pomegranited can you rebase this to trigger the newly required build status on CI? I'm not sure how to trigger it otherwise...

[pomegranited]

@bradenmacdonald Sure thing -- rebase pushed.

[openedx-webhooks]

@pomegranited 🎉 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]

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

[edx-pipeline-bot]

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