Fix propositions return value #1082
Conversation
…at-ing the arrays together
…erience edge have changed.
| @@ -70,6 +70,9 @@ describe("mergeLifecycleResponses", () => { | |||
| ] | |||
| } | |||
| ] | |||
| }, | |||
| { | |||
| propositions: [] | |||
There was a problem hiding this comment.
A unit test that coves the "merge two arrays" code path would be good.
| ...lifecycleOnResponseReturnValues, | ||
| ...consumerOnResponseReturnValues, | ||
| ...lifecycleOnBeforeRequestReturnValues | ||
| ); | ||
| ].reduce((accumulator, currentValue) => { |
There was a problem hiding this comment.
This is the defect I was going to tell you about in our meeting the other day. much improved 🏆
There was a problem hiding this comment.
i would really like this accumulation of results to be available as the lifecycle progresses. I know we discussed it possibly being less performant, but might it be worth trying it out? Changing from promise.all to a sequential evaluation where the responses are accumulated. that still seems like an ideal solution for the decisioning engine propositions to be added.
| ...lifecycleOnResponseReturnValues, | ||
| ...consumerOnResponseReturnValues, | ||
| ...lifecycleOnBeforeRequestReturnValues | ||
| ); | ||
| ].reduce((accumulator, currentValue) => { |
There was a problem hiding this comment.
Would it help to extract this code into a separate util and unit test that alone?
… can be easily tested
| @@ -0,0 +1,26 @@ | |||
| import assign from "./assign"; | |||
| @@ -0,0 +1,65 @@ | |||
| import assignConcatArrayValues from "../../../../src/utils/assignConcatArrayValues"; | |||
There was a problem hiding this comment.
Looks great @jonsnyder ! I think @jasonwaters 's comment is worth discussing, but it can be after the release.
Description
While writing documentation wiki pages for the apply propositions, I noticed that the send event call was not returning anything in propositions. It was always an empty array. After investigation, I found that this was caused by the DecisioningEngine also returning propositions from its onBeforeEvent lifecycle method. This PR changes how the lifecycle return values are merged together. If the merged values are arrays, then the arrays are concat'ed together instead of overwritten.
Additionally, I found a few more tests failing because experience edge is now returning default-content-items when before it wasn't. I've updated the tests to be more lenient on their expectations regarding the number of propositions returned.
Lastly, there are two tests for applyResponse that I skipped because I noticed there are a bunch of new propositions returned from experience edge that are messing up the expectations. See https://jira.corp.adobe.com/browse/PDCL-11302
Related Issue
Motivation and Context
Screenshots (if appropriate):
Types of changes
Checklist: