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

New Quizzes #612

Merged
merged 3 commits into from
May 4, 2023
Merged

New Quizzes #612

merged 3 commits into from
May 4, 2023

Conversation

Thetwam
Copy link
Member

@Thetwam Thetwam commented Apr 21, 2023

Proposed Changes

  • Add coverage for New Quizzes endpoints

Note: These endpoints follow a different base URL than most other endpoints (/api/quiz/v1/ instead of /api/v1/)

Fixes #608

@Thetwam Thetwam self-assigned this Apr 21, 2023
@codecov
Copy link

codecov bot commented Apr 21, 2023

Codecov Report

Merging #612 (c4f3052) into develop (0d03d35) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##           develop      #612   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           72        73    +1     
  Lines         3691      3726   +35     
=========================================
+ Hits          3691      3726   +35     
Impacted Files Coverage Δ
canvasapi/course.py 100.00% <100.00%> (ø)
canvasapi/new_quiz.py 100.00% <100.00%> (ø)
canvasapi/paginated_list.py 100.00% <100.00%> (ø)

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

@Thetwam Thetwam marked this pull request as ready for review May 1, 2023 18:20
@Thetwam Thetwam requested a review from jessemcbride as a code owner May 1, 2023 18:20
@Thetwam Thetwam changed the title New Quizzes (WIP) New Quizzes May 1, 2023
@Thetwam Thetwam requested a review from jonespm May 1, 2023 18:20
@bennettscience
Copy link
Contributor

Everything looks good to me. I had to go back and read the docstring for the requester._url param, but I think it's good to go.

Is it worth adding a note to the library docs about how this module works a little differently? Or does it not matter to the user that much so long as it works?

response = self._requester.request(
"POST",
endpoint,
_url=self._requester.original_url + "/api/quiz/v1/" + endpoint,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if instead of defining the_urlon all these calls we could modify requester.py to handle both of these patterns? Would eliminate a little redundancy. Would probably need like

self.base_url = base_url + "/api/v1"
self.new_quizzes_url = base_url + "/api/quiz/v1"

And then in requester.request would have to have some way of determining if it's a new quiz. I feel like there will be more methods so having this a a little cleaner now could be of benefit.

I'm not sure the best way to handle that though. The easy solution might be to re-use url? Just an idea, not sure if it makes it significantly better.

        :param _url: Optional argument to specify the base URL to use for the request.
            If set to "new_quizzes", the `new_quizzes_url` instance variable will be used.
            If set to any other value (including None), the `base_url` instance variable will be used.
            If this is selected and an endpoint is provided, the endpoint will be ignored
            and only the `_url` argument will be used.
        if _url == "new_quizzes":
            base_url = self.new_quizzes_url
        else:
            base_url = self.base_url

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point. The same could be done for the graphql endpoint in the requester.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I thought about that a little when implementing that method but that was only a single call. This is repeated a few times for each endpoint. But that could also be made to use this method.

Copy link
Contributor

Choose a reason for hiding this comment

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

This could also be done on a separate issue just to get this in quicker, but I think it should be improved before future new_quizzes get added.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to merge this in for now, but have spun this comment off into its own issue: #619. Will seek to complete that issue before doing a full release.

Copy link
Contributor

@jonespm jonespm left a comment

Choose a reason for hiding this comment

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

Tested these methods out, look like they're working as expected! Thanks for the addition.

@Thetwam Thetwam added this to the CanvasAPI v3.2.0 milestone May 4, 2023
@Thetwam Thetwam merged commit 2c8be50 into ucfopen:develop May 4, 2023
@Thetwam Thetwam deleted the issue/608-new-quizzes branch May 4, 2023 18:52
Thetwam added a commit that referenced this pull request May 25, 2023
* Allow PaginatedList to use metadata for pagination (#614)

* Update `PaginatedList`

Based on #605, `PaginatedList` could not process requests which return
pagination info in the response body. This update checks for `Link` headers
before checking the `meta` property in the response body.

* Update based on review

* Update pag list test with no header no next meta to run properly. Update changelog.

---------

Co-authored-by: Matthew Emond <me@ucf.edu>

* New Quizzes (#612)

* Begin adding coverage for New Quizzes (WIP)

* Finish New Quizzes endpoints

* Pin urllib3 to <2 for tests due to issue with requests-mock. jamielennox/requests-mock#228

* Course Create Discussion Attach Files #621 (#622)

* Fix issue where Course.create_discussion_topic doesn't accept attachment files

* Add file tests for create_discussion_topic

* Delete method for course and group pages (#624)

* method delete group and course pages, update tests

* format fix

* Add Caitlin to authors. Add contribution to changelog. Formwatting tweaks.

---------

Co-authored-by: Matthew Emond <me@ucf.edu>

* Remove Jesse as codeowner. Add Matthew Jones and Brian Bennett

* Add support for Python 3.11 (#625)

* markdown styling tweak to be in line with prettier's defaults

* Bump version to 3.2.0

---------

Co-authored-by: Brian <brian.bennett2@gmail.com>
Co-authored-by: Caitlin Fabian <89735646+Caitlin-Fabian@users.noreply.github.com>
bennettscience added a commit to bennettscience/canvasapi that referenced this pull request May 10, 2024
Based on ucfopen#612 and as an issue in ucfopen#619, this adds the `new_quizzes` and
`graphql` keyword endpoints in the requester module. Methods which
specify these locations will have the correct URL supplied by the
library in the `_url` parameter for the requester.
bennettscience added a commit to bennettscience/canvasapi that referenced this pull request May 11, 2024
Based on ucfopen#612 and as an issue in ucfopen#619, this adds the `new_quizzes` and
`graphql` keyword endpoints in the requester module. Methods which
specify these locations will have the correct URL supplied by the
library in the `_url` parameter for the requester.

Calls from the `canvas`, `course`, and `new_quizzes` modules have been
updated to use they keywords rather than a formatted URL string.

All tests passing.
Thetwam added a commit that referenced this pull request May 12, 2024
* Use `new_quizzes` and `graphql` keywords

Based on #612 and as an issue in #619, this adds the `new_quizzes` and
`graphql` keyword endpoints in the requester module. Methods which
specify these locations will have the correct URL supplied by the
library in the `_url` parameter for the requester.

Calls from the `canvas`, `course`, and `new_quizzes` modules have been
updated to use they keywords rather than a formatted URL string.

All tests passing.

* Better variable checks

Rearrange to check for `_url` first, then assign the request url more
explicitely.

* fix silly typos

---------

Co-authored-by: Matthew Emond <me@ucf.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Quizzes
3 participants