Added ability to override settings via config #22089
Conversation
|
Warning Rate limit exceeded@cmraible has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
WalkthroughThis pull request introduces a comprehensive enhancement to the settings handling mechanism in the Ghost CMS. The changes focus on implementing settings overrides, which allows for dynamically modifying settings values at runtime. The modifications span multiple files across the core server and test directories, introducing new functionality for managing settings with the ability to override default values while preserving their original configuration. Changes
Sequence DiagramsequenceDiagram
participant Config as Configuration
participant SettingsService as Settings Service
participant CacheManager as Cache Manager
participant API as Settings API
Config->>SettingsService: Provide settingsOverrides
SettingsService->>CacheManager: Initialize with overrides
CacheManager-->>CacheManager: Process settings with overrides
API->>CacheManager: Request settings
CacheManager-->>API: Return settings with overrides applied
Possibly related PRs
Suggested Labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
|
This PR was originally drafted as #22088, and includes only the backend changes. The frontend changes to admin-x-settings will be committed separately. |
![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)
ghost/core/core/shared/settings-cache/CacheManager.js (2)
21-22: Initialize settingsOverrides in constructor.The
settingsOverridesproperty is declared but not initialized. Consider initializing it to an empty object in the constructor to prevent potential undefined access.constructor({publicSettings}) { this.settingsCache; - this.settingsOverrides; + this.settingsOverrides = {}; this.publicSettings = publicSettings;
57-70: Optimize override check and add type validation.The current implementation has a few potential improvements:
Object.keys(this.settingsOverrides).includes(key)is less efficient than direct property access- No type validation is performed on override values
- if (this.settingsOverrides && Object.keys(this.settingsOverrides).includes(key)) { + if (this.settingsOverrides?.[key] !== undefined) { // Wrap the override value in an object in case it's a boolean + const overrideValue = this.settingsOverrides[key]; + if (cacheEntry && typeof overrideValue !== typeof cacheEntry.value) { + throw new Error(`Invalid override type for ${key}`); + } override = {value: this.settingsOverrides[key]}; }ghost/core/test/unit/shared/settings-cache.test.js (1)
101-108: Add edge case tests for settingsOverrides.Consider adding tests for:
- Multiple overrides
- Invalid override types
- Undefined/null override values
it('handles multiple settingsOverrides correctly', function () { cache = createCacheManager({ setting1: false, setting2: 'test' }); cache.set('setting1', {value: true}); cache.set('setting2', {value: 'original'}); should(cache.get('setting1')).eql(false); should(cache.get('setting2')).eql('test'); }); it('validates override types', function () { cache = createCacheManager({ setting1: 'invalid' }); cache.set('setting1', {value: true}); should(() => cache.get('setting1')).throw(/Invalid override type/); });ghost/core/test/e2e-api/admin/settings.test.js (1)
663-684: Enhance settings override e2e tests.Consider adding tests for:
- Attempting to modify an overridden setting via API
- Multiple overridden settings
- Override removal behavior
it('prevents modification of overridden settings via API', async function () { await agent.put('settings/') .body({ settings: [{ key: 'email_track_clicks', value: true }] }) .expectStatus(200) .expect(({body}) => { const setting = body.settings.find(s => s.key === 'email_track_clicks'); assert.equal(setting.value, false); assert.equal(setting.is_read_only, true); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/settings.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/settings.js(1 hunks)ghost/core/core/server/services/settings/settings-service.js(2 hunks)ghost/core/core/shared/settings-cache/CacheManager.js(4 hunks)ghost/core/test/e2e-api/admin/members.test.js(1 hunks)ghost/core/test/e2e-api/admin/settings.test.js(1 hunks)ghost/core/test/unit/shared/settings-cache.test.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Lint
- GitHub Check: Ghost-CLI tests
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: i18n
- GitHub Check: Database tests (Node 18.12.1, mysql8)
🔇 Additional comments (6)
ghost/core/core/shared/settings-cache/CacheManager.js (1)
157-159: LGTM: Efficient use of get method.Good choice to use the
getmethod ingetAllto ensure overrides are consistently applied.ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/settings.js (1)
6-6: LGTM: Clean addition of is_read_only property.The change correctly exposes the read-only status of settings to API consumers, which is essential for UI feedback.
ghost/core/test/unit/shared/settings-cache.test.js (1)
13-20: LGTM: Well-structured helper function.The
createCacheManagerhelper improves test readability and reduces duplication.ghost/core/test/e2e-api/admin/members.test.js (1)
971-971: Update setting value format to match new settings structure.The change from
settingsCache.set('email_verification_required', false)tosettingsCache.set('email_verification_required', {value: false})aligns with the new settings structure that supports overrides. The setting is now stored as an object with avalueproperty instead of a direct boolean value.ghost/core/core/server/services/settings/settings-service.js (2)
8-8: LGTM: Import config module for accessing settings overrides.The addition of the config import is necessary to access the
hostSettings:settingsOverridesconfiguration.
75-76: Initialize settings overrides from config.The changes enable the settings override functionality by:
- Retrieving overrides from
hostSettings:settingsOverridesconfig with a fallback to an empty object- Passing the overrides to
SettingsCache.initThis implementation aligns with the PR objective of allowing settings to be overridden via configuration.
ref https://linear.app/ghost/issue/ENG-1974/create-config-option-to-forcibly-disable-email-track-clicks
hostSettings:settingsOverrides, which accepts key/value pairs of settings keys -> values. The value passed in here will override whatever value is set for the associated setting key in the databaseis_read_only: trueproperty to any setting that is overridden, which is included in the /api/admin/settings endpoint. This value can be used by the frontend to disable the control to prevent a user from trying to change the value.get()andgetAll()methods.