CORE-45102 Removed errorsEnabled. No longer failing commands due to consent. by Aaronius · Pull Request #490 · adobe/alloy

Aaronius · 2020-05-06T21:52:03Z

Description

No longer reject command promises if the user has declined consent. Remove errorsEnabled configuration option. We may add it back later if we still find it is necessary, but now that promises aren't rejected due to consent, there will be much fewer failures and any failures that do occur probably deserve attention.

Related Issue

https://jira.corp.adobe.com/browse/CORE-45102

Motivation and Context

I discussed this with the team, then with internal beta customers, then with Joe Khoury and Justin Grover. When a command is executed and the user is opted out, the promise returned from the command will now be resolved rather than rejected. The promise will be resolved with an object, but the object may not contain all the properties it would have had the user opted in. We also decided to remove the errorsEnabled configuration option. We may add it back later, but the primary reason for it, in my opinion and if I recall correctly, was because of the sheer quantity of errors we would be throwing when the customer attempts to execute commands and the user has opted out. Now that executing a command when the user is opted out is not considered an error, the number of errors thrown will be greatly reduced and really will deserve the customer's attention.

For all commands (even commands that have nothing to do with consent, like setDebug), we will always resolve the promise with an object as long as no error is thrown. When we have data to add to the object, we will do so and the data will be segregated into their own properties on the object (for example, decisions will go under result.decisions, identities will go under result.identities). This is somewhat akin to how we have an options parameter for every command for extensibility so that we can add options/properties later if needed.

We discussed always adding another property to the result object like optedOut or even including the consent status for each purpose, so that customers could programmatically know why the result object may not contain the full data they were expecting. This has its own shortcomings, however. We suspect that in the future, when granular consent purposes are introduced, that after the user opted into some purposes and opted out of other purposes, when a command like sendEvent is executed, Alloy may still send the request to Konductor, and Konductor would have logic based on the consent status of each purpose which results in a partial response. As an arbitrary example, maybe it returns identities but not decisions) for a particular request/response due to the users being opted into one purpose but not another. In such a case, Alloy would resolve the command's promise with a result object containing only identities and not decisions.

At that point, having a property on the result object like optedOut is too broad, since the user only opted out of some purposes and not others. Including the consent status for all purposes on the result object doesn't seem to be particularly useful. For example, would the customer use it to determine whether they can access result.decisions? Why wouldn't the customer check to see if result.decisions is defined directly? If customers are wanting to programmatically react to consent-based logic that is occurring within Konductor, then the best way to do that would be to have Konductor provide additional details in its response regarding why certain data was or was not included, which Alloy could then surface on the result object. Until we have granular purposes and this becomes more of a reality, any command that requires consent in order to complete will log a warning if the user is opted out and will resolve the command promise with an empty object.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement (non-breaking change which does not add functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA or I'm an Adobe employee.
I have made any necessary test changes and all tests pass.
I have run the Sandbox successfully.

…onsent.

Aaronius · 2020-05-06T21:55:45Z

src/core/consent/createConsentStateMachine.js

+
+const createDeclinedConsentError = () => {
+  const error = new Error(DECLINED_CONSENT);
+  error.code = DECLINED_CONSENT_ERROR_CODE;


Note that we add a code to the error. This is how, up in Core, we can segregate errors due to consent being declined versus other kinds of errors. There are a few ways we could have done this (e.g., using a different error "class" or detecting using a regex ran against the error's message instead of a checking a code), but I felt like this was a decent way of handling it.

It is decent, and easy to extend and add other important codes in the future, and simple than sub classing. 👍

Aaronius · 2020-05-06T21:58:14Z

src/core/executeCommandFactory.js

+    })
+      .then(result => {
+        // We should always be returning an object from every command.
+        return isObject(result) ? result : {};


This is important to call out. Any time a component's command does not return an object or a promise which resolves to an object, we make sure we return an object to the customer. An alternative to this is to make every component command return an object from within the component's code itself, but we run a greater risk of an engineering creating a new command and forgetting to return an object.

We always return a result object just to be consistent across commands.

Aaronius · 2020-05-06T21:58:41Z

src/core/handleErrorFactory.js

+  // In the case of declined consent, we've opted to not reject the promise
+  // returned to the customer, but instead resolve the promise with an
+  // empty result object.
+  if (error.code === DECLINED_CONSENT_ERROR_CODE) {


Here's where we use the code to segregate consent problems vs non-consent problems.

Aaronius · 2020-05-06T22:01:49Z

test/functional/specs/C2587.js

-})(
-  "Test C2587: Throw error when executing command that doesn't exist",
-  async t => {
-    const { error } = await t.getBrowserConsoleMessages();


This was testing that an error was logged to the console rather than testing that the command's promise was actually rejected.

Aaronius · 2020-05-06T22:02:24Z

test/functional/specs/C2588.js

+  await configureAlloyInstance(orgMainConfigMain);
+  const errorMessage = await configureAlloyInstance(orgMainConfigMain);

-  const { error } = await t.getBrowserConsoleMessages();


This was testing that an error was logged to the console rather than testing that the command's promise was actually rejected.

Aaronius · 2020-05-06T22:04:28Z

test/unit/specs/components/Identity/createComponent.spec.js

-    ]);
+    identityManager = jasmine.createSpyObj("identityManager", {
+      addToPayload: undefined,
+      sync: Promise.resolve()


This is to match how identity is currently functioning.

Aaronius · 2020-05-06T22:07:09Z

test/unit/specs/components/Identity/createComponent.spec.js

-      .then(identities => {
-        ecid = identities.ECID;
-      });
+      .then(onResolved);


I switched this to use a spy because I think it's easier to read and, if something fails (like identity is supposed to be identities, the jasmine output is easier to understand.

jfkhoury · 2020-05-06T22:10:05Z

src/components/EventMerge/index.js

 import { callback } from "../../utils/validation";

+const generateEventMergeIdResult = () => {
+  return {


WHAT!!???? Now I have to do result.eventMergeId?? 🤣

Aaronius · 2020-05-06T22:12:30Z

test/unit/specs/core/executeCommandFactory.spec.js

-    const setDebugCommand = jasmine
-      .createSpy()
-      .and.returnValue(Promise.resolve("logResult"));
+    const setDebugCommand = jasmine.createSpy();


Not returning a promise or any result reflects the current reality of the setDebugCommand.

jfkhoury · 2020-05-06T22:17:56Z

test/functional/helpers/consoleLogger/index.js

-  const level = logLevel;
-  let cursor = 0;
-
-  const getNewMessages = async () => {


I think Martin's E2E test is using this getNewMessages helper and that's why it's failing in CI.

jfkhoury · 2020-05-06T22:32:15Z

test/functional/specs/C2594.js

-  await t.expect(errorMessage).contains("The user declined consent.");
+  const result = await sendEventPromise;
+  await t.expect(result).eql({});
+  await logger.warn.expectMessageMatching(/user declined consent/);


jfkhoury · 2020-05-06T22:33:51Z

test/functional/specs/C35448.js

-  const visitorEcid = await getVisitorEcid(orgMainConfigMain.orgId);
-  await t.expect(getAlloyEcid()).eql(visitorEcid);
+  // Don't await the visitor ECID before executing the getIdentity command.
+  // This helps ensure that Alloy is actually waiting for Visitor.


Aaronius · 2020-05-06T23:24:47Z

test/functional/specs/C14412.js

-
-  await t.expect(errorMessage).ok("Expected the event command to be rejected");
-  await t.expect(errorMessage).contains("user declined consent");
-});


I deleted this test because it was a duplicate of what we're doing in C14414.js. I'll delete the corresponding test case in test rail assuming this PR gets merged like this.

jfkhoury · 2020-05-06T23:40:34Z

test/functional/helpers/consoleLogger/index.js

-    const newMessages = consoleMessages.slice(cursor);
-    cursor = consoleMessages.length;
-    return newMessages;
+import { t } from "testcafe";


Love this revamped version!!!

jfkhoury · 2020-05-06T23:40:58Z

test/functional/specs/C2580.js

  await injectScript(alloyLibrary);
-  const newMessages = await logger.getNewMessages();
-  await t.expect(newMessages).match(/Executing getLibraryInfo command/);
+  await logger.log.expectMessageMatching(/Executing getLibraryInfo command/);


jfkhoury

👍

* Install @playwright/test as an explicit dependency * Inline fixture.jsx to avoid file:// CORS issues * Use raw element objects instead of React.createElement to avoid serialization issues * Replace npm-run-all with concurrently * Use a dev server to serve the dist/ dir during tests

CORE-45102 Removed errorsEnabled. No longer failing commands due to c…

11ab0ac

…onsent.

Aaronius commented May 6, 2020

View reviewed changes

CORE-45102 Fix spy name.

8c2738d

jfkhoury reviewed May 6, 2020

View reviewed changes

Aaronius commented May 6, 2020

View reviewed changes

jfkhoury reviewed May 6, 2020

View reviewed changes

CORE-45102 Fix documentation.

265d517

Aaronius commented May 6, 2020

View reviewed changes

Aaronius added 2 commits May 6, 2020 17:31

Merge branch 'master' into remove-errors-enabled

7cf79a6

CORE-45102 Updating C2580 to use revamped console logger.

91e9bb4

jfkhoury reviewed May 6, 2020

View reviewed changes

jonsnyder approved these changes May 7, 2020

View reviewed changes

jfkhoury approved these changes May 7, 2020

View reviewed changes

Aaronius merged commit 69e3baf into master May 7, 2020

Aaronius deleted the remove-errors-enabled branch May 7, 2020 18:50

Conversation

Aaronius commented May 6, 2020

Description

Related Issue

Motivation and Context

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfkhoury left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants