Move city and country fields to the very end.

[cahrens]

OSPR-1155

@dianakhuang and @efischer19 please review. Here is a sandbox with teams and cohorts both enabled on the demo course: https://reportfix.sandbox.edx.org/courses/course-v1:edX+DemoX+Demo_Course/instructor#view-data_download

FYI @doctoryes and @regisb

[cahrens]

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]

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

[efischer19]

👍 Everything looks okay to me.

Worth noting - we had the None versus <empty string> discussion on the original PR, and with team/cohort enabled there's also the possibility of [unassigned] and [unavailable]. Should we make a ticket about standardizing the "no data available" placeholder on this report? This is still something that existed prior to this PR so there's no need to block this merge, but wanted to raise the point.

[dianakhuang]

👍

[cahrens]

@efischer19 Hah! I hadn't noticed that, but it is true that pretty much every column has its own special way of indicating "unset". I agree it would be nice to standardize, but then it is a compatibility issue-- something to consider though.

[cahrens]

@efischer19's screen shot scared me because City and Country were both showing as empty in the CSV (I had only tried the "List profile information inline" option). However, restarting the celery workers fixed the issue.

[catong]

@cahrens Under what circumstances might City show with value of "None" in some cases and blank in others?

[cahrens]

@catong This must be an issue with funky user data on stage (note the same pattern for other fields like "Level of Education" and "Goals"). It should always be "None".

[catong]

OK, thanks for confirming.