Repository: ucfopen/canvasapi

Issue:
Support uploading files for submission comments
As far as I can tell, `canvasapi` 0.6.0 has no support for [uploading files to be used in submission comments](https://canvas.instructure.com/doc/api/submission_comments.html). I would like to automate parts of my grading workflow that require this capability. It would be nice for `canvasapi` to help me get that done.

Note that [the complete file-upload workflow](https://canvas.instructure.com/doc/api/file.file_uploads.html) is rather complex. It would be great to have an elegant `canvasapi` interface that simplifies all of this. Barring that, access to each of the primitive steps would at least be better than nothing.

Discussion:

Patch:
diff --git a/.gitignore b/.gitignore
index 1987e91..c8fa7a6 100644
--- a/.gitignore
+++ b/.gitignore
@@ -12,6 +12,7 @@
 /build/
 /coverage.xml
 /dist/
+/docs/_build/
 /env/
 /htmlcov/
 \#*#
diff --git a/canvasapi/course.py b/canvasapi/course.py
index 7b87e2d..d5b64d9 100644
--- a/canvasapi/course.py
+++ b/canvasapi/course.py
@@ -231,8 +231,6 @@ class Course(CanvasObject):
         :calls: `POST /api/v1/courses/:course_id/files \
         <https://canvas.instructure.com/doc/api/courses.html#method.courses.create_file>`_
 
-        :param path: The path of the file to upload.
-        :type path: str
         :param file: The file or path of the file to upload.
         :type file: file or str
         :returns: True if the file uploaded successfully, False otherwise, \
@@ -1101,12 +1099,14 @@ class Course(CanvasObject):
             'courses/%s/assignments/%s/submissions' % (self.id, assignment_id),
             _kwargs=combine_kwargs(**kwargs)
         )
+        response_json = response.json()
+        response_json.update(course_id=self.id)
 
-        return Submission(self._requester, response.json())
+        return Submission(self._requester, response_json)
 
     def list_submissions(self, assignment_id, **kwargs):
         """
-        Makes a submission for an assignment.
+        Get all existing submissions for an assignment.
 
         :calls: `GET /api/v1/courses/:course_id/assignments/:assignment_id/submissions  \
         <https://canvas.instructure.com/doc/api/submissions.html#method.submissions_api.index>`_
@@ -1121,6 +1121,7 @@ class Course(CanvasObject):
             self._requester,
             'GET',
             'courses/%s/assignments/%s/submissions' % (self.id, assignment_id),
+            {'course_id': self.id},
             _kwargs=combine_kwargs(**kwargs)
         )
 
@@ -1144,6 +1145,7 @@ class Course(CanvasObject):
             self._requester,
             'GET',
             'courses/%s/students/submissions' % (self.id),
+            {'course_id': self.id},
             _kwargs=combine_kwargs(**kwargs)
         )
 
@@ -1165,7 +1167,10 @@ class Course(CanvasObject):
             'courses/%s/assignments/%s/submissions/%s' % (self.id, assignment_id, user_id),
             _kwargs=combine_kwargs(**kwargs)
         )
-        return Submission(self._requester, response.json())
+        response_json = response.json()
+        response_json.update(course_id=self.id)
+
+        return Submission(self._requester, response_json)
 
     def update_submission(self, assignment_id, user_id, **kwargs):
         """
@@ -1186,12 +1191,15 @@ class Course(CanvasObject):
             _kwargs=combine_kwargs(**kwargs)
         )
 
+        response_json = response.json()
+        response_json.update(course_id=self.id)
+
         submission = self.get_submission(assignment_id, user_id)
 
-        if 'submission_type' in response.json():
-            super(Submission, submission).set_attributes(response.json())
+        if 'submission_type' in response_json:
+            super(Submission, submission).set_attributes(response_json)
 
-        return Submission(self._requester, response.json())
+        return Submission(self._requester, response_json)
 
     def list_gradeable_students(self, assignment_id):
         """
diff --git a/canvasapi/section.py b/canvasapi/section.py
index f9f86ca..89fe900 100644
--- a/canvasapi/section.py
+++ b/canvasapi/section.py
@@ -124,12 +124,14 @@ class Section(CanvasObject):
             'sections/%s/assignments/%s/submissions' % (self.id, assignment_id),
             _kwargs=combine_kwargs(**kwargs)
         )
+        response_json = response.json()
+        response_json.update(section_id=self.id)
 
-        return Submission(self._requester, response.json())
+        return Submission(self._requester, response_json)
 
     def list_submissions(self, assignment_id, **kwargs):
         """
-        Makes a submission for an assignment.
+        Get all existing submissions for an assignment.
 
         :calls: `GET /api/v1/sections/:section_id/assignments/:assignment_id/submissions  \
         <https://canvas.instructure.com/doc/api/submissions.html#method.submissions_api.index>`_
@@ -144,6 +146,7 @@ class Section(CanvasObject):
             self._requester,
             'GET',
             'sections/%s/assignments/%s/submissions' % (self.id, assignment_id),
+            {'section_id': self.id},
             _kwargs=combine_kwargs(**kwargs)
         )
 
@@ -167,6 +170,7 @@ class Section(CanvasObject):
             self._requester,
             'GET',
             'sections/%s/students/submissions' % (self.id),
+            {'section_id': self.id},
             _kwargs=combine_kwargs(**kwargs)
         )
 
@@ -188,7 +192,10 @@ class Section(CanvasObject):
             'sections/%s/assignments/%s/submissions/%s' % (self.id, assignment_id, user_id),
             _kwargs=combine_kwargs(**kwargs)
         )
-        return Submission(self._requester, response.json())
+        response_json = response.json()
+        response_json.update(section_id=self.id)
+
+        return Submission(self._requester, response_json)
 
     def update_submission(self, assignment_id, user_id, **kwargs):
         """
@@ -211,10 +218,13 @@ class Section(CanvasObject):
 
         submission = self.get_submission(assignment_id, user_id)
 
-        if 'submission_type' in response.json():
-            super(Submission, submission).set_attributes(response.json())
+        response_json = response.json()
+        response_json.update(section_id=self.id)
+
+        if 'submission_type' in response_json:
+            super(Submission, submission).set_attributes(response_json)
 
-        return Submission(self._requester, response.json())
+        return Submission(self._requester, response_json)
 
     def mark_submission_as_read(self, assignment_id, user_id):
         """
diff --git a/canvasapi/submission.py b/canvasapi/submission.py
index f1afb73..9592f55 100644
--- a/canvasapi/submission.py
+++ b/canvasapi/submission.py
@@ -3,6 +3,7 @@ from __future__ import absolute_import, division, print_function, unicode_litera
 from six import python_2_unicode_compatible
 
 from canvasapi.canvas_object import CanvasObject
+from canvasapi.upload import Uploader
 
 
 @python_2_unicode_compatible
@@ -10,3 +11,31 @@ class Submission(CanvasObject):
 
     def __str__(self):
         return "{}".format(self.id)
+
+    def upload_comment(self, file, **kwargs):
+        """
+        Upload a file to attach to this submission comment.
+
+        :calls: `POST \
+        /api/v1/courses/:course_id/assignments/:assignment_id/submissions/:user_id/comments/files \
+        <https://canvas.instructure.com/doc/api/submission_comments.html#method.submission_comments_api.create_file>`_
+
+        :param file: The file or path of the file to upload.
+        :type file: file or str
+        :returns: True if the file uploaded successfully, False otherwise, \
+            and the JSON response from the API.
+        :rtype: tuple
+        """
+        if not hasattr(self, 'course_id'):
+            raise ValueError('Must use a course to upload file comments.')
+
+        return Uploader(
+            self._requester,
+            'courses/{}/assignments/{}/submissions/{}/comments/files'.format(
+                self.course_id,
+                self.assignment_id,
+                self.user_id
+            ),
+            file,
+            **kwargs
+        ).start()


Test patch:
diff --git a/tests/fixtures/submission.json b/tests/fixtures/submission.json
new file mode 100644
index 0000000..4ea46c1
--- /dev/null
+++ b/tests/fixtures/submission.json
@@ -0,0 +1,44 @@
+{
+	"get_by_id_course": {
+		"method": "GET",
+		"endpoint": "courses/1/assignments/1/submissions/1",
+		"data": {
+			"id": 1,
+			"assignment_id": 1,
+			"user_id": 1,
+			"html_url": "http://example.com/courses/1/assignments/1/submissions/1",
+			"submission_type": "online_upload"
+		},
+		"status_code": 200
+	},
+	"get_by_id_section": {
+		"method": "GET",
+		"endpoint": "sections/1/assignments/1/submissions/1",
+		"data": {
+				"id": 1,
+				"assignment_id": 1,
+				"user_id": 1,
+				"html_url": "http://example.com/sections/1/assignments/1/submissions/1",
+				"submission_type": "online_upload"
+		},
+		"status_code": 200
+	},
+	"upload_comment": {
+		"method": "POST",
+		"endpoint": "courses/1/assignments/1/submissions/1/comments/files",
+		"data": {
+			"upload_url": "http://example.com/api/v1/files/upload_response_upload_url",
+			"upload_params": {
+				"some_param": "param123",
+				"a_different_param": "param456"
+			}
+		}
+	},
+	"upload_comment_final": {
+		"method": "POST",
+		"endpoint": "files/upload_response_upload_url",
+		"data": {
+			"url": "great_url_success"
+		}
+	}
+}
\ No newline at end of file
diff --git a/tests/test_course.py b/tests/test_course.py
index b2bc030..f3b42ed 100644
--- a/tests/test_course.py
+++ b/tests/test_course.py
@@ -1,5 +1,4 @@
 from __future__ import absolute_import, division, print_function, unicode_literals
-import os
 import unittest
 import uuid
 import warnings
@@ -26,7 +25,7 @@ from canvasapi.user import User
 from canvasapi.submission import Submission
 from canvasapi.user import UserDisplay
 from tests import settings
-from tests.util import register_uris
+from tests.util import cleanup_file, register_uris
 
 
 @requests_mock.Mocker()
@@ -169,12 +168,7 @@ class TestCourse(unittest.TestCase):
         self.assertIsInstance(response[1], dict)
         self.assertIn('url', response[1])
 
-        # http://stackoverflow.com/a/10840586
-        # Not as stupid as it looks.
-        try:
-            os.remove(filename)
-        except OSError:
-            pass
+        cleanup_file(filename)
 
     # reset()
     def test_reset(self, m):
diff --git a/tests/test_group.py b/tests/test_group.py
index 4388ec2..3ecf80a 100644
--- a/tests/test_group.py
+++ b/tests/test_group.py
@@ -1,5 +1,4 @@
 from __future__ import absolute_import, division, print_function, unicode_literals
-import os
 import unittest
 import uuid
 
@@ -15,7 +14,7 @@ from canvasapi.file import File
 from canvasapi.folder import Folder
 from canvasapi.tab import Tab
 from tests import settings
-from tests.util import register_uris
+from tests.util import cleanup_file, register_uris
 
 
 @requests_mock.Mocker()
@@ -145,12 +144,8 @@ class TestGroup(unittest.TestCase):
         self.assertTrue(response[0])
         self.assertIsInstance(response[1], dict)
         self.assertIn('url', response[1])
-        # http://stackoverflow.com/a/10840586
-        # Not as stupid as it looks.
-        try:
-            os.remove(filename)
-        except OSError:
-            pass
+
+        cleanup_file(filename)
 
     # preview_processed_html()
     def test_preview_processed_html(self, m):
diff --git a/tests/test_submission.py b/tests/test_submission.py
index 8f178c1..e526424 100644
--- a/tests/test_submission.py
+++ b/tests/test_submission.py
@@ -1,11 +1,12 @@
 from __future__ import absolute_import, division, print_function, unicode_literals
 import unittest
+import uuid
 
 import requests_mock
 
 from canvasapi import Canvas
 from tests import settings
-from tests.util import register_uris
+from tests.util import cleanup_file, register_uris
 
 
 @requests_mock.Mocker()
@@ -16,13 +17,36 @@ class TestSubmission(unittest.TestCase):
 
         with requests_mock.Mocker() as m:
             register_uris({
-                'section': ['get_by_id', 'get_submission']
+                'course': ['get_by_id'],
+                'section': ['get_by_id'],
+                'submission': ['get_by_id_course', 'get_by_id_section']
             }, m)
 
+            self.course = self.canvas.get_course(1)
+            self.submission_course = self.course.get_submission(1, 1)
             self.section = self.canvas.get_section(1)
-            self.submission = self.section.get_submission(1, 1)
+            self.submission_section = self.section.get_submission(1, 1)
 
     # __str__()
     def test__str__(self, m):
-        string = str(self.submission)
+        string = str(self.submission_course)
         self.assertIsInstance(string, str)
+
+    # upload_comment()
+    def test_upload_comment(self, m):
+        register_uris({'submission': ['upload_comment', 'upload_comment_final']}, m)
+
+        filename = 'testfile_submission_%s' % uuid.uuid4().hex
+        with open(filename, 'w+') as file:
+            response = self.submission_course.upload_comment(file)
+
+        self.assertTrue(response[0])
+        self.assertIsInstance(response[1], dict)
+        self.assertIn('url', response[1])
+
+        cleanup_file(filename)
+
+    def test_upload_comment_section(self, m):
+        # Sections do not support uploading file comments
+        with self.assertRaises(ValueError):
+            self.submission_section.upload_comment('fakefilename.txt')
diff --git a/tests/test_uploader.py b/tests/test_uploader.py
index ad7a964..bf04942 100644
--- a/tests/test_uploader.py
+++ b/tests/test_uploader.py
@@ -1,5 +1,4 @@
 from __future__ import absolute_import, division, print_function, unicode_literals
-import os
 import unittest
 import uuid
 
@@ -8,7 +7,7 @@ import requests_mock
 from canvasapi.canvas import Canvas
 from canvasapi.upload import Uploader
 from tests import settings
-from tests.util import register_uris
+from tests.util import cleanup_file, register_uris
 
 
 @requests_mock.Mocker()
@@ -24,12 +23,7 @@ class TestUploader(unittest.TestCase):
     def tearDown(self):
         self.file.close()
 
-        # http://stackoverflow.com/a/10840586
-        # Not as stupid as it looks.
-        try:
-            os.remove(self.filename)
-        except OSError:
-            pass
+        cleanup_file(self.filename)
 
     # start()
     def test_start(self, m):
diff --git a/tests/test_user.py b/tests/test_user.py
index 1a4517e..f13a5ef 100644
--- a/tests/test_user.py
+++ b/tests/test_user.py
@@ -1,5 +1,4 @@
 from __future__ import absolute_import, division, print_function, unicode_literals
-import os
 import unittest
 import uuid
 
@@ -20,7 +19,7 @@ from canvasapi.page_view import PageView
 from canvasapi.user import User
 from canvasapi.login import Login
 from tests import settings
-from tests.util import register_uris
+from tests.util import cleanup_file, register_uris
 
 
 @requests_mock.Mocker()
@@ -202,12 +201,7 @@ class TestUser(unittest.TestCase):
         self.assertIsInstance(response[1], dict)
         self.assertIn('url', response[1])
 
-        # http://stackoverflow.com/a/10840586
-        # Not as stupid as it looks.
-        try:
-            os.remove(filename)
-        except OSError:
-            pass
+        cleanup_file(filename)
 
     # list_groups()
     def test_list_groups(self, m):
diff --git a/tests/util.py b/tests/util.py
index c7d74ea..3b92514 100644
--- a/tests/util.py
+++ b/tests/util.py
@@ -1,5 +1,6 @@
 from __future__ import absolute_import, division, print_function, unicode_literals
 import json
+import os
 
 import requests_mock
 
@@ -51,3 +52,15 @@ def register_uris(requirements, requests_mocker):
                 )
             except Exception as e:
                 print(e)
+
+
+def cleanup_file(filename):
+    """
+    Remove a test file from the system. If the file doesn't exist, ignore.
+
+    `Not as stupid as it looks. <http://stackoverflow.com/a/10840586>_`
+    """
+    try:
+        os.remove(filename)
+    except OSError:
+        pass