feat(dashboards): add Snowflake APIs for marketing#346
feat(dashboards): add Snowflake APIs for marketing#346mrautela365 wants to merge 6 commits intofeat/LFXV2-1220-pr1-persona-typesfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
There was a problem hiding this comment.
Pull request overview
Adds backend support for the upcoming marketing dashboard by exposing new analytics endpoints backed by Snowflake queries.
Changes:
- Added Snowflake query methods in
ProjectServicefor web activities, email CTR, social reach, and social media metrics. - Added corresponding controller handlers with request validation + structured logging.
- Registered 4 new GET routes under the analytics router.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| apps/lfx-one/src/server/services/project.service.ts | Implements 4 new Snowflake-backed marketing query methods and transforms results into shared response shapes. |
| apps/lfx-one/src/server/controllers/analytics.controller.ts | Adds 4 new request handlers validating query params and returning the new service responses. |
| apps/lfx-one/src/server/routes/analytics.route.ts | Registers 4 new marketing dashboard GET routes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
17af13d to
0e73461
Compare
0e73461 to
b8463ce
Compare
b8463ce to
6b696da
Compare
Add 4 server-side endpoints for ED marketing dashboard data: - GET /web-activities-summary — website visit metrics - GET /email-ctr — email click-through rate metrics - GET /social-reach — paid social/ad campaign metrics - GET /social-media — social media overview and follower trends All queries target Platinum Snowflake views. LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
- getSocialReach: use PAID_SOCIAL_REACH tables with FOUNDATION_NAME filter and pre-computed ROAS/ROAS_MOM_PCT columns - getSocialMedia: add SUM() aggregations and GROUP BY for overview, platform breakdown, and follower trend queries - getEmailCtr: remove project filter from campaigns query, add LF_SUB_DOMAIN_CLASSIFICATION, fix CTR conversion - getSocialReach controller: accept foundationName param LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Update getWebActivitiesSummary and getEmailCtr JSDoc to reference the actual Platinum tables being queried. LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Remove totalSends/totalClicks from campaign group mapping and totalEngagements from monthly social media mapping. These values were hardcoded to 0 because the Snowflake tables lack the corresponding columns, and no UI component consumes them. LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
6b696da to
88f019b
Compare
Email CTR: Snowflake values are already percentages (e.g. 2.32 = 2.32%) but code was treating them as decimals and multiplying by 100, producing 232% instead of 2.3%. Remove the double conversion. Social Reach: Aggregate totalSpend/totalRevenue from BY_PROJECT_MONTH table instead of hardcoding 0. LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Campaign query was missing WHERE clause, returning all projects instead of filtering by foundation. Also added nullish coalescing on CTR values to prevent NaN when Snowflake returns NULL columns. LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
🚀 Deployment StatusYour branch has been deployed to: https://ui-pr-346.dev.v2.cluster.linuxfound.info Deployment Details:
The deployment will be automatically removed when this PR is closed. |
MRashad26
left a comment
There was a problem hiding this comment.
PR Review — feat(dashboards): add Snowflake APIs for marketing
Author: mrautela365 (non-technical member)
Size: +503 / -0 across 3 files (backend-only)
What's done well
- Controller pattern matches existing codebase (try/catch,
logger.startOperation/success/error,next(error)) - Proper input validation with
ServiceValidationError.forField()on all 4 endpoints - Parameterized SQL queries with
?placeholders — no SQL injection risk Promise.allfor parallel Snowflake query execution within each method- Auth covered by global
authMiddleware— no per-route guards needed (matches codebase convention) - Null safety with
??on Snowflake values that can returnNULL
Critical note
All 9 Snowflake tables use ANALYTICS.PLATINUM.* schema, but the entire existing codebase uses ANALYTICS.PLATINUM_LFX_ONE.*. If these tables do not exist in the Snowflake environment, all 4 endpoints will fail — and PR #347 (frontend cards) should not be merged until this PR's endpoints are confirmed working, since the UI will silently show empty/zero data with no error indication.
Issues — all reported as inline blockers below
| LF_SUB_DOMAIN_CLASSIFICATION, | ||
| SUM(TOTAL_SESSIONS_LAST_30_DAYS) AS TOTAL_SESSIONS, | ||
| SUM(TOTAL_PAGE_VIEWS_LAST_30_DAYS) AS TOTAL_PAGE_VIEWS | ||
| FROM ANALYTICS.PLATINUM.WEB_ACTIVITIES_SUMMARY |
There was a problem hiding this comment.
🚫 Blocker: All 9 Snowflake tables use ANALYTICS.PLATINUM.* — a schema that doesn't exist in this codebase
Problem: This PR introduces 9 new table references under ANALYTICS.PLATINUM.*:
ANALYTICS.PLATINUM.WEB_ACTIVITIES_SUMMARYANALYTICS.PLATINUM.WEB_ACTIVITIES_BY_PROJECTANALYTICS.PLATINUM.EMAIL_CTR_SUMMARYANALYTICS.PLATINUM.EMAIL_CTR_BY_MONTHANALYTICS.PLATINUM.PAID_SOCIAL_REACH_BY_PROJECT_MONTHANALYTICS.PLATINUM.PAID_SOCIAL_REACH_BY_PROJECT_CHANNEL_MONTHANALYTICS.PLATINUM.SOCIAL_MEDIA_OVERVIEWANALYTICS.PLATINUM.SOCIAL_MEDIA_PLATFORM_BREAKDOWNANALYTICS.PLATINUM.SOCIAL_MEDIA_FOLLOWER_TREND
The entire existing codebase uses ANALYTICS.PLATINUM_LFX_ONE.* tables (e.g., FOUNDATION_ISSUES_RESOLUTION, PROJECT_PULL_REQUESTS_WEEKLY). None of the 9 new table names appear anywhere else in the repo.
Why it's a problem: If ANALYTICS.PLATINUM.* is not a valid Snowflake schema, all 4 endpoints will return 500 errors. Because the controller's catchError swallows failures and the frontend (PR #347) uses catchError(() => of(defaultResponse)), the UI will silently show zeros/empty cards — indistinguishable from 'no data.' This is the #1 risk for this entire PR chain (#346 → #347 → subsequent PRs). PR #347's frontend changes should not be merged until these endpoints are confirmed working.
Fix:
- Confirm with the data/analytics team that
ANALYTICS.PLATINUMis a valid, accessible schema (distinct fromANALYTICS.PLATINUM_LFX_ONE). - If these are new tables, add a comment documenting when they were provisioned and their expected availability.
- Test at least one endpoint manually against the Snowflake environment before merging.
There was a problem hiding this comment.
Fixed. ANALYTICS.PLATINUM.* is a valid production Snowflake schema — distinct from ANALYTICS.PLATINUM_LFX_ONE.*. The split reflects different data pipelines: marketing/HubSpot tables use PLATINUM, while LFX-curated views (foundation-health, board-member) use PLATINUM_LFX_ONE. Added a comment documenting this at line 1671. Confirmed working in deploy preview.
| * Get web activities summary grouped by domain category | ||
| * Queries ANALYTICS.PLATINUM.WEB_ACTIVITIES_SUMMARY and ANALYTICS.PLATINUM.WEB_ACTIVITIES_BY_PROJECT | ||
| */ | ||
| public async getWebActivitiesSummary(foundationSlug: string): Promise<WebActivitiesSummaryResponse> { |
There was a problem hiding this comment.
🚫 Blocker: Shared interfaces may not exist yet — verify PR #345 adds them
Problem: This file imports 4 new response interfaces from @lfx-one/shared/interfaces:
WebActivitiesSummaryResponseEmailCtrResponseSocialMediaResponseSocialReachResponse
These interfaces do not exist in the current analytics-data.interface.ts (which has 30+ other response types). The PR description says it depends on PR #345 (shared types).
Why it's a problem: If PR #345 hasn't been merged or these types don't match the shapes returned here, the build will fail. Additionally, if the interface shapes in #345 don't align with the actual Snowflake column names (e.g., campaignGroups property in EmailCtrResponse), runtime serialization will silently produce incorrect data.
Fix: Verify that PR #345 defines all 4 interfaces with shapes matching the return objects in this PR. Ensure the base branch feat/LFXV2-1220-pr1-persona-types includes the merged #345 changes.
There was a problem hiding this comment.
Fixed. PR #345 defines all 4 interfaces in analytics-data.interface.ts. The chained base branches ensure these are available. Build passes across all PRs.
| const startTime = logger.startOperation(req, 'get_web_activities_summary'); | ||
|
|
||
| try { | ||
| const foundationSlug = req.query['foundationSlug'] as string | undefined; |
There was a problem hiding this comment.
🚫 Blocker: Inconsistent parameter naming — foundationSlug vs foundationName across the 4 endpoints
Problem: getWebActivitiesSummary accepts foundationSlug (filtering by PROJECT_SLUG), while the other 3 endpoints accept foundationName (filtering by FOUNDATION_NAME / PROJECT_NAME). The frontend (PR #347) calls them as:
this.analyticsService.getWebActivitiesSummary(foundation.slug)
this.analyticsService.getEmailCtr(foundation.name)Why it's a problem:
- API inconsistency — consumers must remember which endpoint takes a slug vs a name. Every other analytics endpoint in this controller uses a single identifier pattern.
- Fragile coupling —
foundation.slugandfoundation.nameare different values. If a foundation's display name changes but slug doesn't (or vice versa), one endpoint breaks while the other works. - Email CTR queries by
PROJECT_NAME(line 1739, 1751, 1762) but the parameter is calledfoundationName— this is semantically confusing since projects ≠ foundations.
Fix: Align all 4 endpoints on a single identifier. If the Snowflake tables use different filter columns, document why — but the API surface should ideally use one parameter (preferably foundationSlug since slugs are stable identifiers).
There was a problem hiding this comment.
By design — documented in a comment at line 1672. Web Activities uses foundationSlug because WEB_ACTIVITIES_* tables key on PROJECT_SLUG. Email CTR, Social Reach, and Social Media use foundationName because those Platinum tables key on FOUNDATION_NAME/PROJECT_NAME. The frontend passes the correct identifier for each endpoint (foundation.slug vs foundation.name). Unifying would require either a backend JOIN (fragile) or Snowflake schema changes.
| } | ||
| } | ||
|
|
||
| public async getSocialReach(req: Request, res: Response, next: NextFunction): Promise<void> { |
There was a problem hiding this comment.
🚫 Blocker: getSocialReach controller method missing JSDoc comment
Problem: The other 3 new controller methods (getWebActivitiesSummary, getEmailCtr, getSocialMedia) all have JSDoc blocks:
/**
* GET /api/analytics/web-activities-summary
* Get web activities summary grouped by domain category
*/But getSocialReach has no JSDoc at all.
Why it's a problem: Every existing method in analytics.controller.ts follows the /** GET /api/analytics/... */ JSDoc pattern. This breaks consistency and makes the endpoint harder to discover via IDE hover/search.
Fix: Add the missing JSDoc:
/**
* GET /api/analytics/social-reach
* Get paid social reach and ROAS data
*/
public async getSocialReach(...)There was a problem hiding this comment.
Fixed in commit 7d3eb2c. JSDoc added.
| const channelGroups = channelResult.rows.map((row) => ({ | ||
| channel: row.CHANNEL, | ||
| totalImpressions: row.IMPRESSIONS, | ||
| totalSpend: 0, |
There was a problem hiding this comment.
🚫 Blocker: channelGroups hardcodes totalSpend: 0, totalRevenue: 0, roas: 0 — dead data
Problem: The channel breakdown query only selects CHANNEL and SUM(IMPRESSIONS), but the response object includes:
const channelGroups = channelResult.rows.map((row) => ({
channel: row.CHANNEL,
totalImpressions: row.IMPRESSIONS,
totalSpend: 0, // ← hardcoded
totalRevenue: 0, // ← hardcoded
roas: 0, // ← hardcoded
}));Why it's a problem:
- If these columns exist in
PAID_SOCIAL_REACH_BY_PROJECT_CHANNEL_MONTH, they should be queried — the data is just being thrown away. - If they don't exist in the table, they shouldn't be in the response interface — the frontend will show zeros that look like real data ("$0 spend") when it's actually "data not available."
- The shared interface (
SocialReachResponse) will have these fields typed asnumber, giving consumers no way to distinguish "actually zero" from "not queried."
Fix: Either:
- Add
SPEND,REVENUE, and computeROASin the channel query if the columns exist, OR - Remove these fields from the channel group mapping and interface — don't send fake zeros.
There was a problem hiding this comment.
Documented with a comment at line 1929. PAID_SOCIAL_REACH_BY_PROJECT_CHANNEL_MONTH only has IMPRESSIONS — SPEND/REVENUE columns don't exist on this table. The frontend guards with worst.totalSpend > 0 so no misleading recommendations appear. Will remove the zero fields when the analytics team confirms whether channel-level spend data will be added.
|
|
||
| const [impressionsResult, roasKpiResult, monthlyRoasResult, monthlyImpressionsResult, channelResult] = await Promise.all([ | ||
| this.snowflakeService.execute<{ TOTAL_IMPRESSIONS: number; TOTAL_SPEND: number; TOTAL_REVENUE: number }>(impressionsQuery, [foundationName]), | ||
| this.snowflakeService.execute<{ ROAS: number; ROAS_MOM_PCT: number }>(roasKpiQuery, [foundationName, foundationName]), |
There was a problem hiding this comment.
🚫 Blocker: roasKpiQuery requires foundationName passed twice — fragile and hard to read
Problem: The ROAS KPI query uses a correlated subquery with FOUNDATION_NAME = ? in both the outer query and the inner SELECT MAX(CAMPAIGN_MONTH) subquery, requiring:
this.snowflakeService.execute<...>(roasKpiQuery, [foundationName, foundationName])Why it's a problem: Passing the same bind variable twice is error-prone — if someone reorders the WHERE clauses or adds another filter, the positional binds silently shift. Every other query in this PR uses a single bind.
Fix: Rewrite using a CTE or QUALIFY to eliminate the double bind:
SELECT ROAS, ROAS_MOM_PCT
FROM ANALYTICS.PLATINUM.PAID_SOCIAL_REACH_BY_PROJECT_MONTH
WHERE FOUNDATION_NAME = ?
AND CAMPAIGN_MONTH < DATE_TRUNC('MONTH', CURRENT_DATE())
QUALIFY ROW_NUMBER() OVER (ORDER BY CAMPAIGN_MONTH DESC) = 1This is cleaner, uses one bind, and is a standard Snowflake pattern.
There was a problem hiding this comment.
Fixed in commit 7d3eb2c. Rewritten with QUALIFY ROW_NUMBER() OVER (ORDER BY CAMPAIGN_MONTH DESC) = 1 — single bind, cleaner.
| const overviewQuery = ` | ||
| SELECT | ||
| SUM(TOTAL_FOLLOWERS) AS TOTAL_FOLLOWERS, | ||
| SUM(PLATFORMS_ACTIVE) AS PLATFORMS_ACTIVE, |
There was a problem hiding this comment.
🚫 Blocker: SUM(PLATFORMS_ACTIVE) — contradicts PR #347's change from SUM → MAX on the same column
Problem: This query uses SUM(PLATFORMS_ACTIVE) to aggregate across foundation sub-project rows:
SUM(PLATFORMS_ACTIVE) AS PLATFORMS_ACTIVEBut PR #347 changes the existing SOCIAL_MEDIA_OVERVIEW_BY_FOUNDATION_MONTH query from SUM(PLATFORMS_ACTIVE) to MAX(PLATFORMS_ACTIVE) with the comment: "Use MAX to avoid double-counting across sub-project rows."
Why it's a problem: These two PRs are in the same chain (#345 → #346 → #347) and they contradict each other on the same column semantics. If MAX is correct for #347 (because PLATFORMS_ACTIVE is a foundation-level constant repeated per sub-project row), then SUM is wrong here — it will over-count. If SUM is correct here (because PLATFORMS_ACTIVE varies per row), then #347's change is wrong.
Fix: Determine the semantics of PLATFORMS_ACTIVE in SOCIAL_MEDIA_OVERVIEW:
- If it's a foundation-level constant → use
MAXin both PRs - If it varies per sub-project → use
SUMin both PRs
Then align both queries.
There was a problem hiding this comment.
Fixed — aligned to MAX in both locations. PLATFORMS_ACTIVE is a foundation-level constant repeated per sub-project row in SOCIAL_MEDIA_OVERVIEW. MAX is correct to avoid over-counting.
| const totalPlatforms = overview.PLATFORMS_ACTIVE; | ||
| const changePercentage = overview.FOLLOWER_GROWTH_PCT ?? 0; | ||
|
|
||
| const platformIconMap: Record<string, string> = { |
There was a problem hiding this comment.
🚫 Blocker: platformIconMap hardcoded in the backend service — presentation logic in the wrong layer
Problem: The getSocialMedia method maps platform names to Font Awesome icon classes:
const platformIconMap: Record<string, string> = {
Twitter: 'fa-brands fa-x-twitter',
LinkedIn: 'fa-brands fa-linkedin',
YouTube: 'fa-brands fa-youtube',
...
};Why it's a problem:
- Layer violation — Icon class names (
fa-brands fa-x-twitter) are purely a frontend/presentation concern. The backend service should return data, not UI rendering instructions. - Maintenance burden — If the frontend switches icon libraries (e.g., from Font Awesome to Material Icons), this backend code needs to change.
- Incomplete fallback — The
|| 'fa-light fa-globe'default will silently produce a generic icon for any new platform (e.g., Threads) without any indication to the frontend that the mapping is missing.
Fix: Remove iconClass from the backend response. Let the frontend handle the platform → icon mapping — the Angular component already knows the icon library being used. The response should only return platform: row.PLATFORM_NAME.
There was a problem hiding this comment.
Fixed in commit 7d3eb2c. platformIconMap moved to SocialMediaDrawerComponent on the frontend. Backend now returns only platform: row.PLATFORM_NAME. iconClass made optional on the SocialMediaPlatform interface.
| * @param foundationName - Foundation name used to filter metrics (e.g., 'The Linux Foundation') | ||
| * @returns Social media response with followers, platform breakdown, and trend data | ||
| */ | ||
| public async getSocialMedia(foundationName: string): Promise<SocialMediaResponse> { |
There was a problem hiding this comment.
🚫 Blocker: getSocialMedia method placed after private utility methods — breaks file organization
Problem: In project.service.ts, the private utility methods getProjectSfidByUid (marked @private) and getProjectIdBySlug are at ~line 1930-1997. The new getSocialMedia method is placed after them at line 2002.
Meanwhile, getSocialReach (the other new public method) is correctly placed at line 1812 — before the private methods section.
Why it's a problem: The file organization has public query methods grouped together, followed by private utilities at the end. Placing a public method after the private section breaks this pattern and makes it harder to find.
Fix: Move getSocialMedia to be adjacent to getSocialReach (before the private methods section), keeping all 4 new public methods together.
There was a problem hiding this comment.
Fixed in commit 7d3eb2c. getSocialMedia moved to be adjacent to getSocialReach, before the private utility methods section.
| PROJECT_NAME, | ||
| LF_SUB_DOMAIN_CLASSIFICATION, | ||
| CTR_LAST_6_MONTHS AS AVG_CTR | ||
| FROM ANALYTICS.PLATINUM.EMAIL_CTR_SUMMARY |
There was a problem hiding this comment.
🚫 Blocker: campaignQuery comment says "all projects" but query filters by single PROJECT_NAME
Problem: The comment for query 3 says:
// Query 3: CTR by campaign/project (horizontal bar) from email_ctr_summary — all projectsBut the query has WHERE PROJECT_NAME = ?, which filters to a single foundation's data — not "all projects."
Why it's a problem: Misleading comments are worse than no comments. A future developer reading "all projects" may assume this returns cross-foundation data and build UI logic around that assumption. Additionally, this query hits the same table (EMAIL_CTR_SUMMARY) as query 1 with the same filter — if the intent is to get all projects under a foundation, the filter column should be FOUNDATION_NAME, not PROJECT_NAME.
Fix:
- If the intent is all projects under the foundation: change the filter to
WHERE FOUNDATION_NAME = ?(or whatever column represents the parent foundation). - If the intent is just the single project: fix the comment to say "CTR for the specified project."
- Either way, update the comment to match the actual query behavior.
There was a problem hiding this comment.
Fixed in commit 7d3eb2c. Comment updated to accurately reflect the query behavior — filters by single PROJECT_NAME, not 'all projects'.
Summary
project.service.tsfor marketing dataanalytics.controller.tswith error handlinganalytics.route.tsEndpoints
/web-activities-summaryWEB_ACTIVITIES_SUMMARY+BY_PROJECT/email-ctrEMAIL_CTR_SUMMARY+BY_MONTH/social-reachPAID_ADS_BY_CAMPAIGN_CHANNEL_MONTH/social-mediaSOCIAL_MEDIA_OVERVIEW+PLATFORM_BREAKDOWN+FOLLOWER_TRENDContext
PR 2 of 5 — backend-only, no frontend changes. Endpoints exist but nothing calls them yet.
Depends on: #345 (shared types)
PR sequence: #345 → PR 2 (this) → PR 3a → PR 3b → PR 4 → PR 5
Test plan
yarn buildpassesyarn lintpassesLFXV2-1220
🤖 Generated with Claude Code