Skip to content

Move city and country fields to the very end. #12475

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 16, 2016
Merged

Move city and country fields to the very end. #12475

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
5 changes: 5 additions & 0 deletions lms/djangoapps/instructor/tests/test_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2519,6 +2519,9 @@ def test_get_students_features(self):
Test that some minimum of information is formatted
correctly in the response to get_students_features.
"""
for student in self.students:

@cahrens cahrens May 16, 2016 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

When I was making the code modification below, I first deleted city and country and then ran the test modified in the original PR (#11620). I was disappointed to see that no tests failed because the method being tested wasn't the "get_students_features" method-- it was instead a utility method to convert the value to something that could be returned to get_students_features. Therefore I added this test as well (verifying that it fails if City and Country aren't added in get_students_features).

Note though that this test is going through the JSON (non-CSV) code path, and therefore I can't test that City and Country are at the end. I don't see any existing tests that are generating the CSV file and looking at its contents, probably because of the complication with the celery instructor task-- there is only coverage that the instructor task is kicked off via a mock. I don't think it is worth tackling that sort of integration test, but can reconsider if reviewers disagree.

@efischer19 efischer19 May 16, 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.

I agree with the decision that it's not worth tackling that sort of integration test.

student.profile.city = "Mos Eisley {}".format(student.id)
student.profile.save()

@dianakhuang dianakhuang May 16, 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.

Why are we adding a city but not a country?

@cahrens cahrens May 16, 2016 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

In this test, I decided to have country use the default value (when nothing has been set). There are more complete tests of the utility method ultimately called by get_students_features in https://github.com/edx/edx-platform/pull/11620/files#diff-4b0f77beb25859a66087d2500f498d5aL109

url = reverse('get_students_features', kwargs={'course_id': self.course.id.to_deprecated_string()})
response = self.client.get(url, {})
res_json = json.loads(response.content)
Expand All @@ -2530,6 +2533,8 @@ def test_get_students_features(self):
][0]
self.assertEqual(student_json['username'], student.username)
self.assertEqual(student_json['email'], student.email)
self.assertEqual(student_json['city'], student.profile.city)
self.assertEqual(student_json['country'], "")

@ddt.data(True, False)
def test_get_students_features_cohorted(self, is_cohorted):
Expand Down
10 changes: 7 additions & 3 deletions 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', 'city', 'country'
'goals',
]

# Provide human-friendly and translatable names for these features. These names
Expand All @@ -1264,8 +1264,6 @@ 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):
Expand All @@ -1277,6 +1275,12 @@ def get_students_features(request, course_id, csv=False): # pylint: disable=red
query_features.append('team')
query_features_names['team'] = _('Team')

# For compatibility reasons, city and country should always appear last.
query_features.append('city')
query_features_names['city'] = _('City')
query_features.append('country')
query_features_names['country'] = _('Country')

if not csv:
student_data = instructor_analytics.basic.enrolled_students_features(course_key, query_features)
response_payload = {
Expand Down