FIX: proper handling of group memberships

[zaibkhan]

No description provided.

[codoki-pr-intelligence]

Codoki PR Review

Summary: Load group members correctly, bind to controller, prevent empty list
What’s good: Routing logic is simplified by removing intermediate _members caching and consistently invoking model.findMembers() in setupController.
Review Status: ❌ Requires changes
Overall Priority: High
Review Update:
• Coverage: Reviewed all 16 files across 2 batches

Issues (Critical & High only)

Severity	Issue	Why it matters
High	Correctness — Misleading 422 after successful member removal due to save	…/admin/groups_controller.rb Calling save after deleting through a HABTM/has_many-through association can cause the response to incorrectly return 422 if unrelated group validations fail, even though the membership was already removed (join row deleted). This creates inconsistent state and can lead clients to retry a 'failed' removal that already succeeded.
High	Correctness — add_members may return 422 despite persisting changes	…/admin/groups_controller.rb Similar to remove_member, saving the Group after adding members via join operations risks returning 422 on unrelated validations while changes may have already been applied (e.g., some users added). This produces partial success masked as failure.
High	Correctness — Unclamped negative offset can cause server error	…/controllers/groups_controller.rb A negative offset (e.g., offset=-5) can be passed externally and may raise an error in ActiveRecord/Pg (offset must be >= 0), causing a 500. The client clamps offset, but the server should defensively clamp as well.
High	Correctness — Members not bound to controller leads to empty list	…/routes/admin_group_route.js The template expects a 'members' collection, but this route no longer sets controller.members; simply calling findMembers() without binding its result will render an empty list. Either set controller.members from the resolved promise or update templates to iterate model.members if findMembers mutates the model.
High	Correctness — Template expects 'members' but route never sets it	…/routes/group-members.js.es6 The group/members.hbs template now iterates 'members', but this route sets only the model and does not populate controller.members. Without binding the fetched array, the UI will show no members until some other side-effect occurs.

Showing top 5 issues. Critical: 0, High: 5. See inline suggestions for more.

Key Feedback (click to expand)

• Needs improvement: Bind the fetched members to the controller (or update templates to use model.members) to avoid rendering an empty list; also consider basic error handling on the promise.
• Testing: Add an acceptance/integration test for both admin and public group members routes that stubs model.findMembers to resolve with a known array and asserts that the UI renders those members (and hides the remove button for automatic groups). Include a negative test for a rejected promise to verify fallback UI.
• Documentation: Document the contract of Group#findMembers (returns a promise, whether it mutates model.members) and which controller/template property is the source of truth (controller.members vs model.members).
• Compatibility: If templates are intended to iterate members directly, ensure all routes set controller.members; otherwise standardize on model.members across routes/templates for consistency.
• Performance: No material performance regressions in the diff; removing duplicate caching of _members is positive.
• Security: No new security surfaces apparent in these changes.
• Open questions: Does Group#findMembers populate group.members? If so, should templates use model.members instead of members to avoid duplicating state on the controller?

Confidence: 2/5 — Not ready to merge (5 high · status: Requires changes)

Sequence Diagram

sequenceDiagram
    participant AdminGroupRoute
    participant Controller
    participant GroupModel
    AdminGroupRoute->>GroupModel: setupController(model)
    AdminGroupRoute->>Controller: set("model", model)
    AdminGroupRoute->>GroupModel: findMembers()
    Note over GroupModel: returns Promise of members

Loading

React with 👍 or 👎 if you found this review useful.

[codoki-pr-intelligence]

⚠️ High: Calling save after deleting through a HABTM/has_many-through association can cause the response to incorrectly return 422 if unrelated group validations fail, even though the membership was already removed (join row deleted). This creates inconsistent state and can lead clients to retry a 'failed' removal that already succeeded.

Suggested change

	if group.save
	```suggestion
	# membership deletion persists immediately; avoid unrelated validation failures
	render json: success_json

[codoki-pr-intelligence]

⚠️ High: Similar to remove_member, saving the Group after adding members via join operations risks returning 422 on unrelated validations while changes may have already been applied (e.g., some users added). This produces partial success masked as failure.

Suggested change

	if group.save
	```suggestion
	# adding members via the association persists; avoid unrelated validation failures
	render json: success_json

[codoki-pr-intelligence]

⚠️ High: A negative offset (e.g., offset=-5) can be passed externally and may raise an error in ActiveRecord/Pg (offset must be >= 0), causing a 500. The client clamps offset, but the server should defensively clamp as well.

Suggested change

	offset = params[:offset].to_i
	```suggestion
	offset = [params[:offset].to_i, 0].max

[codoki-pr-intelligence]

⚠️ High: The template expects a 'members' collection, but this route no longer sets controller.members; simply calling findMembers() without binding its result will render an empty list. Either set controller.members from the resolved promise or update templates to iterate model.members if findMembers mutates the model.

Suggested change

	model.findMembers();
	model.findMembers().then(function(members) { controller.set('members', members); });

[codoki-pr-intelligence]

⚠️ High: The group/members.hbs template now iterates 'members', but this route sets only the model and does not populate controller.members. Without binding the fetched array, the UI will show no members until some other side-effect occurs.

Suggested change

	model.findMembers();
	model.findMembers().then(function(members) { controller.set('members', members); });