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

Issue/636 duplicate assignment #637

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

dsavransky
Copy link
Contributor

Proposed Changes

Fixes #636 .

@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Merging #637 (d8a7d90) into develop (02d42cb) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##           develop      #637   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           73        73           
  Lines         3732      3735    +3     
=========================================
+ Hits          3732      3735    +3     
Files Coverage Δ
canvasapi/assignment.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Thetwam Thetwam added the hacktoberfest-accepted Indicates valid PRs for Hacktoberfest that we might not quite be ready to merge in yet. label Oct 30, 2023
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be overthinking it, but if the result_type key is passed, it will return a Quiz object rather than an Assignment. I'm not that type-safe when writing, but this could be a case where someone runs into issues. @Thetwam is it worth adding a test?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch. Yeah we'll want to handle that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @bennettscience and @Thetwam. Happy to make changes, but I could use a little guidance. I've tested this out on my university's Canvas installation, and I can get two qualitatively different outputs if I pass the result_type="Quiz" kwarg. However, in each case, the duplicated assignment on Canvas is just an ordinary assignment - the only thing that's different are the keywords in the response json that's generated. Similarly, I can pass either of these outputs into an Assignment object or into a Quiz object, with no issues either way. I also get failures when trying to duplicate anything other than a regular assignment (e.g. a quiz or an external tool assignment).

So: what exactly do you want to happen here? Do you want Assignment.duplicate to check the format of the returned json and return a Quiz object in the case where result_type="Quiz", or should it still always return an Assignment object regardless of inputs? In the latter case, maybe I'd need to manually set the is_quiz_assignment attribute?

I'm struggling a bit, as I don't really understand the utility or intent of the result_type functionality in the original API, so any help would be greatly appreciated. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dsavransky Can you see if your instance has the newquizzes_on_quiz_page setting enabled? Re-reading the docs, that setting has to be enabled for the Quiz keyword to serialize it in a Quiz object:

When the root account has the feature ‘newquizzes_on_quiz_page` enabled and this argument is set to “Quiz” the response will be serialized into a quiz format(quizzes);

You shouldn't need to specify the is_quiz_assignment attribute because that's sent from Canvas. Maybe that's a better indicator?

I'm not sure what the intent was on Instructure's side other than to separate out New Quiz functions from Classic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have admin access to the Canvas instance I'm using for testing, so I'm not actually sure how to check that setting (open to suggestions). Here's what I observe: when I omit the result_type keyword, the JSON that's returned includes the is_quiz_assignment key. When I set result_type="Quiz", that key is not in the returned JSON. Instead, it includes: 'quiz_type': 'quizzes.next'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bennettscience @Thetwam I'd like to eventually get this merged in if possible - could you please provide any guidance on your preferred solution here?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch. Yeah we'll want to handle that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Indicates valid PRs for Hacktoberfest that we might not quite be ready to merge in yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate Assignment
3 participants