-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve error messages when missing identity cookies #838
Conversation
Failing functional tests will be resolved when pull request #835 is merged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see some functional tests. Let's talk about this in standup.
orgIdsFromCookies.length > 0 && | ||
!includes(orgIdsFromCookies, orgId) | ||
) { | ||
errorMessage = `An identity for organzation ${orgId} was not found. Valid organizations on this page are: ${orgIdsFromCookies.join( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Valid organizations on this page" may not be true. Another page could have set a cookie. Maybe, "Valid organizations on this domain". But that may be wrong too as there could be a third party cookie that you are reading. Ideally we would only look at the cookies returned via cookieTransfer when in 3rd party collection mode.
src/utils/domainMatchesApex.js
Outdated
export default (domain, apexDomain) => { | ||
const regex = new RegExp(`${apexDomain}$`); | ||
return regex.test(domain); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is equivalent to the existing util "endsWith"
import injectExtractOrgIdsFromCookies from "../../../../../src/components/Identity/injectExtractOrgIdsFromCookies"; | ||
import { cookieJar } from "../../../../../src/utils"; | ||
|
||
describe("injectExtractOrIdsFromCookies", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo "OrIds" => "OrgIds"
"97D1F3F459CE0AD80A495CBE" | ||
]); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test for when there are no cookies?
", " | ||
)}`; | ||
} else if (!domainMatchesApex(edgeDomain, apexDomain)) { | ||
errorMessage = `An identity was not set properly because edge domain ${edgeDomain} does not match apex domain ${apexDomain}, and ${apexDomain} may not allow third-party cookies.`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to show this message when the customer is using third party data collection like adobedc.net, but I don't want to recreate the logic that Konductor uses to determine when to send cookies via cookie transfer and when to send them via set-cookie headers.
First of all, good work on this fix @carterworks , anything Identity & cookies can be painful. Question: It seems that both of these exceptions can/are being handled by Edge:
Should we test to see if Konductor already returns an error for mismatched Org ID & Datastream ID, and how it handles 2, and simply log their error? I'm asking because we are working towards a future where Alloy won't even care about the Identity cookie, so why not try to simplify this logic now. Thoughts? |
From what I can tell, Konductor has an error message for missing/incorrect datastream IDs, but not for organization IDs. Also, I cannot figure out a way to block cookies from being set. I've tried iframe stuff (can't figure out how to run TestCafé in both a browser page and an iframe inside that page at the same time), content security policy stuff (can't block cookies with CSP–you can only block third-party cookies with |
Closed in favor of the more simple PR #846, which adds a functional test and tweaks the language of the error message slightly. |
Description
The goal of this ticket was to improve the error messages when a identity cookie is not set after a Konductor response. We identified two potential causes for this
("example.com" is used as an example of an edge domain and "adobe.com" is used as an example of an apex domain).
In addition to these two new error messages, the catch-all error message has been updated. It now reads as "An identity was not set properly. Please verify that cookies returned from demdex.net can be set on this page."
Related Issue
PDCL-7279–Better error message when Web SDK can't find identity cookie
Motivation and Context
Previous customers have found that the existing error message was not actionable. These new error message provide more useful next steps for the customer.
Types of changes
Checklist: