Improved email verification trigger tests#27026
Conversation
|
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 new test helper module exporting 🚥 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)
Comment |
aileen
force-pushed
the
gva-732-verification-flow-tests
branch
from
be32a18 to
a3e529f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ghost/core/test/legacy/api/admin/members-importer.test.js (1)
356-395: Add cleanup for nock interceptor to match other tests.This test creates a nock interceptor via
setupMockVerificationWebhookbut doesn't clean it up. For consistency withmembers.test.js(which usestry/finallywithnock.cleanAll()), consider adding cleanup to prevent potential test pollution.♻️ Suggested cleanup pattern
it('Can import members with host emailVerification limits for large imports - new flow', async function () { mockManager.mockLabsEnabled('verificationFlow'); await models.Settings.edit([{ key: 'email_verification_required', value: false }], {context: {internal: true}}); assert(!settingsCache.get('email_verification_required'), 'Email verification should not be required'); // If this test fails, check if the total members that have been created with fixtures has increased a lot, and if required, increase the amount of imported members const {receivedWebhookRequests} = setupMockVerificationWebhook({ apiThreshold: 2, adminThreshold: 2, importThreshold: 1 // note: this one isn't really used because (totalMembers - members_created_in_last_30_days) is larger and used instead }); + try { const awaitCompletion = jobManager.awaitCompletion('members-import'); const res = await request .post(localUtils.API.getApiQuery(`members/upload/`)) .field('labels', ['new-global-label']) .attach('membersfile', path.join(__dirname, '/../../../utils/fixtures/csv/valid-members-import-large-501.csv')) .set('Origin', config.get('url')) .expect('Content-Type', /json/) .expect('Cache-Control', testUtils.cacheRules.private) .expect(202); assert.equal(res.headers['x-cache-invalidate'], undefined); const jsonResponse = res.body; assertExists(jsonResponse); assertExists(jsonResponse.meta); // Wait for the job to finish await awaitCompletion; assert(!!settingsCache.get('email_verification_required'), 'Email verification should now be required'); assert(receivedWebhookRequests.length > 0, 'Expected to receive webhook requests'); + } finally { + nock.cleanAll(); + } });Note: You'll need to add
const nock = require('nock');to the imports if not already present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/legacy/api/admin/members-importer.test.js` around lines 356 - 395, The test 'Can import members with host emailVerification limits for large imports - new flow' creates a nock interceptor via setupMockVerificationWebhook but doesn't clean it up; wrap the test body (the code that calls setupMockVerificationWebhook, awaits awaitCompletion, and asserts receivedWebhookRequests) in a try/finally where the finally calls nock.cleanAll(), and add const nock = require('nock') to the test imports if it's not already present so the nock.cleanAll() call is available to clear the interceptor after the test.ghost/core/test/integration/services/email-service/batch-sending.test.js (1)
505-557: Consider capturing the returned scope object for better test control.The
setupMockVerificationWebhookfunction creates and returns a nock scope that can be used to assert on interceptor behavior or manually manage cleanup. The current test doesn't capture this return value, limiting visibility and control. While the test framework'smockManager.restore()does callnock.cleanAll()in the afterEach hook, explicitly capturing and managing the scope would improve test clarity and allow for scope-specific assertions if needed.♻️ Suggested improvement
it('Cannot send an email if verification is required', async function () { // Update this test to check the new flow instead. mockManager.mockLabsEnabled('verificationFlow'); // First enable import thresholds - setupMockVerificationWebhook({ + const {scope} = setupMockVerificationWebhook({ apiThreshold: 100, adminThreshold: 100, importThreshold: 100 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/integration/services/email-service/batch-sending.test.js` around lines 505 - 557, The test currently calls setupMockVerificationWebhook(...) but ignores its returned nock scope; capture that return (e.g., const scope = setupMockVerificationWebhook(...)) so you can assert on or explicitly clean that scope before finishing the test; update the test around the calls to setupMockVerificationWebhook and ensure you call scope.done() or scope.abort() as appropriate (in addition to existing mockManager.mockLabsEnabled('verificationFlow') and configUtils.restore()) to improve control and visibility of the interceptor behavior.
🤖 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/test/integration/services/email-service/batch-sending.test.js`:
- Around line 505-557: The test currently calls
setupMockVerificationWebhook(...) but ignores its returned nock scope; capture
that return (e.g., const scope = setupMockVerificationWebhook(...)) so you can
assert on or explicitly clean that scope before finishing the test; update the
test around the calls to setupMockVerificationWebhook and ensure you call
scope.done() or scope.abort() as appropriate (in addition to existing
mockManager.mockLabsEnabled('verificationFlow') and configUtils.restore()) to
improve control and visibility of the interceptor behavior.
In `@ghost/core/test/legacy/api/admin/members-importer.test.js`:
- Around line 356-395: The test 'Can import members with host emailVerification
limits for large imports - new flow' creates a nock interceptor via
setupMockVerificationWebhook but doesn't clean it up; wrap the test body (the
code that calls setupMockVerificationWebhook, awaits awaitCompletion, and
asserts receivedWebhookRequests) in a try/finally where the finally calls
nock.cleanAll(), and add const nock = require('nock') to the test imports if
it's not already present so the nock.cleanAll() call is available to clear the
interceptor after the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 203cc5fd-0372-405a-92a1-1a212e0f6f9e
📒 Files selected for processing (4)
ghost/core/test/e2e-api/admin/members.test.jsghost/core/test/integration/services/email-service/batch-sending.test.jsghost/core/test/legacy/api/admin/members-importer.test.jsghost/core/test/utils/verification-webhook-test-utils.ts
aileen
changed the title
verificationFlow labs flag
| @@ -502,13 +503,13 @@ describe.skip('Batch sending tests', function () { | |||
| }); | |||
|
|
|||
| it('Cannot send an email if verification is required', async function () { | |||
There was a problem hiding this comment.
The test is actually set to be skipped in the parent subscribe block, since those tests appear to be flaky. The changes to this test pass in isolation.
| apiThreshold: 2, | ||
| adminThreshold: 2, | ||
| importThreshold: 1, // note: this one isn't really used because (totalMembers - members_created_in_last_30_days) is larger and used instead | ||
| verified: false, |
There was a problem hiding this comment.
verified is set to false by default by the test util.
| // relies on that value being true already. | ||
| }); | ||
|
|
||
| it('Can still import members once email verification is required but does not send email', async function () { |
There was a problem hiding this comment.
Not a huge fan that this test relies on the data manipulation of the previous test, but since it's a legacy test, I didn't change more than needed.
| await restoreEmailVerificationUtils(); | ||
| }); | ||
|
|
||
| it('Can import members with host emailVerification limits for large imports - webhook flow', async function () { |
There was a problem hiding this comment.
I will remove the email checking tests above with this one, when promoting the labs flag to GA
| const webhookSecret = DEFAULT_WEBHOOK_SECRET; | ||
| const webhookEndpoint = new URL(webhookUrl); | ||
|
|
||
| await models.Settings.edit([{ |
There was a problem hiding this comment.
Using settingsCache.set didn't work for the tests. It didn't trigger the events and I wasn't able to successfully update the settings. The direct update of the Settings model was a pattern I saw used in other tests and adopted it here.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/verification-trigger.js`:
- Around line 107-122: The bug is that _handleMemberCreatedEvent uses
events.data.length (only the current page) while testImportThreshold uses
events.meta.pagination.total, so imports larger than the default page size are
undercounted; update both _handleMemberCreatedEvent and/or getSignupEvents calls
to request all results explicitly instead of relying on default pagination—e.g.,
call getSignupEvents({}, {source: 'member', limit:
<explicit-all-or-large-limit>}) or switch _handleMemberCreatedEvent to use
events.meta.pagination.total so both paths count the same total and still
respect the comment about write-behind timing when deciding to call
_startVerificationProcess.
In `@ghost/core/test/utils/email-verification-utils.ts`:
- Around line 69-77: The webhook mock's body parsing (inside the .reply
callback) currently does JSON.parse(rawBody) which can throw on empty or
non-JSON payloads; update the parsing for parsedBody to safely attempt
JSON.parse(rawBody) inside a try/catch and, on error, fall back to using rawBody
(string) or an empty object so the helper doesn't throw and tests can assert
payload shape; target the parsedBody logic and the same requestBody/rawBody
variables in this function 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: 509af433-207d-4380-a7be-31d8249a5a29
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/members.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
ghost/core/core/server/services/verification-trigger.jsghost/core/test/e2e-api/admin/members.test.jsghost/core/test/integration/services/email-service/batch-sending.test.jsghost/core/test/legacy/api/admin/members-importer.test.jsghost/core/test/utils/email-verification-utils.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- ghost/core/test/integration/services/email-service/batch-sending.test.js
- ghost/core/test/legacy/api/admin/members-importer.test.js
- ghost/core/test/e2e-api/admin/members.test.js
aileen
force-pushed
the
gva-732-verification-flow-tests
branch
from
abc5eee to
5b44a64
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ghost/core/test/integration/services/email-service/batch-sending.test.js (2)
519-556:⚠️ Potential issue | 🟡 MinorClean up the stub and helper state in
finally.This file does not call
sinon.restore()inafterEach, andrestoreEmailVerificationUtils()only runs on the happy path. If an assertion fails before the end of the test, both thegetSignupEventsstub and the verification helper state leak into later cases in an already-flaky suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/integration/services/email-service/batch-sending.test.js` around lines 519 - 556, The test leaks stubs/state because getSignupEvents is stubbed and restoreEmailVerificationUtils() is only called on the happy path; ensure cleanup always runs by adding a finally/afterEach cleanup that calls sinon.restore() and restoreEmailVerificationUtils() (or calls restoreEmailVerificationUtils() inside a try/finally around the test body) so the sinon stub getSignupEvents and any verification helper state are always restored even when assertions fail; locate the stub created via sinon.stub(events, 'getSignupEvents') and the call to restoreEmailVerificationUtils() and make their teardown unconditional.
519-525:⚠️ Potential issue | 🟡 MinorReturn the full signup-events page shape here.
The new verification-trigger path reads the page payload when calculating import counts. This stub only returns
meta.pagination.total, so the test no longer matches the repository contract and can start failing or bypassing the new count logic when this skipped suite is re-enabled.🧩 Example fix
const getSignupEvents = sinon.stub(events, 'getSignupEvents').resolves({ + data: Array.from({length: 101}, () => ({})), meta: { pagination: { - total: 100000 + total: 101, + limit: 101 } } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/integration/services/email-service/batch-sending.test.js` around lines 519 - 525, The stubbed getSignupEvents in the test only returns meta.pagination.total but the verification-trigger path expects the full page payload shape; update the sinon.stub for events.getSignupEvents to resolve with a complete page object including properties like events (an array of event objects matching the shape used in the code), meta.pagination (total, limit, page, pages) and any other fields consumed by the import-count logic so the test matches the repository contract and exercises the new count logic.
🧹 Nitpick comments (1)
ghost/core/test/unit/server/services/verification-trigger.test.js (1)
370-383: This case doesn't actually verify “does not subscribe”.Because
DomainEvents.subscribeis restored before the trigger is created, this only proves that no source-event lookup happens after dispatch. If the constructor still subscribes and the handler exits early, the test still passes. Either assert against the subscribe stub or rename the case to the weaker behavior it really checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/verification-trigger.test.js` around lines 370 - 383, The test currently restores DomainEvents.subscribe before creating the trigger so it cannot assert whether the constructor subscribed; update the test to either (A) keep DomainEvents.subscribe stubbed and assert it was not called (use the existing domainEventsStub to stub/spy DomainEvents.subscribe and then sinon.assert.notCalled(domainEventsStub) after creating createVerificationTrigger with isVerified: true), or (B) if you intend to test only that the handler doesn't react, rename the test to reflect that it verifies no handler invocation (e.g., "Does not invoke handler when site is already verified") and leave the current assertions against eventStub/MemberCreatedEvent dispatch. Use the symbols DomainEvents.subscribe, domainEventsStub, createVerificationTrigger, eventStub and MemberCreatedEvent to locate and apply 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/test/e2e-api/admin/members.test.js`:
- Around line 1018-1039: The test currently uses
receivedWebhookRequests.find(...) which will pass if one matching webhook exists
even when duplicates are sent; replace the find() with a filter: const
matchingRequests = receivedWebhookRequests.filter((request) => { ...same
predicate... }); then assert.strictEqual(matchingRequests.length, 1, 'Expected
exactly one verification webhook to be sent'); finally set const matchingRequest
= matchingRequests[0] and keep the existing timestamp/signature checks using
matchingRequest; this ensures the test fails if more than one matching webhook
is emitted.
---
Outside diff comments:
In `@ghost/core/test/integration/services/email-service/batch-sending.test.js`:
- Around line 519-556: The test leaks stubs/state because getSignupEvents is
stubbed and restoreEmailVerificationUtils() is only called on the happy path;
ensure cleanup always runs by adding a finally/afterEach cleanup that calls
sinon.restore() and restoreEmailVerificationUtils() (or calls
restoreEmailVerificationUtils() inside a try/finally around the test body) so
the sinon stub getSignupEvents and any verification helper state are always
restored even when assertions fail; locate the stub created via
sinon.stub(events, 'getSignupEvents') and the call to
restoreEmailVerificationUtils() and make their teardown unconditional.
- Around line 519-525: The stubbed getSignupEvents in the test only returns
meta.pagination.total but the verification-trigger path expects the full page
payload shape; update the sinon.stub for events.getSignupEvents to resolve with
a complete page object including properties like events (an array of event
objects matching the shape used in the code), meta.pagination (total, limit,
page, pages) and any other fields consumed by the import-count logic so the test
matches the repository contract and exercises the new count logic.
---
Nitpick comments:
In `@ghost/core/test/unit/server/services/verification-trigger.test.js`:
- Around line 370-383: The test currently restores DomainEvents.subscribe before
creating the trigger so it cannot assert whether the constructor subscribed;
update the test to either (A) keep DomainEvents.subscribe stubbed and assert it
was not called (use the existing domainEventsStub to stub/spy
DomainEvents.subscribe and then sinon.assert.notCalled(domainEventsStub) after
creating createVerificationTrigger with isVerified: true), or (B) if you intend
to test only that the handler doesn't react, rename the test to reflect that it
verifies no handler invocation (e.g., "Does not invoke handler when site is
already verified") and leave the current assertions against
eventStub/MemberCreatedEvent dispatch. Use the symbols DomainEvents.subscribe,
domainEventsStub, createVerificationTrigger, eventStub and MemberCreatedEvent to
locate and apply 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: fffb7c13-edca-40e7-b6c8-2a48b0fa44c2
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/members.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
ghost/core/core/server/services/verification-trigger.jsghost/core/test/e2e-api/admin/members.test.jsghost/core/test/integration/services/email-service/batch-sending.test.jsghost/core/test/legacy/api/admin/members-importer.test.jsghost/core/test/unit/server/services/verification-trigger.test.jsghost/core/test/utils/email-verification-utils.ts
✅ Files skipped from review due to trivial changes (1)
- ghost/core/core/server/services/verification-trigger.js
🚧 Files skipped from review as they are similar to previous changes (2)
- ghost/core/test/legacy/api/admin/members-importer.test.js
- ghost/core/test/utils/email-verification-utils.ts
aileen
force-pushed
the
gva-732-verification-flow-tests
branch
2 times, most recently
from
36223be to
70c4bc8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ghost/core/test/utils/email-verification-utils.ts (2)
110-110:nock.cleanAll()is synchronous —awaitis unnecessary.This won't cause issues, but
cleanAllreturnsvoid, not aPromise.♻️ Suggested fix
- await nock.cleanAll(); + nock.cleanAll();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/utils/email-verification-utils.ts` at line 110, Remove the unnecessary await on the synchronous nock.cleanAll() call in the test helper; locate the line calling await nock.cleanAll() and change it to a plain nock.cleanAll() so it no longer awaits a void return from the nock.cleanAll() function.
70-74: Extract nested ternary for readability.The nested ternary makes this block harder to parse at a glance. Extracting to a helper or using explicit conditionals would improve clarity, though this is optional given it's test utility code.
♻️ Suggested refactor
- const rawBody = Buffer.isBuffer(requestBody) ? - requestBody.toString('utf8') : - typeof requestBody === 'string' ? - requestBody : - JSON.stringify(requestBody); + let rawBody: string; + if (Buffer.isBuffer(requestBody)) { + rawBody = requestBody.toString('utf8'); + } else if (typeof requestBody === 'string') { + rawBody = requestBody; + } else { + rawBody = JSON.stringify(requestBody); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/utils/email-verification-utils.ts` around lines 70 - 74, The nested ternary used to compute rawBody from requestBody is hard to read; replace it with a small helper or explicit conditionals (e.g., a function like normalizeRequestBody(requestBody) or an if/else block) that handles Buffer, string, and other types and returns the utf8 string or JSON string; update the usage where rawBody is assigned so the logic is clearer and unit-test behavior is unchanged (referencing the rawBody variable and the requestBody input in email-verification-utils.ts).ghost/core/core/server/services/verification-trigger.js (1)
232-232: PreferNumber.isFiniteover globalisFinite.The global
isFinitecoerces non-numeric values (e.g.,isFinite("10")returnstrue), whereasNumber.isFinitedoes not. SinceimportThresholdcould hypothetically receive a string from misconfiguration,Number.isFiniteis safer.♻️ Suggested fix
- if (isFinite(importThreshold) && currentImport > importThreshold) { + if (Number.isFinite(importThreshold) && currentImport > importThreshold) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/verification-trigger.js` at line 232, Replace the use of the global isFinite in the conditional that checks importThreshold (the if that currently references importThreshold and currentImport) with Number.isFinite to avoid coercion of non-numeric values; specifically update the conditional that reads the importThreshold check so it uses Number.isFinite(importThreshold) while leaving the currentImport > importThreshold comparison unchanged.
🤖 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/verification-trigger.js`:
- Line 232: Replace the use of the global isFinite in the conditional that
checks importThreshold (the if that currently references importThreshold and
currentImport) with Number.isFinite to avoid coercion of non-numeric values;
specifically update the conditional that reads the importThreshold check so it
uses Number.isFinite(importThreshold) while leaving the currentImport >
importThreshold comparison unchanged.
In `@ghost/core/test/utils/email-verification-utils.ts`:
- Line 110: Remove the unnecessary await on the synchronous nock.cleanAll() call
in the test helper; locate the line calling await nock.cleanAll() and change it
to a plain nock.cleanAll() so it no longer awaits a void return from the
nock.cleanAll() function.
- Around line 70-74: The nested ternary used to compute rawBody from requestBody
is hard to read; replace it with a small helper or explicit conditionals (e.g.,
a function like normalizeRequestBody(requestBody) or an if/else block) that
handles Buffer, string, and other types and returns the utf8 string or JSON
string; update the usage where rawBody is assigned so the logic is clearer and
unit-test behavior is unchanged (referencing the rawBody variable and the
requestBody input in email-verification-utils.ts).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b775c39-f2c0-44f3-a2b2-5ed5bfb8be1e
📒 Files selected for processing (4)
ghost/core/core/server/services/verification-trigger.jsghost/core/test/e2e-api/admin/members.test.jsghost/core/test/unit/server/services/verification-trigger.test.jsghost/core/test/utils/email-verification-utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/test/unit/server/services/verification-trigger.test.js
aileen
force-pushed
the
gva-732-verification-flow-tests
branch
from
70c4bc8 to
dad9c11
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test/utils/email-verification-utils.ts`:
- Around line 110-111: The call to nock.cleanAll() is synchronous so remove the
unnecessary await before nock.cleanAll() and leave the await on the async
configUtils.restore() call intact; update the cleanup block that currently calls
await nock.cleanAll(); await configUtils.restore(); to call nock.cleanAll();
await configUtils.restore(); so only the async restore uses await.
🪄 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: 78fee93b-f782-4cc9-a9a2-1f6e948aafc6
📒 Files selected for processing (4)
ghost/core/core/server/services/verification-trigger.jsghost/core/test/e2e-api/admin/members.test.jsghost/core/test/unit/server/services/verification-trigger.test.jsghost/core/test/utils/email-verification-utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/server/services/verification-trigger.js
aileen
force-pushed
the
gva-732-verification-flow-tests
branch
from
dad9c11 to
3d319fd
Compare
aileen
changed the title
aileen
force-pushed
the
gva-732-verification-flow-tests
branch
3 times, most recently
from
4b9cf63 to
2860908
Compare
| // paginated response data, which may be truncated by the default limit. | ||
| // Admin and API imports are usually only one member at a time, so this | ||
| // should be sufficient to prevent off-by-one errors without risking inaccurate | ||
| // counts from larger imports. |
There was a problem hiding this comment.
// TODO: Fix off-by-one issue in event dispatch: <issue id>
Might be a good comment, we're acknowledging that there's a problem here, we should defer the extra details to an issue
There was a problem hiding this comment.
More thinking: Feels like this could be an issue with bookshelf-pagination?
We're seeing the pagination data lag behind the event data, which implies a race condition on the model. I'm not very confident, but I'll do some spelunking into the Framework repo
There was a problem hiding this comment.
Yeah I think that this is the case:
2 things to look into:
- Pagination plugin should transact to make sure the count query and data queries are based on the same data
- If the data query returns less than the page limit then we should build the pagination data from the data query (i.e. meta.pagination.total = data.length)
sam-lord
left a comment
sam-lord
left a comment
There was a problem hiding this comment.
Woo hoo!!! Lets get it merged
| // paginated response data, which may be truncated by the default limit. | ||
| // Admin and API imports are usually only one member at a time, so this | ||
| // should be sufficient to prevent off-by-one errors without risking inaccurate | ||
| // counts from larger imports. |
There was a problem hiding this comment.
More thinking: Feels like this could be an issue with bookshelf-pagination?
We're seeing the pagination data lag behind the event data, which implies a race condition on the model. I'm not very confident, but I'll do some spelunking into the Framework repo
| // paginated response data, which may be truncated by the default limit. | ||
| // Admin and API imports are usually only one member at a time, so this | ||
| // should be sufficient to prevent off-by-one errors without risking inaccurate | ||
| // counts from larger imports. |
There was a problem hiding this comment.
Yeah I think that this is the case:
2 things to look into:
- Pagination plugin should transact to make sure the count query and data queries are based on the same data
- If the data query returns less than the page limit then we should build the pagination data from the data query (i.e. meta.pagination.total = data.length)
| * @param {number} [options.newsletterCount] - The number of newsletters to expect in the response | ||
| * @returns {Promise<object>} The created member | ||
| */ | ||
| const createMemberThroughApi = async (options) => { |
ref [#27013](#27013) This consolidates the email verification test updates ahead of the `verificationFlow` GA rollout, where the legacy email path will be removed in favor of webhook-based verification. It refactors the admin and unit tests around the new helpers, hardens `email-verification-utils.ts`, tightens the webhook assertions, and updates `VerificationTrigger` counting so `api` and `admin` signup events behave correctly once the new flow becomes the default.
aileen
force-pushed
the
gva-732-verification-flow-tests
branch
from
2860908 to
c70ece4
Compare
|
2 participants
Summary
verification-webhook-test-utils.tsto replace duplicated nock/config boilerplateverificationFlowflag enabledverification-trigger.test.jsunit test to use shared helpersref GVA-732
Test plan