[s20-55] Add s3 storage support

scormxblock/scormxblock.py

@@ -1,34 +1,53 @@
+try:
+    from cStringIO import StringIO as BytesIO
+except ImportError:
+    from io import BytesIO
 import json
 import hashlib
 import re
 import os
 import logging
 import pkg_resources
-import shutil
 import xml.etree.ElementTree as ET
+import mimetypes
 
 from functools import partial
-from django.conf import settings
+
+import zipfile
 from django.core.files import File
 from django.core.files.storage import default_storage
 from django.template import Context, Template
 from django.utils import timezone
 from webob import Response
-from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers
+from storages.backends.s3boto import S3BotoStorage
 
 from xblock.core import XBlock
 from xblock.fields import Scope, String, Float, Boolean, Dict, DateTime, Integer
 from xblockutils.resources import ResourceLoader
 from web_fragments.fragment import Fragment
 
-
 # Make '_' a no-op so we can scrape strings
 _ = lambda text: text
 loader = ResourceLoader(__name__)
 log = logging.getLogger(__name__)
 
-SCORM_ROOT = os.path.join(settings.MEDIA_ROOT, 'scorm')
-SCORM_URL = os.path.join(settings.MEDIA_URL, 'scorm')
+
+class FileIter(object):
+    def __init__(self, _file, _type='application/octet-stream'):
+        self._file = _file
+        self.wrapper = lambda d: d
+        if _type.startswith('text'):
+            self.wrapper = lambda d: unicode(d, 'utf-8', 'replace')
+
+    def __iter__(self):
+        try:
+            while True:
+                data = self._file.read(65536)
+                if not data:
+                    return
+                yield self.wrapper(data)
+        finally:
+            self._file.close()
 
 
 @XBlock.needs('i18n')
@@ -98,8 +117,6 @@ class ScormXBlock(XBlock):
         scope=Scope.settings
     )
 
-    has_author_view = True
-
     def resource_string(self, path):
         """Handy helper for getting resources from our kit."""
         data = pkg_resources.resource_string(__name__, path)
@@ -134,15 +151,6 @@ def studio_view(self, context=None):
         frag.initialize_js('ScormStudioXBlock')
         return frag
 
-    def author_view(self, context=None):
-        html = loader.render_django_template(
-            "static/html/author_view.html",
-            context=context,
-            i18n_service=self.runtime.service(self, 'i18n')
-        )
-        frag = Fragment(html)
-        return frag
-
     @XBlock.handler
     def studio_submit(self, request, suffix=''):
         self.display_name = request.params['display_name']
@@ -154,42 +162,37 @@ def studio_submit(self, request, suffix=''):
         if hasattr(request.params['file'], 'file'):
             scorm_file = request.params['file'].file
 
-            # First, save scorm file in the storage for mobile clients
+            if default_storage.exists(self.folder_base_path):
+                log.info(
+                    'Removing previously uploaded "%s"', self.folder_base_path
+                )
+                self.recursive_delete(self.folder_base_path)
+
             self.scorm_file_meta['sha1'] = self.get_sha1(scorm_file)
             self.scorm_file_meta['name'] = scorm_file.name
             self.scorm_file_meta['path'] = path = self._file_storage_path()
             self.scorm_file_meta['last_updated'] = timezone.now().strftime(DateTime.DATETIME_FORMAT)
 
-            if default_storage.exists(path):
-                log.info('Removing previously uploaded "{}"'.format(path))
-                default_storage.delete(path)
-
+            # First, extract zip file
+            with zipfile.ZipFile(scorm_file, "r") as scorm_zipfile:
+                for zipinfo in scorm_zipfile.infolist():
+                    if not zipinfo.filename.endswith("/"):
+                        zip_file = BytesIO()
+                        zip_file.write(scorm_zipfile.open(zipinfo.filename).read())
+                        default_storage.save(
+                            os.path.join(self.folder_path, zipinfo.filename),
+                            zip_file,
+                        )
+                        zip_file.close()
+
+            scorm_file.seek(0)
+
+            # Then, save scorm file in the storage for mobile clients
             default_storage.save(path, File(scorm_file))
             self.scorm_file_meta['size'] = default_storage.size(path)
             log.info('"{}" file stored at "{}"'.format(scorm_file, path))
 
-            # Check whether SCORM_ROOT exists
-            if not os.path.exists(SCORM_ROOT):
-                os.mkdir(SCORM_ROOT)
-
-            # Now unpack it into SCORM_ROOT to serve to students later
-            path_to_file = os.path.join(SCORM_ROOT, self.location.block_id)
-
-            if os.path.exists(path_to_file):
-                shutil.rmtree(path_to_file)
-
-            if hasattr(scorm_file, 'temporary_file_path'):
-                os.system('unzip {} -d {}'.format(scorm_file.temporary_file_path(), path_to_file))
-            else:
-                temporary_path = os.path.join(SCORM_ROOT, scorm_file.name)
-                temporary_zip = open(temporary_path, 'wb')
-                scorm_file.open()
-                temporary_zip.write(scorm_file.read())
-                temporary_zip.close()
-                os.system('unzip {} -d {}'.format(temporary_path, path_to_file))
-                os.remove(temporary_path)
-
-            self.set_fields_xblock(path_to_file)
+            self.set_fields_xblock()
 
         return Response(json.dumps({'result': 'success'}), content_type='application/json', charset="utf8")
 
@@ -271,33 +274,44 @@ def get_context_studio(self):
     def get_context_student(self):
         scorm_file_path = ''
         if self.scorm_file:
-            scheme = 'https' if settings.HTTPS == 'on' else 'http'
-            scorm_file_path = '{}://{}{}'.format(
-                scheme,
-                configuration_helpers.get_value('site_domain', settings.ENV_TOKENS.get('LMS_BASE')),
-                self.scorm_file
-            )
+            if isinstance(default_storage, S3BotoStorage):
+                scorm_file_path = self.runtime.handler_url(self, 's3_file', self.scorm_file)
+            else:
+                scorm_file_path = default_storage.url(self.scorm_file)
 
         return {
             'scorm_file_path': scorm_file_path,
             'completion_status': self.get_completion_status(),
             'scorm_xblock': self
         }
 
+    @XBlock.handler
+    def s3_file(self, request, suffix=''):
+        filename = suffix.split('?')[0]
+        _type, encoding = mimetypes.guess_type(filename)
+        _type = _type or 'application/octet-stream'
+        res = Response(content_type=_type)
+        res.app_iter = FileIter(default_storage.open(filename, 'rb'), _type)
+        return res
+
     def render_template(self, template_path, context):
         template_str = self.resource_string(template_path)
         template = Template(template_str)
         return template.render(Context(context))
 
-    def set_fields_xblock(self, path_to_file):
+    def set_fields_xblock(self):
         self.path_index_page = 'index.html'
+
+        imsmanifest_path = os.path.join(self.folder_path, "imsmanifest.xml")
         try:
-            tree = ET.parse('{}/imsmanifest.xml'.format(path_to_file))
+            imsmanifest_file = default_storage.open(imsmanifest_path)
         except IOError:
             pass
         else:
+            tree = ET.parse(imsmanifest_file)
+            imsmanifest_file.seek(0)
             namespace = ''
-            for node in [node for _, node in ET.iterparse('{}/imsmanifest.xml'.format(path_to_file), events=['start-ns'])]:
+            for node in [node for _, node in ET.iterparse(imsmanifest_file, events=['start-ns'])]:
                 if node[0] == '':
                     namespace = node[1]
                     break
@@ -317,7 +331,7 @@ def set_fields_xblock(self, path_to_file):
             else:
                 self.version_scorm = 'SCORM_12'
 
-        self.scorm_file = os.path.join(SCORM_URL, '{}/{}'.format(self.location.block_id, self.path_index_page))
+        self.scorm_file = os.path.join(self.folder_path, self.path_index_page)
 
     def get_completion_status(self):
         completion_status = self.lesson_status
@@ -330,15 +344,28 @@ def _file_storage_path(self):
         Get file path of storage.
         """
         path = (
-            '{loc.org}/{loc.course}/{loc.block_type}/{loc.block_id}'
-            '/{sha1}{ext}'.format(
-                loc=self.location,
-                sha1=self.scorm_file_meta['sha1'],
+            '{folder_path}{ext}'.format(
+                folder_path=self.folder_path,
                 ext=os.path.splitext(self.scorm_file_meta['name'])[1]
             )
         )
         return path
 
+    @property
+    def folder_base_path(self):
+        """
+        Path to the folder where packages will be extracted.
+        """
+        return os.path.join(self.location.block_type, self.location.course, self.location.block_id)
+
+    @property
+    def folder_path(self):
+        """
+        This path needs to depend on the content of the scorm package. Otherwise,
+        served media files might become stale when the package is update.
+        """
+        return os.path.join(self.folder_base_path, self.scorm_file_meta["sha1"])
+
     def get_sha1(self, file_descriptor):
         """
         Get file hex digest (fingerprint).
@@ -368,6 +395,18 @@ def student_view_data(self):
             }
         return {}
 
+    def recursive_delete(self, root):
+        """
+        Recursively delete the contents of a directory in the Django default storage.
+        Unfortunately, this will not delete empty folders, as the default FileSystemStorage
+        implementation does not allow it.
+        """
+        directories, files = default_storage.listdir(root)
+        for directory in directories:
+            self.recursive_delete(os.path.join(root, directory))
+        for f in files:
+            default_storage.delete(os.path.join(root, f))
+
     @staticmethod
     def workbench_scenarios():
         """A canned scenario for display in the workbench."""
@@ -377,4 +416,4 @@ def workbench_scenarios():
                 <scormxblock/>
                 </vertical_demo>
              """),
-        ]
+        ]

scormxblock/tests.py

@@ -13,6 +13,24 @@
 
 @ddt
 class ScormXBlockTests(unittest.TestCase):
+    class MockZipf:
+        def __init__(self):
+            self.files = [mock.Mock(filename='foo.csv')]
+
+        def __iter__(self):
+            return iter(self.files)
+
+        def __enter__(self):
+            return self
+
+        def __exit__(self, exc_type, exc_val, exc_tb):
+            return True
+
+        def infolist(self):
+            return self.files
+
+        def open(self, *args, **kwargs):
+            return self.files[0].filename
 
     def make_one(self, **kw):
         """
@@ -39,7 +57,7 @@ def test_fields_xblock(self):
         self.assertEqual(block.data_scorm, {})
         self.assertEqual(block.lesson_score, 0)
         self.assertEqual(block.weight, 1)
-        self.assertEqual(block.has_score, False)
+        self.assertTrue(block.has_score)
         self.assertEqual(block.icon_class, 'video')
         self.assertEqual(block.width, None)
         self.assertEqual(block.height, 450)
@@ -63,23 +81,31 @@ def test_save_settings_scorm(self):
         self.assertEqual(block.height, 450)
 
     @freeze_time("2018-05-01")
+    @mock.patch('scormxblock.ScormXBlock.recursive_delete')
     @mock.patch('scormxblock.ScormXBlock.set_fields_xblock')
-    @mock.patch('scormxblock.scormxblock.shutil')
-    @mock.patch('scormxblock.scormxblock.SCORM_ROOT')
     @mock.patch('scormxblock.scormxblock.os')
-    @mock.patch('scormxblock.scormxblock.zipfile')
+    @mock.patch('scormxblock.scormxblock.zipfile.ZipFile')
     @mock.patch('scormxblock.scormxblock.File', return_value='call_file')
     @mock.patch('scormxblock.scormxblock.default_storage')
     @mock.patch('scormxblock.ScormXBlock._file_storage_path', return_value='file_storage_path')
     @mock.patch('scormxblock.ScormXBlock.get_sha1', return_value='sha1')
-    def test_save_scorm_zipfile(self, get_sha1, file_storage_path, default_storage, mock_file, zipfile,
-                                mock_os, SCORM_ROOT, shutil, set_fields_xblock):
+    def test_save_scorm_zipfile(
+            self,
+            get_sha1,
+            file_storage_path,
+            default_storage,
+            mock_file,
+            mock_zipfile,
+            mock_os,
+            set_fields_xblock,
+            recursive_delete,
+    ):
         block = self.make_one()
         mock_file_object = mock.Mock()
         mock_file_object.configure_mock(name='scorm_file_name')
         default_storage.configure_mock(size=mock.Mock(return_value='1234'))
         mock_os.configure_mock(path=mock.Mock(join=mock.Mock(return_value='path_join')))
-
+        mock_zipfile.return_value = ScormXBlockTests.MockZipf()
         fields = {
             'display_name': 'Test Block',
             'has_score': 'True',
@@ -100,18 +126,13 @@ def test_save_scorm_zipfile(self, get_sha1, file_storage_path, default_storage,
 
         get_sha1.assert_called_once_with(mock_file_object)
         file_storage_path.assert_called_once_with()
-        default_storage.exists.assert_called_once_with('file_storage_path')
-        default_storage.delete.assert_called_once_with('file_storage_path')
-        default_storage.save.assert_called_once_with('file_storage_path', 'call_file')
+        default_storage.exists.assert_called_once_with('path_join')
+        recursive_delete.assert_called_once_with('path_join')
+        default_storage.save.assert_any_call('file_storage_path', 'call_file')
         mock_file.assert_called_once_with(mock_file_object)
-
         self.assertEqual(block.scorm_file_meta, expected_scorm_file_meta)
-
-        zipfile.ZipFile.assert_called_once_with(mock_file_object, 'r')
-        mock_os.path.join.assert_called_once_with(SCORM_ROOT, 'block_id')
-        mock_os.path.exists.assert_called_once_with('path_join')
-        shutil.rmtree.assert_called_once_with('path_join')
-        set_fields_xblock.assert_called_once_with('path_join')
+        default_storage.save.assert_any_call('path_join', 'foo.csv')
+        set_fields_xblock.assert_called_once_with()
 
     def test_build_file_storage_path(self):
         block = self.make_one(
@@ -122,13 +143,14 @@ def test_build_file_storage_path(self):
 
         self.assertEqual(
             file_storage_path,
-            'org/course/block_type/block_id/sha1.html'
+            'block_type/course/block_id/sha1.html'
         )
 
     @mock.patch('scormxblock.ScormXBlock._file_storage_path', return_value='file_storage_path')
     @mock.patch('scormxblock.scormxblock.default_storage')
     def test_student_view_data(self, default_storage, file_storage_path):
         block = self.make_one(
+            scorm_file="url_zip_file",
             scorm_file_meta={'last_updated': '2018-05-01', 'size': 1234}
         )
         default_storage.configure_mock(url=mock.Mock(return_value='url_zip_file'))
@@ -142,7 +164,8 @@ def test_student_view_data(self, default_storage, file_storage_path):
             {
                 'last_modified': '2018-05-01',
                 'scorm_data': 'url_zip_file',
-                'size': 1234
+                'size': 1234,
+                'index_page': None
             }
         )