Skip to content
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

Slight tweaks to the domain analytics value #35758

Merged
merged 3 commits into from
Feb 12, 2025

Conversation

mjriley
Copy link
Contributor

@mjriley mjriley commented Feb 11, 2025

Technical Summary

This is a slight tweak on the previous solution created in #35748. I was bothered by the duplication, but couldn't find any better way to handle it. After looking through some documentation on custom tags, however, I realized as is a cleaner version that eliminates the duplication.

Feature Flag

Safety Assurance

Safety story

Verified locally that the domain value was being populated correctly on both the raw_doc page (which needs domain) and the app builder panel (which needs request.domain).

Automated test coverage

No tests

QA Plan

No QA

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@mjriley mjriley added the product/invisible Change has no end-user visible impact label Feb 11, 2025
@mjriley mjriley requested a review from biyeun as a code owner February 11, 2025 21:05
Copy link
Contributor

@jingcheng16 jingcheng16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I learned firstof tag too.

@mjriley mjriley requested a review from orangejenny as a code owner February 12, 2025 18:41
@mjriley mjriley force-pushed the mjr/analytics-domain-cleanup branch from 3874dec to 6437f81 Compare February 12, 2025 19:01
@jingcheng16
Copy link
Contributor

@mjriley Do you know why the test failed on this PR not on the two previous PRs that touches metrics?

@mjriley
Copy link
Contributor Author

mjriley commented Feb 12, 2025

@mjriley Do you know why the test failed on this PR not on the two previous PRs that touches metrics?

Yep. The change here introduced a new context variable, metrics_domain (because it felt risky to override domain with request.domain). We had a few tests that verified the entire context for some app builder pages, and so they were failing when detecting the new variable

@mjriley mjriley merged commit fea74fe into master Feb 12, 2025
14 checks passed
@mjriley mjriley deleted the mjr/analytics-domain-cleanup branch February 12, 2025 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/invisible Change has no end-user visible impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants