✨ Added One-Time-Code flow to member sign-in#25187
Conversation
WalkthroughThis PR removes the private feature flags Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📥 CommitsReviewing files that changed from the base of the PR and between 797548b075cc145dfe41766dbf368a4f7148e86a and b3ccf4b. ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (16)
💤 Files with no reviewable changes (4)
🚧 Files skipped from review as they are similar to previous changes (6)
🧰 Additional context used📓 Path-based instructions (2)ghost/core/core/server/**📄 CodeRabbit inference engine (AGENTS.md)
Files:
ghost/core/core/server/services/**📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-09-02T12:37:00.421ZApplied to files:
🧬 Code graph analysis (5)apps/portal/src/components/pages/MagicLinkPage.js (3)
apps/portal/src/actions.js (1)
apps/portal/src/tests/SigninFlow.test.js (1)
apps/portal/src/components/pages/MagicLinkPage.test.js (2)
ghost/core/test/e2e-api/members/send-magic-link.test.js (6)
⏰ 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). (3)
🔇 Additional comments (10)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #25187 +/- ##
==========================================
- Coverage 71.44% 71.43% -0.02%
==========================================
Files 1529 1529
Lines 116172 116170 -2
Branches 13983 13978 -5
==========================================
- Hits 82995 82981 -14
- Misses 32168 32198 +30
+ Partials 1009 991 -18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/portal/src/components/pages/MagicLinkPage.test.js (1)
314-323: Rename stale test title mentioning lab flagTitle says “even with lab flag” but labs are empty. Rename for clarity.
- test('does not render form without otcRef even with lab flag', () => { + test('does not render form without otcRef', () => {apps/portal/src/tests/SigninFlow.test.js (1)
143-147: Consistent OTC signaling and assertionsGood: signin payloads include includeOTC: true; mocks return otc_ref; labs default to {}. Matches OTC-by-request behavior. Based on learnings.
You already have expectOTCEnabledApiCall below; consider reusing it in earlier tests to reduce duplication.
Also applies to: 194-199, 223-229, 268-273, 297-302, 326-331, 436-437
ghost/core/test/unit/server/services/lib/magic-link/index.test.js (1)
104-121: Harden assertions and lock intended scope (signin-only)
- Avoid brittle call-count: asserting “calledTwice” ties tests to internal implementation.
- Add a test ensuring includeOTC is ignored for signup (intent: OTC is for sign-in).
Apply this to relax the call-count dependency:
- assert(mockSingleUseTokenProvider.getRefByToken.calledTwice); + assert(mockSingleUseTokenProvider.getRefByToken.callCount >= 1);Add a test to lock signup behavior:
it('does not include OTC for signup even when includeOTC=true', async function () { const options = buildOptions(); const service = new MagicLink(options); const args = { email: 'test@example.com', tokenData: {id: '420'}, includeOTC: true, type: 'signup' }; const result = await service.sendMagicLink(args); assert.equal(result.otcRef, null); assert(options.getText.firstCall.calledWithExactly('FAKEURL', 'signup', 'test@example.com', null)); assert(options.getHTML.firstCall.calledWithExactly('FAKEURL', 'signup', 'test@example.com', null)); assert(options.getSubject.firstCall.calledWithExactly('signup', null)); });Please confirm the intended constraint is “OTC applies to signin only.”
Also applies to: 125-141, 145-162, 163-187, 188-211, 213-235, 239-260
ghost/core/test/e2e-api/members/send-magic-link.test.js (1)
792-797: Tighten helper semantics and OTC extraction; add signup guard
Ensure we can explicitly test includeOTC: false (boolean) vs missing param:
- Update helper to send the field when false is passed.
Reduce chance of matching unrelated numbers when extracting the code:
- Prefer a label-anchored regex if available in templates.
Helper change:
function sendMagicLinkRequest(email, emailType = 'signin', otc = false) { const body = {email, emailType}; - if (otc) { + if (otc !== undefined) { body.includeOTC = otc; } return membersAgent.post('/api/send-magic-link').body(body); }
- OTC extraction (if email content includes a “Code:” label):
- const otc = mail.text.match(/\d{6}/)[0]; + const match = mail.text.match(/code:\s*(\d{6})/i); + const otc = match && match[1];
- Add a guard test to ensure signup never includes OTC even if requested:
it('Does not include OTC for signup even when includeOTC=true', async function () { const response = await sendMagicLinkRequest('member1@test.com', 'signup', true).expectStatus(201); assert(!response.body.otc_ref); const mail = mockManager.assert.sentEmail({to: 'member1@test.com'}); assertNoOTCInEmailContent(mail); });Also applies to: 799-804, 806-811, 813-821, 845-858, 860-873, 907-929
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 2d1570e and 9e1554d53419c67ea9c3fec1a4e65890cb9097ce.
⛔ Files ignored due to path filters (3)
ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/members/__snapshots__/send-magic-link.test.js.snapis excluded by!**/*.snapghost/core/test/unit/frontend/helpers/__snapshots__/ghost_head.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (16)
apps/admin-x-settings/src/components/settings/advanced/labs/PrivateFeatures.tsx(0 hunks)apps/portal/src/actions.js(1 hunks)apps/portal/src/components/pages/MagicLinkPage.js(2 hunks)apps/portal/src/components/pages/MagicLinkPage.test.js(5 hunks)apps/portal/src/data-attributes.js(1 hunks)apps/portal/src/index.js(0 hunks)apps/portal/src/tests/SigninFlow.test.js(7 hunks)apps/portal/src/tests/data-attributes.test.js(3 hunks)ghost/admin/app/services/feature.js(0 hunks)ghost/core/core/frontend/helpers/ghost_head.js(1 hunks)ghost/core/core/server/services/lib/magic-link/MagicLink.js(2 hunks)ghost/core/core/server/services/members/members-api/controllers/RouterController.js(2 hunks)ghost/core/core/shared/labs.js(0 hunks)ghost/core/test/e2e-api/members/send-magic-link.test.js(3 hunks)ghost/core/test/unit/server/services/lib/magic-link/index.test.js(7 hunks)ghost/core/test/unit/server/services/members/members-api/controllers/RouterController.test.js(5 hunks)
💤 Files with no reviewable changes (4)
- apps/portal/src/index.js
- ghost/admin/app/services/feature.js
- apps/admin-x-settings/src/components/settings/advanced/labs/PrivateFeatures.tsx
- ghost/core/core/shared/labs.js
🧰 Additional context used
📓 Path-based instructions (3)
ghost/core/core/frontend/**
📄 CodeRabbit inference engine (AGENTS.md)
Frontend and theme rendering for core should be under ghost/core/core/frontend/
Files:
ghost/core/core/frontend/helpers/ghost_head.js
ghost/core/core/server/**
📄 CodeRabbit inference engine (AGENTS.md)
Backend core logic should reside under ghost/core/core/server/
Files:
ghost/core/core/server/services/lib/magic-link/MagicLink.jsghost/core/core/server/services/members/members-api/controllers/RouterController.js
ghost/core/core/server/services/**
📄 CodeRabbit inference engine (AGENTS.md)
Backend services should be implemented under ghost/core/core/server/services/
Files:
ghost/core/core/server/services/lib/magic-link/MagicLink.jsghost/core/core/server/services/members/members-api/controllers/RouterController.js
🧠 Learnings (2)
📓 Common learnings
Learnt from: kevinansfield
PR: TryGhost/Ghost#25118
File: apps/portal/src/actions.js:160-173
Timestamp: 2025-10-09T15:31:06.587Z
Learning: When reviewing PRs that introduce feature-flagged changes (e.g., `labs?.membersSigninOTCAlpha`), avoid suggesting modifications to non-flagged code paths unless they're directly related to the PR's objectives. Keep the scope focused on the feature-flag-specific changes only.
📚 Learning: 2025-09-02T12:37:00.421Z
Learnt from: kevinansfield
PR: TryGhost/Ghost#24779
File: ghost/core/core/server/services/members/members-api/members-api.js:238-240
Timestamp: 2025-09-02T12:37:00.421Z
Learning: The otcVerification parameter in Ghost's members API functions like getTokenDataFromMagicLinkToken is optional and only relevant for OTC (one-time code) verification flows. Existing call sites that don't use OTC verification don't need to be updated as the parameter will be undefined, which is the expected behavior.
Applied to files:
ghost/core/core/server/services/lib/magic-link/MagicLink.jsapps/portal/src/tests/SigninFlow.test.js
🧬 Code graph analysis (6)
ghost/core/core/frontend/helpers/ghost_head.js (2)
ghost/core/core/shared/labs.js (1)
settingsCache(13-13)ghost/core/core/frontend/services/proxy.js (1)
settingsCache(2-2)
apps/portal/src/actions.js (1)
ghost/core/test/e2e-api/members/send-magic-link.test.js (7)
response(799-800)response(806-807)response(847-848)response(862-863)response(880-881)response(900-901)response(908-909)
apps/portal/src/components/pages/MagicLinkPage.js (3)
apps/portal/src/actions.js (1)
otcRef(125-125)apps/portal/src/data-attributes.js (1)
otcRef(103-103)ghost/core/test/e2e-api/members/send-magic-link.test.js (2)
otcRef(915-915)otcRef(1013-1013)
ghost/core/test/e2e-api/members/send-magic-link.test.js (3)
ghost/core/core/server/services/members/SingleUseTokenProvider.js (2)
require(2-2)require(5-5)ghost/core/core/server/web/shared/middleware/api/spam-prevention.js (1)
otcVerification(290-319)ghost/core/test/utils/e2e-framework.js (1)
resetRateLimits(174-178)
apps/portal/src/tests/data-attributes.test.js (2)
apps/portal/src/index.js (1)
labs(27-27)ghost/core/core/shared/labs.js (1)
labs(59-59)
apps/portal/src/tests/SigninFlow.test.js (1)
apps/portal/src/App.js (1)
App(45-1045)
⏰ 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: Admin tests - Chrome
- GitHub Check: Legacy tests (Node 22.13.1, sqlite3)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Legacy tests (Node 22.13.1, mysql8)
- GitHub Check: Acceptance tests (Node 22.13.1, mysql8)
- GitHub Check: Acceptance tests (Node 22.13.1, sqlite3)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Build & Push
🔇 Additional comments (14)
ghost/core/core/frontend/helpers/ghost_head.js (1)
66-66: LGTM! OTC feature flag attributes removed correctly.The removal of OTC-related data attributes aligns with the PR objective to remove feature flag gating. The locale attribute correctly remains in place.
Note: The AI summary incorrectly states that the locale attribute was removed, but the code shows it's still present.
ghost/core/core/server/services/members/members-api/controllers/RouterController.js (2)
668-671: LGTM! OTC flow now driven by response data.The check for
signIn.otcRefcorrectly determines whether to return OTC reference in the response, removing dependency on labs feature flags.
789-806: LGTM! Request-driven OTC handling.The OTC inclusion is now determined solely by the request body parameter, removing labs-based gating. The defensive check for both boolean
trueand string'true'handles potential type variations from different client implementations.ghost/core/core/server/services/lib/magic-link/MagicLink.js (2)
88-95: LGTM! OTC generation driven by request options.The conditional OTC generation now relies solely on
options.includeOTC, removing labs-based feature flag dependency. Error handling with Sentry capture remains intact.
109-116: LGTM! otcRef returned only when OTC successfully generated.The logic correctly returns
otcRefonly when an OTC has been successfully generated, preventing clients from showing OTC input fields when the email doesn't contain a code.ghost/core/test/unit/server/services/members/members-api/controllers/RouterController.test.js (1)
860-929: LGTM! Tests updated to reflect otcRef-driven OTC flow.The test suite correctly validates the new behavior where OTC response handling is driven by the presence of
otcRefin the response, independent of labs feature flags. The test fixture at line 883 appropriately setslabsService.isSetto returnfalse, confirming the removal of labs-based gating.Test coverage includes:
- OTC reference returned when present
- Standard response when otcRef is null
- Standard response when otcRef is undefined
apps/portal/src/data-attributes.js (1)
50-50: LGTM! OTC eligibility simplified.The OTC flag evaluation now depends solely on the form's data attributes (
data-members-otc="true") and email type, removing labs feature flag dependency. This aligns with the PR objective allowing custom forms to opt into the OTC flow.apps/portal/src/tests/data-attributes.test.js (2)
183-229: LGTM! Test updated to reflect attribute-driven OTC flow.The test correctly validates that OTC magic link requests and Portal opening are triggered when
data-members-otc="true"is set on the form, independent of labs flags. The emptylabsobject at line 196 confirms the removal of feature flag dependency.
231-283: LGTM! Exception handling test updated.The test validates that OTC action exceptions are properly captured and logged, with the empty
labsobject at line 244 confirming the removal of feature flag dependency.apps/portal/src/components/pages/MagicLinkPage.js (2)
224-281: LGTM! OTC form rendering driven by otcRef presence.The
renderOTCFormmethod now relies solely on the presence ofotcRefto determine whether to display the OTC input form, removing labs feature flag dependency. The early return at line 228-230 ensures the form is only rendered when an OTC reference exists.
283-294: LGTM! Simplified OTC form visibility logic.The render logic cleanly determines OTC form visibility based solely on
otcRefpresence, removing all labs-based conditional rendering. This provides a clear, single source of truth for whether to show the OTC input interface.apps/portal/src/actions.js (1)
81-111: Backend OTC implementation is efficient with proper safeguards in place—no action required.The unconditional
includeOTC: truechange is safe. Backend analysis confirms:
- OTC derivation is lightweight:
deriveOTC()uses deterministic HMAC hashing, not expensive generation- Rate limiting enforced: Spam prevention middleware applies multiple limits (
otc_verification.freeRetries,otc_verification_enumeration.freeRetries) to prevent abuse and brute force- Graceful error handling: System continues without OTC if derivation fails (not a critical path)
- Security safeguards: Constant-time comparison (
crypto.timingSafeEqual) prevents timing attacksThe change aligns with PR objectives and backend architecture supports unconditional OTC inclusion without performance concerns.
apps/portal/src/components/pages/MagicLinkPage.test.js (1)
7-14: OTC rendering keyed to otcRef-only looks goodGood cleanup: labs removed; OTC form visibility correctly depends solely on otcRef.
Also applies to: 33-41, 80-86, 87-99
ghost/core/test/e2e-api/members/send-magic-link.test.js (1)
336-340: OTC email snapshot looks goodScrubbing six-digit sequences keeps snapshots stable while validating OTC presence.
kevinansfield
force-pushed
the
release-members-OTC
branch
from
9e1554d to
33ed1af
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
apps/portal/src/tests/data-attributes.test.js (2)
183-229: OTC via data-members-otc works as intended; labs no longer needed hereTest correctly asserts includeOTC and startSigninOTCFromCustomForm with trimmed email. Consider dropping the unused labs parameter when calling formSubmitHandler to reduce noise.
244-283: OTC error capture test is solid; trim + redundant labsAssertions on captureException/console.error are good and resilient. labs is no longer used by formSubmitHandler; you can omit it from the call to make intent clearer.
apps/portal/src/components/pages/MagicLinkPage.test.js (2)
9-13: Default labs stub is fine, but redundantlabs isn’t used for OTC anymore; you can drop it from setupTest defaults to declutter.
36-40: setupOTCTest: labs override can be removedGiven gating is only on otcRef, labs: {} is unnecessary here.
apps/portal/src/data-attributes.js (1)
19-20: Signature cleanup is correct; remove stale labs plumbingformSubmitHandler no longer uses labs. handleDataAttributes still accepts/forwards labs; remove it to avoid dead params.
Apply this diff in handleDataAttributes to drop labs:
-export function handleDataAttributes({siteUrl, site = {}, member, labs = {}, doAction, captureException} = {}) { +export function handleDataAttributes({siteUrl, site = {}, member, doAction, captureException} = {}) { @@ - function submitHandler(event) { - formSubmitHandler({event, errorEl, form, siteUrl, submitHandler, labs, doAction, captureException}); - } + function submitHandler(event) { + formSubmitHandler({event, errorEl, form, siteUrl, submitHandler, doAction, captureException}); + }apps/portal/src/actions.js (1)
92-101: Show OTC UI only when otc_ref presentCorrect gating. Optional: trim email in payload to de-whitespace user input (pageData already trims).
Apply:
- const payload = { - ...data, + const payload = { + ...data, + email: (data?.email || '').trim(), emailType: 'signin', integrityToken, includeOTC: true };apps/portal/src/tests/SigninFlow.test.js (1)
436-437: labs prop in OTC Integration Flow is unnecessarySince OTC is not labs-gated, you can omit labs={{}} when rendering App to simplify tests.
ghost/core/test/e2e-api/members/send-magic-link.test.js (2)
401-416: Avoid brittle exact body keys in blocked-domain signin testsFuture API additions could break
Object.keys(body).should.eql(['otc_ref']). Prefer asserting presence/type only.Apply:
- .expect(({body}) => { - Object.keys(body).should.eql(['otc_ref']); - body.otc_ref.should.be.a.String().and.match(/^[a-f0-9-]{36}$/); - }); + .expect(({body}) => { + should.exist(body.otc_ref); + body.otc_ref.should.be.a.String().and.match(/^[a-f0-9-]{36}$/); + });And similarly for the second test:
- .expect(({body}) => { - should.exist(body.otc_ref); - body.otc_ref.should.be.a.String().and.match(/^[a-f0-9-]{36}$/); - }); + .expect(({body}) => { + should.exist(body.otc_ref); + body.otc_ref.should.be.a.String().and.match(/^[a-f0-9-]{36}$/); + });Also applies to: 419-436
812-820: Add explicit status/body assertions to reduce flakinessA couple of tests await the request but don't assert status/body. Add expectations to fail fast on regressions. Also, optionally tighten the "no OTC" regex to avoid false positives on the word "code".
Apply:
- await sendMagicLinkRequest('member1@test.com', 'signin', true); + await sendMagicLinkRequest('member1@test.com', 'signin', true) + .expectStatus(201);- await sendMagicLinkRequest('member1@test.com', emailType, false); + await sendMagicLinkRequest('member1@test.com', emailType, false) + .expectEmptyBody() + .expectStatus(201);Optional (outside this hunk): refine the no-OTC detector to use word boundaries.
- const otcRegex = /\d{6}|\scode\s|\sotc\s/i; + const otcRegex = /\d{6}|\bcode\b|\botc\b/i;Also applies to: 822-831
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 9e1554d53419c67ea9c3fec1a4e65890cb9097ce and 33ed1afbb6763326a0ef41ed12929773389aeb01.
⛔ Files ignored due to path filters (3)
ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/members/__snapshots__/send-magic-link.test.js.snapis excluded by!**/*.snapghost/core/test/unit/frontend/helpers/__snapshots__/ghost_head.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (16)
apps/admin-x-settings/src/components/settings/advanced/labs/PrivateFeatures.tsx(0 hunks)apps/portal/src/actions.js(1 hunks)apps/portal/src/components/pages/MagicLinkPage.js(2 hunks)apps/portal/src/components/pages/MagicLinkPage.test.js(5 hunks)apps/portal/src/data-attributes.js(2 hunks)apps/portal/src/index.js(0 hunks)apps/portal/src/tests/SigninFlow.test.js(7 hunks)apps/portal/src/tests/data-attributes.test.js(3 hunks)ghost/admin/app/services/feature.js(0 hunks)ghost/core/core/frontend/helpers/ghost_head.js(1 hunks)ghost/core/core/server/services/lib/magic-link/MagicLink.js(2 hunks)ghost/core/core/server/services/members/members-api/controllers/RouterController.js(2 hunks)ghost/core/core/shared/labs.js(0 hunks)ghost/core/test/e2e-api/members/send-magic-link.test.js(3 hunks)ghost/core/test/unit/server/services/lib/magic-link/index.test.js(7 hunks)ghost/core/test/unit/server/services/members/members-api/controllers/RouterController.test.js(5 hunks)
💤 Files with no reviewable changes (4)
- ghost/admin/app/services/feature.js
- ghost/core/core/shared/labs.js
- apps/portal/src/index.js
- apps/admin-x-settings/src/components/settings/advanced/labs/PrivateFeatures.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/portal/src/components/pages/MagicLinkPage.js
- ghost/core/core/server/services/lib/magic-link/MagicLink.js
- ghost/core/core/server/services/members/members-api/controllers/RouterController.js
- ghost/core/core/frontend/helpers/ghost_head.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kevinansfield
PR: TryGhost/Ghost#25118
File: apps/portal/src/actions.js:160-173
Timestamp: 2025-10-09T15:31:06.587Z
Learning: When reviewing PRs that introduce feature-flagged changes (e.g., `labs?.membersSigninOTCAlpha`), avoid suggesting modifications to non-flagged code paths unless they're directly related to the PR's objectives. Keep the scope focused on the feature-flag-specific changes only.
📚 Learning: 2025-09-02T12:37:00.421Z
Learnt from: kevinansfield
PR: TryGhost/Ghost#24779
File: ghost/core/core/server/services/members/members-api/members-api.js:238-240
Timestamp: 2025-09-02T12:37:00.421Z
Learning: The otcVerification parameter in Ghost's members API functions like getTokenDataFromMagicLinkToken is optional and only relevant for OTC (one-time code) verification flows. Existing call sites that don't use OTC verification don't need to be updated as the parameter will be undefined, which is the expected behavior.
Applied to files:
apps/portal/src/tests/SigninFlow.test.js
🧬 Code graph analysis (3)
apps/portal/src/actions.js (1)
ghost/core/test/e2e-api/members/send-magic-link.test.js (7)
response(799-800)response(806-807)response(847-848)response(862-863)response(880-881)response(900-901)response(908-909)
apps/portal/src/tests/SigninFlow.test.js (1)
apps/portal/src/App.js (1)
App(45-1045)
apps/portal/src/tests/data-attributes.test.js (2)
apps/portal/src/index.js (1)
labs(27-27)ghost/core/core/shared/labs.js (1)
labs(59-59)
⏰ 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). (12)
- GitHub Check: E2E Tests (Shard 2/2)
- GitHub Check: E2E Tests (Shard 1/2)
- GitHub Check: Inspect Docker Image
- GitHub Check: Legacy tests (Node 22.13.1, sqlite3)
- GitHub Check: Legacy tests (Node 22.13.1, mysql8)
- GitHub Check: Lint
- GitHub Check: Admin tests - Chrome
- GitHub Check: Acceptance tests (Node 22.13.1, mysql8)
- GitHub Check: Acceptance tests (Node 22.13.1, sqlite3)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Admin-X Settings tests
- GitHub Check: Unit tests (Node 22.13.1)
🔇 Additional comments (24)
apps/portal/src/components/pages/MagicLinkPage.test.js (3)
80-86: Gating OTC form solely on otcRef is correctCleanly reflects the new flow; looks good.
89-97: Negative scenario reads wellEnsures form is absent without otcRef; good coverage.
316-323: No otcRef → no form, regardless of labsAccurately tests the updated behavior; LGTM.
ghost/core/test/unit/server/services/lib/magic-link/index.test.js (7)
104-120: No includeOTC → no otcRefCorrectly asserts otcRef omission and no ref lookup when includeOTC isn’t set.
125-141: Graceful handling without getRefByTokenGood fallback: continues email send, otcRef null.
145-161: Null ref path coveredSkips deriveOTC and keeps otcRef null; good defensive test.
163-187: includeOTC=true path verifies ref + OTC propagation into emailsAsserts tokenProvider interactions and OTC inclusion in subject/text/html; solid coverage.
188-210: includeOTC=false path verifiedEnsures no OTC calls and null passed to email content functions.
213-234: Email content functions assertions (OTC) are preciseValidates argument wiring; looks good.
239-260: Missing deriveOTC handled cleanlyOTC omitted when deriveOTC is unavailable; expected behavior.
ghost/core/test/unit/server/services/members/members-api/controllers/RouterController.test.js (3)
883-884: labsService isSet(false) is fine; behavior no longer gated by labsKeeps tests honest about flag removal; OTC behavior is response-driven.
904-911: otc_ref returned only when presentChecks Content-Type and body; aligns with RouterController behavior.
913-929: No otcRef → standard 201 CreatedNegative cases (null/undefined) handled; correct response shape asserted.
apps/portal/src/actions.js (1)
88-89: Always request OTC for Portal sign-inThis aligns with the new flow; server will include magic link + code when available.
apps/portal/src/tests/SigninFlow.test.js (2)
143-147: Explicitly mocking otc_ref is appropriateEnsures OTC path is exercised; good setup for the default signin test.
197-199: Expecting includeOTC: true is correct across signin scenariosAssertions now reflect Portal’s unconditional OTC request.
Also applies to: 226-228, 272-273, 300-302, 330-331
apps/portal/src/data-attributes.js (1)
50-50: OTC condition is good; trim email to avoid whitespace issueswantsOTC logic matches the new flow. Trim the email before sending to the API to avoid leading/trailing spaces from custom forms.
- let email = emailInput?.value; + let email = emailInput?.value?.trim();The search for leftover labs-based OTC flags (
rg -n 'membersSigninOTC'andrg -n 'dataset\.\s*membersOtc') returned no matches; please manually confirm no flags remain.ghost/core/test/e2e-api/members/send-magic-link.test.js (7)
336-341: OTC signin snapshot looks goodSnapshot adds OTC scenario with scrubbing — solid coverage.
792-811: Response-shape assertions for includeOTC vs default are correctEmpty body by default; otc_ref present only when requested — LGTM.
845-873: includeOTC param coercion tests (true/false, string variants) look goodCovers both boolean and string cases; assertions on response and email content are solid.
875-897: OTC generation failure path is well handledStubbing
deriveOTCto throw and asserting success without OTC is the right behavior. Cleanup viafinallyis correct.
899-905: Non-existent member + includeOTC returns 400 without otc_refBehavior and assertions make sense. Good negative coverage.
907-944: /verify-otc happy-path and redirect precedence tests are solidHelper is clean; assertions on token and otc_verification presence and r param precedence look correct.
Also applies to: 946-958
960-977: OTC verification rate-limiting tests are comprehensiveCovers IP-based enumeration and per-otcRef counters, plus independence between refs. Config override/reset hygiene looks correct.
Also applies to: 979-1009, 1011-1042, 1044-1079
kevinansfield
force-pushed
the
release-members-OTC
branch
from
33ed1af to
797548b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/portal/src/data-attributes.js (1)
209-223: Remove unusedlabsparameter.The
labsparameter is passed toformSubmitHandlerat line 220, but after the changes at line 19,formSubmitHandlerno longer uses it. The parameter can be safely removed from this function call.Apply this diff to remove the unused parameter:
let errorEl = form.querySelector('[data-members-error]'); function submitHandler(event) { - formSubmitHandler({event, errorEl, form, siteUrl, submitHandler, labs, doAction, captureException}); + formSubmitHandler({event, errorEl, form, siteUrl, submitHandler, doAction, captureException}); } form.addEventListener('submit', submitHandler);ghost/core/core/server/services/lib/magic-link/MagicLink.js (1)
47-59: Consider removing unusedlabsServiceproperty.After the changes at lines 88 and 109, the
labsServiceproperty is no longer used in this class. Consider removing it from the constructor options and instance properties to keep the code clean.Apply this diff to remove the unused property:
constructor(options) { if (!options || !options.transporter || !options.tokenProvider || !options.getSigninURL) { throw new IncorrectUsageError({message: 'Missing options. Expects {transporter, tokenProvider, getSigninURL}'}); } this.transporter = options.transporter; this.tokenProvider = options.tokenProvider; this.getSigninURL = options.getSigninURL; this.getText = options.getText || defaultGetText; this.getHTML = options.getHTML || defaultGetHTML; this.getSubject = options.getSubject || defaultGetSubject; this.sentry = options.sentry || undefined; - this.labsService = options.labsService || undefined; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 33ed1afbb6763326a0ef41ed12929773389aeb01 and 797548b075cc145dfe41766dbf368a4f7148e86a.
⛔ Files ignored due to path filters (3)
ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/members/__snapshots__/send-magic-link.test.js.snapis excluded by!**/*.snapghost/core/test/unit/frontend/helpers/__snapshots__/ghost_head.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (16)
apps/admin-x-settings/src/components/settings/advanced/labs/PrivateFeatures.tsx(0 hunks)apps/portal/src/actions.js(1 hunks)apps/portal/src/components/pages/MagicLinkPage.js(2 hunks)apps/portal/src/components/pages/MagicLinkPage.test.js(5 hunks)apps/portal/src/data-attributes.js(2 hunks)apps/portal/src/index.js(0 hunks)apps/portal/src/tests/SigninFlow.test.js(12 hunks)apps/portal/src/tests/data-attributes.test.js(3 hunks)ghost/admin/app/services/feature.js(0 hunks)ghost/core/core/frontend/helpers/ghost_head.js(1 hunks)ghost/core/core/server/services/lib/magic-link/MagicLink.js(2 hunks)ghost/core/core/server/services/members/members-api/controllers/RouterController.js(2 hunks)ghost/core/core/shared/labs.js(0 hunks)ghost/core/test/e2e-api/members/send-magic-link.test.js(3 hunks)ghost/core/test/unit/server/services/lib/magic-link/index.test.js(7 hunks)ghost/core/test/unit/server/services/members/members-api/controllers/RouterController.test.js(5 hunks)
💤 Files with no reviewable changes (4)
- ghost/core/core/shared/labs.js
- apps/portal/src/index.js
- ghost/admin/app/services/feature.js
- apps/admin-x-settings/src/components/settings/advanced/labs/PrivateFeatures.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- ghost/core/core/server/services/members/members-api/controllers/RouterController.js
- apps/portal/src/components/pages/MagicLinkPage.js
- apps/portal/src/tests/data-attributes.test.js
🧰 Additional context used
📓 Path-based instructions (3)
ghost/core/core/frontend/**
📄 CodeRabbit inference engine (AGENTS.md)
Frontend and theme rendering for core should be under ghost/core/core/frontend/
Files:
ghost/core/core/frontend/helpers/ghost_head.js
ghost/core/core/server/**
📄 CodeRabbit inference engine (AGENTS.md)
Backend core logic should reside under ghost/core/core/server/
Files:
ghost/core/core/server/services/lib/magic-link/MagicLink.js
ghost/core/core/server/services/**
📄 CodeRabbit inference engine (AGENTS.md)
Backend services should be implemented under ghost/core/core/server/services/
Files:
ghost/core/core/server/services/lib/magic-link/MagicLink.js
🧠 Learnings (2)
📓 Common learnings
Learnt from: kevinansfield
PR: TryGhost/Ghost#25118
File: apps/portal/src/actions.js:160-173
Timestamp: 2025-10-09T15:31:06.587Z
Learning: When reviewing PRs that introduce feature-flagged changes (e.g., `labs?.membersSigninOTCAlpha`), avoid suggesting modifications to non-flagged code paths unless they're directly related to the PR's objectives. Keep the scope focused on the feature-flag-specific changes only.
📚 Learning: 2025-09-02T12:37:00.421Z
Learnt from: kevinansfield
PR: TryGhost/Ghost#24779
File: ghost/core/core/server/services/members/members-api/members-api.js:238-240
Timestamp: 2025-09-02T12:37:00.421Z
Learning: The otcVerification parameter in Ghost's members API functions like getTokenDataFromMagicLinkToken is optional and only relevant for OTC (one-time code) verification flows. Existing call sites that don't use OTC verification don't need to be updated as the parameter will be undefined, which is the expected behavior.
Applied to files:
apps/portal/src/tests/SigninFlow.test.jsghost/core/core/server/services/lib/magic-link/MagicLink.js
🧬 Code graph analysis (6)
ghost/core/test/e2e-api/members/send-magic-link.test.js (8)
ghost/core/core/server/services/members/members-api/controllers/RouterController.js (6)
tokenProvider(700-700)require(4-4)require(6-6)redirectUrl(734-734)ghost/core/test/e2e-api/members/signin.test.js (46)
magicLink(51-51)magicLink(62-62)magicLink(73-73)magicLink(84-84)magicLink(103-103)magicLink(122-122)magicLink(153-153)magicLink(165-165)magicLink(195-195)magicLink(247-247)magicLink(300-300)magicLink(342-342)magicLink(366-372)magicLinkUrl(52-52)magicLinkUrl(63-63)magicLinkUrl(74-74)magicLinkUrl(85-85)magicLinkUrl(104-104)magicLinkUrl(123-123)magicLinkUrl(154-154)magicLinkUrl(166-166)magicLinkUrl(196-196)magicLinkUrl(248-248)magicLinkUrl(301-301)magicLinkUrl(343-343)magicLinkUrl(373-373)token(53-53)token(64-64)token(75-75)token(86-86)token(105-105)token(124-124)token(155-155)token(167-167)token(197-197)token(249-249)token(302-302)token(344-344)token(374-374)require(1-1)ghost/core/core/server/services/members/middleware.js (7)
membersService(4-4)token(151-151)token(162-162)token(174-174)require(9-12)redirectUrl(382-382)redirectUrl(399-399)ghost/core/core/server/services/members/members-api/members-api.js (3)
body(2-2)require(1-1)require(17-17)ghost/core/test/unit/server/services/members/SingleUseTokenProvider.test.js (2)
tokenProvider(11-11)otc(333-333)ghost/core/core/server/services/members/SingleUseTokenProvider.js (2)
require(2-2)require(5-5)ghost/core/core/server/web/shared/middleware/api/spam-prevention.js (1)
otcVerification(290-319)ghost/core/test/utils/e2e-framework.js (1)
resetRateLimits(174-178)
apps/portal/src/components/pages/MagicLinkPage.test.js (2)
apps/portal/src/tests/SigninFlow.test.js (2)
utils(29-31)utils(85-87)apps/portal/src/utils/test-utils.js (2)
utils(34-34)utils(45-45)
apps/portal/src/tests/SigninFlow.test.js (1)
apps/portal/src/App.js (1)
App(45-1045)
ghost/core/core/frontend/helpers/ghost_head.js (1)
ghost/core/core/shared/labs.js (1)
settingsCache(13-13)
apps/portal/src/actions.js (1)
ghost/core/test/e2e-api/members/send-magic-link.test.js (7)
response(799-800)response(806-807)response(847-848)response(862-863)response(880-881)response(900-901)response(908-909)
apps/portal/src/data-attributes.js (2)
apps/portal/src/components/pages/MagicLinkPage.test.js (1)
form(50-50)apps/portal/src/tests/data-attributes.test.js (1)
form(31-36)
⏰ 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). (10)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Legacy tests (Node 22.13.1, mysql8)
- GitHub Check: Legacy tests (Node 22.13.1, sqlite3)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Acceptance tests (Node 22.13.1, mysql8)
- GitHub Check: Lint
- GitHub Check: Acceptance tests (Node 22.13.1, sqlite3)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Admin tests - Chrome
- GitHub Check: Build & Push
🔇 Additional comments (21)
ghost/core/core/frontend/helpers/ghost_head.js (1)
51-67: LGTM!The removal of
members-signin-otcandmembers-signin-otc-alphadata attributes is correct. The OTC flow now relies onotc_refin API responses rather than these frontend flags.ghost/core/core/server/services/lib/magic-link/MagicLink.js (2)
88-95: LGTM!The removal of
labsService.isSet('membersSigninOTC')check is correct. OTC retrieval is now driven solely byoptions.includeOTC, aligning with the PR's objective to remove feature flag gating.
109-116: LGTM!The removal of the
labsService.isSet('membersSigninOTC')check is correct. TheotcRefis now returned whenever an OTC is successfully derived, independent of feature flags.apps/portal/src/actions.js (1)
81-111: LGTM!The changes correctly implement the new OTC flow:
- Line 88: Always includes
includeOTC: truein signin requests, removing the labs flag dependency.- Line 92: Checks only for
response?.otc_refto determine OTC flow, which is the correct behavior.This aligns with the PR's objective to make OTC behavior driven by API responses rather than feature flags.
apps/portal/src/components/pages/MagicLinkPage.test.js (3)
7-40: LGTM!The test setup changes correctly reflect the new OTC flow:
- Line 9: Default
labsis now{}, removing the previousmembersSigninOTC: falseflag.- Line 36:
setupOTCTestno longer needslabs: {membersSigninOTC: true}and useslabs: {}instead.Tests now rely solely on
otcRefpresence to determine OTC behavior, which is the correct approach.
79-99: LGTM!The test updates correctly reflect the simplified OTC rendering logic:
- Line 80: Test renamed to "renders OTC form when otcRef exists" (removed flag reference).
- Line 89: Scenarios simplified from multiple flag combinations to a single scenario checking only
otcRef: null.This aligns with the OTC form now being controlled entirely by
otcRefpresence rather than feature flags.
313-323: LGTM!The edge case test correctly validates that the OTC form is not rendered when
otcRefis null, regardless of labs configuration. Line 316 now useslabs: {}instead of checking flag states.ghost/core/test/unit/server/services/lib/magic-link/index.test.js (1)
104-210: LGTM!The test suite updates correctly reflect the new OTC flow:
- Line 104: Describe block renamed from "sendMagicLink with OTC flags" to "sendMagicLink with OTC".
- Tests now use
buildOptions()consistently (lines 106, 125, 145, 164, 189, 213, 239) instead of passing custom labsService configurations.- Tests validate OTC behavior based on
includeOTCoption (lines 163, 188) rather than labsService flag state.All changes align with removing feature flag gating from OTC functionality.
ghost/core/test/unit/server/services/members/members-api/controllers/RouterController.test.js (1)
860-930: LGTM!The test suite updates correctly reflect the new OTC response handling:
- Line 860: Describe block renamed from "membersSigninOTC feature flag" to "OTC response handling".
- Line 883:
labsService.isSetnow returnsfalseby default, emphasizing that OTC behavior is no longer flag-dependent.- Lines 904-929: Tests simplified to focus on
otcRefpresence:
- Line 904: "should return otc_ref when otcRef exists" (removed flag reference)
- Line 913: "should not return otc_ref when no otcRef"
- Line 922: "should not return otc_ref when otcRef is undefined"
All changes align with OTC response behavior being driven by
otcRefpresence rather than feature flags.apps/portal/src/tests/SigninFlow.test.js (4)
123-131: LGTM!The helper function
expectOTCEnabledSendMagicLinkAPICallprovides a clean way to verify that signin API calls include the required OTC parameters:
includeOTC: trueemailType: 'signin'integrityTokenThis reduces duplication across tests and ensures consistent validation of the OTC-enabled flow.
146-176: LGTM!The test updates correctly reflect the new OTC flow:
- Lines 154-156:
sendMagicLinkis now mocked to returnotc_ref, enabling the OTC flow in the default signin test.- Line 175: Uses the new helper function to validate the OTC-enabled API call.
This aligns with OTC being driven by the presence of
otc_refin responses rather than feature flags.
389-429: LGTM!The OTC integration flow setup correctly removes feature flag dependencies:
- Line 416: Renders
Appwithlabs={{}}instead oflabs={{membersSigninOTC: true}}.- OTC flow is triggered by
sendMagicLinkreturningotc_ref(lines 399-403) rather than by feature flags.This aligns with the PR's objective to make OTC behavior response-driven.
450-506: LGTM!The integration tests correctly use the
expectOTCEnabledSendMagicLinkAPICallhelper:
- Lines 457, 484, 501: Consistent validation of OTC-enabled API calls across different test scenarios.
This ensures all signin flows include
includeOTC: trueand validates the new OTC behavior.ghost/core/test/e2e-api/members/send-magic-link.test.js (8)
336-340: LGTM! Good snapshot test for OTC email content.The test appropriately verifies the email content when OTC is requested, using the scrubEmailContent helper to normalize tokens and OTC codes for consistent snapshots.
792-810: LGTM! Clear tests for OTC response behavior.These tests effectively verify that
otc_refis returned only when explicitly requested viaincludeOTC: true, confirming the opt-in nature of the OTC feature.
812-832: LGTM! Comprehensive email content verification.Good use of helper functions and parameterized tests to verify OTC presence/absence in email content across different email types.
834-843: LGTM! Important regression test.This test ensures the traditional magic link flow continues to work correctly alongside the new OTC feature.
845-873: LGTM! Thorough edge case testing.Excellent coverage of type coercion scenarios, verifying that both boolean and string representations of true/false work correctly for the
includeOTCparameter.
875-897: LGTM! Excellent error handling coverage.This test demonstrates robust failure handling—when OTC generation fails, the system gracefully degrades by sending a magic link without OTC rather than failing the entire request. The verification that the stub was called (line 884) ensures the test is actually exercising the failure path.
899-958: LGTM! Comprehensive verification endpoint coverage.The
sendAndVerifyOTChelper (lines 907-929) is well-designed and supports testing various redirect scenarios. The tests cover:
- Non-existent member rejection (security)
- Basic verification flow with redirect URL validation
- Referer header handling
- Explicit redirect parameter override
960-1081: LGTM! Comprehensive rate limiting coverage.Excellent test coverage for OTC verification rate limiting:
- IP-based enumeration limiting prevents brute-force attacks across codes
- Per-code limiting prevents brute-force attacks on specific codes
- Independent tracking of different
otcRefvaluesThe defensive assertion at line 1049 ensuring
otcVerificationLimit < otcVerificationEnumerationLimitis particularly good—it validates the test setup and prevents false positives.
| it('allows signins from email domains blocked in config', async function () { | ||
| const email = 'hello-enabled@blocked-domain-config.com'; | ||
| await membersService.api.members.create({email, name: 'Member Test'}); | ||
|
|
||
| await membersAgent.post('/api/send-magic-link') | ||
| .body({ | ||
| email, | ||
| emailType: 'signin', | ||
| includeOTC: true | ||
| }) | ||
| .expectStatus(201) | ||
| .expect(({body}) => { | ||
| Object.keys(body).should.eql(['otc_ref']); | ||
| body.otc_ref.should.be.a.String().and.match(/^[a-f0-9-]{36}$/); | ||
| }); | ||
| }); | ||
|
|
||
| it('allows signins from email domains blocked in settings', async function () { | ||
| settingsCache.set('all_blocked_email_domains', {value: ['blocked-domain-setting.com']}); | ||
|
|
||
| const email = 'hello-enabled@blocked-domain-setting.com'; | ||
| await membersService.api.members.create({email, name: 'Member Test'}); | ||
|
|
||
| await membersAgent.post('/api/send-magic-link') | ||
| .body({ | ||
| email, | ||
| emailType: 'signin', | ||
| includeOTC: true | ||
| }) | ||
| .expectStatus(201) | ||
| .expect(({body}) => { | ||
| should.exist(body.otc_ref); | ||
| body.otc_ref.should.be.a.String().and.match(/^[a-f0-9-]{36}$/); | ||
| }); | ||
| }); | ||
| await membersAgent.post('/api/send-magic-link') | ||
| .body({ | ||
| email, | ||
| emailType: 'signin', | ||
| includeOTC: true | ||
| }) | ||
| .expectStatus(201) | ||
| .expect(({body}) => { | ||
| Object.keys(body).should.eql(['otc_ref']); | ||
| body.otc_ref.should.be.a.String().and.match(/^[a-f0-9-]{36}$/); | ||
| }); | ||
| }); | ||
|
|
||
| describe('with membersSigninOTC feature flag disabled', function () { | ||
| beforeEach(function () { | ||
| settingsCache.set('labs', {value: JSON.stringify({membersSigninOTC: false})}); | ||
| }); | ||
| it('allows signins from email domains blocked in settings', async function () { | ||
| settingsCache.set('all_blocked_email_domains', {value: ['blocked-domain-setting.com']}); | ||
|
|
||
| it('allows signins from email domains blocked in config', async function () { | ||
| const email = 'hello-disabled@blocked-domain-config.com'; | ||
| await membersService.api.members.create({email, name: 'Member Test'}); | ||
| const email = 'hello-enabled@blocked-domain-setting.com'; | ||
| await membersService.api.members.create({email, name: 'Member Test'}); | ||
|
|
||
| await membersAgent.post('/api/send-magic-link') | ||
| .body({ | ||
| email, | ||
| emailType: 'signin' | ||
| }) | ||
| .expectEmptyBody() | ||
| .expectStatus(201); | ||
| }); | ||
|
|
||
| it('allows signins from email domains blocked in settings', async function () { | ||
| settingsCache.set('all_blocked_email_domains', {value: ['blocked-domain-setting.com']}); | ||
|
|
||
| const email = 'hello-disabled@blocked-domain-setting.com'; | ||
| await membersService.api.members.create({email, name: 'Member Test'}); | ||
|
|
||
| await membersAgent.post('/api/send-magic-link') | ||
| .body({ | ||
| email, | ||
| emailType: 'signin' | ||
| }) | ||
| .expectEmptyBody() | ||
| .expectStatus(201); | ||
| }); | ||
| await membersAgent.post('/api/send-magic-link') | ||
| .body({ | ||
| email, | ||
| emailType: 'signin', | ||
| includeOTC: true | ||
| }) | ||
| .expectStatus(201) | ||
| .expect(({body}) => { | ||
| should.exist(body.otc_ref); | ||
| body.otc_ref.should.be.a.String().and.match(/^[a-f0-9-]{36}$/); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inconsistent response body assertions.
Lines 414-415 use a strict assertion expecting the response body to contain only otc_ref:
Object.keys(body).should.eql(['otc_ref']);Lines 433-434 use a lenient assertion checking only that otc_ref exists:
should.exist(body.otc_ref);Both tests verify similar scenarios (signin with blocked domains and includeOTC: true). The inconsistency suggests unclear expectations about the response structure.
Standardize the assertions. If the response should contain only otc_ref, use the strict assertion in both tests. If other fields are acceptable, use the lenient assertion consistently:
Option 1 (strict):
.expect(({body}) => {
+ Object.keys(body).should.eql(['otc_ref']);
- should.exist(body.otc_ref);
body.otc_ref.should.be.a.String().and.match(/^[a-f0-9-]{36}$/);
});Option 2 (lenient):
.expect(({body}) => {
- Object.keys(body).should.eql(['otc_ref']);
+ should.exist(body.otc_ref);
body.otc_ref.should.be.a.String().and.match(/^[a-f0-9-]{36}$/);
});🤖 Prompt for AI Agents
In ghost/core/test/e2e-api/members/send-magic-link.test.js around lines 402-436,
the two tests use inconsistent response assertions (one checks
Object.keys(body).should.eql(['otc_ref']) while the other uses
should.exist(body.otc_ref)); standardize them to the strict expectation: replace
the lenient should.exist(body.otc_ref) assertion in the second test with the
strict assertion Object.keys(body).should.eql(['otc_ref']) and keep the existing
otc_ref format check so both tests assert the response contains only otc_ref and
that it matches the UUID-like pattern.
closes https://linear.app/ghost/issue/BER-2520/ - Portal's sign-in flow will now include a one-time-code (OTC) in emails and display a code input form after submitting the standard email sign-in form - OTC usage is optional for members, magic links are still included alongside the code - reduces sign-in friction in scenarios such as in-app browsers - custom sign-in forms can opt in to the same OTC flow by adding a `data-members-otc=true"` - in this scenario, Portal will open the code input modal once a member submits their email in the custom form
kevinansfield
force-pushed
the
release-members-OTC
branch
from
797548b to
b3ccf4b
Compare
1 participant
closes https://linear.app/ghost/issue/BER-2520/
data-members-otc=true"