Improved gift redemption error to show specific failure reason#27630
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:
WalkthroughServer gift redeemability checks now attach explicit Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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-error-message-BER-3569
branch
from
1fa6d22 to
7647fbf
Compare
![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: 1
🧹 Nitpick comments (1)
ghost/core/test/unit/server/services/members/middleware.test.js (1)
100-125: ⚡ Quick winExpand edge-case coverage for
errorCoderedirect behavior.Nice additions. Two small missing cases: non-string
err.codeshould be ignored, and stringerrorCodeshould still serialize correctly when redirecting to an on-site referrer/hash URL.✅ Suggested test additions
+ it('does not append errorCode when code is non-string', async function () { + req.url = '/members?token=test&action=subscribe'; + req.query = {token: 'test', action: 'subscribe'}; + + const err = new Error('boom'); + err.code = 123; + membersService.ssr.exchangeTokenForSession.rejects(err); + + await membersMiddleware.createSessionFromMagicLink(req, res, next); + + sinon.assert.notCalled(next); + sinon.assert.calledOnce(res.redirect); + assert.equal(res.redirect.firstCall.args[0], '/blah/?action=subscribe&success=false'); + }); + + it('appends errorCode when redirecting to an on-site referrer hash URL', async function () { + req.url = '/members?token=test&action=subscribe&r=https%3A%2F%2Fsite.com%2Fblah%2F'; + req.query = { + token: 'test', + action: 'subscribe', + r: 'https://site.com/blah/#/portal/account?giftRedemption=true' + }; + + const err = new Error('This gift has expired.'); + err.code = 'GIFT_EXPIRED'; + membersService.ssr.exchangeTokenForSession.rejects(err); + + await membersMiddleware.createSessionFromMagicLink(req, res, next); + + sinon.assert.notCalled(next); + sinon.assert.calledOnce(res.redirect); + assert.equal(res.redirect.firstCall.args[0], 'https://site.com/blah/?action=subscribe&errorCode=GIFT_EXPIRED&success=false#/portal/account?giftRedemption=true'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/members/middleware.test.js` around lines 100 - 125, Add tests for two edge cases in createSessionFromMagicLink: (1) when membersService.ssr.exchangeTokenForSession rejects with an Error whose code is non-string (e.g., a number or object) assert that res.redirect is called without an errorCode query param (reference: membersMiddleware.createSessionFromMagicLink, membersService.ssr.exchangeTokenForSession, err.code, res.redirect); (2) when the request comes from an on-site referrer or a URL containing a hash/fragment, simulate such a referrer in req (or req.get('Referrer')) and reject with an Error that has a string code, then assert the redirect preserves the referrer/fragment and includes errorCode serialized correctly in the resulting redirect URL (reference: membersMiddleware.createSessionFromMagicLink, res.redirect, errorCode query param).
🤖 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/src/actions.js`:
- Around line 279-296: The catch block that creates the error notification
currently tears down the portal flow (sets showPopup: false, lastPage: null,
popupNotification: null) which dismisses the popup for pre-redemption errors;
update the returned object in that branch so the popup remains open for
pre-redemption failures by setting showPopup: true and preserving existing
navigation state instead of clearing it (use values from state for lastPage,
pageQuery and popupNotification or simply don't overwrite them), leaving only
the notification/notificationSequence changed; keep createNotification,
getGiftRedemptionErrorMessage and removePortalLinkFromUrl usage as-is.
---
Nitpick comments:
In `@ghost/core/test/unit/server/services/members/middleware.test.js`:
- Around line 100-125: Add tests for two edge cases in
createSessionFromMagicLink: (1) when membersService.ssr.exchangeTokenForSession
rejects with an Error whose code is non-string (e.g., a number or object) assert
that res.redirect is called without an errorCode query param (reference:
membersMiddleware.createSessionFromMagicLink,
membersService.ssr.exchangeTokenForSession, err.code, res.redirect); (2) when
the request comes from an on-site referrer or a URL containing a hash/fragment,
simulate such a referrer in req (or req.get('Referrer')) and reject with an
Error that has a string code, then assert the redirect preserves the
referrer/fragment and includes errorCode serialized correctly in the resulting
redirect URL (reference: membersMiddleware.createSessionFromMagicLink,
res.redirect, errorCode query param).
🪄 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: 87173cf1-2d74-467d-b044-1f9a36e2642e
📒 Files selected for processing (78)
apps/portal/package.jsonapps/portal/src/actions.jsapps/portal/src/components/notification.jsapps/portal/src/utils/errors.jsapps/portal/src/utils/gift-redemption-notification.jsapps/portal/src/utils/notifications.jsapps/portal/test/app.test.jsapps/portal/test/portal-links.test.jsapps/portal/test/unit/components/notification.test.jsapps/portal/test/unit/components/pages/gift-redemption-page.test.jsapps/portal/test/unit/notifications.test.jsghost/core/core/server/services/gifts/gift-service.tsghost/core/core/server/services/members/middleware.jsghost/core/test/unit/server/services/gifts/gift-service.test.tsghost/core/test/unit/server/services/members/middleware.test.jsghost/i18n/locales/af/portal.jsonghost/i18n/locales/ar/portal.jsonghost/i18n/locales/bg/portal.jsonghost/i18n/locales/bn/portal.jsonghost/i18n/locales/bs/portal.jsonghost/i18n/locales/ca/portal.jsonghost/i18n/locales/context.jsonghost/i18n/locales/cs/portal.jsonghost/i18n/locales/da/portal.jsonghost/i18n/locales/de-CH/portal.jsonghost/i18n/locales/de/portal.jsonghost/i18n/locales/el/portal.jsonghost/i18n/locales/en/portal.jsonghost/i18n/locales/eo/portal.jsonghost/i18n/locales/es/portal.jsonghost/i18n/locales/et/portal.jsonghost/i18n/locales/eu/portal.jsonghost/i18n/locales/fa/portal.jsonghost/i18n/locales/fi/portal.jsonghost/i18n/locales/fr/portal.jsonghost/i18n/locales/gd/portal.jsonghost/i18n/locales/he/portal.jsonghost/i18n/locales/hi/portal.jsonghost/i18n/locales/hr/portal.jsonghost/i18n/locales/hu/portal.jsonghost/i18n/locales/id/portal.jsonghost/i18n/locales/is/portal.jsonghost/i18n/locales/it/portal.jsonghost/i18n/locales/ja/portal.jsonghost/i18n/locales/ko/portal.jsonghost/i18n/locales/kz/portal.jsonghost/i18n/locales/lt/portal.jsonghost/i18n/locales/lv/portal.jsonghost/i18n/locales/mk/portal.jsonghost/i18n/locales/mn/portal.jsonghost/i18n/locales/ms/portal.jsonghost/i18n/locales/nb/portal.jsonghost/i18n/locales/ne/portal.jsonghost/i18n/locales/nl/portal.jsonghost/i18n/locales/nn/portal.jsonghost/i18n/locales/pa/portal.jsonghost/i18n/locales/pl/portal.jsonghost/i18n/locales/pt-BR/portal.jsonghost/i18n/locales/pt/portal.jsonghost/i18n/locales/ro/portal.jsonghost/i18n/locales/ru/portal.jsonghost/i18n/locales/si/portal.jsonghost/i18n/locales/sk/portal.jsonghost/i18n/locales/sl/portal.jsonghost/i18n/locales/sq/portal.jsonghost/i18n/locales/sr-Cyrl/portal.jsonghost/i18n/locales/sr/portal.jsonghost/i18n/locales/sv/portal.jsonghost/i18n/locales/sw/portal.jsonghost/i18n/locales/ta/portal.jsonghost/i18n/locales/th/portal.jsonghost/i18n/locales/tr/portal.jsonghost/i18n/locales/uk/portal.jsonghost/i18n/locales/ur/portal.jsonghost/i18n/locales/uz/portal.jsonghost/i18n/locales/vi/portal.jsonghost/i18n/locales/zh-Hant/portal.jsonghost/i18n/locales/zh/portal.json
✅ Files skipped from review due to trivial changes (11)
- apps/portal/test/unit/components/pages/gift-redemption-page.test.js
- apps/portal/package.json
- apps/portal/test/app.test.js
- ghost/i18n/locales/pl/portal.json
- ghost/i18n/locales/uz/portal.json
- apps/portal/test/portal-links.test.js
- ghost/i18n/locales/sk/portal.json
- ghost/i18n/locales/en/portal.json
- ghost/i18n/locales/sl/portal.json
- ghost/i18n/locales/ne/portal.json
- ghost/i18n/locales/ca/portal.json
🚧 Files skipped from review as they are similar to previous changes (4)
- ghost/core/core/server/services/members/middleware.js
- apps/portal/src/components/notification.js
- apps/portal/test/unit/components/notification.test.js
- ghost/core/test/unit/server/services/gifts/gift-service.test.ts
sagzy
force-pushed
the
gift-error-message-BER-3569
branch
2 times, most recently
from
f1de187 to
29636fc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/portal/src/actions.js (1)
279-296:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep pre-redemption failures in-popup instead of tearing down the flow.
At Line 291–Line 294, the catch path always closes the popup and clears state. This catch also handles logged-out pre-redeem failures (
sendMagicLink), so users lose input/context on retry.💡 Suggested adjustment
return { action: 'redeemGift:failed', - showPopup: false, - lastPage: null, - pageQuery: '', - popupNotification: null, + ...(state.member ? { + showPopup: false, + lastPage: null, + pageQuery: '', + popupNotification: null + } : { + showPopup: true, + lastPage: state.lastPage, + pageQuery: state.pageQuery, + popupNotification: state.popupNotification + }), notification, notificationSequence: notification.count };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/portal/src/actions.js` around lines 279 - 296, The catch block returning the action 'redeemGift:failed' tears down the popup and clears state (showPopup: false, lastPage: null, pageQuery: '', popupNotification: null) which loses user input for pre-redemption failures like sendMagicLink; update the return in apps/portal/src/actions.js so that on pre-redemption failures (or by default for this catch) you preserve the popup and flow state — keep showPopup true, retain lastPage, pageQuery, popupNotification and the existing state, and only update notification/notificationSequence (and action) instead of clearing the modal context.
🤖 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/src/utils/gift-redemption-notification.js`:
- Around line 27-30: The current code sets subtitle = error?.message ||
t('Something went wrong...') which can expose raw backend text to members;
change it to default to the localized generic message (t('Something went wrong,
please try again later.')) and only override subtitle inside the existing
switch(error.code) for known safe codes in this file (the subtitle variable and
the switch block). Do not use error?.message as the default; if you must capture
raw error details for debugging, send error?.message to logs or a
non-user-facing telemetry call but never display it directly. Also apply the
same change to the other occurrence around the 52–54 area so both paths only
expose localized messages unless a known error.code sets a safe localized
override.
---
Duplicate comments:
In `@apps/portal/src/actions.js`:
- Around line 279-296: The catch block returning the action 'redeemGift:failed'
tears down the popup and clears state (showPopup: false, lastPage: null,
pageQuery: '', popupNotification: null) which loses user input for
pre-redemption failures like sendMagicLink; update the return in
apps/portal/src/actions.js so that on pre-redemption failures (or by default for
this catch) you preserve the popup and flow state — keep showPopup true, retain
lastPage, pageQuery, popupNotification and the existing state, and only update
notification/notificationSequence (and action) instead of clearing the modal
context.
🪄 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: 3f4dd0e1-cb89-4168-8ede-3bd753e4b5d0
📒 Files selected for processing (75)
apps/portal/src/actions.jsapps/portal/src/components/notification.jsapps/portal/src/utils/errors.jsapps/portal/src/utils/gift-redemption-notification.jsapps/portal/src/utils/notifications.jsapps/portal/test/app.test.jsapps/portal/test/portal-links.test.jsapps/portal/test/unit/components/notification.test.jsapps/portal/test/unit/components/pages/gift-redemption-page.test.jsapps/portal/test/unit/notifications.test.jsghost/core/core/server/services/members/middleware.jsghost/core/test/unit/server/services/members/middleware.test.jsghost/i18n/locales/af/portal.jsonghost/i18n/locales/ar/portal.jsonghost/i18n/locales/bg/portal.jsonghost/i18n/locales/bn/portal.jsonghost/i18n/locales/bs/portal.jsonghost/i18n/locales/ca/portal.jsonghost/i18n/locales/context.jsonghost/i18n/locales/cs/portal.jsonghost/i18n/locales/da/portal.jsonghost/i18n/locales/de-CH/portal.jsonghost/i18n/locales/de/portal.jsonghost/i18n/locales/el/portal.jsonghost/i18n/locales/en/portal.jsonghost/i18n/locales/eo/portal.jsonghost/i18n/locales/es/portal.jsonghost/i18n/locales/et/portal.jsonghost/i18n/locales/eu/portal.jsonghost/i18n/locales/fa/portal.jsonghost/i18n/locales/fi/portal.jsonghost/i18n/locales/fr/portal.jsonghost/i18n/locales/gd/portal.jsonghost/i18n/locales/he/portal.jsonghost/i18n/locales/hi/portal.jsonghost/i18n/locales/hr/portal.jsonghost/i18n/locales/hu/portal.jsonghost/i18n/locales/id/portal.jsonghost/i18n/locales/is/portal.jsonghost/i18n/locales/it/portal.jsonghost/i18n/locales/ja/portal.jsonghost/i18n/locales/ko/portal.jsonghost/i18n/locales/kz/portal.jsonghost/i18n/locales/lt/portal.jsonghost/i18n/locales/lv/portal.jsonghost/i18n/locales/mk/portal.jsonghost/i18n/locales/mn/portal.jsonghost/i18n/locales/ms/portal.jsonghost/i18n/locales/nb/portal.jsonghost/i18n/locales/ne/portal.jsonghost/i18n/locales/nl/portal.jsonghost/i18n/locales/nn/portal.jsonghost/i18n/locales/pa/portal.jsonghost/i18n/locales/pl/portal.jsonghost/i18n/locales/pt-BR/portal.jsonghost/i18n/locales/pt/portal.jsonghost/i18n/locales/ro/portal.jsonghost/i18n/locales/ru/portal.jsonghost/i18n/locales/si/portal.jsonghost/i18n/locales/sk/portal.jsonghost/i18n/locales/sl/portal.jsonghost/i18n/locales/sq/portal.jsonghost/i18n/locales/sr-Cyrl/portal.jsonghost/i18n/locales/sr/portal.jsonghost/i18n/locales/sv/portal.jsonghost/i18n/locales/sw/portal.jsonghost/i18n/locales/ta/portal.jsonghost/i18n/locales/th/portal.jsonghost/i18n/locales/tr/portal.jsonghost/i18n/locales/uk/portal.jsonghost/i18n/locales/ur/portal.jsonghost/i18n/locales/uz/portal.jsonghost/i18n/locales/vi/portal.jsonghost/i18n/locales/zh-Hant/portal.jsonghost/i18n/locales/zh/portal.json
✅ Files skipped from review due to trivial changes (29)
- apps/portal/test/unit/components/pages/gift-redemption-page.test.js
- ghost/i18n/locales/en/portal.json
- ghost/i18n/locales/bg/portal.json
- ghost/i18n/locales/ca/portal.json
- apps/portal/test/app.test.js
- ghost/i18n/locales/nn/portal.json
- ghost/i18n/locales/fr/portal.json
- ghost/i18n/locales/pa/portal.json
- ghost/i18n/locales/pl/portal.json
- ghost/i18n/locales/ja/portal.json
- ghost/i18n/locales/el/portal.json
- ghost/i18n/locales/th/portal.json
- ghost/i18n/locales/eo/portal.json
- ghost/i18n/locales/nb/portal.json
- ghost/i18n/locales/sl/portal.json
- ghost/i18n/locales/uz/portal.json
- ghost/i18n/locales/es/portal.json
- ghost/i18n/locales/sk/portal.json
- ghost/i18n/locales/it/portal.json
- ghost/i18n/locales/fa/portal.json
- ghost/i18n/locales/ur/portal.json
- ghost/i18n/locales/pt/portal.json
- ghost/i18n/locales/is/portal.json
- ghost/i18n/locales/ne/portal.json
- ghost/i18n/locales/bs/portal.json
- ghost/i18n/locales/lv/portal.json
- ghost/i18n/locales/mn/portal.json
- ghost/i18n/locales/cs/portal.json
- ghost/i18n/locales/sq/portal.json
🚧 Files skipped from review as they are similar to previous changes (31)
- apps/portal/test/portal-links.test.js
- ghost/i18n/locales/lt/portal.json
- ghost/core/test/unit/server/services/members/middleware.test.js
- ghost/i18n/locales/sr-Cyrl/portal.json
- ghost/i18n/locales/id/portal.json
- ghost/i18n/locales/tr/portal.json
- ghost/i18n/locales/ms/portal.json
- ghost/i18n/locales/hr/portal.json
- ghost/i18n/locales/fi/portal.json
- ghost/i18n/locales/nl/portal.json
- ghost/i18n/locales/zh/portal.json
- ghost/i18n/locales/ro/portal.json
- ghost/i18n/locales/de/portal.json
- ghost/i18n/locales/vi/portal.json
- ghost/i18n/locales/hi/portal.json
- ghost/i18n/locales/ta/portal.json
- ghost/i18n/locales/ru/portal.json
- ghost/i18n/locales/eu/portal.json
- ghost/i18n/locales/bn/portal.json
- ghost/i18n/locales/he/portal.json
- ghost/i18n/locales/uk/portal.json
- ghost/i18n/locales/mk/portal.json
- ghost/i18n/locales/sw/portal.json
- ghost/i18n/locales/af/portal.json
- ghost/i18n/locales/pt-BR/portal.json
- ghost/i18n/locales/sr/portal.json
- ghost/i18n/locales/ko/portal.json
- apps/portal/src/utils/notifications.js
- ghost/i18n/locales/kz/portal.json
- apps/portal/src/utils/errors.js
- ghost/i18n/locales/context.json
- Both the magic-link and logged-in redemption paths previously showed the same generic dead-end error regardless of cause, leaving members with no actionable information - Now surface the actual backend reason (e.g. account already has an active subscription, gift expired) so members understand what went wrong and how to proceed
sagzy
force-pushed
the
gift-error-message-BER-3569
branch
from
29636fc to
9461783
Compare
sagzy
force-pushed
the
gift-error-message-BER-3569
branch
from
9461783 to
cab4e32
Compare
5 tasks
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.
closes https://linear.app/ghost/issue/BER-3569