Skip to content

Auth converting to new course_id syntax #1926

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.

Sign up for GitHub

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

Closed
dmitchell wants to merge 1 commit into from
Closed

Auth converting to new course_id syntax #1926

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 34 additions & 11 deletions lms/djangoapps/courseware/roles.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
from django.contrib.auth.models import User, Group

from xmodule.modulestore import Location
from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError
from xmodule.modulestore.django import loc_mapper
from xmodule.modulestore.locator import CourseLocator


class CourseContextRequired(Exception):
Expand Down Expand Up @@ -134,20 +137,40 @@ class CourseRole(GroupBasedRole):
A named role in a particular course
"""
def __init__(self, role, location, course_context=None):
# pylint: disable=no-member
loc = Location(location)
legacy_group_name = '{0}_{1}'.format(role, loc.course)

if loc.category.lower() == 'course':
course_id = loc.course_id
else:
# TODO: figure out how to make the group name generation lazy so it doesn't force the
# loc mapping?
if not hasattr(location, 'course_id'):
location = Location(location)

@cpennington cpennington Dec 12, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add a comment about what the possible types of location are? It's not clear that you're doing this because location might be a BlockLocator, might be a Location compatible thing, or might be an actual Location

# direct copy from auth.authz.get_all_course_role_groupnames will refactor to one impl asap
groupnames = []
try:
groupnames.append('{0}_{1}'.format(role, location.course_id))
except InvalidLocationError: # will occur on old locations where location is not of category course
if course_context is None:
raise CourseContextRequired()
course_id = course_context
else:
groupnames.append('{0}_{1}'.format(role, course_context))

group_name = '{0}_{1}'.format(role, course_id)

super(CourseRole, self).__init__([group_name, legacy_group_name])
# pylint: disable=no-member
if isinstance(location, Location):
# least preferred legacy role_course format
groupnames.append('{0}_{1}'.format(role, location.course))
try:
locator = loc_mapper().translate_location(location.course_id, location, False, False)
groupnames.append('{0}_{1}'.format(role, locator.course_id))
except (InvalidLocationError, ItemNotFoundError):
# if it's never been mapped, the auth won't be via the Locator syntax
pass

@cpennington cpennington Dec 12, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is the order of the groupnames different in this case than in the other? In particular, the preferred groups should be earlier in the list.

@dmitchell dmitchell Dec 12, 2013

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The old code had the least preferred (legacy) first.

This has the most preferred (L 147 & L 152), then the alts with the one not requiring any db access before the preferred one requiring db access. I can swap the order of L 157 & 158ff if you want. If we go to a generator, the order I coded would make more sense, but idk if it makes sense to go to a generator nor whether it'd follow this code (idk how yield tracks where it left off).

elif isinstance(location, CourseLocator):
# handle old Location syntax
old_location = loc_mapper().translate_locator_to_location(location, get_course=True)
if old_location:
# the slashified version of the course_id (myu/mycourse/myrun)
groupnames.append('{0}_{1}'.format(role, old_location.course_id))
# add the least desirable but sometimes occurring format.
groupnames.append('{0}_{1}'.format(role, old_location.course))

super(CourseRole, self).__init__(groupnames)


class OrgRole(GroupBasedRole):
Expand Down