ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Only show download button for canvas files 

fixes MAT-28
flag=none

test plan:
- Create a new page
- Switch to html view
- Add an instructure_file_link to an external file
    <a class="instructure_file_link instructure_scribd_file"
      href="https://google.com">google</a>
- Add an instructure_file_link to a mailto link
    <a class="instructure_file_link instructure_scribd_file"
      href="mailto:test@test.com">mailto</a>
- Add an instructure_file_link to a course file
    <a class="instructure_file_link instructure_scribd_file"
      href="/courses/1/files/1">file</a>
- Add an instructure_file_link to a course file download
    <a class="instructure_file_link instructure_scribd_file"
      href="/courses/1/files/1/download">download</a>
- Add an instructure_file_link to a course file preview
    <a class="instructure_file_link instructure_scribd_file"
      href="/courses/1/files/1/preview">preview</a>
- Add an instructure_file_link to a course navigation item
    <a class="instructure_file_link instructure_scribd_file"
      href="/courses/1/pages/a-page">page</a>
- Download button should only show for files

Change-Id: I97069ca5dfd8b539a4da2eb47fba48bdde594412
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/262493
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Ed Schiebel <eschiebel@instructure.com>
QA-Review: Ed Schiebel <eschiebel@instructure.com>
Product-Review: Nate Armstrong <narmstrong@instructure.com>
This commit is contained in:
Nate Armstrong 2021-04-07 16:15:53 -06:00
parent 5bde2688dc
commit 1ad4207dcf
5 changed files with 71 additions and 32 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
lib/api/html/url_proxy.rb
View File

@ -55,10 +55,10 @@ module Api
%r{^/users/(#{ID})/files$} => ['Folder', :api_v1_user_folder_url, :user_id, {:id => 'root'}],
# Get file
%r{^/courses/(#{ID})/files/(#{ID})/} => ['File', :api_v1_course_attachment_url, :course_id, :id],
%r{^/groups/(#{ID})/files/(#{ID})/} => ['File', :api_v1_group_attachment_url, :group_id, :id],
%r{^/users/(#{ID})/files/(#{ID})/} => ['File', :api_v1_user_attachment_url, :user_id, :id],
%r{^/files/(#{ID})/} => ['File', :api_v1_attachment_url, :id],
%r{^/courses/(#{ID})/files/(#{ID})} => ['File', :api_v1_course_attachment_url, :course_id, :id],
%r{^/groups/(#{ID})/files/(#{ID})} => ['File', :api_v1_group_attachment_url, :group_id, :id],
%r{^/users/(#{ID})/files/(#{ID})} => ['File', :api_v1_user_attachment_url, :user_id, :id],
%r{^/files/(#{ID})} => ['File', :api_v1_attachment_url, :id],
# List quizzes
%r{^/courses/(#{ID})/quizzes$} => ['[Quiz]', :api_v1_course_quizzes_url, :course_id],

48
spec/lib/api/html/url_proxy_spec.rb
View File

@ -84,12 +84,58 @@ module Api
end
describe "#api_endpoint_info" do
it "maps good paths through to endpoints with return types" do
it 'maps quizzes' do
endpoint_info = proxy.api_endpoint_info("/courses/42/quizzes/24")
expect(endpoint_info['data-api-returntype']).to eq("Quiz")
expect(endpoint_info['data-api-endpoint']).to eq("http://example.com/api/v1/courses/42/quizzes/24")
end
it 'maps course files' do
endpoint_info = proxy.api_endpoint_info('/courses/1/files/230/preview')
expect(endpoint_info['data-api-returntype']).to eq("File")
expect(endpoint_info['data-api-endpoint']).to eq("http://example.com/api/v1/courses/1/files/230")
endpoint_info = proxy.api_endpoint_info('/courses/1/files/230/download')
expect(endpoint_info['data-api-returntype']).to eq("File")
expect(endpoint_info['data-api-endpoint']).to eq("http://example.com/api/v1/courses/1/files/230")
endpoint_info = proxy.api_endpoint_info('/courses/1/files/230?wrap=1')
expect(endpoint_info['data-api-returntype']).to eq("File")
expect(endpoint_info['data-api-endpoint']).to eq("http://example.com/api/v1/courses/1/files/230")
end
it 'does not map folders' do
endpoint_info = proxy.api_endpoint_info('/courses/1/files/folder/this_is_a_folder')
expect(endpoint_info['data-api-returntype']).to be_nil
expect(endpoint_info['data-api-endpoint']).to be_nil
end
it 'maps user files' do
endpoint_info = proxy.api_endpoint_info('/users/1/files/2?wrap=1')
expect(endpoint_info['data-api-returntype']).to eq("File")
expect(endpoint_info['data-api-endpoint']).to eq("http://example.com/api/v1/users/1/files/2")
endpoint_info = proxy.api_endpoint_info('/users/1/files/2/preview')
expect(endpoint_info['data-api-returntype']).to eq("File")
expect(endpoint_info['data-api-endpoint']).to eq("http://example.com/api/v1/users/1/files/2")
end
it 'maps group files' do
endpoint_info = proxy.api_endpoint_info('/groups/1/files/2?wrap=1')
expect(endpoint_info['data-api-returntype']).to eq("File")
expect(endpoint_info['data-api-endpoint']).to eq("http://example.com/api/v1/groups/1/files/2")
endpoint_info = proxy.api_endpoint_info('/groups/1/files/2/preview')
expect(endpoint_info['data-api-returntype']).to eq("File")
expect(endpoint_info['data-api-endpoint']).to eq("http://example.com/api/v1/groups/1/files/2")
end
it 'maps root files' do
endpoint_info = proxy.api_endpoint_info('/files/1')
expect(endpoint_info['data-api-returntype']).to eq("File")
expect(endpoint_info['data-api-endpoint']).to eq("http://example.com/api/v1/files/1")
end
it 'unescapes urls for sessionless launch endpoints' do
endpoint_info = proxy.api_endpoint_info('/courses/2/external_tools/retrieve?url=https%3A%2F%2Flti-tool-provider.herokuapp.com%2Flti_tool')
expect(endpoint_info['data-api-returntype']).to eq('SessionlessLaunchUrl')

31
spec/selenium/rcs/user_content_post_processing_spec.rb
View File

@ -31,7 +31,7 @@ describe 'user_content post processing' do
uploaded_data: fixture_file_upload('files/a_file.txt', 'text/plain')
)
@file.save!
@file_url = "http://#{HostUrl.default_host}/users/#{@teacher.id}/files/#{@file.id}"
@file_url = "/users/#{@teacher.id}/files/#{@file.id}"
end
def create_wiki_page_with_content(page_title, page_content)
@ -119,30 +119,21 @@ describe 'user_content post processing' do
expect(download_btn).to have_attribute('download')
end
it 'omits download button if an external link' do
it 'omits download button if not a file link' do
create_wiki_page_with_content(
'page',
"<a id='link1' class='instructure_file_link'
href='http://instructure.com'>external link</a>"
"<a id='page' class='instructure_file_link'
href='/courses/#{@course.id}/pages/other-page'>internal link</a>
<a id='external' class='instructure_file_link'
href='http://instructure.com'>external link</a>
<a id='mailto' class='instructure_file_link'
href='mailto:example@example.com'>example@example.com</a>"
)
get "/courses/#{@course.id}/pages/page"
# the link has an external-link button
expect(f('.ui-icon-extlink')).to be_displayed
expect(f('#link1')).not_to contain_css('.file_download_btn')
end
it 'omits download button if internal link' do
create_wiki_page_with_content(
'page',
"<a id='link1' class='instructure_file_link'
href='/courses/#{@course.id}/pages/other-page'>internal link</a>"
)
get "/courses/#{@course.id}/pages/page"
# look up the link by the data-api-returntype
# because this is how we determine to hide the download button for internal links
expect(f("a[data-api-returntype='Page']")).to be_displayed
expect(f('a#page')).to be_displayed
expect(f('a#external')).to be_displayed
expect(f('a#mailto')).to be_displayed
expect(f('body')).not_to contain_css('a.file_download_btn')
end
end

1
ui/features/k5_dashboard/react/HomeroomAnnouncement.js
View File

@ -82,6 +82,7 @@ export default function HomeroomAnnouncement({
title={attachment.filename}
/* classes request download button and preview overlay in instructure.js's postprocessing */
className="instructure_file_link preview_in_overlay"
data-api-returntype="File"
>
{attachment.display_name}
</a>

15
ui/features/navigation_header/jquery/instructure.js vendored
View File

@ -44,7 +44,7 @@ import 'jquery-tinypubsub' /* /\.publish\(/ */
import 'jqueryui/resizable'
import 'jqueryui/sortable'
import 'jqueryui/tabs'
import '../../../boot/initializers/trackGoogleAnalyticsEventsOnClick.js'
import '../../../boot/initializers/trackGoogleAnalyticsEventsOnClick'
let preview_counter = 0
function previewId() {
@ -206,11 +206,13 @@ export function enhanceUserContent() {
$('a.instructure_file_link, a.instructure_scribd_file').each(function () {
const $link = $(this)
$('.user_content.unenhanced:visible')
const apiReturnType = $link.data('api-returntype')
const isFile = apiReturnType == null || apiReturnType === 'File'
if ($link.find('.ui-icon-extlink').length || !isFile) {
// a bug in the new RCE added instructure_file_link class name to external and internal links for a while
const href = new URL($link[0].href)
const matchesCanvasFile = href.pathname.match(
/(?:\/(courses|groups|users)\/(\d+))?\/files\/(\d+)/
)
if (!matchesCanvasFile) {
// a bug in the new RCE added instructure_file_link class name to all links
// only proceed if this is a canvas file link
return
}
let $download_btn, $preview_link, $preview_container
@ -220,7 +222,6 @@ export function enhanceUserContent() {
const preview_id = previewId()
$preview_container = $('<div role="region" />').attr('id', preview_id).css('display', 'none')
if (ENV.FEATURES?.rce_better_file_downloading) {
const href = new URL($link[0].href)
const qs = href.searchParams
qs.delete('wrap')
qs.append('download_frd', '1')