🐛 Fixed incorrect slug increments for post title changes #22618
Conversation
WalkthroughThe changes update the slug generation process across multiple parts of the system. In the Possibly related PRs
Suggested labels
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/app/controllers/lexical-editor.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/unit/controllers/editor-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/mirage/config/slugs.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'
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (6)
🧰 Additional context used🧬 Code Definitions (2)ghost/admin/tests/unit/controllers/editor-test.js (1)
ghost/core/test/unit/server/models/base/index.test.js (3)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (8)
✨ 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 (
|
allouis
force-pushed
the
onc-794-improve-slug-generation-collision-detection
branch
from
9b8aa66 to
3b1c93d
Compare
![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 (1)
ghost/core/test/e2e-api/admin/slugs.test.js (1)
36-45: Test case for handling same-resource slug collision looks good!This test verifies the new functionality where a resource can retain its slug when the ID is provided. It uses a specific ID that appears to be from the fixtures (
6194d3ce51e2700162531a71).Consider using a fixture reference rather than a hard-coded ID to improve maintainability:
- .get('slugs/post/integrations/6194d3ce51e2700162531a71') + .get(`slugs/post/integrations/${fixtureManager.get('integrations', 0).id}`)
📜 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__/slugs.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
ghost/admin/app/controllers/lexical-editor.js(2 hunks)ghost/admin/app/services/slug-generator.js(1 hunks)ghost/admin/mirage/config/slugs.js(1 hunks)ghost/admin/tests/integration/services/slug-generator-test.js(2 hunks)ghost/core/core/server/api/endpoints/slugs.js(3 hunks)ghost/core/core/server/models/base/plugins/generate-slug.js(1 hunks)ghost/core/core/server/web/api/endpoints/admin/routes.js(1 hunks)ghost/core/test/e2e-api/admin/slugs.test.js(1 hunks)ghost/core/test/unit/server/models/base/index.test.js(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
ghost/core/test/unit/server/models/base/index.test.js (3)
ghost/core/core/server/models/base/plugins/generate-slug.js (2)
security(2-2)slug(20-20)ghost/core/core/server/models/user.js (1)
security(8-8)ghost/core/test/unit/server/models/user.test.js (1)
security(7-7)
ghost/core/test/e2e-api/admin/slugs.test.js (2)
ghost/core/test/e2e-api/admin/settings-files.test.js (1)
agent(6-6)ghost/core/test/e2e-api/admin/custom-theme-settings.test.js (1)
agent(8-8)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Database tests (Node 22.13.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 18.12.1, mysql8)
- GitHub Check: Admin tests - Chrome
🔇 Additional comments (15)
ghost/core/core/server/web/api/endpoints/admin/routes.js (1)
171-171: Looks good! Adding new route to support slug generation with resource ID.This change adds a new route that includes a resource ID parameter for slug generation while maintaining the existing route for backward compatibility. This is the right approach to maintain non-breaking changes as mentioned in the PR objective.
ghost/core/core/server/models/base/plugins/generate-slug.js (1)
42-46: Nice solution for preventing unnecessary slug increments.This change properly implements the logic to avoid slug increments when the slug belongs to the same post being edited. The conditional check verifies if the model ID matches the found slug's ID before applying any increments, which addresses the core issue described in the PR.
ghost/core/test/unit/server/models/base/index.test.js (1)
42-76: Well-crafted tests for the new slug generation behavior.These two test cases thoroughly validate the new slug generation logic:
- First test confirms that when a slug exists but for a different model ID, it correctly increments the slug.
- Second test verifies that when a slug exists for the same model ID, it correctly returns the original slug without increments.
The stubbing approach with Model.findOne is elegant and clearly demonstrates both scenarios.
ghost/admin/mirage/config/slugs.js (1)
12-18: Good implementation of Mirage route to match backend changes.This new route handler correctly mirrors the backend API changes by supporting the ID parameter in the slug generation endpoint. The implementation maintains consistency with the existing route handler logic, which is appropriate since the behavior should be similar.
ghost/core/core/server/api/endpoints/slugs.js (3)
22-26: Appropriate addition of the ID parameter to the options arrayThe addition of the
idoption to the options array is well-implemented. This change aligns with the PR objectives by allowing clients to query slug availability with a resource ID parameter.
36-40: Well-structured validation for the new ID parameterThe validation for the
idoption is correctly implemented as not required (required: false), which maintains backward compatibility with existing API calls. This ensures that clients can still query slug availability without specifying an ID.
47-49: Correctly passing the ID to the model's generateSlug methodThe query method has been appropriately updated to pass the
modelIdparameter to thegenerateSlugfunction. This correctly implements the fix for slug availability checks by allowing the model to know which resource is being edited.ghost/admin/app/services/slug-generator.js (2)
13-13: Method signature updated to support the new modelId parameterThe method signature has been appropriately extended to include the optional
modelIdparameter, maintaining backward compatibility with existing calls while adding the new functionality.
21-26: Well-implemented conditional URL constructionThe implementation correctly constructs different API URLs based on whether a
modelIdis provided. This change properly supports the backend API changes and ensures that slug checks can optionally include the resource ID.ghost/admin/app/controllers/lexical-editor.js (2)
787-787: Correctly passing post ID to the slugGenerator in updateSlugTaskThe
updateSlugTaskmethod now properly passes the post ID as an additional parameter to thegenerateSlugmethod. This ensures that when a post's slug is being updated, the system can identify that this is an existing post checking its own slug availability.
964-964: Correctly passing post ID to the slugGenerator in generateSlugTaskThe
generateSlugTaskmethod also properly passes the post ID to thegenerateSlugmethod. This ensures consistent behavior between manual slug updates and automatic slug generation based on title changes.ghost/admin/tests/integration/services/slug-generator-test.js (2)
8-33: Well-structured test endpoint stub with ID supportThe
stubSlugEndpointfunction has been effectively updated to handle both cases - with and without an ID parameter. The conditional logic properly sets up different endpoint stubs and includes appropriate assertions to verify the correct parameters are passed.
69-79: Good test coverage for the new functionalityThe new test case properly verifies that the slug generator service works correctly when an ID is provided. This ensures that the changes are properly tested and the functionality works as expected.
ghost/core/test/e2e-api/admin/slugs.test.js (2)
25-34: Test case for slug collision without ID looks good!This test case correctly verifies the default behavior when there's a slug collision without providing a resource ID. It ensures that the API increments slugs when a collision is detected and no specific resource ID is provided.
47-56: Test case for handling different-resource slug collision looks good!This test properly verifies what happens when an ID that doesn't match the resource is provided. Using
000000000000000000000000as a non-existent ID is a good approach for testing this scenario.
| if (modelId) { | ||
| url = this.get('ghostPaths.url').api('slugs', slugType, name, modelId); | ||
| } else { | ||
| url = this.get('ghostPaths.url').api('slugs', slugType, name); |
There was a problem hiding this comment.
Are there reproducible scenarios where modelId may not exist given the changes?
There was a problem hiding this comment.
Yeah, this service is used for generating slugs for tags and users too, and we don't pass the modelId there!
|
Blowing my mind a bit, I was so tunnel-visioned on the frontend that I hadn't considered this option at all 👀 |
ref https://linear.app/ghost/issue/ONC-794 When checking slug availability without specifying the ID of the specific resource being modified, the system can't distinguish between a new resource and an existing one. This causes false "slug already in use" results when a client checks availability of a slug that's already assigned to the resource being updated. For example, if post ID "123" has slug "snowman" and the client checks if the slug "snowman" is available (without specifying it's for post "123"), the system returns "snowman-2" unnecessarily. This commonly happens during title edits that would generate the same slug, like adding a trailing space. This allows clients to ask "is the slug 'snowman' available for resource ID '123'?" which handles the case where the resource already owns that slug. This is a non-breaking change - the existing API continues to work at both the HTTP and method call levels, with the modelId being optional.
closes https://linear.app/ghost/issue/ONC-794 By passing the post ID when generating a slug, the system can now properly determine if the slug is already assigned to the post being edited. This correctly handles cases like whitespace-only title changes where the same slug should be maintained rather than being incremented unnecessarily.
allouis
force-pushed
the
onc-794-improve-slug-generation-collision-detection
branch
from
3b1c93d to
d4a5d4e
Compare
|
I've added a test to ensure that the id is passed to the |
ref https://linear.app/ghost/issue/ONC-794
When checking slug availability without specifying the ID of the specific resource
being modified, the system can't distinguish between a new resource and an existing
one. This causes false "slug already in use" results when a client checks availability
of a slug that's already assigned to the resource being updated.
For example, if post ID "123" has slug "snowman" and the client checks if the slug
"snowman" is available (without specifying it's for post "123"), the system returns
"snowman-2" unnecessarily. This commonly happens during title edits that would
generate the same slug, like adding a trailing space.
This allows clients to ask "is the slug 'snowman' available for resource ID
'123'?" which handles the case where the resource already owns that slug.
This is a non-breaking change - the existing API continues to work at both the
HTTP and method call levels, with the modelId being optional.
By passing the post ID when generating a slug, the system can now properly
determine if the slug is already assigned to the post being edited. This
correctly handles cases like whitespace-only title changes where the same
slug should be maintained rather than being incremented unnecessarily.