fix: Add disclaimer about onboard status

[tkotthakota-adobe]

https://jira.corp.adobe.com/browse/SITES-41933

• add disclaimer about onboard status

Test:
https://cq-dev.slack.com/archives/C060T2PPF8V/p1774835480125529

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[github-actions]

This PR will trigger no release when merged.

[tkotthakota-adobe]

@rpapani
Can you get full mapping for these?

https://github.com/adobe/spacecat-task-processor/blob/main/src/tasks/opportunity-status-processor/audit-opportunity-map.js

https://github.com/adobe/spacecat-task-processor/blob/main/src/tasks/opportunity-status-processor/opportunity-dependency-map.js

[solaris007]

Hey @tkotthakota-adobe,

Strengths

• Conservative fallback on DB errors: catch block marks all remaining audit types as pending, preventing false-positive green statuses (handler.js:281-284)
• Hourglass gating skips unnecessary getSuggestions() calls when audit is pending - avoids stale data and reduces DB load (handler.js:645-647)
• Smart filtering of infrastructure audit types from disclaimer via getOpportunitiesForAudit(t).length > 0 (handler.js:744-746)
• isRecheck strict comparison (=== true) prevents truthy coercion from unexpected SQS payloads (handler.js:747)
• Thorough test coverage: 9 new test cases plus correctly updated existing tests

Issues

Important (Should Fix)

1. getOpportunityTitle receives audit types instead of opportunity types in disclaimer - handler.js:748

relevantPendingTypes contains audit type strings, but getOpportunityTitle is designed for opportunity types. For experimentation-opportunities, the disclaimer shows "Experimentation Opportunities" (kebab fallback) instead of the actual opportunity name. Multiple reviewers flagged this independently.

Fix: Map audit types to opportunity types first:

const pendingOpportunityNames = relevantPendingTypes
  .flatMap((t) => getOpportunitiesForAudit(t))
  .map(getOpportunityTitle);
const pendingList = [...new Set(pendingOpportunityNames)].join(', ');

2. auditedAt NaN silently treated as completed - handler.js:273-274

If audit.getAuditedAt() returns null or an unparseable string, new Date(value).getTime() returns NaN. NaN < onboardStartTime is false, so the audit lands in completedAuditTypes - the opposite of the conservative intent.

Fix: Add a NaN guard:

const auditedAt = new Date(audit.getAuditedAt()).getTime();
if (!onboardStartTime || Number.isNaN(auditedAt) || auditedAt < onboardStartTime) {
  pendingAuditTypes.push(auditType);
} else {
  completedAuditTypes.push(auditType);
}

3. Duplicate entries in pendingAuditTypes on mid-loop error - handler.js:281-284

If processing throws after some types are already pushed to pendingAuditTypes, the catch block's auditTypes.filter((t) => !completedAuditTypes.includes(t)) re-adds them, creating duplicates. The disclaimer would show repeated audit names.

Fix: Clear pendingAuditTypes before the fallback push:

pendingAuditTypes.length = 0;
pendingAuditTypes.push(...auditTypes.filter((t) => !completedAuditTypes.includes(t)));

4. Cross-repo duplication with companion PR #1986 - Both PRs implement audit completion checking with different anchor timestamps (onboardStartTime here vs lastAuditRunTime in api-service) and different data access patterns (Audit.allLatestForSite vs LatestAudit.allBySiteId). These can disagree about pending status as the system evolves.

Fix: Track via follow-up Jira ticket; extract shared logic to a shared package.

Minor (Nice to Have)

5. Redundant onboardStartTime guard - handler.js:273 - The caller already ensures onboardStartTime is truthy. The inner check is harmless but adds confusion about the function's contract.

6. auditedAt === onboardStartTime boundary - handler.js:273 - When timestamps match exactly, the audit is classified as "completed." Worth documenting the intended behavior with a test case.

7. False "All audits completed" when onboardStartTime is falsy + isRecheck is true - If onboardStartTime is falsy but auditTypes.length > 0 and isRecheck is true, the DB check is skipped, pendingAuditTypes stays empty, and the "All audits completed" message fires even though no check was performed.

Recommendations

• Prioritize fixes #1-#3 - they are correctness bugs that surface under plausible conditions.
• Consider adding a timeout/expiry to the "pending" state. If an audit fails silently (never writes a DB record), the hourglass shows indefinitely.
• Add a test for the onboardStartTime = undefined + isRecheck = true path to lock down the expected behavior.
• Create a follow-up Jira ticket to extract shared audit-opportunity mapping, title functions, and pending-audit logic into a shared package.

Assessment

Ready to merge? With fixes

The core logic is sound and well-tested. Issues 1 (audit-type naming mismatch), 2 (NaN-as-completed), and 3 (duplicate entries) are correctness bugs that should be fixed before merge. Issue 4 (cross-repo duplication) can be a tracked follow-up.

Next Steps

1. Fix the getOpportunityTitle audit-type vs opportunity-type mismatch (#1).
2. Add NaN guard in checkAuditCompletionFromDB (#2).
3. Fix duplicate entries in catch fallback (#3).
4. Create a follow-up Jira ticket for shared package extraction (#4).
5. Minor items are optional.

[tkotthakota-adobe]

@solaris007 Addressed review comments

  1. getOpportunityTitle called on audit types — Disclaimer now maps audit types → opportunity types → titles via                                  
  flatMap(getOpportunitiesForAudit).map(getOpportunityTitle) with dedup via Set.
  2. auditedAt NaN treated as completed — Added Number.isNaN(auditedAt) guard so unparseable timestamps are treated as pending, not silently       
  completed.                                                                                                                                       
  3. Duplicate entries in catch fallback — Added pendingAuditTypes.length = 0 before the fallback push to prevent duplicates when the error occurs
  mid-loop.                                                                                                                                        
                                                            
  Acknowledged as non-issues (no change needed):                                                                                                   
                                                            
  4. Redundant onboardStartTime guard — Removed the inner !onboardStartTime check since the caller always provides it.                             
  5. auditedAt === onboardStartTime boundary — Not a real concern; there will always be a delay between onboard start and audit completion, so
  exact equality won't occur in practice.                                                                                                          
  6. False "All audits completed" with falsy onboardStartTime — onboardStartTime is always sent, so this scenario can't happen. isRecheck guard
  restored to its original form.    

[tkotthakota-adobe]

@solaris007 Thank you for the valuable review comments. Addressed them. Tests results are added to description.

  1. checkAuditCompletionFromDB removed — replaced by shared pure 
  function                                                               
  The local async DB-querying function has been deleted. In its place,
  the handler now fetches audits explicitly and delegates classification 
  to the shared computeAuditCompletion:                     
  import { ..., computeAuditCompletion } from                            
  '@adobe/spacecat-shared-utils';                           
                                 
  const latestAudits = await Audit.allLatestForSite(siteId);
  const completion = computeAuditCompletion(auditTypes, onboardStartTime,
   latestAudits);                                                        
  pendingAuditTypes = completion.pendingAuditTypes;                      
                                                            
  2. onboardStartTime continues to come from the SQS message             
  The task-processor still reads onboardStartTime from taskContext (the  
  SQS message), not from the DB. This is the same value the api-service  
  computed with Date.now() at onboard time and stored as                 
  onboardConfig.lastStartTime — consistent anchor, different source per  
  consumer by design.                                       

  3. Error handling preserved with conservative fallback
  On DB failure, pendingAuditTypes is set to all auditTypes (show
  everything as pending), matching the original behaviour.               
   
  4. Comparison semantics unified (<=)                                   
  The old local function used < (strict). The shared function uses <= —
  an exact match at the boundary is treated as pending (in-flight audit  
  captured before the run completed). This is the more correct behaviour
  and is now consistent across both consumers.                           
                                                            
  5. AUDIT_OPPORTUNITY_MAP, getOpportunityTitle, getAuditsForOpportunity 
  all from shared-utils
  No local copies of opportunity mapping logic — all imported from       
  @adobe/spacecat-shared-utils.   

[solaris007]

Hey @tkotthakota-adobe,

Strong rework - the shared-utils extraction is clean and all major findings from the prior review are resolved.

Previously Flagged Issues - Resolution Status

All 4 important and 3 minor findings from the prior review are addressed:

• Important #1 (audit types passed to getOpportunityTitle): Resolved - disclaimer now correctly chains getOpportunitiesForAudit(t) before getOpportunityTitle().
• Important #2 (auditedAt NaN as completed): Resolved - computeAuditCompletion in shared-utils explicitly checks Number.isNaN() and treats NaN as pending.
• Important #3 (duplicate entries on mid-loop error): Resolved - per-audit-type loop replaced with single DB call + pure function. Catch block does pendingAuditTypes = [...auditTypes] (fresh copy, no accumulation).
• Important #4 (cross-repo duplication): Resolved - local audit-opportunity-map.js and opportunity-dependency-map.js deleted, all mapping logic now in @adobe/spacecat-shared-utils@1.107.0.
• Minor #5-#6 (redundant guard, boundary): Resolved - guard justified as optimization, computeAuditCompletion documents the <= boundary decision in JSDoc.
• Minor #7 (false "all complete" with falsy onboardStartTime): Still present - see Important #1 below.

Strengths

• Clean extraction to shared-utils - computeAuditCompletion is a pure function (no side effects, no DB calls, explicit NaN handling). The four modules under src/opportunity/ in shared-utils are well-structured: pure data maps, pure query helpers, pure computation. This is exactly how cross-service contracts should be factored.
• Single DB call for audit completion (handler.js:534-537) - Audit.allLatestForSite(siteId) replaces what could have been N queries. The pure function processes results in-memory. Eliminates the entire class of loop-accumulation bugs.
• Conservative error handling (handler.js:543-546) - DB failure falls back to "all pending" rather than "all complete." Users see hourglass indicators and a disclaimer rather than false green statuses. Correct failure mode.
• Hourglass skips getSuggestions() (handler.js:575-577) - when a source audit is pending, the handler shows :hourglass_flowing_sand: and avoids the expensive and misleading getSuggestions() call. Good UX and performance.
• Infrastructure audit filtering (handler.js:684-686) - relevantPendingTypes correctly excludes audit types like scrape-top-pages that don't produce user-visible opportunities.
• Thorough test coverage - 9 new disclaimer tests covering stale audit, missing record, isRecheck complete, no-disclaimer-when-complete, empty auditTypes, hourglass, DB error fallback, siteUrl in hint, and infrastructure filtering. Tests use real shared-utils functions (not stubs), validating the handler+library contract end-to-end.
• Supply chain is clean - both bumped packages are @adobe/-scoped on npm with proper integrity hashes. No new packages introduced.

Issues

Important (Should Fix)

1. Minor #7 is still live: false "All audits completed" when onboardStartTime is absent - handler.js:~697

When onboardStartTime is falsy, the audit completion DB check is skipped (correctly), so pendingAuditTypes stays []. But the disclaimer block checks else if (isRecheck) without verifying onboardStartTime was set. If isRecheck=true and onboardStartTime is missing (legacy sites), the user sees "All audits have completed" without any check having been performed.

Fix (one token):

} else if (isRecheck && onboardStartTime) {

Add a test for onboardStartTime=undefined + isRecheck=true to verify no "all complete" message is sent.

2. ~20 existing tests silently hit the audit error path - opportunity-status-processor.test.js

The outer beforeEach builds context.dataAccess without an Audit property. Any test that sets message.taskContext.onboardStartTime triggers Audit.allLatestForSite(siteId), which throws TypeError, caught by the catch block. These tests pass but silently exercise the error fallback path instead of the happy path. The Audit.allLatestForSite stub was added to the new disclaimer tests and one existing test but not to the ~20 others.

Fix: Add Audit: { allLatestForSite: sinon.stub().resolves([]) } to the outer beforeEach context so these tests stop silently exercising the error path.

Minor (Nice to Have)

3. Pluralization mismatch in disclaimer message - handler.js:~693

The plural suffix is based on relevantPendingTypes.length (audit type count), but the displayed list is opportunity names. One audit type can map to multiple opportunities. Consider rephrasing to "Some audits may still be in progress. Affected areas: {pendingList}."

4. statusMessages string-based filtering is fragile - handler.js:630-634

Data source and opportunity statuses are pushed into the same statusMessages array, then split apart by string matching (!msg.includes('RUM'), etc.). If a future opportunity title contains "RUM" or "GSC", it would be silently filtered out. Pre-existing, not introduced by this PR. Worth a follow-up to use separate arrays.

Recommendations

• Consider logging the full computeAuditCompletion result at debug level for production troubleshooting.
• Follow-up: separate dataSourceMessages and opportunityMessages into distinct arrays to eliminate the string-based filtering fragility.

Assessment

Ready to merge? With fixes (two low-effort items).

The shared-utils extraction is architecturally sound, all 4 major prior findings are resolved, the dependency changes are clean, and the test coverage is thorough. The two remaining Important items are a one-token code fix (#1) and a one-line stub addition to test setup (#2) - both straightforward.

Next Steps

1. Add && onboardStartTime guard to the isRecheck branch + add the missing test case.
2. Add Audit.allLatestForSite stub to the outer beforeEach context.
3. Minor items are optional.

[tkotthakota-adobe]

Hey @solaris007
Fixed below issues raised

Important #1 — False "All audits completed" for legacy sites
  Added && onboardStartTime guard to the isRecheck branch. Without it, legacy sites with no onboardStartTime would see "All      
  audits have completed" even though no check was performed.                                                                     
                                                                                                                                 
  // Before                                                                                                                      
  } else if (isRecheck) {                                                                                                        
  // After
  } else if (isRecheck && onboardStartTime) {                                                                                    
                                                            
Important #2 — ~20 tests silently hitting error fallback                                                                       
  Added Audit: { allLatestForSite: sandbox.stub().resolves([]) } to the outer beforeEach. Tests that set onboardStartTime were
  throwing TypeError on Audit.allLatestForSite (missing from dataAccess) and passing only because the catch block swallowed it.  
                                                            
New test                                                                                                                       
  Added 'does not send "all complete" when isRecheck=true but onboardStartTime is absent (legacy site)' to cover the fixed guard.