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/dim_teachers #65

Merged
merged 3 commits into from
Jan 29, 2024
Merged

fix/dim_teachers #65

merged 3 commits into from
Jan 29, 2024

Conversation

allison-code-dot-org
Copy link
Collaborator

What's the fix?

Add PII info (email, username, name) to dim_teachers and make underlying staging model incremental.

Why?

Marketing is often needing to pull teacher email addresses (and sometimes names) based on whether or not they still use code.org. Our models would allow them to do this if these PII fields were added to dim_teachers. This PII is already made available to them (and the org) through various google sheets, so was approved by Travis to be user-facing in Trevor. No PII is in the code itself.

Copy link
Contributor

@bakerfranke bakerfranke left a comment

Choose a reason for hiding this comment

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

LGTM. This interacts with an open PR I have right now, but I don't think it conflicts.

Overall, I agree that adding this pii is inevitable for our internal use cases. I'm not sure how we should manage from a security/privacy standpoint.

A future solution would be to make something like a dim_users_pii table that we could limit access to (somehow) and instruct people to only join to it if they need the PII.

@jordan-springer
Copy link
Collaborator

how do we feel about putting this model (and its nodes) in the analytics_pii schema? cc @allison-code-dot-org @bakerfranke

Copy link
Collaborator

@jordan-springer jordan-springer left a comment

Choose a reason for hiding this comment

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

Looks good, please add tests and documentation where applicable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants