Use the same structure for canvas files listing as any other source

Use the schema://content we use for other content sources. This is already the case at the "via url" level. We can't remove support for canvas_files & file_id while launching assignments as old assignment will still have those parameters as part of the deep link.
hypothesis · Mar 22, 2024 · 2d36462 · 2d36462
1 parent afe31e5
commit 2d36462
Show file tree

Hide file tree

Showing 8 changed files with 43 additions and 30 deletions.
diff --git a/lms/services/canvas_api/client.py b/lms/services/canvas_api/client.py
@@ -250,11 +250,13 @@ def list_files(self, course_id, sort="position") -> list[dict]:
             schema=self._ListFilesSchema,
         )
 
-        # Set the mime type. We currently only list PDF files, so we know it
-        # is always application/pdf. We could use the `content-type` property
-        # from the API response if we supported other content types.
         for file in files:
+            # Set the mime type. We currently only list PDF files, so we know it
+            # is always application/pdf. We could use the `content-type` property
+            # from the API response if we supported other content types.
             file["mime_type"] = "application/pdf"
+            # Add our own internal ID for this file
+            file["id"] = f"canvas://file/course/{course_id}/file_id/{file['lms_id']}"
 
         # Canvas' pagination is broken as it sorts by fields that allows duplicates.
         # This can lead to objects being skipped or duplicated across pages.
@@ -275,7 +277,7 @@ def list_files(self, course_id, sort="position") -> list[dict]:
                 {
                     "type": "canvas_file",
                     "course_id": course_id,
-                    "lms_id": file["id"],
+                    "lms_id": file["lms_id"],
                     "name": file["display_name"],
                     "size": file["size"],
                     "parent_lms_id": file["folder_id"],
@@ -346,7 +348,7 @@ class _ListFilesSchema(RequestsResponseSchema):
         many = True
 
         display_name = fields.Str(required=True)
-        id = fields.Integer(required=True)
+        lms_id = fields.Integer(required=True, data_key="id")
         updated_at = fields.String(required=True)
         size = fields.Integer(required=True)
         folder_id = fields.Integer(required=True)

diff --git a/lms/static/scripts/frontend_apps/components/ContentSelector.tsx b/lms/static/scripts/frontend_apps/components/ContentSelector.tsx
@@ -170,16 +170,15 @@ export default function ContentSelector({
   };
 
   // file.id is a URL with a `blackboard://`, `d2l://` or `moodle://` prefix.
-  const selectFileAsURL = (file: File) => selectURL(file.id);
 
+  const selectFileAsURL = (file: File) => selectURL(file.id);
   const selectPageAsURL = (page: File, lms: string) => {
     const name = `${lms} page: ${page.display_name}`;
     selectURL(page.id, name);
   };
 
   const selectCanvasFile = (file: File) => {
-    cancelDialog();
-    onSelectContent({ type: 'file', file });
+    selectURL(file.id);
   };
 
   const selectYouTubeURL = (url: string, title?: string) => {

diff --git a/lms/static/scripts/frontend_apps/components/FilePickerApp.tsx b/lms/static/scripts/frontend_apps/components/FilePickerApp.tsx
@@ -66,6 +66,9 @@ function formatContentURL(content: URLContent) {
   if (content.url.startsWith('canvas-studio://')) {
     return 'Video in Canvas Studio';
   }
+  if (content.url.startsWith('canvas://file')) {
+    return 'PDF file in Canvas';
+  }
   if (content.url.startsWith('d2l://')) {
     return 'PDF file in D2L';
   }
@@ -82,8 +85,6 @@ function contentDescription(content: Content) {
   switch (content.type) {
     case 'url':
       return formatContentURL(content);
-    case 'file':
-      return 'PDF file in Canvas';
     default:
       /* istanbul ignore next */
       throw new Error('Unknown content type');

diff --git a/lms/static/scripts/frontend_apps/components/test/ContentSelector-test.js b/lms/static/scripts/frontend_apps/components/test/ContentSelector-test.js
@@ -274,10 +274,11 @@ describe('ContentSelector', () => {
       {
         name: 'canvas',
         dialogName: 'canvasFile',
-        file: { id: 123 },
+        file: { id: 'canvas://file/123' },
         result: {
-          type: 'file',
-          file: { id: 123 },
+          type: 'url',
+          url: 'canvas://file/123',
+          name: undefined,
         },
         missingFilesHelpLink:
           'https://community.canvaslms.com/t5/Instructor-Guide/How-do-I-upload-a-file-to-a-course/ta-p/618',

diff --git a/lms/static/scripts/frontend_apps/components/test/FilePickerApp-test.js b/lms/static/scripts/frontend_apps/components/test/FilePickerApp-test.js
@@ -303,7 +303,7 @@ describe('FilePickerApp', () => {
         summary: 'PDF file in D2L',
       },
       {
-        content: { type: 'file', id: 'abcd' },
+        content: { type: 'url', url: 'canvas://file/ID' },
         summary: 'PDF file in Canvas',
       },
       {

diff --git a/lms/views/lti/deep_linking.py b/lms/views/lti/deep_linking.py
@@ -253,10 +253,7 @@ def _get_assignment_configuration(request) -> dict:
         if title := request.parsed_params.get("title"):
             params["title"] = title
 
-        if content["type"] == "file":
-            params["canvas_file"] = "true"
-            params["file_id"] = content["file"]["id"]
-        elif content["type"] == "url":
+        if content["type"] == "url":
             params["url"] = content["url"]
         else:
             raise ValueError(f"Unknown content type: '{content['type']}'")

diff --git a/tests/unit/lms/services/canvas_api/client_test.py b/tests/unit/lms/services/canvas_api/client_test.py
@@ -351,11 +351,19 @@ def test_list_files(
         )
 
         response = canvas_api_client.list_files("COURSE_ID")
-        expected_response = [
-            {**file, "mime_type": "application/pdf"} for file in list_files_json
-        ]
 
-        assert response == expected_response
+        assert response == [
+            {
+                "updated_at": file["updated_at"],
+                "lms_id": file["id"],
+                "folder_id": file["folder_id"],
+                "display_name": file["display_name"],
+                "size": file["size"],
+                "id": f'canvas://file/course/COURSE_ID/file_id/{file["id"]}',
+                "mime_type": "application/pdf",
+            }
+            for file in list_files_json
+        ]
         self.assert_session_send(
             http_session,
             "api/v1/courses/COURSE_ID/files",
@@ -403,7 +411,13 @@ def test_list_duplicate_files(self, canvas_api_client, http_session):
         response = canvas_api_client.list_files("COURSE_ID")
 
         expected_file = files[0].copy()
-        expected_file.update({"mime_type": "application/pdf"})
+        expected_file.update(
+            {
+                "mime_type": "application/pdf",
+                "lms_id": 1,
+                "id": "canvas://file/course/COURSE_ID/file_id/1",
+            }
+        )
         assert response == [expected_file]
 
     def test_list_files_with_folders(
@@ -448,11 +462,12 @@ def test_list_files_with_folders(
             {
                 "display_name": "File at root",
                 "size": 12345,
-                "id": 1,
+                "lms_id": 1,
                 "folder_id": 100,
                 "updated_at": "updated_at_1",
                 "type": "File",
                 "mime_type": "application/pdf",
+                "id": "canvas://file/course/COURSE_ID/file_id/1",
             },
             {
                 "id": 200,
@@ -464,11 +479,12 @@ def test_list_files_with_folders(
                     {
                         "display_name": "File in folder",
                         "size": 12345,
-                        "id": 2,
+                        "lms_id": 2,
                         "folder_id": 200,
                         "updated_at": "updated_at_2",
                         "type": "File",
                         "mime_type": "application/pdf",
+                        "id": "canvas://file/course/COURSE_ID/file_id/2",
                     },
                     {
                         "id": 300,
@@ -480,11 +496,12 @@ def test_list_files_with_folders(
                             {
                                 "display_name": "File in folder nested",
                                 "size": 12345,
-                                "id": 3,
+                                "lms_id": 3,
                                 "folder_id": 300,
                                 "updated_at": "updated_at_3",
                                 "type": "File",
                                 "mime_type": "application/pdf",
+                                "id": "canvas://file/course/COURSE_ID/file_id/3",
                             }
                         ],
                     },

diff --git a/tests/unit/lms/views/lti/deep_linking_test.py b/tests/unit/lms/views/lti/deep_linking_test.py
@@ -190,10 +190,6 @@ def test_it_for_v11(
     @pytest.mark.parametrize(
         "content,expected_from_content",
         [
-            (
-                {"type": "file", "file": {"id": 1}},
-                {"canvas_file": "true", "file_id": 1},
-            ),
             (
                 {"type": "url", "url": "https://example.com"},
                 {"url": "https://example.com"},