[AMSDK-11077] Read XDM shared state from IdentityEdge#165
[AMSDK-11077] Read XDM shared state from IdentityEdge#165emdobrin merged 20 commits intoadobe:feature/consentfrom
Conversation
| import Foundation | ||
|
|
||
| /// Encodable extension used by `EdgeEventHandle` and `EdgeEventError` | ||
| extension Encodable { |
There was a problem hiding this comment.
This is a duplicate of what is in AEPServices. Sometimes it causes the compiler to get confused.
|
@nporter-adbe we will also need to declare the IdentityEdge dependency in the podspec as well |
|
@emdobrin Adding identity edge to the podspec dependencies causes CI failure, we may need to add it later: https://app.circleci.com/pipelines/github/adobe/aepsdk-edge-ios/440/workflows/6609f1f2-7e57-4458-a2ac-765e36ca636d/jobs/469/parallel-runs/0/steps/0-106 |
Codecov Report
@@ Coverage Diff @@
## feature/consent #165 +/- ##
===================================================
- Coverage 90.95% 90.18% -0.77%
===================================================
Files 23 21 -2
Lines 807 774 -33
===================================================
- Hits 734 698 -36
- Misses 73 76 +3 |
| /// The Experience Cloud ID to be sent with this request | ||
| var experienceCloudId: String? | ||
| /// XDM payloads to be attached to the request | ||
| var xdmPayloads: [[String: AnyCodable]] = [] |
There was a problem hiding this comment.
can we rename this to something that indicates that this is the top level xdm payload of the request, maybe requestXdmPayload
There was a problem hiding this comment.
I made the change as suggested locally, but I actually find the original name of xdmPayloads to be more concise and self-explanatory.
Example:
requestBuilder.xdmPayloads = [AnyCodable.from(dictionary: identityState) ?? [:]]
vs
requestBuilder.requestXdmPayload = [AnyCodable.from(dictionary: identityState) ?? [:]]
- In many cases
requestin the variable name for the XDM payloads is redundant given it's attached to the request builder. - The original name indicates its plural, indicating it could contain multiple XDM payloads.
| // This is not expected to happen. Continue without ECID | ||
| Log.warning(label: LOG_TAG, "processHit - An unexpected error has occurred, ECID is nil.") | ||
| } | ||
| requestBuilder.xdmPayloads = [AnyCodable.from(dictionary: identityState) ?? [:]] |
There was a problem hiding this comment.
We might need to extract the identityMap here as well. If Identity XDM Shared States gets updated with new fields in the future and we shouldn't include those in the Edge requests, this will break the network requests for old implementations of IdentityEdge.
There was a problem hiding this comment.
If this is the case, then this breaks the contract that we have laid out in previous conversations of the Edge extension being able to blindly attach the IdentityMap.
If that is required we can change it later, we shouldn't be pulling out the IdentityMap then grooming it before the network request is sent out. I thought the point was that the map would be in the correct format and we would not need to make any modifications to it before sending it.
There was a problem hiding this comment.
I am referring to the case where com.adobe.identityedge XDM shared state becomes:
{ "identityMap": {
"ECID" : [
{
"id" : "example"
}
]
},
"newIdentityMixin" : {
"new": "structure"
}
}
In this case, only identityMap should be attached on network requests to Experience Edge. newIdentityMixin may be used in other types of requests or other location in the payload.
There was a problem hiding this comment.
I find it a bit odd that we must parse the Identity shared state in the Edge extension, but I've updated the PR to only include the IdentityMap.
ecd2dcb
IdentityMap