Skip to content
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.

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

fix: gender field in create_user_gdpr_testing script #34199

Closed

Conversation

jerryankur
Copy link
Contributor

Description

In the create_user_gdpr_testing script, the gender property of the UserProfile was set to 'gdpr test gender', which is more than max_length=6 defined in User Model.

#34142

This PR fixes the issue to assign the correct choice 'o' (out of 'f', 'm', 'o').

@jerryankur jerryankur requested a review from a team as a code owner February 7, 2024 02:44
@openedx-webhooks
Copy link

Thanks for the pull request, @jerryankur! 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 as you can:

  • 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.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Feb 7, 2024
@jerryankur
Copy link
Contributor Author

hi @pomegranited , can you review this as well.

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Feb 7, 2024
@pomegranited
Copy link
Contributor

Hi @jerryankur , I am not sure when I'll be able to review this.. Other core contributors might be able to get to it sooner?

@mphilbrick211 mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Feb 8, 2024
@pomegranited
Copy link
Contributor

Hi @jerryankur , thank you for this fix! It gets around the first error thrown by this management command, but then there is another error.

./manage.py lms create_user_gdpr_testing
# django.core.exceptions.FieldError: Cannot resolve keyword 'catalog' into field.

Could you please fix the rest of these errors, and add a test that validates that this management command runs and creates the expected data? Examples of management command tests in the platform can be found here:

@pomegranited
Copy link
Contributor

Nudge @jerryankur 😃

@justinhynes
Copy link
Contributor

Updated status to waiting on author. Once the requests from @pomegranited are taken care of, we can also try and fast track this for review on our side too.

@pomegranited
Copy link
Contributor

Thanks @justinhynes!

Do you know if this create_user_gdpr_testing management command is even used anymore? It seems unlikely given all the errors it throws, so maybe it's better to remove it than to spend time fixing it?

@jerryankur
Copy link
Contributor Author

@pomegranited thanks for your feedback. i will have a look on it. i was unavailable for a week.

@pomegranited
Copy link
Contributor

No worries at all @jerryankur. It's part of my job to check in here, so I do it :) But it's ok if you don't have time to respond; we can pick up again whenever you do. Thank you for your time!

@justinhynes
Copy link
Contributor

Thanks @justinhynes!

Do you know if this create_user_gdpr_testing management command is even used anymore? It seems unlikely given all the errors it throws, so maybe it's better to remove it than to spend time fixing it?

Unfortunately, I don't. Our team only recently has picked up maintainer duties for the user_api, and I'm not very familiar with this functionality. If fixing this results in too much time spinning wheels, I think it is totally sane to create a public issue (and deferring the fix for this management command) and/or moving towards deprecation/removal.

@obscherler
Copy link

Do you know if this create_user_gdpr_testing management command is even used anymore? It seems unlikely given all the errors it throws, so maybe it's better to remove it than to spend time fixing it?

I don’t know. Do you think GDPR compliant is something unimportant for Open edX to be? Do you think the user retirement pipeline is so well documented and so trivial to set up that it doesn’t need testing?

I’ve spent the week trying to set up retirement. The Tutor plugin has out of order steps which cause the LMS to return Bad Request errors with no explanation, it has irrelevant steps, the ecommerce is missing the user retirement API endpoint, and the LMS is missing the retire_mailings endpoint. I’ve already exhausted 21 GDPR test users, very glad I didn’t have to register those manually and only had to fix the create_user_gdpr_testing command (which involved changing the gender to o like @jerryankur did, removing the aforementioned catalog property, and for me, changing the course it enrols the test user into.)

@pomegranited
Copy link
Contributor

@obscherler

Do you think GDPR compliant is something unimportant for Open edX to be?

No, I think it's important for Open edX to continue with its GDPR compliance efforts.

Do you think the user retirement pipeline is so well documented and so trivial to set up that it doesn’t need testing?

I wish this were the case, but no.. retirement pipeline isn't easy to set up, and I don't know how accurate the docs are anymore.

But glad to hear this management command was useful for your testing, so we should keep it :)

Do you want to submit your fixes to a new PR, and we can close this one?

@obscherler
Copy link

I’ll try doing that in the next few days.

I’ve finally managed to get the pipeline to run with the services we use, except for ecommerce, where I can’t find a trace of the /api/v2/user/retire endpoint. Where would be the best place to discuss that? The ecommerce repo doesn’t have issues, and is being deprecated (however I don’t know the timeline,) the tubular repo doesn’t accept bug reports, and is being deprecated as well. Should I try in edx-platform, where the retirement scripts are being moved? Thanks.

@jsnwesson jsnwesson added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. needs maintainer attention Issue or PR specifically needs the attention of the maintainer. labels Mar 5, 2024
Copy link
Member

@deborahgu deborahgu left a comment

Choose a reason for hiding this comment

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

Just marking as changes requested in the comment here by @pomegranited,

@deborahgu
Copy link
Member

I’ve finally managed to get the pipeline to run with the services we use, except for ecommerce, where I can’t find a trace of the /api/v2/user/retire endpoint. Where would be the best place to discuss that? The ecommerce repo doesn’t have issues, and is being deprecated (however I don’t know the timeline,) the tubular repo doesn’t accept bug reports, and is being deprecated as well. Should I try in edx-platform, where the retirement scripts are being moved? Thanks.

@obscherler, I'd suggest asking on slack, since this seems more like a request for assistance getting the pipeline running than a clearly defined bug report.

@pomegranited
Copy link
Contributor

@obscherler

I’ll try doing that in the next few days.

Thank you, and no rush. :)

I’ve finally managed to get the pipeline to run with the services we use, except for ecommerce, where I can’t find a trace of the /api/v2/user/retire endpoint. Where would be the best place to discuss that?

Does this comment on extensions.api.v2.retirement.EcommerceIdView provide any clues? Otherwise I agree with @deborahgu , best to ask on Slack.

Specifically this is used to retire users identified by "ecommerce-{id}" from Segment.

edX uses Segment for a lot of things, but I don't think much of this made it out to the community. The tubular script references this endpoint too, so maybe this script's reference to /api/v2/user/retire never actually worked?

But ya, the edx-platform migration for these scripts is probably the best place to ask questions, e.g on this Slack thread.

@pomegranited
Copy link
Contributor

Hi @jerryankur and @obscherler , it's been a while since there's been any action on this PR, so I'm going to close it out.
But feel free to open a new one and ping me if you get time to work on this.

@openedx-webhooks
Copy link

@jerryankur Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs maintainer attention Issue or PR specifically needs the attention of the maintainer. open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants