Perf unify group add bulk add#1
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request aims to significantly improve the performance and consistency of managing user memberships within groups. By introducing a dedicated GroupManager and enhancing existing bulk add/remove functionalities, the changes centralize complex logic related to user attributes, trust levels, and notification settings. This refactoring streamlines operations that involve adding or removing multiple users from groups, leading to more efficient processing and a more robust system for group administration. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
📝 WalkthroughWalkthroughThis pull request refactors group membership operations by introducing a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant GroupManager
participant Group
participant Database
participant CategoryUser
participant TagUser
participant Job
Client->>GroupManager: add(user_ids, automatic: true)
GroupManager->>Group: bulk_add(user_ids, automatic: true)
Group->>Database: INSERT group_users (notification_level) RETURNING user_id
Database-->>Group: added_user_ids
Group->>Group: Update flair_group_id/primary_group_id for new users
Group->>Group: Recalculate user count
Group->>CategoryUser: bulk_set_category_notifications(group, added_user_ids)
CategoryUser->>Database: INSERT ... SELECT with ON CONFLICT
CategoryUser->>CategoryUser: auto_watch/auto_track
Group->>TagUser: bulk_set_tag_notifications(group, added_user_ids)
TagUser->>Database: INSERT ... SELECT with ON CONFLICT
TagUser->>TagUser: auto_watch/auto_track
Group->>Job: Enqueue trust level updates (if grant_trust_level > 0)
Job-->>Database: Process trust level changes
Group->>Client: Return added_user_ids
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
codeant-ai
Bot
added
the
size:XL
Sequence DiagramThis PR routes single-user group add and remove operations through the bulk membership path. The flow now applies group defaults consistently on add, and performs cleanup plus trust level recalculation on remove, while publishing membership updates in one pass. sequenceDiagram
participant Caller
participant GroupManager
participant Group
participant Database
participant Jobs
participant EventBus
Caller->>GroupManager: Add or remove group members
GroupManager->>Group: Call bulk add or bulk remove
alt Bulk add path
Group->>Database: Insert memberships and update core user fields
Group->>Database: Apply category and tag notification defaults in bulk
Group->>Jobs: Enqueue trust level grant when configured
Group->>EventBus: Emit user added events and refresh categories
else Bulk remove path
Group->>Database: Delete memberships and clear linked profile fields
Group->>Database: Recompute or clear title when needed
Group->>Jobs: Enqueue trust level recalculation when configured
Group->>EventBus: Emit user removed events and refresh categories
end
Generated by CodeAnt AI |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring of group membership management. The introduction of the GroupManager service object centralizes the logic for adding and removing users from groups, improving maintainability. The performance improvements in bulk_add and bulk_remove, such as using RETURNING user_id and optimizing title updates, are excellent. The test suite has been substantially improved to cover these new complex scenarios. I have a few minor suggestions to improve the robustness of handling user ID parameters.
| if user_id = opts[:user_id] | ||
| builder.where("tu.user_id = :user_id", user_id: user_id) | ||
| end | ||
| user_ids = opts[:user_ids] || opts[:user_id] |
There was a problem hiding this comment.
To improve robustness, it's a good practice to ensure user_ids is always an array. While ActiveRecord's IN clause can handle a single integer from opts[:user_id], explicitly converting to an array with Array() makes the code clearer and safer against future changes.
user_ids = Array(opts[:user_ids] || opts[:user_id])| if user_id = opts[:user_id] | ||
| builder.where("tu.user_id = :user_id", user_id: user_id) | ||
| builder.where2("tu1.user_id = :user_id", user_id: user_id) | ||
| user_ids = opts[:user_ids] || opts[:user_id] |
There was a problem hiding this comment.
Similar to the change in auto_track, let's ensure user_ids is an array for robustness. Using Array(opts[:user_ids] || opts[:user_id]) will handle both single IDs and arrays of IDs gracefully.
user_ids = Array(opts[:user_ids] || opts[:user_id])| if user_id = opts[:user_id] | ||
| builder.where("tu.user_id = :user_id", user_id: user_id) | ||
| end | ||
| user_ids = opts[:user_ids] || opts[:user_id] |
There was a problem hiding this comment.
For consistency and robustness, it would be best to ensure user_ids is an array here as well. Using Array(opts[:user_ids] || opts[:user_id]) will safely handle cases where either a single ID or an array of IDs is passed.
user_ids = Array(opts[:user_ids] || opts[:user_id])| if user_id = opts[:user_id] | ||
| builder.where("tu.user_id = :user_id", user_id: user_id) | ||
| end | ||
| user_ids = opts[:user_ids] || opts[:user_id] |
There was a problem hiding this comment.
To maintain consistency with the other updated methods and improve robustness, let's ensure user_ids is an array here. Array(opts[:user_ids] || opts[:user_id]) is a safe way to handle this.
user_ids = Array(opts[:user_ids] || opts[:user_id])
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| SELECT 1 FROM group_users AS gu | ||
| WHERE gu.user_id = u.id AND | ||
| gu.group_id = :group_id | ||
| ) |
There was a problem hiding this comment.
Suggestion: The NOT EXISTS check in the bulk insert is still race-prone under concurrent requests: two transactions can both pass the check and one will crash with a unique-key violation. Use ON CONFLICT (group_id, user_id) DO NOTHING so concurrent adds are safely deduplicated instead of raising. [race condition]
Severity Level: Critical 🚨
- ❌ PUT /g/:id/owners can fail under concurrent adds.
- ⚠️ Automatic associated-group additions can intermittently error.| SELECT 1 FROM group_users AS gu | |
| WHERE gu.user_id = u.id AND | |
| gu.group_id = :group_id | |
| ) | |
| ON CONFLICT (group_id, user_id) DO NOTHING |
Steps of Reproduction ✅
1. Hit `PUT /g/:id/owners` (route defined at `config/routes.rb:1202`) twice concurrently
for the same `group_id` and `user_id`.
2. Both requests run `GroupsController#add_owners`
(`app/controllers/groups_controller.rb:442-456`), pass `!group.users.include?(user)`, then
call `group.add(user)` at line 454.
3. `Group#add` (`app/models/group.rb:820-823`) delegates to `GroupManager#add`
(`lib/group_manager.rb:10-13`) and then `Group#bulk_add`, whose SQL still uses `WHERE ...
AND NOT EXISTS` (`app/models/group.rb:890-895`).
4. Because `group_users` has a unique index on membership
(`db/migrate/20130416004933_group_users.rb:11`), one concurrent insert can raise
unique-key violation instead of being ignored, causing request failure on this unrescued
path.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** app/models/group.rb
**Line:** 892:895
**Comment:**
*Race Condition: The `NOT EXISTS` check in the bulk insert is still race-prone under concurrent requests: two transactions can both pass the check and one will crash with a unique-key violation. Use `ON CONFLICT (group_id, user_id) DO NOTHING` so concurrent adds are safely deduplicated instead of raising.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| removed_user_ids = group_users_to_remove.map(&:user_id) | ||
|
|
||
| Group.transaction do | ||
| group_users_to_be_destroyed = group_users.includes(:user).where(user_id: user_ids).destroy_all | ||
| group_users_to_be_destroyed.each do |group_user| | ||
| trigger_user_removed_event(group_user.user) | ||
| enqueue_user_removed_from_group_webhook_events(group_user) | ||
| group_users.where(user_id: removed_user_ids).delete_all | ||
|
|
||
| User.where(primary_group_id: self.id, id: removed_user_ids).update_all(primary_group_id: nil) | ||
| User.where(flair_group_id: self.id, id: removed_user_ids).update_all(flair_group_id: nil) | ||
|
|
||
| if self.title.present? | ||
| DB.exec(<<~SQL, user_ids: removed_user_ids, title: self.title) | ||
| UPDATE users u | ||
| SET title = NULL | ||
| WHERE u.id IN (:user_ids) | ||
| AND u.title = :title | ||
| AND NOT EXISTS ( | ||
| SELECT 1 FROM group_users gu | ||
| JOIN groups g ON g.id = gu.group_id | ||
| WHERE gu.user_id = u.id | ||
| AND g.title IS NOT NULL AND g.title <> '' | ||
| ) | ||
| AND NOT EXISTS ( | ||
| SELECT 1 FROM user_badges ub | ||
| JOIN badges b ON b.id = ub.badge_id | ||
| WHERE ub.user_id = u.id | ||
| AND b.allow_title = true | ||
| ) | ||
| SQL | ||
|
|
||
| User | ||
| .where(id: removed_user_ids, title: self.title) | ||
| .find_each { |user| user.update_column(:title, user.next_best_title) } | ||
| end | ||
|
|
||
| recalculate_user_count | ||
| end |
There was a problem hiding this comment.
Suggestion: The code derives removed users before deletion, then always emits side effects for that precomputed list; if another transaction already removed some rows, this method can still clear fields, publish events, and enqueue webhooks for users that were not actually removed in this call. Derive removed_user_ids from DELETE ... RETURNING and short-circuit when nothing was deleted. [race condition]
Severity Level: Major ⚠️
- ⚠️ DELETE /g/:id/members can emit duplicate removal webhooks.
- ⚠️ Unnecessary trust recalculation jobs for already-removed memberships.| removed_user_ids = group_users_to_remove.map(&:user_id) | |
| Group.transaction do | |
| group_users_to_be_destroyed = group_users.includes(:user).where(user_id: user_ids).destroy_all | |
| group_users_to_be_destroyed.each do |group_user| | |
| trigger_user_removed_event(group_user.user) | |
| enqueue_user_removed_from_group_webhook_events(group_user) | |
| group_users.where(user_id: removed_user_ids).delete_all | |
| User.where(primary_group_id: self.id, id: removed_user_ids).update_all(primary_group_id: nil) | |
| User.where(flair_group_id: self.id, id: removed_user_ids).update_all(flair_group_id: nil) | |
| if self.title.present? | |
| DB.exec(<<~SQL, user_ids: removed_user_ids, title: self.title) | |
| UPDATE users u | |
| SET title = NULL | |
| WHERE u.id IN (:user_ids) | |
| AND u.title = :title | |
| AND NOT EXISTS ( | |
| SELECT 1 FROM group_users gu | |
| JOIN groups g ON g.id = gu.group_id | |
| WHERE gu.user_id = u.id | |
| AND g.title IS NOT NULL AND g.title <> '' | |
| ) | |
| AND NOT EXISTS ( | |
| SELECT 1 FROM user_badges ub | |
| JOIN badges b ON b.id = ub.badge_id | |
| WHERE ub.user_id = u.id | |
| AND b.allow_title = true | |
| ) | |
| SQL | |
| User | |
| .where(id: removed_user_ids, title: self.title) | |
| .find_each { |user| user.update_column(:title, user.next_best_title) } | |
| end | |
| recalculate_user_count | |
| end | |
| removed_user_ids = nil | |
| Group.transaction do | |
| removed_user_ids = | |
| DB.query_single( | |
| <<~SQL, | |
| DELETE FROM group_users | |
| WHERE group_id = :group_id | |
| AND user_id IN (:user_ids) | |
| RETURNING user_id | |
| SQL | |
| group_id: self.id, | |
| user_ids: group_users_to_remove.map(&:user_id), | |
| ) | |
| next if removed_user_ids.empty? | |
| User.where(primary_group_id: self.id, id: removed_user_ids).update_all(primary_group_id: nil) | |
| User.where(flair_group_id: self.id, id: removed_user_ids).update_all(flair_group_id: nil) | |
| if self.title.present? | |
| DB.exec(<<~SQL, user_ids: removed_user_ids, title: self.title) | |
| UPDATE users u | |
| SET title = NULL | |
| WHERE u.id IN (:user_ids) | |
| AND u.title = :title | |
| AND NOT EXISTS ( | |
| SELECT 1 FROM group_users gu | |
| JOIN groups g ON g.id = gu.group_id | |
| WHERE gu.user_id = u.id | |
| AND g.title IS NOT NULL AND g.title <> '' | |
| ) | |
| AND NOT EXISTS ( | |
| SELECT 1 FROM user_badges ub | |
| JOIN badges b ON b.id = ub.badge_id | |
| WHERE ub.user_id = u.id | |
| AND b.allow_title = true | |
| ) | |
| SQL | |
| User | |
| .where(id: removed_user_ids, title: self.title) | |
| .find_each { |user| user.update_column(:title, user.next_best_title) } | |
| end | |
| recalculate_user_count | |
| end | |
| return [] if removed_user_ids.blank? | |
| group_users_to_remove.select! { |group_user| removed_user_ids.include?(group_user.user_id) } |
Steps of Reproduction ✅
1. Trigger two concurrent removals for the same member through `DELETE /g/:id/members`
(route at `config/routes.rb:1204`), which executes `GroupsController#remove_member`
(`app/controllers/groups_controller.rb:558-589`) and calls `group.remove(user)` at line
576.
2. `Group#remove` (`app/models/group.rb:829-831`) delegates through `GroupManager#remove`
(`lib/group_manager.rb:15-17`) to `Group#bulk_remove`.
3. In `bulk_remove`, membership is snapshotted before delete (`group_users_to_remove` and
`removed_user_ids` at `app/models/group.rb:962-966`); if another transaction deletes
first, current `delete_all` at line 968 can remove zero rows.
4. Method still executes side effects using stale `removed_user_ids`: clears profile
fields (`970-996`), enqueues trust recalculation (`1002`), publishes category refresh
(`1005`), and emits removal event/webhook (`1007-1010`) for users not actually removed by
this call.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** app/models/group.rb
**Line:** 965:999
**Comment:**
*Race Condition: The code derives removed users before deletion, then always emits side effects for that precomputed list; if another transaction already removed some rows, this method can still clear fields, publish events, and enqueue webhooks for users that were not actually removed in this call. Derive `removed_user_ids` from `DELETE ... RETURNING` and short-circuit when nothing was deleted.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.|
|
||
| it "removes all users from the group upon version change" do | ||
| # TODO: plugin passes an AR relation to group.remove instead of a single user — fix callsite to use GroupManager | ||
| xit "removes all users from the group upon version change" do |
There was a problem hiding this comment.
Suggestion: This change disables a regression test for membership cleanup on policy version change, which allows a known production bug to ship unnoticed. Keep this as an active test so CI fails until the underlying group-removal path is fixed. [logic error]
Severity Level: Critical 🚨
- ❌ Policy version edits can leave stale group memberships.
- ⚠️ CI skips regression check in plugin spec suite.
- ⚠️ Membership-cleanup bug can ship without detection.| xit "removes all users from the group upon version change" do | |
| it "removes all users from the group upon version change" do |
Steps of Reproduction ✅
1. Edit a policy post and change `[policy ... version=2 ...]` so cooked processing hits
`on(:post_process_cooked)` in `plugins/discourse-policy/plugin.rb:71-145`.
2. In that hook, version-change logic at `plugins/discourse-policy/plugin.rb:135-144`
calls `Group.find_by(... )&.remove(previously_accepted_users)`, where
`previously_accepted_users` comes from `post_policy.accepted_policy_users` (an AR
relation, `app/models/post_policy.rb:56-57`).
3. `Group#remove` expects a single user object (`app/models/group.rb:829-832`) and
dereferences `user.id`; passing a relation breaks cleanup behavior for accepted users on
version bump.
4. The only regression spec asserting this path is explicitly skipped (`xit`) at
`plugins/discourse-policy/spec/plugin_spec.rb:121`, so CI never executes this guard and
won't fail when this cleanup path is broken.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** plugins/discourse-policy/spec/plugin_spec.rb
**Line:** 121:121
**Comment:**
*Logic Error: This change disables a regression test for membership cleanup on policy version change, which allows a known production bug to ship unnoticed. Keep this as an active test so CI fails until the underlying group-removal path is fixed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| expect( | ||
| CategoryUser.where(user_id: user2.id, category_id: category1.id).first.notification_level, | ||
| ).to eq(CategoryUser.notification_levels[:watching]) | ||
| end |
There was a problem hiding this comment.
Suggestion: The multi-user category test does not verify category2 for user2, so a bug where tracking defaults are applied only to the first user would still pass. Add the missing assertion so the test actually validates both defaults for both users. [logic error]
Severity Level: Major ⚠️
- ⚠️ Group member bulk-add regressions can bypass test coverage.
- ❌ Category tracking defaults may silently miss later users.
- ⚠️ API path `/groups/:id/members` reliability weakens over refactors.| end | |
| expect( | |
| CategoryUser.where(user_id: user2.id, category_id: category2.id).first.notification_level, | |
| ).to eq(CategoryUser.notification_levels[:tracking]) |
Steps of Reproduction ✅
1. Use the current PR spec at `spec/models/group_user_spec.rb:371-387` (`it "sets category
notifications for multiple users"`), which calls
`GroupUser.bulk_set_category_notifications(group, [user1.id, user2.id])` at line 376.
2. Note the production call path: `PUT /groups/:id/members` route
(`config/routes.rb:1207`) -> `GroupsController#add_members`
(`app/controllers/groups_controller.rb:375-415`) -> `group.add(user)`
(`app/controllers/groups_controller.rb:777-779`) -> `Group#bulk_add`
(`app/models/group.rb:873-957`) -> `GroupUser.bulk_set_category_notifications`
(`app/models/group.rb:947`, `app/models/group_user.rb:212-232`).
3. Introduce a regression in `app/models/group_user.rb:217-223` so category tracking
default is applied only to the first ID in `user_ids` for one category (a realistic
refactor mistake in bulk SQL).
4. Run only this example (`bundle exec rspec
spec/models/group_user_spec.rb:<line_of_test>`). It still passes because assertions at
`spec/models/group_user_spec.rb:378-387` never validate `user2 + category2`, so the
regression is not detected.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** spec/models/group_user_spec.rb
**Line:** 387:387
**Comment:**
*Logic Error: The multi-user category test does not verify `category2` for `user2`, so a bug where tracking defaults are applied only to the first user would still pass. Add the missing assertion so the test actually validates both defaults for both users.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| expect(TagUser.where(user_id: user2.id, tag_id: tag1.id).first.notification_level).to eq( | ||
| TagUser.notification_levels[:watching], | ||
| ) | ||
| end |
There was a problem hiding this comment.
Suggestion: The multi-user tag test also omits the tag2 assertion for user2, so failures that only affect non-first users are not caught. Add the missing expectation to validate bulk behavior across all users and configured tag levels. [logic error]
Severity Level: Major ⚠️
- ⚠️ Bulk tag-default regressions evade spec detection.
- ❌ Later users can miss configured tracking tag defaults.
- ⚠️ Group membership API behavior can drift unnoticed.| end | |
| expect(TagUser.where(user_id: user2.id, tag_id: tag2.id).first.notification_level).to eq( | |
| TagUser.notification_levels[:tracking], | |
| ) |
Steps of Reproduction ✅
1. Use the current PR test `it "sets tag notifications for multiple users"` at
`spec/models/group_user_spec.rb:420-435`, which invokes
`GroupUser.bulk_set_tag_notifications(group, [user1.id, user2.id])` at line 425.
2. Confirm real execution path for affected feature: `PUT /groups/:id/members`
(`config/routes.rb:1207`) -> `GroupsController#add_members`
(`app/controllers/groups_controller.rb:375-415`) -> `Group#bulk_add`
(`app/models/group.rb:873-957`) -> `GroupUser.bulk_set_tag_notifications`
(`app/models/group.rb:948`, `app/models/group_user.rb:234-255`).
3. Introduce a regression in `app/models/group_user.rb:239-246` so one default tag level
is applied only to first user ID during bulk insert/upsert.
4. Run this spec example. It still passes because lines `427-435` assert `user1/tag1`,
`user1/tag2`, and `user2/tag1` only; missing `user2/tag2` lets the bug escape.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** spec/models/group_user_spec.rb
**Line:** 436:436
**Comment:**
*Logic Error: The multi-user tag test also omits the `tag2` assertion for `user2`, so failures that only affect non-first users are not caught. Add the missing expectation to validate bulk behavior across all users and configured tag levels.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
app/jobs/regular/bulk_grant_trust_level.rb (2)
13-21: Consider error resilience for bulk operations.If
Promotion.recalculateorTrustLevelGranter.grantraises an exception for a single user, the entire job fails and remaining users aren't processed. For large user sets, consider whether individual user failures should be logged and skipped rather than halting the batch.💡 Optional: Add per-user error handling
User .where(id: user_ids) .find_each do |user| - if recalculate - Promotion.recalculate(user, use_previous_trust_level: true) - else - TrustLevelGranter.grant(trust_level, user) + begin + if recalculate + Promotion.recalculate(user, use_previous_trust_level: true) + else + TrustLevelGranter.grant(trust_level, user) + end + rescue => e + Rails.logger.warn("BulkGrantTrustLevel failed for user #{user.id}: #{e.message}") end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/jobs/regular/bulk_grant_trust_level.rb` around lines 13 - 21, The bulk loop (User.where(...).find_each) currently lets exceptions from Promotion.recalculate(user, use_previous_trust_level: true) or TrustLevelGranter.grant(trust_level, user) abort the whole job; wrap the per-user work in a begin/rescue so failures for one user are caught, logged (include user.id, the action attempted — Promotion.recalculate or TrustLevelGranter.grant — and the exception message/backtrace) and then next the loop so remaining users continue; optionally increment a failure metric or report to error tracking inside the rescue to surface problematic users.
11-21: Consider documenting the mutually exclusive parameters.When both
trust_levelandrecalculateare provided, therecalculatepath silently takes precedence andtrust_levelis ignored. While the current callers (fromGroupmodel) only pass one or the other, adding a brief comment or explicit validation could prevent confusion for future maintainers.💡 Optional: Add clarifying comment or explicit check
raise Discourse::InvalidParameters.new(:user_ids) if user_ids.blank? raise Discourse::InvalidParameters.new(:trust_level) if trust_level.blank? && !recalculate +# Note: recalculate mode (used when removing users from groups) takes precedence over trust_level grant mode User .where(id: user_ids)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/jobs/regular/bulk_grant_trust_level.rb` around lines 11 - 21, The code silently prefers recalculate over trust_level when both are provided; update bulk_grant_trust_level.rb to make this explicit by either adding a concise comment above the conditional explaining that recalculate takes precedence (mentioning the parameters trust_level and recalculate and the branches Promotion.recalculate(user, use_previous_trust_level: true) vs TrustLevelGranter.grant(trust_level, user)) or add an explicit validation that raises (e.g., Discourse::InvalidParameters) when both trust_level.present? and recalculate are true, so callers cannot accidentally pass both and be surprised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/group.rb`:
- Around line 962-1012: Snapshotting group_users_to_remove before the
transaction can produce stale results; instead perform the removal inside the
transaction with a DELETE ... RETURNING (or SELECT ... FOR UPDATE then delete)
and derive removed_user_ids and the user records from that returned set so all
side-effects (User updates, title adjustments, recalc, Jobs.enqueue,
bulk_publish_category_updates, trigger_user_removed_event,
enqueue_user_removed_from_group_webhook_events) operate only on the actually
deleted memberships; replace the current group_users.where(user_id:
removed_user_ids).delete_all and the precomputed group_users_to_remove with a
DB-level delete that returns the deleted rows, set removed_user_ids =
returned_rows.map(&:user_id) and use returned_rows.map(&:user) for the
subsequent event/webhook/publish calls and for the User update queries and
recalc logic.
- Line 935: The call to recalculate_user_count in the bulk paths must be
replaced with an atomic delta update instead of a snapshot recount: compute the
number of inserted and deleted GroupUser rows within the transaction and call
ActiveRecord::Base.connection.increment_counter / decrement_counter (or
Group.increment!(:user_count, delta) / decrement!(:user_count, delta)) on the
Group record so the change is applied atomically; update the code that currently
calls recalculate_user_count (referenced as recalculate_user_count and the
GroupUser bulk-insert/delete code paths) to derive inserted_count and
deleted_count and apply those deltas inside the same transaction rather than
recomputing the full count.
- Around line 880-896: The INSERT uses a racy NOT EXISTS and must be made safe
for concurrent calls: modify the SQL in Group#bulk_add (the INSERT INTO
group_users SELECT ... WHERE u.id IN (:user_ids) ...) to use Postgres conflict
handling on the unique key instead of relying on NOT EXISTS—specifically target
the unique constraint on (group_id, user_id) (or the index name) and use ON
CONFLICT DO NOTHING so duplicate concurrent inserts are silently ignored; keep
RETURNING user_id as-is so only newly-inserted rows are returned.
In `@app/models/tag_user.rb`:
- Around line 174-175: The current check uses user_ids.present? which leaves an
empty array (user_ids: []) causing no predicate and unbounded updates; update
both spots (in auto_watch and auto_track where user_ids is read from opts and
used in builder.where("tu.user_id IN (:user_ids)", user_ids:)) to explicitly
handle an empty array: if user_ids is an empty Array, add a predicate that
matches no rows (e.g. builder.where('1=0') or equivalent) instead of skipping
the where clause; otherwise keep the existing IN predicate when user_ids has
values.
In `@plugins/discourse-policy/spec/plugin_spec.rb`:
- Around line 120-121: Re-enable the skipped spec (change xit back to it in
plugin_spec.rb) and fix the underlying callsite so we don't pass an ActiveRecord
relation to Group#remove; locate the code that calls group.remove(...) with a
relation and replace it with the GroupManager API (e.g.,
GroupManager.remove_members(group, users) or iterate and call group.remove(user)
for each member) so the removal operates on individual User records rather than
an AR relation.
---
Nitpick comments:
In `@app/jobs/regular/bulk_grant_trust_level.rb`:
- Around line 13-21: The bulk loop (User.where(...).find_each) currently lets
exceptions from Promotion.recalculate(user, use_previous_trust_level: true) or
TrustLevelGranter.grant(trust_level, user) abort the whole job; wrap the
per-user work in a begin/rescue so failures for one user are caught, logged
(include user.id, the action attempted — Promotion.recalculate or
TrustLevelGranter.grant — and the exception message/backtrace) and then next the
loop so remaining users continue; optionally increment a failure metric or
report to error tracking inside the rescue to surface problematic users.
- Around line 11-21: The code silently prefers recalculate over trust_level when
both are provided; update bulk_grant_trust_level.rb to make this explicit by
either adding a concise comment above the conditional explaining that
recalculate takes precedence (mentioning the parameters trust_level and
recalculate and the branches Promotion.recalculate(user,
use_previous_trust_level: true) vs TrustLevelGranter.grant(trust_level, user))
or add an explicit validation that raises (e.g., Discourse::InvalidParameters)
when both trust_level.present? and recalculate are true, so callers cannot
accidentally pass both and be surprised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40914a92-48e9-44f2-9b2b-9d63e5e34982
📒 Files selected for processing (17)
app/jobs/regular/bulk_grant_trust_level.rbapp/models/category_user.rbapp/models/group.rbapp/models/group_user.rbapp/models/tag_user.rblib/group_manager.rbplugins/discourse-data-explorer/spec/guardian_spec.rbplugins/discourse-policy/spec/plugin_spec.rbspec/lib/group_manager_spec.rbspec/lib/guardian_spec.rbspec/lib/post_creator_spec.rbspec/lib/search_spec.rbspec/models/group_spec.rbspec/models/group_user_spec.rbspec/models/user_spec.rbspec/requests/groups_controller_spec.rbspec/serializers/current_user_serializer_spec.rb
| sql = <<~SQL | ||
| INSERT INTO group_users | ||
| (group_id, user_id, created_at, updated_at) | ||
| (group_id, user_id, notification_level, created_at, updated_at) | ||
| SELECT | ||
| #{self.id}, | ||
| :group_id, | ||
| u.id, | ||
| CURRENT_TIMESTAMP, | ||
| CURRENT_TIMESTAMP | ||
| :notification_level, | ||
| :now, | ||
| :now | ||
| FROM users AS u | ||
| WHERE u.id IN (:user_ids) | ||
| AND NOT EXISTS ( | ||
| SELECT 1 FROM group_users AS gu | ||
| WHERE gu.user_id = u.id AND | ||
| gu.group_id = :group_id | ||
| ) | ||
| RETURNING user_id |
There was a problem hiding this comment.
Make bulk_add idempotent under concurrent inserts.
The NOT EXISTS check is racy. Two requests adding the same membership can both pass it, and the loser will hit the unique index on (group_id, user_id), aborting the whole batch. Since Group#add now funnels through this path, duplicate concurrent adds become request failures instead of a no-op.
🐛 Proposed fix
- sql = <<~SQL
- INSERT INTO group_users
- (group_id, user_id, notification_level, created_at, updated_at)
- SELECT
- :group_id,
- u.id,
- :notification_level,
- :now,
- :now
- FROM users AS u
- WHERE u.id IN (:user_ids)
- AND NOT EXISTS (
- SELECT 1 FROM group_users AS gu
- WHERE gu.user_id = u.id AND
- gu.group_id = :group_id
- )
- RETURNING user_id
- SQL
+ sql = <<~SQL
+ INSERT INTO group_users (group_id, user_id, notification_level, created_at, updated_at)
+ SELECT :group_id, u.id, :notification_level, :now, :now
+ FROM users AS u
+ WHERE u.id IN (:user_ids)
+ ON CONFLICT (group_id, user_id) DO NOTHING
+ RETURNING user_id
+ SQL🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/group.rb` around lines 880 - 896, The INSERT uses a racy NOT
EXISTS and must be made safe for concurrent calls: modify the SQL in
Group#bulk_add (the INSERT INTO group_users SELECT ... WHERE u.id IN (:user_ids)
...) to use Postgres conflict handling on the unique key instead of relying on
NOT EXISTS—specifically target the unique constraint on (group_id, user_id) (or
the index name) and use ON CONFLICT DO NOTHING so duplicate concurrent inserts
are silently ignored; keep RETURNING user_id as-is so only newly-inserted rows
are returned.
| end | ||
|
|
||
| # update group user count | ||
| recalculate_user_count |
There was a problem hiding this comment.
Avoid snapshot-based user_count recomputes in the bulk paths.
These methods now bypass the per-row GroupUser counter callbacks, so this recount is the only counter maintenance here. Recounting inside each transaction is race-prone: concurrent add/remove transactions can each compute a count that excludes the other's uncommitted rows, and the last commit wins. Apply the actual inserted/deleted delta atomically instead.
🐛 Proposed fix
- recalculate_user_count
+ if added_user_ids.present?
+ DB.exec(
+ "UPDATE groups SET user_count = user_count + :delta WHERE id = :group_id",
+ group_id: id,
+ delta: added_user_ids.size,
+ )
+ end
...
- recalculate_user_count
+ DB.exec(
+ "UPDATE groups SET user_count = user_count - :delta WHERE id = :group_id",
+ group_id: id,
+ delta: removed_user_ids.size,
+ )Also applies to: 998-998
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/group.rb` at line 935, The call to recalculate_user_count in the
bulk paths must be replaced with an atomic delta update instead of a snapshot
recount: compute the number of inserted and deleted GroupUser rows within the
transaction and call ActiveRecord::Base.connection.increment_counter /
decrement_counter (or Group.increment!(:user_count, delta) /
decrement!(:user_count, delta)) on the Group record so the change is applied
atomically; update the code that currently calls recalculate_user_count
(referenced as recalculate_user_count and the GroupUser bulk-insert/delete code
paths) to derive inserted_count and deleted_count and apply those deltas inside
the same transaction rather than recomputing the full count.
| group_users_to_remove = group_users.includes(:user).where(user_id: user_ids).to_a | ||
| return [] if group_users_to_remove.empty? | ||
|
|
||
| removed_user_ids = group_users_to_remove.map(&:user_id) | ||
|
|
||
| Group.transaction do | ||
| group_users_to_be_destroyed = group_users.includes(:user).where(user_id: user_ids).destroy_all | ||
| group_users_to_be_destroyed.each do |group_user| | ||
| trigger_user_removed_event(group_user.user) | ||
| enqueue_user_removed_from_group_webhook_events(group_user) | ||
| group_users.where(user_id: removed_user_ids).delete_all | ||
|
|
||
| User.where(primary_group_id: self.id, id: removed_user_ids).update_all(primary_group_id: nil) | ||
| User.where(flair_group_id: self.id, id: removed_user_ids).update_all(flair_group_id: nil) | ||
|
|
||
| if self.title.present? | ||
| DB.exec(<<~SQL, user_ids: removed_user_ids, title: self.title) | ||
| UPDATE users u | ||
| SET title = NULL | ||
| WHERE u.id IN (:user_ids) | ||
| AND u.title = :title | ||
| AND NOT EXISTS ( | ||
| SELECT 1 FROM group_users gu | ||
| JOIN groups g ON g.id = gu.group_id | ||
| WHERE gu.user_id = u.id | ||
| AND g.title IS NOT NULL AND g.title <> '' | ||
| ) | ||
| AND NOT EXISTS ( | ||
| SELECT 1 FROM user_badges ub | ||
| JOIN badges b ON b.id = ub.badge_id | ||
| WHERE ub.user_id = u.id | ||
| AND b.allow_title = true | ||
| ) | ||
| SQL | ||
|
|
||
| User | ||
| .where(id: removed_user_ids, title: self.title) | ||
| .find_each { |user| user.update_column(:title, user.next_best_title) } | ||
| end | ||
|
|
||
| recalculate_user_count | ||
| end | ||
|
|
||
| recalculate_user_count | ||
| if self.grant_trust_level.present? && !self.grant_trust_level.zero? | ||
| Jobs.enqueue(:bulk_grant_trust_level, user_ids: removed_user_ids, recalculate: true) | ||
| end | ||
|
|
||
| true | ||
| bulk_publish_category_updates(group_users_to_remove.map(&:user)) | ||
|
|
||
| group_users_to_remove.each do |group_user| | ||
| trigger_user_removed_event(group_user.user) | ||
| enqueue_user_removed_from_group_webhook_events(group_user) | ||
| end | ||
|
|
||
| removed_user_ids |
There was a problem hiding this comment.
Drive removals and side effects from the actual DELETE.
group_users_to_remove is snapshotted before the transaction. If another request removes some of those rows first, delete_all will delete fewer memberships than removed_user_ids implies, but this method will still return those ids and emit user_removed_from_group events/webhooks for them. Use DELETE ... RETURNING (or lock the rows before snapshotting) and derive removed_user_ids, users, and webhook payloads from that result set.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/group.rb` around lines 962 - 1012, Snapshotting
group_users_to_remove before the transaction can produce stale results; instead
perform the removal inside the transaction with a DELETE ... RETURNING (or
SELECT ... FOR UPDATE then delete) and derive removed_user_ids and the user
records from that returned set so all side-effects (User updates, title
adjustments, recalc, Jobs.enqueue, bulk_publish_category_updates,
trigger_user_removed_event, enqueue_user_removed_from_group_webhook_events)
operate only on the actually deleted memberships; replace the current
group_users.where(user_id: removed_user_ids).delete_all and the precomputed
group_users_to_remove with a DB-level delete that returns the deleted rows, set
removed_user_ids = returned_rows.map(&:user_id) and use
returned_rows.map(&:user) for the subsequent event/webhook/publish calls and for
the User update queries and recalc logic.
| user_ids = opts[:user_ids] || opts[:user_id] | ||
| builder.where("tu.user_id IN (:user_ids)", user_ids:) if user_ids.present? |
There was a problem hiding this comment.
Guard explicit empty user_ids to avoid unbounded updates.
At Lines 174 and 208, user_ids: [] results in no tu.user_id predicate, which can update all users instead of no users in bulk paths.
🛠️ Proposed fix
- user_ids = opts[:user_ids] || opts[:user_id]
- builder.where("tu.user_id IN (:user_ids)", user_ids:) if user_ids.present?
+ if opts.key?(:user_ids)
+ user_ids = Array(opts[:user_ids]).compact
+ return if user_ids.empty?
+ elsif opts[:user_id].present?
+ user_ids = [opts[:user_id]]
+ end
+ builder.where("tu.user_id IN (:user_ids)", user_ids: user_ids) if user_idsApply the same pattern in both auto_watch and auto_track.
Also applies to: 208-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/tag_user.rb` around lines 174 - 175, The current check uses
user_ids.present? which leaves an empty array (user_ids: []) causing no
predicate and unbounded updates; update both spots (in auto_watch and auto_track
where user_ids is read from opts and used in builder.where("tu.user_id IN
(:user_ids)", user_ids:)) to explicitly handle an empty array: if user_ids is an
empty Array, add a predicate that matches no rows (e.g. builder.where('1=0') or
equivalent) instead of skipping the where clause; otherwise keep the existing IN
predicate when user_ids has values.
| # TODO: plugin passes an AR relation to group.remove instead of a single user — fix callsite to use GroupManager | ||
| xit "removes all users from the group upon version change" do |
There was a problem hiding this comment.
Don’t skip this regression test; it currently masks a real membership-removal bug.
Switching to xit on Line 121 suppresses coverage for a behavior that can leave previously accepted users in the group after policy version changes.
💡 Proposed fix
# plugins/discourse-policy/spec/plugin_spec.rb
- xit "removes all users from the group upon version change" do
+ it "removes all users from the group upon version change" do# plugins/discourse-policy/plugin.rb
- previously_accepted_users = post_policy.accepted_policy_users
- Group.find_by(id: post_policy.add_users_to_group)&.remove(previously_accepted_users)
+ if (group = Group.find_by(id: post_policy.add_users_to_group))
+ user_ids = post_policy.accepted_policy_users.pluck(:user_id)
+ GroupManager.new(group).remove(user_ids)
+ end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # TODO: plugin passes an AR relation to group.remove instead of a single user — fix callsite to use GroupManager | |
| xit "removes all users from the group upon version change" do | |
| # TODO: plugin passes an AR relation to group.remove instead of a single user — fix callsite to use GroupManager | |
| it "removes all users from the group upon version change" do |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/discourse-policy/spec/plugin_spec.rb` around lines 120 - 121,
Re-enable the skipped spec (change xit back to it in plugin_spec.rb) and fix the
underlying callsite so we don't pass an ActiveRecord relation to Group#remove;
locate the code that calls group.remove(...) with a relation and replace it with
the GroupManager API (e.g., GroupManager.remove_members(group, users) or iterate
and call group.remove(user) for each member) so the removal operates on
individual User records rather than an AR relation.
2 participants
CodeAnt-AI Description
Speed up group membership changes and apply group settings consistently
What Changed
Impact
✅ Faster large group updates✅ Fewer missed group notification settings✅ More accurate titles after group changes💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Release Notes
New Features
Refactor