Fixed posts list newsletter analytics hover logic #24599
Conversation
ref https://linear.app/ghost/issue/PROD-2386/post-newsletter-analytics-hover-doesnt-respect-one-flag-disabled - updated show/hide logic to respect settings for clicks/opens - added fallback to sent when both clicks+opens disabled W/r to the issue, this fixes the logic in the Posts List component. I'll tackle the Top Posts updates separately. The logic is as follows, for posts with email data: - Show clicks column if click tracking enabled - Show open column is open tracking enabled - Show sent column if open tracking and click tracking are disabled Tooltip behavior should respect the settings as well: - Sent always shown - Clicks shown if click tracking enabled - Opens shown if open tracking enabled The idea here is that Sent data should always be shown, and the other two should only be shown if the corresponding setting is enabled. There's some slight differences here with number of columns vs. Top Posts which is why I'm separating it out, as that component has less horizontal space to work with.
WalkthroughThe changes update the template logic for displaying newsletter analytics in the posts list, ensuring all email-related metrics and links are conditionally rendered based on the presence of email data and individual analytics feature flags for opens and clicks. The Mirage serializer for posts is modified to always include embedded email data in the serialized response. Additionally, new acceptance tests are introduced to verify the correct display of email analytics sections and columns under various conditions related to email tracking and data presence. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.38.6)ghost/admin/tests/acceptance/content-test.jsNote ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ghost/admin/app/components/posts-list/list-item-analytics.hbs(3 hunks)ghost/admin/mirage/serializers/post.js(1 hunks)ghost/admin/tests/acceptance/content-test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: kevinansfield
PR: TryGhost/Ghost#23770
File: apps/admin-x-settings/src/components/settings/email/newsletters/NewsletterDetailModalLabs.tsx:435-450
Timestamp: 2025-06-10T11:07:10.800Z
Learning: In Ghost's newsletter customization features, when promoting a feature from alpha to beta status, the feature flag guards are updated to make the feature available under both the `emailCustomization` (beta) and `emailCustomizationAlpha` (alpha) flags. This is done by either removing conditional guards entirely when the component is already behind both flags, or by updating conditionals to check for either flag.
Learnt from: kevinansfield
PR: TryGhost/Ghost#23824
File: ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs:919-926
Timestamp: 2025-06-13T11:57:58.226Z
Learning: In `ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs`, some style blocks (e.g., `.latest-post p` and `.latest-post p a`) still use the legacy colour `#73818c` on purpose; they are later overridden by `emailCustomization` feature rules, as noted by inline TODO comments. These occurrences should not be flagged as inconsistencies.
📚 Learning: the pending activity utilities in the ghost activitypub module are covered by tests in the file `app...
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Applied to files:
ghost/admin/tests/acceptance/content-test.js
📚 Learning: the pending activity utilities in ghost's activitypub module are thoroughly tested in `apps/admin-x-...
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`, which covers `generatePendingActivityId`, `isPendingActivity`, and `generatePendingActivity` functions.
Applied to files:
ghost/admin/tests/acceptance/content-test.js
📚 Learning: in the ghost test framework, when testing csv exports via the admin api, the response `body` field i...
Learnt from: ErisDS
PR: TryGhost/Ghost#23588
File: ghost/core/test/e2e-api/admin/backup.test.js:136-148
Timestamp: 2025-05-29T10:37:26.369Z
Learning: In the Ghost test framework, when testing CSV exports via the admin API, the response `body` field is empty while the actual CSV data is provided through the `text` field. Tests should use `expectEmptyBody()` and then validate the CSV content via `.expect(({text}) => ...)` - this is not contradictory behavior.
Applied to files:
ghost/admin/tests/acceptance/content-test.js
📚 Learning: in ghost's newsletter customization features, when promoting a feature from alpha to beta status, th...
Learnt from: kevinansfield
PR: TryGhost/Ghost#23770
File: apps/admin-x-settings/src/components/settings/email/newsletters/NewsletterDetailModalLabs.tsx:435-450
Timestamp: 2025-06-10T11:07:10.800Z
Learning: In Ghost's newsletter customization features, when promoting a feature from alpha to beta status, the feature flag guards are updated to make the feature available under both the `emailCustomization` (beta) and `emailCustomizationAlpha` (alpha) flags. This is done by either removing conditional guards entirely when the component is already behind both flags, or by updating conditionals to check for either flag.
Applied to files:
ghost/admin/tests/acceptance/content-test.jsghost/admin/app/components/posts-list/list-item-analytics.hbs
📚 Learning: in `ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs`, some style b...
Learnt from: kevinansfield
PR: TryGhost/Ghost#23824
File: ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs:919-926
Timestamp: 2025-06-13T11:57:58.226Z
Learning: In `ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs`, some style blocks (e.g., `.latest-post p` and `.latest-post p a`) still use the legacy colour `#73818c` on purpose; they are later overridden by `emailCustomization` feature rules, as noted by inline TODO comments. These occurrences should not be flagged as inconsistencies.
Applied to files:
ghost/admin/tests/acceptance/content-test.jsghost/admin/app/components/posts-list/list-item-analytics.hbs
📚 Learning: in ghost project, app.js files under core/server/web are intentionally excluded from unit test cover...
Learnt from: ErisDS
PR: TryGhost/Ghost#23582
File: ghost/core/.c8rc.json:24-24
Timestamp: 2025-05-29T07:45:35.714Z
Learning: In Ghost project, app.js files under core/server/web are intentionally excluded from unit test coverage because they are not easily unit-testable due to being entry points with initialization code and side effects.
Applied to files:
ghost/admin/tests/acceptance/content-test.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup
🔇 Additional comments (11)
ghost/admin/mirage/serializers/post.js (1)
15-15: Good addition to support email analytics conditionals.Adding 'email' to the default includes ensures that email data is consistently available in test responses, which supports the template logic that checks for
@post.emailpresence.ghost/admin/tests/acceptance/content-test.js (2)
1006-1024: Well-structured test suite for newsletter analytics.The test suite is properly organized with clear documentation explaining the conditions that affect the computed properties. The setup correctly creates an administrator user and authenticates the session.
1026-1059: Comprehensive test for email analytics section visibility.The test properly verifies that:
- Posts with email data show the email analytics section
- Posts without email data don't show the section
The test setup creates appropriate test data and uses proper selectors to verify the UI behavior.ghost/admin/app/components/posts-list/list-item-analytics.hbs (8)
203-203: Good addition of email data conditional wrapper.Adding the
{{#if @post.email}}check ensures the entire newsletter performance metrics block only renders when email data is available, which aligns with the PR objectives.
208-215: Clean implementation of always-visible sent data.The sent metric is now always displayed within the email analytics block, removing the previous conditional logic. The comment clearly explains the intent.
216-225: Proper conditional display for opens analytics.The opens metric is now conditionally displayed based on
@post.showEmailOpenAnalytics, which provides cleaner separation of concerns.
226-235: Consistent conditional display for clicks analytics.The clicks metric follows the same pattern as opens, using
@post.showEmailClickAnalyticsfor conditional display.
238-248: Clean opens column implementation.The opens column link is now independently controlled by
@post.showEmailOpenAnalytics, removing the previous nested conditional logic.
250-260: Consistent clicks column implementation.The clicks column follows the same clean pattern as the opens column.
262-272: Smart fallback for sent column display.The addition of a sent column that appears when both click and open analytics are disabled ensures that users always see some newsletter metric. This addresses the core requirement from the PR objectives.
274-274: Proper closing of email conditional block.The closing
{{/if}}correctly ends the email data conditional wrapper.
| it('displays newsletter columns based on email tracking settings', async function () { | ||
| // Test 1: When both tracking options are disabled, show sent column | ||
| let email1 = this.server.create('email', { | ||
| emailCount: 15000, | ||
| trackOpens: false, | ||
| trackClicks: false | ||
| }); | ||
|
|
||
| // Create post that would show sent column | ||
| this.server.create('post', { | ||
| status: 'published', | ||
| hasBeenEmailed: true, | ||
| email: email1, | ||
| // Override computed properties for testing | ||
| showEmailOpenAnalytics: false, | ||
| showEmailClickAnalytics: false | ||
| }); | ||
|
|
||
| await visit('/posts'); | ||
|
|
||
| // Verify sent column appears with proper formatting | ||
| expect(find('[data-test-analytics-sent]'), 'sent column').to.exist; | ||
| expect(find('[data-test-analytics-sent] .gh-content-email-stats-value').textContent.trim()).to.equal('15,000'); | ||
| expect(find('[data-test-analytics-opens]'), 'opens column when disabled').to.not.exist; | ||
| expect(find('[data-test-analytics-clicks]'), 'clicks column when disabled').to.not.exist; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Thorough test for tracking settings behavior.
The test correctly validates that when both tracking options are disabled, only the sent column appears with proper formatting. However, there's a discrepancy in the test setup.
The test creates email data with trackOpens: false, trackClicks: false but then overrides computed properties directly on the post model:
- // Override computed properties for testing
- showEmailOpenAnalytics: false,
- showEmailClickAnalytics: falseThis approach bypasses the actual computed property logic. Consider either:
- Setting up the proper conditions (settings, user role, etc.) to make the computed properties return false naturally
- Adding a comment explaining why direct property override is necessary for this test scenario
The direct override might be acceptable if the computed properties depend on complex conditions that are difficult to set up in tests, but it should be documented.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In ghost/admin/tests/acceptance/content-test.js around lines 1061 to 1087, the
test overrides computed properties on the post model directly, bypassing the
actual logic that determines these properties. To fix this, either adjust the
test setup to naturally produce false values for showEmailOpenAnalytics and
showEmailClickAnalytics by configuring the relevant settings or user roles, or
add a clear comment explaining why the direct override is necessary due to
complexity in setting up those conditions in the test environment.
|
@cmraible FYI I worked for a bit on adding more complete tests. Getting Mirage to appropriately mock the settings turns out to be quite difficult (at least it has been stumping me), so I added the minimum cases that only depend on the post model data and moved on. |
ref https://linear.app/ghost/issue/PROD-2386/post-newsletter-analytics-hover-doesnt-respect-one-flag-disabled ref #24599 - updated display logic in Top Posts Building on the other PR (see ref), this updates the Newsletter analytics column in the Top Posts component. Logic is very similar to the Posts List column, except there's one fewer column in Top Posts, as there's less horizontal space here. Column display: - Hidden if no post email data - Show open rate if open rate tracking enabled - Show click rate if open rate tracking disabled *and* click rate tracking enabled - Show sent count if open rate tracking and click rate tracking are disabled Tooltip: - Always display Sent count - Display click count if click tracking enabled - Display open rate if open rate enabled
|
|
||
| // Verify sent column appears with proper formatting | ||
| expect(find('[data-test-analytics-sent]'), 'sent column').to.exist; | ||
| expect(find('[data-test-analytics-sent] .gh-content-email-stats-value').textContent.trim()).to.equal('15,000'); |
There was a problem hiding this comment.
I think this should be 15k per @peterzimon but I'm not 100% sure that's what he meant by "make sure it's formatted with thousand indicators".
There was a problem hiding this comment.
You're right, we should abbreviate it, e.g. 15k — then users can see the exact number in the tooltip and in the post analytics page.
ref https://linear.app/ghost/issue/PROD-2386/post-newsletter-analytics-hover-doesnt-respect-one-flag-disabled ref #24599 - updated display logic in Top Posts Building on the other PR (see ref), this updates the Newsletter analytics column in the Top Posts component. Logic is very similar to the Posts List column, except there's one fewer column in Top Posts, as there's less horizontal space here. Column display: - Hidden if no post email data - Show open rate if open rate tracking enabled - Show click rate if open rate tracking disabled *and* click rate tracking enabled - Show sent count if open rate tracking and click rate tracking are disabled Tooltip: - Always display Sent count - Display click count if click tracking enabled - Display open rate if open rate enabled
ref https://linear.app/ghost/issue/PROD-2386/post-newsletter-analytics-hover-doesnt-respect-one-flag-disabled
W/r to the issue, this fixes the logic in the Posts List component. I'll tackle the Top Posts updates separately.
The logic is as follows, for posts with email data:
Tooltip behavior should respect the settings as well:
The idea here is that Sent data should always be shown, and the other two should only be shown if the corresponding setting is enabled. There's some slight differences here with number of columns vs. Top Posts which is why I'm separating it out, as that component has less horizontal space to work with.