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 3 commits into
base: main
Choose a base branch
from
+113 −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
3 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
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
79 changes: 57 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,45 @@ 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.debug('[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.
*/
isHandlerEnabledForOrg(type, org) {
const handler = this.getHandlers()?.[type];
if (!handler) {
Expand Down Expand Up @@ -243,20 +277,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
56 changes: 56 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,62 @@ 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;
});

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());
});

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
Loading