Skip to content

fix(slack/onboard-llmo): close modal upon submission #1283

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
+126 −36
Draft

fix(slack/onboard-llmo): close modal upon submission #1283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6967df9
fix(slack/onboard-llmo): fire and forget onboarding process
cwjwisse Sep 30, 2025
9108d4f
fix(slack/onboard-llmo): apply fire and forget to all modals
cwjwisse Sep 30, 2025
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
68 changes: 48 additions & 20 deletions src/support/slack/actions/onboard-llmo-modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,8 +670,6 @@ export function onboardLLMOModal(lambdaContext) {
originalThreadTs,
});

// eslint-disable-next-line max-statements-per-line
await new Promise((resolve) => { setTimeout(resolve, 500); });
await ack();

// Create a slack context for the onboarding process
Expand All @@ -692,9 +690,12 @@ export function onboardLLMOModal(lambdaContext) {
threadTs: responseThreadTs,
};

await onboardSite({
// Return immediately after ack() to make sure the modal closes
onboardSite({
brandName, baseURL: brandURL, imsOrgId, deliveryType,
}, lambdaContext, slackContext);
}, lambdaContext, slackContext).catch((error) => {
log.error(`Error in background onboarding for site ${brandURL}:`, error);
});

log.debug(`Onboard LLMO modal processed for user ${user.id}, site ${brandURL}`);
} catch (e) {
Expand Down Expand Up @@ -759,14 +760,27 @@ export function addEntitlementsAction(lambdaContext) {
};
/* c8 ignore end */

await createEntitlementAndEnrollment(site, lambdaContext, slackContext);
await client.chat.postMessage({
channel: originalChannel,
text: `:white_check_mark: Successfully ensured LLMO entitlements and enrollments for *${brandURL}* (brand: *${existingBrand}*).`,
thread_ts: originalThreadTs,
});

log.debug(`Added entitlements for site ${siteId} (${brandURL}) for user ${user.id}`);
// Fire-and-forget: Start the entitlement process without awaiting
// This allows the handler to return immediately after ack() while the
// entitlement process continues in the background
(async () => {
try {
await createEntitlementAndEnrollment(site, lambdaContext, slackContext);
await client.chat.postMessage({
channel: originalChannel,
text: `:white_check_mark: Successfully ensured LLMO entitlements and enrollments for *${brandURL}* (brand: *${existingBrand}*).`,
thread_ts: originalThreadTs,
});
log.debug(`Added entitlements for site ${siteId} (${brandURL}) for user ${user.id}`);
} catch (error) {
log.error(`Error in background entitlement process for site ${brandURL}:`, error);
await client.chat.postMessage({
channel: originalChannel,
text: `:x: Failed to add LLMO entitlements: ${error.message}`,
thread_ts: originalThreadTs,
});
}
})();
} catch (error) {
log.error('Error adding entitlements:', error);
}
Expand Down Expand Up @@ -925,16 +939,30 @@ export function updateIMSOrgModal(lambdaContext) {
};
/* c8 ignore end */

await checkOrg(newImsOrgId, site, lambdaContext, slackContext);
await createEntitlementAndEnrollment(site, lambdaContext, slackContext);
// Fire-and-forget: Start the org update process without awaiting
// This allows the handler to return immediately after ack() while the
// update process continues in the background
(async () => {
try {
await checkOrg(newImsOrgId, site, lambdaContext, slackContext);
await createEntitlementAndEnrollment(site, lambdaContext, slackContext);

await client.chat.postMessage({
channel: responseChannel,
text: `:white_check_mark: Successfully updated organization and applied LLMO entitlements for *${brandURL}* (brand: *${existingBrand}*)`,
thread_ts: responseThreadTs,
});
await client.chat.postMessage({
channel: responseChannel,
text: `:white_check_mark: Successfully updated organization and applied LLMO entitlements for *${brandURL}* (brand: *${existingBrand}*)`,
thread_ts: responseThreadTs,
});

log.debug(`Updated org and applied entitlements for site ${siteId} (${brandURL}) for user ${user.id}`);
log.debug(`Updated org and applied entitlements for site ${siteId} (${brandURL}) for user ${user.id}`);
} catch (error) {
log.error(`Error in background org update for site ${brandURL}:`, error);
await client.chat.postMessage({
channel: responseChannel,
text: `:x: Failed to update organization: ${error.message}`,
thread_ts: responseThreadTs,
});
}
})();
} catch (error) {
log.error('Error updating organization:', error);
}
Expand Down
94 changes: 78 additions & 16 deletions test/support/slack/actions/onboard-llmo-modal.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1188,7 +1188,62 @@ example-com:
expect(mockAck).to.have.been.calledOnce;
});

it('should print error message if onboarding throws an error', async () => {
it('should return immediately after ack without waiting for onboarding to complete', async () => {
const mockBody = {
view: {
state: {
values: {
brand_name_input: {
brand_name: { value: 'Test Brand' },
},
ims_org_input: {
ims_org_id: { value: 'ABC123@AdobeOrg' },
},
delivery_type_input: {
delivery_type: {
selected_option: { value: 'aem_edge' },
},
},
},
},
private_metadata: JSON.stringify({
originalChannel: 'C1234567890',
originalThreadTs: '1234567890.123456',
brandURL: 'https://example.com',
}),
},
user: { id: 'U1234567890' },
};

const mockAck = sandbox.stub();
const mockClient = {
chat: {
postMessage: sandbox.stub().resolves(),
},
};

// Create a slow onboarding process to verify fire-and-forget behavior
const lambdaCtx = createDefaultMockLambdaCtx(sandbox);
const slowOnboardingPromise = new Promise((resolve) => {
setTimeout(resolve, 1000); // Simulate slow operation
});
lambdaCtx.dataAccess.Site.findByBaseURL = sandbox.stub().returns(slowOnboardingPromise);

const { onboardLLMOModal } = mockedModule;
const handler = onboardLLMOModal(lambdaCtx);

const startTime = Date.now();
await handler({ ack: mockAck, body: mockBody, client: mockClient });
const endTime = Date.now();

// Handler should return quickly (well under 1 second), not wait for the slow operation
expect(endTime - startTime).to.be.lessThan(500);
expect(mockAck).to.have.been.calledOnce;
expect(mockAck).to.have.been.calledWith(); // Called without errors
expect(lambdaCtx.log.debug).to.have.been.calledWith('Onboard LLMO modal processed for user U1234567890, site https://example.com');
});

it('should log error if onboarding throws an error in background', async () => {
const mockBody = {
view: {
state: {
Expand Down Expand Up @@ -1224,13 +1279,20 @@ example-com:

await handler({ ack: mockAck, body: mockBody, client: mockClient });

expect(lambdaCtx.log.error).to.have.been.calledWith('Error handling onboard site modal:', sinon.match.instanceOf(Error));
expect(mockAck).to.have.been.calledWith({
response_action: 'errors',
errors: {
brand_name_input: 'There was an error processing the onboarding request.',
},
// The handler should acknowledge successfully even if onboarding will fail
expect(mockAck).to.have.been.calledOnce;
expect(mockAck).to.have.been.calledWith();

// Wait a bit for the background promise to settle
await new Promise((resolve) => {
setTimeout(resolve, 100);
});

// The error should be logged by the background error handler
expect(lambdaCtx.log.error).to.have.been.calledWith(
sinon.match(/Error in background onboarding for site/),
sinon.match.instanceOf(Error),
);
});

it('should return validation error when IMS org ID is not provided', async () => {
Expand Down Expand Up @@ -1572,9 +1634,10 @@ example-com:
};

const mockAck = sandbox.stub();
const postMessageStub = sandbox.stub().resolves();
const mockClient = {
chat: {
postMessage: sandbox.stub(),
postMessage: postMessageStub,
update: sandbox.stub().resolves(),
},
views: {
Expand All @@ -1593,6 +1656,7 @@ example-com:

await handler({ ack: mockAck, body: mockBody, client: mockClient });

// Verify immediate synchronous behavior
expect(mockAck).to.have.been.called;

// Check that the original message was updated to prevent re-triggering
Expand All @@ -1603,8 +1667,8 @@ example-com:
blocks: [],
});

// Check that the function completed successfully by verifying the debug log
expect(lambdaCtx.log.debug).to.have.been.calledWith('Added entitlements for site site123 (https://example.com) for user user123');
// Verify site was looked up
expect(mockSiteModel.findById).to.have.been.calledWith('site123');
});

it('should handle errors during entitlement addition', async () => {
Expand Down Expand Up @@ -2028,13 +2092,11 @@ example-com:

await handler({ ack: mockAck, body: mockBody, client: mockClient });

// Verify immediate synchronous behavior
expect(mockAck).to.have.been.called;
expect(mockClient.chat.postMessage).to.have.been.calledWith({
channel: 'channel123',
text: ':white_check_mark: Successfully updated organization and applied LLMO entitlements for *https://example.com* (brand: *Test Brand*)',
thread_ts: 'thread123',
});
expect(lambdaCtx.log.debug).to.have.been.calledWith('Updated org and applied entitlements for site site123 (https://example.com) for user user123');

// Verify site was looked up
expect(mockSiteModel.findById).to.have.been.calledWith('site123');
});

it('should return validation error when IMS org ID is not provided', async () => {
Expand Down