Skip to content

fix: site-level handler overrides were ignored, org-level disable always won #1590

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tkotthakota-adobe wants to merge 4 commits into
base: main
Choose a base branch
from
+159 −22
Open

fix: site-level handler overrides were ignored, org-level disable always won #1590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8400b46
site-level handler overrides were ignored, org-level disable always won
tkotthakota-adobe May 6, 2026
ab23fd4
test
tkotthakota-adobe May 6, 2026
7462cfe
address review comments
tkotthakota-adobe May 6, 2026
a1e410a
review comments
tkotthakota-adobe May 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 65 additions & 22 deletions packages/spacecat-shared-data-access/src/models/configuration/configuration.model.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,21 @@ class Configuration {
.map((job) => job.type);
}

/**
* Returns whether a handler is enabled for a given site.
*
* Precedence (highest to lowest):
* 1. disabled.sites includes siteId → false (site explicitly disabled)
* 2. enabled.sites includes siteId → true (site explicitly enabled; overrides org-level)
* 3. disabled.orgs includes orgId → false (org disabled, no site-level override present)
* 4. enabledByDefault → true
* 5. enabled.orgs includes orgId → true
* 6. otherwise → false
*
* Note: disabled.sites and enabled.sites are assumed disjoint; disabled.sites wins if violated
* (see #updatedHandler which keeps these lists disjoint on every write).
* isHandlerEnabledForOrg uses a different (simpler) precedence — see its JSDoc.
*/
isHandlerEnabledForSite(type, site) {
const handler = this.getHandlers()?.[type];
if (!handler) {
Expand All @@ -183,26 +198,53 @@ class Configuration {
const siteId = site.getId();
const orgId = site.getOrganizationId();

if (handler.disabled) {
const sites = handler.disabled.sites || [];
const orgs = handler.disabled.orgs || [];
if (sites.includes(siteId) || orgs.includes(orgId)) {
return false;
const disabledSites = handler.disabled?.sites || [];
const enabledSites = handler.enabled?.sites || [];
const disabledOrgs = handler.disabled?.orgs || [];
const enabledOrgs = handler.enabled?.orgs || [];

// Site-level takes priority over org-level: an explicit site enable/disable
// overrides the org-level setting, allowing a single site to be upgraded to
// a paid profile even if the org is in disabled.orgs.
if (disabledSites.includes(siteId)) {
return false;
}
if (enabledSites.includes(siteId)) {
Copy link
Copy Markdown
Contributor Author

@tkotthakota-adobe tkotthakota-adobe May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment by @solaris007
Important: This flips existing config rows where a site is in enabled.sites and the org is in disabled.orgs from disabled to enabled at deploy time. Run a query against prod to enumerate affected rows before merging.

Copy link
Copy Markdown
Contributor Author

@tkotthakota-adobe tkotthakota-adobe May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not have prod access @solaris007 What is the best way to do this query?

if (disabledOrgs.includes(orgId)) {
this.log.info('[isHandlerEnabledForSite] site-override: site in enabled.sites overrides org in disabled.orgs', { type, siteId, orgId });
}
return true;
}

if (disabledOrgs.includes(orgId)) {
return false;
}
if (handler.enabledByDefault) {
return true;
}
if (handler.enabled) {
const sites = handler.enabled.sites || [];
const orgs = handler.enabled.orgs || [];
return sites.includes(siteId) || orgs.includes(orgId);
}

return false;
return enabledOrgs.includes(orgId);
}

/**
* Returns whether a handler is enabled for a given org.
*
* Precedence (highest to lowest):
* 1. disabled.orgs includes orgId → false
* 2. enabledByDefault → true
* 3. enabled.orgs includes orgId → true
* 4. otherwise → false
*
* Note: unlike isHandlerEnabledForSite, there is no site-level override here.
* An org in disabled.orgs is always disabled regardless of site-level entries.
*
* Cross-system consequence: this method is the gate for org-scoped fan-out
* (e.g. report digests keyed on orgId in spacecat-jobs-dispatcher and
* spacecat-reporting-worker). A paid site in enabled.sites whose org is in
* disabled.orgs will receive site-targeted handler evaluation (true via
* isHandlerEnabledForSite) but will NOT receive org-level digest payloads
* (false here). This asymmetry is intentional: org-level reports follow org
* policy; site-level overrides do not propagate to org-scoped fan-out.
*/
isHandlerEnabledForOrg(type, org) {
Copy link
Copy Markdown
Member

@solaris007 solaris007 May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important: cross-system consequence of the asymmetric model is not documented.

Tracing the consumer call sites: isHandlerEnabledForOrg is used as a gate in spacecat-jobs-dispatcher/src/handlers/reports.js and spacecat-reporting-worker/src/digest/handler-external.js (the latter enumerates all sites in the org without re-checking per site when the org is disabled).

Implication: a paid site in enabled.sites whose org is in disabled.orgs will receive site-targeted reports (correct, this is what the PR enables) but will NOT receive org-level digests routed through orgId - the digest handler short-circuits at the org gate. The PR's stated invariant "site-level handler overrides were ignored, org-level disable always won" still holds in the digest fan-out path.

This is an in-repo doc fix. Pick one:

  • (a) Declare it intentional: extend this JSDoc to say "intended for org-scoped report fan-out; site-level overrides do not propagate here", document in PR description.
  • (b) File a Jira for the digest handler to re-check per-site, link from PR description as a known limitation.

Either is defensible. The asymmetric model is documented at the method level but the cross-system consequence is invisible today.

const handler = this.getHandlers()?.[type];
if (!handler) {
Expand Down Expand Up @@ -243,20 +285,21 @@ class Configuration {
}

if (enabled) {
if (handler.enabledByDefault) {
handler.disabled[entityKey] = handler.disabled[entityKey]
/* c8 ignore next */
.filter((id) => id !== entityId) || [];
} else {
handler.enabled[entityKey] = Array
.from(new Set([...(handler.enabled[entityKey] || []), entityId]));
}
// Always remove from disabled first (handles re-enabling a previously disabled entry).
handler.disabled[entityKey] = (handler.disabled[entityKey] || [])
.filter((id) => id !== entityId);
// Always add to enabled so isHandlerEnabledForSite can find the site in enabled.sites
// and return true even when the org is in disabled.orgs (site-level override).
handler.enabled[entityKey] = Array
.from(new Set([...(handler.enabled[entityKey] || []), entityId]));
} else if (handler.enabledByDefault) {
handler.disabled[entityKey] = Array
.from(new Set([...(handler.disabled[entityKey] || []), entityId]));
handler.enabled[entityKey] = (handler.enabled[entityKey] || [])
.filter((id) => id !== entityId);
} else {
/* c8 ignore next */
handler.enabled[entityKey] = handler.enabled[entityKey].filter((id) => id !== entityId) || [];
handler.enabled[entityKey] = (handler.enabled[entityKey] || [])
.filter((id) => id !== entityId);
}

handlers[type] = handler;
Expand Down
94 changes: 94 additions & 0 deletions ...es/spacecat-shared-data-access/test/unit/models/configuration/configuration.model.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,98 @@ describe('ConfigurationModel', () => {
expect(isEnabled).to.be.true;
});

it('returns false for a site whose org is in disabled.orgs and the site is not explicitly listed', () => {
instance.addHandler('org-disabled-handler', {
enabledByDefault: true,
enabled: { sites: [], orgs: [] },
disabled: { sites: [], orgs: [site.getOrganizationId()] },
});

expect(instance.isHandlerEnabledForSite('org-disabled-handler', site)).to.be.false;
});

Comment thread
tkotthakota-adobe marked this conversation as resolved.
it('returns true when site is in enabled.sites even if its org is in disabled.orgs', () => {
instance.addHandler('site-override-handler', {
enabledByDefault: true,
Comment thread
tkotthakota-adobe marked this conversation as resolved.
enabled: { sites: [site.getId()], orgs: [] },
disabled: { sites: [], orgs: [site.getOrganizationId()] },
});

expect(instance.isHandlerEnabledForSite('site-override-handler', site)).to.be.true;
});

it('returns true for non-default handler when site is in enabled.sites and org is in disabled.orgs', () => {
instance.addHandler('paid-handler', {
enabledByDefault: false,
enabled: { sites: [site.getId()], orgs: [] },
disabled: { sites: [], orgs: [site.getOrganizationId()] },
});
Copy link
Copy Markdown
Member

@solaris007 solaris007 May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important - the headline production scenario lacks an end-to-end test.

The PR exists to fix the free-trial -> paid upgrade path: org has handler X with enabledByDefault: false and is in disabled.orgs; we upgrade site S via updateHandlerSites('X', siteId, true); we expect isHandlerEnabledForSite('X', S) to return true afterwards. The new "paid-handler" test above is a READ-PATH-only assertion - it pre-populates state directly via addHandler and never calls updateHandlerSites. Test 2 below ("removes site from disabled.sites when re-enabling for a non-default handler") exercises the write path but with disabled.orgs: [], so the override-against-disabled-org case is not asserted post-write.

Without a test that combines write + read, a future regression that breaks updateHandlerSites for non-default handlers (e.g., someone re-introducing the if (!enabledByDefault) guard the prior review asked you to remove) would not be caught: the read-path "paid-handler" test would still pass because it bypasses the write path. Add one test:

it('upgrades non-default handler from disabled org via updateHandlerSites', () => {
  instance.addHandler('paid-handler', {
    enabledByDefault: false,
    enabled: { sites: [], orgs: [] },
    disabled: { sites: [], orgs: [site.getOrganizationId()] },
  });
  instance.updateHandlerSites('paid-handler', site.getId(), true);
  expect(instance.isHandlerEnabledForSite('paid-handler', site)).to.be.true;
});

This is the test that tells future maintainers what the PR is for.


expect(instance.isHandlerEnabledForSite('paid-handler', site)).to.be.true;
Copy link
Copy Markdown
Member

@solaris007 solaris007 May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important: headline end-to-end test missing (Round 2 unresolved).

The PR fixes the free-trial -> paid upgrade path, but no test composes write-path-then-read-path with the full org-disabled context. Test 3 (this test) pre-populates enabled.sites via addHandler (read path only). Test 4 below exercises the write path but without org in disabled.orgs. Neither tests the exact scenario the PR is for.

Fix: one chained test, approximately 10 lines:

it('enables a paid handler for a site whose org is in disabled.orgs (free-trial -> paid)', () => {
  instance.addHandler('paid-handler', {
    enabledByDefault: false,
    enabled: { sites: [], orgs: [] },
    disabled: { sites: [], orgs: [site.getOrganizationId()] },
  });
  expect(instance.isHandlerEnabledForSite('paid-handler', site)).to.be.false;
  instance.updateHandlerSites('paid-handler', site.getId(), true);
  expect(instance.isHandlerEnabledForSite('paid-handler', site)).to.be.true;
  expect(instance.getHandler('paid-handler').enabled.sites).to.include(site.getId());
});

});

it('enables a paid handler for a site whose org is in disabled.orgs (free-trial -> paid)', () => {
instance.addHandler('paid-handler', {
enabledByDefault: false,
enabled: { sites: [], orgs: [] },
disabled: { sites: [], orgs: [site.getOrganizationId()] },
});

expect(instance.isHandlerEnabledForSite('paid-handler', site)).to.be.false;
instance.updateHandlerSites('paid-handler', site.getId(), true);
expect(instance.isHandlerEnabledForSite('paid-handler', site)).to.be.true;
expect(instance.getHandler('paid-handler').enabled.sites).to.include(site.getId());
});

it('removes site from disabled.sites when re-enabling for a non-default handler', () => {
instance.addHandler('cleanup-handler', {
enabledByDefault: false,
enabled: { sites: [], orgs: [] },
disabled: { sites: [site.getId()], orgs: [] },
});

instance.updateHandlerSites('cleanup-handler', site.getId(), true);

expect(instance.getHandler('cleanup-handler').disabled.sites).to.not.include(site.getId());
expect(instance.getHandler('cleanup-handler').enabled.sites).to.include(site.getId());
});

it('removes site from enabled.sites when disabling a default handler', () => {
instance.addHandler('default-cleanup-handler', {
enabledByDefault: true,
enabled: { sites: [site.getId()], orgs: [] },
disabled: { sites: [], orgs: [] },
});

instance.updateHandlerSites('default-cleanup-handler', site.getId(), false);

expect(instance.getHandler('default-cleanup-handler').enabled.sites).to.not.include(site.getId());
Copy link
Copy Markdown
Member

@solaris007 solaris007 May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important - enabledByDefault: false disable branch is not directly tested.

#updatedHandler has three write paths after this PR's simplification:

  1. Enable: remove from disabled + add to enabled (any handler) - covered by Test 2 ("cleanup-handler")
  2. Disable + enabledByDefault: true: add to disabled + remove from enabled - covered by Test 3 ("default-cleanup-handler")
  3. Disable + enabledByDefault: false (the else branch at configuration.model.js:292-294): only remove from enabled, do NOT add to disabled - NOT covered.

With the new "always add to enabled on enable" change in this PR, the asymmetry of branch 3 is now load-bearing: a paid handler that gets enabled (now in enabled.sites) and then disabled must NOT linger in enabled.sites. Add at least:

it('removes site from enabled.sites when disabling for a non-default handler', () => {
  instance.addHandler('paid-disable-handler', {
    enabledByDefault: false,
    enabled: { sites: [site.getId()], orgs: [] },
    disabled: { sites: [], orgs: [] },
  });
  instance.updateHandlerSites('paid-disable-handler', site.getId(), false);
  expect(instance.getHandler('paid-disable-handler').enabled.sites).to.not.include(site.getId());
  expect(instance.getHandler('paid-disable-handler').disabled.sites).to.not.include(site.getId());
});

This pins the contract that disabling a non-default handler is "not in either list" (default-off applies), distinct from disabling a default handler ("in disabled list, default-on overridden").

expect(instance.getHandler('default-cleanup-handler').disabled.sites).to.include(site.getId());
Copy link
Copy Markdown
Member

@solaris007 solaris007 May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important: enabledByDefault: false disable branch is reached but not behaviorally asserted (Round 2 unresolved).

The existing test at line 349-353 exercises this branch through disableHandlerForSite('organic-keywords', site), but its assertion is expect(...disabled.sites).to.not.include(siteId) - a tautology since organic-keywords.disabled.sites was never populated by the prior enable. The branch's actual job (removing siteId from enabled.sites so the round-trip leaves no stale override) is not asserted.

With this PR's "always add to enabled on enable" change, a paid handler can land in enabled.sites for sites whose org is in disabled.orgs. If the disable path's enabled.sites filter is broken or removed in a future refactor, a disabled handler would still read true via step 2 of the precedence ladder. Line-coverage tools would not catch it.

Fix: add expect(...enabled.sites).to.not.include(siteId) to the line 349-353 test, or add a dedicated round-trip test on a paid handler. 3-5 lines.

});

it('returns false when site is in both enabled.sites and disabled.sites (disabled.sites wins)', () => {
instance.addHandler('conflict-handler', {
enabledByDefault: false,
enabled: { sites: [site.getId()], orgs: [] },
disabled: { sites: [site.getId()], orgs: [] },
});

expect(instance.isHandlerEnabledForSite('conflict-handler', site)).to.be.false;
});

it('enables a site idempotently (repeated enable does not grow enabled.sites)', () => {
instance.addHandler('idempotent-handler', {
enabledByDefault: false,
enabled: { sites: [], orgs: [] },
disabled: { sites: [], orgs: [] },
});

instance.updateHandlerSites('idempotent-handler', site.getId(), true);
instance.updateHandlerSites('idempotent-handler', site.getId(), true);

expect(instance.getHandler('idempotent-handler').enabled.sites.length).to.equal(1);
});

it('updates handler orgs for a handler disabled by default with enabled', () => {
instance.updateHandlerOrgs('lhs-mobile', org.getId(), true);
expect(instance.getHandler('lhs-mobile').enabled.orgs).to.include(org.getId());
Expand Down Expand Up @@ -292,8 +384,10 @@ describe('ConfigurationModel', () => {

it('disables a handler for a site', () => {
instance.enableHandlerForSite('organic-keywords', site);
expect(instance.getHandler('organic-keywords').enabled.sites).to.include(site.getId());
instance.disableHandlerForSite('organic-keywords', site);
expect(instance.getHandler('organic-keywords').disabled.sites).to.not.include(site.getId());
expect(instance.getHandler('organic-keywords').enabled.sites).to.not.include(site.getId());
});

it('enables a handler for an organization', () => {
Expand Down
Loading