Skip to content

Commit bb6527f

Browse files
maneframemaneframe
committed
fixes for api translation of attachment links
test plan: * create an object with html user content (such as an assignment description) * include a file download link (e.g. "/courses/:id/files/:file_id/download") * retrieve the object through the API * confirm that the translated link (that includes a verifier param), still has "/courses/:id" rather than just "/files/:file_id/download" * include a file preview link (e.g. "/courses/:id/files/:file_id/preview") * retrieve the object through the API * confirm that the translated link has "/courses/:id" and still has "/preview" as well as a verifier param * confirm that following the link in the browser results in downloading the file closes #CNVS-5213 #CNVS-5214 #CNVS-5215 Change-Id: Ib2bb6b1857055dbfe2d1b9e0873600beaa70bf75 Reviewed-on: https://gerrit.instructure.com/19512 Reviewed-by: Brian Palmer <brianp@instructure.com> Tested-by: Jenkins <jenkins@instructure.com> QA-Review: Adam Phillipps <adam@instructure.com> Product-Review: Bracken Mosbacker <bracken@instructure.com>
1 parent bc3f473 commit bb6527f

8 files changed

Lines changed: 153 additions & 55 deletions

File tree

‎lib/api.rb‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,17 @@ def api_user_content(html, context = @context, user = @current_user)
275275
end
276276
end
277277
next unless obj && rewriter.user_can_view_content?(obj)
278-
file_download_url(obj.id, :verifier => obj.uuid, :download => '1', :host => host, :protocol => protocol)
278+
279+
if ["Course", "Group", "Account", "User"].include?(obj.context_type)
280+
if match.rest.start_with?("/preview")
281+
url = self.send("#{obj.context_type.downcase}_file_preview_url", obj.context_id, obj.id, :verifier => obj.uuid, :host => host, :protocol => protocol)
282+
else
283+
url = self.send("#{obj.context_type.downcase}_file_download_url", obj.context_id, obj.id, :verifier => obj.uuid, :download => '1', :host => host, :protocol => protocol)
284+
end
285+
else
286+
url = file_download_url(obj.id, :verifier => obj.uuid, :download => '1', :host => host, :protocol => protocol)
287+
end
288+
url
279289
end
280290
html = rewriter.translate_content(html)
281291

‎spec/apis/api_spec_helper.rb‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -127,16 +127,20 @@ def should_translate_user_content(course)
127127
content = %{
128128
<p>
129129
Hello, students.<br>
130-
This will explain everything: <img src="/courses/#{course.id}/files/#{attachment.id}/preview" alt="important">
130+
This will explain everything: <img id="1" src="/courses/#{course.id}/files/#{attachment.id}/preview" alt="important">
131+
This won't explain anything: <img id="2" src="/courses/#{course.id}/files/#{attachment.id}/download" alt="important">
131132
Also, watch this awesome video: <a href="/media_objects/qwerty" class="instructure_inline_media_comment video_comment" id="media_comment_qwerty"><img></a>
132133
And refer to this <a href="/courses/#{course.id}/wiki/awesome-page">awesome wiki page</a>.
133134
</p>
134135
}
135136
html = yield content
136137
doc = Nokogiri::HTML::DocumentFragment.parse(html)
137-
img = doc.at_css('img')
138-
img.should be_present
139-
img['src'].should == "http://www.example.com/files/#{attachment.id}/download?verifier=#{attachment.uuid}"
138+
img1 = doc.at_css('img#1')
139+
img1.should be_present
140+
img1['src'].should == "http://www.example.com/courses/#{course.id}/files/#{attachment.id}/preview?verifier=#{attachment.uuid}"
141+
img2 = doc.at_css('img#2')
142+
img2.should be_present
143+
img2['src'].should == "http://www.example.com/courses/#{course.id}/files/#{attachment.id}/download?verifier=#{attachment.uuid}"
140144
video = doc.at_css('video')
141145
video.should be_present
142146
video['poster'].should match(%r{http://www.example.com/media_objects/qwerty/thumbnail})

‎spec/apis/user_content_spec.rb‎

Lines changed: 51 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,32 +19,72 @@
1919
require File.expand_path(File.dirname(__FILE__) + '/api_spec_helper')
2020

2121
describe UserContent, :type => :integration do
22-
it "should translate file links to directly-downloadable urls" do
22+
it "should translate course file download links to directly-downloadable urls" do
2323
course_with_teacher(:active_all => true)
2424
attachment_model
2525
@assignment = @course.assignments.create!(:title => "first assignment", :description => <<-HTML)
2626
<p>
2727
Hello, students.<br>
28-
This will explain everything: <img src="/courses/#{@course.id}/files/#{@attachment.id}/preview" alt="important">
28+
This will explain everything: <img src="/courses/#{@course.id}/files/#{@attachment.id}/download" alt="important">
2929
</p>
3030
HTML
3131

3232
json = api_call(:get,
33-
"/api/v1/courses/#{@course.id}/assignments/#{@assignment.id}",
34-
{ :controller => 'assignments_api', :action => 'show',
35-
:format => 'json', :course_id => @course.id.to_s, :id => @assignment.id.to_s })
33+
"/api/v1/courses/#{@course.id}/assignments/#{@assignment.id}",
34+
{ :controller => 'assignments_api', :action => 'show',
35+
:format => 'json', :course_id => @course.id.to_s, :id => @assignment.id.to_s })
3636

3737
doc = Nokogiri::HTML::DocumentFragment.parse(json['description'])
38-
doc.at_css('img')['src'].should == "http://www.example.com/files/#{@attachment.id}/download?verifier=#{@attachment.uuid}"
38+
doc.at_css('img')['src'].should == "http://www.example.com/courses/#{@course.id}/files/#{@attachment.id}/download?verifier=#{@attachment.uuid}"
39+
end
40+
41+
it "should translate group file download links to directly-downloadable urls" do
42+
course_with_teacher(:active_all => true)
43+
@group = @course.groups.create!(:name => "course group")
44+
attachment_model(:context => @group)
45+
@group.add_user(@teacher)
46+
@group_topic = @group.discussion_topics.create!(:title => "group topic", :user => @teacher, :message => <<-HTML)
47+
<p>
48+
Hello, students.<br>
49+
This will explain everything: <img src="/groups/#{@group.id}/files/#{@attachment.id}/download" alt="important">
50+
</p>
51+
HTML
52+
53+
json = api_call(:get,
54+
"/api/v1/groups/#{@group.id}/discussion_topics/#{@group_topic.id}",
55+
{ :controller => 'discussion_topics_api', :action => 'show',
56+
:format => 'json', :group_id => @group.id.to_s, :topic_id => @group_topic.id.to_s })
57+
58+
doc = Nokogiri::HTML::DocumentFragment.parse(json['message'])
59+
doc.at_css('img')['src'].should == "http://www.example.com/groups/#{@group.id}/files/#{@attachment.id}/download?verifier=#{@attachment.uuid}"
3960
end
4061

41-
it "should translate file links to directly-downloadable urls for deleted and replaced files" do
62+
it "should translate file download links to directly-downloadable urls for deleted and replaced files" do
4263
course_with_teacher(:active_all => true)
4364
attachment_model
4465
@attachment.destroy
4566
attachment2 = Attachment.create!(:folder => @attachment.folder, :context => @attachment.context, :filename => @attachment.filename, :uploaded_data => StringIO.new("first"))
4667
@context.attachments.find(@attachment.id).id.should == attachment2.id
4768

69+
@assignment = @course.assignments.create!(:title => "first assignment", :description => <<-HTML)
70+
<p>
71+
Hello, students.<br>
72+
This will explain everything: <img src="/courses/#{@course.id}/files/#{@attachment.id}/download" alt="important">
73+
</p>
74+
HTML
75+
76+
json = api_call(:get,
77+
"/api/v1/courses/#{@course.id}/assignments/#{@assignment.id}",
78+
{ :controller => 'assignments_api', :action => 'show',
79+
:format => 'json', :course_id => @course.id.to_s, :id => @assignment.id.to_s })
80+
81+
doc = Nokogiri::HTML::DocumentFragment.parse(json['description'])
82+
doc.at_css('img')['src'].should == "http://www.example.com/courses/#{@course.id}/files/#{attachment2.id}/download?verifier=#{attachment2.uuid}"
83+
end
84+
85+
it "should translate file preview links to directly-downloadable preview urls" do
86+
course_with_teacher(:active_all => true)
87+
attachment_model
4888
@assignment = @course.assignments.create!(:title => "first assignment", :description => <<-HTML)
4989
<p>
5090
Hello, students.<br>
@@ -53,12 +93,12 @@
5393
HTML
5494

5595
json = api_call(:get,
56-
"/api/v1/courses/#{@course.id}/assignments/#{@assignment.id}",
57-
{ :controller => 'assignments_api', :action => 'show',
58-
:format => 'json', :course_id => @course.id.to_s, :id => @assignment.id.to_s })
96+
"/api/v1/courses/#{@course.id}/assignments/#{@assignment.id}",
97+
{ :controller => 'assignments_api', :action => 'show',
98+
:format => 'json', :course_id => @course.id.to_s, :id => @assignment.id.to_s })
5999

60100
doc = Nokogiri::HTML::DocumentFragment.parse(json['description'])
61-
doc.at_css('img')['src'].should == "http://www.example.com/files/#{attachment2.id}/download?verifier=#{attachment2.uuid}"
101+
doc.at_css('img')['src'].should == "http://www.example.com/courses/#{@course.id}/files/#{@attachment.id}/preview?verifier=#{@attachment.uuid}"
62102
end
63103

64104
it "should translate media comment links to embedded video tags" do

‎spec/apis/v1/discussion_topics_api_spec.rb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1357,7 +1357,7 @@ def call_mark_entry_unread(course, topic, entry)
13571357
v0_r1 = v0['replies'][1]
13581358
v0_r1['id'].should == @reply2.id
13591359
v0_r1['user_id'].should == @teacher.id
1360-
v0_r1['message'].should == "<p><a href=\"http://#{Account.default.domain}/files/#{@reply2_attachment.id}/download?verifier=#{@reply2_attachment.uuid}\">This is a file link</a></p>\n <p>This is a video:\n <video poster=\"http://#{Account.default.domain}/media_objects/0_abcde/thumbnail?height=448&amp;type=3&amp;width=550\" data-media_comment_type=\"video\" preload=\"none\" class=\"instructure_inline_media_comment\" data-media_comment_id=\"0_abcde\" controls=\"controls\" src=\"http://#{Account.default.domain}/courses/#{@course.id}/media_download?entryId=0_abcde&amp;redirect=1&amp;type=mp4\">link</video>\n </p>"
1360+
v0_r1['message'].should == "<p><a href=\"http://#{Account.default.domain}/courses/#{@course.id}/files/#{@reply2_attachment.id}/download?verifier=#{@reply2_attachment.uuid}\">This is a file link</a></p>\n <p>This is a video:\n <video poster=\"http://#{Account.default.domain}/media_objects/0_abcde/thumbnail?height=448&amp;type=3&amp;width=550\" data-media_comment_type=\"video\" preload=\"none\" class=\"instructure_inline_media_comment\" data-media_comment_id=\"0_abcde\" controls=\"controls\" src=\"http://#{Account.default.domain}/courses/#{@course.id}/media_download?entryId=0_abcde&amp;redirect=1&amp;type=mp4\">link</video>\n </p>"
13611361
v0_r1['parent_id'].should == @root1.id
13621362
v0_r1['created_at'].should == @reply2.created_at.as_json
13631363
v0_r1['updated_at'].should == @reply2.updated_at.as_json

‎spec/apis/v1/submissions_api_spec.rb‎

Lines changed: 35 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -551,7 +551,7 @@ def submit_homework(assignment, student, opts = {:body => "test!"})
551551
submit_homework(a1, student1, :media_comment_id => "54321", :media_comment_type => "video")
552552
sub1 = submit_homework(a1, student1) { |s| s.attachments = [attachment_model(:context => student1, :folder => nil)] }
553553

554-
sub2 = submit_homework(a1, student2, :url => "http://www.instructure.com") { |s| s.attachment = attachment_model(:context => s, :filename => 'snapshot.png', :content_type => 'image/png'); s.attachments = [attachment_model(:context => a1, :filename => 'ss2.png', :content_type => 'image/png')] }
554+
sub2 = submit_homework(a1, student2, :url => "http://www.instructure.com") { |s| s.attachment = attachment_model(:context => student2, :filename => 'snapshot.png', :content_type => 'image/png'); s.attachments = [attachment_model(:context => student2, :filename => 'ss2.png', :content_type => 'image/png')] }
555555

556556
media_object(:media_id => "3232", :context => student1, :user => student1, :media_type => "audio")
557557
a1.grade_student(student1, {:grade => '90%', :comment => "Well here's the thing...", :media_comment_id => "3232", :media_comment_type => "audio", :grader => @teacher})
@@ -728,6 +728,22 @@ def submit_homework(assignment, student, opts = {:body => "test!"})
728728
"grade_matches_current_submission"=>true,
729729
"attachments" =>
730730
[
731+
{"content-type" => "image/png",
732+
"display_name" => "snapshot.png",
733+
"filename" => "snapshot.png",
734+
"url" => "http://www.example.com/files/#{sub2.attachment.id}/download?download_frd=1&verifier=#{sub2.attachment.uuid}",
735+
"id" => sub2.attachment.id,
736+
"size" => sub2.attachment.size,
737+
'unlock_at' => nil,
738+
'locked' => false,
739+
'hidden' => false,
740+
'lock_at' => nil,
741+
'locked_for_user' => false,
742+
'hidden_for_user' => false,
743+
'created_at' => sub2.attachment.created_at.as_json,
744+
'updated_at' => sub2.attachment.updated_at.as_json,
745+
'thumbnail_url' => sub2.attachment.thumbnail_url
746+
},
731747
{"content-type" => "image/png",
732748
"display_name" => "ss2.png",
733749
"filename" => "ss2.png",
@@ -743,23 +759,7 @@ def submit_homework(assignment, student, opts = {:body => "test!"})
743759
'created_at' => sub2.attachments.first.reload.created_at.as_json,
744760
'updated_at' => sub2.attachments.first.updated_at.as_json,
745761
'thumbnail_url' => sub2.attachments.first.thumbnail_url,
746-
},
747-
{"content-type" => "image/png",
748-
"display_name" => "snapshot.png",
749-
"filename" => "snapshot.png",
750-
"url" => "http://www.example.com/files/#{sub2.attachment.id}/download?download_frd=1&verifier=#{sub2.attachment.uuid}",
751-
"id" => sub2.attachment.id,
752-
"size" => sub2.attachment.size,
753-
'unlock_at' => nil,
754-
'locked' => false,
755-
'hidden' => false,
756-
'lock_at' => nil,
757-
'locked_for_user' => false,
758-
'hidden_for_user' => false,
759-
'created_at' => sub2.attachment.created_at.as_json,
760-
'updated_at' => sub2.attachment.updated_at.as_json,
761-
'thumbnail_url' => sub2.attachment.thumbnail_url,
762-
},
762+
}
763763
],
764764
"score"=>9,
765765
"workflow_state" => "graded",
@@ -769,7 +769,22 @@ def submit_homework(assignment, student, opts = {:body => "test!"})
769769
"submission_type"=>"online_url",
770770
"user_id"=>student2.id,
771771
"attachments" =>
772-
[
772+
[{"content-type" => "image/png",
773+
"display_name" => "snapshot.png",
774+
"filename" => "snapshot.png",
775+
"url" => "http://www.example.com/files/#{sub2.attachment.id}/download?download_frd=1&verifier=#{sub2.attachment.uuid}",
776+
"id" => sub2.attachment.id,
777+
"size" => sub2.attachment.size,
778+
'unlock_at' => nil,
779+
'locked' => false,
780+
'hidden' => false,
781+
'lock_at' => nil,
782+
'locked_for_user' => false,
783+
'hidden_for_user' => false,
784+
'created_at' => sub2.attachment.created_at.as_json,
785+
'updated_at' => sub2.attachment.updated_at.as_json,
786+
'thumbnail_url' => sub2.attachment.thumbnail_url,
787+
},
773788
{"content-type" => "image/png",
774789
"display_name" => "ss2.png",
775790
"filename" => "ss2.png",
@@ -785,23 +800,7 @@ def submit_homework(assignment, student, opts = {:body => "test!"})
785800
'created_at' => sub2.attachments.first.created_at.as_json,
786801
'updated_at' => sub2.attachments.first.updated_at.as_json,
787802
'thumbnail_url' => sub2.attachments.first.thumbnail_url,
788-
},
789-
{"content-type" => "image/png",
790-
"display_name" => "snapshot.png",
791-
"filename" => "snapshot.png",
792-
"url" => "http://www.example.com/files/#{sub2.attachment.id}/download?download_frd=1&verifier=#{sub2.attachment.uuid}",
793-
"id" => sub2.attachment.id,
794-
"size" => sub2.attachment.size,
795-
'unlock_at' => nil,
796-
'locked' => false,
797-
'hidden' => false,
798-
'lock_at' => nil,
799-
'locked_for_user' => false,
800-
'hidden_for_user' => false,
801-
'created_at' => sub2.attachment.created_at.as_json,
802-
'updated_at' => sub2.attachment.updated_at.as_json,
803-
'thumbnail_url' => sub2.attachment.thumbnail_url,
804-
},
803+
}
805804
],
806805
"submission_comments"=>[],
807806
"score"=>9,

‎spec/factories/attachment_factory.rb‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,8 @@ def attachment_model(opts={})
2626
end
2727

2828
def valid_attachment_attributes(opts={})
29-
@context ||= opts[:context] || Course.first || course_model(:reusable => true)
29+
@context = opts[:context] || @context
30+
@context ||= Course.first || course_model(:reusable => true)
3031
@folder = Folder.root_folders(@context).find{|f| f.name == 'unfiled'} || Folder.root_folders(@context).first || folder_model
3132
@attributes_res = {
3233
:context => @context,

‎spec/integration/files_spec.rb‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,50 @@
146146
# ensure that the user wasn't logged in by the normal means
147147
controller.instance_variable_get(:@current_user).should be_nil
148148
end
149+
150+
it "should be able to use verifier in course context" do
151+
course_with_teacher(:active_all => true, :user => user_with_pseudonym)
152+
a1 = attachment_model(:uploaded_data => stub_png_data, :content_type => 'image/png', :context => @course)
153+
HostUrl.stubs(:file_host_with_shard).returns(['files-test.host', Shard.default])
154+
get "http://test.host/courses/#{@course.id}/files/#{a1.id}/download?verifier=#{a1.uuid}"
155+
response.should be_redirect
156+
157+
uri = URI.parse response['Location']
158+
qs = Rack::Utils.parse_nested_query(uri.query)
159+
uri.host.should == 'files-test.host'
160+
uri.path.should == "/courses/#{@course.id}/files/#{a1.id}/course%20files/test%20my%20file%3F%20hai!%26.png"
161+
qs['verifier'].should == a1.uuid
162+
location = response['Location']
163+
reset!
164+
165+
get location
166+
response.should be_success
167+
response.content_type.should == 'image/png'
168+
# ensure that the user wasn't logged in by the normal means
169+
controller.instance_variable_get(:@current_user).should be_nil
170+
end
171+
172+
it "should be able to directly download in course context preview links with verifier" do
173+
course_with_teacher(:active_all => true, :user => user_with_pseudonym)
174+
a1 = attachment_model(:uploaded_data => stub_png_data, :content_type => 'image/png', :context => @course)
175+
HostUrl.stubs(:file_host_with_shard).returns(['files-test.host', Shard.default])
176+
get "http://test.host/courses/#{@course.id}/files/#{a1.id}/preview?verifier=#{a1.uuid}"
177+
response.should be_redirect
178+
179+
uri = URI.parse response['Location']
180+
qs = Rack::Utils.parse_nested_query(uri.query)
181+
uri.host.should == 'files-test.host'
182+
uri.path.should == "/courses/#{@course.id}/files/#{a1.id}/course%20files/test%20my%20file%3F%20hai!%26.png"
183+
qs['verifier'].should == a1.uuid
184+
location = response['Location']
185+
reset!
186+
187+
get location
188+
response.should be_success
189+
response.content_type.should == 'image/png'
190+
# ensure that the user wasn't logged in by the normal means
191+
controller.instance_variable_get(:@current_user).should be_nil
192+
end
149193

150194
it "should update module progressions for html safefiles iframe" do
151195
HostUrl.stubs(:file_host_with_shard).returns(['files-test.host', Shard.default])

‎spec/models/discussion_topic/materialized_view_spec.rb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ def map_to_ids_and_replies(list)
7676
# verify the api_user_content functionality in a non-request context
7777
html_message = json[0]['replies'][1]['message']
7878
html = Nokogiri::HTML::DocumentFragment.parse(html_message)
79-
html.at_css('a')['href'].should == "http://localhost/files/#{@reply2_attachment.id}/download?verifier=#{@reply2_attachment.uuid}"
79+
html.at_css('a')['href'].should == "http://localhost/courses/#{@course.id}/files/#{@reply2_attachment.id}/download?verifier=#{@reply2_attachment.uuid}"
8080
html.at_css('video')['src'].should == "http://localhost/courses/#{@course.id}/media_download?entryId=0_abcde&redirect=1&type=mp4"
8181

8282
# the deleted entry will be marked deleted and have no summary

0 commit comments

Comments
 (0)