[DIA-2757] UI test for the applies condition #714
Conversation
carmelo-iriti
changed the title
cmplibrary/src/debug/java/com/sourcepoint/cmplibrary/creation/ComponentFactory.kt
Show resolved
Hide resolved
cmplibrary/src/main/java/com/sourcepoint/cmplibrary/campaign/CampaignManager.kt
Show resolved
Hide resolved
cmplibrary/src/main/java/com/sourcepoint/cmplibrary/campaign/CampaignManager.kt
Show resolved
Hide resolved
| @SerialName("GPPData") @Serializable(with = JsonMapSerializer::class) val gppData: Map<String, JsonElement>? = null, | ||
| @SerialName("uuid") var uuid: String?, | ||
| @SerialName("webConsentPayload") val webConsentPayload: JsonObject? = null, | ||
| ) |
There was a problem hiding this comment.
from my pov, the thing that you refactored here looked more readable and it followed KCS, but it might be the matter of taste here, so I'm ok with that
There was a problem hiding this comment.
I'd go ahead and remove the annotations all together, keeping only the ones that are strictly necessary (those whose attribute name differ from the api response).
@bohdan-go-wombat thanks for sharing the KCS link, it seems that annotations without arguments are allowed to be in the same line as the example:
@Test fun foo() { /*...*/ }|
|
||
| internal fun getConnectionManager(appCtx: Context): ConnectionManager = ConnectionManager.create(appCtx) | ||
|
|
||
| internal fun networkClient(appCtx: Context, netClient: OkHttpClient, responseManage: ResponseManager, logger: Logger): NetworkClient { |
There was a problem hiding this comment.
why do we need and instance of Context here, since we don't use it?
There was a problem hiding this comment.
because using the variants technique, we must have the signature of the method the same in all files (the contents can be different)
| @@ -31,6 +31,7 @@ internal fun CcpaCS.toCCPAConsentInternal(): CCPAConsentInternal { | |||
| thisContent = JSONObject(), | |||
| signedLspa = signedLspa, | |||
| webConsentPayload = webConsentPayload, | |||
| uspstring = this.uspstring ?: "1YNN" | |||
There was a problem hiding this comment.
can we use constant here, instead of hardcoded value?
| @@ -131,26 +131,25 @@ internal fun MessagesParamReq.toConsentStatusParamReq( | |||
| ) | |||
| } | |||
|
|
|||
| internal fun CCPA.toCcpaCS() = CcpaCS( | |||
| internal fun CCPA.toCcpaCS(applies: Boolean?) = CcpaCS( | |||
There was a problem hiding this comment.
now I'm thinking, wouldn't it be better for us not to update applies value anywhere, except from the response of the /meta-data call? that will ease-out this flow, so that we don't need to pass an extra value into the ext function, and we will not worry about the value of applies having wrong value
wdyt?
There was a problem hiding this comment.
I set applies as a parameter in order to make it explicit, in this function we are converting one type into another and this parameter force us to check what we are sending and also to use the applies value from the single source of truth which is the meta-data call.
In this way we are sure to avoid the inconsistency of the data that Chris was writing in more that one ticket
cmplibrary/src/release/java/com/sourcepoint/cmplibrary/creation/ComponentFactory.kt
Show resolved
Hide resolved
| @SerialName("GPPData") @Serializable(with = JsonMapSerializer::class) val gppData: Map<String, JsonElement>? = null, | ||
| @SerialName("uuid") var uuid: String?, | ||
| @SerialName("webConsentPayload") val webConsentPayload: JsonObject? = null, | ||
| ) |
There was a problem hiding this comment.
I'd go ahead and remove the annotations all together, keeping only the ones that are strictly necessary (those whose attribute name differ from the api response).
@bohdan-go-wombat thanks for sharing the KCS link, it seems that annotations without arguments are allowed to be in the same line as the example:
@Test fun foo() { /*...*/ }| "$specificationVersion$opportunityToOptOut$optOutSale$lspaCoveredTransaction" | ||
| } else DEFAULT_CCPA_USP_STRING | ||
| val usPString = "$specificationVersion$opportunityToOptOut$optOutSale$lspaCoveredTransaction" | ||
| logger?.computation( |
There was a problem hiding this comment.
@carmelo-iriti these loggers are really getting on the way of readability. Can we get rid of it?
There was a problem hiding this comment.
Unfortunately no, because I use them to check if the conditions are matched and in production they are not logging anything (using the veriants technique)
|
@carmelo-iriti I believe this PR should have been broken down. The title says UI tests for applies condition, but I see changes for the usp string, logic changes, tests and code styling changes all in the same group. |
|
@andresilveirah this is the original code styling which was changed in some pr |
|
@andresilveirah the changes for the uspstring have to stay in order to make the tests working |
Agree. Many things were changed in the previous PRs. And they were changed for a reason. I suggest we keep things the way they are unless there's a strong argument against it. |
|
@andresilveirah originally I chose the standard code style for kotlin where the annotations are in line with the variable different than java where every time you have a new line |
|
@carmelo-iriti understood, thanks for the info. Do you mind reverting back the styling changes just so we can focus on the actual changes in this PR? If necessary, we can address code styling changes in a separate PR only for that. |
|
The one issue I saw with the applies value is when you click on the Save & Close button in the privacy manager the onConsentReady and onSpFinish. This issue was submitted on https://sourcepoint.atlassian.net/browse/DIA-2901 |
|
The consent.ccpa?.consents?.uspstring and the consent.ccpa?.consents?.applies were aligned in my tests |
|
@cmaurersp Thanks, I skipped the UI test for the gdpr PM 👍🏻 |
No description provided.