User PII dictionary table #529

pomegranited · 2023-11-29T04:42:31Z

Adds event_sink.user_pii dictionary table created by joining external_id and user_profile tables, using the most recent user_profile row. This dictionary is cached in memory, refreshing every ASPECTS_PII_CACHE_LIFETIME seconds.

Also partitions the user_profile (and external_id) tables to prepare for user retirement deletions.

Note: A user may have more than one external_user_id, and so event_sink.user_pii may show more than one row per user.

Related information

Addresses #449
Depends on openedx-unsupported/openedx-event-sink-clickhouse#63
Based off #521 which adds the event_sink.external_id table.

Testing instructions

Enable Aspects PII data sinks:
tutor config save -s ASPECTS_ENABLE_PII=True
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
Apply migrations from this PR:
tutor dev do alembic -c "upgrade head"
Login to Superset as a superuser, and navigate to the SQL Lab
Check that the user_pii table is empty:
select * from event_sink.user_pii
Create a new user:
tutor dev do createuser any [email protected]
Re-check the user_pii table -- should have an entry for the new user
Login to the LMS as the new user and change some of the Account details
Re-check the user_pii table -- should show the updated data for the user within 10s.
Enrol in a course (this should create an external_id row).
Re-check the user_pii table -- should show the updated external_user_id for the user within 10s.
Check migration rollback works:
tutor dev do alembic -c "downgrade -3"

To do before merge:

Merge & tag Adds user retirement sink openedx-unsupported/openedx-event-sink-clickhouse#63 first
Update openedx-event-sink-clickhouse dependency

fix: add missing external_id to list of models fix: add username for event sink chore: reformat for quality checks

openedx-webhooks · 2023-11-29T04:42:37Z

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.

tutoraspects/templates/aspects/apps/aspects/migrations/alembic/versions/0028_user_pii.py

Ian2012

I'll approve, but I think @bmtcril would have an opinion on this one

run off an in-memory dictionary joining the PII tables.

bmtcril · 2023-12-01T18:29:03Z

tutoraspects/templates/aspects/apps/aspects/migrations/alembic/versions/0028_user_pii.py

+            password '{{ CLICKHOUSE_ADMIN_PASSWORD }}'
+            db '{{ ASPECTS_EVENT_SINK_DATABASE }}'
+            query "
+                with most_recent_user_profile as (


I think this will fan out, I don't see any limiting that only pulls the most recent profile or most recent external id in either of these queries?

Sorry @bmtcril -- I lost this clause somewhere along the line, but I've re-added it, see this line and this one.

I'm only pulling the most recent user_profile row, and all the external_id rows, since a single user may have more than one external ID (depending on the external_id_type, or if external IDs get regenerated with a new secret key).

Hmm, I didn't know external_id's could be changed. I'm reasonably sure they are currently immutable (or at least nothing updates them) 1:1 with user id / type. If nothing else we want to limit the external_id_type to xapi, but if there's a place where secret key is involved can you point it out? That would complicate several things in unexpected ways. :)

Hmm, I didn't know external_id's could be changed. I'm reasonably sure they are currently immutable (or at least nothing updates them) 1:1 with user id / type.

Ah, my apologies.. I was thinking of the old anonymous user ids, which were hashed on the Django app SECRET_KEY. External IDs are immutable, as you expected.

If nothing else we want to limit the external_id_type to xapi

Ok that makes sense. I wasn't sure if there were any other types of external_ids used in our events. Will update the query that generates the dict/table.

Please see 57a4f56.

FYI I also added a dataset for the new user_pii table with 9ceb57b.

so that user retirements can delete user PII records.

tutoraspects/templates/aspects/jobs/init/clickhouse/init-clickhouse.sh

Ian2012

Can you rebase on main?

pomegranited · 2023-12-07T01:09:24Z

FYI @Ian2012 I've rebased , but I want to address this comment before this gets merged.

so the event sink plugin knows which PII tables are in use.

or NULL if the user is not yet assigned an external_id.

sourced from the event_sink.user_pii table.

bmtcril

Awesome, looks good to me! 🎉

openedx-webhooks · 2023-12-11T22:04:06Z

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

Ian2012 and others added 3 commits November 17, 2023 15:11

feat: add external id table

bcd308f

fix: add missing external_id to list of models fix: add username for event sink chore: reformat for quality checks

feat: partition the event_sink.external_id table by user_id % 100

c45aefe

feat: partition the event_sink.user_profile table by user_id % 100

24db8d1

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

pomegranited requested review from bmtcril and Ian2012 November 29, 2023 05:30

itsjeyd added the core contributor PR author is a Core Contributor (who may or may not have write access to this repo). label Nov 29, 2023

Ian2012 force-pushed the cag/create_external_id_table branch from bcd308f to 3c50bd7 Compare November 29, 2023 16:11

Ian2012 reviewed Nov 29, 2023

View reviewed changes

tutoraspects/templates/aspects/apps/aspects/migrations/alembic/versions/0028_user_pii.py Show resolved Hide resolved

Base automatically changed from cag/create_external_id_table to main November 29, 2023 17:21

Ian2012 changed the base branch from main to bmtcril/bump_version November 29, 2023 17:24

Ian2012 changed the base branch from bmtcril/bump_version to main November 29, 2023 17:24

Ian2012 force-pushed the jill/user_pii branch from 93c5f06 to 7622363 Compare November 29, 2023 17:26

Ian2012 approved these changes Nov 29, 2023

View reviewed changes

pomegranited changed the title ~~User PII table~~ User PII dictionary table Nov 30, 2023

pomegranited mentioned this pull request Nov 30, 2023

Connect the exernal_user_ids to the User Retirement Process #449

Closed

feat: adds event_sink.user_pii table

7ced3f7

run off an in-memory dictionary joining the PII tables.

bmtcril reviewed Dec 1, 2023

View reviewed changes

pomegranited force-pushed the jill/user_pii branch from 7622363 to 7ced3f7 Compare December 4, 2023 03:09

pomegranited mentioned this pull request Dec 4, 2023

Adds user retirement sink openedx-unsupported/openedx-event-sink-clickhouse#63

Merged

9 tasks

fix: grant DELETE access to the CMS Clickhouse user on the event_sink db

b97a690

so that user retirements can delete user PII records.

Ian2012 reviewed Dec 5, 2023

View reviewed changes

tutoraspects/templates/aspects/jobs/init/clickhouse/init-clickhouse.sh Show resolved Hide resolved

Ian2012 reviewed Dec 5, 2023

View reviewed changes

Merge branch 'main' into jill/user_pii

fcfeb9f

feat: pass EVENT_SINK_CLICKHOUSE_PII_MODELS to the openedx settings

defb3f1

so the event sink plugin knows which PII tables are in use.

pomegranited requested review from bmtcril and Ian2012 December 7, 2023 06:08

Ian2012 approved these changes Dec 7, 2023

View reviewed changes

pomegranited added 3 commits December 11, 2023 08:51

chore: bumps openedx-event-sink-clickhouse to 0.5.0

017a754

fix: user_pii uses external_id_type=xapi

57a4f56

or NULL if the user is not yet assigned an external_id.

feat: adds user_pii dataset

9ceb57b

sourced from the event_sink.user_pii table.

bmtcril approved these changes Dec 11, 2023

View reviewed changes

Ian2012 merged commit 29566cf into main Dec 11, 2023
6 checks passed

Ian2012 deleted the jill/user_pii branch December 11, 2023 22:04

bmtcril mentioned this pull request Dec 12, 2023

feat: move second-tier MVs into dbt project (FC-0033) openedx/aspects-dbt#34

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User PII dictionary table #529

User PII dictionary table #529

pomegranited commented Nov 29, 2023 •

edited

Loading

openedx-webhooks commented Nov 29, 2023

Ian2012 left a comment

bmtcril Dec 1, 2023

pomegranited Dec 4, 2023

bmtcril Dec 7, 2023

pomegranited Dec 10, 2023

pomegranited Dec 11, 2023

pomegranited Dec 11, 2023

Ian2012 left a comment

pomegranited commented Dec 7, 2023

bmtcril left a comment

openedx-webhooks commented Dec 11, 2023

User PII dictionary table #529

User PII dictionary table #529

Conversation

pomegranited commented Nov 29, 2023 • edited Loading

Related information

Testing instructions

To do before merge:

openedx-webhooks commented Nov 29, 2023

Ian2012 left a comment

Choose a reason for hiding this comment

bmtcril Dec 1, 2023

Choose a reason for hiding this comment

pomegranited Dec 4, 2023

Choose a reason for hiding this comment

bmtcril Dec 7, 2023

Choose a reason for hiding this comment

pomegranited Dec 10, 2023

Choose a reason for hiding this comment

pomegranited Dec 11, 2023

Choose a reason for hiding this comment

pomegranited Dec 11, 2023

Choose a reason for hiding this comment

Ian2012 left a comment

Choose a reason for hiding this comment

pomegranited commented Dec 7, 2023

bmtcril left a comment

Choose a reason for hiding this comment

openedx-webhooks commented Dec 11, 2023

pomegranited commented Nov 29, 2023 •

edited

Loading