Add a content libraries import from courseware API and management command [SE-4619]

[jvdm]

Description

This PR introduces the import from course functionality to content libraries.

The functionality is exposed by (a.) A RESTful API, and (b.) A management command. Both are added to Content Libraries V2. They allow block importing from a local modulestore instance. Additionally, the command line supports importing blocks from a remote platform instance through API calls.

The management command allows platform operators to import blocks from existing courseware and remote instances into blockstore to allow them to be accessed through Libraries v2.

The APIs allow course teams to manage background tasks (Celery workers) that will perform the importing while providing persisted information for querying the state of the tasks. It is suitable to be used by UI interfaces to allow customers of Libraries V2 to select Organisations and Courses to import content from and query the state and progress of the import task.

Adding this command and API doesn't cause any immediate implication to users or operators.

Reviewers

• @arbrandes
• @bradenmacdonald
• @doctoryes

Supporting information and related

See the changes to Content Libraries MFE.

Testing instructions

1. Deploy an EdX devstack.
2. Enable API access to the admin user.
3. Follow the instruction to enable Content Libraries and Blockstore.
4. Log in to Studio, go to Content Libraries V2 UI (default devstack URL: http://localhost:3001).
5. Create a library, grab its "usage key" from the URL.

Then execute the following:

For API-based importing:

sudo -Hu edxapp bash
cd && . edxapp_env  && . ./venvs/edxapp/bin/activate && cd edx-platform/
OAUTH_KEY=
OAUTH_SECRET=
cat > oauth.creds <<EOF
${OAUTH_KEY)
$(OAUTH_SECRET)
EOF
python manage.py cms content_libraries_import lib:OC:cs course-v1:edX+DemoX+Demo_Course \
    api \
    --lms-url http://edx.devstack.lms:18000 --studio-url http://edx.devstack.studio:18010 --oauth-creds-file oauth.creds 

For local importing:

python manage.py cms content_libraries_import lib:OC:cs course-v1:edX+DemoX+Demo_Course modulestore

To test the APIs, please refer to the MFE PR.

Deadline

None.

Other information

No dependencies or special concerns.

Settings:

EDXAPP_FEATURES:
    ENABLE_LIBRARY_AUTHORING_MICROFRONTEND: true

EDXAPP_LIBRARY_AUTHORING_MICROFRONTEND_URL: "https://library-authoring.{{ EDXAPP_LMS_BASE }}"

TRUSTED_DOMAINS:
  - "library-authoring.{{ EDXAPP_LMS_BASE }}"

EDXAPP_ENABLE_CORS_HEADERS: yes
EDXAPP_ENABLE_CROSS_DOMAIN_CSRF_COOKIE: yes
EDXAPP_CORS_ORIGIN_WHITELIST: "{{ TRUSTED_DOMAINS }}"
EDXAPP_LOGIN_REDIRECT_WHITELIST: "{{ TRUSTED_DOMAINS }}"
EDXAPP_CSRF_TRUSTED_ORIGINS: "{{ TRUSTED_DOMAINS }}"

MFE_DEPLOY_STANDALONE_NGINX: yes

MFES:
  - name: library-authoring
    repo: frontend-app-library-authoring
    env_extra:
        STUDIO_BASE_URL: https://{{ EDXAPP_CMS_BASE }}

[openedx-webhooks]

Thanks for the pull request, @jvdm! I've created OSPR-5800 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]

@jvdm Thank you for your contribution. Please let me know it is ready for our review.

[jvdm]

@jvdm Thank you for your contribution. Please let me know it is ready for our review.

It is ready. Thanks.

[natabene]

@sarina I thinks this might be with your team, could someone review it? If not, please point me in the right direction.

[sarina]

Content libraries & blockstore belong to T&L, thanks!

[natabene]

@sarina Indeed, thanks, sorry about that.

[arch-bom-gocd-alerts]

📣 💥 Heads-up: You must either rebase onto master or merge master into your branch to avoid breaking the build.

We recently removed diff-quality and introduced lint-amnesty. This means that the automated quality check that has run on your branch doesn't work the same way it will on master. If you have introduced any quality failures, they might pass on the PR but then break the build on master.

This branch has been detected to not have commit 2e33565 as an ancestor. Here's how to see for yourself:

git merge-base --is-ancestor 2e335653 jvdm/libraries-v2-import-from-courseware && echo "You're all set" || echo "Please rebase onto master or merge master to your branch"

If you have any questions, please reach out to the Architecture team (either #edx-shared-architecture on Open edX Slack or #architecture on edX internal).

[natabene]

@jvdm Could you look into the failing test?

[arbrandes]

@marcotuts, this is the backend component of the import-from-course-to-library feature of frontend-app-library-authoring, in case can also review it.

@bradenmacdonald, as core contributor and original author of a lot of related code, is this something you'd have time to review?

[bradenmacdonald]

@arbrandes Yes I'd be happy to review.

[bradenmacdonald]

Very nice work. Just some minor comments and requests.

[bradenmacdonald]

Can you please add a comment to explain why you're calling a get_ method without using its result? Is this to verify that the block does in fact exist?

Also, is silently ignoring LibraryBlockAlreadyExists the best way to handle it? Are these imports supposed to be idempotent?

If we expect people to use a workflow that involves running the same import command against a course many times over, then this is fine. But I would think that at least printing a warning here is in order. After all, some people may use a workflow of trying to import content from several different courses into the same content library, and if those different courses contained some different content with the same block ID, only one would get imported (with no warning about conflicts), although it would have all the static files from every different version of that block in each course.

To clarify: Studio gives each block a very random usage_id when you create a block in Studio, but many courses on edx.org are hand-authored and those ones have custom IDs for each block like "problem1", "problem2", etc. where collisions among different courses can be very likely. It's not something we need to spend a lot of time designing around, but we should at least warn about it if it may be happening.

[jvdm]

Can you please add a comment to explain why you're calling a get_ method without using its result? Is this to verify that the block does in fact exist?

Indeed, we want to make sure the library block is there since the blockstore_key was generated by hand. I admit I included it as a "assert"-like safety-net on my end rather than an actual scenario I've faced. However, it looks like you're OK with it as long as we clarify why we are doing it. I'll add a comment.

Also, is silently ignoring LibraryBlockAlreadyExists the best way to handle it? Are these imports supposed to be idempotent?

You are right. Those imports are supposed to be idempotent. Or at least partial failures should leave the import in a state where retrying is safe. Also, the command should allow sync from the courseware's course into the library by re-running with the same parameters. You got the right intention.

To clarify: Studio gives each block a very random usage_id when you create a block in Studio, but many courses on edx.org are hand-authored, and those have custom IDs for each block like "problem1", "problem2", etc. where collisions among different courses can be very likely. It's not something we need to spend a lot of time designing around, but we should at least warn about it if it may be happening.

That's a nice piece of information. I naively thought that modulestore block_id would carry the course_id within them. Knowing that this is not the case, does it make sense to compose the Blockstore's block usage_id with the course_key something like: usage_id=hash(modulestore_key.course_key + modulestore_key.block_id)?

[bradenmacdonald]

does it make sense to compose the Blockstore's block usage_id with the course_key something like: usage_id=hash(modulestore_key.course_key + modulestore_key.block_id)?

That's a solution that I've used before for similar problems, and I think it would work well, but it does add a layer of complexity/indirection that can make it harder to debug issues with content (because you can't easily tell from the resulting block_id which course it came from).

I would suggest that we either:
(1) Put some explicit comments in the code explaining that this is meant to be idempotent, and when the command is run, count the number of existing blocks and just print a message like ("17 blocks already existed in the destination library and were not imported."). We don't necessarily have to worry about or support the situation where users import from multiple courses to the same library, as long as we're clear about what we're doing and the potential issues for that use case.
or
(2) Do what you're suggesting but use usage_id = modulestore_key.block_id + "_" + hash(modulestore_key.course_key)[0,3] so that humans can look at the ID and tell what the original course-scoped block ID was.

[jvdm]

I like your suggested approach. Deal.

[bradenmacdonald]

It seems inefficient to get all static asset files each time through this loop - but I understand it makes for simpler code so 🤷🏼‍♀️ .

I am not aware of a bulk API to retrieve static files metadata from blockstore bundles. Indeed, that would be a great follow up for this for massive course imports (e.g. > 10K blocks). I am glad you seem inclined to agree that this can be done as a follow-up while keeping this first version simpler.

A couple points:

I think you could very easily just move this files = [ ... ] outside of the loop, and at the end of the inner loop just call files.append(filename) to achieve the same result. Wouldn't that have the same result without much more complexity?

These various get_ methods do have aggressive caching so it's normally OK to call them as often as you want, but the cache is tied to the draft version and gets cleared every time you make a change, like adding a new static asset file, so this current approach is a bit inefficient.

And re the bulk API, it does exist: get_bundle_files_cached, and to filter the results of that to those belonging to a specific XBlock you need to select those with the filename prefix returned by get_static_prefix_for_definition(_lookup_usage_key(blockstore_key)[0]) which is exactly what get_library_block_static_asset_files does.

I don't think you need to implement the latter for now, but I definitely think it's worth moving this outside of the loop.

[jvdm]

I think you could very easily just move this files = [ ... ] outside of the loop, and at the end of the inner loop just call files.append(filename) to achieve the same result. Wouldn't that have the same result without much more complexity?

+1

And re the bulk API, it does exist: get_bundle_files_cached, and to filter the results of that to those belonging to a specific XBlock you need to select those with the filename prefix returned by get_static_prefix_for_definition(_lookup_usage_key(blockstore_key)[0]) which is exactly what get_library_block_static_asset_files does.

Awesome!

I don't think you need to implement the latter for now, but I definitely think it's worth moving this outside of the loop.

Agreed, will take a note to follow up. Thank you.

[bradenmacdonald]

Seems odd that these two write lines aren't combined into a single self.write_error call?

[jvdm]

self.write_error() writes to stdout, while this line writes to stderr. This allows 2>/dev/null style calls, where stdout provides only the summary of what happened.

[bradenmacdonald]

Is that a common convention? (and if so, why is the functionality not already baked into the superclass here?) If so, then we're all good. If not super common, please just include a comment explaining it for the dummies like me :)

[jvdm]

Is that a common convention?

Talking about the *nix command line in general? I've seen it many times, although I am not entitled to say, "this is a common convention around all command lines in the world". I guess it would be a point for style and discussion. I've seen many commands regard stderr as a place to log errors and diagnostic-related information, while stdout is used to show expected output or summarized progress. One usage pattern is: You call the command with foobar 2>errors.log, get a cleaner output, easier to see the big picture, and if something went wrong and you want to dive into the details of the errors, you check stderr. It is advantageous when your command will be used in scripts or automated environments. BTW, this is what I mean when I said "2>/dev/null-style calls", although redirecting to /dev/null, you are pretty much ignoring the details. My point was, you can redirect stderr to another place other than stdout.

why is the functionality not already baked into the superclass here?

It is. They allow you to write to different output streams and allows you to pretty-print the output as well. I just added two wrappers called write_error(...) and write_success(...) to colorize the output according to upstream's convention of what would be a good color for error and success. I could just have used self.stdout.write(self.style.ERROR(...)) instead. I think it is a little bit longer and repetitive, hence the wrapper method.

If not super common, please just include a comment explaining it

I'll get rid of the helper method, which will make it explicit and less confusing.

[bradenmacdonald]

This docstring and the query_parameter docs below are wrong, because this API is only about courses, not libraries.

In fact, I don't really think this belongs in the libraries REST API area because it has nothing to do with libraries. It only lists courses that the user has Studio access to.

I believe that our current LabXchange import code makes one call to the existing /api/courses/v1/courses/ API to get all courses the user can read, then makes one call to /api/enrollment/v1/ to get all courses that the user has Instructor access to, and then combines that data.

So I don't think that a new API is needed, but if you want to make it easier for API clients, can we just add an optional query parameter to the existing /api/courses/v1/courses/ endpoint to allow filtering by role?

[jvdm]

So I don't think that a new API is needed, but if you want to make it easier for API clients, can we just add an optional query parameter to the existing /api/courses/v1/courses/ endpoint to allow filtering by role?

I totally agree here. But it is not immediately trivial to include the filter to /api/courses/v1/courses, and it would take some additional effort to get there. My suggestion is, let's go with the two API calls while we plan the work on expanding that API to accommodate this use case, since this is obviously the long-term solution.

We would have to update our MFE patch @gabor-boros. Any blockers against this approach?

[gabor-boros]

@jvdm We thought about this approach with @arbrandes, though in the middle of the implementation of the frontend I proposed to have this endpoint to simplify API client calls and I found a blocker that time.

Since then, the frontend implementation has been changed, and this approach could be solved on the frontend if needed, however, I'm a bit afraid that the API calls would be pretty slow in that case.

[bradenmacdonald]

@gabor-boros Why would those two API calls be significantly slower than this one? (I'm assuming you can make the two calls in parallel btw)

[gabor-boros]

I cannot remember exactly how the courses API response fields are prepared on the backend. As far as I remember -- I may remember incorrectly -- the data returned by that API collected from several places that slows the processing of 50 course responses compared to this API.

Though I'd like to highlight again that I thought about the solution you proposed in the first place.

[bradenmacdonald]

@arbrandes @gabor-boros We already use the existing two APIs from the LabXchange app for this exact same use case, and I believe it works fine. To handle the case of staff users who have access to hundreds of courses, the backend caches (per-user) the full list of courses that a user can see. The frontend can then get that data from the backend, paginated however it wants.

Code (not open source) is here and here.

In the new use case here, there is no separate backend to do caching but the frontend is presumably a SPA so it can just retrieve and cache the entire list of courses, then cross-reference it with the list of roles and display it paginated. In fact it's arguably better to do that on the frontend so you can display a progress bar / loading message while it loads.

[bradenmacdonald]

And to be clear: I'm happy with everything else in this PR and am currently giving it a manual test before I approve it. However, I don't like the idea of merging in an API endpoint to the library REST API section that doesn't really have anything to do with libraries, and that has so much overlap with other existing platform APIs. Although it may be convenient for this particular project now, it creates a messier result for the overall platform API set in general. And once people start using an API, it's hard to remove, so it's not a good idea to merge an API that you plan to remove anyways, unless it's very clearly marked as deprecated/unstable/temporary (which it is not).

So I would strongly prefer to leave it out of this PR for now. If necessary, can you put the code for the temporary LibraryImportableCourseView on another branch and continue using it locally for development purposes for another week or two until the MFE can be converted to use the standard APIs, and/or until we gather more information that shows a compelling need for a separate API?

[arbrandes]

@bradenmacdonald,

it can just retrieve and cache the entire list of courses, then cross-reference it with the list of roles and display it paginated.

I am suspicious of doing frontend pagination based on two or more disparate, and potentially large, data sources. Sure, it's doable, but it is complicated, second-guesses DRF, and is generally worse as far as frontend performance goes. Sure, if Open edX gives us no other solution that's what we'll have to do, but I think we should at least aim towards porting the functionality to the right namespace.

[arbrandes]

Addendum: the user-facing side supports searching, which would further complicate the frontend (and pagination).

[bradenmacdonald]

OK. My bottom line is that the endpoint in question is a type of course search/list API and it belongs in the same namespace as the other course search/list APIs, or perhaps even should be just a filter/parameter on one of those existing APIs. And because of that, I think it should at least be submitted as a separate PR so as not to hold up the rest of this course import functionality, and to make it easier for people who work on the course APIs to be aware of it and comment.

[bradenmacdonald]

@jvdm Please address the test failure and any remaining comments that you'd like to do now, and ping me when this is ready for final test + approval. If you want to defer addressing some of my comments until future, it's fine with me, as long as there is a relevant comment in the codebase itself for explanation/context/tracking.

[jvdm]

@bradenmacdonald:

@jvdm Please address the test failure and any remaining comments that you'd like to do now, and ping me when this is ready for final test + approval.

I've followed up on the pending comments, ready for final testing and approval.

The remaining points that are outstanding are:

• TODO Making the command more efficient with bulk imports
• TODO Change MFE UI to consume better import course API

I am capturing those as a follow to this PR.

[bradenmacdonald]

Began testing this today but got held up by devstack issues, sorry for the delayed approval.

[bradenmacdonald]

Looking great! I just tested this and am conditionally approving if you make two changes:

1. fix the hash function issue I pointed out.
2. Move the importable_blocks REST API to a separate PR for now.

• I tested this: testing both a modulestore and an oauth-based import of the demo course to a library
• I read through the code
• I checked for accessibility issues: n/a, no UI
• Includes documentation - comments and docstrings yes

[jvdm]

@bradenmacdonald:

2. Move the importable_blocks REST API to a separate PR for now.

If we don't ship the API, the MFE won't be able to onboard the backend. So what is the plan moving forward?

[bradenmacdonald]

@jvdm

If we don't ship the API, the MFE won't be able to onboard the backend. So what is the plan moving forward?

I am assuming that there is no immediate need to use the MFE with courses.edx.org as its backend in the coming week; if that is the case, let me know.

I was thinking that for now, you'd just move the existing API endpoint code to a separate branch/PR so that the dev team working on this project can continue using it to test the MFE, for local development, etc. And in then in that PR we can discuss if that's the right approach (I don't think it is) or figure out a new approach. But basically I think it's better to maintain that temporary endpoint as a branch rather than merging it into the codebase with the intention of soon changing or removing it.

As for what the ultimate solution should be, I think that it would be one of (A) modify the existing course list API to return the data we need, when the right filter/parameters are passed, (B) modify the frontend to just use the existing two APIs (course list + get course roles), or (C) show that this new API is something distinct and necessary, and add it to the platform (but in the course API namespace, not the library API namespace, and in a separate PR with a relevant title so that people watching for course-related API PRs will see it).

[jvdm]

@bradenmacdonald:

I was thinking that for now, you'd just move the existing API endpoint code to a separate branch/PR so that the dev team working on this project can continue using it to test the MFE, for local development, etc. And in then in that PR we can discuss if that's the right approach (I don't think it is) or figure out a new approach. But basically I think it's better to maintain that temporary endpoint as a branch rather than merging it into the codebase with the intention of soon changing or removing it.

OK, thanks for clarifying the next steps. I'll make the updates to the PR and ping you after that.

[jvdm]

@bradenmacdonald:

I was thinking that for now, you'd just move the existing API endpoint code to a separate branch/PR so that the dev team working on this project can continue using it to test the MFE, for local development, etc. And in then in that PR we can discuss if that's the right approach (I don't think it is) or figure out a new approach. But basically I think it's better to maintain that temporary endpoint as a branch rather than merging it into the codebase with the intention of soon changing or removing it.

OK, thanks for clarifying the next steps. I'll make the updates to the PR and ping you after that.

Done. BTW, here's the (draft) PR with the importable courses API: https://github.com/edx/edx-platform/pull/28464

[bradenmacdonald]

Thanks, good to merge now! Just needs to be squashed down to 1-2 commits, although I can do that as I merge if you like.

I will merge this tomorrow AM.

[jvdm]

Thanks, good to merge now! Just needs to be squashed down to 1-2 commits, although I can do that as I merge if you like.

I will merge this tomorrow AM.

Rebased onto origin/master and squashed.

[edx-status-bot]

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

[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.