Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Course.list_multiple_submissions mismanages grouped response #61

Closed
liblit opened this issue Aug 27, 2017 · 3 comments
Closed

Course.list_multiple_submissions mismanages grouped response #61

liblit opened this issue Aug 27, 2017 · 3 comments
Assignees
Labels

Comments

@liblit
Copy link
Contributor

liblit commented Aug 27, 2017

The Course.list_multiple_submissions method returns a paginated list of Submission instances. However, printing any of these instances using its default string conversion fails:

submissions = course.list_multiple_submissions(student_ids='all', assignment_ids=(123, 456))
for submission in submissions:
    print(submission)

AttributeError: 'Submission' object has no attribute 'id'

If instead I print each submission’s __class__ and attribute dictionary, I see something rather strange. The class is indeed canvasapi.submission.Submission. However, the attributes are not those that a Submission should have. Ignoring the double-underscore internal attributes, each submission instance has:

  • _requester
  • attributes
  • integration_id
  • section_id
  • set_attributes
  • sis_user_id
  • submissions
  • to_json
  • user_id

The sis_user_id and user_id attributes tell me that this is some sort of person-identifying structure along the lines of a User or Enrollment object. But it does not have the complete set of attributes for either of those. The value of the submissions attribute is a list of dicts; I believe that each of these dicts corresponds to one Submission object.

The Course.list_multiple_submissions method produces a GET /api/v1/courses/:course_id/students/submissions request, and the documentation for that request shows that its response can take one of two forms depending on the grouped parameter. I can see that the Course.list_multiple_submissions implementation always hard-codes grouped=False, but the response coming back looks exactly like the Canvas API documentation example of a grouped response, complete with those submissions attributes giving lists of Submission objects.

So it seems that the grouped=False aspect of the request is not working as intended. I don’t know why; is this a Canvas bug or a canvasapi bug? Either way, the current behavior certainly is not working as intended. canvasapi should either request a non-grouped response in a way that works, or else it should process the grouped response in a way that respects the actual structure and meaning of the returned data.

For now I am working around this myself by building new Submission instances out of the dicts in each of the submissions attributes, like this:

groups = course.list_multiple_submissions(student_ids='all', assignment_ids=assignmentIds)
submissions = [Submission(course._requester, raw) for raw in chain.from_iterable(group.submissions for group in groups)]

But clearly that is not how the API is intended to be used.

@Thetwam
Copy link
Member

Thetwam commented Sep 15, 2017

This appears to be (in part) an issue with Canvas. The documentation for the grouped parameter indicates that despite being of type boolean, passing any value at all (even false, 0, etc) will act as if it were set to true

If this argument is present, the response will be grouped by student, rather than a flat array of submissions.

I can change our hardcoded grouped=False to grouped='' to patch this issue. The intention behind adding grouped=False in the first place was to prevent Canvas sending a response format (grouped) that we are currently incapable of parsing. However, this may alter user expectations, so adding some kind of warning to the user that setting grouped to another value isn't permitted seems appropriate.

@Thetwam Thetwam self-assigned this Sep 15, 2017
@Thetwam Thetwam added this to the CanvasAPI v0.7.0 milestone Sep 15, 2017
@Thetwam
Copy link
Member

Thetwam commented Sep 15, 2017

Implemented a similar fix to what I mentioned above in PR #76. Instead of passing grouped='', we just remove the param entirely, and warn the user about what we did.

This fix directly resolves the issue you were having, but does not add functionality to handle results returned in the grouped format. I will create a separate issue for this.

@Thetwam
Copy link
Member

Thetwam commented Sep 15, 2017

New issue is #77

@Thetwam Thetwam closed this as completed Oct 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants