Streamline and test uploading ... remove requirement that uploads be images, can…

Streamline and test uploading ... remove requirement that uploads be images, can now upload any file.

common/lib/xmodule/xmodule/combined_open_ended_module.py

@@ -306,7 +306,7 @@ class CombinedOpenEndedFields(object):
     )
     peer_grade_finished_submissions_when_none_pending = Boolean(
         display_name='Allow "overgrading" of peer submissions',
-        help=("Allow students to peer grade submissions that already have the requisite number of graders, "
+        help=("EXPERIMENTAL FEATURE.  Allow students to peer grade submissions that already have the requisite number of graders, "
               "but ONLY WHEN all submissions they are eligible to grade already have enough graders.  "
               "This is intended for use when settings for `Required Peer Grading` > `Peer Graders per Response`"),
         default=False,

common/lib/xmodule/xmodule/js/src/combinedopenended/display.coffee

@@ -372,7 +372,8 @@ class @CombinedOpenEnded
       answer_area_div = @$(@answer_area_div_sel)
       answer_area_div.html(response.student_response)
     else
-      @can_upload_files = pre_can_upload_files
+      @submit_button.show()
+      @submit_button.attr('disabled', false)
       @gentle_alert response.error
 
   confirm_save_answer: (event) =>
@@ -385,23 +386,27 @@ class @CombinedOpenEnded
     event.preventDefault()
     @answer_area.attr("disabled", true)
     max_filesize = 2*1000*1000 #2MB
-    pre_can_upload_files = @can_upload_files
     if @child_state == 'initial'
       files = ""
+      valid_files_attached = false
       if @can_upload_files == true
         files = @$(@file_upload_box_sel)[0].files[0]
         if files != undefined
+          valid_files_attached = true
           if files.size > max_filesize
-            @can_upload_files = false
             files = ""
-        else
-          @can_upload_files = false
+            # Don't submit the file in the case of it being too large, deal with the error locally.
+            @submit_button.show()
+            @submit_button.attr('disabled', false)
+            @gentle_alert "You are trying to upload a file that is too large for our system.  Please choose a file under 2MB or paste a link to it into the answer box."
+            return
 
       fd = new FormData()
       fd.append('student_answer', @answer_area.val())
       fd.append('student_file', files)
-      fd.append('can_upload_files', @can_upload_files)
+      fd.append('valid_files_attached', valid_files_attached)
 
+      that=this
       settings =
         type: "POST"
         data: fd

common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_image_submission.py

-"""
-This contains functions and classes used to evaluate if images are acceptable (do not show improper content, etc), and
-to send them to S3.
-"""
-
-try:
-    from PIL import Image
-
-    ENABLE_PIL = True
-except:
-    ENABLE_PIL = False
-
-from urlparse import urlparse
-import requests
-from boto.s3.connection import S3Connection
-from boto.s3.key import Key
-import logging
-
-log = logging.getLogger(__name__)
-
-#Domains where any image linked to can be trusted to have acceptable content.
-TRUSTED_IMAGE_DOMAINS = [
-    'wikipedia',
-    'edxuploads.s3.amazonaws.com',
-    'wikimedia',
-]
-
-#Suffixes that are allowed in image urls
-ALLOWABLE_IMAGE_SUFFIXES = [
-    'jpg',
-    'png',
-    'gif',
-    'jpeg'
-]
-
-#Maximum allowed dimensions (x and y) for an uploaded image
-MAX_ALLOWED_IMAGE_DIM = 2000
-
-#Dimensions to which image is resized before it is evaluated for color count, etc
-MAX_IMAGE_DIM = 150
-
-#Maximum number of colors that should be counted in ImageProperties
-MAX_COLORS_TO_COUNT = 16
-
-#Maximum number of colors allowed in an uploaded image
-MAX_COLORS = 400
-
-
-class ImageProperties(object):
-    """
-    Class to check properties of an image and to validate if they are allowed.
-    """
-
-    def __init__(self, image_data):
-        """
-        Initializes class variables
-        @param image: Image object (from PIL)
-        @return: None
-        """
-        self.image = Image.open(image_data)
-        image_size = self.image.size
-        self.image_too_large = False
-        if image_size[0] > MAX_ALLOWED_IMAGE_DIM or image_size[1] > MAX_ALLOWED_IMAGE_DIM:
-            self.image_too_large = True
-        if image_size[0] > MAX_IMAGE_DIM or image_size[1] > MAX_IMAGE_DIM:
-            self.image = self.image.resize((MAX_IMAGE_DIM, MAX_IMAGE_DIM))
-            self.image_size = self.image.size
-
-    def count_colors(self):
-        """
-        Counts the number of colors in an image, and matches them to the max allowed
-        @return: boolean true if color count is acceptable, false otherwise
-        """
-        colors = self.image.getcolors(MAX_COLORS_TO_COUNT)
-        if colors is None:
-            color_count = MAX_COLORS_TO_COUNT
-        else:
-            color_count = len(colors)
-
-        too_many_colors = (color_count <= MAX_COLORS)
-        return too_many_colors
-
-    def check_if_rgb_is_skin(self, rgb):
-        """
-        Checks if a given input rgb tuple/list is a skin tone
-        @param rgb: RGB tuple
-        @return: Boolean true false
-        """
-        colors_okay = False
-        try:
-            r = rgb[0]
-            g = rgb[1]
-            b = rgb[2]
-            check_r = (r > 60)
-            check_g = (r * 0.4) < g < (r * 0.85)
-            check_b = (r * 0.2) < b < (r * 0.7)
-            colors_okay = check_r and check_b and check_g
-        except:
-            pass
-
-        return colors_okay
-
-    def get_skin_ratio(self):
-        """
-        Gets the ratio of skin tone colors in an image
-        @return: True if the ratio is low enough to be acceptable, false otherwise
-        """
-        colors = self.image.getcolors(MAX_COLORS_TO_COUNT)
-        is_okay = True
-        if colors is not None:
-            skin = sum([count for count, rgb in colors if self.check_if_rgb_is_skin(rgb)])
-            total_colored_pixels = sum([count for count, rgb in colors])
-            bad_color_val = float(skin) / total_colored_pixels
-            if bad_color_val > .4:
-                is_okay = False
-
-        return is_okay
-
-    def run_tests(self):
-        """
-        Does all available checks on an image to ensure that it is okay (size, skin ratio, colors)
-        @return: Boolean indicating whether or not image passes all checks
-        """
-        image_is_okay = False
-        try:
-            #image_is_okay = self.count_colors() and self.get_skin_ratio() and not self.image_too_large
-            image_is_okay = not self.image_too_large
-        except:
-            log.exception("Could not run image tests.")
-
-        if not ENABLE_PIL:
-            image_is_okay = True
-
-        #log.debug("Image OK: {0}".format(image_is_okay))
-
-        return image_is_okay
-
-
-class URLProperties(object):
-    """
-    Checks to see if a URL points to acceptable content.  Added to check if students are submitting reasonable
-    links to the peer grading image functionality of the external grading service.
-    """
-
-    def __init__(self, url_string):
-        self.url_string = url_string
-
-    def check_if_parses(self):
-        """
-        Check to see if a URL parses properly
-        @return: success (True if parses, false if not)
-        """
-        success = False
-        try:
-            self.parsed_url = urlparse(self.url_string)
-            success = True
-        except:
-            pass
-
-        return success
-
-    def check_suffix(self):
-        """
-        Checks the suffix of a url to make sure that it is allowed
-        @return: True if suffix is okay, false if not
-        """
-        good_suffix = False
-        for suffix in ALLOWABLE_IMAGE_SUFFIXES:
-            if self.url_string.endswith(suffix):
-                good_suffix = True
-                break
-        return good_suffix
-
-    def run_tests(self):
-        """
-        Runs all available url tests
-        @return: True if URL passes tests, false if not.
-        """
-        url_is_okay = self.check_suffix() and self.check_if_parses()
-        return url_is_okay
-
-    def check_domain(self):
-        """
-        Checks to see if url is from a trusted domain
-        """
-        success = False
-        for domain in TRUSTED_IMAGE_DOMAINS:
-            if domain in self.url_string:
-                success = True
-                return success
-        return success
-
-
-def run_url_tests(url_string):
-    """
-    Creates a URLProperties object and runs all tests
-    @param url_string: A URL in string format
-    @return: Boolean indicating whether or not URL has passed all tests
-    """
-    url_properties = URLProperties(url_string)
-    return url_properties.run_tests()
-
-
-def run_image_tests(image):
-    """
-    Runs all available image tests
-    @param image: PIL Image object
-    @return: Boolean indicating whether or not all tests have been passed
-    """
-    success = False
-    try:
-        image_properties = ImageProperties(image)
-        success = image_properties.run_tests()
-    except:
-        log.exception("Cannot run image tests in combined open ended xmodule.  May be an issue with a particular image,"
-                      "or an issue with the deployment configuration of PIL/Pillow")
-    return success
-
-
-def upload_to_s3(file_to_upload, keyname, s3_interface):
-    '''
-    Upload file to S3 using provided keyname.
-
-    Returns:
-        public_url: URL to access uploaded file
-    '''
-
-    #This commented out code is kept here in case we change the uploading method and require images to be
-    #converted before they are sent to S3.
-    #TODO: determine if commented code is needed and remove
-    #im = Image.open(file_to_upload)
-    #out_im = cStringIO.StringIO()
-    #im.save(out_im, 'PNG')
-
-    try:
-        conn = S3Connection(s3_interface['access_key'], s3_interface['secret_access_key'])
-        bucketname = str(s3_interface['storage_bucket_name'])
-        bucket = conn.create_bucket(bucketname.lower())
-
-        k = Key(bucket)
-        k.key = keyname
-        k.set_metadata('filename', file_to_upload.name)
-        k.set_contents_from_file(file_to_upload)
-
-        #This commented out code is kept here in case we change the uploading method and require images to be
-        #converted before they are sent to S3.
-        #k.set_contents_from_string(out_im.getvalue())
-        #k.set_metadata("Content-Type", 'images/png')
-
-        k.set_acl("public-read")
-        public_url = k.generate_url(60 * 60 * 24 * 365)   # URL timeout in seconds.
-
-        return True, public_url
-    except:
-        #This is a dev_facing_error
-        error_message = "Could not connect to S3 to upload peer grading image.  Trying to utilize bucket: {0}".format(
-            bucketname.lower())
-        log.error(error_message)
-        return False, error_message
-
-
-def get_from_s3(s3_public_url):
-    """
-    Gets an image from a given S3 url
-    @param s3_public_url: The URL where an image is located
-    @return: The image data
-    """
-    r = requests.get(s3_public_url, timeout=2)
-    data = r.text
-    return data

common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_module.py

@@ -651,15 +651,12 @@ class OpenEndedModule(openendedchild.OpenEndedChild):
             return self.out_of_sync_error(data)
 
         # add new history element with answer and empty score and hint.
-        success, data = self.append_image_to_student_answer(data)
+        success, error_message, data = self.append_file_link_to_student_answer(data)
         if success:
             data['student_answer'] = OpenEndedModule.sanitize_html(data['student_answer'])
             self.new_history_entry(data['student_answer'])
             self.send_to_grader(data['student_answer'], system)
             self.change_state(self.ASSESSING)
-        else:
-            # This is a student_facing_error
-            error_message = "There was a problem saving the image in your submission.  Please try a different image, or try pasting a link to an image into the answer box."
 
         return {
             'success': success,

common/lib/xmodule/xmodule/open_ended_grading_classes/self_assessment_module.py

@@ -179,14 +179,11 @@ class SelfAssessmentModule(openendedchild.OpenEndedChild):
 
         error_message = ""
         # add new history element with answer and empty score and hint.
-        success, data = self.append_image_to_student_answer(data)
+        success, error_message, data = self.append_file_link_to_student_answer(data)
         if success:
             data['student_answer'] = SelfAssessmentModule.sanitize_html(data['student_answer'])
             self.new_history_entry(data['student_answer'])
             self.change_state(self.ASSESSING)
-        else:
-            # This is a student_facing_error
-            error_message = "There was a problem saving the image in your submission.  Please try a different image, or try pasting a link to an image into the answer box."
 
         return {
             'success': success,

common/lib/xmodule/xmodule/tests/test_combined_open_ended.py

@@ -12,7 +12,7 @@ import logging
 import unittest
 
 from lxml import etree
-from mock import Mock, MagicMock, ANY
+from mock import Mock, MagicMock, ANY, patch
 from pytz import UTC
 
 from xmodule.open_ended_grading_classes.openendedchild import OpenEndedChild
@@ -26,7 +26,7 @@ from xmodule.progress import Progress
 from xmodule.tests.test_util_open_ended import (
     MockQueryDict, DummyModulestore, TEST_STATE_SA_IN,
     MOCK_INSTANCE_STATE, TEST_STATE_SA, TEST_STATE_AI, TEST_STATE_AI2, TEST_STATE_AI2_INVALID,
-    TEST_STATE_SINGLE, TEST_STATE_PE_SINGLE
+    TEST_STATE_SINGLE, TEST_STATE_PE_SINGLE, MockUploadedFile
 )
 
 from xblock.field_data import DictFieldData
@@ -374,7 +374,7 @@ class OpenEndedModuleTest(unittest.TestCase):
         # Submit a student response to the question.
         test_module.handle_ajax(
             "save_answer",
-            {"student_answer": submitted_response, "can_upload_files": False, "student_file": None},
+            {"student_answer": submitted_response},
             get_test_system()
         )
         # Submitting an answer should clear the stored answer.
@@ -753,7 +753,7 @@ class OpenEndedModuleXmlTest(unittest.TestCase, DummyModulestore):
 
         #Simulate a student saving an answer
         html = module.handle_ajax("get_html", {})
-        module.handle_ajax("save_answer", {"student_answer": self.answer, "can_upload_files": False, "student_file": None})
+        module.handle_ajax("save_answer", {"student_answer": self.answer})
         html = module.handle_ajax("get_html", {})
 
         #Mock a student submitting an assessment
@@ -902,3 +902,78 @@ class OpenEndedModuleXmlAttemptTest(unittest.TestCase, DummyModulestore):
         #Try to reset, should fail because only 1 attempt is allowed
         reset_data = json.loads(module.handle_ajax("reset", {}))
         self.assertEqual(reset_data['success'], False)
+
+class OpenEndedModuleXmlImageUploadTest(unittest.TestCase, DummyModulestore):
+    """
+    Test if student is able to upload images properly.
+    """
+    problem_location = Location(["i4x", "edX", "open_ended", "combinedopenended", "SampleQuestionImageUpload"])
+    answer_text = "Hello, this is my amazing answer."
+    file_text = "Hello, this is my amazing file."
+    file_name = "Student file 1"
+    answer_link = "http://www.edx.org"
+    autolink_tag = "<a href="
+
+    def setUp(self):
+        self.test_system = get_test_system()
+        self.test_system.open_ended_grading_interface = None
+        self.test_system.s3_interface = test_util_open_ended.S3_INTERFACE
+        self.test_system.xqueue['interface'] = Mock(
+            send_to_queue=Mock(side_effect=[1, "queued"])
+        )
+        self.setup_modulestore(COURSE)
+
+    def test_file_upload_fail(self):
+        """
+        Test to see if a student submission without a file attached fails.
+        """
+        module = self.get_module_from_location(self.problem_location, COURSE)
+
+        #Simulate a student saving an answer
+        response = module.handle_ajax("save_answer", {"student_answer": self.answer_text})
+        response = json.loads(response)
+        self.assertFalse(response['success'])
+        self.assertIn('error', response)
+
+    @patch(
+        'xmodule.open_ended_grading_classes.openendedchild.S3Connection',
+        test_util_open_ended.MockS3Connection
+    )
+    @patch(
+        'xmodule.open_ended_grading_classes.openendedchild.Key',
+        test_util_open_ended.MockS3Key
+    )
+    def test_file_upload_success(self):
+        """
+        Test to see if a student submission with a file is handled properly.
+        """
+        module = self.get_module_from_location(self.problem_location, COURSE)
+
+        #Simulate a student saving an answer with a file
+        response = module.handle_ajax("save_answer", {
+            "student_answer": self.answer_text,
+            "valid_files_attached": True,
+            "student_file": [MockUploadedFile(self.file_name, self.file_text)],
+        })
+
+        response = json.loads(response)
+        self.assertTrue(response['success'])
+        self.assertIn(self.file_name, response['student_response'])
+        self.assertIn(self.autolink_tag, response['student_response'])
+
+    def test_link_submission_success(self):
+        """
+        Students can submit links instead of files.  Check that the link is properly handled.
+        """
+        module = self.get_module_from_location(self.problem_location, COURSE)
+
+        # Simulate a student saving an answer with a link.
+        response = module.handle_ajax("save_answer", {
+            "student_answer": "{0} {1}".format(self.answer_text, self.answer_link)
+            })
+
+        response = json.loads(response)
+
+        self.assertTrue(response['success'])
+        self.assertIn(self.answer_link, response['student_response'])
+        self.assertIn(self.autolink_tag, response['student_response'])

common/lib/xmodule/xmodule/tests/test_self_assessment.py

@@ -133,7 +133,7 @@ class SelfAssessmentTest(unittest.TestCase):
         self.assertEqual(test_module.get_display_answer(), saved_response)
 
         # Submit a student response to the question.
-        test_module.handle_ajax("save_answer", {"student_answer": submitted_response, "can_upload_files": False, "student_file": None}, get_test_system())
+        test_module.handle_ajax("save_answer", {"student_answer": submitted_response}, get_test_system())
         # Submitting an answer should clear the stored answer.
         self.assertEqual(test_module.stored_answer, None)
         # Confirm that the answer is stored properly.

common/lib/xmodule/xmodule/tests/test_util_open_ended.py

@@ -2,6 +2,8 @@ from xmodule.modulestore import Location
 from xmodule.modulestore.xml import XMLModuleStore
 from xmodule.tests import DATA_DIR, get_test_system
 
+from StringIO import StringIO
+
 OPEN_ENDED_GRADING_INTERFACE = {
     'url': 'blah/',
     'username': 'incorrect',
@@ -12,11 +14,61 @@ OPEN_ENDED_GRADING_INTERFACE = {
 }
 
 S3_INTERFACE = {
-    'aws_access_key': "",
-    'aws_secret_key': "",
-    "aws_bucket_name": "",
+    'access_key': "",
+    'secret_access_key': "",
+    "storage_bucket_name": "",
 }
 
+class MockS3Key(object):
+    """
+    Mock an S3 Key object from boto.  Used for file upload testing.
+    """
+    def __init__(self, bucket):
+        pass
+
+    def set_metadata(self, key, value):
+        setattr(self, key, value)
+
+    def set_contents_from_file(self, fileobject):
+        self.data = fileobject.read()
+
+    def set_acl(self, acl):
+        self.set_metadata("acl", acl)
+
+    def generate_url(self, timeout):
+        return "http://www.edx.org/sample_url"
+
+
+class MockS3Connection(object):
+    """
+    Mock boto S3Connection for testing image uploads.
+    """
+    def __init__(self, access_key, secret_key, **kwargs):
+        """
+        Mock the init call.  S3Connection has a lot of arguments, but we don't need them.
+        """
+        pass
+
+    def create_bucket(self, bucket_name, **kwargs):
+        return "edX Bucket"
+
+class MockUploadedFile(object):
+    """
+    Create a mock uploaded file for image submission tests.
+    value - String data to place into the mock file.
+    return - A StringIO object that behaves like a file.
+    """
+    def __init__(self, name, value):
+        self.mock_file = StringIO()
+        self.mock_file.write(value)
+        self.name = name
+
+    def seek(self, index):
+        return self.mock_file.seek(index)
+
+    def read(self):
+        return self.mock_file.read()
+
 
 class MockQueryDict(dict):
     """

common/test/data/open_ended/combinedopenended/SampleQuestionImageUpload.xml

+<combinedopenended attempts="1" display_name = "Humanities Question -- Machine Assessed" accept_file_upload="True">
+    <rubric>
+        <rubric>
+            <category>
+                <description>Writing Applications</description>
+                <option> The essay loses focus, has little information or supporting details, and the organization makes it difficult to follow.</option>
+                <option> The essay presents a mostly unified theme, includes sufficient information to convey the theme, and is generally organized well.</option>
+            </category>
+            <category>
+                <description> Language Conventions </description>
+                <option> The essay demonstrates a reasonable command of proper spelling and grammar. </option>
+                <option> The essay demonstrates superior command of proper spelling and grammar.</option>
+            </category>
+        </rubric>
+    </rubric>
+    <prompt>
+        <h4>Censorship in the Libraries</h4>
+        <p>"All of us can think of a book that we hope none of our children or any other children have taken off the shelf. But if I have the right to remove that book from the shelf -- that work I abhor -- then you also have exactly the same right and so does everyone else. And then we have no books left on the shelf for any of us." --Katherine Paterson, Author</p>
+        <p>Write a persuasive essay to a newspaper reflecting your views on censorship in libraries. Do you believe that certain materials, such as books, music, movies, magazines, etc., should be removed from the shelves if they are found offensive? Support your position with convincing arguments from your own experience, observations, and/or reading.</p>
+    </prompt>
+    <task>
+        <selfassessment/>
+    </task>
+</combinedopenended>
\ No newline at end of file

common/test/data/open_ended/course/2012_Fall.xml

@@ -2,6 +2,7 @@
   <chapter url_name="Overview">
     <combinedopenended url_name="SampleQuestion"/>
     <combinedopenended url_name="SampleQuestion1Attempt"/>
+      <combinedopenended url_name="SampleQuestionImageUpload"/>
     <peergrading url_name="PeerGradingSample"/>
     <peergrading url_name="PeerGradingScored"/>
     <peergrading url_name="PeerGradingLinked"/>