Admin changes for super editors #22453
Conversation
WalkthroughThis pull request introduces a new user role, "Super Editor," and updates various components of the application to support more granular role checking. Changes include clarifications in the Possibly related PRs
Suggested labels
Suggested reviewers
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
ghost/admin/tests/acceptance/editor/super-editor-lexical-test.jsOops! Something went wrong! :( ESLint: 8.44.0 Error: Failed to load parser '@babel/eslint-parser' declared in 'ghost/admin/.eslintrc.js': Cannot find module '@babel/eslint-parser'
ghost/admin/tests/acceptance/tags-test.jsOops! Something went wrong! :( ESLint: 8.44.0 Error: Failed to load parser '@babel/eslint-parser' declared in 'ghost/admin/.eslintrc.js': Cannot find module '@babel/eslint-parser'
ghost/admin/tests/acceptance/settings-button-test.jsOops! Something went wrong! :( ESLint: 8.44.0 Error: Failed to load parser '@babel/eslint-parser' declared in 'ghost/admin/.eslintrc.js': Cannot find module '@babel/eslint-parser'
✨ 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 (
|
github-actions
bot
added
the
migration
|
It looks like this PR contains a migration 👀 General requirements
Schema changes
Data changes
|
![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/admin/tests/acceptance/members/import-test.js (1)
114-212: Consider reducing duplication between role test blocks.The test code for "Super Editor functions" largely duplicates the "Owner tests" section, with identical test cases and expectations. This suggests both roles have the same capabilities for member imports.
Consider refactoring to use a shared test function that can be called from both the Owner and Super Editor test blocks, passing in the role name as a parameter. This would reduce duplication and make future changes easier to maintain:
function testRoleWithMemberImportCapabilities(roleName) { describe(`${roleName} functions`, function () { beforeEach(async function () { this.server.loadFixtures('configs'); let role = this.server.create('role', {name: roleName}); this.server.create('user', {roles: [role]}); await authenticateSession(); }); it('can open and close import modal', async function () { // Test implementation... }); it('has working happy path for small import...', async function () { // Test implementation... }); // Other shared tests... }); } // Then call it with each role testRoleWithMemberImportCapabilities('Owner'); testRoleWithMemberImportCapabilities('Super Editor');ghost/admin/tests/acceptance/members-test.js (1)
350-462: Comprehensive testing for Super Editor member management.The test suite for Super Editor role thoroughly verifies all the critical member management operations, ensuring that users with this role can navigate, view, edit, and create members. This effectively validates that the new role has the appropriate permissions implemented throughout the application.
Consider refactoring the duplicated test code between Owner and Super Editor blocks using a shared function approach, similar to what was suggested for the import tests. This would reduce maintenance overhead and ensure consistent testing across roles with similar capabilities.
function testRoleWithMemberManagementCapabilities(roleName) { describe(`as ${roleName}`, function () { beforeEach(async function () { // Setup for the specific role }); it('it renders, can be navigated, can edit member', async function () { // Test implementation }); it('can create a new member', async function () { // Test implementation }); // Other shared tests... }); } // Then call with each role testRoleWithMemberManagementCapabilities('owner'); testRoleWithMemberManagementCapabilities('super editor');ghost/admin/tests/unit/models/user-test.js (2)
73-77: Remove duplicate assertion forisEitherEditorThere appears to be a duplicate assertion for the
isEitherEditorproperty at lines 73 and 77. Having the same test twice doesn't provide additional value and could cause confusion during maintenance.expect(model.get('isOwnerOnly')).to.not.be.ok; expect(model.get('isEitherEditor')).to.not.be.ok; expect(model.get('canManageMembers')).to.not.be.ok; expect(model.get('isSuperEditor')).to.not.be.ok; expect(model.get('canManageComments')).to.not.be.ok; - expect(model.get('isEitherEditor')).to.not.be.ok;
93-96: Remove duplicate assertion forisEitherEditorSimilar to the Contributor test case, there's a duplicate assertion for the
isEitherEditorproperty at lines 93 and 95. This redundancy should be removed to maintain clean and maintainable tests.expect(model.get('isOwnerOnly')).to.not.be.ok; expect(model.get('isEitherEditor')).to.not.be.ok; expect(model.get('isSuperEditor')).to.not.be.ok; - expect(model.get('isEitherEditor')).to.not.be.ok; expect(model.get('canManageMembers')).to.not.be.ok; expect(model.get('canManageComments')).to.not.be.ok;
📜 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__/roles.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (38)
apps/admin-x-framework/src/api/roles.ts(1 hunks)apps/admin-x-settings/src/components/settings/general/InviteUserModal.tsx(3 hunks)apps/admin-x-settings/src/components/settings/general/users/RoleSelector.tsx(2 hunks)apps/admin-x-settings/src/hooks/useStaffUsers.tsx(2 hunks)ghost/admin/app/components/gh-nav-menu/main.hbs(1 hunks)ghost/admin/app/helpers/gh-user-can-manage-members.js(1 hunks)ghost/admin/app/helpers/members-count-fetcher.js(1 hunks)ghost/admin/app/models/user.js(1 hunks)ghost/admin/app/routes/member.js(1 hunks)ghost/admin/app/routes/members-activity.js(1 hunks)ghost/admin/app/routes/members-management.js(1 hunks)ghost/admin/app/routes/members.js(1 hunks)ghost/admin/app/routes/members/import.js(1 hunks)ghost/admin/app/templates/member.hbs(1 hunks)ghost/admin/mirage/config/members.js(1 hunks)ghost/admin/mirage/config/tiers.js(1 hunks)ghost/admin/mirage/fixtures/roles.js(1 hunks)ghost/admin/tests/acceptance/dashboard-test.js(1 hunks)ghost/admin/tests/acceptance/editor-test.js(1 hunks)ghost/admin/tests/acceptance/members-activity-test.js(2 hunks)ghost/admin/tests/acceptance/members-test.js(3 hunks)ghost/admin/tests/acceptance/members/import-test.js(1 hunks)ghost/admin/tests/unit/helpers/gh-user-can-manage-members-test.js(1 hunks)ghost/admin/tests/unit/models/user-test.js(5 hunks)ghost/core/core/server/data/migrations/versions/5.112/2025-03-07-12-24-00-add-super-editor.js(1 hunks)ghost/core/core/server/data/migrations/versions/5.112/2025-03-07-12-25-00-add-member-perms-to-super-editor.js(1 hunks)ghost/core/core/server/data/schema/fixtures/fixtures.json(2 hunks)ghost/core/core/server/models/role.js(1 hunks)ghost/core/core/server/models/user.js(1 hunks)ghost/core/test/e2e-api/admin/roles.test.js(2 hunks)ghost/core/test/integration/migrations/migration.test.js(6 hunks)ghost/core/test/regression/models/model_users.test.js(1 hunks)ghost/core/test/unit/server/data/schema/fixtures/fixture-manager.test.js(2 hunks)ghost/core/test/unit/server/data/schema/integrity.test.js(1 hunks)ghost/core/test/utils/fixture-utils.js(1 hunks)ghost/core/test/utils/fixtures/context.js(1 hunks)ghost/core/test/utils/fixtures/data-generator.js(4 hunks)ghost/core/test/utils/fixtures/fixtures.json(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 22.13.1, mysql8)
- GitHub Check: Regression 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: Unit tests (Node 22.13.1)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Admin tests - Chrome
🔇 Additional comments (60)
ghost/core/test/unit/server/data/schema/integrity.test.js (1)
39-39: Hash update correctly reflects fixture changes.The update to the fixtures hash value is appropriate given the introduction of the "Super Editor" role to the system. This change ensures that the integrity test correctly validates the updated fixtures that now include the new role definition.
ghost/core/test/utils/fixtures/context.js (1)
11-11: The 'super_editor' context is correctly added.The new 'super_editor' context entry is properly integrated with the existing user role contexts, following the established pattern in the file. This addition supports the new Super Editor role being introduced in this PR, enabling test cases to properly simulate this user role's context.
ghost/core/test/regression/models/model_users.test.js (1)
208-208: Test expectation updated correctly to include new Super Editor role.The expected user count has been updated from 9 to 10, accurately reflecting the addition of a new Super Editor user in the test fixtures. This ensures that tests using the 'findPage' method with 'all' limit will properly account for all users including the new role.
apps/admin-x-framework/src/api/roles.ts (1)
3-3: Type definition correctly extended to include 'Super Editor' role.The
UserRoleTypehas been properly updated to include the new 'Super Editor' role alongside existing roles. This type definition change is essential as it ensures type safety throughout the application when working with this new role.ghost/admin/app/routes/members-activity.js (1)
1-4: Route inheritance changed to use more specific management route.The
MembersActivityRoutenow extendsMembersManagementRouteinstead of a more generic route class. This change aligns with the introduction of the Super Editor role, likely providing more specialized access control for member-related functionality.ghost/core/core/server/models/user.js (1)
15-15: New 'Super Editor' role added to assignable rolesThe addition of 'Super Editor' to the ASSIGNABLE_ROLES constant allows this new role to be assigned to users through the system. This change enables the 'Super Editor' role functionality across the application, since this constant is used throughout the codebase to validate role assignments.
ghost/admin/mirage/fixtures/roles.js (1)
47-55: 'Super Editor' role added to role fixturesThe new 'Super Editor' role fixture with ID 6 follows the consistent pattern of other roles in the test fixtures. This ensures proper testing and development environment setup for the new role.
ghost/admin/app/components/gh-nav-menu/main.hbs (1)
135-135: Updated permission check for Members menu visibilityThe condition for displaying the Members navigation item has been changed from checking admin privileges to checking member management capabilities. This allows the new 'Super Editor' role to access the Members section without requiring full admin privileges.
ghost/admin/app/templates/member.hbs (1)
41-41: Updated permission check for member actions dropdownThe condition for displaying member action controls (impersonate, logout, delete) has been changed from checking if user is admin to checking if they can manage members. This grants 'Super Editor' users the ability to perform these member management actions.
ghost/admin/app/routes/members-management.js (1)
1-19: Well-structured route with proper authorization checkThe new
MembersManagementRouteclass extendsAuthenticatedRouteand implements necessary permission validation in thebeforeModelhook. It correctly checks both user session validity and the specific permission to manage members before allowing access.I notice there are commented-out sections related to membership settings which might be implemented in the future. The comments explain the rationale for not implementing this check yet, which is helpful for future developers.
ghost/admin/mirage/config/tiers.js (1)
11-12: Adding Super Editor role to allowed read rolesThe addition of the 'Super Editor' role to the
ALLOWED_READ_ROLESarray correctly extends tier reading permissions to this new role. This aligns with the PR objective of adding admin changes for super editors.ghost/core/test/utils/fixture-utils.js (1)
378-379: Adding Super Editor role to permissions fixturesThe addition of the 'Super Editor' role to the
rolesobject in thepermissionsForfunction correctly configures permissions for testing. This enables test cases to properly validate functionality for users with the 'Super Editor' role.ghost/admin/app/routes/member.js (1)
3-7: Updated route inheritance for permission handlingThe route now extends
MembersManagementRouteinstead ofAdminRoute, which properly delegates authorization checks to the specialized route class. This change maintains the existing functionality while adding the necessary permission checks for the Super Editor role.apps/admin-x-settings/src/components/settings/general/users/RoleSelector.tsx (3)
4-4: Good addition of the global data providerThe new import and usage of
useGlobalDataprovides access to the application configuration, which is necessary for implementing the editor beta feature toggle.Also applies to: 8-9
22-57: Well-structured implementation of the Super Editor roleThe code creates a clear separation between standard roles and the beta Super Editor role. The implementation:
- First defines all standard roles
- Then conditionally replaces the editor role with the Super Editor role when the beta flag is enabled
- Includes improved descriptions that clearly communicate the extended capabilities
This approach makes it easy to toggle the feature and maintains clean code organization.
62-62: Properly updated Radio component to use dynamic optionsThe change from hardcoded options to the dynamic
optionsArrayis correctly implemented, allowing the component to adapt based on configuration.ghost/admin/app/helpers/members-count-fetcher.js (1)
37-39: Good permission update to support Super Editor roleThe changes properly broaden access to the members count by:
- Updating the comment to clarify that "roles with permissions to manage members" should fetch the count
- Replacing the specific
isAdmincheck with the more capability-focusedcanManageMemberscheckThis change correctly implements the principle of checking for capabilities rather than specific roles, which is more maintainable and flexible.
ghost/core/test/unit/server/data/schema/fixtures/fixture-manager.test.js (1)
194-194: Tests updated correctly for new fixtures countThe test has been properly updated to accommodate the new Super Editor role:
FIXTURE_COUNTincreased from 112 to 133, reflecting additional permission fixtures- Expected call count for
dataMethodStub.findincreased from 9 to 10These changes ensure the tests match the updated fixture data that now includes permissions for the Super Editor role.
Also applies to: 204-204
ghost/core/test/e2e-api/admin/roles.test.js (1)
24-24: Tests correctly updated for additional roleThe API tests have been properly updated to expect:
- 11 total roles instead of 10 (line 24)
- 5 assignable roles instead of 4 (line 36)
These changes ensure that the tests will pass with the addition of the new Super Editor role.
Also applies to: 36-36
ghost/core/core/server/data/migrations/versions/5.112/2025-03-07-12-25-00-add-member-perms-to-super-editor.js (1)
1-291: Well-structured migration for adding Super Editor permissionsThis migration thoroughly adds all the necessary permissions to the Super Editor role. The use of
combineTransactionalMigrationsensures that all permission additions are executed within a single transaction, which is good for database integrity.The comprehensive set of permissions covers all key areas that a Super Editor would need to manage: members, content, tags, settings, and more. This appears to properly implement the requirements for the new role type.
ghost/admin/mirage/config/members.js (1)
12-16: LGTM: Super Editor role added to allowed rolesThe Super Editor role is correctly added to the ALLOWED_ROLES array, which grants access to the member-related API routes in the mirage mock server. This change is consistent with the overall PR objective of adding member management capabilities to the Super Editor role.
ghost/core/core/server/data/migrations/versions/5.112/2025-03-07-12-24-00-add-super-editor.js (1)
1-31: Well-implemented migration for creating the Super Editor roleThis migration creates the new Super Editor role with an appropriate description ("Editor plus member management"). The code handles edge cases well - it checks if the role already exists before attempting to create it and includes appropriate logging. The down migration properly cleans up by removing the role.
The use of
createTransactionalMigrationensures atomicity of the operation, which is good practice for database migrations.apps/admin-x-settings/src/hooks/useStaffUsers.tsx (2)
30-36: Good addition of multi-role filtering utilityThis new
getUsersByRolesfunction extends the existing functionality to filter users by multiple roles instead of just one. The implementation is clean and follows the same pattern as the existinggetUsersByRolefunction.
50-50: Correctly updated to include Super Editor in editor users groupThe modification to use
getUsersByRoleswith both 'Editor' and 'Super Editor' ensures that users with either role will be included in theeditorUsersresult. This is consistent with the PR's objective of treating Super Editors as an enhanced type of editor.ghost/admin/app/helpers/gh-user-can-manage-members.js (1)
1-12: Well-structured helper implementationThis is a well-structured Ember helper that encapsulates the permission check for managing members. The implementation follows good practices with clear documentation comments and a focused purpose.
ghost/admin/tests/acceptance/editor-test.js (1)
71-80: Good test coverage for Super Editor roleThis test correctly validates that users with the "Super Editor" role aren't redirected from the editor page, following the same pattern as the tests for other roles. This ensures the new role has appropriate editor access permissions.
ghost/admin/app/routes/members/import.js (1)
1-3: Appropriate route inheritance updateThis change correctly updates the import path and parent class from
AdminRoutetoMembersManagementRoute, which is part of restructuring the routing for members management to accommodate the new "Super Editor" role permissions.apps/admin-x-settings/src/components/settings/general/InviteUserModal.tsx (5)
10-11: Added GlobalDataProvider importGood addition of the
useGlobalDatahook to access the configuration for feature flags.
14-14: Updated RoleType definitionThe RoleType has been correctly extended to include the 'super editor' role type.
25-26: Lab feature flag implementationWell-structured implementation that uses a feature flag to control the availability of the Super Editor role.
178-185: Conditional role option replacementThis implementation properly handles the UI presentation of the Super Editor role. The conditional logic replaces the standard Editor option with "Editor (beta mode)" when the feature is enabled, including an appropriate hint that explains the extended capabilities.
186-190: Updated role filtering logicThe filtering logic has been updated to correctly map between the UI "Editor (beta mode)" label and the backend "Super Editor" role name. This ensures proper role assignment in the invitation process.
ghost/admin/app/routes/members.js (1)
1-5: Clean route inheritance refactoringThe route now extends
MembersManagementRouteinstead of the previousAdminRoutebase class, which is likely part of a broader restructuring to accommodate the new Super Editor role permissions.ghost/core/core/server/models/role.js (2)
85-85: Role permission hierarchy modification for ownersThe "Super Editor" role has been added to the permission hierarchy, allowing owners to assign this new role.
87-87: Role permission hierarchy modification for administratorsThe "Super Editor" role has been added to the permission hierarchy, allowing administrators to assign this new role.
ghost/admin/tests/acceptance/members-activity-test.js (2)
20-20: Test description updated to include Super Editor exclusionThe test description has been updated to clarify that non-admins and users without the Super Editor role are redirected.
44-56: Added test coverage for Super Editor roleNew test section added to verify that users with the Super Editor role can access the members activity page, following the same pattern as the existing admin and owner tests.
ghost/core/test/utils/fixtures/data-generator.js (5)
204-211: Added Super Editor user to test fixturesA new user with the Super Editor role has been added to the test data, maintaining consistency with existing user patterns.
308-312: Added Super Editor role definition to test fixturesThe Super Editor role has been defined in the test data generator, following the same structure as other roles.
1449-1450: Updated roles array referencesThe roles array references have been updated to include the new Super Editor role.
1458-1459: Updated users array referencesThe users array references have been updated to include the new Super Editor user.
1498-1504: Added role-user mapping for Super EditorA new mapping between the Super Editor role and user has been added to the roles_users array, maintaining the relationship structure consistent with other roles.
ghost/admin/app/models/user.js (2)
64-65: Adding Super Editor role to User modelThe
isSuperEditorandisEitherEditorcomputed properties effectively introduce support for the new Super Editor role in the User model. This correctly follows the existing pattern of role checks using theequalandorcomputed properties.
71-75: Good approach for transitioning to permissions-based systemAdding these permission-like properties is a great step toward migrating from hard-coded role-based permissions to a more flexible permissions system. The comments documenting this intention are helpful for maintaining the codebase.
ghost/core/test/integration/migrations/migration.test.js (2)
111-246: Super Editor permissions properly integrated into testsThe new Super Editor role has been consistently added to all relevant permission checks in the test suite. This ensures that users with this role will have the appropriate permissions for various actions including notifications, posts, settings, tags, users, etc.
288-299: Updated role count and verificationThe test now correctly expects 11 roles instead of 10, and verifies that the 11th role is 'Super Editor'. This ensures the new role is properly included in the fixtures.
ghost/admin/tests/acceptance/dashboard-test.js (2)
7-7: Updated import source for visitImporting
visitfrom a local helper rather than the default test helpers. This suggests a customized navigation approach for tests, which may provide additional functionality specific to Ghost's testing needs.
13-64: Well-structured test cases for different rolesThe refactored test structure provides clear separation between different user role tests. The new Super Editor test case properly verifies that this role, like Editors and Authors, cannot access the dashboard and is redirected to '/site'.
ghost/core/core/server/data/schema/fixtures/fixtures.json (2)
110-113: Added Super Editor role to fixturesThe new Super Editor role has been properly added to the Role model fixtures with an appropriate description.
924-946: Well-defined permission set for Super EditorThe Super Editor role has been configured with a comprehensive set of permissions that position it between Editor and Administrator roles. Key capabilities include:
- Full access to notifications, posts, and comments
- Read/browse access to settings
- Full access to member management
- Limited theme access
This permission set aligns with the new computed properties added to the User model (
canManageMembersandcanManageComments).ghost/admin/tests/acceptance/members/import-test.js (2)
14-113: Well-structured role-based testing approach.The test organization by role categories (Owner, Super Editor, Editor) provides clear separation and makes the permissions model easy to understand. The Super Editor role tests appropriately verify that this new role has the necessary permissions to manage member imports.
213-227: Good verification of permission boundaries.The Editor functions test properly verifies that users with the Editor role cannot access the members import functionality, confirming that the permission boundary is correctly implemented.
ghost/admin/tests/unit/helpers/gh-user-can-manage-members-test.js (1)
1-38: Well-structured unit tests for role-based member management.These tests clearly validate that the
ghUserCanManageMembershelper correctly identifies which roles can manage members. The test structure appropriately separates the positive case (Owner, Admin, Super Editor) from the negative case (Editor, Author, Contributor), making the permission boundaries clear.ghost/core/test/utils/fixtures/fixtures.json (2)
110-113: Super Editor role definition is appropriate.The addition of the "Super Editor" role to the Role model entries provides a clear name and description for this new role type.
1129-1151: Well-defined permission scope for Super Editor role.The permissions granted to the Super Editor role strike a good balance - allowing full member management capabilities (browse, read, add, edit, destroy) while maintaining appropriate restrictions on administrative settings. This creates a clear permissions tier between regular Editors and Administrators.
ghost/admin/tests/acceptance/members-test.js (2)
10-10: Descriptive test suite naming.Renaming from "Acceptance: Members" to "Acceptance: Members Test" makes the purpose of the test suite clearer.
21-21: Clear permission boundary description.Updated test description correctly specifies that both non-admins and non-super editors should be redirected, which aligns with the new permission model.
ghost/admin/tests/unit/models/user-test.js (3)
119-136: The Owner properties tests look goodThe updated test for Owner role correctly verifies the expected permissions. It properly checks that an Owner can manage members and comments, while not having editor-specific properties.
157-174: Super Editor role tests properly implementedThe test case for the Super Editor role properly validates that this new role:
- Has the
isSuperEditorproperty set to true- Is classified as an
isEitherEditor- Can manage members and comments
- Is distinct from Admin, Owner, Editor, and other roles
This aligns well with the purpose of adding this new role to the system.
60-78: Comprehensive role property testsThe updated test descriptions and additional assertions for each role (Contributor, Author, Editor, Admin) now provide more comprehensive validation of role properties. The tests now properly check not only basic role identification but also specific permissions like
canManageMembers,canManageComments, and the relationship between roles withisEitherEditor.This thorough testing approach will help catch unintended permission changes during future development.
Also applies to: 80-98, 100-117, 138-155
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/admin-x-framework/src/api/users.ts (1)
150-154: Functionality LGTM! Minor code style improvements needed.The function correctly implements the check for both 'Editor' and 'Super Editor' roles which aligns with the introduction of the new 'Super Editor' role. However, there are some code style improvements that could be made:
-export function isEditorUser(user: User) { - let isAnyEditor = user.roles.some(role => role.name === 'Editor') - || user.roles.some(role => role.name === 'Super Editor'); - return isAnyEditor; +export function isEditorUser(user: User) { + const isAnyEditor = user.roles.some(role => + role.name === 'Editor' || role.name === 'Super Editor' + ); + return isAnyEditor; }This optimization reduces the number of iterations through the roles array to just one pass.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
473-476: New Build Assets Step in Unit TestsThe newly added step labeled "Build remaining assets" executes
yarn nx run-many -t build --exclude=ghost-admin, ensuring that all packages (except forghost-admin, which is already built in thejob_setup) are built before unit tests run. Please verify that excludingghost-adminis intentional and that no critical assets or dependencies are inadvertently skipped.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: i18n
- GitHub Check: Database tests (Node 18.12.1, mysql8)
- GitHub Check: Lint
|
As mentioned in slack, found a few spots missing parity editor<>supereditor and we need to cover with tests.
Marking as pending review again. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
ghost/admin/tests/acceptance/editor/super-editor-lexical-test.js (3)
9-30: Add JSDoc comments to the utility function.The
selectTextWithHoverfunction implements complex DOM manipulation logic that would benefit from more detailed documentation. Consider adding JSDoc comments to explain its purpose, parameters, and return value./** * Selects text within an element and simulates hover events over the selection. * * @param {HTMLElement} element - The DOM element containing text to select * @param {number} start - The starting character position (default: 0) * @param {number} end - The ending character position (default: full text length) * @returns {Promise} A resolved promise when the operation is complete */ async function selectTextWithHover(element, start = 0, end = element.textContent.length) {
41-54: Consider expanding test coverage for snippet functionality.This test verifies that a Super Editor can select text and see the snippet menu, but doesn't test actually creating or using snippets. Consider adding additional test cases that verify the complete snippet workflow for Super Editors.
Also, the current test only verifies if the snippet button appears but doesn't test functionality specific to the Super Editor role. Consider adding tests that verify permissions or behaviors unique to Super Editors compared to regular Editors.
42-42: Extract lexical content into a fixture or constant.The hardcoded lexical JSON content makes the test less maintainable. Consider extracting this into a constant at the top of the file or into a separate fixtures file to improve readability and maintainability.
+ const TEST_POST_CONTENT = { + "root": { + "children": [{ + "children": [{ + "detail": 0, + "format": 0, + "mode": "normal", + "style": "", + "text": "This is a test", + "type": "extended-text", + "version": 1 + }], + "direction": "ltr", + "format": "", + "indent": 0, + "type": "paragraph", + "version": 1 + }], + "direction": "ltr", + "format": "", + "indent": 0, + "type": "root", + "version": 1 + } + }; it('can select text and trigger the snippet menu', async function () { - const post = this.server.create('post', {lexical: '{"root":{"children":[{"children":[{"detail":0,"format":0,"mode":"normal","style":"","text":"This is a test","type":"extended-text","version":1}],"direction":"ltr","format":"","indent":0,"type":"paragraph","version":1}],"direction":"ltr","format":"","indent":0,"type":"root","version":1}}'}); + const post = this.server.create('post', {lexical: JSON.stringify(TEST_POST_CONTENT)});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/admin/tests/acceptance/editor/super-editor-lexical-test.js(1 hunks)ghost/admin/tests/acceptance/tags-test.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup
🔇 Additional comments (1)
ghost/admin/tests/acceptance/tags-test.js (1)
41-41: Good clarification on test descriptionThe updated description "when logged in as administrator" provides better clarity about which role the tests are for, making the test organization more explicit.
| describe('Super Editor tests', function () { | ||
| beforeEach(async function () { | ||
| this.server.loadFixtures(); | ||
| await loginAsRole('Super Editor', this.server); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify Super Editor role permissions.
Based on the PR objectives, there were several discrepancies noted between Editors and Super Editors. Ensure this test adequately verifies the expected behavior for Super Editors regarding snippets functionality, which was one of the areas mentioned in the PR comments.
🏁 Script executed:
#!/bin/bash
# Look for related tests or configurations for snippet permissions
echo "Searching for snippet-related permissions for different roles..."
rg -A 5 -B 5 "snippet.*permission|permission.*snippet" --glob "*.js"
# Check if there are any other tests for Super Editor and snippets
echo "Checking for other Super Editor snippets tests..."
rg -l "Super Editor.*snippet|snippet.*Super Editor" --glob "*.js"
# Look for role definitions to verify Super Editor permissions
echo "Examining role definitions for Super Editor..."
rg -A 10 "Super Editor" --glob "*.js" | grep -E "can|permission|ability"Length of output: 36771
Action Required: Extend Super Editor Acceptance Test for Snippet Functionality
The current test in ghost/admin/tests/acceptance/editor/super-editor-lexical-test.js only logs in as a Super Editor (line 38) without actually verifying the snippet-related behaviors. Although our integration and migration tests already confirm that Super Editors receive the correct snippet permissions (e.g., Browse, Read, Edit, Add, and Delete snippets), the acceptance test should also simulate and assert the expected UI interactions.
Please consider updating the test to include steps such as:
- Simulating text selection in the editor.
- Triggering the snippet menu.
- Asserting that the snippet controls appear and function as intended for a Super Editor.
This ensures that the UI behavior aligns with the PR objectives regarding the discrepancies between Editors and Super Editors.
There was a problem hiding this comment.
I've got it selecting text and triggering the snippet menu.
There was a problem hiding this comment.
🧩 Analysis chain
@cathysarisky That's a good start with implementing the text selection and triggering the snippet menu. Could you please share what assertions you've added to verify that the snippet functionality works correctly for Super Editors? I'd like to make sure the test fully validates that Super Editors have the proper permissions to manage snippets as mentioned in the PR objectives.
🏁 Script executed:
#!/bin/bash
# View the full content of the super-editor-lexical-test.js file
cat ghost/admin/tests/acceptance/editor/super-editor-lexical-test.js
Length of output: 2595
🏁 Script executed:
#!/bin/bash
# Look for related tests or UI components for snippet functionality
echo "Searching for snippet-related tests..."
rg -l "snippet.*test|test.*snippet" --glob "*.js"
# Check for snippet management in controllers or components
echo "Looking for snippet management code..."
rg -l "canManageSnippets" --glob "*.js"
# Look for implementations of snippet functionality
echo "Examining snippet implementation..."
rg -A 5 "Save as snippet|manage snippet" --glob "*.js"
Length of output: 887
@cathysarisky Great job on implementing the text selection and triggering the snippet menu! I can see your test confirms that the "Save as snippet" button appears correctly for Super Editors.
To fully validate the Super Editor's snippet management capabilities (which was one of the discrepancies noted in the PR), you might want to extend the test to include:
- Clicking the "Save as snippet" button and creating a new snippet
- Verifying the snippet is successfully created
- Optionally, testing that the Super Editor can also manage (edit or delete) existing snippets
Would you like me to suggest some code examples for these additional test steps?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get the full content of the current test file
echo "Current test file content:"
cat ghost/admin/tests/acceptance/editor/super-editor-lexical-test.js
# Look for related tests that handle snippet creation and management
echo -e "\nRelated snippet test examples:"
rg -l "snippet.*test" --glob "*.js" | xargs head -n 20
# Look for UI components related to snippet creation
echo -e "\nSnippet creation related code:"
rg -A 10 "Save as snippet" --glob "*.js"
# Look for snippet management related code
echo -e "\nSnippet management related code:"
rg -A 10 "canManageSnippets" --glob "*.js"
Length of output: 5292
🏁 Script executed:
#!/bin/bash
# Look for click and interaction helpers that we might need
echo "Looking for click and interaction helpers:"
rg -l "click|clickTrigger" --glob "**/*helpers*.js" | head -n 5
# Find more details on how snippets are created and managed in the UI
echo -e "\nSnippet creation UI flow:"
rg -A 15 "snippet.*create|create.*snippet" --glob "*.js" --glob "*.hbs"
# Find how snippets are stored and retrieved
echo -e "\nSnippet storage and retrieval:"
rg -A 5 "snippet.*store|store.*snippet|snippet.*model" --glob "*.js"
# Check how other tests verify created content
echo -e "\nExamples of verifying created content in tests:"
rg -A 5 "expect.*created|assert.*created" --glob "**/*test*.js" | head -n 30
Length of output: 19314
@cathysarisky Here's how you can extend the test to include creating and verifying a snippet:
it('can create and save a snippet', async function () {
const post = this.server.create('post', {lexical: '{"root":{"children":[{"children":[{"detail":0,"format":0,"mode":"normal","style":"","text":"This is a test snippet","type":"extended-text","version":1}],"direction":"ltr","format":"","indent":0,"type":"paragraph","version":1}],"direction":"ltr","format":"","indent":0,"type":"root","version":1}}'});
await visit(`/editor/post/${post.id}`);
await waitFor('[data-secondary-instance="false"] [data-lexical-editor]');
// Select text
await selectTextWithHover(find('.kg-prose p span'));
await settled();
// Wait for snippet button to appear and click it
await waitUntil(() => document.querySelectorAll('[data-kg-toolbar-button="snippet"]').length > 0);
const snippetButton = document.querySelector('[data-kg-toolbar-button="snippet"]');
await click(snippetButton);
// Wait for snippet form dialog to appear
await waitFor('.modal-content');
// Fill in snippet name
await fillIn('.modal-content input[type="text"]', 'Test Snippet');
// Click save button
await click('.modal-content button[type="button"]:contains("Save")');
// Verify success notification
await waitFor('.gh-notification.gh-notification-green');
expect(find('.gh-notification').textContent.trim()).to.include('Snippet saved as "Test Snippet"');
// Verify the snippet was created in the store
const snippets = this.owner.lookup('controller:lexical-editor').get('_snippets');
const savedSnippet = snippets.find(s => s.name === 'Test Snippet');
expect(savedSnippet).to.exist;
});
it('can use a previously created snippet', async function () {
// First create a snippet in the backend
this.server.create('snippet', {
name: 'Reusable Snippet',
lexical: '{"root":{"children":[{"children":[{"detail":0,"format":0,"mode":"normal","style":"","text":"This is a reusable snippet","type":"extended-text","version":1}],"direction":"ltr","format":"","indent":0,"type":"paragraph","version":1}],"direction":"ltr","format":"","indent":0,"type":"root","version":1}}'
});
// Create a post and visit the editor
const post = this.server.create('post', {lexical: '{"root":{"children":[{"children":[{"detail":0,"format":0,"mode":"normal","style":"","text":"Testing snippets here","type":"extended-text","version":1}],"direction":"ltr","format":"","indent":0,"type":"paragraph","version":1}],"direction":"ltr","format":"","indent":0,"type":"root","version":1}}'});
await visit(`/editor/post/${post.id}`);
// Trigger the snippet menu
await click('[data-kg-plus-button]');
await waitFor('[data-kg-card-menu-item="Snippet"]');
await click('[data-kg-card-menu-item="Snippet"]');
// Select our snippet from the list
await waitFor('.gh-snippet-list-item:contains("Reusable Snippet")');
await click('.gh-snippet-list-item:contains("Reusable Snippet")');
// Verify the snippet content was inserted
expect(find('[data-lexical-editor]')).to.contain.text('This is a reusable snippet');
});You'll need to add these imports at the top of the file:
import {click, fillIn} from '@ember/test-helpers';Note that some selector details might need adjustments based on the actual HTML structure of your app. The :contains() pseudo-selector might not be directly supported, but Ember has alternative ways to find elements containing specific text.
Would you like me to explain any part of this code in more detail?
ghost/admin/tests/acceptance/editor/super-editor-lexical-test.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ghost/admin/tests/acceptance/tags-test.js (1)
168-231: Consider adding the missing test cases for super editorsThe administrator test suite includes several test cases that are not covered in the super editor suite:
- Testing public vs internal tags (separation and visibility)
- Loading a tag via slug
- 404 handling for non-existent tags
- Creating a new tag
Consider adding these tests for super editors to ensure complete parity in testing between the two roles.
ghost/admin/tests/acceptance/editor/super-editor-lexical-test.js (1)
8-28: Helper function implementation is good but has room for improvement.The
selectTextWithHoverfunction correctly simulates text selection and hover events, but the empty return statement at line 27 is unnecessary and could be removed for cleaner code.- async function selectTextWithHover(element, start = 0, end = element.textContent.length) { - const range = document.createRange(); - const sel = window.getSelection(); - - range.setStart(element.childNodes[0], start); - range.setEnd(element.childNodes[0], end); - - sel.removeAllRanges(); - sel.addRange(range); - - // Trigger selection change event - document.dispatchEvent(new Event('selectionchange')); - await settled(); - - // Simulate moving the mouse over the selection - await triggerEvent(element, 'mouseover'); - await settled(); - await triggerEvent(element,'mouseenter'); - await settled(); - return; - } + async function selectTextWithHover(element, start = 0, end = element.textContent.length) { + const range = document.createRange(); + const sel = window.getSelection(); + + range.setStart(element.childNodes[0], start); + range.setEnd(element.childNodes[0], end); + + sel.removeAllRanges(); + sel.addRange(range); + + // Trigger selection change event + document.dispatchEvent(new Event('selectionchange')); + await settled(); + + // Simulate moving the mouse over the selection + await triggerEvent(element, 'mouseover'); + await settled(); + await triggerEvent(element,'mouseenter'); + await settled(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/admin/tests/acceptance/editor/super-editor-lexical-test.js(1 hunks)ghost/admin/tests/acceptance/tags-test.js(2 hunks)
🔇 Additional comments (6)
ghost/admin/tests/acceptance/tags-test.js (5)
41-41: Good clarification of test descriptionThis change makes the test context clearer by specifying the admin role, which is important now that we're testing multiple roles.
168-174: LGTM: Proper test setup for Super Editor roleThe test setup correctly creates a user with the "Super editor" role and authenticates the session. This ensures the tests properly validate Super Editor permissions.
175-179: Test addresses tags navigation issue for super editorsGood implementation of the basic navigation test. This addresses the "Tags showing in navigation" issue mentioned in the PR comments by verifying that super editors can access the tags page and the navigation item is properly highlighted.
180-208: Test verifies tag editing functionality for super editorsThis test ensures that super editors have the ability to edit tags, which is an important part of the role's permissions. The implementation is thorough and includes verification of all steps in the editing process.
210-230: Test verifies tag deletion functionality for super editorsThis test effectively validates that super editors can delete tags, which is a critical permission check. The test includes verification of the deletion modal and confirms the tag is removed from the list.
ghost/admin/tests/acceptance/editor/super-editor-lexical-test.js (1)
39-52: Test needs to be extended to fully verify Super Editor snippet capabilities.The current test verifies that a Super Editor can select text and see the "Save as snippet" button, but it doesn't test the complete workflow of creating and managing snippets, which was one of the key discrepancies mentioned in the PR comments.
As mentioned in previous review comments, this test should be extended to include:
- Clicking the "Save as snippet" button
- Creating a new snippet
- Verifying the snippet is successfully created
- Testing that the Super Editor can manage (edit or delete) existing snippets
This ensures the test fully validates the Super Editor's snippet management capabilities.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ghost/admin/tests/acceptance/settings-button-test.js (1)
1-36: Consider adding tests for the other missing functionality mentioned in the PR comments.Based on the PR comments, there are still discrepancies between editor and super editor functionality that need addressing, such as tags in navigation, snippet management, settings in footer, and visibility input in the editor sidebar. Consider expanding these tests to cover those areas.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/admin/tests/acceptance/settings-button-test.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Admin tests - Chrome
🔇 Additional comments (6)
ghost/admin/tests/acceptance/settings-button-test.js (6)
1-8: Appropriate test imports for Ember acceptance testing.The imports are correctly set up for an Ember acceptance test, including authentication utilities, test helpers, Mocha/Chai for test structure and assertions, and Mirage for API mocking.
10-13: Good test suite setup.The test suite is properly configured with the necessary hooks for application testing and Mirage for API mocking.
14-18: Proper test isolation with session invalidation.The beforeEach hook appropriately invalidates any existing user session before each test, ensuring clean test isolation.
19-24: Correct test implementation for editor role.This test properly verifies that the settings button is present for users with the Editor role. The test follows a good pattern: log in as an editor, visit the site, and check for the presence of the settings button.
25-29: Correct test implementation for author role.This test correctly verifies that the settings button is not present for users with the Author role, using appropriate assertions.
30-34: Appropriate test for new Super Editor role.This test correctly verifies that the settings button is present for users with the Super Editor role, which aligns with the PR objective to support this new role. The test follows the same pattern as the editor test for consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ghost/admin/tests/acceptance/tags-test.js (2)
268-268: Remove the debug console.log statement.This appears to be debugging code that was left in the final implementation. Console.log statements should be removed from production and test code.
- console.log('role is', role)
168-362: Consider reducing duplication in test casesThe test cases for editor and super editor roles are identical, which creates significant code duplication. Consider extracting these common test scenarios into shared test helper functions to improve maintainability.
You could create a shared function like this:
function testTagsCapabilities(roleName) { describe(`as a ${roleName}`, function () { beforeEach(async function () { let role = this.server.create('role', {name: roleName}); this.server.create('user', {roles: [role]}); await authenticateSession(); }); it('lists public and internal tags separately', async function () { // Existing test code }); it('can edit tags', async function () { // Existing test code }); it('can delete tags', async function () { // Existing test code }); }); } // Then call it with different roles testTagsCapabilities('Editor'); testTagsCapabilities('Super Editor');This would significantly reduce duplication while maintaining the same test coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ghost/admin/mirage/config/roles.js(1 hunks)ghost/admin/tests/acceptance/editor/super-editor-lexical-test.js(1 hunks)ghost/admin/tests/acceptance/settings-button-test.js(1 hunks)ghost/admin/tests/acceptance/tags-test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ghost/admin/tests/acceptance/editor/super-editor-lexical-test.js
- ghost/admin/tests/acceptance/settings-button-test.js
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Lint
- GitHub Check: Admin tests - Chrome
🔇 Additional comments (3)
ghost/admin/mirage/config/roles.js (1)
4-4: Added Super Editor role to assignable rolesThis change adds role ID 6 (the Super Editor role based on PR context) to the list of assignable roles when querying with permission "assign". This correctly implements the PR objective of making admin changes for super editors.
ghost/admin/tests/acceptance/tags-test.js (2)
265-362: Addressing PR comments: tags showing in navigationWhile the tests verify that the tags page functions correctly for super editors, they don't specifically address the PR comment about "Tags showing in navigation" discrepancies between editors and super editors. Consider adding a specific test case that verifies the navigation visibility and accessibility.
You might need a test case similar to this:
it('shows tags in navigation', async function () { await visit('/'); // Verify tags appear in navigation menu for super editors expect(find('[data-test-nav="tags"]')).to.exist; expect(find('[data-test-nav="tags"]')).to.be.visible; // Verify clicking tags in navigation menu routes to tags page await click('[data-test-nav="tags"]'); expect(currentURL()).to.equal('tags'); });This would explicitly test that the tags link is visible and functional in the navigation for super editors, addressing the comment about "Tags showing in navigation".
168-362:✅ Verification successful
Validate differences between roles
The current tests for editor and super editor are identical, which doesn't validate any actual differences between these roles. If both roles should behave identically with tags, this is appropriate, but the PR comments suggest there may be differences to verify.
If the behavioral expectations for both roles are indeed identical for tags, please add a comment clarifying this. If there are expected differences, add test cases that specifically verify those differences.
🏁 Script executed:
#!/bin/bash # Search for any role-specific handling of tags in routes or templates echo "Searching for role-specific tag handling in the codebase..." rg -n "tag.*editor" --type js ghost/admin/ rg -n "isEditor.*tag" --type js ghost/admin/ rg -n "isSuperEditor.*tag" --type js ghost/admin/Length of output: 1634
Clarify Identical Role Behavior for Tags
The tests for the editor and super editor are identical, and our search of the codebase didn’t reveal any role-specific logic for tags (beyond an unrelated result). If both roles are intended to have the same tag management capabilities, please add an inline comment in the tests clarifying that this identical behavior is deliberate. Otherwise, if differences are expected, please add or update test cases to verify the distinct behavior for each role.
Ref #22445
This PR has the admin section changes (and associated tests) for super editors, including routing adjustments.