-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
Added new setting in the database: blocked_email_domains [migration] #22046
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis pull request introduces a new setting called Changes
Possibly related PRs
Suggested labels
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
It looks like this PR contains a migration 👀 General requirements
Schema changes
Data changes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ghost/core/core/server/data/migrations/versions/5.108/2025-01-23-02-51-10-add-blocked-email-domains-setting.js (1)
1-8
: LGTM! Implementation looks correct.The migration properly sets up the new setting with appropriate type and grouping.
Add a comment explaining the migration's purpose.
Consider adding a brief comment explaining that this migration adds support for blocking specific email domains during member signups.
const {addSetting} = require('../../utils'); +// Add setting to support blocking specific email domains during member signups module.exports = addSetting({ key: 'blocked_email_domains', value: '[]', type: 'array', group: 'members' });
ghost/core/core/server/api/endpoints/utils/serializers/input/settings.js (1)
77-78
: Consider grouping with other member-related settings.For better organization, consider moving
blocked_email_domains
near other member-related settings likemembers_signup_access
andmembers_support_address
.'donations_suggested_amount', 'recommendations_enabled', 'body_font', 'heading_font', - 'blocked_email_domains' + // ... other settings ... + 'members_signup_access', + 'members_support_address', + 'blocked_email_domains', + // ... remaining settings ...ghost/core/test/unit/server/data/exporter/index.test.js (1)
239-239
: Add a comment explaining the allowedKeysLength value.Consider adding a comment to document that the value was incremented due to the addition of the
blocked_email_domains
setting.- const allowedKeysLength = 89; + // Incremented to 89 after adding 'blocked_email_domains' setting + const allowedKeysLength = 89;ghost/core/core/server/data/schema/default-settings/default-settings.json (1)
319-321
: Implementation looks good, but consider adding validations.The new
blocked_email_domains
setting is correctly placed in the members section with appropriate type and default value.Consider adding:
- Validation rules for domain format (e.g., regex pattern for valid domain names)
- Maximum array size limit to prevent performance issues
- Documentation comments about the expected format of domain entries (e.g., "example.com" vs "@example.com")
Example validation structure:
"blocked_email_domains": { "defaultValue": "[]", - "type": "array" + "type": "array", + "validations": { + "maxLength": 1000, + "validateArrayItems": { + "matches": "^[a-zA-Z0-9][a-zA-Z0-9-]{1,61}[a-zA-Z0-9]\\.[a-zA-Z]{2,}$" + } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ghost/core/core/server/api/endpoints/utils/serializers/input/settings.js
(1 hunks)ghost/core/core/server/data/migrations/versions/5.108/2025-01-23-02-51-10-add-blocked-email-domains-setting.js
(1 hunks)ghost/core/core/server/data/schema/default-settings/default-settings.json
(1 hunks)ghost/core/test/unit/server/data/exporter/index.test.js
(1 hunks)ghost/core/test/unit/server/data/schema/integrity.test.js
(1 hunks)
🔇 Additional comments (2)
ghost/core/test/unit/server/data/schema/integrity.test.js (1)
40-40
: LGTM! Hash update is correct.The settings hash has been properly updated to reflect the addition of the new
blocked_email_domains
setting.ghost/core/test/unit/server/data/exporter/index.test.js (1)
Line range hint
1-8
: Verify integration with member signup flow.While the database changes look good, please ensure:
- The member signup flow validates against blocked domains
- UI exists for publishers to manage blocked domains
- Domain entries are properly validated (format, duplicates, etc.)
Let's verify the integration points:
Deployed to staging with ID: |
Deployed to staging with ID: |
ref https://linear.app/ghost/issue/ENG-1973
ref https://app.incident.io/ghost/incidents/132
blocked_email_domains
(array, default:[]
)