Include non-obsolete location info in student profile report by regisb · Pull Request #11620 · openedx/openedx-platform

regisb · 2016-02-22T16:57:32Z

The student profile report that can be downloaded from the course staff
dashboard included the "location" field (which is obsolete) and the
mailing address, which is seldom completed. Here, we add the "country"
and "city" fields to the csv report. Note that this breaks compatibility
with earlier reports.

openedx-webhooks · 2016-02-22T16:57:42Z

Thanks for the pull request, @regisb! I've created OSPR-1155 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

supporting documentation
edx-code email threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

nedbat · 2016-02-22T18:01:06Z

@regisb thanks, but it looks like some Python test failures are due to your changes: Country(code=None) is not JSON serializable

regisb · 2016-02-23T11:01:10Z

@nedbat Hmmmm... the fact that the we need JSON-dumpable data makes this PR more complex than I thought. It's probably the reason why the country code was not sent in the profile report in the first place :-)
Is this feature acceptable, despite the added complexity?

nedbat · 2016-02-23T11:30:28Z

@regisb I'll leave you to work that out with the T&L team.

jbarciauskas · 2016-02-23T15:28:09Z

What does "break compatibility" mean in this context @regisb ?

regisb · 2016-02-23T15:32:08Z

@jbarciauskas If people have written scripts that parse the user reports, e.g: Excel macros, then these scripts will probably fail because of the added columns.

pdpinch · 2016-02-23T18:43:41Z

Ironically, I just had a user ask about this today.

Can you add some examples of before and after reports?

FYI @JoeMartis

regisb · 2016-02-24T08:50:14Z

@pdpinch In the attached screenshot, I generated the report for a course with one student coming from South Georgia and the South Sandwich Islands (code=GS).

And here is a screenshot of what is displayed when we click the "List enrolled students' profile information" button in the course dashboard.

JoeMartis · 2016-02-24T14:13:03Z

+1 for this PR - it would really help with our OFAC efforts.

jbarciauskas · 2016-03-04T15:34:09Z

FYI @scottrish is going to shop this around for feedback on the backwards compatibility concerns

jbarciauskas · 2016-03-30T17:09:13Z

@regisb would it be possible to reorder this so that we don't move the "goals" position, but are just strictly adding columns to the end?

Sure. But note that this query_features array does not determine the order of the columns in the Excel document. The values of PROFILE_FEATURES in instructor_analytics/basic.py do.

regisb · 2016-03-31T11:54:05Z

@jbarciauskas do you wish to move the city/country fields at the end of the excel document?

jbarciauskas · 2016-03-31T13:03:41Z

@regisb Yeah I think that would address the most likely compatibility issues, right?

regisb · 2016-03-31T13:07:31Z

@jbarciauskas Right. I'll make the change.

scottrish · 2016-04-20T12:02:01Z

@regisb Where does the city field get populated from?

cahrens · 2016-05-09T17:33:58Z

I am asking for one more test case to cover when city and country are not present. Otherwise, I am 👍 on this PR.

@catong, I think we should wait to merge this until https://openedx.atlassian.net/browse/DOC-2946 is ready, since this change will need to be flagged in the release for compatibility reasons. We can coordinate when this one is ready to merge.

regisb · 2016-05-10T07:21:56Z

@cahrens I agree that the inconsistency between "None" and "" is a bit annoying. None is a python-specific value, so IMHO it doesn't make much sense to include it in a csv file. If we need to keep this value for backwards compatibility reasons, it should be relatively easy to print undefined countries as "None" by patching the extract_attr function. I can fix this behaviour either way, just let me know if I should make changes or create a different PR.

I'll also be adding a test for users with no city/country.

cahrens · 2016-05-10T14:53:58Z

@regisb My team discussed "None" vs. "", and decided we are fine with the current implementation (inconsistency between City and Country). So no further work is needed on that end.

cahrens · 2016-05-10T14:54:41Z

Nit: I think you could just remove this block and rely on the default behavior of the fields.

cahrens · 2016-05-10T15:00:48Z

@regisb Thanks for the additional test point. It could be simplified somewhat, but it's up to you whether or not you wish to do that. Just let me know if you'd like to make any more changes before I merge.

regisb · 2016-05-10T15:07:36Z

@cahrens: Your suggestions do make sense. I took them into account and updated the PR.

The student profile report that can be downloaded from the course staff dashboard included the "location" field (which is obsolete) and the mailing address, which is seldom completed. Here, we add the "country" and "city" fields to the csv report. To do so, we need to be able to dump the new fields to JSON so we convert the user fields to unicode when needed. Note that this breaks compatibility with earlier reports.

cahrens · 2016-05-10T19:06:56Z

jenkins run js

regisb · 2016-05-11T13:51:52Z

Thanks everyone, great job!

cahrens · 2016-05-11T13:52:51Z

Yes, thanks for the submission, @regisb!

cahrens · 2016-05-13T21:16:00Z

+        'country': _('Country'),
    }

    if is_course_cohorted(course.id):


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.

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

What's the issue exactly?

cahrens · 2016-05-14T10:12:11Z

I described it above-- City and Country will not be the last two columns if either cohorts or teams are enabled. But those are optional columns, so it's a bit funky. I need to talk with folks here on Monday about what is desired.

regisb · 2016-05-14T10:27:07Z

Ok if necessary I'll have time to work on a new pr on Tuesday.

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U needs triage labels Feb 22, 2016

openedx-webhooks added community manager review waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage community manager review labels Feb 22, 2016

regisb force-pushed the regisb/include_geo_data_in_student_profile_report branch from d88e573 to df50aee Compare February 23, 2016 10:58

openedx-webhooks added product review PR requires product review before merging and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Feb 23, 2016

regisb force-pushed the regisb/include_geo_data_in_student_profile_report branch from df50aee to 9da1d59 Compare February 23, 2016 12:34

jbarciauskas reviewed Mar 30, 2016
View reviewed changes

regisb force-pushed the regisb/include_geo_data_in_student_profile_report branch from 9da1d59 to ca93e7c Compare March 31, 2016 11:53

regisb force-pushed the regisb/include_geo_data_in_student_profile_report branch 2 times, most recently from 3b1e1dc to b5fc703 Compare March 31, 2016 14:34

openedx-webhooks added open edx community review and removed product review PR requires product review before merging labels Apr 20, 2016

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed engineering review labels May 9, 2016

regisb force-pushed the regisb/include_geo_data_in_student_profile_report branch from 51d7c30 to aa9d9f8 Compare May 10, 2016 09:43

openedx-webhooks added engineering review and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels May 10, 2016

cahrens reviewed May 10, 2016
View reviewed changes

regisb force-pushed the regisb/include_geo_data_in_student_profile_report branch from aa9d9f8 to 0dea135 Compare May 10, 2016 15:06

regisb force-pushed the regisb/include_geo_data_in_student_profile_report branch from 0dea135 to 704b5ac Compare May 10, 2016 15:45

regisb force-pushed the regisb/include_geo_data_in_student_profile_report branch from 704b5ac to fe41e50 Compare May 10, 2016 16:25

cahrens merged commit 8340703 into openedx:master May 11, 2016

regisb deleted the regisb/include_geo_data_in_student_profile_report branch May 11, 2016 13:45

cahrens reviewed May 13, 2016
View reviewed changes

cahrens mentioned this pull request May 16, 2016

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

Merged

baby636 mentioned this pull request Nov 5, 2022

[Snyk] Fix for 1 vulnerabilities baby636/edx-platform#33

Open

baby636 mentioned this pull request Dec 26, 2022

[Snyk] Fix for 1 vulnerabilities baby636/edx-platform#41

Open

sumodgeorge mentioned this pull request Nov 28, 2023

[Snyk] Fix for 6 vulnerabilities sumodgeorge/edx-platform#15

Open

sumodgeorge mentioned this pull request Dec 20, 2023

[Snyk] Fix for 2 vulnerabilities sumodgeorge/edx-platform#17

Open

sumodgeorge mentioned this pull request May 14, 2024

[Snyk] Fix for 2 vulnerabilities sumodgeorge/edx-platform#33

Open

Uh oh!

Conversation

regisb commented Feb 22, 2016

Uh oh!

openedx-webhooks commented Feb 22, 2016

Uh oh!

nedbat commented Feb 22, 2016

Uh oh!

regisb commented Feb 23, 2016

Uh oh!

nedbat commented Feb 23, 2016

Uh oh!

jbarciauskas commented Feb 23, 2016

Uh oh!

regisb commented Feb 23, 2016

Uh oh!

pdpinch commented Feb 23, 2016

Uh oh!

regisb commented Feb 24, 2016

Uh oh!

JoeMartis commented Feb 24, 2016

Uh oh!

jbarciauskas commented Mar 4, 2016

Uh oh!

jbarciauskas Mar 30, 2016

Choose a reason for hiding this comment

Uh oh!

regisb Mar 31, 2016

Choose a reason for hiding this comment

Uh oh!

regisb commented Mar 31, 2016

Uh oh!

jbarciauskas commented Mar 31, 2016

Uh oh!

regisb commented Mar 31, 2016

Uh oh!

scottrish commented Apr 20, 2016

Uh oh!

cahrens commented May 9, 2016

Uh oh!

regisb commented May 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cahrens commented May 10, 2016

Uh oh!

cahrens May 10, 2016

Choose a reason for hiding this comment

Uh oh!

cahrens commented May 10, 2016

Uh oh!

regisb commented May 10, 2016

Uh oh!

cahrens commented May 10, 2016

Uh oh!

regisb commented May 11, 2016

Uh oh!

cahrens commented May 11, 2016

Uh oh!

cahrens May 13, 2016

Choose a reason for hiding this comment

Uh oh!

catong May 13, 2016

Choose a reason for hiding this comment

Uh oh!

regisb May 14, 2016

Choose a reason for hiding this comment

Uh oh!

cahrens commented May 14, 2016

Uh oh!

regisb commented May 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

regisb commented May 10, 2016 •

edited

Loading