From 9632f4fed877ce8add00f5787a327174f84efd20 Mon Sep 17 00:00:00 2001
From: Peter Fogg <pfogg@edx.org>
Date: Mon, 19 Aug 2013 11:27:17 -0400
Subject: [PATCH] Address PR comments:

- Internationalize upload errors.
- Move upload tests into their own files.
- Refactor upload dialog acceptance tests.
---
 .../contentstore/features/common.py           |  13 ++
 .../features/course-settings.feature          |   1 +
 .../contentstore/features/course-settings.py  |  18 ++-
 .../contentstore/features/textbooks.py        |  12 +-
 .../coffee/spec/models/textbook_spec.coffee   |  29 -----
 .../coffee/spec/models/upload_spec.coffee     |  33 +++++
 .../coffee/spec/views/textbook_spec.coffee    | 118 ------------------
 .../coffee/spec/views/upload_spec.coffee      | 118 ++++++++++++++++++
 cms/static/js/models/uploads.js               |   7 +-
 cms/templates/js/upload-dialog.underscore     |   2 +-
 cms/templates/settings.html                   |   2 +-
 11 files changed, 184 insertions(+), 169 deletions(-)
 create mode 100644 cms/static/coffee/spec/models/upload_spec.coffee
 create mode 100644 cms/static/coffee/spec/views/upload_spec.coffee

diff --git a/cms/djangoapps/contentstore/features/common.py b/cms/djangoapps/contentstore/features/common.py
index 516659fadb..1cc71097a0 100644
--- a/cms/djangoapps/contentstore/features/common.py
+++ b/cms/djangoapps/contentstore/features/common.py
@@ -5,9 +5,11 @@ from lettuce import world, step
 from nose.tools import assert_true
 
 from auth.authz import get_user_by_email, get_course_groupname_for_role
+from django.conf import settings
 
 from selenium.webdriver.common.keys import Keys
 import time
+import os
 from django.contrib.auth.models import Group
 
 from logging import getLogger
@@ -15,6 +17,8 @@ logger = getLogger(__name__)
 
 from terrain.browser import reset_data
 
+TEST_ROOT = settings.COMMON_TEST_DATA_ROOT
+
 ###########  STEP HELPERS ##############
 
 
@@ -257,3 +261,12 @@ def type_in_codemirror(index, text):
     g._element.send_keys(text)
     if world.is_firefox():
         world.trigger_event('div.CodeMirror', index=index, event='blur')
+
+
+def upload_file(filename):
+    file_css = '.upload-dialog input[type=file]'
+    upload = world.css_find(file_css).first
+    path = os.path.join(TEST_ROOT, filename)
+    upload._element.send_keys(os.path.abspath(path))
+    button_css = '.upload-dialog .action-upload'
+    world.css_click(button_css)
diff --git a/cms/djangoapps/contentstore/features/course-settings.feature b/cms/djangoapps/contentstore/features/course-settings.feature
index 69183bc3da..8f00452efe 100644
--- a/cms/djangoapps/contentstore/features/course-settings.feature
+++ b/cms/djangoapps/contentstore/features/course-settings.feature
@@ -76,6 +76,7 @@ Feature: Course Settings
   Scenario: User can upload course image
     Given I have opened a new course in Studio
     When I select Schedule and Details
+    And I click the "Upload Course Image" button
     And I upload a new course image
     Then I should see the new course image
     And the image URL should be present in the field
diff --git a/cms/djangoapps/contentstore/features/course-settings.py b/cms/djangoapps/contentstore/features/course-settings.py
index 0847c62a18..570c49a8c4 100644
--- a/cms/djangoapps/contentstore/features/course-settings.py
+++ b/cms/djangoapps/contentstore/features/course-settings.py
@@ -4,9 +4,8 @@
 from lettuce import world, step
 from terrain.steps import reload_the_page
 from selenium.webdriver.common.keys import Keys
-from common import type_in_codemirror
+from common import type_in_codemirror, upload_file
 from django.conf import settings
-import os
 
 from nose.tools import assert_true, assert_false, assert_equal
 
@@ -150,16 +149,15 @@ def test_change_course_overview(_step):
     type_in_codemirror(0, "<h1>Overview</h1>")
 
 
+@step('I click the "Upload Course Image" button')
+def click_upload_button(_step):
+    button_css = '.action-upload-image'
+    world.css_click(button_css)
+
+
 @step('I upload a new course image$')
 def upload_new_course_image(_step):
-    upload_css = '.action-upload-image'
-    world.css_click(upload_css)
-    file_css = '.upload-dialog input[type=file]'
-    upload = world.css_find(file_css)
-    path = os.path.join(TEST_ROOT, 'image.jpg')
-    upload._element.send_keys(os.path.abspath(path))
-    button_css = '.upload-dialog .action-upload'
-    world.css_click(button_css)
+    upload_file('image.jpg')
 
 
 @step('I should see the new course image$')
diff --git a/cms/djangoapps/contentstore/features/textbooks.py b/cms/djangoapps/contentstore/features/textbooks.py
index d9c08ec6eb..b432b84d4f 100644
--- a/cms/djangoapps/contentstore/features/textbooks.py
+++ b/cms/djangoapps/contentstore/features/textbooks.py
@@ -3,7 +3,7 @@
 
 from lettuce import world, step
 from django.conf import settings
-import os
+from common import upload_file
 
 TEST_ROOT = settings.COMMON_TEST_DATA_ROOT
 
@@ -24,14 +24,8 @@ def assert_create_new_textbook_msg(_step):
 
 
 @step(u'I upload the textbook "([^"]*)"$')
-def upload_file(_step, file_name):
-    file_css = '.upload-dialog input[type=file]'
-    upload = world.css_find(file_css)
-    # uploading the file itself
-    path = os.path.join(TEST_ROOT, 'uploads', file_name)
-    upload._element.send_keys(os.path.abspath(path))
-    button_css = ".upload-dialog .action-upload"
-    world.css_click(button_css)
+def upload_textbook(_step, file_name):
+    upload_file(file_name)
 
 
 @step(u'I click (on )?the New Textbook button')
diff --git a/cms/static/coffee/spec/models/textbook_spec.coffee b/cms/static/coffee/spec/models/textbook_spec.coffee
index 6e601ecf68..d88e09f57a 100644
--- a/cms/static/coffee/spec/models/textbook_spec.coffee
+++ b/cms/static/coffee/spec/models/textbook_spec.coffee
@@ -196,32 +196,3 @@ describe "CMS.Collections.ChapterSet", ->
         # try going back one
         @collection.remove(@collection.last())
         expect(@collection.nextOrder()).toEqual(2)
-
-
-describe "CMS.Models.FileUpload", ->
-    beforeEach ->
-        @model = new CMS.Models.FileUpload()
-
-    it "is unfinished by default", ->
-        expect(@model.get("finished")).toBeFalsy()
-
-    it "is not uploading by default", ->
-        expect(@model.get("uploading")).toBeFalsy()
-
-    it "is valid by default", ->
-        expect(@model.isValid()).toBeTruthy()
-
-    it "is valid for PDF files", ->
-        file = {"type": "application/pdf"}
-        @model.set("selectedFile", file);
-        expect(@model.isValid()).toBeTruthy()
-
-    it "is invalid for text files", ->
-        file = {"type": "text/plain"}
-        @model.set("selectedFile", file);
-        expect(@model.isValid()).toBeFalsy()
-
-    it "is invalid for PNG files", ->
-        file = {"type": "image/png"}
-        @model.set("selectedFile", file);
-        expect(@model.isValid()).toBeFalsy()
diff --git a/cms/static/coffee/spec/models/upload_spec.coffee b/cms/static/coffee/spec/models/upload_spec.coffee
new file mode 100644
index 0000000000..e4be3b9a80
--- /dev/null
+++ b/cms/static/coffee/spec/models/upload_spec.coffee
@@ -0,0 +1,33 @@
+describe "CMS.Models.FileUpload", ->
+    beforeEach ->
+        @model = new CMS.Models.FileUpload()
+
+    it "is unfinished by default", ->
+        expect(@model.get("finished")).toBeFalsy()
+
+    it "is not uploading by default", ->
+        expect(@model.get("uploading")).toBeFalsy()
+
+    it "is valid by default", ->
+        expect(@model.isValid()).toBeTruthy()
+
+    it "is valid for PDF files by default", ->
+        file = {"type": "application/pdf"}
+        @model.set("selectedFile", file);
+        expect(@model.isValid()).toBeTruthy()
+
+    it "is invalid for text files by default", ->
+        file = {"type": "text/plain"}
+        @model.set("selectedFile", file);
+        expect(@model.isValid()).toBeFalsy()
+
+    it "is invalid for PNG files by default", ->
+        file = {"type": "image/png"}
+        @model.set("selectedFile", file);
+        expect(@model.isValid()).toBeFalsy()
+
+    it "can accept non-PDF files when explicitly set", ->
+        file = {"type": "image/png"}
+        @model.set("mimeType": "image/png")
+        @model.set("selectedFile", file)
+        expect(@model.isValid()).toBeTruthy()
diff --git a/cms/static/coffee/spec/views/textbook_spec.coffee b/cms/static/coffee/spec/views/textbook_spec.coffee
index 5185c9fb47..ade8c4cb6e 100644
--- a/cms/static/coffee/spec/views/textbook_spec.coffee
+++ b/cms/static/coffee/spec/views/textbook_spec.coffee
@@ -311,121 +311,3 @@ describe "CMS.Views.EditChapter", ->
         @view.$(".action-upload").click()
         expect(@model.get("name")).toEqual("rainbows")
         expect(@model.get("asset_path")).toEqual("unicorns")
-
-
-describe "CMS.Views.UploadDialog", ->
-    tpl = readFixtures("upload-dialog.underscore")
-
-    beforeEach ->
-        setFixtures($("<script>", {id: "upload-dialog-tpl", type: "text/template"}).text(tpl))
-        appendSetFixtures($("<script>", {id: "system-feedback-tpl", type: "text/template"}).text(feedbackTpl))
-        CMS.URL.UPLOAD_ASSET = "/upload"
-
-        @model = new CMS.Models.FileUpload()
-        @chapter = new CMS.Models.Chapter()
-        @view = new CMS.Views.UploadDialog(
-          model: @model
-          onSuccess: (response) =>
-            options = {}
-            if !@chapter.get('name')
-              options.name = response.displayname
-            options.asset_path = response.url
-            @chapter.set(options)
-        )
-        spyOn(@view, 'remove').andCallThrough()
-
-        # create mock file input, so that we aren't subject to browser restrictions
-        @mockFiles = []
-        mockFileInput = jasmine.createSpy('mockFileInput')
-        mockFileInput.files = @mockFiles
-        jqMockFileInput = jasmine.createSpyObj('jqMockFileInput', ['get', 'replaceWith'])
-        jqMockFileInput.get.andReturn(mockFileInput)
-        realMethod = @view.$
-        spyOn(@view, "$").andCallFake (selector) ->
-            if selector == "input[type=file]"
-                jqMockFileInput
-            else
-                realMethod.apply(this, arguments)
-
-    afterEach ->
-        delete CMS.URL.UPLOAD_ASSET
-
-    describe "Basic", ->
-        it "should be shown by default", ->
-            expect(@view.options.shown).toBeTruthy()
-
-        it "should render without a file selected", ->
-            @view.render()
-            expect(@view.$el).toContain("input[type=file]")
-            expect(@view.$(".action-upload")).toHaveClass("disabled")
-
-        it "should render with a PDF selected", ->
-            file = {name: "fake.pdf", "type": "application/pdf"}
-            @mockFiles.push(file)
-            @model.set("selectedFile", file)
-            @view.render()
-            expect(@view.$el).toContain("input[type=file]")
-            expect(@view.$el).not.toContain("#upload_error")
-            expect(@view.$(".action-upload")).not.toHaveClass("disabled")
-
-        it "should render an error with an invalid file type selected", ->
-            file = {name: "fake.png", "type": "image/png"}
-            @mockFiles.push(file)
-            @model.set("selectedFile", file)
-            @view.render()
-            expect(@view.$el).toContain("input[type=file]")
-            expect(@view.$el).toContain("#upload_error")
-            expect(@view.$(".action-upload")).toHaveClass("disabled")
-
-
-        it "adds body class on show()", ->
-            @view.show()
-            expect(@view.options.shown).toBeTruthy()
-            # can't test: this blows up the spec runner
-            # expect($("body")).toHaveClass("dialog-is-shown")
-
-        it "removes body class on hide()", ->
-            @view.hide()
-            expect(@view.options.shown).toBeFalsy()
-            # can't test: this blows up the spec runner
-            # expect($("body")).not.toHaveClass("dialog-is-shown")
-
-    describe "Uploads", ->
-        beforeEach ->
-            @requests = requests = []
-            @xhr = sinon.useFakeXMLHttpRequest()
-            @xhr.onCreate = (xhr) -> requests.push(xhr)
-            @clock = sinon.useFakeTimers()
-
-        afterEach ->
-            @xhr.restore()
-            @clock.restore()
-
-        it "can upload correctly", ->
-            @view.upload()
-            expect(@model.get("uploading")).toBeTruthy()
-            expect(@requests.length).toEqual(1)
-            request = @requests[0]
-            expect(request.url).toEqual("/upload")
-            expect(request.method).toEqual("POST")
-
-            request.respond(200, {"Content-Type": "application/json"},
-                    '{"displayname": "starfish", "url": "/uploaded/starfish.pdf"}')
-            expect(@model.get("uploading")).toBeFalsy()
-            expect(@model.get("finished")).toBeTruthy()
-            expect(@chapter.get("name")).toEqual("starfish")
-            expect(@chapter.get("asset_path")).toEqual("/uploaded/starfish.pdf")
-
-        it "can handle upload errors", ->
-            @view.upload()
-            @requests[0].respond(500)
-            expect(@model.get("title")).toMatch(/error/)
-            expect(@view.remove).not.toHaveBeenCalled()
-
-        it "removes itself after two seconds on successful upload", ->
-            @view.upload()
-            @requests[0].respond(200, {"Content-Type": "application/json"},
-                    '{"displayname": "starfish", "url": "/uploaded/starfish.pdf"}')
-            expect(@view.remove).not.toHaveBeenCalled()
-            @clock.tick(2001)
-            expect(@view.remove).toHaveBeenCalled()
diff --git a/cms/static/coffee/spec/views/upload_spec.coffee b/cms/static/coffee/spec/views/upload_spec.coffee
new file mode 100644
index 0000000000..39f1519e05
--- /dev/null
+++ b/cms/static/coffee/spec/views/upload_spec.coffee
@@ -0,0 +1,118 @@
+feedbackTpl = readFixtures('system-feedback.underscore')
+
+describe "CMS.Views.UploadDialog", ->
+    tpl = readFixtures("upload-dialog.underscore")
+
+    beforeEach ->
+        setFixtures($("<script>", {id: "upload-dialog-tpl", type: "text/template"}).text(tpl))
+        appendSetFixtures($("<script>", {id: "system-feedback-tpl", type: "text/template"}).text(feedbackTpl))
+        CMS.URL.UPLOAD_ASSET = "/upload"
+
+        @model = new CMS.Models.FileUpload()
+        @chapter = new CMS.Models.Chapter()
+        @view = new CMS.Views.UploadDialog(
+          model: @model
+          onSuccess: (response) =>
+            options = {}
+            if !@chapter.get('name')
+              options.name = response.displayname
+            options.asset_path = response.url
+            @chapter.set(options)
+        )
+        spyOn(@view, 'remove').andCallThrough()
+
+        # create mock file input, so that we aren't subject to browser restrictions
+        @mockFiles = []
+        mockFileInput = jasmine.createSpy('mockFileInput')
+        mockFileInput.files = @mockFiles
+        jqMockFileInput = jasmine.createSpyObj('jqMockFileInput', ['get', 'replaceWith'])
+        jqMockFileInput.get.andReturn(mockFileInput)
+        realMethod = @view.$
+        spyOn(@view, "$").andCallFake (selector) ->
+            if selector == "input[type=file]"
+                jqMockFileInput
+            else
+                realMethod.apply(this, arguments)
+
+    afterEach ->
+        delete CMS.URL.UPLOAD_ASSET
+
+    describe "Basic", ->
+        it "should be shown by default", ->
+            expect(@view.options.shown).toBeTruthy()
+
+        it "should render without a file selected", ->
+            @view.render()
+            expect(@view.$el).toContain("input[type=file]")
+            expect(@view.$(".action-upload")).toHaveClass("disabled")
+
+        it "should render with a PDF selected", ->
+            file = {name: "fake.pdf", "type": "application/pdf"}
+            @mockFiles.push(file)
+            @model.set("selectedFile", file)
+            @view.render()
+            expect(@view.$el).toContain("input[type=file]")
+            expect(@view.$el).not.toContain("#upload_error")
+            expect(@view.$(".action-upload")).not.toHaveClass("disabled")
+
+        it "should render an error with an invalid file type selected", ->
+            file = {name: "fake.png", "type": "image/png"}
+            @mockFiles.push(file)
+            @model.set("selectedFile", file)
+            @view.render()
+            expect(@view.$el).toContain("input[type=file]")
+            expect(@view.$el).toContain("#upload_error")
+            expect(@view.$(".action-upload")).toHaveClass("disabled")
+
+
+        it "adds body class on show()", ->
+            @view.show()
+            expect(@view.options.shown).toBeTruthy()
+            # can't test: this blows up the spec runner
+            # expect($("body")).toHaveClass("dialog-is-shown")
+
+        it "removes body class on hide()", ->
+            @view.hide()
+            expect(@view.options.shown).toBeFalsy()
+            # can't test: this blows up the spec runner
+            # expect($("body")).not.toHaveClass("dialog-is-shown")
+
+    describe "Uploads", ->
+        beforeEach ->
+            @requests = requests = []
+            @xhr = sinon.useFakeXMLHttpRequest()
+            @xhr.onCreate = (xhr) -> requests.push(xhr)
+            @clock = sinon.useFakeTimers()
+
+        afterEach ->
+            @xhr.restore()
+            @clock.restore()
+
+        it "can upload correctly", ->
+            @view.upload()
+            expect(@model.get("uploading")).toBeTruthy()
+            expect(@requests.length).toEqual(1)
+            request = @requests[0]
+            expect(request.url).toEqual("/upload")
+            expect(request.method).toEqual("POST")
+
+            request.respond(200, {"Content-Type": "application/json"},
+                    '{"displayname": "starfish", "url": "/uploaded/starfish.pdf"}')
+            expect(@model.get("uploading")).toBeFalsy()
+            expect(@model.get("finished")).toBeTruthy()
+            expect(@chapter.get("name")).toEqual("starfish")
+            expect(@chapter.get("asset_path")).toEqual("/uploaded/starfish.pdf")
+
+        it "can handle upload errors", ->
+            @view.upload()
+            @requests[0].respond(500)
+            expect(@model.get("title")).toMatch(/error/)
+            expect(@view.remove).not.toHaveBeenCalled()
+
+        it "removes itself after two seconds on successful upload", ->
+            @view.upload()
+            @requests[0].respond(200, {"Content-Type": "application/json"},
+                    '{"displayname": "starfish", "url": "/uploaded/starfish.pdf"}')
+            expect(@view.remove).not.toHaveBeenCalled()
+            @clock.tick(2001)
+            expect(@view.remove).toHaveBeenCalled()
diff --git a/cms/static/js/models/uploads.js b/cms/static/js/models/uploads.js
index ba557306eb..131c554afd 100644
--- a/cms/static/js/models/uploads.js
+++ b/cms/static/js/models/uploads.js
@@ -15,7 +15,12 @@ CMS.Models.FileUpload = Backbone.Model.extend({
     validate: function(attrs, options) {
         if(attrs.selectedFile && attrs.selectedFile.type !== this.attributes.mimeType) {
             return {
-                message: "Only " + this.attributes.fileType + " files can be uploaded. Please select a file ending in ." + this.attributes.fileType.toLowerCase() + " to upload.",
+                message: _.template(
+                    gettext("Only {fileType} files can be uploaded. Please select a file ending in .{fileExtension} to upload."),
+                    {
+                        fileType: this.attributes.fileType,
+                        fileExtension: this.attributes.fileType.toLowerCase()
+                    }),
                 attributes: {selectedFile: true}
             };
         }
diff --git a/cms/templates/js/upload-dialog.underscore b/cms/templates/js/upload-dialog.underscore
index 2c7fc022bd..2c414e2e46 100644
--- a/cms/templates/js/upload-dialog.underscore
+++ b/cms/templates/js/upload-dialog.underscore
@@ -11,7 +11,7 @@
   <h2 class="title"><%= title %></h2>
   <% if(error) {%>
     <div id="upload_error" class="message message-status message-status error is-shown" name="upload_error">
-      <p><%= gettext(error.message) %></p>
+      <p><%= error.message %></p>
     </div>
   <% } %>
   <p id="dialog-assetupload-description" class="message"><%= message %></p>
diff --git a/cms/templates/settings.html b/cms/templates/settings.html
index 96a8e59d9d..ae1051d357 100644
--- a/cms/templates/settings.html
+++ b/cms/templates/settings.html
@@ -48,7 +48,7 @@ from contentstore import utils
         reset: true
       });
 
-    CMS.URL.UPLOAD_ASSET = '${upload_asset_url}';
+      CMS.URL.UPLOAD_ASSET = '${upload_asset_url}';
     });
 
   </script>