-
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
MA-199 Course Authorization framework in mobile API #6398
Conversation
6835837
to
41ba3b2
Compare
# check enrollment | ||
( | ||
CourseEnrollment.is_enrolled(user, course.id) or | ||
_has_staff_access_to_descriptor(user, course, 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.
I wonder if there's a way we can make the names more obvious. Since it's not just enrollment, it's also beta users though IIRC I had the same problem and didn't come up with anything good.
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 can_load_mobile
method checks enrollment in addition to the other checks.
The can_load_mobile_not_enrolled
method does not check enrollment, but checks release date, beta user, and eventually cohorted content (once supported).
How about can_load_mobile
and can_load_mobile_no_enrollment_check
?
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.
That sounds good to me.
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.
Renamed load_mobile_not_enrolled -> load_mobile_no_enrollment_check in latest commit.
That's all from me. |
from courseware.tests.factories import UserFactory | ||
from student import auth | ||
from student.models import CourseEnrollment | ||
|
||
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase | ||
from xmodule.modulestore.tests.factories import CourseFactory |
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.
Duplicate imports.
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.
Oops! Thanks. I'm surprised pylint doesn't raise this issue.
@BenjiLee @aleffert I believe I have addressed all your comments. Please see the latest commit edx/edx-platform@a221727. |
from xmodule.modulestore.django import modulestore | ||
from static_replace import make_static_urls_absolute, replace_static_urls | ||
|
||
from ..utils import MobileView, mobile_course_access | ||
|
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.
More unused imports:
from rest_framework import permissions
from rest_framework.authentication import OAuth2Authentication, SessionAuthentication
from opaque_keys.edx.keys import CourseKey
from xmodule.modulestore.django import modulestore
👍 |
self.client.login(username=self.username, password=self.password) | ||
|
||
def logout(self): | ||
"""Login test 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.
Logout*
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.
fixed.
👍 |
bf7f076
to
63412f8
Compare
63412f8
to
5349b55
Compare
MA-199 Course Authorization framework in mobile API
Please review. @aleffert @BenjiLee