feat: migrate Insights course index APIs to Snowflake - #213
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces new Snowflake-backed SQL/query/mapper paths for core Insights endpoints, and this migration warrants a final human review for data-shape and operational correctness under production Snowflake data.
Pull request overview
This PR migrates the Insights “course index” endpoints (Programs and Course Summaries) to read from Snowflake behind the global Insights Snowflake flag, while preserving the existing Aurora-backed behavior when the flag is disabled.
Changes:
- Added Snowflake query + mapper layers for program metadata and course summaries.
- Updated
ProgramsViewandCourseSummariesViewto route to Snowflake when the global flag is enabled (and set theX-Insights-Data-Sourceresponse header accordingly). - Added/extended tests covering query construction, mapping behavior, service orchestration, and view routing/header behavior.
File summaries
| File | Description |
|---|---|
| analytics_data_api/v0/views/programs.py | Route programs index to Snowflake behind the global flag and set Insights data source header. |
| analytics_data_api/v0/views/course_summaries.py | Route course summaries GET/POST to Snowflake behind the global flag and preserve existing exclude/programs/recent behaviors. |
| analytics_data_api/v0/tests/views/test_programs.py | Add routing/header tests for Aurora vs Snowflake behavior in ProgramsView. |
| analytics_data_api/v0/tests/views/test_course_summaries.py | Add routing/header tests for Aurora vs Snowflake behavior in CourseSummariesView (including POST). |
| analytics_data_api/tests/test_insights_snowflake.py | Add unit tests for new course summary + program queries, mappers, and service functions. |
| analytics_data_api/insights_snowflake/service.py | Add Snowflake service orchestration for program metadata and course summaries. |
| analytics_data_api/insights_snowflake/queries/programs.py | Add Snowflake SQL query for program metadata rows. |
| analytics_data_api/insights_snowflake/queries/course_summaries.py | Add Snowflake SQL queries for course summary rows, program rows, and recent enrollment rows. |
| analytics_data_api/insights_snowflake/mappers/programs.py | Add mapper to group/shape program metadata rows into the existing API response format. |
| analytics_data_api/insights_snowflake/mappers/course_summaries.py | Add mapper to group/shape course summary rows (including programs and recent deltas) into the existing API response format. |
Review details
Suppressed comments (1)
analytics_data_api/insights_snowflake/queries/course_summaries.py:93
get_course_recent_enrollment_rowsselectsdateandcreated, but the mapper only usescourse_idandcountto computerecent_count_change. Dropping unused columns reduces the amount of data Snowflake has to scan/return.
SELECT
course_id,
"DATE" AS date,
"COUNT" AS count,
created
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| SELECT | ||
| course_id, | ||
| program_id, | ||
| program_type, | ||
| program_title, | ||
| created | ||
| FROM {table_name} | ||
| {where_clause} | ||
| ORDER BY course_id, program_id | ||
| """.format(table_name=table_name, where_clause=where_clause) |
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces new Snowflake-backed query/mapping paths for production APIs, and correctness against real Snowflake data/contracts needs final human verification beyond unit tests.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
analytics_data_api/tests/test_insights_snowflake.py:27
- Avoid using a backslash for line continuation in imports; it’s harder to read and can trigger lint warnings. Prefer a parenthesized import instead.
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: santhosh-apphelix-2u <211942388+santhosh-apphelix-2u@users.noreply.github.com>
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
🔵 Needs a closer look
The current changes include test coverage gaps for the v1 fallback path and a couple of fixable maintainability/performance issues that should be addressed before approval.
Review details
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
analytics_data_api/tests/test_insights_snowflake.py:27
- Avoid using a backslash for line continuation in imports; it’s easier to mis-edit and is inconsistent with the parenthesized import style used elsewhere in this file.
analytics_data_api/v0/tests/views/test_course_summaries.py:288 - This test exercises the Aurora path but currently hits /api/v0 via the shared test helper, so it doesn’t verify the intended v1 endpoint fallback behavior when the global Snowflake flag is disabled.
analytics_data_api/v0/tests/views/test_programs.py:117 - This test is meant to validate the v1 Programs endpoint behavior, but it calls the v0 route; as a result it doesn’t explicitly cover the v1 Aurora fallback when the global Snowflake flag is disabled.
analytics_data_api/insights_snowflake/queries/course_summaries.py:70
- This query selects program_type, program_title, and created, but the course summary Snowflake mapper only uses course_id and program_id; dropping unused columns will reduce data scanned/transferred.
SELECT
course_id,
program_id,
program_type,
program_title,
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
This migrates the Insights course index APIs to Snowflake behind the global Insights Snowflake flag.
Covered endpoints:
Aurora remains the default when the Snowflake flag is off.
Changes