Adds user retirement sink #63

pomegranited · 2023-12-04T03:36:26Z

Description:

Adds an event sink for "user retirement" events.

On receipt of a USER_RETIRE_LMS_MISC Django signal for a given user, the sink will delete all records from the PII tables for that user, currently:

event_sink.user_profile
event_sink.external_id

ISSUE: openedx/tutor-contrib-aspects#449

Relates to: openedx/tutor-contrib-aspects#529

This PR should be merged/tagged first.

Merge deadline: none

Installation instructions:

To install this temporary branch on your local Tutor Aspects stack:

Install the Aspects plugin from User PII dictionary table openedx/tutor-contrib-aspects#529
Install the user retirement plugin, following instructions on https://github.com/hastexo/tutor-contrib-retirement
Add this repo/branch to the LMS requirements:
echo "git+https://github.com/openedx/openedx-event-sink-clickhouse.git@jill/retire-user#egg=openedx-event-sink-clickhouse==0.5.0" >> "$(tutor config printroot)/env/build/openedx/requirements/private.txt"
Enable Aspects PII:
tutor config save -s ASPECTS_ENABLE_PII=True
(optional) Shorten the lifetime of the user_pii dict to 10s to make testing data changes easier:
tutor config save -s ASPECTS_PII_CACHE_LIFETIME=10
(optional) Remove the cooling off period to make it easier to process user retirements:
tutor config save -s RETIREMENT_COOL_OFF_DAYS=0
Rebuild image:
tutor images build openedx --no-cache
Re-init Aspects to use updated settings:
tutor local do init -l aspects
Reboot Tutor to use new images:
tutor local reboot -d

Testing instructions:

Register a new user on the platform, and update that user's Account (aka user profile) details.
Use Superset's SQL Lab to check that the user profile change(s) flow through to the event_sink.user_pii table.
Enrol the user in a course to see external IDs created and flowing into the event_sink.user_pii table.
In the LMS Accounts page, delete the user.
From the command line, run the user retirement pipeline to move the user's retirement request from PENDING into COMPLETED.
tutor local retire-users
After ASPECTS_PII_CACHE_LIFETIME elapses, check Superset SQL Lab to ensure that the user records have been deleted from event_sink.user_pii.

Merge checklist:

Post merge:

Create a tag
Check new version is pushed to PyPI after tag-triggered build is
finished.
Delete working branch (if not needed anymore)

so we can run `make format`

openedx-webhooks · 2023-12-04T03:36:33Z

Thanks for the pull request, @pomegranited! 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.

Ian2012 · 2023-12-04T15:00:19Z

event_sink_clickhouse/signals.py

+try:
+    from openedx.core.djangoapps.user_api.accounts.signals import USER_RETIREMENT_LMS_MISC
+except ImportError:
+    # Tests don't have the platform installed
+    USER_RETIREMENT_LMS_MISC = Signal()
+


We should try to use an openedx event for this one, I think there isn't one but I'm not sure. I can do the contribution if needed

This is the default retirement Signal inside edx-platform, it's used for a lot of things. I totally agree that we should refactor retirement to use openedx-events (and eventually the event bus), but I don't think we should do it as part of this work as I think it would further confuse an already complicated process.

and tests

bmtcril · 2023-12-05T13:16:11Z

tests/test_user_retire.py

+@patch("event_sink_clickhouse.sinks.user_retire.UserRetirementSink.serialize_item")
+@patch("event_sink_clickhouse.sinks.user_retire.UserRetirementSink.is_enabled")
+@patch("event_sink_clickhouse.sinks.user_retire.UserRetirementSink.get_model")
+def test_retire_user(mock_user_model, mock_is_enabled, mock_serialize_item):


Can we get one more test that exercises the many version of this, just to make sure no one breaks it in the future?

Sure thing, see https://github.com/openedx/openedx-event-sink-clickhouse/pull/63/files#diff-69e4bc0dbdddfc867532a028e3b4b6764834376c04b2ac62b5ea5e61ee928145R77

bmtcril · 2023-12-05T13:18:02Z

event_sink_clickhouse/sinks/user_retire.py

+
+    model = "auth_user"
+    unique_key = "id"
+    clickhouse_table_name = ["user_profile", "external_id"]


It's sad that we don't have a more data driven way to deal with this list, but I think it's ok. Can you put a comment here reminding people that this list should exactly match EVENT_SINK_CLICKHOUSE_PII_MODELS in the Aspects plugin? We'll probably want one there, too, to try to keep these from falling out of sync.

What if we read this value from settings, and inject it in tutor-contrib-aspects?

And transform the PII_MODELS, and MODELS, settings into OVERRIDE_SETTINGS. So those cannot be overriden

I don't know if this is a desired capability, but it is possible to define other sinks in plugins, so people may want to override those settings. I'm not 100% sure about the hierarchy of settings here, but I think if there's a default setting in this app, and a setting in tutor-contrib-aspects the latter will override. Then if an operator has other needs their specific config will take precedence. I think that would be my preference.

I've updated tutor-contrib-aspects#529 to pass EVENT_SINK_CLICKHOUSE_PII_MODELS in to the openedx settings, and modified this code to use that setting: edc380f

I'm doing a full rebuild of my Tutor dev stack to test this, but how does it look to you?

(rebuild worked.)

This setting is overriden in production by the Tutor environment, so that: * the full list of Clickhouse PII tables can be controlled by Aspects * end users can add their own PII tables too.

and alters the code to show coverage of this branch.

bmtcril

LGTM!

openedx-webhooks · 2023-12-07T15:34:46Z

@pomegranited 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

fix: adds black to dev requirements

3cf3c42

so we can run `make format`

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Dec 4, 2023

pomegranited force-pushed the jill/retire-user branch 2 times, most recently from 3d9688d to 5a8400a Compare December 4, 2023 03:52

Ian2012 reviewed Dec 4, 2023

View reviewed changes

pomegranited added 2 commits December 5, 2023 12:49

fix: camel case ExternalIdSink

82dd421

feat: adds UserRetirementSink

f2d808b

and tests

pomegranited force-pushed the jill/retire-user branch from 5a8400a to f2d808b Compare December 5, 2023 02:22

Merge branch 'main' into jill/retire-user

06bf9f8

pomegranited marked this pull request as ready for review December 5, 2023 06:11

pomegranited requested review from bmtcril and Ian2012 December 5, 2023 06:12

bmtcril reviewed Dec 5, 2023

View reviewed changes

pomegranited mentioned this pull request Dec 7, 2023

User PII dictionary table openedx/tutor-contrib-aspects#529

Merged

2 tasks

pomegranited added 2 commits December 7, 2023 12:13

feat: adds settings.EVENT_SINK_CLICKHOUSE_PII_MODELS

edc380f

This setting is overriden in production by the Tutor environment, so that: * the full list of Clickhouse PII tables can be controlled by Aspects * end users can add their own PII tables too.

test: adds tests for retiring many serialized users at once

2e41ccd

and alters the code to show coverage of this branch.

pomegranited force-pushed the jill/retire-user branch from 972e37c to 2e41ccd Compare December 7, 2023 02:22

pomegranited requested a review from bmtcril December 7, 2023 06:09

Ian2012 approved these changes Dec 7, 2023

View reviewed changes

bmtcril approved these changes Dec 7, 2023

View reviewed changes

Ian2012 merged commit bc68887 into main Dec 7, 2023
8 checks passed

Ian2012 deleted the jill/retire-user branch December 7, 2023 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds user retirement sink #63

Adds user retirement sink #63

pomegranited commented Dec 4, 2023 •

edited

Loading

openedx-webhooks commented Dec 4, 2023 •

edited

Loading

Ian2012 Dec 4, 2023

bmtcril Dec 4, 2023

bmtcril Dec 5, 2023

pomegranited Dec 7, 2023

bmtcril Dec 5, 2023

Ian2012 Dec 5, 2023

Ian2012 Dec 5, 2023

bmtcril Dec 6, 2023

pomegranited Dec 7, 2023

pomegranited Dec 7, 2023

bmtcril left a comment

openedx-webhooks commented Dec 7, 2023

Adds user retirement sink #63

Adds user retirement sink #63

Conversation

pomegranited commented Dec 4, 2023 • edited Loading

openedx-webhooks commented Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmtcril left a comment

Choose a reason for hiding this comment

openedx-webhooks commented Dec 7, 2023

pomegranited commented Dec 4, 2023 •

edited

Loading

openedx-webhooks commented Dec 4, 2023 •

edited

Loading