Code Review Fixes (#1052)

* use existing Alloy DOM utils * removed unused utils method * renamed to "test:unit:debug" * added a try/catch to handle potential JSON parsing errors * removed ensureSchemaBasedRulesetConsequences * using shorthand object property notation * new line * constants shared between components moved to the shared src/constants module * use alloy utils method includes * use alloy utils method values, objectOf * lint * removeElementById for readability --------- Co-authored-by: Jason Waters <[email protected]>
adobe · Oct 18, 2023 · da6857b · da6857b
1 parent f738fcd
commit da6857b
Show file tree

Hide file tree

Showing 30 changed files with 60 additions and 129 deletions.
diff --git a/package.json b/package.json
@@ -14,7 +14,7 @@
     "format": "prettier --write \"*.{html,js}\" \"{sandbox,src,test,scripts}/**/*.{html,js}\"",
     "test": "npm run test:unit && npm run test:scripts && npm run test:functional",
     "test:unit": "karma start --single-run",
-    "testdebug": "karma start --browsers=Chrome --single-run=false --debug",
+    "test:unit:debug": "karma start --browsers=Chrome --single-run=false --debug",
     "test:unit:watch": "karma start",
     "test:unit:saucelabs:local": "karma start karma.saucelabs.conf.js --single-run",
     "test:unit:coverage": "karma start --single-run --reporters spec,coverage",

diff --git a/sandbox/src/components/InAppMessagesDemo/InAppMessagesStyle.css b/sandbox/src/components/InAppMessagesDemo/InAppMessagesStyle.css
@@ -4,4 +4,4 @@
 }
 .element-spacing {
     margin-right: 10px;
-}
+}
diff --git a/src/components/DecisioningEngine/consequenceAdapters/inAppMessageConsequenceAdapter.js b/src/components/DecisioningEngine/consequenceAdapters/inAppMessageConsequenceAdapter.js
@@ -9,8 +9,8 @@ the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTA
 OF ANY KIND, either express or implied. See the License for the specific language
 governing permissions and limitations under the License.
 */
-import { MESSAGE_IN_APP } from "../../Personalization/constants/schema";
-import { TEXT_HTML } from "../../Personalization/constants/contentType";
+import { MESSAGE_IN_APP } from "../../../constants/schema";
+import { TEXT_HTML } from "../../../constants/contentType";
 
 export default (id, type, detail) => {
   const { html, mobileParameters } = detail;

diff --git a/src/components/DecisioningEngine/createEvaluableRulesetPayload.js b/src/components/DecisioningEngine/createEvaluableRulesetPayload.js
@@ -10,10 +10,7 @@ OF ANY KIND, either express or implied. See the License for the specific languag
 governing permissions and limitations under the License.
 */
 import RulesEngine from "@adobe/aep-rules-engine";
-import {
-  JSON_CONTENT_ITEM,
-  RULESET_ITEM
-} from "../Personalization/constants/schema";
+import { JSON_CONTENT_ITEM, RULESET_ITEM } from "../../constants/schema";
 import { DISPLAY } from "../../constants/eventType";
 import { getActivityId } from "./utils";
 
@@ -31,14 +28,20 @@ const isRulesetItem = item => {
     return false;
   }
 
-  const content =
-    typeof data.content === "string" ? JSON.parse(data.content) : data.content;
+  try {
+    const content =
+      typeof data.content === "string"
+        ? JSON.parse(data.content)
+        : data.content;
 
-  return (
-    content &&
-    Object.prototype.hasOwnProperty.call(content, "version") &&
-    Object.prototype.hasOwnProperty.call(content, "rules")
-  );
+    return (
+      content &&
+      Object.prototype.hasOwnProperty.call(content, "version") &&
+      Object.prototype.hasOwnProperty.call(content, "rules")
+    );
+  } catch (error) {
+    return false;
+  }
 };
 
 export default (payload, eventRegistry, decisionHistory) => {

diff --git a/src/components/DecisioningEngine/createOnResponseHandler.js b/src/components/DecisioningEngine/createOnResponseHandler.js
@@ -9,7 +9,7 @@ the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTA
 OF ANY KIND, either express or implied. See the License for the specific language
 governing permissions and limitations under the License.
 */
-import { PERSONALIZATION_DECISIONS_HANDLE } from "../Personalization/constants/handle";
+import { PERSONALIZATION_DECISIONS_HANDLE } from "../../constants/handle";
 import flattenObject from "../../utils/flattenObject";
 
 export default ({

diff --git a/src/components/DecisioningEngine/index.js b/src/components/DecisioningEngine/index.js
@@ -16,7 +16,6 @@ import createApplyResponse from "./createApplyResponse";
 import createEventRegistry from "./createEventRegistry";
 import createContextProvider from "./createContextProvider";
 import createSubscribeRulesetItems from "./createSubscribeRulesetItems";
-import { ensureSchemaBasedRulesetConsequences } from "./utils";
 import {
   CONTEXT_KEY,
   MOBILE_EVENT_SOURCE,
@@ -56,8 +55,6 @@ const createDecisioningEngine = ({ config, createNamespacedStorage }) => {
         decisionContext = {},
         onResponse = noop
       }) {
-        ensureSchemaBasedRulesetConsequences(event);
-
         onResponse(
           createOnResponseHandler({
             renderDecisions,

diff --git a/src/components/DecisioningEngine/utils.js b/src/components/DecisioningEngine/utils.js
@@ -42,17 +42,6 @@ export const getExpirationDate = retentionPeriod => {
   expirationDate.setDate(expirationDate.getDate() - retentionPeriod);
   return expirationDate;
 };
-
-export const ensureSchemaBasedRulesetConsequences = event => {
-  event.mergeData({
-    __adobe: {
-      ajo: {
-        "in-app-response-format": 2
-      }
-    }
-  });
-};
-
 export const getActivityId = proposition => {
   const { scopeDetails = {} } = proposition;
   const { activity = {} } = scopeDetails;

diff --git a/src/components/Personalization/createApplyPropositions.js b/src/components/Personalization/createApplyPropositions.js
@@ -15,7 +15,7 @@ import {
   DOM_ACTION,
   HTML_CONTENT_ITEM,
   MESSAGE_IN_APP
-} from "./constants/schema";
+} from "../../constants/schema";
 import PAGE_WIDE_SCOPE from "../../constants/pageWideScope";
 
 const SUPPORTED_SCHEMAS = [DOM_ACTION, HTML_CONTENT_ITEM, MESSAGE_IN_APP];

diff --git a/src/components/Personalization/createPersonalizationDetails.js b/src/components/Personalization/createPersonalizationDetails.js
@@ -22,7 +22,7 @@ import {
   REDIRECT_ITEM,
   RULESET_ITEM,
   MESSAGE_FEED_ITEM
-} from "./constants/schema";
+} from "../../constants/schema";
 
 const addPageWideScope = scopes => {
   if (!includes(scopes, PAGE_WIDE_SCOPE)) {

diff --git a/src/components/Personalization/createPreprocessors.js b/src/components/Personalization/createPreprocessors.js
@@ -9,7 +9,7 @@ the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTA
 OF ANY KIND, either express or implied. See the License for the specific language
 governing permissions and limitations under the License.
 */
-import { DOM_ACTION } from "./constants/schema";
+import { DOM_ACTION } from "../../constants/schema";
 import remapHeadOffers from "./dom-actions/remapHeadOffers";
 import remapCustomCodeOffers from "./dom-actions/remapCustomCodeOffers";
 

diff --git a/src/components/Personalization/createSubscribeMessageFeed.js b/src/components/Personalization/createSubscribeMessageFeed.js
@@ -14,7 +14,7 @@ import {
   objectOf,
   string
 } from "../../utils/validation";
-import { MESSAGE_FEED_ITEM } from "./constants/schema";
+import { MESSAGE_FEED_ITEM } from "../../constants/schema";
 import { DISPLAY, INTERACT } from "../../constants/eventType";
 
 const validateOptions = ({ options }) => {

diff --git a/src/components/Personalization/createViewCacheManager.js b/src/components/Personalization/createViewCacheManager.js
@@ -12,7 +12,7 @@ governing permissions and limitations under the License.
 
 import { assign, groupBy } from "../../utils";
 import defer from "../../utils/defer";
-import { DEFAULT_CONTENT_ITEM } from "./constants/schema";
+import { DEFAULT_CONTENT_ITEM } from "../../constants/schema";
 
 export default ({ createProposition }) => {
   const viewStorage = {};

diff --git a/src/components/Personalization/handlers/createProcessInAppMessage.js b/src/components/Personalization/handlers/createProcessInAppMessage.js
@@ -9,7 +9,7 @@ the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTA
 OF ANY KIND, either express or implied. See the License for the specific language
 governing permissions and limitations under the License.
 */
-import { APPLICATION_JSON } from "../constants/contentType";
+import { APPLICATION_JSON } from "../../../constants/contentType";
 
 const DEFAULT_CONTENT = "defaultContent";
 

diff --git a/src/components/Personalization/in-app-message-actions/actions/displayIframeContent.js b/src/components/Personalization/in-app-message-actions/actions/displayIframeContent.js
@@ -11,9 +11,11 @@ governing permissions and limitations under the License.
 */
 
 import { getNonce } from "../../dom-actions/dom";
-import { createElement, parseAnchor, removeElementById } from "../utils";
-import { TEXT_HTML } from "../../constants/contentType";
-import { assign } from "../../../../utils";
+import { parseAnchor, removeElementById } from "../utils";
+import { TEXT_HTML } from "../../../../constants/contentType";
+import { assign, includes, values } from "../../../../utils";
+import { createNode } from "../../../../utils/dom";
+import { objectOf } from "../../../../utils/validation";
 import { PropositionEventType } from "../../../../constants/propositionEventType";
 import { INTERACT } from "../../../../constants/eventType";
 
@@ -27,7 +29,7 @@ const dismissMessage = () =>
   );
 
 const setWindowLocationHref = link => {
-  window.location.assign(link);
+  window.location.href = link;
 };
 
 export const createIframeClickHandler = (
@@ -74,11 +76,13 @@ export const createIframe = (htmlContent, clickHandler) => {
   if (scriptTag) {
     scriptTag.setAttribute("nonce", getNonce());
   }
-  const element = document.createElement("iframe");
-  element.src = URL.createObjectURL(
-    new Blob([htmlDocument.documentElement.outerHTML], { type: TEXT_HTML })
-  );
-  element.id = ALLOY_IFRAME_ID;
+  const element = createNode("iframe", {
+    src: URL.createObjectURL(
+      new Blob([htmlDocument.documentElement.outerHTML], { type: "text/html" })
+    ),
+    id: ALLOY_IFRAME_ID
+  });
+
   element.addEventListener("load", () => {
     const { addEventListener } =
       element.contentDocument || element.contentWindow.document;
@@ -190,32 +194,27 @@ const isValidWebParameters = webParameters => {
 
   const ids = Object.keys(webParameters);
 
-  if (!ids.includes(ALLOY_MESSAGING_CONTAINER_ID)) {
+  if (!includes(ids, ALLOY_MESSAGING_CONTAINER_ID)) {
     return false;
   }
 
-  if (!ids.includes(ALLOY_OVERLAY_CONTAINER_ID)) {
+  if (!includes(ids, ALLOY_OVERLAY_CONTAINER_ID)) {
     return false;
   }
 
-  const values = Object.values(webParameters);
+  const valuesArray = values(webParameters);
 
-  for (let i = 0; i < values.length; i += 1) {
-    if (!Object.prototype.hasOwnProperty.call(values[i], "style")) {
+  for (let i = 0; i < valuesArray.length; i += 1) {
+    if (!objectOf(valuesArray[i], "style")) {
       return false;
     }
 
-    if (!Object.prototype.hasOwnProperty.call(values[i], "params")) {
+    if (!objectOf(valuesArray[i], "params")) {
       return false;
     }
 
     for (let j = 0; j < REQUIRED_PARAMS.length; j += 1) {
-      if (
-        !Object.prototype.hasOwnProperty.call(
-          values[i].params,
-          REQUIRED_PARAMS[j]
-        )
-      ) {
+      if (!objectOf(valuesArray[i].params, REQUIRED_PARAMS[j])) {
         return false;
       }
     }
@@ -272,9 +271,9 @@ export const displayHTMLContentInIframe = (settings = {}, interact) => {
     return;
   }
 
-  const container = createElement(ALLOY_MESSAGING_CONTAINER_ID);
+  const container = createNode("div", { id: ALLOY_MESSAGING_CONTAINER_ID });
   const iframe = createIframe(content, createIframeClickHandler(interact));
-  const overlay = createElement(ALLOY_OVERLAY_CONTAINER_ID);
+  const overlay = createNode("div", { id: ALLOY_OVERLAY_CONTAINER_ID });
 
   if (!isValidWebParameters(webParameters)) {
     webParameters = generateWebParameters(mobileParameters);

diff --git a/src/components/Personalization/in-app-message-actions/utils.js b/src/components/Personalization/in-app-message-actions/utils.js
@@ -10,36 +10,14 @@ OF ANY KIND, either express or implied. See the License for the specific languag
 governing permissions and limitations under the License.
 */
 import { startsWith } from "../../../utils";
-
-export const addStyle = (styleTagId, cssText) => {
-  const existingStyle = document.getElementById(styleTagId);
-  if (existingStyle) {
-    existingStyle.remove();
-  }
-
-  const styles = document.createElement("style");
-  styles.id = styleTagId;
-
-  styles.appendChild(document.createTextNode(cssText));
-  document.head.appendChild(styles);
-};
-
-export const removeElements = cssClassName => {
-  [...document.getElementsByClassName(cssClassName)].forEach(element => {
-    if (!element) {
-      return;
-    }
-    element.remove();
-  });
-};
+import { removeNode } from "../../../utils/dom";
 
 export const removeElementById = id => {
   const element = document.getElementById(id);
   if (element) {
-    element.remove();
+    removeNode(element);
   }
 };
-
 export const parseAnchor = anchor => {
   const nothing = {};
 
@@ -74,8 +52,3 @@ export const parseAnchor = anchor => {
     uuid
   };
 };
-export const createElement = elementTagId => {
-  const element = document.createElement("div");
-  element.id = elementTagId;
-  return element;
-};
diff --git a/src/components/Personalization/index.js b/src/components/Personalization/index.js
@@ -31,7 +31,7 @@ import remapHeadOffers from "./dom-actions/remapHeadOffers";
 import createPreprocess from "./dom-actions/createPreprocess";
 import injectCreateProposition from "./handlers/injectCreateProposition";
 import createAsyncArray from "./utils/createAsyncArray";
-import * as schema from "./constants/schema";
+import * as schema from "../../constants/schema";
 import processDefaultContent from "./handlers/processDefaultContent";
 import { isPageWideSurface } from "./utils/surfaceUtils";
 import createProcessDomAction from "./handlers/createProcessDomAction";

diff --git a/.../Personalization/constants/contentType.js → src/constants/contentType.js b/.../Personalization/constants/contentType.js → src/constants/contentType.js
diff --git a/...nents/Personalization/constants/handle.js → src/constants/handle.js b/...nents/Personalization/constants/handle.js → src/constants/handle.js
diff --git a/...nents/Personalization/constants/schema.js → src/constants/schema.js b/...nents/Personalization/constants/schema.js → src/constants/schema.js
diff --git a/...s/components/DecisioningEngine/consequenceAdapters/inAppMessageConsequenceAdapter.spec.js b/...s/components/DecisioningEngine/consequenceAdapters/inAppMessageConsequenceAdapter.spec.js
@@ -10,7 +10,7 @@ OF ANY KIND, either express or implied. See the License for the specific languag
 governing permissions and limitations under the License.
 */
 import inAppMessageConsequenceAdapter from "../../../../../../src/components/DecisioningEngine/consequenceAdapters/inAppMessageConsequenceAdapter";
-import { TEXT_HTML } from "../../../../../../src/components/Personalization/constants/contentType";
+import { TEXT_HTML } from "../../../../../../src/constants/contentType";
 
 describe("DecisioningEngine:inAppMessageConsequenceAdapter", () => {
   it("handles cjmiam", () => {

diff --git a/...ecs/components/DecisioningEngine/consequenceAdapters/schemaTypeConsequenceAdapter.spec.js b/...ecs/components/DecisioningEngine/consequenceAdapters/schemaTypeConsequenceAdapter.spec.js
@@ -11,7 +11,7 @@ governing permissions and limitations under the License.
 */
 
 import schemaTypeConsequenceAdapter from "../../../../../../src/components/DecisioningEngine/consequenceAdapters/schemaTypeConsequenceAdapter";
-import { TEXT_HTML } from "../../../../../../src/components/Personalization/constants/contentType";
+import { TEXT_HTML } from "../../../../../../src/constants/contentType";
 
 describe("DecisioningEngine:schemaTypeConsequenceAdapter", () => {
   it("handles schema", () => {

diff --git a/test/unit/specs/components/DecisioningEngine/createConsequenceAdapter.spec.js b/test/unit/specs/components/DecisioningEngine/createConsequenceAdapter.spec.js
@@ -10,7 +10,7 @@ OF ANY KIND, either express or implied. See the License for the specific languag
 governing permissions and limitations under the License.
 */
 import createConsequenceAdapter from "../../../../../src/components/DecisioningEngine/createConsequenceAdapter";
-import { TEXT_HTML } from "../../../../../src/components/Personalization/constants/contentType";
+import { TEXT_HTML } from "../../../../../src/constants/contentType";
 
 describe("DecisioningEngine:createConsequenceAdapter", () => {
   const ADAPTED_CONSEQUENCE = {

diff --git a/test/unit/specs/components/DecisioningEngine/createSubscribeRulesetItems.spec.js b/test/unit/specs/components/DecisioningEngine/createSubscribeRulesetItems.spec.js
@@ -11,7 +11,7 @@ governing permissions and limitations under the License.
 */
 import { DOM_ACTION } from "@adobe/alloy/libEs5/components/Personalization/constants/schema";
 import createSubscribeRulesetItems from "../../../../../src/components/DecisioningEngine/createSubscribeRulesetItems";
-import { MESSAGE_FEED_ITEM } from "../../../../../src/components/Personalization/constants/schema";
+import { MESSAGE_FEED_ITEM } from "../../../../../src/constants/schema";
 
 describe("DecisioningEngine:subscribeRulesetItems", () => {
   let subscribeRulesetItems;

diff --git a/test/unit/specs/components/DecisioningEngine/index.spec.js b/test/unit/specs/components/DecisioningEngine/index.spec.js
@@ -138,34 +138,4 @@ describe("createDecisioningEngine:commands:evaluateRulesets", () => {
       propositions: [proposition]
     });
   });
-
-  it("ensures schema-based ruleset consequences", () => {
-    onResponseHandler = onResponse => {
-      onResponse({
-        response: mockRulesetResponseWithCondition({
-          definition: {
-            key: "referringPage.path",
-            matcher: "eq",
-            values: ["/search"]
-          },
-          type: "matcher"
-        })
-      });
-    };
-
-    decisioningEngine.lifecycle.onBeforeEvent({
-      event: mockEvent,
-      renderDecisions: false,
-      decisionContext: {},
-      onResponse: onResponseHandler
-    });
-
-    expect(mergeData).toHaveBeenCalledOnceWith({
-      __adobe: {
-        ajo: {
-          "in-app-response-format": 2
-        }
-      }
-    });
-  });
 });
diff --git a/test/unit/specs/components/Personalization/createOnDecisionHandler.spec.js b/test/unit/specs/components/Personalization/createOnDecisionHandler.spec.js
@@ -10,7 +10,7 @@ OF ANY KIND, either express or implied. See the License for the specific languag
 governing permissions and limitations under the License.
 */
 import createOnDecisionHandler from "../../../../../src/components/Personalization/createOnDecisionHandler";
-import { MESSAGE_FEED_ITEM } from "../../../../../src/components/Personalization/constants/schema";
+import { MESSAGE_FEED_ITEM } from "../../../../../src/constants/schema";
 import injectCreateProposition from "../../../../../src/components/Personalization/handlers/injectCreateProposition";
 
 describe("Personalization::createOnDecisionHandler", () => {
-Original file line number
+Diff line change
@@ Expand Up / @@ -4,4 +4,4 @@ @@
     }
     .element-spacing {
         margin-right: 10px;
-    }
+    }