Skip to content

Include non-obsolete location info in student profile report #11620

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

Merged
cahrens merged 1 commit into from
May 11, 2016
Merged

Include non-obsolete location info in student profile report #11620

Show file tree
Hide file tree
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
4 changes: 3 additions & 1 deletion lms/djangoapps/instructor/views/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1246,7 +1246,7 @@ def get_students_features(request, course_id, csv=False): # pylint: disable=red
query_features = [
'id', 'username', 'name', 'email', 'language', 'location',
'year_of_birth', 'gender', 'level_of_education', 'mailing_address',
'goals'
'goals', 'city', 'country'
]

# Provide human-friendly and translatable names for these features. These names
Expand All @@ -1264,6 +1264,8 @@ def get_students_features(request, course_id, csv=False): # pylint: disable=red
'level_of_education': _('Level of Education'),
'mailing_address': _('Mailing Address'),
'goals': _('Goals'),
'city': _('City'),
'country': _('Country'),
}

if is_course_cohorted(course.id):

@cahrens cahrens May 13, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bummer, I didn't expand below this to notice that if the course is cohorted, the cohorts column is added to the end (and the same with teams). Therefore, City and Country will not be the last two columns in the spreadsheet if either cohorts or teams are present.

We may need to revert this PR for the release. I will follow up with product.

@catong catong May 13, 2016

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.

Thanks @cahrens. Please tag me on any changes so I can reflect updates in the doc.

@regisb regisb May 14, 2016

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.

What's the issue exactly?

Expand Down
17 changes: 14 additions & 3 deletions lms/djangoapps/instructor_analytics/basic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from django.conf import settings
from django.contrib.auth.models import User
from django.core.exceptions import ObjectDoesNotExist
from django.core.serializers.json import DjangoJSONEncoder
from django.core.urlresolvers import reverse
from opaque_keys.edx.keys import UsageKey
import xmodule.graders as xmgraders
Expand All @@ -27,7 +28,8 @@

STUDENT_FEATURES = ('id', 'username', 'first_name', 'last_name', 'is_staff', 'email')
PROFILE_FEATURES = ('name', 'language', 'location', 'year_of_birth', 'gender',
'level_of_education', 'mailing_address', 'goals', 'meta')
'level_of_education', 'mailing_address', 'goals', 'meta',
'city', 'country')
ORDER_ITEM_FEATURES = ('list_price', 'unit_cost', 'status')
ORDER_FEATURES = ('purchase_time',)

Expand Down Expand Up @@ -222,6 +224,15 @@ def enrolled_students_features(course_key, features):
if include_team_column:
students = students.prefetch_related('teams')

def extract_attr(student, feature):

@cahrens cahrens May 9, 2016 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@regisb Do you know why this was not previously needed for "location" and "mailing_address", which aren't actually filled out (why didn't they throw an error also)? In particular, mailing_address is defined in an identical way as city.

mailing_address = models.TextField(blank=True, null=True)
city = models.TextField(blank=True, null=True)
country = CountryField(blank=True, null=True)

Looking back at the history of this PR, it appears that country (which is using CountryField) was the only thing having an issue.

@regisb regisb May 9, 2016

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.

This is because 'country' is not a string: it's an instance of CountryField. When we try to serialize student_dict as JSON, serialization will fail for the values that do not have an acceptable type (int, str, list, etc.). That piece of code with DjangoJSONEncoder was inspired by the JSON-conversion code from django models.

"""Evaluate a student attribute that is ready for JSON serialization"""
attr = getattr(student, feature)
try:
DjangoJSONEncoder().default(attr)
return attr
except TypeError:
return unicode(attr)

def extract_student(student, features):
""" convert student to dictionary """
student_features = [x for x in STUDENT_FEATURES if x in features]
Expand All @@ -236,11 +247,11 @@ def extract_student(student, features):
meta_key = feature.split('.')[1]
meta_features.append((feature, meta_key))

student_dict = dict((feature, getattr(student, feature))
student_dict = dict((feature, extract_attr(student, feature))
for feature in student_features)
profile = student.profile
if profile is not None:
profile_dict = dict((feature, getattr(profile, feature))
profile_dict = dict((feature, extract_attr(profile, feature))
for feature in profile_features)
student_dict.update(profile_dict)

Expand Down
28 changes: 23 additions & 5 deletions lms/djangoapps/instructor_analytics/tests/test_basic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,17 +106,35 @@ def test_enrolled_students_features_username(self):
self.assertIn(userreport['username'], [user.username for user in self.users])

def test_enrolled_students_features_keys(self):
query_features = ('username', 'name', 'email')
query_features = ('username', 'name', 'email', 'city', 'country',)
for user in self.users:

@cahrens cahrens May 9, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

One more request-- please include a user without city and country so that we can verify the code works when those are not specified (assert the expected values).

@regisb regisb May 10, 2016

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.

Done below.

user.profile.city = "Mos Eisley {}".format(user.id)
user.profile.country = "Tatooine {}".format(user.id)
user.profile.save()
for feature in query_features:
self.assertIn(feature, AVAILABLE_FEATURES)
with self.assertNumQueries(1):
userreports = enrolled_students_features(self.course_key, query_features)
self.assertEqual(len(userreports), len(self.users))
for userreport in userreports:

userreports = sorted(userreports, key=lambda u: u["username"])
users = sorted(self.users, key=lambda u: u.username)
for userreport, user in zip(userreports, users):
self.assertEqual(set(userreport.keys()), set(query_features))
self.assertIn(userreport['username'], [user.username for user in self.users])
self.assertIn(userreport['email'], [user.email for user in self.users])
self.assertIn(userreport['name'], [user.profile.name for user in self.users])
self.assertEqual(userreport['username'], user.username)
self.assertEqual(userreport['email'], user.email)
self.assertEqual(userreport['name'], user.profile.name)
self.assertEqual(userreport['city'], user.profile.city)
self.assertEqual(userreport['country'], user.profile.country)

def test_enrolled_student_with_no_country_city(self):
userreports = enrolled_students_features(self.course_key, ('username', 'city', 'country',))
for userreport in userreports:
# This behaviour is somewhat inconsistent: None string fields
# objects are converted to "None", but non-JSON serializable fields
# are converted to an empty string.
self.assertEqual(userreport['city'], "None")
self.assertEqual(userreport['country'], "")

def test_enrolled_students_meta_features_keys(self):
"""
Expand Down