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

feat: add platform model and update course/program models #825

Merged
merged 2 commits into from
Sep 8, 2023

Conversation

asadali145
Copy link
Contributor

@asadali145 asadali145 commented Sep 5, 2023

What are the relevant tickets?

Fixes the last part of mitodl/mitxpro#2698

Description (What does it do?)

Adds staging and intermediate models for courses_platform in xPro. Also, it updates the staging and intermediate models course/program.

How can this be tested?

Run the following commands against QA | Production:
dbt build -s +int__mitxpro__platforms
dbt build -s +int__mitxpro__courses
dbt build -s +int__mitxpro__programs
dbt build -s +stg__mitxpro__app__postgres__courses_platform
dbt build -s +stg__mitxpro__app__postgres__courses_course
dbt build -s +stg__mitxpro__app__postgres__courses_program

I have used schema_suffix: add_platform_model

@asadali145 asadali145 marked this pull request as ready for review September 7, 2023 09:06
@rachellougee rachellougee self-assigned this Sep 7, 2023
Copy link
Contributor

@rachellougee rachellougee left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -14,6 +14,7 @@ select
, programs.program_is_live
, programs.program_readable_id
, programs.program_is_external
, programs.platform_id
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we should add platform_name to int__mitxpro__programs and int__mitxpro__courses since courses_platform is just a lookup table. Otherwise we would need join courses_platform when using courses/programs intermediate models.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please help me understand this one. Should we add the platform name in the course/program tables? The issue explicitly mentions adding a new table for the platforms just for reusability purposes.

Copy link
Contributor

@rachellougee rachellougee Sep 7, 2023

Choose a reason for hiding this comment

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

I am not sure what reusability the issue is referring to. Intermediate models are mostly denormalized tables where we join multiple tables together so they can be used by self or by mart.
https://github.com/mitodl/ol-data-platform/blob/main/src/ol_dbt/docs/staging_guidelines.md#what-is-the-staging-layer

I don't think it matters that much, just one less table to join in the mart or BI when querying intermediate courses/programs as end user would want the name not internal id.

So you don't have to add the platform name in this PR, we can add that later when working on the mart

@asadali145 asadali145 merged commit 3bf2632 into main Sep 8, 2023
2 checks passed
@asadali145 asadali145 deleted the add-platform-model branch September 8, 2023 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants