implement kwargs (fix #631) for upload_comment

[bainco]

Proposed Changes

• Updates upload_comment to allow kwargs to be passed as part of the upload file process.
• non-breaking change

Indirectly fixes #631 and also allows for other arbitrary submission edit properties to be posted along with the attachments.

Limitations

Note, this implementation emulates the original implementation's flaw of returning the outcome of the upload file process but not the actually updated submission object that Submission.edit returns. This means in the test cases, you can only test to see if the file was successfully updated, not if the comment itself was posted on the submission.

It's also worth noting it doesn't directly fix #631 since you still need to manually specify the attempt. Not sure if that should be a default value or not given the way the edit function works currently.

some_submission.upload_comment(
    file, comment={"attempt": some_submission.attempt, "text_comment": "Text that can appear instead of generic 'See Attached Files' message."}

Editors Note: earlier PR wasn't compatible with 3.7. When fixing the issue I nuked the repo. My bad.

Quick Fix

If anyone needs to patch this in the meantime, you can do so wherever you need to use the upload_comment method:

from canvasapi.submission import Submission
from canvasapi.upload import FileOrPathLike, Uploader

def upload_comment(self, file: FileOrPathLike, **kwargs):
    """
    Upload a file to attach to this submission as a 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
    """
    response = Uploader(
        self._requester,
        "courses/{}/assignments/{}/submissions/{}/comments/files".format(
            self.course_id, self.assignment_id, self.user_id
        ),
        file,
        **kwargs
    ).start()

    if response[0]:
        if "comment" in kwargs:
            kwargs["comment"].update({"file_ids": [response[1]["id"]]})
        else:
            kwargs["comment"] = {"file_ids": [response[1]["id"]]}

        self.edit(**kwargs)

    return response

# monkey patching built-in Submission.upload_comment with the new version
Submission.upload_comment = upload_comment

[jonespm]

I'd be concerned about some unexpected side-effects of changing this value as the default hardcoded.

I think these methods should allow this parameter for potential compatibility but it should probably be passed in.

For instance

def download(self, location, use_auth=False):
...
    response = self._requester.request("GET", _url=self.url, use_auth=use_auth)

For each method in here that has a _requester.request (or at least download and get_contents where you identified that this was an issue)

[bainco]

I agree; it would be worth reviewing what other kwargs might also be made accessible at that level for the requester to make this sort of thing easier to diagnose/fix in the future.

I had honestly forgotten this PR was still open so I pushed that commit to fix a private repo not thinking about how it would affect this PR.

Edit: e6127f0 is the only one that actually pertains to this PR and I think it's worth revisiting how upload_comment works in general because it's performing multiple steps (and multiple API calls) on your behalf unlike most of the other functions that limit themselves to a single API call (or multiple API calls for something that's paginated).