FIX: proper handling of group memberships #3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Author
Greptile OverviewConfidence Score: 2/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Admin as Admin UI
participant Controller as admin-group.js.es6
participant Model as group.js
participant Backend as groups_controller.rb
participant DB as Database
Note over Admin,DB: Add Member Flow
Admin->>Controller: addMembers(usernames)
Controller->>Model: addMembers(usernames)
Model->>Backend: PUT /admin/groups/:id/members.json
Backend->>DB: group.add(user)
Backend-->>Model: success_json
Model->>Backend: GET /groups/:name/members.json
Backend->>DB: group.users.limit().offset()
Backend-->>Model: {members, meta}
Model-->>Controller: Updated members list
Controller-->>Admin: UI refresh
Note over Admin,DB: Remove Member Flow
Admin->>Controller: removeMember(member)
Controller->>Model: removeMember(member)
Model->>Backend: DELETE /admin/groups/:id/members.json
Backend->>DB: group.users.delete(user_id)
Backend-->>Model: success_json
Model->>Backend: GET /groups/:name/members.json
Backend->>DB: group.users.limit().offset()
Backend-->>Model: {members, meta}
Model-->>Controller: Updated members list
Controller-->>Admin: UI refresh
Note over Admin,DB: Pagination Flow
Admin->>Controller: next() or previous()
Controller->>Model: set offset, findMembers()
Model->>Backend: GET /groups/:name/members.json?limit&offset
Backend->>DB: group.users.limit().offset()
Backend-->>Model: {members, meta}
Model-->>Controller: Updated page
Controller-->>Admin: Display new page
|
| context ".remove_member" do | ||
|
|
||
| it "cannot remove members from automatic groups" do | ||
| xhr :put, :remove_member, group_id: 1, user_id: 42 |
There was a problem hiding this comment.
syntax: wrong HTTP method - test uses PUT but route expects DELETE
Suggested change
| xhr :put, :remove_member, group_id: 1, user_id: 42 | |
| xhr :delete, :remove_member, group_id: 1, user_id: 42 |
Prompt To Fix With AI
This is a comment left during a code review.
Path: spec/controllers/admin/groups_controller_spec.rb
Line: 115:115
Comment:
**syntax:** wrong HTTP method - test uses `PUT` but route expects `DELETE`
```suggestion
xhr :delete, :remove_member, group_id: 1, user_id: 42
```
How can I resolve this? If you propose a fix, please make it concise.| }, | ||
|
|
||
| destroy: function(){ | ||
| if (!this.get('id')) { return }; |
There was a problem hiding this comment.
logic: missing return statement - breaks promise chain
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/assets/javascripts/discourse/models/group.js
Line: 82:82
Comment:
**logic:** missing return statement - breaks promise chain
How can I resolve this? If you propose a fix, please make it concise.| if (this.get("showingLast")) { return; } | ||
|
|
||
| var group = this.get("model"), | ||
| offset = Math.min(group.get("offset") + group.get("limit"), group.get("user_count")); |
There was a problem hiding this comment.
logic: next calculation is incorrect - when on last page boundary with exact multiple of limit, this allows incrementing past total count
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/assets/javascripts/admin/controllers/admin-group.js.es6
Line: 33:33
Comment:
**logic:** `next` calculation is incorrect - when on last page boundary with exact multiple of limit, this allows incrementing past total count
How can I resolve this? If you propose a fix, please make it concise.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Test 8
Replicated from ai-code-review-evaluation/discourse-greptile#8