Fix validation of modified declarations

CHANGELOG.md

@@ -2,6 +2,15 @@
 
 All changes that impact users of this module are documented in this file, in the [Common Changelog](https://common-changelog.org) format with some additional specifications defined in the CONTRIBUTING file. This codebase adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## Unreleased [patch]
+
+> Development of this release was supported by the [French Ministry for Foreign Affairs](https://www.diplomatie.gouv.fr/fr/politique-etrangere-de-la-france/diplomatie-numerique/) through its ministerial [State Startups incubator](https://beta.gouv.fr/startups/open-terms-archive.html) under the aegis of the Ambassador for Digital Affairs.
+
+### Fixed
+
+- Fix validation of modified declarations when new services were added
+- Fix validation of modified declarations when filters were modified
+
 ## 1.1.1 - 2024-03-28
 
 _Full changeset and discussions: [#1065](https://github.com/OpenTermsArchive/engine/pull/1065)._

package.json

@@ -65,7 +65,6 @@
     "croner": "^4.3.6",
     "cross-env": "^7.0.3",
     "datauri": "^4.1.0",
-    "deep-diff": "^1.0.2",
     "dotenv": "^10.0.0",
     "eslint": "^8.53.0",
     "eslint-config-airbnb-base": "^15.0.0",

scripts/declarations/lint/index.mocha.js

@@ -31,12 +31,12 @@
   if (options.modified) {
     const declarationUtils = new DeclarationUtils(instancePath);
 
-    ({ services: servicesToValidate } = await declarationUtils.getModifiedServiceTermsTypes());
+    ({ services: servicesToValidate } = await declarationUtils.getModifiedServicesAndTermsTypes());
   }
 
   const lintFile = lintAndFixFile(options.fix);
 
   describe('Service declarations lint validation', async function () {
     this.timeout(30000);
 
     servicesToValidate.forEach(serviceId => {
@@ -46,8 +46,8 @@
       const filtersFilePath = path.join(declarationsPath, `${serviceId}.filters.js`);
       const filtersHistoryFilePath = path.join(declarationsPath, `${serviceId}.filters.history.js`);
 
       context(serviceId, async () => {
         before(async function () {
           if (!service) {
             console.log('      (Tests skipped as declaration has been archived)');
             this.skip();

scripts/declarations/utils/fixtures/serviceA.json

@@ -0,0 +1,13 @@
+{
+  "name": "Service",
+  "documents": {
+    "Terms of Service": {
+      "fetch": "https://domain.example/tos",
+      "select": "body"
+    },
+    "Privacy Policy": {
+      "fetch": "https://domain.example/privacy",
+      "select": "body"
+    }
+  }
+}

scripts/declarations/utils/fixtures/serviceAMultipleTermsUpdated.json

@@ -0,0 +1,20 @@
+{
+  "name": "Service",
+  "documents": {
+    "Terms of Service": {
+      "fetch": "https://domain.example/tos",
+      "select": "body",
+      "remove": "footer"
+    },
+    "Privacy Policy": {
+      "fetch": "https://domain.example/privacy",
+      "select": "body",
+      "remove": "footer"
+    },
+    "Imprint": {
+      "fetch": "https://domain.example/imprint",
+      "select": "body",
+      "remove": "footer"
+    }
+  }
+}

scripts/declarations/utils/fixtures/serviceATermsAdded.json

@@ -0,0 +1,17 @@
+{
+  "name": "Service",
+  "documents": {
+    "Terms of Service": {
+      "fetch": "https://domain.example/tos",
+      "select": "body"
+    },
+    "Privacy Policy": {
+      "fetch": "https://domain.example/privacy",
+      "select": "body"
+    },
+    "Imprint": {
+      "fetch": "https://domain.example/imprint",
+      "select": "body"
+    }
+  }
+}

scripts/declarations/utils/fixtures/serviceATermsRemoved.json

@@ -0,0 +1,9 @@
+{
+  "name": "Service",
+  "documents": {
+    "Terms of Service": {
+      "fetch": "https://domain.example/tos",
+      "select": "body"
+    }
+  }
+}

scripts/declarations/utils/fixtures/serviceATermsUpdated.json

@@ -0,0 +1,14 @@
+{
+  "name": "Service",
+  "documents": {
+    "Terms of Service": {
+      "fetch": "https://domain.example/tos",
+      "select": "body",
+      "remove": "footer"
+    },
+    "Privacy Policy": {
+      "fetch": "https://domain.example/privacy",
+      "select": "body"
+    }
+  }
+}

scripts/declarations/utils/fixtures/serviceB.json

@@ -0,0 +1,9 @@
+{
+  "name": "Service B",
+  "documents": {
+    "Terms of Service": {
+      "fetch": "https://domain.example/tos",
+      "select": "body"
+    }
+  }
+}

scripts/declarations/utils/index.js

@@ -1,6 +1,5 @@
 import path from 'path';
 
-import DeepDiff from 'deep-diff';
 import simpleGit from 'simple-git';
 
 export default class DeclarationUtils {
@@ -9,13 +8,17 @@ export default class DeclarationUtils {
     this.defaultBranch = defaultBranch;
   }
 
-  static filePathToServiceId = filePath => path.parse(filePath.replace(/\.history|\.filters/, '')).name;
+  static getServiceIdFromFilePath(filePath) {
+    return path.parse(filePath.replace(/\.history|\.filters/, '')).name;
+  }
 
-  async getJSONFile(path, ref) {
+  async getJSONFromFile(ref, filePath) {
     try {
-      return JSON.parse(await this.git.show([`${ref}:${path}`]));
-    } catch (e) {
-      return {};
+      const fileContent = await this.git.show([`${ref}:${filePath}`]);
+
+      return JSON.parse(fileContent);
+    } catch (error) {
+      // the file does not exist on the requested branch or it is not parsable
     }
   }
 
@@ -24,7 +27,7 @@ export default class DeclarationUtils {
 
     const modifiedFilePaths = (modifiedFilePathsAsString ? modifiedFilePathsAsString.split('\0') : []).filter(str => str !== ''); // split on \0 rather than \n due to the -z option of git diff
 
-    return { modifiedFilePaths, modifiedServicesIds: Array.from(new Set(modifiedFilePaths.map(DeclarationUtils.filePathToServiceId))) };
+    return { modifiedFilePaths, modifiedServicesIds: Array.from(new Set(modifiedFilePaths.map(DeclarationUtils.getServiceIdFromFilePath))) };
   }
 
   async getModifiedServices() {
@@ -33,48 +36,47 @@ export default class DeclarationUtils {
     return modifiedServicesIds;
   }
 
-  async getModifiedServiceTermsTypes() {
-    const { modifiedFilePaths, modifiedServicesIds } = await this.getModifiedData();
+  async getModifiedServicesAndTermsTypes() {
+    const { modifiedFilePaths } = await this.getModifiedData();
     const servicesTermsTypes = {};
 
     await Promise.all(modifiedFilePaths.map(async modifiedFilePath => {
-      const serviceId = DeclarationUtils.filePathToServiceId(modifiedFilePath);
+      const serviceId = DeclarationUtils.getServiceIdFromFilePath(modifiedFilePath);
 
-      if (!modifiedFilePath.endsWith('.json')) {
-        // Here we should compare AST of both files to detect on which function
-        // change has been made, and then find which terms type depends on this
-        // function.
-        // As this is a complicated process, we will just send back all terms types
-        const declaration = await this.getJSONFile(`declarations/${serviceId}.json`, this.defaultBranch);
+      if (modifiedFilePath.endsWith('.filters.js')) {
+        const declaration = await this.getJSONFromFile(this.defaultBranch, `declarations/${serviceId}.json`);
 
-        return Object.keys(declaration.documents);
+        servicesTermsTypes[serviceId] = Object.keys(declaration.documents); // Considering how rarely filters are used, simply return all term types that could potentially be impacted to spare implementing a function change check
+
+        return;
       }
 
-      const defaultFile = await this.getJSONFile(modifiedFilePath, this.defaultBranch);
-      const modifiedFile = await this.getJSONFile(modifiedFilePath, 'HEAD');
+      const originalService = await this.getJSONFromFile(this.defaultBranch, modifiedFilePath);
+      const modifiedService = await this.getJSONFromFile('HEAD', modifiedFilePath);
+      const modifiedServiceTermsTypes = Object.keys(modifiedService.documents);
 
-      const diff = DeepDiff.diff(defaultFile, modifiedFile);
+      if (!originalService) {
+        servicesTermsTypes[serviceId] = modifiedServiceTermsTypes;
 
-      if (!diff) {
-        // This can happen if only a lint has been applied to a document
         return;
       }
 
-      const modifiedTermsTypes = diff.reduce((acc, { path }) => {
-        if (modifiedFilePath.includes('.history')) {
-          acc.add(path[0]);
-        } else if (path[0] == 'documents') {
-          acc.add(path[1]);
-        }
+      const originalServiceTermsTypes = Object.keys(originalService.documents);
+
+      modifiedServiceTermsTypes.forEach(termsType => {
+        const areTermsAdded = !originalServiceTermsTypes.includes(termsType);
+        const areTermsModified = JSON.stringify(originalService.documents[termsType]) != JSON.stringify(modifiedService.documents[termsType]);
 
-        return acc;
-      }, new Set());
+        if (!areTermsAdded && !areTermsModified) {
+          return;
+        }
 
-      servicesTermsTypes[serviceId] = Array.from(new Set([ ...servicesTermsTypes[serviceId] || [], ...modifiedTermsTypes ]));
+        servicesTermsTypes[serviceId] = [termsType].concat(servicesTermsTypes[serviceId] || []);
+      });
     }));
 
     return {
-      services: modifiedServicesIds,
+      services: Object.keys(servicesTermsTypes),
       servicesTermsTypes,
     };
   }