feat(dashboards): add education card and drawer to marketing overview - #1324
feat(dashboards): add education card and drawer to marketing overview#1324mrautela365 wants to merge 8 commits into
Conversation
LFXV2-2023 Website sessions were the second dual-signal on the Social card, pairing a stock (followers) with a flow (sessions) in one tile. Split them: Social becomes followers-only and a new Web card carries the rolling 30-day sessions between Social and Email. The Web card reuses totalMonthlySessions, weeklyTrend and sessionMomChangePct already on BrandReachResponse, so no new endpoint is needed, and wires the previously orphaned MarketingWebsiteVisits drawer type so the existing website-visits drawer is reachable from a card. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
LFXV2-2023 Address post-commit review findings: - Subtitle read 'Sessions (30d) · Last 6 months', implying the 30-day value covered six months. The value and the sparkline are different series from different tables, so label the trend window explicitly. - Social's sparkline comment said 'flat line'; flatSparklineData adds a 2% nudge because a constant array collapses Chart.js's Y range. Describe it as a placeholder rather than a trend. - The Website Visits Drawer e2e suite opened 'marketing-card-website-visits', a testId no longer present in source, so it could not pass. Point it at the new Web card and add a render test for the card. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
LFXV2-2023 The Marketing Metric Cards and drawer suites referenced three testIds absent from source since the ED overview restructure: website-visits, email-ctr and social-media. Repoint them at the ed-evo-* ids the cards actually render, drop the duplicate Website Visits render test now that the Web card covers it, and skip the Social Media Drawer suite since no card sets drawerType MarketingSocialMedia, leaving that drawer unreachable from the UI. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
LFXV2-2023 The social-media drawer was rendered by marketing-overview but no card set drawerType MarketingSocialMedia, so it could not be opened from the UI. The enum member's only remaining use was the template binding that kept the dead component mounted, making it a closed loop with no external consumer. Removes the component, its import and imports entry, the template binding, the enum member and the e2e suite that targeted a card deleted in #1300. The /social-media endpoints, controller, project.service methods and SocialMediaResponse are deliberately kept: the Marketing Impact social-accounts-tab still consumes them. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
…#1310) * feat(dashboards): add education card and drawer to marketing overview LFXV2-2023 Adds an Education card in the second carousel slot, directly after Events, and an Education drawer behind it. Reuses the Health Metrics training-certification endpoint so the two surfaces cannot disagree on enrollment or revenue figures. ENROLLMENTS is a pre-aggregated wide table with the reporting range baked into the column names and no date column, so no monthly grain exists. The sparkline is deliberately flat and the subtitle names the range rather than implying a trend. The analytics service converts HTTP errors into an all-zeros response, a contract the Health Metrics card depends on. Rather than change it, the call site maps that shape (identified by an empty projectId) back to undefined so a failed request stays distinguishable from a foundation with genuinely no training. Both suppress the card, but only real data can render it. Net revenue excludes edX, which contributes enrollments but carries no revenue. The card is suppressed entirely when total enrollments are zero. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org> * fix(dashboards): address education card review findings - Set the Education card category to 'education' rather than 'memberships'. The 'memberships' filter pill is labelled "North Star" and the carousel filters on exact category equality, so Education was listed as a North Star metric. - Gate the Net Revenue stat on revenue-bearing enrollments instead of a non-zero total, so a measured $0 renders as $0. The em dash is reserved for the all-edX case where revenue is genuinely untracked upstream. - Report the leading and highest-earning formats as neutral facts instead of insights. splitByPriority routes every non-warning insight into "Performing Well", which praised the same format the concentration-risk action was flagging. - Correct the carousel sequence comment (Web was missing) and drop the inaccurate zero-enrollment resilience claim on the e2e card test. LFXV2-2023 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org> * fix(review): address education drawer review findings Replace three max-finding sorts with a single-pass maxBy helper: the sorts implied an ordering nothing consumed. Add e2e coverage for the null-vs-zero revenue distinction (edX renders 'not tracked', measured formats render currency) and for the priority-routed insight sections, neither of which had assertions on rendered content. LFXV2-2023 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org> * fix(dashboards): gate education card revenue on tracked state The card subtitle rendered formatCurrency(educationTotalRevenue) unconditionally. Since edX carries no revenue column upstream and is excluded from the sum, an all-edX foundation read "$0.00 net revenue" on the card while the drawer it opens showed an em dash for the same figure — reporting untracked as measured-zero, the exact failure the drawer's hasTrackedRevenue signal exists to prevent. Gate the subtitle on revenue-bearing enrollments, mirroring the drawer, so $0 still renders as $0 when it is a real measurement. Add an e2e test asserting the card and drawer never disagree about whether revenue is tracked, and one asserting no format is flagged as a concentration risk and praised as performing well simultaneously. Score each row once in maxBy instead of re-deriving the incumbent's score on every comparison. LFXV2-2023 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org> * test(dashboards): pin education branches with API fixtures The education e2e tests asserted invariants against live TLF data, which never reaches the untracked-revenue or concentration-risk branches. A rendering regression in either would have passed unnoticed. Adds a stubbed training-certification-summary route with four fixtures (balanced, all-edX, measured-zero, concentrated) so each branch is exercised deterministically. The route is registered before page.goto because the card is built during SSR. Keeps the live-data invariant tests, which still catch integration breakage that mocks cannot. LFXV2-2023 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org> * docs(dashboards): correct e2e suite card inventory The header listed Website Visits, Email CTR and Paid Social Reach, none of which are current card titles, and omitted Social, Web and Education which the suite does cover. Replaces it with the actual inventory and documents why live-data and fixture-driven suites both exist. LFXV2-2023 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org> --------- Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
…nd-social-base Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org> # Conflicts: # apps/lfx-one/e2e/marketing-dashboard.spec.ts # packages/shared/src/constants/dashboard-metrics.constants.ts
PR SummaryMedium Risk Overview The carousel card shows total enrollments and net revenue when revenue is tracked (instructor-led, eLearning, or cert exams present); otherwise the subtitle says net revenue not tracked. The card is omitted when enrollments are zero or when the API error fallback (empty The new Education drawer surfaces enrollments, net revenue, revenue per enrollment, format breakdown (edX revenue as not tracked), and priority-routed actions/insights. Leading formats are neutral facts so the same format is not both flagged under Needs Your Attention and praised in Performing Well. Shared types add Reviewed by Cursor Bugbot for commit 9b2dd30. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository 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:
Comment |
There was a problem hiding this comment.
Pull request overview
Adds an Education metric card and drill-down drawer to the ED Marketing Overview using existing training analytics.
Changes:
- Adds the conditional Education card and analytics wiring.
- Adds enrollment, revenue, format, and recommendation details.
- Adds live and fixture-driven Playwright coverage.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
dashboard-metric.interface.ts |
Adds the Education drawer type. |
analytics-data.interface.ts |
Adds Education view models and dashboard data. |
dashboard-metrics.constants.ts |
Builds and positions the Education card. |
marketing-overview.component.ts |
Fetches Education data and registers the drawer. |
marketing-overview.component.html |
Mounts the Education drawer. |
education-drawer.component.ts |
Implements drawer calculations and insights. |
education-drawer.component.html |
Renders Education metrics and breakdowns. |
marketing-dashboard.spec.ts |
Adds Education card and drawer E2E coverage. |
Review Feedback AddressedNo code changes in this iteration — the single review finding was validated against the codebase and closed with reasoning rather than a fix. No Change Needed
Threads Resolved1 of 1 unresolved threads addressed. No unanswered inline bot comments remain. StatusAll 12 CI checks passing. Awaiting code-owner review from |
…nd-social-base Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
apps/lfx-one/src/app/modules/dashboards/executive-director/components/education-drawer/education-drawer.component.html:56
- This label promises
totalRevenue / totalEnrollments, but the computed value deliberately divides by only instructor-led, eLearning, and cert-exam enrollments. Whenever edX has enrollments, the three adjacent figures no longer reconcile (for example, the balanced fixture shows $225K / 1K while this value is based on 850 enrollments). Label the denominator explicitly so EDs do not read this as revenue across all displayed enrollments.
<span class="text-sm text-gray-500">Revenue / Enrollment</span>
Carries the Education card and drawer to
main. This is the merge-forward of #1310, which merged into this branch rather thanmainbecause it was opened as a stacked PR on top of #1308 / #1309.LFXV2-2023
Why this PR exists
#1308 (web card split) and #1309 (dead social drawer removal) are both merged. #1310 was stacked on this base branch and merged into it — so its work landed here, not on
main, and this branch had no PR of its own. Without this, the education card never reachesmain.What's in it
Only the education work. The diff is 8 files, +882/−11:
education-drawer.component.ts/.html— new drawermarketing-overview.component.ts/.html— card wiringdashboard-metrics.constants.ts— card definition and carousel orderanalytics-data.interface.ts,dashboard-metric.interface.ts— typesmarketing-dashboard.spec.ts— e2e coverageBefore this branch was reconciled with the post-merge
main, the same comparison rendered as 11 files / +938/−709, re-presenting #1309'ssocial-media-drawerdeletions as though this PR were doing them. Merging the squashedmainin restored a single merge base and dropped them — they're already onmain.Conflict resolution
Merging the squashed
mainproduced two conflicts, both resolved to keep both sides' intent:marketing-dashboard.spec.ts—mainhas feat(dashboards): split web sessions into a dedicated overview card #1308's Web-card split assertions; this branch adds the Education card test. Both kept: the assertions close out the Web test, the Education test follows.dashboard-metrics.constants.ts— comment-only. The card-order comment mergedmain's note that display order interleaves categories with this branch's 11-card sequence and the note on why Education uses categoryeducationrather thanmemberships. The sparkline comment keptmain's explanation of the empty-weeklyTrendplaceholder.Resolutions are byte-identical to those reviewed on #1310's merge commit (
5ab8c7ef5).Verification
yarn check-types,yarn lint:check,yarn buildall passsocial-media-drawerreferencesmain; no dev-bypass files in the diffReview history
The content here was reviewed and approved on #1310 by @dealako. The only change since is the conflict resolution above.