Skip to content

Conversation

@9larsons
Copy link
Contributor

@9larsons 9larsons commented Jan 8, 2026

ref d451e8c
ref https://linear.app/ghost/issue/NY-878/

I am reverting this change as I discovered the db data was not quite as expected, which wasn't fully caught in tests because we don't have a true e2e test for these pieces (which I was already working on). Even so, I don't think an e2e test would've captured this issue, which is the linkTo for the link in member details, as the text itself is correct. We may need better db/model coverage. See issue for more discussion and details for the full fix, with change in behavior.


Note

Reverts the previous email-only post attribution change.

  • Simplifies getResourceById to handle post and page identically via models.Post.findOne({id}, {require:false}) without status filtering
  • Removes special-case support/tests for sent (email-only) posts; updates unit tests and mocks accordingly in url-translator.test.js

Written by Cursor Bugbot for commit 5e68164. This will update automatically on new commits. Configure here.

@9larsons 9larsons enabled auto-merge (squash) January 8, 2026 23:11
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 8, 2026

Walkthrough

The changes refactor the getResourceById method in the member attribution URL translator service. The dedicated post retrieval logic is consolidated with page handling to share a unified retrieval path. This removes status-based differentiation for posts, simplifying the code path to return the post if found or null otherwise. Explicit null-checks are added for author and tag retrievals to ensure null safety. Corresponding test mocks are updated to reflect the removed status-based behavior, and a test case asserting email-only posts with 'sent' status is removed.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title clearly and accurately describes the main change: reverting a previous bugfix for email-only post attribution.
Description check ✅ Passed The description is directly related to the changeset, explaining the revert rationale, referencing specific issues and commits, and describing the code behavior changes accurately.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cbd3ef8 and 5e68164.

📒 Files selected for processing (2)
  • ghost/core/core/server/services/member-attribution/url-translator.js
  • ghost/core/test/unit/server/services/member-attribution/url-translator.test.js
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-08T10:26:38.700Z
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 25791
File: ghost/core/core/server/api/endpoints/member-comment-ban.js:64-68
Timestamp: 2026-01-08T10:26:38.700Z
Learning: In the Ghost API, endpoints rely on the serialization layer to prepare frame.data[docName] as a non-empty array before query() executes. Endpoints access frame.data[docName][0] directly (e.g., frame.data.comment_bans[0], frame.data.members[0], frame.data.posts[0]) without per-endpoint validation. This pattern is common across API endpoints. When maintaining or creating endpoints, avoid duplicating validation for frame.data[docName] and ensure the serializer guarantees the shape and non-emptiness. If you add a new endpoint that uses this frame.data[docName], follow the same assumption and avoid redundant checks unless there's a documented exception.

Applied to files:

  • ghost/core/core/server/services/member-attribution/url-translator.js
  • ghost/core/test/unit/server/services/member-attribution/url-translator.test.js
📚 Learning: 2025-03-13T09:00:20.205Z
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 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/core/test/unit/server/services/member-attribution/url-translator.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). (9)
  • GitHub Check: Ghost-CLI tests
  • GitHub Check: Legacy tests (Node 22.18.0, mysql8)
  • GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
  • GitHub Check: Lint
  • GitHub Check: Unit tests (Node 22.18.0)
  • GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
  • GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
  • GitHub Check: Cursor Bugbot
  • GitHub Check: Build & Push Docker Image
🔇 Additional comments (2)
ghost/core/test/unit/server/services/member-attribution/url-translator.test.js (1)

6-12: Mock simplification aligns with the unified retrieval logic.

The simplified Post.findOne mock appropriately reflects the removal of status-based differentiation. The mock now consistently returns a post object regardless of status, which matches the unified post/page retrieval behavior in the implementation.

Note: The PR objectives mention the lack of e2e test coverage for these attribution flows. While you've noted this is being addressed separately, consider adding integration tests that verify the complete attribution flow, including link generation in member details.

ghost/core/core/server/services/member-attribution/url-translator.js (1)

133-141: Unifying post and page retrieval is correct and safe.

The changes properly revert to treating posts and pages identically via a single database query, removing previous status-based differentiation. Verification confirms no code in the member attribution service depends on status filtering—both callers (getResourceDetails and fetchResource) work correctly with the unified approach.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@9larsons 9larsons merged commit 8cb8899 into main Jan 8, 2026
39 checks passed
@9larsons 9larsons deleted the revert-attribution-change branch January 8, 2026 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants