Source domain property directly from the request, not the context #35720
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gherceg
reviewed
Jan 31, 2025
Comment on lines
8
to
9
| {% if request.domain %} | ||
| {% initial_analytics_data 'gtm.domain' request.domain %} |
There was a problem hiding this comment.
What about this? Not sure if prioritizing domain over request.domain is best, but seems to preserve existing behavior the best.
Suggested change
| {% if request.domain %} | |
| {% initial_analytics_data 'gtm.domain' request.domain %} | |
| {% if domain or request.domain %} | |
| {% initial_analytics_data 'gtm.domain' domain or request.domain %} |
There was a problem hiding this comment.
That's fair. I don't think there's any circumstance where we have domain data and would want to exclude it, so this is probably the best short-term fix, but is probably long-term confusing
There was a problem hiding this comment.
Glad I tested this -- domain or request.domain doesn't appear to be working. My guess is django's template syntax doesn't evaluate or the same way python does. Seeing if there's a filter we can use
There was a problem hiding this comment.
Sorry about that! Thanks for testing.
mjriley
force-pushed
the
mjr/google-analytics-domain
branch
from
e91fe3b to
cf77a3c
Compare
gherceg
approved these changes
Jan 31, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Technical Summary
I was noticing that app preview events were setting the domain to
nonedespite a domain being available. This was because the app preview's context does not include thedomain.Feature Flag
Safety Assurance
Safety story
In my local/staging testing, this fixed the issue with app preview while preserving existing behavior for other events. The code itself should be safe, but my main fear is that we might have some code that does not populate the request's
domaindata and instead relies upon the context. I don't think this is likely, and Ajeet agreed that this change should be safe. In the worst case, we'll start receiving some events where domain is incorrectly listed asnone, which is where the app preview currently is.Automated test coverage
No tests
QA Plan
No QA
Rollback instructions
Labels & Review