Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a members API GET endpoint Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
sagzy
force-pushed
the
gift-subscriptions/wire-up-redemption-screen
branch
from
c683148 to
27b5a83
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6831488fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ghost/core/core/server/services/gifts/gift-service.ts (1)
67-83: The dependency injection pattern already uses lazy evaluation via object property getters.Lines 23–30 in
gift-service-wrapper.jspassmemberRepository,staffServiceEmails, andtiersServiceAPIas getter properties, which are evaluated on-demand when accessed by GiftService, not when the service is instantiated. The asyncinit()method ensures these services are only required when initialization runs, deferring the boot-time work.However,
labsServiceon line 32 is passed directly rather than as a getter, inconsistently breaking the lazy pattern. Consider wrapping it as a getter too for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/gifts/gift-service.ts` around lines 67 - 83, The constructor currently assigns labsService directly to this.#labsService breaking the lazy-getter pattern used for other deps; change the constructor to accept labsService the same way (wrap as a getter) and assign a getter (e.g., this.#getLabsService = () => dependencies.labsService) and then update internal uses to call the getter (use this.#getLabsService().isSet(...)) so labsService is evaluated on-demand like `#getMemberRepository`, `#getStaffServiceEmails` and `#getTiersServiceAPI`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/portal/test/api.test.js`:
- Around line 54-69: HumanReadableError.fromApiResponse currently only extracts
user-facing messages for 400, 429, and 500, causing 404 responses like
{errors:[{message:'Gift not found.'}]} to be lost; update
HumanReadableError.fromApiResponse to also handle status 404 by parsing JSON
(check Content-Type) and returning the API-provided errors[0].message as the
human-readable message (falling back to the existing generic message if errors
or message are missing) so calls like ghostApi.gift.fetchRedemptionData({token})
receive the preserved "Gift not found." text.
In `@ghost/core/core/server/services/gifts/gift-service.ts`:
- Around line 163-167: The preflight expiry check currently only inspects
gift.expiredAt, so tokens with gift.expiresAt in the past but not yet swept are
treated as redeemable; update the check in gift-service.ts to also consider
gift.expiresAt by throwing when gift.expiredAt !== null OR (gift.expiresAt !==
null && gift.expiresAt <= now), using the same error/message
(tpl(messages.giftExpired)); also add a regression test asserting that a gift
with expiresAt set to a past timestamp and expiredAt === null is rejected by the
same preflight/redeem check.
---
Nitpick comments:
In `@ghost/core/core/server/services/gifts/gift-service.ts`:
- Around line 67-83: The constructor currently assigns labsService directly to
this.#labsService breaking the lazy-getter pattern used for other deps; change
the constructor to accept labsService the same way (wrap as a getter) and assign
a getter (e.g., this.#getLabsService = () => dependencies.labsService) and then
update internal uses to call the getter (use this.#getLabsService().isSet(...))
so labsService is evaluated on-demand like `#getMemberRepository`,
`#getStaffServiceEmails` and `#getTiersServiceAPI`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de4eb784-c509-42e2-b9c1-70b5e85a7e2e
📒 Files selected for processing (14)
apps/portal/src/app.jsapps/portal/src/components/pages/gift-redemption-page.jsapps/portal/src/utils/api.jsapps/portal/test/api.test.jsapps/portal/test/portal-links.test.jsghost/core/core/server/api/endpoints/gifts-members.jsghost/core/core/server/api/endpoints/index.jsghost/core/core/server/services/gifts/gift-bookshelf-repository.tsghost/core/core/server/services/gifts/gift-service-wrapper.jsghost/core/core/server/services/gifts/gift-service.tsghost/core/core/server/web/members/app.jsghost/core/test/e2e-api/members/gifts.test.jsghost/core/test/unit/server/services/gifts/gift-bookshelf-repository.test.tsghost/core/test/unit/server/services/gifts/gift-service.test.ts
sagzy
force-pushed
the
gift-subscriptions/wire-up-redemption-screen
branch
from
27b5a83 to
cbe4044
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/portal/src/utils/errors.js (2)
22-38: Consider extracting shared JSON error parsing logic.The JSON parsing and error extraction logic in lines 29-37 is nearly identical to lines 12-20 (400/429 handling). A helper function could reduce duplication and ensure consistent behavior across status codes.
♻️ Optional: Extract shared logic
+ async _extractJsonErrorMessage(res) { + try { + const json = await res.json(); + if (json.errors && Array.isArray(json.errors) && json.errors.length > 0 && json.errors[0].message) { + return new HumanReadableError(json.errors[0].message); + } + } catch (e) { + // Failed to decode: ignore + } + return undefined; + } + static async fromApiResponse(res) { // Bad request + Too many requests if (res.status === 400 || res.status === 429) { - try { - const json = await res.json(); - if (json.errors && Array.isArray(json.errors) && json.errors.length > 0 && json.errors[0].message) { - return new HumanReadableError(json.errors[0].message); - } - } catch (e) { - // Failed to decode: ignore - return undefined; - } + return this._extractJsonErrorMessage(res); } if (res.status === 404) { const contentType = (res.headers.get('content-type') || '').toLowerCase(); if (!contentType.includes('application/json')) { return undefined; } - try { - const json = await res.json(); - if (json.errors && Array.isArray(json.errors) && json.errors.length > 0 && json.errors[0].message) { - return new HumanReadableError(json.errors[0].message); - } - } catch (e) { - // Failed to decode: ignore - return undefined; - } + return this._extractJsonErrorMessage(res); }Note: The content-type check you added for 404 is a good defensive measure. You might consider applying it to the 400/429 branch as well for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/portal/src/utils/errors.js` around lines 22 - 38, Extract the duplicated JSON error parsing into a single helper (e.g., extractHumanReadableErrorFromResponse or parseJsonError) that: checks content-type contains "application/json", attempts res.json() in a try/catch, and returns a HumanReadableError when json.errors[0].message exists or undefined otherwise; then replace the inline parsing in the 400/429 branch and the 404 branch to call that helper so both branches share identical behavior and the content-type check is applied consistently.
64-86: Consider adding gift-related error messages for i18n.The new gift redemption flow returns API error messages like "Gift not found.", "This gift has already been redeemed.", "This gift has expired.", "You already have an active subscription.", and "Gift subscriptions are not enabled on this site." These messages will not be translated unless added to
specialMessages.If internationalization is required for the gift feature, consider adding:
t('Gift not found.'); t('This gift has already been redeemed.'); t('This gift has expired.'); t('You already have an active subscription.'); t('Gift subscriptions are not enabled on this site.');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/portal/src/utils/errors.js` around lines 64 - 86, The i18n specialMessages list in setupSpecialMessages currently omits gift-related error strings, so add the new gift messages by invoking the local t function (the closure that pushes into specialMessages) with each gift error: "Gift not found.", "This gift has already been redeemed.", "This gift has expired.", "You already have an active subscription.", and "Gift subscriptions are not enabled on this site." so they are picked up by the i18n-parser and included in specialMessages.ghost/core/test/unit/server/services/gifts/gift-service.test.ts (1)
322-338: Consider using relative dates for time-dependent expiry tests.This test uses a hard-coded past date (
2026-01-15) that works now but creates maintenance burden. Using relative dates makes tests self-documenting and future-proof.♻️ Suggested approach
it('throws BadRequestError when the gift expiry timestamp is in the past but not yet swept', async function () { + const pastDate = new Date(Date.now() - 24 * 60 * 60 * 1000); // 1 day ago giftRepository.getByToken.resolves(buildGift({ - expiresAt: new Date('2026-01-15T00:00:00.000Z'), + expiresAt: pastDate, expiredAt: null }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/gifts/gift-service.test.ts` around lines 322 - 338, The test uses a hard-coded date (new Date('2026-01-15T00:00:00.000Z')) which will become brittle; update the spec in the test that calls giftRepository.getByToken and buildGift to use relative dates instead (e.g., compute expiresAt as a Date based on Date.now() +/- offset so it's consistently in the past or future for the scenario), keep expiredAt as null, and keep the same assertions around service.getRedeemableGiftByToken; update the instantiation via createService() and the mocked getByToken call (symbols: giftRepository.getByToken, buildGift, createService, getRedeemableGiftByToken) so the test uses a dynamically computed expiry timestamp rather than a fixed calendar date.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/portal/src/utils/errors.js`:
- Around line 22-38: Extract the duplicated JSON error parsing into a single
helper (e.g., extractHumanReadableErrorFromResponse or parseJsonError) that:
checks content-type contains "application/json", attempts res.json() in a
try/catch, and returns a HumanReadableError when json.errors[0].message exists
or undefined otherwise; then replace the inline parsing in the 400/429 branch
and the 404 branch to call that helper so both branches share identical behavior
and the content-type check is applied consistently.
- Around line 64-86: The i18n specialMessages list in setupSpecialMessages
currently omits gift-related error strings, so add the new gift messages by
invoking the local t function (the closure that pushes into specialMessages)
with each gift error: "Gift not found.", "This gift has already been redeemed.",
"This gift has expired.", "You already have an active subscription.", and "Gift
subscriptions are not enabled on this site." so they are picked up by the
i18n-parser and included in specialMessages.
In `@ghost/core/test/unit/server/services/gifts/gift-service.test.ts`:
- Around line 322-338: The test uses a hard-coded date (new
Date('2026-01-15T00:00:00.000Z')) which will become brittle; update the spec in
the test that calls giftRepository.getByToken and buildGift to use relative
dates instead (e.g., compute expiresAt as a Date based on Date.now() +/- offset
so it's consistently in the past or future for the scenario), keep expiredAt as
null, and keep the same assertions around service.getRedeemableGiftByToken;
update the instantiation via createService() and the mocked getByToken call
(symbols: giftRepository.getByToken, buildGift, createService,
getRedeemableGiftByToken) so the test uses a dynamically computed expiry
timestamp rather than a fixed calendar date.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 75c7c50e-081b-4248-a44a-63a6d6a79319
📒 Files selected for processing (17)
apps/portal/package.jsonapps/portal/src/app.jsapps/portal/src/components/pages/gift-redemption-page.jsapps/portal/src/utils/api.jsapps/portal/src/utils/errors.jsapps/portal/test/api.test.jsapps/portal/test/errors.test.jsapps/portal/test/portal-links.test.jsghost/core/core/server/api/endpoints/gifts-members.jsghost/core/core/server/api/endpoints/index.jsghost/core/core/server/services/gifts/gift-bookshelf-repository.tsghost/core/core/server/services/gifts/gift-service-wrapper.jsghost/core/core/server/services/gifts/gift-service.tsghost/core/core/server/web/members/app.jsghost/core/test/e2e-api/members/gifts.test.jsghost/core/test/unit/server/services/gifts/gift-bookshelf-repository.test.tsghost/core/test/unit/server/services/gifts/gift-service.test.ts
✅ Files skipped from review due to trivial changes (5)
- apps/portal/test/errors.test.js
- apps/portal/package.json
- apps/portal/src/app.js
- apps/portal/test/api.test.js
- ghost/core/core/server/api/endpoints/index.js
🚧 Files skipped from review as they are similar to previous changes (9)
- ghost/core/core/server/web/members/app.js
- ghost/core/core/server/services/gifts/gift-service-wrapper.js
- ghost/core/core/server/api/endpoints/gifts-members.js
- ghost/core/test/unit/server/services/gifts/gift-bookshelf-repository.test.ts
- apps/portal/src/components/pages/gift-redemption-page.js
- apps/portal/src/utils/api.js
- apps/portal/test/portal-links.test.js
- ghost/core/core/server/services/gifts/gift-bookshelf-repository.ts
- ghost/core/test/e2e-api/members/gifts.test.js
sagzy
force-pushed
the
gift-subscriptions/wire-up-redemption-screen
branch
from
cbe4044 to
7640067
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/server/services/gifts/gift-service.ts (1)
26-33: Consider simplifying thetoJSONreturn type.The
toJSON()method's return type duplicates theRedeemableTierstructure. This could be simplified to reduce maintenance burden.♻️ Suggested simplification
type RedeemableTierRecord = RedeemableTier & { - toJSON?: () => { - id: string; - name: string; - description: string | null; - benefits: string[] | null; - }; + toJSON?: () => RedeemableTier; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/gifts/gift-service.ts` around lines 26 - 33, The explicit toJSON return type in RedeemableTierRecord duplicates RedeemableTier; change the declaration of RedeemableTierRecord (and its toJSON) to reuse the existing RedeemableTier type instead of repeating fields—e.g., make toJSON return RedeemableTier or a narrowed type like Pick<RedeemableTier, 'id'|'name'|'description'|'benefits'> (or Partial/ Omit as appropriate) so the shape is maintained from the original RedeemableTier definition and reduces duplicated maintenance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/core/server/services/gifts/gift-service.ts`:
- Around line 26-33: The explicit toJSON return type in RedeemableTierRecord
duplicates RedeemableTier; change the declaration of RedeemableTierRecord (and
its toJSON) to reuse the existing RedeemableTier type instead of repeating
fields—e.g., make toJSON return RedeemableTier or a narrowed type like
Pick<RedeemableTier, 'id'|'name'|'description'|'benefits'> (or Partial/ Omit as
appropriate) so the shape is maintained from the original RedeemableTier
definition and reduces duplicated maintenance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f82ff5d4-c7cc-4d4c-992e-2ad315ea1ff7
📒 Files selected for processing (17)
apps/portal/package.jsonapps/portal/src/app.jsapps/portal/src/components/pages/gift-redemption-page.jsapps/portal/src/utils/api.jsapps/portal/src/utils/errors.jsapps/portal/test/api.test.jsapps/portal/test/errors.test.jsapps/portal/test/portal-links.test.jsghost/core/core/server/api/endpoints/gifts-members.jsghost/core/core/server/api/endpoints/index.jsghost/core/core/server/services/gifts/gift-bookshelf-repository.tsghost/core/core/server/services/gifts/gift-service-wrapper.jsghost/core/core/server/services/gifts/gift-service.tsghost/core/core/server/web/members/app.jsghost/core/test/e2e-api/members/gifts.test.jsghost/core/test/unit/server/services/gifts/gift-bookshelf-repository.test.tsghost/core/test/unit/server/services/gifts/gift-service.test.ts
✅ Files skipped from review due to trivial changes (2)
- apps/portal/package.json
- apps/portal/src/components/pages/gift-redemption-page.js
🚧 Files skipped from review as they are similar to previous changes (10)
- ghost/core/core/server/api/endpoints/index.js
- apps/portal/test/portal-links.test.js
- ghost/core/core/server/api/endpoints/gifts-members.js
- ghost/core/core/server/web/members/app.js
- apps/portal/src/app.js
- apps/portal/src/utils/api.js
- ghost/core/core/server/services/gifts/gift-bookshelf-repository.ts
- ghost/core/test/unit/server/services/gifts/gift-bookshelf-repository.test.ts
- apps/portal/test/api.test.js
- ghost/core/test/e2e-api/members/gifts.test.js
sagzy
force-pushed
the
gift-subscriptions/wire-up-redemption-screen
branch
from
7640067 to
69e12da
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ghost/core/core/server/services/gifts/gift-service.ts (1)
97-98: Minor: Missing semicolon for consistency.Line 97 is missing a semicolon after
StaffServiceEmails, unlike other properties in the type definition.🔧 Suggested fix
giftEmailService: GiftEmailService; - staffServiceEmails: StaffServiceEmails + staffServiceEmails: StaffServiceEmails; labsService: LabsService;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/gifts/gift-service.ts` around lines 97 - 98, The type/property declaration for staffServiceEmails in gift-service.ts is missing a trailing semicolon for consistency; update the declaration of staffServiceEmails: StaffServiceEmails to include a semicolon so it matches other properties like labsService: LabsService; and maintains consistent punctuation in the type/interface definition.ghost/core/test/e2e-api/members/gifts.test.js (1)
183-195: Consider adding edge case:expires_atin past withexpired_atstill null.The expired gift test sets
expired_at, but doesn't cover the scenario whereexpires_atis in the past butexpired_atis stillnull(gift not yet swept by background job). This edge case is handled in the service code (line 201 ofgift-service.ts), but adding an e2e test here would provide additional regression protection.💡 Suggested additional test case
it('returns 400 when the gift expires_at is in the past but not yet swept', async function () { const pastDate = new Date('2020-01-01T00:00:00.000Z'); const gift = await createGift({ expires_at: pastDate, expired_at: null // Not yet swept by background job }); const {body} = await membersAgent .get(`/api/gifts/${gift.get('token')}/redeem/`) .expectStatus(400); assert.equal(body.errors[0].message, expiredMessage); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/e2e-api/members/gifts.test.js` around lines 183 - 195, Add an E2E test in gifts.test.js that covers the edge case where expires_at is in the past but expired_at is still null: use createGift({expires_at: pastDate, expired_at: null}), call membersAgent.get(`/api/gifts/${gift.get('token')}/redeem/`) and expectStatus(400), then assert body.errors[0].message === expiredMessage; place this alongside the existing "returns 400 when the gift has expired" test so gift-service logic that checks expires_at (not just expired_at) is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/core/core/server/services/gifts/gift-service.ts`:
- Around line 221-235: The local Tier type used in gift-service.ts is missing
the id property, causing a type mismatch with the Tier instances returned by
tiersService.api.read(); update the local type definition named Tier (the type
declared around the top of gift-service.ts) to include id: string; so
tier.toJSON() / tier.id accesses match the type, then re-run typechecks and
adjust any nullable/optional annotations if needed.
---
Nitpick comments:
In `@ghost/core/core/server/services/gifts/gift-service.ts`:
- Around line 97-98: The type/property declaration for staffServiceEmails in
gift-service.ts is missing a trailing semicolon for consistency; update the
declaration of staffServiceEmails: StaffServiceEmails to include a semicolon so
it matches other properties like labsService: LabsService; and maintains
consistent punctuation in the type/interface definition.
In `@ghost/core/test/e2e-api/members/gifts.test.js`:
- Around line 183-195: Add an E2E test in gifts.test.js that covers the edge
case where expires_at is in the past but expired_at is still null: use
createGift({expires_at: pastDate, expired_at: null}), call
membersAgent.get(`/api/gifts/${gift.get('token')}/redeem/`) and
expectStatus(400), then assert body.errors[0].message === expiredMessage; place
this alongside the existing "returns 400 when the gift has expired" test so
gift-service logic that checks expires_at (not just expired_at) is exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dd65612b-f15c-40f4-92de-16f8789ec1fa
📒 Files selected for processing (18)
apps/portal/package.jsonapps/portal/src/app.jsapps/portal/src/components/pages/gift-redemption-page.jsapps/portal/src/utils/api.jsapps/portal/src/utils/errors.jsapps/portal/test/api.test.jsapps/portal/test/errors.test.jsapps/portal/test/portal-links.test.jsghost/core/core/boot.jsghost/core/core/server/api/endpoints/gifts-members.jsghost/core/core/server/api/endpoints/index.jsghost/core/core/server/services/gifts/gift-bookshelf-repository.tsghost/core/core/server/services/gifts/gift-service-wrapper.jsghost/core/core/server/services/gifts/gift-service.tsghost/core/core/server/web/members/app.jsghost/core/test/e2e-api/members/gifts.test.jsghost/core/test/unit/server/services/gifts/gift-bookshelf-repository.test.tsghost/core/test/unit/server/services/gifts/gift-service.test.ts
✅ Files skipped from review due to trivial changes (5)
- apps/portal/package.json
- apps/portal/src/components/pages/gift-redemption-page.js
- ghost/core/test/unit/server/services/gifts/gift-bookshelf-repository.test.ts
- apps/portal/src/app.js
- apps/portal/test/api.test.js
🚧 Files skipped from review as they are similar to previous changes (8)
- ghost/core/core/server/api/endpoints/index.js
- ghost/core/core/server/api/endpoints/gifts-members.js
- ghost/core/core/server/services/gifts/gift-service-wrapper.js
- apps/portal/src/utils/errors.js
- apps/portal/test/portal-links.test.js
- ghost/core/core/server/services/gifts/gift-bookshelf-repository.ts
- apps/portal/test/errors.test.js
- ghost/core/test/unit/server/services/gifts/gift-service.test.ts
sagzy
force-pushed
the
gift-subscriptions/wire-up-redemption-screen
branch
from
69e12da to
bbf1acf
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
ghost/core/core/server/services/gifts/gift-service.ts (1)
203-207: Consider using a distinct error message when the tier is missing.When the tier lookup fails (line 203-207), the error message says "Gift not found." but the actual issue is that the gift's tier doesn't exist. This could be confusing for debugging. Consider using a more specific internal message while keeping the user-facing message generic, or logging the tier lookup failure for observability.
💡 Optional: Add logging for tier lookup failure
if (!tier) { + logging.warn(`Tier ${gift.tierId} not found for gift ${gift.token}`); throw new errors.NotFoundError({ message: tpl(messages.giftNotFound) }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/gifts/gift-service.ts` around lines 203 - 207, The current NotFoundError thrown when the tier lookup fails uses messages.giftNotFound which is misleading; update the check that throws (the if (!tier) block that currently throws errors.NotFoundError with tpl(messages.giftNotFound)) to use a distinct internal message (e.g. messages.giftTierNotFound or a new string) while keeping the user-facing message generic, or log the specific tier lookup failure before throwing the existing giftNotFound error; reference the same throw site, the tier variable, errors.NotFoundError, and tpl(messages.giftNotFound) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/core/core/server/api/endpoints/index.js`:
- Around line 292-294: The giftsMembers export is returned raw and should be
wrapped with the API pipeline like other member endpoints; update the getter for
giftsMembers to return apiFramework.pipeline(require('./gifts-members'), {role:
'members'}) (matching how commentsMembers/feedbackMembers are wrapped) so the
endpoint gets framework validation, serialization, and error handling; then
verify the callers in the members app (where api.feedbackMembers is passed to
http()) now receive a pipeline-wrapped handler for consistent behavior.
In `@ghost/core/test/unit/server/services/gifts/gift-service.test.ts`:
- Around line 49-56: Remove the unresolved merge markers and keep the updated
stub block that includes getByToken; replace the conflicted lines with a clean
initialization using create: sinon.stub(), existsByCheckoutSessionId:
sinon.stub().resolves(false), and getByToken: sinon.stub().resolves(null) so the
stubs for create, existsByCheckoutSessionId, and getByToken are present and the
conflict markers (<<<<<<<, =======, >>>>>>>) are removed.
- Around line 8-16: Remove the unresolved Git conflict markers and restore a
valid declaration for giftRepository by keeping the new shape that includes all
three stubs: create, existsByCheckoutSessionId, and getByToken (i.e., replace
the entire conflicted block containing <<<<<<< HEAD, =======, and >>>>>>> with a
single declaration for giftRepository that defines those three sinon.SinonStub
properties so tests compile).
- Around line 87-124: Resolve the merge conflict by keeping the newer
implementation: return a GiftService constructed with the labsService included
(keep the GiftService instantiation that passes labsService) and retain the
buildGift helper function that constructs a new Gift with defaults (refer to
GiftService, labsService, buildGift and the Gift constructor) so the test file
contains the updated constructor call and the buildGift helper instead of the
old conflicting block.
---
Nitpick comments:
In `@ghost/core/core/server/services/gifts/gift-service.ts`:
- Around line 203-207: The current NotFoundError thrown when the tier lookup
fails uses messages.giftNotFound which is misleading; update the check that
throws (the if (!tier) block that currently throws errors.NotFoundError with
tpl(messages.giftNotFound)) to use a distinct internal message (e.g.
messages.giftTierNotFound or a new string) while keeping the user-facing message
generic, or log the specific tier lookup failure before throwing the existing
giftNotFound error; reference the same throw site, the tier variable,
errors.NotFoundError, and tpl(messages.giftNotFound) when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 96ec2fe9-8f1f-44f7-955f-6078b0b8fc6a
📒 Files selected for processing (19)
apps/portal/package.jsonapps/portal/src/app.jsapps/portal/src/components/pages/gift-redemption-page.jsapps/portal/src/utils/api.jsapps/portal/src/utils/errors.jsapps/portal/test/api.test.jsapps/portal/test/errors.test.jsapps/portal/test/portal-links.test.jsghost/core/core/boot.jsghost/core/core/server/api/endpoints/gifts-members.jsghost/core/core/server/api/endpoints/index.jsghost/core/core/server/services/gifts/gift-bookshelf-repository.tsghost/core/core/server/services/gifts/gift-repository.tsghost/core/core/server/services/gifts/gift-service-wrapper.jsghost/core/core/server/services/gifts/gift-service.tsghost/core/core/server/web/members/app.jsghost/core/test/e2e-api/members/gifts.test.jsghost/core/test/unit/server/services/gifts/gift-bookshelf-repository.test.tsghost/core/test/unit/server/services/gifts/gift-service.test.ts
✅ Files skipped from review due to trivial changes (8)
- ghost/core/core/boot.js
- apps/portal/package.json
- ghost/core/core/server/services/gifts/gift-repository.ts
- apps/portal/src/app.js
- apps/portal/test/errors.test.js
- ghost/core/test/unit/server/services/gifts/gift-bookshelf-repository.test.ts
- apps/portal/src/components/pages/gift-redemption-page.js
- ghost/core/test/e2e-api/members/gifts.test.js
🚧 Files skipped from review as they are similar to previous changes (6)
- ghost/core/core/server/services/gifts/gift-service-wrapper.js
- apps/portal/test/portal-links.test.js
- ghost/core/core/server/api/endpoints/gifts-members.js
- ghost/core/core/server/web/members/app.js
- apps/portal/src/utils/api.js
- apps/portal/test/api.test.js
ghost/core/test/unit/server/services/gifts/gift-service.test.ts
Outdated
Show resolved
Hide resolved
ghost/core/test/unit/server/services/gifts/gift-service.test.ts
Outdated
Show resolved
Hide resolved
ghost/core/test/unit/server/services/gifts/gift-service.test.ts
Outdated
Show resolved
Hide resolved
sagzy
force-pushed
the
gift-subscriptions/wire-up-redemption-screen
branch
4 times, most recently
from
3c7be97 to
3ae8731
Compare
mike182uk
reviewed
Apr 8, 2026
| return true; | ||
| } | ||
|
|
||
| async getRedeemableGiftByToken({token, currentMember}: {token: string; currentMember?: {status: string} | null}) { |
Member
There was a problem hiding this comment.
We should also add a check for refunded_at
Member
There was a problem hiding this comment.
What do you think to moving entity level validation into the Gift entity?
Something like:
class Gift {
isReedemable(reedemDate, memberStatus) {
if (this.expiredAt !== null) {
return false;
}
if (reedemDate > this.expiresAt) {
return false;
}
if (memberStatus !== 'free') {
return false;
}
}
}
This keeps the business rules with the entity, but the service still needs to perform other checks (e.g gift existence, valid tier)
Member
There was a problem hiding this comment.
Could return status from the method also:
export const REDEEM_FAILURE_REASON_EXPIRED = 'expired'
export const REDEEM_FAILURE_REASON_REDEEMED = 'redeemed'
class Gift {
isReedemable(reedemDate, memberStatus) {
if (this.expiredAt !== null) {
return REDEEM_FAILURE_REASON_EXPIRED;
}
if (reedemDate > this.expiresAt) {
return REDEEM_FAILURE_REASON_REDEEMED;
}
}
}
switch (gift.isRedeemable(Date.now(), member.status)) {
case REDEEM_FAILURE_REASON_EXPIRED:
...
Contributor
Author
There was a problem hiding this comment.
Yep great input, thanks! I've kept the member check out of the gift entity, but have otherwise pushed the "is this gift redeemable" business logic to the gift entity in 3ef33cd
Curious what you think!
| assert.equal(body.errors[0].message, alreadyRedeemedMessage); | ||
| }); | ||
|
|
||
| it('returns 400 when the gift has expired', async function () { |
Member
There was a problem hiding this comment.
Should we add a test case that also covers the expiresAt path?
Contributor
Author
There was a problem hiding this comment.
I've actually removed the expires_at (with a s) path, and kept the expired_at (with a d) check only - we also rely on these to be not-null for the already redeemed / consumed / refunded checks, so think we can simplify and do the same for expired
| const {GiftEmailService} = require('./gift-email-service'); | ||
| const membersService = require('../members'); | ||
| const tiersService = require('../tiers/service'); | ||
| const tiersService = require('../tiers'); |
Member
There was a problem hiding this comment.
huh, did we not need to reference the tiers service like this?
Contributor
Author
There was a problem hiding this comment.
tiers/index.js has
module.exports = require('./service');
So const tiersService = require('../tiers'); or const tiersService = require('../tiers/service'); are equivalent
| stripe_payment_intent_id: string; | ||
| consumes_at: Date | null; | ||
| expires_at: Date | null; | ||
| status: 'purchased' | 'redeemed' | 'consumed' | 'expired' | 'refunded'; |
Member
There was a problem hiding this comment.
Hm, wonder if we should move this (and cadence) to Gift and export from there so we don't have these values in multiple places
export type GiftStatus = 'purchased' | ...
export type GiftCadence = 'month' | ...
sagzy
force-pushed
the
gift-subscriptions/wire-up-redemption-screen
branch
from
3ae8731 to
3ef33cd
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref https://linear.app/ghost/issue/BER-3476
Summary
This PR adds a gift redemption preflight endpoint for Portal:
GET /members/api/gifts/:token/redeem/The endpoint is read-only and answers one question before any redemption write happens: can this token be redeemed right now by the current visitor?
It supports both anonymous visitors and logged-in members. If the token is redeemable, it returns the gift details Portal needs to render the redemption screen. If not, it returns a standard members API error response with a user-facing message.
The endpoint validates gift redeemability in this order:
API contract
GET /members/api/gifts/:token/redeem/Success response:
Error response:
Expected error messages include:
"Gift not found.""This gift has already been redeemed.""This gift has expired.""You already have an active subscription.""Gift subscriptions are not enabled on this site."Portal Flow
#/portal/gift/redeem/:tokenGET /members/api/gifts/:token/redeem/