From 8400b4606b57dcbd48ae4418e029adf2a7d8c1b5 Mon Sep 17 00:00:00 2001 From: Tej Kotthakota Date: Tue, 5 May 2026 19:23:02 -0500 Subject: [PATCH 1/3] site-level handler overrides were ignored, org-level disable always won --- .../configuration/configuration.model.js | 47 +++++++++++-------- .../configuration/configuration.model.test.js | 10 ++++ 2 files changed, 37 insertions(+), 20 deletions(-) diff --git a/packages/spacecat-shared-data-access/src/models/configuration/configuration.model.js b/packages/spacecat-shared-data-access/src/models/configuration/configuration.model.js index f8f3ac1d6..c637bc207 100644 --- a/packages/spacecat-shared-data-access/src/models/configuration/configuration.model.js +++ b/packages/spacecat-shared-data-access/src/models/configuration/configuration.model.js @@ -183,24 +183,28 @@ 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)) { + 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); } isHandlerEnabledForOrg(type, org) { @@ -243,20 +247,23 @@ class Configuration { } if (enabled) { - if (handler.enabledByDefault) { - handler.disabled[entityKey] = handler.disabled[entityKey] - /* c8 ignore next */ - .filter((id) => id !== entityId) || []; - } else { + // Always remove from disabled first (handles re-enabling a previously disabled entry). + handler.disabled[entityKey] = (handler.disabled[entityKey] || []) + .filter((id) => id !== entityId); + if (!handler.enabledByDefault) { + // For non-default handlers, explicitly add to enabled list so isHandlerEnabledForSite + // can find the site in enabled.sites and return true even when org is in disabled.orgs. 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; diff --git a/packages/spacecat-shared-data-access/test/unit/models/configuration/configuration.model.test.js b/packages/spacecat-shared-data-access/test/unit/models/configuration/configuration.model.test.js index 969725a9f..67a0cfe17 100755 --- a/packages/spacecat-shared-data-access/test/unit/models/configuration/configuration.model.test.js +++ b/packages/spacecat-shared-data-access/test/unit/models/configuration/configuration.model.test.js @@ -235,6 +235,16 @@ 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; + }); + 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()); From ab23fd4c1a601d0dafa54296fb474be194413883 Mon Sep 17 00:00:00 2001 From: Tej Kotthakota Date: Tue, 5 May 2026 22:37:06 -0500 Subject: [PATCH 2/3] test --- .../models/configuration/configuration.model.test.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/spacecat-shared-data-access/test/unit/models/configuration/configuration.model.test.js b/packages/spacecat-shared-data-access/test/unit/models/configuration/configuration.model.test.js index 67a0cfe17..90f48fc0d 100755 --- a/packages/spacecat-shared-data-access/test/unit/models/configuration/configuration.model.test.js +++ b/packages/spacecat-shared-data-access/test/unit/models/configuration/configuration.model.test.js @@ -245,6 +245,16 @@ describe('ConfigurationModel', () => { expect(instance.isHandlerEnabledForSite('org-disabled-handler', site)).to.be.false; }); + it('returns true when site is in enabled.sites even if its org is in disabled.orgs', () => { + instance.addHandler('site-override-handler', { + enabledByDefault: true, + enabled: { sites: [site.getId()], orgs: [] }, + disabled: { sites: [], orgs: [site.getOrganizationId()] }, + }); + + expect(instance.isHandlerEnabledForSite('site-override-handler', site)).to.be.true; + }); + 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()); From 7462cfe30bb5f7fe5696d414ec64cb0086546510 Mon Sep 17 00:00:00 2001 From: Tej Kotthakota Date: Wed, 6 May 2026 18:40:08 -0500 Subject: [PATCH 3/3] address review comments --- .../configuration/configuration.model.js | 40 ++++++++++++++++--- .../configuration/configuration.model.test.js | 36 +++++++++++++++++ 2 files changed, 70 insertions(+), 6 deletions(-) diff --git a/packages/spacecat-shared-data-access/src/models/configuration/configuration.model.js b/packages/spacecat-shared-data-access/src/models/configuration/configuration.model.js index c637bc207..8c9f13dbd 100644 --- a/packages/spacecat-shared-data-access/src/models/configuration/configuration.model.js +++ b/packages/spacecat-shared-data-access/src/models/configuration/configuration.model.js @@ -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) { @@ -195,6 +210,9 @@ class Configuration { return false; } if (enabledSites.includes(siteId)) { + if (disabledOrgs.includes(orgId)) { + this.log.debug('[isHandlerEnabledForSite] site-override: site in enabled.sites overrides org in disabled.orgs', { type, siteId, orgId }); + } return true; } @@ -207,6 +225,18 @@ class Configuration { 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) { @@ -250,12 +280,10 @@ class Configuration { // Always remove from disabled first (handles re-enabling a previously disabled entry). handler.disabled[entityKey] = (handler.disabled[entityKey] || []) .filter((id) => id !== entityId); - if (!handler.enabledByDefault) { - // For non-default handlers, explicitly add to enabled list so isHandlerEnabledForSite - // can find the site in enabled.sites and return true even when org is in disabled.orgs. - handler.enabled[entityKey] = Array - .from(new Set([...(handler.enabled[entityKey] || []), 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])); diff --git a/packages/spacecat-shared-data-access/test/unit/models/configuration/configuration.model.test.js b/packages/spacecat-shared-data-access/test/unit/models/configuration/configuration.model.test.js index 90f48fc0d..74442563d 100755 --- a/packages/spacecat-shared-data-access/test/unit/models/configuration/configuration.model.test.js +++ b/packages/spacecat-shared-data-access/test/unit/models/configuration/configuration.model.test.js @@ -255,6 +255,42 @@ describe('ConfigurationModel', () => { 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()] }, + }); + + 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()); + 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());