Skip to content

Commit

Permalink
return progress from legacy upload via url
Browse files Browse the repository at this point in the history
fixes RECNVS-401

the javascript has already been adapted to expect a progress instead of
the file status blob, that's what causes the bug. give it what it
expects. the progress results are populated during Attachment#clone_url,
and the progress is flagged completed once the worker successfully
completes the job.

test-plan:
 - with inst-fs turned off in the account
 - have a course with the Google Drive LTI tool enabled
 - have a file upload assignment in that course
 - as a student in the course, click "Submit Assignment" on the file
   upload assignment
 - choose the Google Drive tab, authorize a google account, select a
   file from the drive, and click submit.
 - submit the assignment with the selected file; the submission should
   go through without error (will give an error at this point without
   this commit)
 - click the link to download the submission; should download

Change-Id: I43e991f419a9f81359145d9f40899ecc82675da0
Reviewed-on: https://gerrit.instructure.com/145403
Reviewed-by: Michael Jasper <[email protected]>
Tested-by: Jenkins
Reviewed-by: Andrew Huff <[email protected]>
QA-Review: Collin Parrish <[email protected]>
Product-Review: Jacob Fugal <[email protected]>
  • Loading branch information
lukfugl committed Mar 30, 2018
1 parent 0a7c024 commit 9265aa5
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 39 deletions.
12 changes: 12 additions & 0 deletions app/models/attachment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1744,6 +1744,12 @@ def clone_url(url, duplicate_handling, check_quota, opts={})

self.file_state = 'available'
self.save!

if opts[:progress]
# the UI only needs the id from here
opts[:progress].set_results({ id: self.id })
end

handle_duplicates(duplicate_handling || 'overwrite')
rescue Exception, Timeout::Error => e
self.file_state = 'errored'
Expand All @@ -1765,6 +1771,12 @@ def clone_url(url, duplicate_handling, check_quota, opts={})
else
self.upload_error_message = t :upload_error_unexpected, "An unknown error occurred downloading from %{url}", :url => url
end

if opts[:progress]
opts[:progress].message = self.upload_error_message
opts[:progress].fail!
end

self.save!
end
end
Expand Down
24 changes: 12 additions & 12 deletions lib/api/v1/attachment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -254,18 +254,18 @@ def api_attachment_preflight(context, request, opts = {})

on_duplicate = infer_on_duplicate(params)
if params[:url]
@attachment.send_later_enqueue_args(:clone_url,
{
priority: Delayed::LOW_PRIORITY,
max_attempts: 1,
n_strand: 'file_download'
},
params[:url], on_duplicate, opts[:check_quota])
json = {
id: @attachment.id,
upload_status: 'pending',
status_url: api_v1_file_status_url(@attachment, @attachment.uuid)
}
progress = ::Progress.new(context: @current_user, tag: :upload_via_url)
progress.reset!
progress.process_job(
@attachment,
:clone_url,
{ n_strand: 'file_download', preserve_method_args: true },
params[:url],
on_duplicate,
opts[:check_quota],
{ progress: progress }
)
json = { progress: progress_json(progress, @current_user, session) }
else
on_duplicate = nil if on_duplicate == 'overwrite'
quota_exemption = @attachment.quota_exemption_key if !opts[:check_quota]
Expand Down
67 changes: 40 additions & 27 deletions spec/apis/file_uploads_spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -184,24 +184,26 @@ def attachment_json(attachment, options = {})
local_storage!
# step 1, preflight
json = preflight({ :name => filename, :size => 20, :url => "http://www.example.com/images/delete.png" })
progress_url = json['progress']['url']
progress_id = json['progress']['id']
attachment = Attachment.order(:id).last
expect(attachment.file_state).to eq 'deleted'
status_url = json['status_url']
expect(status_url).to eq "http://www.example.com/api/v1/files/#{attachment.id}/#{attachment.uuid}/status"
expect(progress_url).to be_present

# step 2, download
json = api_call(:get, status_url, {:id => attachment.id.to_s, :controller => 'files', :action => 'api_file_status', :format => 'json', :uuid => attachment.uuid})
expect(json['upload_status']).to eq 'pending'
json = api_call(:get, progress_url, {:id => progress_id, :controller => 'progress', :action => 'show', :format => 'json'})
expect(json['workflow_state']).to eq 'queued'

expect(CanvasHttp).to receive(:get).with("http://www.example.com/images/delete.png").and_yield(FakeHttpResponse.new(200, "asdf"))
run_download_job

json = api_call(:get, status_url, {:id => attachment.id.to_s, :controller => 'files', :action => 'api_file_status', :format => 'json', :uuid => attachment.uuid})
json = api_call(:get, progress_url, {:id => progress_id, :controller => 'progress', :action => 'show', :format => 'json'})

expect(json).to eq({
'upload_status' => 'ready',
'attachment' => attachment_json(attachment.reload, no_doc_preview: true),
})
expect(json['workflow_state']).to eq('completed')
expect(json['results']).to be_present
expect(json['results']['id']).to eq(attachment.id)

attachment.reload
expect(attachment.file_state).to eq 'available'
expect(attachment.size).to eq 4
expect(attachment.user.id).to eq @user.id
Expand All @@ -213,13 +215,15 @@ def attachment_json(attachment, options = {})
local_storage!
# step 1, preflight
json = preflight({ :name => filename, :size => 20, :url => '#@$YA#Y#AGWREG' })
progress_url = json['progress']['url']
progress_id = json['progress']['id']
attachment = Attachment.order(:id).last
expect(json['status_url']).to eq "http://www.example.com/api/v1/files/#{attachment.id}/#{attachment.uuid}/status"
expect(progress_url).to be_present

# step 2, download
run_download_job
json = api_call(:get, json['status_url'], {:id => attachment.id.to_s, :controller => 'files', :action => 'api_file_status', :format => 'json', :uuid => attachment.uuid})
expect(json['upload_status']).to eq 'errored'
json = api_call(:get, progress_url, {:id => progress_id, :controller => 'progress', :action => 'show', :format => 'json'})
expect(json['workflow_state']).to eq 'failed'
expect(json['message']).to eq "Could not parse the URL: \#@$YA#Y#AGWREG"
expect(attachment.reload.file_state).to eq 'errored'
end
Expand All @@ -230,13 +234,15 @@ def attachment_json(attachment, options = {})
local_storage!
# step 1, preflight
json = preflight({ :name => filename, :size => 20, :url => '/images/delete.png' })
progress_url = json['progress']['url']
progress_id = json['progress']['id']
attachment = Attachment.order(:id).last
expect(json['status_url']).to eq "http://www.example.com/api/v1/files/#{attachment.id}/#{attachment.uuid}/status"
expect(progress_url).to be_present

# step 2, download
run_download_job
json = api_call(:get, json['status_url'], {:id => attachment.id.to_s, :controller => 'files', :action => 'api_file_status', :format => 'json', :uuid => attachment.uuid})
expect(json['upload_status']).to eq 'errored'
json = api_call(:get, progress_url, {:id => progress_id, :controller => 'progress', :action => 'show', :format => 'json'})
expect(json['workflow_state']).to eq 'failed'
expect(json['message']).to eq "No host provided for the URL: /images/delete.png"
expect(attachment.reload.file_state).to eq 'errored'
end
Expand All @@ -249,13 +255,15 @@ def attachment_json(attachment, options = {})
# step 1, preflight
expect(CanvasHttp).to receive(:get).with(url).and_yield(FakeHttpResponse.new(404))
json = preflight({ :name => filename, :size => 20, :url => url })
progress_url = json['progress']['url']
progress_id = json['progress']['id']
attachment = Attachment.order(:id).last
expect(json['status_url']).to eq "http://www.example.com/api/v1/files/#{attachment.id}/#{attachment.uuid}/status"
expect(progress_url).to be_present

# step 2, download
run_download_job
json = api_call(:get, json['status_url'], {:id => attachment.id.to_s, :controller => 'files', :action => 'api_file_status', :format => 'json', :uuid => attachment.uuid})
expect(json['upload_status']).to eq 'errored'
json = api_call(:get, progress_url, {:id => progress_id, :controller => 'progress', :action => 'show', :format => 'json'})
expect(json['workflow_state']).to eq 'failed'
expect(json['message']).to eq "Invalid response code, expected 200 got 404"
expect(attachment.reload.file_state).to eq 'errored'
end
Expand All @@ -268,13 +276,15 @@ def attachment_json(attachment, options = {})
# step 1, preflight
expect(CanvasHttp).to receive(:get).with(url).and_raise(Timeout::Error)
json = preflight({ :name => filename, :size => 20, :url => url })
progress_url = json['progress']['url']
progress_id = json['progress']['id']
attachment = Attachment.order(:id).last
expect(json['status_url']).to eq "http://www.example.com/api/v1/files/#{attachment.id}/#{attachment.uuid}/status"
expect(progress_url).to be_present

# step 2, download
run_download_job
json = api_call(:get, json['status_url'], {:id => attachment.id.to_s, :controller => 'files', :action => 'api_file_status', :format => 'json', :uuid => attachment.uuid})
expect(json['upload_status']).to eq 'errored'
json = api_call(:get, progress_url, {:id => progress_id, :controller => 'progress', :action => 'show', :format => 'json'})
expect(json['workflow_state']).to eq 'failed'
expect(json['message']).to eq "The request timed out: http://www.example.com/images/delete.png"
expect(attachment.reload.file_state).to eq 'errored'
end
Expand All @@ -287,14 +297,16 @@ def attachment_json(attachment, options = {})
# step 1, preflight
expect(CanvasHttp).to receive(:get).with(url).and_raise(CanvasHttp::TooManyRedirectsError)
json = preflight({ :name => filename, :size => 20, :url => url })
progress_url = json['progress']['url']
progress_id = json['progress']['id']
attachment = Attachment.order(:id).last
expect(attachment.workflow_state).to eq 'unattached'
expect(json['status_url']).to eq "http://www.example.com/api/v1/files/#{attachment.id}/#{attachment.uuid}/status"
expect(progress_url).to be_present

# step 2, download
run_download_job
json = api_call(:get, json['status_url'], {:id => attachment.id.to_s, :controller => 'files', :action => 'api_file_status', :format => 'json', :uuid => attachment.uuid})
expect(json['upload_status']).to eq 'errored'
json = api_call(:get, progress_url, {:id => progress_id, :controller => 'progress', :action => 'show', :format => 'json'})
expect(json['workflow_state']).to eq 'failed'
expect(json['message']).to eq "Too many redirects"
expect(attachment.reload.file_state).to eq 'errored'
end
Expand Down Expand Up @@ -507,13 +519,14 @@ def run_download_job
@context.write_attribute(:storage_quota, 1.megabyte)
@context.save!
json = preflight({ :name => "test.txt", :url => "http://www.example.com/test" })
status_url = json['status_url']
progress_url = json['progress']['url']
progress_id = json['progress']['id']
attachment = Attachment.order(:id).last
expect(CanvasHttp).to receive(:get).with("http://www.example.com/test").and_yield(FakeHttpResponse.new(200, (" " * 2.megabytes)))
run_jobs

json = api_call(:get, status_url, {:id => attachment.id.to_s, :controller => 'files', :action => 'api_file_status', :format => 'json', :uuid => attachment.uuid})
expect(json['upload_status']).to eq 'errored'
json = api_call(:get, progress_url, {:id => progress_id, :controller => 'progress', :action => 'show', :format => 'json'})
expect(json['workflow_state']).to eq 'failed'
expect(json['message']).to eq "file size exceeds quota limits: #{ActiveSupport::NumberHelper.number_to_delimited(2.megabytes)} bytes"
expect(attachment.file_state).to eq 'deleted'
end
Expand Down

0 comments on commit 9265aa5

Please sign in to comment.