Skip to content

fix: persist site config via setConfig after disableImport #240

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

Merged
prithipalpatwal merged 2 commits into from
Apr 1, 2026
Merged

fix: persist site config via setConfig after disableImport #240

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
876f29d
fix(disable-import-audit): persist site config via setConfig after di…
prithipalpatwal Mar 31, 2026
cebcf9e
fix: import Config from spacecat-shared-data-access public entry
prithipalpatwal Mar 31, 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
4 changes: 4 additions & 0 deletions src/tasks/disable-import-audit-processor/handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
*/

import { ok } from '@adobe/spacecat-shared-http-utils';
import { Config } from '@adobe/spacecat-shared-data-access/src/models/site/config.js';

@tkotthakota-adobe tkotthakota-adobe Mar 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This imports directly from an internal src/ path of the published
package, not from its public entry point. This is fragile — internal
paths can change without a semver bump. It works now but could break on
a lib update.

Ideally Config and toDynamoItem should be exported from the package's
public index.js. Worth checking if there's an alternative export, or
raising it with the shared-data-access team to expose toDynamoItem
publicly.

@prithipalpatwal prithipalpatwal Mar 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR, will merge it tomorrow. Thanks!


import { say } from '../../utils/slack-utils.js';

const TASK_TYPE = 'disable-import-audit-processor';
Expand Down Expand Up @@ -55,6 +57,8 @@ export async function runDisableImportAuditProcessor(message, context) {
siteConfig.disableImport(importType);
}

site.setConfig(Config.toDynamoItem(siteConfig));

const configuration = await Configuration.findLatest();
for (const auditType of auditTypes) {
configuration.disableHandlerForSite(auditType, site);
Expand Down
37 changes: 37 additions & 0 deletions test/tasks/disable-import-audit-processor/disable-import-audit-processor.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@ describe('Disable Import Audit Processor', () => {
let mockSite;
let mockSiteConfig;
let mockConfiguration;
let toDynamoItemStub;

const serializedConfigFixture = {
slack: {},
handlers: {},
imports: [],
};

beforeEach(async () => {
// Reset all stubs
Expand All @@ -41,11 +48,16 @@ describe('Disable Import Audit Processor', () => {
// Mock the say function
mockSay = sandbox.stub().resolves();

toDynamoItemStub = sandbox.stub().returns(serializedConfigFixture);

// Dynamic import with mocked dependencies
const handlerModule = await esmock('../../../src/tasks/disable-import-audit-processor/handler.js', {
'../../../src/utils/slack-utils.js': {
say: mockSay,
},
'@adobe/spacecat-shared-data-access/src/models/site/config.js': {
Config: { toDynamoItem: toDynamoItemStub },
},
});
runDisableImportAuditProcessor = handlerModule.runDisableImportAuditProcessor;

Expand All @@ -56,6 +68,7 @@ describe('Disable Import Audit Processor', () => {

mockSite = {
getConfig: sandbox.stub().returns(mockSiteConfig),
setConfig: sandbox.stub(),
save: sandbox.stub().resolves(),
};

Expand Down Expand Up @@ -114,6 +127,9 @@ describe('Disable Import Audit Processor', () => {
expect(mockSiteConfig.disableImport).to.have.been.calledWith('screaming-frog');
expect(mockSiteConfig.disableImport).to.have.callCount(2);

expect(toDynamoItemStub).to.have.been.calledOnceWith(mockSiteConfig);
expect(mockSite.setConfig).to.have.been.calledOnceWith(serializedConfigFixture);

// Verify audit types were disabled
expect(context.dataAccess.Configuration.findLatest).to.have.been.calledOnce;
expect(mockConfiguration.disableHandlerForSite).to.have.been.calledWith('cwv', mockSite);
Expand Down Expand Up @@ -160,6 +176,9 @@ describe('Disable Import Audit Processor', () => {
expect(mockSite.save).to.have.been.calledOnce;
expect(mockConfiguration.save).to.have.been.calledOnce;

expect(toDynamoItemStub).to.have.been.calledOnceWith(mockSiteConfig);
expect(mockSite.setConfig).to.have.been.calledOnceWith(serializedConfigFixture);

// Verify Slack messages with "None" text
expect(mockSay.firstCall).to.have.been.calledWith(
context.env,
Expand Down Expand Up @@ -190,6 +209,9 @@ describe('Disable Import Audit Processor', () => {
':broom: *For site: https://example.com: Disabled imports*: None *and audits*: None',
);

expect(toDynamoItemStub).to.have.been.calledOnceWith(mockSiteConfig);
expect(mockSite.setConfig).to.have.been.calledOnceWith(serializedConfigFixture);

expect(result).to.exist;
});

Expand All @@ -200,6 +222,8 @@ describe('Disable Import Audit Processor', () => {

// Should complete without error
expect(context.log.info).to.have.been.calledWith('For site: https://example.com: Disabled imports and audits');
expect(toDynamoItemStub).to.have.been.calledOnceWith(mockSiteConfig);
expect(mockSite.setConfig).to.have.been.calledOnceWith(serializedConfigFixture);
expect(mockSay).to.have.been.calledTwice;
expect(result).to.exist;
});
Expand Down Expand Up @@ -275,6 +299,7 @@ describe('Disable Import Audit Processor', () => {
const result = await runDisableImportAuditProcessor(message, context);

expect(context.log.error).to.have.been.calledWith('Error in disable import and audit processor:', sinon.match.instanceOf(Error));
expect(mockSite.setConfig).to.not.have.been.called;
expect(mockSite.save).to.not.have.been.called;
expect(mockConfiguration.save).to.not.have.been.called;

Expand All @@ -296,6 +321,8 @@ describe('Disable Import Audit Processor', () => {
const result = await runDisableImportAuditProcessor(message, context);

expect(context.log.error).to.have.been.calledWith('Error in disable import and audit processor:', saveError);
expect(toDynamoItemStub).to.have.been.calledOnceWith(mockSiteConfig);
expect(mockSite.setConfig).to.have.been.calledOnceWith(serializedConfigFixture);
expect(mockSay).to.have.been.calledWith(
context.env,
context.log,
Expand All @@ -313,6 +340,8 @@ describe('Disable Import Audit Processor', () => {
const result = await runDisableImportAuditProcessor(message, context);

expect(context.log.error).to.have.been.calledWith('Error in disable import and audit processor:', saveError);
expect(toDynamoItemStub).to.have.been.calledOnceWith(mockSiteConfig);
expect(mockSite.setConfig).to.have.been.calledOnceWith(serializedConfigFixture);
expect(mockSay).to.have.been.calledWith(
context.env,
context.log,
Expand All @@ -330,6 +359,7 @@ describe('Disable Import Audit Processor', () => {
const result = await runDisableImportAuditProcessor(message, context);

expect(context.log.error).to.have.been.calledWith('Error in disable import and audit processor:', lookupError);
expect(mockSite.setConfig).to.not.have.been.called;
expect(mockSay).to.have.been.calledWith(
context.env,
context.log,
Expand All @@ -347,6 +377,8 @@ describe('Disable Import Audit Processor', () => {
const result = await runDisableImportAuditProcessor(message, context);

expect(context.log.error).to.have.been.calledWith('Error in disable import and audit processor:', configError);
expect(toDynamoItemStub).to.have.been.calledOnceWith(mockSiteConfig);
expect(mockSite.setConfig).to.have.been.calledOnceWith(serializedConfigFixture);
expect(mockSay).to.have.been.calledWith(
context.env,
context.log,
Expand All @@ -365,6 +397,8 @@ describe('Disable Import Audit Processor', () => {
const result = await runDisableImportAuditProcessor(message, context);

// Should still complete successfully despite Slack error
expect(toDynamoItemStub).to.have.been.calledOnceWith(mockSiteConfig);
expect(mockSite.setConfig).to.have.been.calledOnceWith(serializedConfigFixture);
expect(mockSite.save).to.have.been.calledOnce;
expect(mockConfiguration.save).to.have.been.calledOnce;
expect(result).to.exist;
Expand Down Expand Up @@ -421,6 +455,9 @@ describe('Disable Import Audit Processor', () => {
':broom: *For site: https://example.com: Disabled imports*: single-import *and audits*: single-audit',
);

expect(toDynamoItemStub).to.have.been.calledOnceWith(mockSiteConfig);
expect(mockSite.setConfig).to.have.been.calledOnceWith(serializedConfigFixture);

expect(result).to.exist;
});
});
Expand Down
Loading