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

Added new function to call canvas service 'List submissions for multiple assignments' from the Submission API #45

Merged

Conversation

Elonso95
Copy link

No description provided.

@bgarciaentornos
Copy link

Hi @buckett, hope you're doing ok.

EDF is working on some LTI tool integrated with Canvas using this library and @Elonso95 has noticed some missing methods/fields. We've told him how to proceed, so he has followed the existing development patterns to make the updates. I think he has added unit tests where existing.

Please review them (#46 is the other one) when you have some time and let us know whatever he has to change to get these merged.

Thanks a lot and regards.

Copy link
Member

@buckett buckett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Canvas API library tries to help the programmer make the right calls without having to constantly refer to the documentation and the Java type system is used to help with this. Having multipurpose objects makes it harder to see what needs to be done.

Comment on lines 63 to 72

/**
* Filter the list of submissions by the given student ids..
* @param ids List of ids to filter submissions by
* @return This object to allow adding more options
*/
public GetSubmissionsOptions studentIds(List<String> ids) {
optionsMap.put("student_ids[]", ids);
return this;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting this in a class called GetMultipleSubmissionsOptions would make more sense as the two calls take very different arguments and normally the Options objects have constructors that take all the required arguments.

https://canvas.instructure.com/doc/api/submissions.html#method.submissions_api.index
https://canvas.instructure.com/doc/api/submissions.html#method.submissions_api.for_students

@@ -62,6 +62,16 @@ public SubmissionImpl(String canvasBaseUrl, Integer apiVersion, OauthToken oauth
final String url = buildCanvasUrl(String.format("sections/%s/assignments/%d/submissions", options.getCanvasId(), options.getAssignmentId()), options.getOptionsMap());
return getListFromCanvas(url);
}

@Override
public List<Submission> getCourseSubmissionMultipleAssignments(final GetSubmissionsOptions options) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should probably take a GetMultipleSubmissionsOptions object.

@Elonso95 Elonso95 force-pushed the retrieve_multiple_assignment_submissions branch 2 times, most recently from 4b823ad to 757b27d Compare April 8, 2022 10:16
@Elonso95 Elonso95 force-pushed the retrieve_multiple_assignment_submissions branch from 757b27d to a055dbc Compare April 19, 2022 07:25
@Elonso95
Copy link
Author

Hi @buckett. We solved the problem in the tests that prevented the maven build.

Hope everything now works!

@buckett
Copy link
Member

buckett commented Apr 21, 2022

Hi @buckett. We solved the problem in the tests that prevented the maven build.

Hope everything now works!

Thanks, getting closer, it's just failing on the JavaDoc generation.

@Elonso95 Elonso95 force-pushed the retrieve_multiple_assignment_submissions branch from a055dbc to 703e166 Compare April 22, 2022 06:56
@Elonso95
Copy link
Author

Thanks @buckett now it all seems correct!

@buckett buckett merged commit dc13e5e into oxctl:master Apr 22, 2022
@buckett
Copy link
Member

buckett commented Apr 22, 2022

Thanks.

@bgarciaentornos
Copy link

Hi @buckett , this was the last update of the library we needed for our current project. Would it be possible to release a new version when possible? Thanks a lot, much appreciated.

@buckett
Copy link
Member

buckett commented Apr 22, 2022

@bgarciaentornos
Copy link

Brill! Thanks again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants