Fixed duplicate tag insertion in assignTagToUserPosts#26528
Fixed duplicate tag insertion in assignTagToUserPosts#26528Ray0907 wants to merge 2 commits intoTryGhost:mainfrom
Conversation
WalkthroughCode formatting and module require paths were normalized across the users service. The assignTagToUserPosts implementation was refactored to use a Set for filtering posts already tagged, updated bulk-insert construction, and adjusted event dispatch and addActions call signatures. Minor, non-functional formatting changes were made in password reset and user destruction workflows. Tests were added/expanded: a new suite covers assignTagToUserPosts behavior (including mocked posts_authors/posts_tags interactions, addActions tracking, and event dispatch) and resetAllPasswords assertions. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on March 3
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/core/server/services/users.js (1)
127-143:⚠️ Potential issue | 🟠 MajorGuard against empty
usersPostIdsafter filtering to prevent a Knexinsert([])error.Before this fix the broken comparison meant filtering never removed anything, so
usersPostIdswas always non-empty when reachinginsert. Now that filtering works correctly, the scenario where all user posts are already tagged producesusersPostIds = [], which flows intoknex('posts_tags').insert([]). Knex rejects an empty insert array ("Empty.insert()call detected!") in most adapters, causing the entire user-deletion transaction to fail.Add the same early-return pattern used at line 96 right after the filtering block:
🐛 Proposed fix
const taggedPostIdSet = new Set(taggedPostIds.map(post => post.post_id)); usersPostIds = usersPostIds.filter(postId => !taggedPostIdSet.has(postId)); } + if (usersPostIds.length === 0) { + return; + } + // assign tag to posts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/users.js` around lines 127 - 143, After filtering tagged post IDs, guard against an empty usersPostIds array before calling knex insert and addActions: check if usersPostIds.length === 0 and return early (skip the this.models.Base.knex('posts_tags') .insert(...) and this.models.Post.addActions('edited', usersPostIds, ...)) so you don't call knex('posts_tags').insert([]) which throws; apply the same early-return pattern used earlier around line 96 in this service function to avoid executing the insert and addActions when usersPostIds is empty.
🧹 Nitpick comments (1)
ghost/core/test/unit/server/services/users/users-service.test.js (1)
130-134: Missing assertion onDomainEvents.dispatchinvocation.
dispatchStubis set up but never asserted, so the event-dispatch leg ofassignTagToUserPostsis untested. Add an assertion matching what the PR description claims is verified:✅ Proposed addition
assert.deepEqual(addActions.args[0][1], ['post-1']); +assert.equal(dispatchStub.calledOnce, true); +assert.deepEqual(dispatchStub.args[0][0].postIds, ['post-1']);(Adjust the property path to match the actual shape of
PostsBulkAddTagsEvent.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/users/users-service.test.js` around lines 130 - 134, The test for assignTagToUserPosts is missing an assertion that DomainEvents.dispatch (dispatchStub) was called; add assertions after the existing checks to verify dispatchStub.calledOnce and that dispatchStub.args[0][0] (or the actual first arg path) matches the expected PostsBulkAddTagsEvent payload — e.g., confirm the event type/class and that its payload contains the tag id and the array of post ids (['post-1']) so the event-dispatch leg is covered; update the property path to match PostsBulkAddTagsEvent shape used by assignTagToUserPosts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/core/test/unit/server/services/users/users-service.test.js`:
- Line 58: The test creates a sinon.stub for DomainEvents.dispatch inline which
won't be restored if an assertion throws; move the stub setup into a beforeEach
(create dispatchStub = sinon.stub(DomainEvents, 'dispatch')) and restore it in
an afterEach (dispatchStub.restore() or sinon.restore()) so
DomainEvents.dispatch is always cleaned up; update the users-service.test.js to
remove the inline stub at the top of the test and instead use
beforeEach/afterEach hooks around the tests that reference dispatchStub.
---
Outside diff comments:
In `@ghost/core/core/server/services/users.js`:
- Around line 127-143: After filtering tagged post IDs, guard against an empty
usersPostIds array before calling knex insert and addActions: check if
usersPostIds.length === 0 and return early (skip the
this.models.Base.knex('posts_tags') .insert(...) and
this.models.Post.addActions('edited', usersPostIds, ...)) so you don't call
knex('posts_tags').insert([]) which throws; apply the same early-return pattern
used earlier around line 96 in this service function to avoid executing the
insert and addActions when usersPostIds is empty.
---
Nitpick comments:
In `@ghost/core/test/unit/server/services/users/users-service.test.js`:
- Around line 130-134: The test for assignTagToUserPosts is missing an assertion
that DomainEvents.dispatch (dispatchStub) was called; add assertions after the
existing checks to verify dispatchStub.calledOnce and that
dispatchStub.args[0][0] (or the actual first arg path) matches the expected
PostsBulkAddTagsEvent payload — e.g., confirm the event type/class and that its
payload contains the tag id and the array of post ids (['post-1']) so the
event-dispatch leg is covered; update the property path to match
PostsBulkAddTagsEvent shape used by assignTagToUserPosts.
ghost/core/test/unit/server/services/users/users-service.test.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ghost/core/core/server/services/users.js (1)
91-100:idis spuriously present in the options argument.The second argument to
findOneis the options/frame object (context,transacting).idbelongs only in the first (query criteria) argument; having it also in the options object is a no-op but misleading.🧹 Proposed cleanup
const author = await this.models.User.findOne( { id, }, { - id, context, transacting, }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/users.js` around lines 91 - 100, The options object passed to models.User.findOne mistakenly includes the query field id; update the call in users.js (the const author = await this.models.User.findOne(...) invocation) so that the first argument contains the id criterion and the second argument only contains options like context and transacting (remove id from the options object), keeping the function signature and behavior unchanged.ghost/core/test/unit/server/services/users/users-service.test.js (1)
93-181: No test for the new post-filtering empty guard (lines 153–155 ofusers.js).The commit adds a guard returning early when all posts are already tagged, but the only test exercises the partial-overlap case (one new post, one already-tagged). A scenario where both posts are already tagged would cover that new branch and confirm
insertis never called.💡 Suggested additional test
it("returns early when all posts already have the user tag", async function () { const insertedRows = []; const addActions = sinon.stub().resolves(); // same mockOptions structure, but posts_tags already contains both post-1 and post-2 // (patch only the posts_tags select to return both) const mockOptions = { // ... (same structure as the existing test) models: { Base: { knex: (tableName) => { if (tableName === "posts_authors") { return { transacting() { return this; }, where() { return this; }, select() { return Promise.resolve([ {post_id: "post-1"}, {post_id: "post-2"}, ]); }, }; } if (tableName === "posts_tags") { return { transacting() { return this; }, where() { return this; }, select() { // Both posts are already tagged return Promise.resolve([ {post_id: "post-1"}, {post_id: "post-2"}, ]); }, insert(rows) { insertedRows.push(...rows); return Promise.resolve(); }, }; } }, }, // ... (same User, Tag, Post mocks) }, }; const usersService = new Users(mockOptions); await usersService.assignTagToUserPosts({ id: "user-id", context: {}, transacting: {} }); assert.equal(insertedRows.length, 0); assert.equal(addActions.called, false); assert.equal(dispatchStub.called, false); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/users/users-service.test.js` around lines 93 - 181, Add a unit test that covers the early-return guard in Users.assignTagToUserPosts so the branch when all posts are already tagged is exercised: create a test similar to the existing "does not reinsert posts..." case but have the Base.knex mock for "posts_tags".select() return both post-1 and post-2, then instantiate new Users(mockOptions) and call assignTagToUserPosts({id: "user-id", context: {}, transacting: {}}); assert insertedRows.length === 0, addActions was not called, and dispatchStub was not called to verify no insert or actions are performed when the post list is empty after filtering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/core/test/unit/server/services/users/users-service.test.js`:
- Around line 113-116: ESLint object-curly-spacing errors come from spaces
inside object literals in the mocked return values; locate the
Promise.resolve([...]) mocks in users-service.test.js (the return
Promise.resolve blocks that yield { post_id: "post-1" }, { post_id: "post-2" }
and the similar trio around the later block) and remove the inner spaces so
object literals use {post_id: "post-1"} style (apply the same fix to the other
affected objects in that test).
---
Duplicate comments:
In `@ghost/core/test/unit/server/services/users/users-service.test.js`:
- Around line 85-91: The test should stub DomainEvents.dispatch in beforeEach
and always restore it in afterEach to avoid leaking the stub across tests;
ensure a dispatchStub variable is declared in the surrounding describe scope,
create the stub with sinon.stub(DomainEvents, "dispatch") inside beforeEach, and
call dispatchStub.restore() inside afterEach so the original
DomainEvents.dispatch is reinstated even if assertions fail.
---
Nitpick comments:
In `@ghost/core/core/server/services/users.js`:
- Around line 91-100: The options object passed to models.User.findOne
mistakenly includes the query field id; update the call in users.js (the const
author = await this.models.User.findOne(...) invocation) so that the first
argument contains the id criterion and the second argument only contains options
like context and transacting (remove id from the options object), keeping the
function signature and behavior unchanged.
In `@ghost/core/test/unit/server/services/users/users-service.test.js`:
- Around line 93-181: Add a unit test that covers the early-return guard in
Users.assignTagToUserPosts so the branch when all posts are already tagged is
exercised: create a test similar to the existing "does not reinsert posts..."
case but have the Base.knex mock for "posts_tags".select() return both post-1
and post-2, then instantiate new Users(mockOptions) and call
assignTagToUserPosts({id: "user-id", context: {}, transacting: {}}); assert
insertedRows.length === 0, addActions was not called, and dispatchStub was not
called to verify no insert or actions are performed when the post list is empty
after filtering.
1 participant
What does this PR do?
Fixed a bug in
assignTagToUserPostswhere posts that already had theuser tag were being re-inserted, causing duplicate tag assignments.
Why is this change needed?
The original filter logic compared raw row objects (
{post_id: 'xxx'})against string IDs using
Array.includes(), so it never matched.This meant every call to
assignTagToUserPostswould insert duplicateposts_tagsrows for posts that already had the tag.How does it work?
post_idfrom the raw query results into aSetusersPostIds(already mapped to strings) against the SetSet.has()also improves performance from O(n) to O(1) per lookupChecklist
Guide
Note
Low Risk
Small, well-scoped change to tag-assignment filtering logic plus a targeted unit test; low risk aside from potential edge cases in the tag lookup/query results.
Overview
Prevents
assignTagToUserPostsfrom re-insertingposts_tagsrows for posts that already have the user’s internal#slugtag by correctly filtering existing tagged posts (building aSetofpost_ids from the query results) and early-returning when nothing remains to tag.Adds a unit test that stubs
DomainEvents.dispatchand verifies only untagged posts are inserted, withPost.addActionsand the dispatched event receiving the filtered post IDs.Written by Cursor Bugbot for commit 6256827. This will update automatically on new commits. Configure here.