feat(dashboards): wire focus filter across all marketing impact tabs#800
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThreads focus-derived classification into marketing analytics: updates focus program types/mapping, frontend components to derive/pass classification, controller validation, and ProjectService SQL filters; removes program-level “not yet available” UI banners. ChangesMarketing Focus Program Classification Filtering
Sequence Diagram(s)sequenceDiagram
participant UI as Marketing UI Component
participant Rx as RxJS combineLatest
participant ClientService as AnalyticsService (client)
participant Server as AnalyticsController
participant Project as ProjectService
participant Snowflake
UI->>Rx: subscribe(slug$, focusProgram$)
Rx->>Rx: derive classification via FOCUS_TO_CLASSIFICATION
Rx->>ClientService: call get*(slug, classification)
ClientService->>Server: HTTP GET /analytics?foundationSlug=...&classification=...
Server->>Project: projectService.get*(foundationSlug, classification)
Project->>Snowflake: execute SQL with LF_SUB_DOMAIN_CLASSIFICATION bind
Snowflake-->>Project: results
Project-->>Server: aggregated results
Server-->>ClientService: JSON response
ClientService-->>UI: resolved data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR wires the Marketing Impact “Focus” filter end-to-end by mapping UI focus pills to Snowflake LF_SUB_DOMAIN_CLASSIFICATION, passing that classification through the Angular analytics service, BFF controller, and Snowflake queries, and ensuring key tabs re-fetch data when focus changes.
Changes:
- Updated shared focus-program types/options and added
FOCUS_TO_CLASSIFICATION+VALID_CLASSIFICATIONSto align with new Snowflake classification values. - Added optional
classificationquery param support to multiple marketing-impact analytics endpoints (client → controller validation → Snowflake SQL filtering). - Updated Attribution and Performance Marketing tabs to re-fetch on focus changes and removed “not yet available at program level” banners.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/interfaces/marketing-impact.interface.ts | Updates MarketingImpactFocusProgram union to the new focus IDs used across the app. |
| packages/shared/src/constants/marketing-impact.constants.ts | Updates focus pill options and maps focus IDs to Snowflake classification strings; adds VALID_CLASSIFICATIONS. |
| apps/lfx-one/src/server/services/project.service.ts | Adds optional classification filtering to Snowflake queries for social reach, attribution, brand reach, and revenue impact (selectively where supported). |
| apps/lfx-one/src/server/controllers/analytics.controller.ts | Accepts/validates classification query param and forwards it to ProjectService for the updated endpoints. |
| apps/lfx-one/src/app/shared/services/analytics.service.ts | Adds optional classification query param plumbing to client analytics requests. |
| apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.ts | Re-fetches performance marketing data on focus changes and passes mapped classification. |
| apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.html | Removes focus-level “not yet available” info banner. |
| apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.ts | Passes mapped classification into Overview tab’s getRevenueImpact and getBrandReach. |
| apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.ts | Re-fetches attribution data on focus changes and passes mapped classification. |
| apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.html | Removes focus-level “not yet available” info banner. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/lfx-one/src/server/services/project.service.ts (1)
5062-5070:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAggregate
PAID_ADS_ATTRIBUTIONon the single-foundation path.Once
LF_SUB_DOMAIN_CLASSIFICATIONis part of this model, the non-umbrella query can return multiple rows for a foundation whenclassificationis omitted. This method still consumes the first row only, so the “All programs” Revenue Impact view can underreport spend/revenue by dropping the other classifications.Proposed fix
: ` - SELECT TOTAL_SPEND_YTD, TOTAL_IMPRESSIONS_YTD, TOTAL_CLICKS_YTD, - LINEAR_ROAS_YTD, AVG_CPC_YTD, CTR_YTD, CONVERSION_RATE_YTD, - FIRST_TOUCH_REVENUE_YTD, LAST_TOUCH_REVENUE_YTD, - LINEAR_REVENUE_YTD, TIME_DECAY_REVENUE_YTD, - SPEND_YOY_CHANGE_PCT, IMPRESSIONS_YOY_CHANGE_PCT + SELECT SUM(TOTAL_SPEND_YTD) AS TOTAL_SPEND_YTD, + SUM(TOTAL_IMPRESSIONS_YTD) AS TOTAL_IMPRESSIONS_YTD, + SUM(TOTAL_CLICKS_YTD) AS TOTAL_CLICKS_YTD, + CASE + WHEN SUM(TOTAL_SPEND_YTD) > 0 THEN SUM(LINEAR_REVENUE_YTD) / SUM(TOTAL_SPEND_YTD) + ELSE 0 + END AS LINEAR_ROAS_YTD, + AVG(AVG_CPC_YTD) AS AVG_CPC_YTD, + AVG(CTR_YTD) AS CTR_YTD, + AVG(CONVERSION_RATE_YTD) AS CONVERSION_RATE_YTD, + SUM(FIRST_TOUCH_REVENUE_YTD) AS FIRST_TOUCH_REVENUE_YTD, + SUM(LAST_TOUCH_REVENUE_YTD) AS LAST_TOUCH_REVENUE_YTD, + SUM(LINEAR_REVENUE_YTD) AS LINEAR_REVENUE_YTD, + SUM(TIME_DECAY_REVENUE_YTD) AS TIME_DECAY_REVENUE_YTD, + AVG(SPEND_YOY_CHANGE_PCT) AS SPEND_YOY_CHANGE_PCT, + AVG(IMPRESSIONS_YOY_CHANGE_PCT) AS IMPRESSIONS_YOY_CHANGE_PCT FROM ANALYTICS.PLATINUM_LFX_ONE.PAID_ADS_ATTRIBUTION WHERE FOUNDATION_SLUG = ? ${classificationFilter} `;Also applies to: 5217-5217
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/server/services/project.service.ts` around lines 5062 - 5070, The query against ANALYTICS.PLATINUM_LFX_ONE.PAID_ADS_ATTRIBUTION must aggregate rows for a foundation when classification is omitted; change the SELECT to use SUM(...) for raw totals (TOTAL_SPEND_YTD, TOTAL_IMPRESSIONS_YTD, TOTAL_CLICKS_YTD, FIRST_TOUCH_REVENUE_YTD, LAST_TOUCH_REVENUE_YTD, LINEAR_REVENUE_YTD, TIME_DECAY_REVENUE_YTD, SPEND_YOY_CHANGE_PCT, IMPRESSIONS_YOY_CHANGE_PCT as appropriate) and compute derived rates from those sums (e.g., CTR_YTD = SUM(TOTAL_CLICKS_YTD)/NULLIF(SUM(TOTAL_IMPRESSIONS_YTD),0), AVG_CPC_YTD = SUM(TOTAL_SPEND_YTD)/NULLIF(SUM(TOTAL_CLICKS_YTD),0), CONVERSION_RATE_YTD and LINEAR_ROAS_YTD similarly) so the single-foundation path doesn’t drop other classification rows; keep using the existing classificationFilter parameter but ensure the SQL includes GROUP BY FOUNDATION_SLUG (or performs aggregation unconditionally) and apply the same change at the other occurrence referenced around line 5217.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/lfx-one/src/server/services/project.service.ts`:
- Around line 5062-5070: The query against
ANALYTICS.PLATINUM_LFX_ONE.PAID_ADS_ATTRIBUTION must aggregate rows for a
foundation when classification is omitted; change the SELECT to use SUM(...) for
raw totals (TOTAL_SPEND_YTD, TOTAL_IMPRESSIONS_YTD, TOTAL_CLICKS_YTD,
FIRST_TOUCH_REVENUE_YTD, LAST_TOUCH_REVENUE_YTD, LINEAR_REVENUE_YTD,
TIME_DECAY_REVENUE_YTD, SPEND_YOY_CHANGE_PCT, IMPRESSIONS_YOY_CHANGE_PCT as
appropriate) and compute derived rates from those sums (e.g., CTR_YTD =
SUM(TOTAL_CLICKS_YTD)/NULLIF(SUM(TOTAL_IMPRESSIONS_YTD),0), AVG_CPC_YTD =
SUM(TOTAL_SPEND_YTD)/NULLIF(SUM(TOTAL_CLICKS_YTD),0), CONVERSION_RATE_YTD and
LINEAR_ROAS_YTD similarly) so the single-foundation path doesn’t drop other
classification rows; keep using the existing classificationFilter parameter but
ensure the SQL includes GROUP BY FOUNDATION_SLUG (or performs aggregation
unconditionally) and apply the same change at the other occurrence referenced
around line 5217.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 95ec9653-f952-4e95-95b4-946170961228
📒 Files selected for processing (10)
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.htmlapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.tsapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.tsapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.htmlapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.tsapps/lfx-one/src/app/shared/services/analytics.service.tsapps/lfx-one/src/server/controllers/analytics.controller.tsapps/lfx-one/src/server/services/project.service.tspackages/shared/src/constants/marketing-impact.constants.tspackages/shared/src/interfaces/marketing-impact.interface.ts
💤 Files with no reviewable changes (2)
- apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.html
- apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.html
- validation.helper.ts: extract getValidatedClassification() helper to deduplicate classification parsing across 6 controller actions (per copilot[bot]) - analytics.service.ts: extract buildFoundationParams() helper to deduplicate param construction across 6 methods (per copilot[bot]) - project.service.ts: aggregate PAID_ADS_ATTRIBUTION non-umbrella query with SUM/CASE for derived rates to prevent row-dropping when classification is omitted (per coderabbit[bot]); also fix umbrella query to compute ROAS/CTR/CPC from sums instead of naive AVG Resolves 3 review threads. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Review Feedback AddressedCommit: 58500c0 Changes Made
Threads Resolved2 of 2 inline threads addressed. CodeRabbit out-of-diff finding (PAID_ADS_ATTRIBUTION aggregation) also addressed in this commit. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/lfx-one/src/server/helpers/validation.helper.ts (1)
148-156: ⚡ Quick winAdd a JSDoc block for the exported classification validator.
getValidatedClassificationis exported and should include a short JSDoc for params/return/throw behavior.As per coding guidelines "Add JSDoc comments for public functions and exported modules".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/server/helpers/validation.helper.ts` around lines 148 - 156, Add a JSDoc comment block above the exported function getValidatedClassification describing its purpose, parameters, return value, and throws behavior: document that it accepts (req: Request) and (operation: string), returns a string | undefined for a validated classification, and throws ServiceValidationError.forField when the classification is present but not in VALID_CLASSIFICATIONS; also mention it reads the query via getStringQueryParam and list the allowed values (or reference VALID_CLASSIFICATIONS) in the description.apps/lfx-one/src/app/shared/services/analytics.service.ts (1)
901-902: ⚡ Quick winKeep JSDoc aligned with the new
classificationparameter.These public methods now accept
classification, but their docs still only describefoundationSlug. Please add@param classificationto each updated method docblock.As per coding guidelines "Add JSDoc comments for public functions and exported modules".
Also applies to: 1208-1209, 1256-1257, 1281-1283
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/app/shared/services/analytics.service.ts` around lines 901 - 902, The JSDoc for the public analytics methods that now accept an optional classification parameter (for example getSocialReach) is missing `@param` for classification; update each affected method's docblock (including the docblocks at the other changed locations) to add a line like "`@param` classification Optional string to filter results by classification" (or similar wording consistent with existing docs), ensuring the type and optionality are specified and the description explains how the parameter is used; apply this to getSocialReach and the other public methods that were changed in the same file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/lfx-one/src/app/shared/services/analytics.service.ts`:
- Around line 901-902: The JSDoc for the public analytics methods that now
accept an optional classification parameter (for example getSocialReach) is
missing `@param` for classification; update each affected method's docblock
(including the docblocks at the other changed locations) to add a line like
"`@param` classification Optional string to filter results by classification" (or
similar wording consistent with existing docs), ensuring the type and
optionality are specified and the description explains how the parameter is
used; apply this to getSocialReach and the other public methods that were
changed in the same file.
In `@apps/lfx-one/src/server/helpers/validation.helper.ts`:
- Around line 148-156: Add a JSDoc comment block above the exported function
getValidatedClassification describing its purpose, parameters, return value, and
throws behavior: document that it accepts (req: Request) and (operation:
string), returns a string | undefined for a validated classification, and throws
ServiceValidationError.forField when the classification is present but not in
VALID_CLASSIFICATIONS; also mention it reads the query via getStringQueryParam
and list the allowed values (or reference VALID_CLASSIFICATIONS) in the
description.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 850ae845-f611-4da6-be55-666dc5b15159
📒 Files selected for processing (4)
apps/lfx-one/src/app/shared/services/analytics.service.tsapps/lfx-one/src/server/controllers/analytics.controller.tsapps/lfx-one/src/server/helpers/validation.helper.tsapps/lfx-one/src/server/services/project.service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/lfx-one/src/server/services/project.service.ts
- project.service.ts: fix getSocialReach JSDoc to reference only PAID_SOCIAL_REACH_BY_PROJECT_CHANNEL_MONTH (per copilot[bot]) - analytics.service.ts: update classification param examples from 'Events'/'Corporate' to 'LF Events'/'LF Corporate' (per copilot[bot]) Resolves 3 review threads. 2 threads about monthlyTrendQuery on PAID_SOCIAL_REACH_BY_PROJECT_MONTH are false positives — that table now has LF_SUB_DOMAIN_CLASSIFICATION per data team update. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Review Feedback Addressed — Cycle 2Commit: d792b7e Changes Made
No Change Needed
Threads Resolved5 of 5 unresolved threads addressed (3 code fixes, 2 false positives). |
Address coderabbitai[bot] nitpick comments: - validation.helper.ts: add JSDoc to getValidatedClassification() - analytics.service.ts: add @param classification to getSocialReach, getBrandReach, getRevenueImpact, getMarketingAttribution JSDocs LFXV2-1972 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Review Feedback AddressedCommit: 72dbade Changes Made
No Change Needed
Threads ResolvedAll 7 threads were already resolved in prior iterations. This commit addresses 2 nitpick items from the coderabbitai[bot] review body. |
Update MarketingImpactFocusProgram type and FOCUS_TO_CLASSIFICATION mapping to match new Snowflake LF_SUB_DOMAIN_CLASSIFICATION values (LF Corporate, LF Events, LF Training, LFX Platform, Project Websites). Add classification query param support to getSocialReach, getBrandReach, getRevenueImpact, and getMarketingAttribution endpoints. Wire Attribution and Performance Marketing tabs with combineLatest pattern to re-fetch on focus change; pass classification to getRevenueImpact and getBrandReach in Overview tab. Remove "not yet available" info banners from Attribution and Performance Marketing templates. Apply classification filter selectively — only to Snowflake tables that have the column. LFXV2-1972 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
PAID_SOCIAL_REACH_BY_PROJECT_MONTH now has LF_SUB_DOMAIN_CLASSIFICATION. Add GROUP BY aggregation and classification WHERE clause to the monthlyTrendQuery in getRevenueImpact so the paid media monthly trend respects the focus filter. LFXV2-1972 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
- validation.helper.ts: extract getValidatedClassification() helper to deduplicate classification parsing across 6 controller actions (per copilot[bot]) - analytics.service.ts: extract buildFoundationParams() helper to deduplicate param construction across 6 methods (per copilot[bot]) - project.service.ts: aggregate PAID_ADS_ATTRIBUTION non-umbrella query with SUM/CASE for derived rates to prevent row-dropping when classification is omitted (per coderabbit[bot]); also fix umbrella query to compute ROAS/CTR/CPC from sums instead of naive AVG Resolves 3 review threads. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
- project.service.ts: fix getSocialReach JSDoc to reference only PAID_SOCIAL_REACH_BY_PROJECT_CHANNEL_MONTH (per copilot[bot]) - analytics.service.ts: update classification param examples from 'Events'/'Corporate' to 'LF Events'/'LF Corporate' (per copilot[bot]) Resolves 3 review threads. 2 threads about monthlyTrendQuery on PAID_SOCIAL_REACH_BY_PROJECT_MONTH are false positives — that table now has LF_SUB_DOMAIN_CLASSIFICATION per data team update. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Address coderabbitai[bot] nitpick comments: - validation.helper.ts: add JSDoc to getValidatedClassification() - analytics.service.ts: add @param classification to getSocialReach, getBrandReach, getRevenueImpact, getMarketingAttribution JSDocs LFXV2-1972 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
64168de to
5414922
Compare
| export function getValidatedClassification(req: Request, operation: string): string | undefined { | ||
| const classification = getStringQueryParam(req, 'classification'); | ||
| if (classification && !VALID_CLASSIFICATIONS.has(classification)) { | ||
| throw ServiceValidationError.forField('classification', `Invalid classification value. Allowed: ${[...VALID_CLASSIFICATIONS].join(', ')}`, { | ||
| operation, | ||
| }); | ||
| } | ||
| return classification; | ||
| } |
| throw ServiceValidationError.forField('classification', `Invalid classification value. Allowed: ${[...VALID_CLASSIFICATIONS].join(', ')}`, { | ||
| operation, | ||
| }); |
| FROM ANALYTICS.PLATINUM_LFX_ONE.PAID_SOCIAL_REACH_BY_PROJECT_MONTH | ||
| WHERE CAMPAIGN_MONTH >= DATE_TRUNC('month', DATEADD(month, -6, CURRENT_DATE())) | ||
| AND CAMPAIGN_MONTH < DATE_TRUNC('month', CURRENT_DATE()) | ||
| ${classificationFilter} | ||
| GROUP BY CAMPAIGN_MONTH | ||
| ORDER BY CAMPAIGN_MONTH DESC | ||
| LIMIT 6 |
| import type { AttributionModelOption, MarketingImpactFocusProgram, MarketingImpactTabOption } from '../interfaces/marketing-impact.interface'; | ||
|
|
||
| /** Focus program filter options for the Marketing Impact FOCUS bar. */ | ||
| /** Focus program filter options for the Marketing Impact FOCUS bar. Labels match Snowflake LF_SUB_DOMAIN_CLASSIFICATION values. */ |
Summary
MarketingImpactFocusProgramtype andFOCUS_TO_CLASSIFICATIONmapping to match new SnowflakeLF_SUB_DOMAIN_CLASSIFICATIONvalues: LF Corporate, LF Events, LF Training, LFX Platform, Project Websitesclassificationquery param support togetSocialReach,getBrandReach,getRevenueImpact, andgetMarketingAttribution(service → controller → Snowflake SQL)combineLatest([slug$, focus$])to re-fetch on focus change; pass classification togetRevenueImpactandgetBrandReachin Overview tabPIPELINE_SUMMARYdoes not; all other models includingPAID_SOCIAL_REACH_BY_PROJECT_MONTHnow support it)Context
Follows 7 data-side PRs (merged May 18–26) that added
LF_SUB_DOMAIN_CLASSIFICATIONto 5 Platinum Snowflake models. This PR completes the UI wiring so the Focus filter works across all Marketing Impact dashboard tabs.Test plan
LFXV2-1972
🤖 Generated with Claude Code