-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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 Blocks API #10411
Course Blocks API #10411
Conversation
4261c40
to
a00e453
Compare
""" | ||
Validates and returns the requested_user, while checking permissions. | ||
""" | ||
requested_username = cleaned_data.get('user', '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If user
is required, does this need the fallback string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not necessary, but given the TODO note on the user field above, I'm inclined to leave it in place to make later modifications easier.
a00e453
to
d5843c7
Compare
|
||
**Example requests**: | ||
|
||
GET /api/courses/v1/blocks/?course_id=<course_id> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same as the endpoint below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The endpoint below includes all the indented lines prefixed with &
, so they're not the same, unless I'm missing what you mean by "below."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the same endpoint as far as the baseurl goes, but it just includes more parameters.
456f00e
to
8881c30
Compare
|
||
|
||
# pylint: disable=no-member | ||
class TestBlocksView(TestBlocksViewMixin, SharedModuleStoreTestCase): # pylint: disable=missing-docstring |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this pylint disable still needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nerp. Removed.
@jcdyer I've completed my 1st pass. Sorry for putting some of my comments on the commit itself (especially the 1st commit). They'll get lost once we squash, but as long as we go through them beforehand, it should be fine. |
|
||
|
||
@view_auth_classes() | ||
class BlocksView(DeveloperErrorViewMixin, ListAPIView): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Docstrings to 80 chars
Done with 1st pass. |
Done with pass of fixups. |
069076b
to
4242b0e
Compare
5ff4822
to
a5be9d4
Compare
2d77a66
to
1439b71
Compare
b331497
to
2f75481
Compare
👍 |
2f75481
to
2a68049
Compare
jenkins run quality |
1 similar comment
jenkins run quality |
2a68049
to
46523b1
Compare
👍 |
Reviewers: @BenjiLee @jcdyer
Proctored Exam commit: @chrisndodge
FYI @aleffert