Skip to content

Comments

Added validation for outbox statuses#26545

Merged
troyciesco merged 1 commit intomainfrom
NY-809_outbox-auto-email-status-validation
Feb 24, 2026
Merged

Added validation for outbox statuses#26545
troyciesco merged 1 commit intomainfrom
NY-809_outbox-auto-email-status-validation

Conversation

@troyciesco
Copy link
Contributor

closes NY-809

  • No user-facing changes
  • Adds schema validation to the outbox.status field
  • While developing outbox functionality we deliberately left this validation off so that we could develop more easily. Now that we have a live use case (welcome emails) it makes sense to formalize the valid values for this field

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 23, 2026

Walkthrough

The pull request updates the outbox.status field in the database schema to include validation constraints. The status field, which previously accepted any string value up to 50 characters with a default of pending, now enforces that the value must be one of four specific states: pending, processing, failed, or completed. This is implemented by adding a validations property with an isIn constraint to the field definition. Corresponding unit tests are added to verify that attempting to create an Outbox entry with an invalid status value results in a ValidationError being thrown.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Added validation for outbox statuses' directly and clearly summarizes the main change—adding schema validation constraints to the outbox status field.
Description check ✅ Passed The description is directly related to the changeset, explaining the purpose of the validation addition, the reasoning for adding it now, and clarifying there are no user-facing changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch NY-809_outbox-auto-email-status-validation

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@github-actions github-actions bot added the migration [pull request] Includes migration for review label Feb 23, 2026
@troyciesco troyciesco force-pushed the NY-809_outbox-auto-email-status-validation branch from e71eba1 to 8eaf1c8 Compare February 23, 2026 19:33
@troyciesco troyciesco force-pushed the NY-809_outbox-auto-email-status-validation branch from 8eaf1c8 to 82fda97 Compare February 23, 2026 20:13
@troyciesco troyciesco removed the migration [pull request] Includes migration for review label Feb 23, 2026
@TryGhost TryGhost deleted a comment from github-actions bot Feb 23, 2026
@troyciesco troyciesco marked this pull request as ready for review February 23, 2026 21:42
@troyciesco troyciesco requested a review from cmraible February 23, 2026 21:42
Copy link
Contributor Author

@troyciesco troyciesco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmraible i think an isIn change doesn't require a migration but shout if i'm wrong!

assert(Array.isArray(err));
assert.equal(err.length, 1);
assert.equal((err[0] instanceof errors.ValidationError), true);
assert.match(err[0].context, /outbox\.status/);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this pattern is how it's done elsewhere, like member-created-event.test.js

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
ghost/core/test/unit/server/models/outbox.test.js (1)

47-62: Consider async/await to surface the sentinel message on unexpected success.

The .catch() at line 56 sits in the same chain as the .then() sentinel at line 53-55. If Outbox.add() unexpectedly resolves, .then() throws the sentinel error and .catch() immediately catches it — the test still fails (because Array.isArray(new Error(...)) is false), but the diagnostic message will be an opaque AssertionError rather than "expected ValidationError".

♻️ Cleaner async/await equivalent
-    it('rejects invalid status values', function () {
-        return models.Outbox.add({
-            event_type: 'MemberCreatedEvent',
-            payload: '{}',
-            status: 'not-a-real-status'
-        })
-            .then(function () {
-                throw new Error('expected ValidationError');
-            })
-            .catch(function (err) {
-                assert(Array.isArray(err));
-                assert.equal(err.length, 1);
-                assert.equal((err[0] instanceof errors.ValidationError), true);
-                assert.match(err[0].context, /outbox\.status/);
-            });
+    it('rejects invalid status values', async function () {
+        let caught;
+        try {
+            await models.Outbox.add({
+                event_type: 'MemberCreatedEvent',
+                payload: '{}',
+                status: 'not-a-real-status'
+            });
+            assert.fail('expected ValidationError');
+        } catch (err) {
+            caught = err;
+        }
+        assert(Array.isArray(caught));
+        assert.equal(caught.length, 1);
+        assert.ok(caught[0] instanceof errors.ValidationError);
+        assert.match(caught[0].context, /outbox\.status/);
+    });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ghost/core/test/unit/server/models/outbox.test.js` around lines 47 - 62, The
promise-chain test for models.Outbox.add currently throws a sentinel Error
inside .then() which is then caught by the following .catch(), hiding the
intended message; rewrite the test to use async/await (make the it callback
async), await models.Outbox.add({ ... }), and wrap that await in a try/catch so
that on unexpected resolution you throw new Error('expected ValidationError')
outside the catch that asserts the ValidationError shape, or alternatively await
and assert failure with a direct throw if no error was thrown—update references
to models.Outbox.add and the test's .then/.catch flow accordingly.
🤖 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/unit/server/models/outbox.test.js`:
- Around line 47-62: The promise-chain test for models.Outbox.add currently
throws a sentinel Error inside .then() which is then caught by the following
.catch(), hiding the intended message; rewrite the test to use async/await (make
the it callback async), await models.Outbox.add({ ... }), and wrap that await in
a try/catch so that on unexpected resolution you throw new Error('expected
ValidationError') outside the catch that asserts the ValidationError shape, or
alternatively await and assert failure with a direct throw if no error was
thrown—update references to models.Outbox.add and the test's .then/.catch flow
accordingly.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fe8aa7e and 82fda97.

📒 Files selected for processing (2)
  • ghost/core/core/server/data/schema/schema.js
  • ghost/core/test/unit/server/models/outbox.test.js

Copy link
Collaborator

@cmraible cmraible left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@troyciesco troyciesco merged commit 1dc579a into main Feb 24, 2026
33 checks passed
@troyciesco troyciesco deleted the NY-809_outbox-auto-email-status-validation branch February 24, 2026 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants