[BB-2059] Remove deprecated city field from student features report

[kaizoku]

This PR updates the student features report to pull from UserProfile.meta['city'] if it is set, in favor over UserProfile.city.

Context:
There are two distinct ways to store a city for users in edx-platform: one directly in UserProfile.city, and another in UserProfile.meta.

UserProfile.meta is used by adding "city" to the extended_profile_fields. This is then available via the account settings editor. The UserProfile.city field is not used or available via the account settings page.

Conversely to use a City field in the registration form, the REGISTRATION_EXTRA_FIELDS["city"] must be configured, and then entries are stored in UserProfile.city, which can't later be edited from the account settings page since it's not included in the field set.

Since the UserProfile.meta['city'] field is not set unless specifically configured, we choose this over the UserProfile.city field when generating the student features report.

Dependencies: None

Testing instructions:

1. Add "extended_profile_fields":"city" (to the SiteConfiguration)[http://localhost:18000/admin/site_configuration/siteconfiguration/1/change/]
2. Set {"city": "Test City"} in the UserProfile.meta field for a (test user)[http://localhost:18000/admin/auth/user/5/change/].
3. Generate a report of student profiles with the "Download profile information as CSV" button in the instructor view for the edX DemoX Course.
4. Check that the "city" field is filled in from the value set in step Fix js that prevented single step OE problems (ie only self assessment) ... #2.

Reviewers

• (@lgp171188)
• edX reviewer[s] TBD

[openedx-webhooks]

Thanks for the pull request, @kaizoku! I've created OSPR-4114 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

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

• supporting documentation
• Open edX discussion forum 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 be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[lgp171188]

@kaizoku, 👍

• I tested that the city field is now stored in UserProfile.meta.
• I read through the code
• I checked for accessibility issues NA
• Includes documentation

[natabene]

@kaizoku Thank you for your contribution. Please let me know once it is ready for our review.

[giovannicimolin]

@natabene I've added this to the OSPR doc.

[kaizoku]

@natabene thanks! This is ready for review.

[natabene]

@giovannicimolin I am lining this up for our review.

[kaizoku]

@natabene, is this still in the queue for review?

[natabene]

@kaizoku Yes, it is still waiting for a review. We have very little bandwidth right now, so heads up it may take more time.

[kaizoku]

@natabene, understood. Thanks for the update!

[marcotuts]

I'm moving this back to product review - It isn't clear to me what the impact here is between city/country - Can you help me understand what the impact is to the reports with this change?

[kaizoku]

@marcotuts, since the city field is no longer updated in the UserProfile serializer (edx@ae0333c) or from the account settings view, this just removes it from the student profile report.

The side effect of the revert is that country isn't added as the final field of the report. I could add another commit to move country to the final field similar to the what the commit being reverted in this PR does, but still leave out the city field.

[marcotuts]

Sounds good to me.

If the change currently results in the removal of the country field from the report I'd advocate for keeping that so we don't alter any workflow expectations here around that field, but removing the unused city field makes sense to me.

Pushing this past product review again thanks for the detail @kaizoku

[kaizoku]

Thanks for checking into this @marcotuts. I can confirm that this change keeps the country field, since it's still the field in use from the Account page and within the platform.

[natabene]

@bradenmacdonald Please feel free to continue with this as a core committer.

[bradenmacdonald]

@kaizoku From the code alone, it's not obvious why this is being done this way, so can you please:

1. Add a comment to the code itself explaining the context here and/or linking to this PR? (e.g. # There are two separate places where the city value can be stored, one used by account settings and the other used by the registration form. If the account settings value (meta.city) is set, it takes precedence.
2. Rebase and squash to one commit with a conventional commit message.

Since @lgp171188 hasn't approved this fully and I'm not sure if he tested it, I will then do a manual test, approve, and merge.

[arch-bom-gocd-alerts]

📣 💥 Heads-up: You must either rebase onto master or merge master into your branch to avoid breaking the build.

We recently removed diff-quality and introduced lint-amnesty. This means that the automated quality check that has run on your branch doesn't work the same way it will on master. If you have introduced any quality failures, they might pass on the PR but then break the build on master.

This branch has been detected to not have commit 2e33565 as an ancestor. Here's how to see for yourself:

git merge-base --is-ancestor 2e335653 josh/revert_city_student_features_report && echo "You're all set" || echo "Please rebase onto master or merge master to your branch"

If you have any questions, please reach out to the Architecture team (either #edx-shared-architecture on Open edX Slack or #architecture on edX internal).

[kaizoku]

Thanks for the review and suggestions @bradenmacdonald. I used your suggested comment as it's pretty comprehensive for a concise comment. I rebased, squashed, and updated the commit message with some more details from our discussion here and a reference to this PR for all of the details.

I'm not positive if this qualifies as a "fix" or "feat" tag for the conventional commit type. I used "fix" since while it modifies a feature it seems more like a fix.

[edx-status-bot]

Your PR has finished running tests. There were no failures.

[bradenmacdonald]

👍

• I tested this: set profile city, meta city, and both fields for three different users, confirmed that the report reads from both fields but prefers meta.
• I read through the code
• I checked for accessibility issues: n/a
• Includes documentation: now has inline comment, yes

[openedx-webhooks]

@kaizoku 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.