WT-1598 Add sample rate to Conditional Display - #1791
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1791 +/- ##
==========================================
+ Coverage 90.08% 90.11% +0.03%
==========================================
Files 182 182
Lines 13219 13264 +45
==========================================
+ Hits 11908 11953 +45
Misses 1311 1311 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
The current sample-rate discovery yields duplicate per-block entries for repeated nested sample_rate occurrences, which can produce misleading/duplicated validation error messages.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds an experiment “sample rate” capability to the CMS Conditional Display system so content can be shown to a random percentage of eligible visitors, with a single shared roll per page and a GA4-compatible experiment_view event emitted for in-sample visitors.
Changes:
- Adds
sample_ratetoConditionalDisplayBlock, renders a newcondition-sample-ratewrapper, and enforces per-page consistency viaPage.clean()/experiment_sample_rate. - Adds a new
flare-sample-rateJS bundle that applies.in-experiment-sampleto<html>(when approved to run) and pushes anexperiment_viewevent towindow.dataLayer. - Adds/updates unit and Django tests plus pattern-library variants, and wires bundle + data attributes into
cms/base-flare.html.
File summaries
| File | Description |
|---|---|
| tests/unit/spec/cms/flare-sample-rate.js | Adds Jasmine coverage for sample-rate parsing, sampling behavior, and dataLayer event emission. |
| springfield/cms/tests/test_pages.py | Verifies <html> data attributes and conditional inclusion of the flare-sample-rate bundle. |
| springfield/cms/tests/test_conditional_display.py | Extends conditional-display wrapper assertions and adds tests for nested sample-rate discovery + clean() validation. |
| springfield/cms/templates/pattern-library/components/flare/conditional-display/conditional-display.yaml | Adds pattern-library sample-rate variants documenting in-sample vs out-of-sample behavior. |
| springfield/cms/templates/pattern-library/components/flare/conditional-display/conditional-display.html | Passes sample_rate into the conditional-display include for pattern-library rendering. |
| springfield/cms/templates/components/conditional-display.html | Implements the condition-sample-rate wrapper and preview indicator. |
| springfield/cms/templates/cms/base-flare.html | Adds <html> experiment data attributes and conditionally loads the flare-sample-rate bundle in <head>. |
| springfield/cms/models/base.py | Adds sample-rate discovery across StreamFields, experiment_sample_rate, and clean()-time enforcement of a single rate per page. |
| springfield/cms/fixtures/conditional_display_fixtures.py | Extends fixtures to generate sample_rate conditions and sample variants. |
| springfield/cms/blocks.py | Adds sample_rate field to ConditionalDisplayBlock (Decimal 0.01–100). |
| media/static-bundles.json | Registers the new flare-sample-rate bundle. |
| media/js/cms/flare-sample-rate.es6.js | Implements sample-rate evaluation, gating via isApprovedToRun(), and GA4-ish event push. |
| media/js/cms/flare-sample-rate-init.es6.js | Initializes the sample-rate logic on page load. |
| media/css/cms/flare-utilities.css | Reveals sample-rate conditional blocks when .in-experiment-sample is present on <html>. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
146c4ea to
ebcd833
Compare
|
Blocking: Sample rate on a Pencil Banner Snippet hides the banner permanently PencilBannerSnippet.settings is a StreamField of ConditionalDisplayBlock (snippets.py:552), so Sample rate (%) appears in the snippet editor. But iter_sample_rated_blocks() only walks StreamFields declared on the page, and a pencil banner arrives via PencilBannerPlacement — a separate model. Its rate is never found. |
| from decimal import Decimal | ||
|
|
||
| from django.conf import settings | ||
| from django.core.exceptions import ValidationError |
There was a problem hiding this comment.
| from django.core.exceptions import ValidationError | |
| from django.core.exceptions import NON_FIELD_ERRORS, ValidationError |
There was a problem hiding this comment.
related to Pencil Banner Snippet comment above
| def iter_sample_rated_blocks(self): | ||
| """Yield (field_name, block_index, block_type, sample_rate) for every top-level | ||
| block on this page whose Conditional Display settings, at any nesting depth, | ||
| set a sample rate. A top-level block yields one entry per distinct rate it | ||
| contains, even when that rate is repeated across several nested Conditional | ||
| Display blocks (e.g. multiple cards in a cards list) — so a page's block-level | ||
| validation error doesn't list the same block more than once. | ||
| """ | ||
| for field in self._meta.get_fields(): | ||
| if not isinstance(field, StreamField): | ||
| continue | ||
| for block_index, raw_block in enumerate(getattr(self, field.name).raw_data): | ||
| sample_rates = set(_sample_rates_in_raw_block_data(raw_block.get("value"))) | ||
| for sample_rate in sample_rates: | ||
| yield field.name, block_index, raw_block.get("type"), sample_rate |
There was a problem hiding this comment.
| def iter_sample_rated_blocks(self): | |
| """Yield (field_name, block_index, block_type, sample_rate) for every top-level | |
| block on this page whose Conditional Display settings, at any nesting depth, | |
| set a sample rate. A top-level block yields one entry per distinct rate it | |
| contains, even when that rate is repeated across several nested Conditional | |
| Display blocks (e.g. multiple cards in a cards list) — so a page's block-level | |
| validation error doesn't list the same block more than once. | |
| """ | |
| for field in self._meta.get_fields(): | |
| if not isinstance(field, StreamField): | |
| continue | |
| for block_index, raw_block in enumerate(getattr(self, field.name).raw_data): | |
| sample_rates = set(_sample_rates_in_raw_block_data(raw_block.get("value"))) | |
| for sample_rate in sample_rates: | |
| yield field.name, block_index, raw_block.get("type"), sample_rate | |
| def iter_sample_rated_blocks(self): | |
| """Yield (field_name, index, source_type, sample_rate) for everything on this | |
| page whose Conditional Display settings, at any nesting depth, set a sample | |
| rate. A source yields one entry per distinct rate it contains, even when that | |
| rate is repeated across several nested Conditional Display blocks (e.g. | |
| multiple cards in a cards list) — so a page's validation error doesn't list | |
| the same source more than once. | |
| field_name is the page StreamField a block came from, or None for a pencil | |
| banner, whose rate lives on a snippet rather than a field of this page. | |
| """ | |
| for field in self._meta.get_fields(): | |
| if not isinstance(field, StreamField): | |
| continue | |
| for block_index, raw_block in enumerate(getattr(self, field.name).raw_data): | |
| sample_rates = set(_sample_rates_in_raw_block_data(raw_block.get("value"))) | |
| for sample_rate in sample_rates: | |
| yield field.name, block_index, raw_block.get("type"), sample_rate | |
| for index, snippet in enumerate(self.sample_rated_pencil_banners()): | |
| sample_rates = set() | |
| for raw_block in snippet.settings.raw_data: | |
| sample_rates.update(_sample_rates_in_raw_block_data(raw_block.get("value"))) | |
| for sample_rate in sorted(sample_rates): | |
| yield None, index, "pencil banner", sample_rate | |
| def sample_rated_pencil_banners(self): | |
| """The pencil banner snippets placed on this page, which carry their own | |
| Conditional Display settings and so can set a sample rate of their own. | |
| Pages that take no pencil banners have none. | |
| """ | |
| return getattr(self, "pencil_banners", []) |
There was a problem hiding this comment.
related to Pencil Banner Snippet comment above
| def clean(self): | ||
| super().clean() | ||
|
|
||
| field_names_by_rate = defaultdict(set) | ||
| block_labels_by_rate = defaultdict(list) | ||
| for field_name, block_index, block_type, sample_rate in self.iter_sample_rated_blocks(): | ||
| field_names_by_rate[sample_rate].add(field_name) | ||
| block_labels_by_rate[sample_rate].append(f"{block_index + 1} ({block_type})") | ||
|
|
||
| if len(block_labels_by_rate) <= 1: | ||
| return | ||
|
|
||
| rate_descriptions = [f"{rate}%: block {', '.join(labels)}" for rate, labels in sorted(block_labels_by_rate.items())] | ||
| message = f"Blocks set different sample rates: {'; '.join(rate_descriptions)}. Every block on a page must use the same sample rate." | ||
| affected_field_names = set().union(*field_names_by_rate.values()) | ||
| raise ValidationError(dict.fromkeys(affected_field_names, message)) |
There was a problem hiding this comment.
| def clean(self): | |
| super().clean() | |
| field_names_by_rate = defaultdict(set) | |
| block_labels_by_rate = defaultdict(list) | |
| for field_name, block_index, block_type, sample_rate in self.iter_sample_rated_blocks(): | |
| field_names_by_rate[sample_rate].add(field_name) | |
| block_labels_by_rate[sample_rate].append(f"{block_index + 1} ({block_type})") | |
| if len(block_labels_by_rate) <= 1: | |
| return | |
| rate_descriptions = [f"{rate}%: block {', '.join(labels)}" for rate, labels in sorted(block_labels_by_rate.items())] | |
| message = f"Blocks set different sample rates: {'; '.join(rate_descriptions)}. Every block on a page must use the same sample rate." | |
| affected_field_names = set().union(*field_names_by_rate.values()) | |
| raise ValidationError(dict.fromkeys(affected_field_names, message)) | |
| def clean(self): | |
| super().clean() | |
| field_names_by_rate = defaultdict(set) | |
| labels_by_rate = defaultdict(list) | |
| for field_name, index, source_type, sample_rate in self.iter_sample_rated_blocks(): | |
| field_names_by_rate[sample_rate].add(field_name) | |
| label = f"block {index + 1} ({source_type})" if field_name else f"{source_type} {index + 1}" | |
| labels_by_rate[sample_rate].append(label) | |
| if len(labels_by_rate) <= 1: | |
| return | |
| rate_descriptions = [f"{rate}%: {', '.join(labels)}" for rate, labels in sorted(labels_by_rate.items())] | |
| message = f"Blocks set different sample rates: {'; '.join(rate_descriptions)}. Every block on a page must use the same sample rate." | |
| # A pencil banner's rate lives on a snippet, and the page form has no field to | |
| # hang the error on, so those surface as a non-field error instead. | |
| affected_field_names = set().union(*field_names_by_rate.values()) | |
| error_targets = {field_name or NON_FIELD_ERRORS for field_name in affected_field_names} | |
| raise ValidationError(dict.fromkeys(error_targets, message)) |
There was a problem hiding this comment.
related to Pencil Banner Snippet comment above
|
Not blocking, but pencil banners currently are not enabled for the /thanks/ page. Can tuck into this PR, or will need to follow up with another. |
Adds a "sample rate" field to the Conditional Display block. Showing
the block only if the visitor is randomly determined to be in the
specified sample.
- Adds sample_rate field on ConditionalDisplayBlock (DecimalBlock,
0.01-100%).
- Adds flare-sample-rate JS bundle
- rolls one random number per page if isApprovedToRun()
- logs a GA4 experiment_view event matching visitors
- Multiple components can be displayed based on sample-rate
but the sample rate must match.
- Page.clean() now rejects a save if two sample-rated blocks on the
same page disagree on the rate.
iter_sample_rated_blocks() yielded one entry per nested sample_rate occurrence, so a single top-level block containing several nested Conditional Display blocks (e.g. multiple cards in one cards list) could list itself more than once in clean()'s mismatch error message. Deduplicate rates per top-level block before yielding, so each block contributes at most one entry per distinct rate it contains.
…play Setting a sample rate on a Pencil Banner Snippet's Show To block made the banner permanently invisible: Page.experiment_sample_rate only scans a page's own StreamFields, and a banner is reached through a placement relation instead, so its rate is never seen. The page never gets a data-experiment-sample-rate attribute or the reveal JS bundle, so the banner's condition-sample-rate wrapper never leaves display:none. ConditionalDisplayBlock is now a factory function taking include_sample_rate (default True, unchanged for every existing page block usage). PencilBannerSnippet.settings uses ConditionalDisplayBlock(include_sample_rate=False), so the field is no longer offered where it can't work.
50190cb to
c80e259
Compare
One-line summary
Adds a "sample rate" field to the Conditional Display block. Showing the block only if the visitor is randomly determined to be in the specified sample.
Significant changes and points to review
Issue / Bugzilla link
https://mozilla-hub.atlassian.net/browse/WT-1598
Testing
Make sure you have GCP/DNT disabled
window.dataLayershows an experiment?automation=true, or GCP enabled to check block is disabled when not approved to run. (then disable GCP again)