Make blockstore an installable package [SE-4987]

[jvdm]

Description

This is a follow-up PR built on top of @symbolist's #135. It's still part of #97, and is a step towards its completion.

The previous description holds, and is copied verbatim here for reference:

• Move apps/api to apps/rest_api.
• Copy the latest code in edx-platform/openedx/core/lib/blockstore_api to blockstore/apps/api. This has been done to make the review of Blockstore as App [SE-3321] Blockstore as App [SE-3321] #97 easier since we will get a useful diff of changes made to each of the API methods.
• Makes the apps into an installable package.
• It also disables the migration code which changes the MySQL character set to utfmb4 to prevent accidentally running it against an edxapp database.

Author Comments, Concerns, and Open Questions

Co-authored-by: Usman Khalid 2200617@gmail.com

Test Instructions

In your devstack:

• Go to your devstack, enable frontend-app-library-authoring, put everything up.

• Drop any potential blockstore tables, see:

  make dev.shell.mysql57
  mysql -u root
  DROP DATABASE blockstore_db;
  

• Provision blockstore by following its README steps. This should create the tables from master.

• Run tests with make test.

• Go to localhost:3001, create a library.

• Create any block inside the library.

• Got to blockstore and stop blockstore development service.

• Checkout the development branch jvdm/packagify-blockstore.

• Provision blockstore one more time.

• Open localhost:3001 and verify the library and block opens successfully.

• Add blockstore.apps.bundles.apps.BundlesConfig, to lms/envs/common.py::INSTALLED_APPS.

• Install the package in LMS:

  make dev.shell.lms
  pip install -e /edx/src/blockstore
  python manage.py lms migrate
  

• Open the django shell python manage.py lms shell and check the API methods
  can be called:

  from blockstore.api import *
  create_collection("Collection") # This should fail with requests.exceptions.ConnectionError
  

[openedx-webhooks]

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

[kdmccormick]

Thanks for the PR @jvdm @jristau1984 - I'll leave a review this week.

[ormsbee]

Minor bit about Django versions, and some questions about what the final state is going to be.

Another question: What was the root cause of #135 getting reverted (#138)?

Thank you.

[ormsbee]

Shouldn't this be Django 3.2?

[jvdm]

+1

[ormsbee]

Is this necessary to do? These API calls return models that echo models that are in blockstore itself. Also, it's inconsistent with more recent practice in OEP-49 to create a separate set of attrs data structures to pass back in these situations.

I realize this is an intermediate state, and I imagine a lot of the models brought over from edx-platform's client-side code. My main concern is that we'd be introducing an api module with some implied level of stability, and then possibly changing all of it.

Is the plan for what happens to the API redundancies written down somewhere? I'm fine with this as an intermediate state, but I'd like to understand where we're going with collapsing these down?

[symbolist]

@ormsbee

I agree that eventually we should only have one set of attrs. However, the problem is that there are differences between the two sets of attrs. The easiest way to allow fast rollbacks and to keep the changes easy to understand was to keep them all in the methods.api file. If we try to collapse them now, it would require making changes to callers in various places in edx-platform and make rollbacks harder. This way ensures that from the point of view of callers, the API hasn't changed. And once blockstore is an in-process app, we can incrementally update the APIs and their callers.

My main concern is that we'd be introducing an api module with some implied level of stability, and then possibly changing all of it.

Agree with this. @jvdm We should remove this file for now and only add methods later that we want to mark stable.

Is the plan for what happens to the API redundancies written down somewhere? I'm fine with this as an intermediate state, but I'd like to understand where we're going with collapsing these down?

Nope, not yet. I was thinking we would look at the usages in edx-platform and then see how the APIs should be updated for better performance and ergonomics – the system boundary has changed and there may be opportunities for these?

[jvdm]

@jvdm We should remove this file for now and only add methods later that we want to mark stable.

+1

[ormsbee]

I was thinking we would look at the usages in edx-platform and then see how the APIs should be updated for better performance and ergonomics – the system boundary has changed and there may be opportunities for these?

Sounds good.

[symbolist]

@jvdm Can you please keep the original commits? They are different changes and have separate commit messages so keeping them separate is important.

[symbolist]

@ormsbee

Another question: What was the root cause of #135 getting reverted (#138)?

Hmm, I left a comment somewhere but am unable to find it now. Basically I had updated the label of the bundles app to namespace it but forgot it would cause the models to assume a different table name (and none of the reviewers caught that because everyone was testing with a fresh db). 🙂

[jvdm]

@ormsbee Leaving a ping here, we have updated the PR with the requested changes you left. Let us know if you would like to follow up on anything else. Thank you.

[ormsbee]

Thanks @jvdm. I'm approving the changes with two minor caveats:

1. Please change the commit message to include more of the context from the PR message.
2. I don't think I have the access to deploy this any longer, so I'm tagging @kenclary and @connorhaugh on that front.

Also, in the future, please don't squash the commits until the review is finished. It just makes it easier to see what's changed since the last review round. Thank you.

[jvdm]

@ormsbee:

Please change the commit message to include more of the context from the PR message.

Done.

I don't think I have the access to deploy this any longer, so I'm tagging @kenclary and @connorhaugh on that front.

Cool, thanks.

[kdmccormick]

@jvdm , once this merges, what remains to be done in #97 ?

[openedx-webhooks]

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

[connorhaugh]

This seems to have deployed with no noticeable errors.

[symbolist]

@kdmccormick

once this merges, what remains to be done in #97 ?

Steps 2-4 mentioned in https://tasks.opencraft.com/browse/EDXCL-4 🙂

[kdmccormick]

Thanks @symbolist , I think that ticket requires a tasks.opencraft.com account -- would I be able to get an account?

[jristau1984]

Steps 2-4 from the ticket above copied here for reference:

2 - Replace calls to Blockstore from edx-platform to the Python API from the new, migrated, blockstore package.

3 - There are a few tests which are integration tests but need to become unit tests (since blockstore is going to be an installed app). These need to be verified and any fixes made.

4 - There is a bug related to how the media urls for files in bundles are passed around in the devstack that needs to be resolved.

[kdmccormick]

Thanks @jristau1984 !

It seems like EDXCL-4's Step 2 corresponds to Step 2 from the TNL migration plan, which is great.

Heads up that the TNL plan asks for Step 2 to be done behind a feature flag, and that it requires Step 1 from the TNL plan (giving edxapp access to the blockstore DB) to be done first.