fix: Grouping of URLs for a site fail when no CWV handler exist in th…

…e site config
adobe · Dec 11, 2024 · 91e856b · 91e856b
1 parent 4cb5175
commit 91e856b
Show file tree

Hide file tree

Showing 6 changed files with 41 additions and 25 deletions.
diff --git a/src/controllers/audits.js b/src/controllers/audits.js
@@ -155,11 +155,15 @@ function AuditsController(dataAccess) {
       return notFound('Site not found');
     }
 
-    const config = site.getConfig();
-    const handlerConfig = config.getHandlerConfig(auditType);
-    if (!handlerConfig) {
-      return notFound('Audit type not found');
+    const configuration = await dataAccess.getConfiguration();
+    const registeredAudits = configuration.getHandlers();
+    if (!registeredAudits[auditType]) {
+      return notFound(`The "${auditType}" is not present in the configuration. List of allowed audits:`
+        + ` ${Object.keys(registeredAudits).join(', ')}.`);
     }
+
+    const siteConfig = site.getConfig();
+
     if (Array.isArray(excludedURLs)) {
       for (const url of excludedURLs) {
         if (!isValidUrl(url)) {
@@ -171,9 +175,9 @@ function AuditsController(dataAccess) {
 
       const newExcludedURLs = excludedURLs.length === 0
         ? []
-        : Array.from(new Set([...(config.getExcludedURLs(auditType) || []), ...excludedURLs]));
+        : Array.from(new Set([...(siteConfig.getExcludedURLs(auditType) || []), ...excludedURLs]));
 
-      config.updateExcludedURLs(auditType, newExcludedURLs);
+      siteConfig.updateExcludedURLs(auditType, newExcludedURLs);
     }
 
     if (Array.isArray(manualOverwrites)) {
@@ -195,12 +199,12 @@ function AuditsController(dataAccess) {
 
       hasUpdates = true;
 
-      const existingOverrides = config.getManualOverwrites(auditType);
+      const existingOverrides = siteConfig.getManualOverwrites(auditType);
       const newManualOverwrites = manualOverwrites.length === 0
         ? []
         : mergeOverrides(existingOverrides, manualOverwrites);
 
-      config.updateManualOverwrites(auditType, newManualOverwrites);
+      siteConfig.updateManualOverwrites(auditType, newManualOverwrites);
     }
 
     if (Array.isArray(groupedURLs)) {
@@ -211,19 +215,20 @@ function AuditsController(dataAccess) {
       }
       hasUpdates = true;
 
-      const currentGroupedURLs = config.getGroupedURLs(auditType) || [];
+      const currentGroupedURLs = siteConfig.getGroupedURLs(auditType) || [];
       const patchedGroupedURLs = groupedURLs.length === 0
         ? []
         : Array.from(new Set([...currentGroupedURLs, ...groupedURLs]));
 
-      config.updateGroupedURLs(auditType, patchedGroupedURLs);
+      siteConfig.updateGroupedURLs(auditType, patchedGroupedURLs);
     }
 
     if (hasUpdates) {
-      const configObj = Config.toDynamoItem(config);
+      const configObj = Config.toDynamoItem(siteConfig);
       site.updateConfig(configObj);
       await dataAccess.updateSite(site);
-      return ok(handlerConfig);
+      const auditConfig = siteConfig.getHandlerConfig(auditType);
+      return ok(auditConfig);
     }
     return badRequest('No updates provided');
   };

diff --git a/src/controllers/sites-audits-toggle.js b/src/controllers/sites-audits-toggle.js
@@ -81,12 +81,12 @@ export default (dataAccess) => {
             return { status: 404, message: `Site with baseURL: ${baseURL} not found.` };
           }
 
-          const validAuditTypes = Object.keys(configuration.getHandlers());
-          if (!validAuditTypes.includes(auditType)) {
+          const registeredAudits = configuration.getHandlers();
+          if (!registeredAudits[auditType]) {
             return {
               status: 404,
-              message: `The "${auditType}" is not present in the configuration. List of allowed audit`
-                + ` types: ${validAuditTypes.join(', ')}.`,
+              message: `The "${auditType}" is not present in the configuration. List of allowed audits:`
+                + ` ${Object.keys(registeredAudits).join(', ')}.`,
             };
           }
 

diff --git a/src/support/slack/commands/toggle-site-audit.js b/src/support/slack/commands/toggle-site-audit.js
@@ -67,10 +67,10 @@ export default (context) => {
         return;
       }
 
-      const validAuditTypes = Object.keys(configuration.getHandlers());
-      if (!validAuditTypes.includes(auditType)) {
-        await say(`${ERROR_MESSAGE_PREFIX}The "${auditType}" is not present in the configuration.\nList of allowed audit`
-            + ` types:\n${validAuditTypes.join('\n')}.`);
+      const registeredAudits = configuration.getHandlers();
+      if (!registeredAudits[auditType]) {
+        await say(`${ERROR_MESSAGE_PREFIX}The "${auditType}" is not present in the configuration.\nList of allowed`
+            + ` audits:\n${Object.keys(registeredAudits).join('\n')}.`);
         return;
       }
 

diff --git a/test/controllers/audits.test.js b/test/controllers/audits.test.js
@@ -98,8 +98,15 @@ describe('Audits Controller', () => {
     },
   ].map((audit) => createAudit(audit));
 
+  const handlers = { some_audit: {}, 'broken-backlinks': {} };
+
+  const mockConfiguration = {
+    getHandlers: sandbox.stub().returns(handlers),
+  };
+
   const mockDataAccess = {
     getAuditsForSite: sandbox.stub(),
+    getConfiguration: sandbox.stub().resolves(mockConfiguration),
     getLatestAudits: sandbox.stub(),
     getLatestAuditsForSite: sandbox.stub(),
     getLatestAuditForSite: sandbox.stub(),
@@ -540,7 +547,11 @@ describe('Audits Controller', () => {
 
       expect(result.status).to.equal(404);
       const error = await result.json();
-      expect(error).to.have.property('message', 'Audit type not found');
+      expect(error).to.have.property(
+        'message',
+        `The "${auditType}" is not present in the configuration. List of allowed audits:`
+        + ` ${Object.keys(handlers).join(', ')}.`,
+      );
     });
 
     it('merges manual overwrites correctly', async () => {

diff --git a/test/controllers/sites-audits-toggle.test.js b/test/controllers/sites-audits-toggle.test.js
@@ -528,8 +528,8 @@ describe('Sites Audits Controller', () => {
           + 'is not present in the configuration, but it did not match the expected output.',
       ).to.deep.equal({
         status: 404,
-        message: `The "${auditType}" is not present in the configuration. List of allowed audit`
-          + ` types: ${Object.keys(handlers).join(', ')}.`,
+        message: `The "${auditType}" is not present in the configuration. List of allowed audits:`
+          + ` ${Object.keys(handlers).join(', ')}.`,
       });
 
       expect(

diff --git a/test/support/slack/commands/toggle-site-audit.test.js b/test/support/slack/commands/toggle-site-audit.test.js
@@ -270,8 +270,8 @@ describe('UpdateSitesAuditsCommand', () => {
 
       exceptsAtBadRequest();
       expect(
-        slackContextMock.say.calledWith(`${ERROR_MESSAGE_PREFIX}The "${auditType}" is not present in the configuration.\nList of allowed audit`
-          + ` types:\n${Object.keys(handlers).join('\n')}.`),
+        slackContextMock.say.calledWith(`${ERROR_MESSAGE_PREFIX}The "${auditType}" is not present in the configuration.\nList of allowed`
+          + ` audits:\n${Object.keys(handlers).join('\n')}.`),
         'Expected error message was not called',
       ).to.be.true;
     });