Added ability to override settings via config #22088
Conversation
…errides to specific tests
- We need to return the entire object from getAll, not just the value. The override should only affect the value, not the entire object.
…esponses and to allow overriding a value to false
…an be passed directly to the disabled attribute
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
apps/admin-x-framework/src/api/settings.tsOops! Something went wrong! :( ESLint: 8.44.0 ESLint couldn't find the plugin "eslint-plugin-react-hooks". (The package "eslint-plugin-react-hooks" was not found when loaded as a Node module from the directory "/apps/admin-x-framework".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react-hooks" was referenced from the config file in "apps/admin-x-framework/.eslintrc.cjs". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. apps/admin-x-settings/test/acceptance/membership/analytics.test.tsOops! Something went wrong! :( ESLint: 8.44.0 ESLint couldn't find the plugin "eslint-plugin-react-hooks". (The package "eslint-plugin-react-hooks" was not found when loaded as a Node module from the directory "/apps/admin-x-settings".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react-hooks" was referenced from the config file in "apps/admin-x-settings/.eslintrc.cjs". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. apps/admin-x-framework/src/test/acceptance.tsOops! Something went wrong! :( ESLint: 8.44.0 ESLint couldn't find the plugin "eslint-plugin-react-hooks". (The package "eslint-plugin-react-hooks" was not found when loaded as a Node module from the directory "/apps/admin-x-framework".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react-hooks" was referenced from the config file in "apps/admin-x-framework/.eslintrc.cjs". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
WalkthroughThis pull request introduces an optional Changes
Sequence DiagramsequenceDiagram
participant UI as User Interface
participant Settings as Settings Service
participant Cache as Settings Cache
participant API as Settings API
UI->>Settings: Request setting
Settings->>Cache: Check cached settings
Cache-->>Settings: Return settings
Settings->>API: Fetch settings if needed
API-->>Settings: Return settings with overrides
Settings->>UI: Provide settings with read-only status
Possibly related PRs
Suggested Labels
✨ Finishing Touches
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 (
|
![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 (7)
apps/admin-x-framework/test/unit/utils/api/settings.test.tsx (1)
36-58: Add test case for setting without override property.The test suite for
isSettingReadOnlyis well structured but could be more comprehensive.Add this test case to handle settings without an override property:
describe('isSettingReadOnly', function () { + it('returns false if the setting has no override property', function () { + const settings = [ + {key: 'test_key', value: 'test_value'} + ]; + const value = isSettingReadOnly(settings, 'test_key'); + expect(value).toEqual(false); + }); // existing tests... });apps/admin-x-framework/src/api/settings.ts (1)
86-92: Enhance error handling in isSettingReadOnly.While the implementation is correct, it could be more explicit about handling missing settings.
Consider this enhanced implementation:
export function isSettingReadOnly(settings: Setting[] | null | undefined, key: string): boolean | undefined { if (!settings) { return undefined; } const setting = settings.find(d => d.key === key); - return setting?.override || false; + if (!setting) { + return false; // Explicitly handle missing settings + } + return setting.override || false; }apps/admin-x-settings/test/acceptance/membership/analytics.test.ts (1)
61-83: Enhance test coverage for read-only settings.The test verifies the disabled state but should also verify that the setting remains unchanged after attempting to save.
Enhance the test with save verification:
test('Supports read only settings', async ({page}) => { + const {lastApiRequests} = await mockApi({page, requests: { ...globalDataRequests, browseSettings: {method: 'GET', path: /^\/settings\/\?group=/, response: updatedSettingsResponse([ {key: 'members_track_sources', value: false}, {key: 'email_track_opens', value: false}, {key: 'email_track_clicks', value: false, override: true}, {key: 'outbound_link_tagging', value: false} - ])} + ])}, + editSettings: {method: 'PUT', path: '/settings/', response: updatedSettingsResponse([ + {key: 'email_track_clicks', value: false, override: true} + ])} }}); await page.goto('/'); const section = page.getByTestId('analytics'); await expect(section).toBeVisible(); const newsletterClicksToggle = await section.getByLabel(/Newsletter clicks/); await expect(newsletterClicksToggle).not.toBeChecked(); await expect(newsletterClicksToggle).toBeDisabled(); + // Verify save doesn't change overridden setting + await section.getByRole('button', {name: 'Save'}).click(); + expect(lastApiRequests.editSettings?.body.settings).not.toContainEqual({ + key: 'email_track_clicks', + value: true + }); });apps/admin-x-settings/src/components/settings/membership/Analytics.tsx (1)
5-5: LGTM! Consider adding a tooltip for disabled state.The implementation correctly handles the read-only state for email tracking settings. However, users might benefit from understanding why the toggle is disabled.
Consider adding a tooltip to explain why the toggle is disabled when
isEmailTrackClicksReadOnlyis true:<Toggle checked={trackEmailClicks} direction='rtl' disabled={isEmailTrackClicksReadOnly} + tooltip={isEmailTrackClicksReadOnly ? "This setting has been disabled by your administrator" : undefined} gap='gap-0' label='Newsletter clicks' labelClasses='py-4 w-full' onChange={(e) => { handleToggleChange('email_track_clicks', e); }} />Also applies to: 23-23, 68-68
ghost/core/test/unit/shared/settings-cache.test.js (1)
13-20: LGTM! Consider adding one more test case.The test coverage is comprehensive, testing both the basic functionality and edge cases of settings overrides.
Consider adding a test case to verify that overrides work correctly when the setting type is not boolean:
it('.get() handles non-boolean overrides', function () { cache = createCacheManager({ members_support_address: '[email protected]' }); cache.set('members_support_address', {value: '[email protected]'}); should(cache.get('members_support_address')).eql('[email protected]'); });Also applies to: 101-112, 120-139
ghost/core/core/shared/settings-cache/CacheManager.js (2)
22-22: Add JSDoc for the settingsOverrides property.Document the purpose and type of the settingsOverrides property.
+ /** @prop {Object} settingsOverrides - key/value pairs of settings which are overridden via config */ this.settingsOverrides;
57-70: Optimize the override check.The current implementation using
Object.keys().includes()is less efficient than directly checking the property.- 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 - override = {value: this.settingsOverrides[key]}; + override = {value: this.settingsOverrides[key]}; }
📜 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 (12)
apps/admin-x-framework/src/api/settings.ts(2 hunks)apps/admin-x-framework/src/test/acceptance.ts(1 hunks)apps/admin-x-framework/test/unit/utils/api/settings.test.tsx(1 hunks)apps/admin-x-settings/src/components/settings/membership/Analytics.tsx(3 hunks)apps/admin-x-settings/test/acceptance/membership/analytics.test.ts(1 hunks)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)package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Browser tests
- GitHub Check: Comments-UI tests
- GitHub Check: Ghost-CLI tests
- GitHub Check: Admin-X Settings tests
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 18.12.1, mysql8)
- GitHub Check: Admin tests - Chrome
🔇 Additional comments (9)
ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/settings.js (1)
6-6: LGTM! Verify override property usage.The addition of 'override' to the picked properties is correct and aligns with the PR objective.
Let's verify that the override property is being used correctly throughout the codebase:
✅ Verification successful
✓ Override property implementation verified
The override property is correctly integrated across the system:
- Set in the settings cache manager
- Exposed through the API mapper
- Consumed by the admin framework to determine setting override status
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for override property usage in settings-related files rg -g '!*.{test,spec}.*' -g '!*test*' 'override.*setting'Length of output: 303
Script:
#!/bin/bash # Search for settings mapper usage and override property echo "=== Settings Mapper Usage ===" rg -g '!*.{test,spec}.*' -g '!*test*' "require.*mappers/settings'\)" echo -e "\n=== Override Property in Settings ===" rg -g '!*.{test,spec}.*' -g '!*test*' "setting.*override|override.*setting" -A 2Length of output: 1593
ghost/core/core/server/services/settings/settings-service.js (1)
8-8: LGTM! Clean implementation of settings override.The implementation correctly retrieves and passes settings overrides from the configuration, with proper fallback to an empty object.
Also applies to: 75-76
ghost/core/core/shared/settings-cache/CacheManager.js (1)
188-193: LGTM! Clean implementation of settings override initialization.The initialization of settingsOverrides is properly implemented with clear parameter documentation.
apps/admin-x-framework/src/test/acceptance.ts (2)
214-214: LGTM: Function signature updated to support settings overrides.The parameter type now includes an optional
overrideproperty, aligning with the new settings override functionality.
220-224: Improved value handling and override support.The changes improve the implementation by:
- Using
undefinedcheck instead of truthy check for better handling of falsy values- Conditionally including the override property only when it's true
ghost/core/test/e2e-api/admin/settings.test.js (2)
663-671: LGTM: Well-structured test setup for settings overrides.The test suite properly sets up the environment by configuring the settings override in the beforeEach hook.
672-684: Comprehensive test coverage for settings overrides.The test verifies that the API correctly respects settings overrides by:
- Making a GET request to fetch settings
- Validating the response status and body
- Matching against snapshots for regression testing
ghost/core/test/e2e-api/admin/members.test.js (1)
971-971: Updated setting structure to match new format.The setting is now correctly set using the new object structure with a
valueproperty.package.json (1)
39-39: Added useful Docker shell access script.The new
docker:shellscript provides a convenient way to access an interactive shell within the Docker container, which is valuable for debugging and development.
…y instead of override
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ghost/core/core/shared/settings-cache/CacheManager.js (1)
57-70: Consider adding type validation for override values.While the override handling is implemented correctly, there's no validation of the override values against the setting's expected type. This could lead to type mismatches.
if (this.settingsOverrides && Object.keys(this.settingsOverrides).includes(key)) { + const setting = this.settingsCache.get(key); + if (setting && setting.type) { + const overrideValue = this.settingsOverrides[key]; + if (typeof overrideValue !== setting.type) { + debug(`Type mismatch for override ${key}: expected ${setting.type}, got ${typeof overrideValue}`); + } + } override = {value: this.settingsOverrides[key]}; }ghost/core/test/unit/shared/settings-cache.test.js (1)
101-112: Add test cases for type validation.Consider adding test cases to verify that type mismatches in overrides are handled appropriately.
it('.get() validates override types', function () { cache = createCacheManager({ string_setting: 123 // number instead of string }); cache.set('string_setting', {value: 'test', type: 'string'}); should(cache.get('string_setting')).eql(123); // Verify that a warning was logged });
📜 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 (7)
apps/admin-x-framework/src/api/settings.ts(2 hunks)apps/admin-x-framework/src/test/acceptance.ts(1 hunks)apps/admin-x-framework/test/unit/utils/api/settings.test.tsx(1 hunks)apps/admin-x-settings/test/acceptance/membership/analytics.test.ts(1 hunks)ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/settings.js(1 hunks)ghost/core/core/shared/settings-cache/CacheManager.js(4 hunks)ghost/core/test/unit/shared/settings-cache.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/settings.js
- apps/admin-x-framework/src/api/settings.ts
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Browser tests
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Signup-form tests
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Lint
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Database tests (Node 18.12.1, mysql8)
- GitHub Check: Admin tests - Chrome
- GitHub Check: Admin-X Settings tests
- GitHub Check: Comments-UI tests
🔇 Additional comments (7)
ghost/core/core/shared/settings-cache/CacheManager.js (2)
22-22: LGTM: Added settingsOverrides property.The property is correctly initialized at the class level.
188-193: LGTM: Updated init method with settingsOverrides parameter.The initialization is handled correctly, with the override parameter being optional.
ghost/core/test/unit/shared/settings-cache.test.js (2)
13-20: LGTM: Well-structured helper function.The
createCacheManagerhelper improves test readability and reduces code duplication.
120-139: LGTM: Comprehensive test for getAll with overrides.The test properly verifies that overrides are respected in the full settings object.
apps/admin-x-framework/test/unit/utils/api/settings.test.tsx (1)
1-59: LGTM: Well-structured test suite with good coverage.The tests thoroughly cover the settings utility functions, including edge cases and error conditions.
apps/admin-x-settings/test/acceptance/membership/analytics.test.ts (1)
61-83: LGTM: Comprehensive acceptance test for read-only settings.The test properly verifies both the disabled state and the value of the overridden setting.
apps/admin-x-framework/src/test/acceptance.ts (1)
214-224: LGTM: Clean implementation of settings response with read-only support.The function correctly handles both the value and read-only status of settings while preserving existing behavior.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #22088 +/- ##
==========================================
+ Coverage 74.01% 74.03% +0.02%
==========================================
Files 1272 1272
Lines 75840 76025 +185
Branches 10102 10135 +33
==========================================
+ Hits 56130 56284 +154
- Misses 18766 18792 +26
- Partials 944 949 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Deployed to staging with ID: |
|
Tested and working on staging — going to break this PR into one for backend and one for frontend now. |
ref https://linear.app/ghost/issue/ENG-1974/create-config-option-to-forcibly-disable-email-track-clicks