Skip to content

Commit

Permalink
unify file upload completion
Browse files Browse the repository at this point in the history
refs CNVS-257

when completing an upload by posting to the preflight response' url with
preflight params and then pinging success url, etc. do it all through
one method that's the source of truth on "how", instead of implementing
a bajillion times in a bajillion ways

test-plan:
  [preamble]
  * except where otherwise noted, test each of these for each configuration of:
    - local storage (inst-fs disabled and config/file_store.yml set to local);
    - S3 storage (inst-fs disabled, config/file_store.yml set to s3, and
      config/amazon_s3.yml present); and
    - inst-fs storage (inst-fs enabled, service running)
  * when testing with inst-fs, you'll want the service running with
    REQUIRE_AUTH=false; the commit that fixes inst-fs auth is dependent on this
    commit
  * except where otherwise noted, uploads should be fully functional for each of
    these categories
  * these categories should exercise all the modified code, but are not
    necessarily exhaustive of possible upload sites. if you think of others you
    want to test, go ahead

  [course and user files]
  * go to /files
  * upload a file
  * repeat for /courses/x/files

  [file upload quiz question]
  * create a quiz with a question with the "File Upload" type
  * as a student, take the quiz
  * upload a file as the answer to that question

  [course image]
  * enable course images (a feature flag at /accounts/self/settings)
  * go to /courses/x/settings and
  * open the course image dialog
  * upload an image for the course

  [upload file to module]
  * go to /courses/x/modules
  * create a module
  * open add module item dialog
  * select "File" as the item type, and then "[ New File ]"
  * select and upload a file

  [content migration]
  * go to /courses/x/content_exports
  * create and download a course export (a *.imscc file)
  * go to /courses/x/content_migrations
  * upload the previously downloaded *.imscc file
  * NOTE: when attempting with inst-fs the import _job_ will fail. but the upload
    portion should still work; verify by downloading the file associated with the
    import. fixing the job is a separate work task (RECNVS-16)

  [user avatar]
  * enable user avatars (a checkbox at /accounts/self/settings)
  * go to /profile/settings
  * upload a new avatar
  * NOTE: with inst-fs enabled, local storage or S3 will still be used. this is a
    separate bug (RECNVS-237)

  [upload submission]
  * have an assignment with submission type "File Upload"
  * as a student, upload a file as a submission to the assignment
  * NOTE: with inst-fs enabled, local storage or S3 will still be used. this is a
    separate bug (RECNVS-237)

Change-Id: Iad52879add953326880aabf3c75657e32f8d3e8e
Reviewed-on: https://gerrit.instructure.com/138068
Reviewed-by: Andrew Huff <[email protected]>
Reviewed-by: Clay Diffrient <[email protected]>
Tested-by: Jenkins
QA-Review: Collin Parrish <[email protected]>
Product-Review: Jacob Fugal <[email protected]>
  • Loading branch information
lukfugl committed Jan 27, 2018
1 parent 2841204 commit 0a15ef7
Show file tree
Hide file tree
Showing 16 changed files with 323 additions and 360 deletions.
55 changes: 28 additions & 27 deletions app/coffeescripts/models/ContentMigration.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@ define [
'underscore'
'jquery'
'Backbone'
'jsx/shared/upload_file'
'jquery.instructure_forms'
], (_,$, Backbone) ->
], (_,$, Backbone, uploader) ->
class ContentMigration extends Backbone.Model
urlRoot: => "/api/v1/courses/#{@get('course_id')}/content_migrations"
@dynamicDefaults = {}
Expand Down Expand Up @@ -66,44 +67,44 @@ define [
models = collection.map (model) -> model
_.each models, (model) -> collection.remove model

# Handles two cases. Migration with and without a file.
# When saving we extract the fileElement (input type="file" dom node)
# so we can then pass it into our custom "multipart" upload function.
# Handles two cases. Migration with a file and without a file.
#
# Steps:
# * Get file element
# * Save the migration model which should start the process but does
# not up upload the file at this point
# * Create a temp backbone model that uses the url recieved back from
# the first migration save. This is the url used to save the actual
# migration file.
# * Save the temp backbone model which will upload the file to amazon
# and return a 302 from which we can start processing the migration
# file.
# The presence of a file in the migration is indicated by a
# `pre_attachment` field on the model. This field will contain a
# `fileElement` (a DOM node for an <input type="file">). A migration
# without a file (e.g. a course copy) is executed as a normal save.
#
# @returns deffered object
# A migration with a file (e.g. an import) is executed in multiple stages.
# We first set aside the fileElement and save the remainder of the
# migration. In addition to creating the migration on the server, this acts
# as the preflight for the upload of the migration's file, with the
# preflight results being stored back into the model's `pre_attachment`. We
# then complete the upload as for any other file upload. Once completed, we
# reload the model to set the polling url, then resolve the deferred.
#
# @returns deferred object
# @api public backbone override

save: =>
return super unless @get('pre_attachment') # No attachment, regular save

dObject = $.Deferred()
resolve = =>
# sets the poll url
this.fetch success: =>
dObject.resolve()
reject = (message) => dObject.rejectWith(this, message)

fileElement = @get('pre_attachment').fileElement
delete @get('pre_attachment').fileElement
file = fileElement.files[0]

super _.omit(arguments[0], 'file'),
error: (xhr) => dObject.rejectWith(this, xhr.responseText)
success: (model, xhr, options) =>
tempModel = new Backbone.Model(@get('pre_attachment').upload_params)
tempModel.url = => @get('pre_attachment').upload_url
tempModel.set('attachment', fileElement)

tempModel.save null,
multipart: fileElement
success: (model, xhr) =>
return dObject.rejectWith(this, xhr.message) if xhr.message
this.fetch success: => dObject.resolve() # sets the poll url
error: (message) => dObject.rejectWith(this, message)
error: (xhr) => reject(xhr.responseText)
success: (model, xhr, options) =>
uploader.completeUpload(@get('pre_attachment'), file, ignoreResult: true).
catch(reject).
then(resolve)

dObject

Expand Down
52 changes: 18 additions & 34 deletions app/coffeescripts/models/File.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@ define [
'jquery'
'underscore'
'../models/FilesystemObject'
'jsx/shared/upload_file'
'jquery.ajaxJSON'
], ($, _, FilesystemObject) ->
], ($, _, FilesystemObject, uploader) ->

# Simple model for creating an attachment in canvas
#
Expand Down Expand Up @@ -50,46 +51,29 @@ define [
save: (attrs = {}, options = {}) ->
return super unless @get('file')
@set attrs
dfrd = $.Deferred()
el = @get('file')
name = (el.value || el.name).split(/[\/\\]/).pop()
$.ajaxJSON @preflightUrl, 'POST', {name, on_duplicate: 'rename'},
(data) =>
@saveFrd data, dfrd, el, options
(error) =>
dfrd.reject(error)
options.error?(error)
dfrd

saveFrd: (data, dfrd, el, options) =>
# account for attachments wrapped in array per JSON API format
if data.attachments && data.attachments[0]
data = data.attachments[0]
@uploadParams = data.upload_params
@set @uploadParams
el.name = data.file_param
@url = -> data.upload_url
dfrd = $.Deferred()
onUpload = (data) =>
@set(data)
dfrd.resolve(data)
options.success?(data)
onError = (error) =>
dfrd.reject(error)
options.error?(error)
FilesystemObject::save.call this, null,
multipart: true
success: (data) =>
if @uploadParams.success_url
# s3 upload, need to ping success_url to finalize and get back
# attachment information
$.ajaxJSON(@uploadParams.success_url, 'GET', {}, onUpload, onError)
else if data.location?
# inst-fs upload, need to request attachment information from
# location
$.ajaxJSON(data.location, 'GET', {}, onUpload, onError)
else
# local-storage upload, this _is_ the attachment information
onUpload(data)
error: onError

file = @get('file')
filename = (file.value || file.name).split(/[\/\\]/).pop()
file = file.files[0]
preflightData =
name: filename
on_duplicate: 'rename'
uploader.uploadFile(@preflightUrl, preflightData, file)
.then(onUpload)
.catch(onError)

dfrd

isFile: true

toJSON: ->
return super unless @get('file')
Expand Down
4 changes: 2 additions & 2 deletions app/coffeescripts/models/FilesystemObject.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ define [

copyToContext: (newFolder, options = {}) ->
attrs = @setAttributes($.extend({}, @attributes), options)
type = if @saveFrd then "file" else "folder"
type = if @isFile then "file" else "folder"
attrs["source_#{type}_id"] = attrs.id
delete attrs.id

Expand All @@ -89,7 +89,7 @@ define [

updateCollection: (model, newFolder, options) ->
# add it to newFolder's children
objectType = if @saveFrd then 'files' else 'folders' #TODO find a better way to infer type
objectType = if @isFile then 'files' else 'folders' #TODO find a better way to infer type
collection = newFolder[objectType]

if options.dup == 'overwrite' # remove the overwritten object from the collection
Expand Down
26 changes: 6 additions & 20 deletions app/coffeescripts/react_files/modules/BaseUploader.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@

define [
'jquery'
'jsx/shared/upload_file'
'jquery.ajaxJSON'
], ($) ->
], ($, uploader) ->

# Base uploader with common api between File and Zip uploads
# (where zip is expanded)
Expand All @@ -33,14 +34,6 @@ define [
onProgress: (percentComplete, file) ->
#noop will be set up a level

createFormData: () ->
data = @uploadData.upload_params
formData = new FormData()
Object.keys(data).forEach (key) ->
formData.append(key, data[key])
formData.append('file', @file)
formData

createPreFlightParams: ->
params =
name: @options.name || @file.name
Expand Down Expand Up @@ -69,17 +62,10 @@ define [

#actual upload based on kickoff / preflight
_actualUpload: () ->
@_xhr = new XMLHttpRequest
@_xhr.upload.addEventListener('progress', @trackProgress, false)
@_xhr.onload = @onUploadPosted
@_xhr.onerror = @deferred.reject
@_xhr.onabort = @deferred.reject
@_xhr.open 'POST', @uploadData.upload_url, true
@_xhr.send @createFormData()

# when using s3 uploads you now need to manually hit the success_url
# when using local uploads you have already been auto-redirected (even
# though we requested no_redirect) to the succes_url at this point
uploader.completeUpload(@uploadData, @file, onProgress: @trackProgress)
.then(@onUploadPosted)
.catch(@deferred.reject)

onUploadPosted: (event) =>
# should be implemented in extensions

Expand Down
21 changes: 1 addition & 20 deletions app/coffeescripts/react_files/modules/FileUploader.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -25,26 +25,7 @@ define [

class FileUploader extends BaseUploader

onUploadPosted: (event) =>
if event.target.status >= 400
@deferred.reject(event.target.status)
return

if @uploadData.upload_params.success_url
# s3 upload, need to ping success_url to finalize and get back
# attachment information
$.getJSON(@uploadData.upload_params.success_url).then(@onUploadSuccess)
else
response = $.parseJSON(event.target.response)
if event.target.status == 201
# inst-fs upload, need to request attachment information from
# location
$.getJSON(response.location).then(@onUploadSuccess)
else
# local-storage upload, this _is_ the attachment information
@onUploadSuccess(response)

onUploadSuccess: (fileJson) =>
onUploadPosted: (fileJson) =>
file = @addFileToCollection(fileJson)
@deferred.resolve(file)

Expand Down
15 changes: 2 additions & 13 deletions app/coffeescripts/react_files/modules/ZipUploader.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -48,19 +48,8 @@ define [
@contentMigrationId = data.id
@_actualUpload()

onUploadPosted: (uploadResults) =>
if (uploadResults.target && uploadResults.target.status >= 400)
@deferred.reject()
return

if @uploadData.upload_params.success_url
# s3 upload, need to ping success_url to finalize before polling the
# content migration
$.getJSON(@uploadData.upload_params.success_url).then(@getContentMigration)
else if uploadResponse.target.status == 201
# local-storage or inst-fs upload. we don't need to attachment
# information, we can just start polling the content migration
@getContentMigration()
onUploadPosted: =>
@getContentMigration()

# get the content migration when ready and use progress api to pull migration progress
getContentMigration: =>
Expand Down
54 changes: 6 additions & 48 deletions app/coffeescripts/views/profiles/AvatarDialogView.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,10 @@ define [
'./UploadFileView'
'./TakePictureView'
'./GravatarView'
'jsx/shared/upload_file'
'jst/profiles/avatarDialog'
'jst/profiles/avatar'
], (I18n, $, _, DialogBaseView, UploadFileView, TakePictureView, GravatarView, template, avatarTemplate) ->
], (I18n, $, _, DialogBaseView, UploadFileView, TakePictureView, GravatarView, uploader, template, avatarTemplate) ->

class AvatarDialogView extends DialogBaseView

Expand Down Expand Up @@ -145,53 +146,10 @@ define [
}).fail((xhr) => @handleErrorUpdating(xhr.responseText))

onPreflight: (image, response) =>
@image = image
preflightResponse = response[0]
@postAvatar(preflightResponse).then(_.partial(@onPostAvatar, preflightResponse))

postAvatar: (preflightResponse) =>
image = @image
req = new FormData

delete @image

###
xsslint xssable.receiver.whitelist req
###
req.append(k, v) for k, v of preflightResponse.upload_params
req.append(preflightResponse.file_param, image, 'profile.jpg')
dataType = if preflightResponse.success_url then 'xml' else 'json'
$.ajax(preflightResponse.upload_url, {
contentType: false
data: req
dataType: dataType
processData: false
type: 'POST'
error: (xhr) => @handleErrorUpdating(xhr.responseText)
})

onPostAvatar: (preflightResponse, postAvatarResponse) =>
if preflightResponse.success_url
# s3 upload, need to ping success_url to finalize and get back
# attachment information
@s3Success(preflightResponse, postAvatarResponse).then(@onUploadSuccess)
else if postAvatarResponse.location
# inst-fs upload, need to request attachment information from location
@getCanvasJSON(postAvatarResponse.location).then(@onUploadSuccess)
else
# local-storage upload, this _is_ the attachment information
@onUploadSuccess(postAvatarResponse)

getCanvasJSON: (location) =>
$.getJSON("#{location}?include=avatar")

s3Success: (preflightResponse, s3Response) =>
$s3 = $(s3Response)
$.getJSON(preflightResponse.success_url, {
bucket: $s3.find('Bucket').text()
key: $s3.find('Key').text()
etag: $s3.find('ETag').text()
})
preflight = response[0]
uploader.completeUpload(preflight, image, filename: 'profile.jpg', includeAvatar: true)
.then(@onUploadSuccess)
.catch((xhr) => @handleErrorUpdating(xhr.responseText))

onUploadSuccess: (response) =>
@waitAndSaveUserAvatar(response.avatar.token, response.avatar.url)
Expand Down
7 changes: 4 additions & 3 deletions app/jsx/course_settings/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ import axios from 'axios'
import $ from 'jquery'
import I18n from 'i18n!actions'
import Helpers from './helpers'
import rawUploadFile from '../shared/upload_file'
import { uploadFile as rawUploadFile } from '../shared/upload_file'
import 'compiled/jquery.rails_flash_notifications' /* $.flashWarning */

const Actions = {

Expand Down Expand Up @@ -187,8 +188,8 @@ import rawUploadFile from '../shared/upload_file'
no_redirect: true
};
rawUploadFile(url, data, file, ajaxLib)
.then((successResp) => {
dispatch(this.prepareSetImage(successResp.data.url, successResp.data.id, courseId, ajaxLib));
.then((attachment) => {
dispatch(this.prepareSetImage(attachment.url, attachment.id, courseId, ajaxLib));
}).catch((_response) => {
dispatch(this.errorUploadingImage());
});
Expand Down
Loading

0 comments on commit 0a15ef7

Please sign in to comment.