Added automated emails design API endpoint#27065
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a new API controller for automated email design with 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cmraible
changed the title
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ghost/core/test/e2e-api/admin/automated-email-design.test.js (1)
90-144: Exercise both permission actions for each non-admin role.Lines 91-143 only assert GET for editor/author and PUT for contributor, but
ghost/core/core/server/api/endpoints/automated-email-design.js:39-55gatesreadandeditseparately. A permission or route miswire on contributor-read or editor/author-edit would still pass this suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/e2e-api/admin/automated-email-design.test.js` around lines 90 - 144, Tests currently only exercise one permission action per role (editor/author GET, contributor PUT) while the automated-email-design endpoint enforces separate 'read' and 'edit' permissions; update the tests so each non-admin role calls both agent.get('automated_emails/design') and agent.put('automated_emails/design') with the same expectation of 403 and the same snapshot assertions. Locate the three tests using loginAsEditor, loginAsAuthor, and loginAsContributor and add the missing counterpart request for each (editor/author should also call PUT; contributor should also call GET) reusing the existing .body, .expectStatus(403), .matchBodySnapshot({errors:[{id: anyErrorId}]}) and .matchHeaderSnapshot({...}) assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/core/core/server/api/endpoints/automated-email-design.js`:
- Around line 56-71: The query handler currently allows an incoming payload to
include an id which is passed directly into models.EmailDesignSetting.edit; to
prevent primary-key overwrite, explicitly reject or remove an id in the
singleton payload: in the async query(frame) function (near resolveDefaultDesign
and models.EmailDesignSetting.edit) check if data.id !== undefined and throw a
ValidationError (similar to the slug check) or delete data.id before calling
models.EmailDesignSetting.edit so the defaultDesign id is the only id used when
updating.
In `@ghost/core/test/e2e-api/admin/automated-email-design.test.js`:
- Around line 4-8: The shared matcher matchEmailDesignSetting requires
updated_at to be anyISODateTime but the seeded default row can have updated_at:
null on first read; change matchEmailDesignSetting so updated_at accepts either
anyISODateTime or null (e.g., updated_at: oneOf(anyISODateTime, null) or the
project's equivalent), and apply the same change to the other matchers in this
test file that assert updated_at (the other occurrences of the updated_at
matcher used for initial GET and subsequent edit assertions) so the initial read
passes while edits still validate a real timestamp.
---
Nitpick comments:
In `@ghost/core/test/e2e-api/admin/automated-email-design.test.js`:
- Around line 90-144: Tests currently only exercise one permission action per
role (editor/author GET, contributor PUT) while the automated-email-design
endpoint enforces separate 'read' and 'edit' permissions; update the tests so
each non-admin role calls both agent.get('automated_emails/design') and
agent.put('automated_emails/design') with the same expectation of 403 and the
same snapshot assertions. Locate the three tests using loginAsEditor,
loginAsAuthor, and loginAsContributor and add the missing counterpart request
for each (editor/author should also call PUT; contributor should also call GET)
reusing the existing .body, .expectStatus(403), .matchBodySnapshot({errors:[{id:
anyErrorId}]}) and .matchHeaderSnapshot({...}) assertions.
🪄 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: e9f66c82-a223-4c2f-8de7-01f4de16c744
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/automated-email-design.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
ghost/core/core/server/api/endpoints/automated-email-design.jsghost/core/core/server/api/endpoints/index.jsghost/core/core/server/models/automated-email.jsghost/core/core/server/services/member-welcome-emails/constants.jsghost/core/core/server/web/api/endpoints/admin/routes.jsghost/core/test/e2e-api/admin/automated-email-design.test.js
- Changed missing default design error from NotFoundError to InternalServerError since the row is seeded by migration and should always exist - Fixed slug guard to use 'in' operator, preventing null from bypassing validation
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ghost/core/test/e2e-api/admin/automated-email-design.test.js (2)
111-165: Consider covering full role × method permission matrix.Current checks validate denial per role, but not both verbs for each role. A table-driven test for
GETandPUTacross editor/author/contributor would tighten regression coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/e2e-api/admin/automated-email-design.test.js` around lines 111 - 165, Replace the three separate tests with a table-driven loop inside the "Permissions" describe that iterates roles (editor, author, contributor) and HTTP verbs (GET, PUT) to assert 403 for each role×method combination; for each row call the appropriate agent.loginAsEditor/loginAsAuthor/loginAsContributor, perform agent.get('automated_emails/design') or agent.put('automated_emails/design') (for PUT include a minimal body like {automated_email_design:[{background_color:'dark'}]}), and assert .expectStatus(403) with the same .matchBodySnapshot and .matchHeaderSnapshot expectations currently used so every role×verb permutation is covered.
1-1: Prefer Node built-in specifier forassert.Use
node:assert/strictat Line 1 for consistency with Node builtin imports.Proposed change
-const assert = require('assert/strict'); +const assert = require('node:assert/strict');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/e2e-api/admin/automated-email-design.test.js` at line 1, Replace the CommonJS require specifier for the assert module so it uses Node's built-in specifier; locate the top-level require('assert/strict') call and change it to use the Node built-in specifier (node:assert/strict) to ensure consistent builtin imports in the test file.
🤖 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/e2e-api/admin/automated-email-design.test.js`:
- Around line 111-165: Replace the three separate tests with a table-driven loop
inside the "Permissions" describe that iterates roles (editor, author,
contributor) and HTTP verbs (GET, PUT) to assert 403 for each role×method
combination; for each row call the appropriate
agent.loginAsEditor/loginAsAuthor/loginAsContributor, perform
agent.get('automated_emails/design') or agent.put('automated_emails/design')
(for PUT include a minimal body like
{automated_email_design:[{background_color:'dark'}]}), and assert
.expectStatus(403) with the same .matchBodySnapshot and .matchHeaderSnapshot
expectations currently used so every role×verb permutation is covered.
- Line 1: Replace the CommonJS require specifier for the assert module so it
uses Node's built-in specifier; locate the top-level require('assert/strict')
call and change it to use the Node built-in specifier (node:assert/strict) to
ensure consistent builtin imports in the test file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5dc6db89-1170-45f0-801c-53359fe43cd8
📒 Files selected for processing (2)
ghost/core/core/server/api/endpoints/automated-email-design.jsghost/core/test/e2e-api/admin/automated-email-design.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/server/api/endpoints/automated-email-design.js
troyciesco
left a comment
troyciesco
left a comment
There was a problem hiding this comment.
other than the one nit sonarcloud (and coderabbit i guess) had that looks worth fixing, lgtm!
| delete data.id; | ||
|
|
||
| // Reject slug changes — the slug is an immutable identifier | ||
| if ('slug' in data) { |
There was a problem hiding this comment.
note: i gave some thought to whether we should delete this too if it's included, instead of throwing an error. i think throwing the error is better, because 1) the form on the front end, even if it might send all the design settings every time, prob shouldn't send the slug because it doesn't need to and 2) if the slug is editable later (would it be? don't even know) hitting the error during development will be better instead of silently deleting. And this endpoint would be different by then anyway with 2+ rows in the db.
just mentioning in case you think of a more compelling reason to delete!
|
cmraible
deleted the
chris-ny-1138-add-automated-email-design-settings-api-endpoint
branch
2 participants
closes https://linear.app/ghost/issue/NY-1138/add-automated-email-design-settings-api-endpoint
Summary
GETandPUT /automated_emails/designfor reading and editing the shared automated email design settingsdefault-automated-email) rather than requiring clients to know the IDemail_design_settingpermissions (admin/owner only) and prevents slug modificationemail_design_settingsas an internal persistence detail instead of exposing a generic table-shaped APIDEFAULT_EMAIL_DESIGN_SETTING_SLUGconstant to the sharedconstants.jsmodule to avoid duplicationAdvantages over a more generic
email-design-settingsBREAD resourceemail_design_settingstable, it only cares about "editing design settings for welcome emails". Ultimately this means more flexibility in changing the schema in the future without breaking the APITest plan