Fix image checking logic

common/lib/xmodule/xmodule/combined_open_ended_module.py

@@ -39,7 +39,7 @@ MAX_SCORE = 1
 MAX_SCORE_ALLOWED = 3
 
 IS_SCORED = False
-ACCEPT_FILE_UPLOAD = False
+ACCEPT_FILE_UPLOAD = True
 
 #Contains all reasonable bool and case combinations of True
 TRUE_DICT = ["True", True, "TRUE", "true"]

common/lib/xmodule/xmodule/open_ended_image_submission.py

@@ -3,7 +3,12 @@ This contains functions and classes used to evaluate if images are acceptable (d
 to send them to S3.
 """
 
-from PIL import Image
+try:
+    from PIL import Image
+    ENABLE_PIL = True
+except:
+    ENABLE_PIL = False
+
 from urlparse import urlparse
 import requests
 from boto.s3.connection import S3Connection
@@ -46,13 +51,13 @@ class ImageProperties(object):
     """
     Class to check properties of an image and to validate if they are allowed.
     """
-    def __init__(self, image):
+    def __init__(self, image_data):
         """
         Initializes class variables
         @param image: Image object (from PIL)
         @return: None
         """
-        self.image = image
+        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:
@@ -158,10 +163,6 @@ class URLProperties(object):
         @return: True if URL passes tests, false if not.
         """
         url_is_okay = self.check_suffix() and self.check_if_parses() and self.check_domain()
-        log.debug(self.url_string)
-        log.debug("Suffix : {0}".format(self.check_suffix()))
-        log.debug("Parses:{0}".format(self.check_if_parses()))
-        log.debug("Check Domain:{0}".format(self.check_domain()))
         return url_is_okay
 
     def check_domain(self):
@@ -191,8 +192,14 @@ def run_image_tests(image):
     @param image: PIL Image object
     @return: Boolean indicating whether or not all tests have been passed
     """
-    image_properties = ImageProperties(image)
-    return image_properties.run_tests()
+    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):

common/lib/xmodule/xmodule/openendedchild.py

@@ -27,8 +27,6 @@ import open_ended_image_submission
 
 from datetime import datetime
 
-from PIL import Image
-
 log = logging.getLogger("mitx.courseware")
 
 # Set the default number of max attempts.  Should be 1 for production
@@ -290,25 +288,20 @@ class OpenEndedChild(object):
         s3_public_url = ""
         try:
             image_data.seek(0)
-            image = Image.open(image_data)
-            image_ok = open_ended_image_submission.run_image_tests(image)
-            log.debug("Image ok: {0}".format(image_ok))
-            success = True
+            image_ok = open_ended_image_submission.run_image_tests(image_data)
         except:
             log.exception("Could not create image and check it.")
 
-        if success and image_ok:
+        if image_ok:
             image_key = image_data.name + datetime.now().strftime("%Y%m%d%H%M%S")
 
             try:
                 image_data.seek(0)
                 success, s3_public_url = open_ended_image_submission.upload_to_s3(image_data, image_key)
             except:
-                success = False
                 log.exception("Could not upload image to S3.")
 
-        log.debug(s3_public_url)
-        return success, s3_public_url
+        return success, image_ok, s3_public_url
 
     def check_for_image_and_upload(self, get_data):
         """
@@ -320,15 +313,16 @@ class OpenEndedChild(object):
         has_file_to_upload = False
         uploaded_to_s3 = False
         image_tag = ""
+        image_ok = False
         if 'can_upload_files' in get_data:
             if get_data['can_upload_files'] == 'true':
                 has_file_to_upload = True
                 file = get_data['student_file'][0]
-                uploaded_to_s3, s3_public_url = self.upload_image_to_s3(file)
+                uploaded_to_s3, image_ok, s3_public_url = self.upload_image_to_s3(file)
                 if uploaded_to_s3:
                     image_tag = self.generate_image_tag_from_url(s3_public_url, file.name)
 
-        return has_file_to_upload, uploaded_to_s3, image_tag
+        return has_file_to_upload, uploaded_to_s3, image_ok, image_tag
 
     def generate_image_tag_from_url(self, s3_public_url, image_name):
         """
@@ -353,11 +347,11 @@ class OpenEndedChild(object):
             #If the question does not accept file uploads, do not do anything
             return True, get_data
 
-        has_file_to_upload, uploaded_to_s3, image_tag = self.check_for_image_and_upload(get_data)
+        has_file_to_upload, uploaded_to_s3, image_ok, image_tag = self.check_for_image_and_upload(get_data)
         if uploaded_to_s3 and has_file_to_upload:
             get_data['student_answer'] += image_tag
             overall_success = True
-        elif has_file_to_upload and not uploaded_to_s3:
+        elif has_file_to_upload and not uploaded_to_s3 and image_ok:
             #In this case, an image was submitted by the student, but the image could not be uploaded to S3.  Likely
             #a config issue (development vs deployment).  For now, just treat this as a "success"
             log.warning("Student AJAX post to combined open ended xmodule indicated that it contained an image, "
@@ -365,7 +359,7 @@ class OpenEndedChild(object):
                         "issue with this deployment, but it could also indicate a problem with S3 or with the"
                         "student image itself.")
             overall_success = True
-        else:
+        elif not has_file_to_upload:
             #If there is no file to upload, probably the student has embedded the link in the answer text
             success, get_data['student_answer'] = self.check_for_url_in_text(get_data['student_answer'])
             overall_success = success