🐛 Fixed CTA for public preview card not showing on post previews #23182
Conversation
WalkthroughThe change removes an extraneous blank line after retrieving the post from the API and deletes the line that sets Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
cathysarisky
changed the title
|
@cathysarisky thanks for raising the issue, and for taking the time to work on a fix! I think you've found the correct root cause here — Admittedly, this really isn't well documented, but the first signal to me that something's not quite right is that this PR requires code from Ghost/ghost/core/core/bridge.js Lines 1 to 11 in 8b72ea4 Our pie-in-the-sky long term vision is that the frontend might one day be a completely separate app, which would be a client of the server/content API. If there were no other way to do this, I wouldn't block this PR on it, but it is an initial indication to me that something's not quite right. We also have an informal policy of adding tests for any bug fixes — I totally trust you that this fixes the bug, but without a test it's really easy for someone to come along in the future, remove this code and not realize they've broken anything. I'm context loaded on this problem, as we've had a customer raise a similar issue (basically the same thing, but in the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/core/core/frontend/services/routing/controllers/previews.js (1)
30-30: Remove debug console.log statement.This console.log statement appears to be a debugging artifact and should be removed before merging to production.
- console.log('post', post);🧰 Tools
🪛 ESLint
[error] 30-30: Unexpected console statement.
(no-console)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/core/core/frontend/services/routing/controllers/previews.js(2 hunks)
🧰 Additional context used
🪛 ESLint
ghost/core/core/frontend/services/routing/controllers/previews.js
[error] 30-30: Unexpected console statement.
(no-console)
[error] 64-64: 'member' is assigned a value but never used.
(no-unused-vars)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 22.13.1, mysql8)
- GitHub Check: Ghost-CLI tests
🔇 Additional comments (1)
ghost/core/core/frontend/services/routing/controllers/previews.js (1)
1-73: Add test coverage for preview access functionality.This change fixes an issue with the content CTA not displaying correctly in post previews, but no tests were added to verify the fix or prevent future regressions.
As mentioned in the PR comments, there's an informal policy to add tests for bug fixes. Consider adding tests that verify:
- The correct display of content CTA in post previews with different access scenarios
- Proper handling of public previews where post.html is present but access should be restricted
Can you also verify if this fix addresses the similar issue in the
/email/route that was mentioned in the PR comments?🧰 Tools
🪛 ESLint
[error] 30-30: Unexpected console statement.
(no-console)
[error] 64-64: 'member' is assigned a value but never used.
(no-unused-vars)
ghost/core/core/frontend/services/routing/controllers/previews.js
Outdated
Show resolved
Hide resolved
|
Just converting to a draft so Coderabbit calms down |
cmraible
changed the title
cmraible
force-pushed
the
fix-preview-access
branch
from
9ba6c16 to
3413440
Compare
closes #23181
closes https://linear.app/ghost/issue/ONC-921/oss-issue-content-cta-does-not-display-on-preview-when-it-should
refs https://linear.app/ghost/issue/ONC-930/support-escalation-re-freepaywalls-on-web-versions-of-email-only-posts
When a post is set to members only or paid only and there is a public preview card present, the "This post is for members only" CTA wasn't appearing in the post preview, even though the private content was correctly omitted. The CTA was correctly being displayed on the actual post on the frontend, however.
This pattern of setting
post.accessin the frontend controller route was previously used in all of theentry,previewandemailrouters. This commit from 2020 switches this pattern to calculate the value ofpost.accessin the content API rather than the frontend, and this samepost.access = !!post.htmlline was removed from theentrycontroller at that time. However, the change wasn't carried over to theemailorpreviewcontrollers.This commit carries the pattern over to the
previewcontroller, and adds a test to ensure the CTA is rendered in the preview route. I'll follow up with another PR to do the same for theemailcontroller.