feat(campaigns): add linkedin ad accounts and set default#911
feat(campaigns): add linkedin ad accounts and set default#911mrautela365 wants to merge 1 commit into
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:
WalkthroughLinkedIn Ads defaults and account list are added; the Ads service now reads optional env variables and falls back to the new defaults, auto-resolving org IDs from the expanded account list when a non-default account is selected. ChangesLinkedIn default account/org fallback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
There was a problem hiding this comment.
Pull request overview
Adds additional server-side LinkedIn ad account metadata and introduces a default ad account/org fallback so the LinkedIn ads service can run without requiring LINKEDIN_AD_ACCOUNT_ID / LINKEDIN_ORG_ID to be set.
Changes:
- Added default LinkedIn ad account + org IDs, and expanded the server-side list of supported LinkedIn ad accounts.
- Updated LinkedIn ads service to use defaults when
LINKEDIN_AD_ACCOUNT_ID/LINKEDIN_ORG_IDare not provided via environment.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/lfx-one/src/server/constants/linkedin.constants.ts | Adds default account/org constants and expands the server-side LinkedIn ad accounts list. |
| apps/lfx-one/src/server/services/linkedin-ads.service.ts | Uses default account/org IDs when env vars are not set. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/lfx-one/src/server/services/linkedin-ads.service.ts (1)
40-46: ⚡ Quick winAdd logging to indicate when defaults vs environment variables are used.
The fallback to default account/org IDs is a significant behavioral change from the previous required-env-var pattern (via
getLinkedInEnv). Without logging, operators cannot easily determine which account is being used in a given deployment, making debugging and auditing difficult.🔍 Proposed fix to add observability
function getAccountId(): string { - return process.env['LINKEDIN_AD_ACCOUNT_ID'] || LINKEDIN_DEFAULT_ACCOUNT_ID; + const envValue = process.env['LINKEDIN_AD_ACCOUNT_ID']; + if (envValue) { + logger.debug(undefined, 'linkedin_config', `Using LinkedIn account ID from env: ${envValue}`); + return envValue; + } + logger.debug(undefined, 'linkedin_config', `Using default LinkedIn account ID: ${LINKEDIN_DEFAULT_ACCOUNT_ID}`); + return LINKEDIN_DEFAULT_ACCOUNT_ID; } function getOrgId(): string { - return process.env['LINKEDIN_ORG_ID'] || LINKEDIN_DEFAULT_ORG_ID; + const envValue = process.env['LINKEDIN_ORG_ID']; + if (envValue) { + logger.debug(undefined, 'linkedin_config', `Using LinkedIn org ID from env: ${envValue}`); + return envValue; + } + logger.debug(undefined, 'linkedin_config', `Using default LinkedIn org ID: ${LINKEDIN_DEFAULT_ORG_ID}`); + return LINKEDIN_DEFAULT_ORG_ID; }🤖 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/linkedin-ads.service.ts` around lines 40 - 46, Update getAccountId and getOrgId to log which source (env var vs default) and the actual ID chosen so operators can see when fallbacks are used; read process.env['LINKEDIN_AD_ACCOUNT_ID'] / process.env['LINKEDIN_ORG_ID'], choose the value as now, and call the module's logger (e.g., processLogger or the file's existing logger) to emit an info-level message like "Using LinkedIn account ID from ENV" vs "Using default LinkedIn account ID" including the chosen ID; ensure both functions log before returning the ID.
🤖 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/server/services/linkedin-ads.service.ts`:
- Around line 40-46: Update getAccountId and getOrgId to log which source (env
var vs default) and the actual ID chosen so operators can see when fallbacks are
used; read process.env['LINKEDIN_AD_ACCOUNT_ID'] /
process.env['LINKEDIN_ORG_ID'], choose the value as now, and call the module's
logger (e.g., processLogger or the file's existing logger) to emit an info-level
message like "Using LinkedIn account ID from ENV" vs "Using default LinkedIn
account ID" including the chosen ID; ensure both functions log before returning
the ID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1731a390-bb91-4d91-bcd5-3ac6f1f874f2
📒 Files selected for processing (2)
apps/lfx-one/src/server/constants/linkedin.constants.tsapps/lfx-one/src/server/services/linkedin-ads.service.ts
Address review comments from copilot-pull-request-reviewer, coderabbitai[bot]: - linkedin-ads.service.ts: auto-resolve org ID from LINKEDIN_ACCOUNTS when ad account is overridden via env var, preventing account/org mismatch (per Copilot) - linkedin-ads.service.ts: add debug logging to getAccountId() and getOrgId() for deployment observability (per CodeRabbit) LFXV2-2180 Resolves 1 review thread. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Review Feedback AddressedCommit: c84e56e Changes Made
Threads Resolved1 of 1 unresolved threads addressed. CodeRabbit nitpick also addressed in the same commit. |
Address review comments from copilot-pull-request-reviewer, coderabbitai[bot]: - linkedin-ads.service.ts: auto-resolve org ID from LINKEDIN_ACCOUNTS when ad account is overridden via env var, preventing account/org mismatch (per Copilot) - linkedin-ads.service.ts: add debug logging to getAccountId() and getOrgId() for deployment observability (per CodeRabbit) LFXV2-2180 Resolves 1 review thread. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
5e9e549 to
25777da
Compare
Review Feedback AddressedNo Change Needed
Threads Resolved2 of 2 unresolved threads addressed — both were based on a stale view of the code; the current implementation already follows the suggested patterns. |
Address review comments from copilot-pull-request-reviewer, coderabbitai[bot]: - linkedin-ads.service.ts: auto-resolve org ID from LINKEDIN_ACCOUNTS when ad account is overridden via env var, preventing account/org mismatch (per Copilot) - linkedin-ads.service.ts: add debug logging to getAccountId() and getOrgId() for deployment observability (per CodeRabbit) LFXV2-2180 Resolves 1 review thread. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
0dd9959 to
1adedca
Compare
MRashad26
left a comment
There was a problem hiding this comment.
Automated review + code-standards-enforcer audit (report-only). do-not-merge label — review only.
🔒 Secrets / sensitive-data check (requested)
No secrets exposed. No credentials, tokens, or keys are added in this diff. The LinkedIn access token remains env-sourced via getAccessToken() → getLinkedInEnv('LINKEDIN_ACCESS_TOKEN') (untouched). The added values are ad-account IDs and org IDs — non-credential identifiers (org IDs are public LinkedIn company URNs; account IDs are inert without the access token). They are correctly placed in the server-only linkedin.constants.ts, whose header states they "must NOT ship to the client bundle" — so they stay out of the browser bundle. They are now committed to a public repo, which is acceptable since none are secrets.
Overview
Adds 9 LinkedIn ad accounts to LINKEDIN_ACCOUNTS, introduces LINKEDIN_DEFAULT_ACCOUNT_ID/LINKEDIN_DEFAULT_ORG_ID (LF Events / 208777), and changes getAccountId()/getOrgId() from throw-on-missing-env to default-with-fallback, auto-resolving org from account when overridden.
Code Standards Audit
- 🔴 Critical: none
- 🟡 Warning: none
- 🔵 Info: local
LinkedInAdAccountConfiginterface vs the "interfaces live in@lfx-one/shared" rule (inline — with the server-only nuance); account/org mismatch on unknown-account override now proceeds with only a warn (inline);LINKEDIN_DEFAULT_ORG_IDduplicates the default account's orgId (inline); PR body has the🤖 Generated with [Claude Code]footer (commits themselves are clean — signed-off, no AI co-author ✅); no## Test plansection ✅. - ✅ Correctly followed: server-only constants kept out of shared/client per the file's documented architecture, MIT header,
logger.warning/logger.debug(noconsole.log), all lines ≤160, signed-off commits.
Test coverage
No tests added. getOrgId() now has a real resolution matrix (env set / known account / unknown account / default) but the functions are module-private, so they aren't directly testable without export. If this logic is worth locking down, consider exporting a small resolveLinkedInOrg(accountId, env) helper and unit-testing the four branches.
Verdict: PASS (code) — gated by the do-not-merge label.
Add 9 LinkedIn ad accounts (CNCF, LF Education, Agentic AI Foundation, OpenSearch x2, OpenJS x2, LF Agentic AI, OpenSSF) to both shared and server-side account lists. Set LF Events (509430019) as default account when LINKEDIN_AD_ACCOUNT_ID env var is not set. Derive LINKEDIN_DEFAULT_ORG_ID from the account lookup to avoid drift. Update resolveOrgId to check env var override, then shared accounts, then server accounts, then default. Addresses review feedback: - Fail-fast on unknown override (Rashad) - Derive default org from account lookup instead of hardcoding (Rashad) - Keep interface in shared package per convention (Rashad) Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
1adedca to
7e75593
Compare
Review Feedback AddressedCommit: 7e75593 Rebased on main (post #902 + #917 merges) and addressed all 3 review comments. Changes Made
Threads Resolved3 of 3 unresolved threads addressed. |
Summary
LINKEDIN_AD_ACCOUNT_IDoverrides when setJIRA
LFXV2-2180 — child of LFXV2-2023
Files changed
apps/lfx-one/src/server/constants/linkedin.constants.ts— added accounts + default constantsapps/lfx-one/src/server/services/linkedin-ads.service.ts— use default fallback for account/org ID🤖 Generated with Claude Code