[AMSDK-11130] Collect consent#166
Conversation
| @testable import AEPEdge | ||
| import XCTest | ||
|
|
||
| class EdgePublicAPITests: XCTestCase { |
| @testable import AEPServices | ||
| import Foundation | ||
|
|
||
| /// MockDataQueue - see also AEPServices/Mocks |
There was a problem hiding this comment.
MockDataQueue, MockHitProcessor and TestableExtensionRuntime are copied from core.
Codecov Report
@@ Coverage Diff @@
## feature/consent #166 +/- ##
===================================================
+ Coverage 90.18% 93.06% +2.88%
===================================================
Files 21 24 +3
Lines 774 793 +19
===================================================
+ Hits 698 738 +40
+ Misses 76 55 -21 |
PravinPK
left a comment
There was a problem hiding this comment.
Neat. The logic looks good to me
| let consentRequests = self.getNetworkRequestsWith(url: FunctionalTestConst.EX_EDGE_CONSENT_URL_STR, httpMethod: HttpMethod.post, timeout: 1) | ||
| XCTAssertEqual(0, consentRequests.count) | ||
| } | ||
|
|
There was a problem hiding this comment.
good to have a few more tests here.. with Consents obtained from defaultConfig only and no Consent API's are called
I guess we for those test we should wait for that Bootup Consent PR to be merged.
There was a problem hiding this comment.
There are some more tests for this scenario in EdgeExtensionTests -> testBootup with different Consent shared states (see testBootup_whenNoConsentSharedState_usesDefaultYes)
| beginProcessing() | ||
| case .no: | ||
| clear() | ||
| beginProcessing() |
There was a problem hiding this comment.
do we need beginProcessing() here ?
There was a problem hiding this comment.
Yes, this is required for us to keep queuing and processing consent update events while collect consent is no, while ignoring experience events.
| self = .yes | ||
| case "n": | ||
| self = .no | ||
| default: |
There was a problem hiding this comment.
The default case is a bit confusing for me, as we have two default values in our constants based on specific scenarios. I wonder if it's the correct thing to default to pending here.
What if we made it a failable initializer and returned nil instead of providing a default value as it can vary based on the situation?
init?(rawValue: Self.RawValue)
There was a problem hiding this comment.
Don't think it will help much because I am already unwrapping the string before calling CollectConsent(rawValue:) - see getCollectConsentOrDefault. Changed to return the Defaults.COLLECT_CONSENT_PENDING.
The only case where we use default yes is on bootup when Consent extension is not registered.
| guard !hasBooted else { return } | ||
|
|
||
| // check if consent extension is registered | ||
| if let consentSharedState = getXDMSharedState(EdgeConstants.SharedState.Consent.SHARED_OWNER_NAME, nil) { |
There was a problem hiding this comment.
I think we should be reading the EventHub shared state to determine if the consent extension is registered. I think this approach of checking if the shared state exists to determine if an extension is registered can lead to false negatives.
We've used this method in our legacy codebase before, but I think we've found it tricky in some situations. Just because shared state doesn't exist for an extension doesn't mean it doesn't exist.
There was a problem hiding this comment.
I looked at using either the shared state or MobileCore.getRegisteredExtensions API but found that it only includes the friendly name and version of each extension, whereas other extensions should rely on is the extension name (aka shared state name) as the friendly name can anytime change. I discussed a bit about this with @shalehaha and we are considering it, but are still evaluating the implications for Assurance UI before making any changes.
I am willing to change this logic to check for hub shared state/getRegisteredExtensions as soon as the extension name is included in there.
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: