[SE-3405] Django app to allow user retirement via API - #25800
Conversation
|
Thanks for the pull request, @pkulkark! I've created OSPR-5290 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:
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. |
7a56fb7 to
1da08d0
Compare
|
@pkulkark Thank you for your contribution. Please let me know once this is ready for our review. |
1da08d0 to
b22471d
Compare
mavidser
left a comment
There was a problem hiding this comment.
LGTM 👍
- I tested this: I retired existing users (single & multiple) and tested error cases.
- I read through the code
- Includes documentation
|
@natabene This should be ready for edx's review now |
bradenmacdonald
left a comment
There was a problem hiding this comment.
Not sure if I'll be doing the technical review here or not, once it gets to that point, but I had some preliminary questions and comments.
There was a problem hiding this comment.
Do we need this app in Studio? I feel like having it only in the LMS would be sufficient, but I might be missing something.
There was a problem hiding this comment.
Since the user accounts are shared with Studio, I thought it should be available there too. Please let me know if that's not required.
There was a problem hiding this comment.
I don't know the answer to whether or not it needs to be available in the CMS as well as the LMS. But if Studio starts up/operates fine without this app being included, it should be removed.
@jmbowman In what conditions would a Django app need to be required in both the LMS and CMS? When it defines 1+ Django models? Some other condition? Does this app need to be included in the CMS here?
There was a problem hiding this comment.
My impression is that this whole workflow is usually called just "user retirement" and that GDPR compliance is one, but not necessarily the only, use case for the user retirement flow. Is there a specific reason to call this new django app "GDPR user retirement" vs. "user retirement" or "user retirement REST API" ?
There was a problem hiding this comment.
There's no specific reason. I just went with that in order to differentiate from the existing retirement related code. Also noticed GDPR usually being mentioned alongside user retirement: like here for example.
There was a problem hiding this comment.
I would prefer that this Django app (and feature) be called bulk_user_retirement without specifically referring to the GDPR. That name more accurately reflects the feature - and makes no implied claims about GDPR compliance.
There was a problem hiding this comment.
Thank you. That's a great suggestion. I've changed it to bulk_user_retirement and removed the GDPR reference everywhere.
There was a problem hiding this comment.
Nit: it's confusing that this variable is called request_body, because it's not the request body, it's a specific request parameter value, which was decoded from the request body.
There was a problem hiding this comment.
Makes sense. I changed it to request_usernames.
bbbeadf to
3259085
Compare
(cherry picked from commit b22471d6760aa8e874e947de15144d2262696fd8)
|
@pkulkark Thanks for addressing the review; sorry I haven't re-reviewed yet. Will do so tomorrow. |
|
Actually from Jira it looks like @macdiesel will be reviewing this, which is great as I'm not super familiar with user retirement. |
|
Hey all, so we deliberately did not write this feature as we wanted to ensure that retirements could only be executed by the users requesting them. Thank you for the submission but I'm inclined to close this PR. I'm curious though, what is the use case here? Is this a use case edX needs? |
|
@macdiesel I'll let @pkulkark clarify the use case when she can, but I think it could be this: Say you're a company that offers a suite of services to your customer - they enroll in your portal, and then you provision accounts for them on your Discourse forum, your Open edX LMS, your Zendesk, your Slack, and so on - all with one master login via your customer portal (single sign-on). Some time later they decide to close their account and request GDPR deletion of all the data that your company has about them. In the same way that you provisioned all their various accounts automatically, you now want to retire them all via an API. Another similar case is employee onboarding/offboarding - someone joins edX, they get their account created automatically; they leave edX, their account gets retired automatically. (edX probably doesn't do that but you can see how other orgs may want that.) |
(cherry picked from commit b22471d6760aa8e874e947de15144d2262696fd8)
|
Thanks @bradenmacdonald, that's exactly the use case here. CC @macdiesel |
|
We have the same use case at MIT Open Learning:
|
|
@macdiesel Actually, we had the same use case at one of our White Label sites. This is a desirable feature when you have an Open edX instance and you are bound by a contract with your corporate client to remove all their data, including user data, from your instance after the contract ends. |
|
If a feature flag could be added for this that would disable the feature on edx.org I'll pass the PR to one of the engineers on the team for review. |
|
@doctoryes Addressed the remaining comments 🙂 |
|
Thanks for sticking with all the requested changes here, @pkulkark ! Looks good - I'll merge now. |
|
Ohh - oops! @pkulkark : The new edx-platform standard is to use conventional commit messages: https://open-edx-proposals.readthedocs.io/en/latest/oep-0051-bp-conventional-commits.html Could you please squash/rebase your commits to conform to this new standard? Thank you! |
doctoryes
left a comment
There was a problem hiding this comment.
Merge blocked on conventional commits.
a243bac to
fac5076
Compare
|
Thank you so much for the review @doctoryes! I've squashed the commits to use the new standard. |
|
@pkulkark Looks like something happened during the rebase - now shows 288 files changed? |
|
@doctoryes Sorry about that! Will fix that right away |
This adds a new django app to allow the GDPR user retirement via Open edX's REST API. Prior to this the only way to trigger the user retirement was either by the user themself clicking "Delete my account" in the account setting page or via creating a User Retirement request by admin. With these changes, the user retirement process can be triggered using REST API.
fac5076 to
162fd44
Compare
|
Your PR has finished running tests. There were no failures. |
|
@pkulkark 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
Thanks again, @pkulkark ! |
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
(cherry picked from commit b22471d6760aa8e874e947de15144d2262696fd8)
(cherry picked from commit b22471d6760aa8e874e947de15144d2262696fd8)
This PR adds a new django app to allow the GDPR user retirement via Open edX's REST API. Prior to this the only way to trigger the user retirement was either by the user themself clicking "Delete my account" in the account setting page or via creating a User Retirement request by admin. With these changes, the user retirement process can be triggered using REST API.
Jira Tickets: OSPR-5290
Sandbox Url:
LMS: https://pr25800.sandbox.opencraft.hosting/
Studio: https://studio.pr25800.sandbox.opencraft.hosting/
Testing Instructions:
retirement-service-backend-service.retirement_userusing the curl command:gdpr_retirement_apiusing the curl command:PENDING.Authors notes and concerns:
Reviewers: