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

Adds django storage backend for ORA2 file uploads. #1018

Merged
merged 1 commit into from Jul 14, 2017

Conversation

pomegranited
Copy link
Contributor

@pomegranited pomegranited commented Jun 30, 2017

Adds a fileupload backend named "django", which uses the default django storage backend.

OpenCraft needs this change because the current swift backend doesn't work with OVH's SWIFT service. The swift backend relies on generating temporary POST/GET URLs which are used to upload/download files, but OVH throws CORS-related errors when attempting to use these URLs in the browser.

Instead of attempting to change the swift backend to work with OVH, we decided to add a django storage backend, which will work with the currently configured django storage settings on the platform. This fixes the issue for us with OVH, but also allows file uploads to work by default in the devstack.

JIRA tickets: OSPR-1808

Sandbox URL:

Partner information: 3rd party-hosted open edX instance

Merge deadline: None

Testing instructions:

Devstacks use django FileSystemStorage by default, so they're a good place to test this change using django.core.files.storage.FileSystemStorage.

  1. Checkout open-craft:jill/ora2-django-storage, the branch used in https://github.com/edx/edx-platform/pull/15446. This change:
    • adds the ORA2 fileupload URLs to lms/urls.py
    • sets ORA2_FILEUPLOAD_BACKEND = "django" in lms/envs/devstack.py
    • updates requirements/edx/base.txt to install ORA2 from this branch.
  2. In vagrant as the edxapp user, run paver run_all_servers
  3. Sign in to Studio, and create a new course.
  4. Add an Open Response Assessment problem, and click Edit.
  5. Click Settings, and scroll down to set:
    • File Uploads Response: Optional or Required.
    • File Upload Types: select any option.
  6. Preview the new Unit
  7. Upload one or more files of the appropriate type, add captions, and click Upload Files to save.
  8. Ensure that the uploaded files are saved as expected, and can be viewed/downloaded.
  9. Refresh the unit to make sure the uploaded files are still displayed.

The sandbox shown above uses SWIFT storage on OVH, and so it's a good place to test this change using swift.storage.SwiftStorage.

  1. Sign into the sandbox server using any standard test account.
  2. Enrol in the test course, ORA2 Django Storage.
  3. Visit the configured ORA2 unit
  4. Follow steps 7-9 above to verify file uploads.

Author notes and concerns:

This new django backend could in theory replace the existing backends, since most deployments are likely to use the same storage mechanism for ORA2 that they use on the platform.
However, in practice, it was impossible to make the django backend preserve the same file destinations between all mechanisms, and so changing the backend on an existing deployment could result in lost submissions.
Note: ORA2 appends the [/{index}] suffix for multiple file uploads, when {index} > 0. When {index} == 0, it is omitted.
I.e.,

  • ORA2_FILEUPLOAD_BACKEND = 's3' (default): stores to {bucket}/{prefix}/{key}[/{index}]
  • ORA2_FILEUPLOAD_BACKEND = 'swift': stores to {bucket}/{prefix}/{key}[/{index}]
  • ORA2_FILEUPLOAD_BACKEND = 'filesystem': stores two files, {media_root}/{bucket}/{prefix}/{key}[/{index}]/content and {media_root}/{bucket}/{prefix}/{key}[/{index}]/metadata.json, and creates any subdirectories required.
  • ORA2_FILEUPLOAD_BACKEND = 'django': stores to {storage_root}/{prefix}/{key}[_{index}].
    The destination filename must be flattened from {key}[/{index}] to {key}[_{index}] because it must be storage-agnostic, and so can't create subdirectories. Also, it must gracefully handle the cases where {index} == 0 and is therefore omitted from the key by ORA2, and where {index} > 0, when it is included.
    Specifically, if we didn't replace the dir separator with an unerscore and the default django storage points to the local filesystem, then the first file would write to {storage_root}/{prefix}/{key}, and the next file would fail to write to {storage_root}/{prefix}/{key}/1, because {storage_root}/{prefix}/{key} already exists as a file, not a directory.

Reviewers

@openedx-webhooks
Copy link

Thanks for the pull request, @pomegranited! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

Create an OSPR issue for this pull request.

@pomegranited pomegranited force-pushed the jill/django-file-upload branch 2 times, most recently from 532e35f to a60c429 Compare July 1, 2017 01:19
@pomegranited pomegranited force-pushed the jill/django-file-upload branch 10 times, most recently from 5128692 to 197ee00 Compare July 3, 2017 00:27
@pomegranited pomegranited changed the title [WIP] Adds django storage backend for ORA2 file uploads. Adds django storage backend for ORA2 file uploads. Jul 3, 2017
Copy link
Contributor

@bdero bdero left a comment

Choose a reason for hiding this comment

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

I just had one request to add docstrings. Once tests pass, this is a 👍 from me!

  • I tested this: I made a user on the sandbox and verified that file uploads work for ORA2 (I also verified the sandbox settings)
  • I read through the code
  • I checked for accessibility issues N/A
  • Includes documentation

def tearDown(self):
self.backend.remove_file(self.key)

def test_get_backend(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please include docstrings on these tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed by bb88ed1

@pomegranited pomegranited force-pushed the jill/django-file-upload branch 3 times, most recently from 84d18e9 to 3a9ec7e Compare July 4, 2017 14:38
@openedx-webhooks
Copy link

Thanks for the pull request, @pomegranited! I've created OSPR-1808 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email 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 still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

If you like, you can add yourself to the AUTHORS file for this repo, though that isn't required. Please see the CONTRIBUTING file for more information.

@pomegranited
Copy link
Contributor Author

Hi @nedbat ! We've asked for this and its related https://github.com/edx/edx-platform/pull/15446 to be considered for the Ginkgo release. Do you think that's feasible? Is there anything else we can do to make these PRs easier to verify and merge?

CC @gsong

@nedbat
Copy link
Contributor

nedbat commented Jul 13, 2017

We're trying to move this up the queue to get it in before the Ginkgo rc.

@pomegranited
Copy link
Contributor Author

Thank you, @nedbat !

Copy link
Contributor

@efischer19 efischer19 left a comment

Choose a reason for hiding this comment

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

This looks great to me, I've confirmed it works on my local devstack and on your swift sandbox. Let's squash and get this merged.

Could you expand on this comment a bit? How exactly would submissions be lost? I'm understanding it as "there's no way to move from location A to location B", which seems reasonable. This isn't setting a default, so deployments that already use file upload will not be affected unless they opt-in, correct?

However, in practice, it was impossible to make the django backend preserve the same file destinations between all mechanisms, and so changing the backend on an existing deployment could result in lost submissions.

@pomegranited
Copy link
Contributor Author

pomegranited commented Jul 14, 2017

Thank you @efischer19 ! I've squashed and rebased on the latest master, and will merge once tests pass. EDIT: Just saw your comment https://github.com/edx/edx-platform/pull/15446#discussion_r127292036 -- will leave the tagging to you.

Could you expand on this comment a bit? How exactly would submissions be lost? I'm understanding it as "there's no way to move from location A to location B", which seems reasonable.

Yes, that's correct. What I was trying to say was, if you change the ORA2 fileupload backend from e.g. filesystem to django-storage, even if your django-storage backend is configured to use the filesystem too, the destination for the ORA2 submissions will differ. New submissions will work fine, but any existing submissions will not be found.

This isn't setting a default, so deployments that already use file upload will not be affected unless they opt-in, correct?

Correct.

@pomegranited pomegranited merged commit 2f57641 into openedx:master Jul 14, 2017
@pomegranited pomegranited deleted the jill/django-file-upload branch July 14, 2017 01:37
@pomegranited pomegranited restored the jill/django-file-upload branch July 14, 2017 01:38
@bradenmacdonald bradenmacdonald deleted the jill/django-file-upload branch July 19, 2017 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engineering review open-source-contribution PR author is not from Axim or 2U
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants