refactor(membership): make SetGroupMemberRole an upsert by whoAbhishekSah · Pull Request #1603 · raystack/frontier

whoAbhishekSah · 2026-05-13T05:30:24Z

Summary

SetGroupMemberRole now upserts: it adds the principal as a group member if they don't already have a group policy, or changes their role if they do. New adds validate that the principal is a member of the parent organization. The min-owner constraint still guards demotions.

This unifies add and role-change into a single public RPC so the SDK can use one mutation for group membership writes, removing the need to expose AddGroupMember as a proto RPC.

Why

While planning the SDK migration off the legacy AddGroupUsers RPC, we hit the question: do we expose a new AddGroupMember RPC, or relax SetGroupMemberRole to upsert? Going with upsert because:

One public RPC for membership writes is simpler from the SDK's perspective.
The strict-set check (ErrNotMember) wasn't load-bearing for any caller.
AddGroupMember (service-only) is unchanged and continues to be used internally by OnGroupCreated for the group-create owner add, where strict semantics make sense.

Test plan

go build ./...
go test ./core/membership/... ./internal/api/v1beta1connect/...
gofmt -l clean

Follow-ups

SDK migration off legacy AddGroupUsers → loops SetGroupMemberRole.
Frontier PR 3 (original plan): migrate AddGroupUsers handler to membership, delete AddMember/AddUsers/addMemberPolicy from core/group/service.go.

vercel · 2026-05-13T05:30:28Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
frontier	Ready	Preview, Comment	May 13, 2026 5:38am

coderabbitai · 2026-05-13T05:30:35Z

Warning

Rate limit exceeded

@whoAbhishekSah has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 52 minutes and 36 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3f8bd77f-a6fd-439e-8b7e-75ef5f42faad

📥 Commits

Reviewing files that changed from the base of the PR and between 9f84bde and e774125.

📒 Files selected for processing (4)

core/membership/service.go
core/membership/service_test.go
internal/api/v1beta1connect/group.go
internal/api/v1beta1connect/group_test.go

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coveralls · 2026-05-13T05:36:41Z

Coverage Report for CI Build 25780601827

Coverage decreased (-0.003%) to 42.288%

Details

Coverage decreased (-0.003%) from the base build.
Patch coverage: 11 uncovered changes across 1 file (10 of 21 lines covered, 47.62%).
No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
core/membership/service.go	19	8	42.11%

Coverage Regressions

No coverage regressions found.

Coverage Stats


Relevant Lines:	37559
Covered Lines:	15883
Line Coverage:	42.29%
Coverage Strength:	11.9 hits per line

💛 - Coveralls

SetGroupMemberRole now adds a new member when the principal has no existing group policy, and changes the role otherwise. New adds validate that the principal is a member of the parent organization. The min-owner constraint still applies to demotions. This unifies add and role-change into one public RPC so the SDK can use a single mutation for group membership writes, eliminating the need to expose AddGroupMember as a proto RPC. - Drops ErrNotMember from the SetGroupMemberRole path; ErrNotOrgMember surfaces instead when an upsert-add targets a non-org-member. - Handler error mapping updated; new ErrNotOrgMember Connect mapping. - New unit tests cover both upsert paths: add (with org-member check) and the existing change paths remain intact. AddGroupMember (service-only) is unchanged and continues to be used internally by OnGroupCreated, where the creator is always a fresh add. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Make explicit that both paths use the requested role: add with role on a new add, replace existing role with the requested role on a change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

whoAbhishekSah · 2026-05-13T05:47:22Z

Manual Testing Results - SetGroupMemberRole Upsert

Tested the new upsert behavior where SetGroupMemberRole can now add new members (not just change existing ones).

Test Environment

Org: pr1596-b557d6
Group1: Alice (owner), Bob, Charlie (members)
Dave: In org, NOT in group (test subject for upsert add)
Eve: NOT in org

New Upsert Add Behavior

#	Test Case	Expected	Actual	Status
1	Add Dave to group via SetGroupMemberRole	`{}` (success)	`{}`	✅ Pass
2	Verify Dave in member list	Dave listed	Dave listed	✅ Pass
3	Promote Dave to owner (role change)	`{}` (success)	`{}`	✅ Pass
4	Dave can update group (owner perms)	Success	Success	✅ Pass
5	Add Charlie to group2 directly as owner	`{}` (success)	`{}`	✅ Pass
6	Idempotent - set same role again	`{}` (success)	`{}`	✅ Pass

Unhappy Path (Constraints Preserved)

#	Test Case	Expected	Actual	Status
7	Add Eve (not in org)	`failed_precondition`	`principal is not a member of the organization`	✅ Pass
8	Demote last owner	`failed_precondition`	`group must have at least one owner...`	✅ Pass
9	Invalid principal type	`invalid_argument`	`unsupported principal type`	✅ Pass
10	Non-existent user	`not_found`	`not_found`	✅ Pass

Summary

Category	Passed	Total
Upsert Add	6	6
Unhappy Path	4	4
Total	10	10

The upsert behavior works correctly:

New members can be added with any valid group role
Existing members' roles can still be changed
Org membership validation enforced for new adds
Last owner constraint still protected

After the SetGroupMemberRole upsert in #1603, AddGroupMember and the add-path of SetGroupMemberRole did the same work. The three callers (OnGroupCreated, AddGroupUsers handler, invitation accept) are all fresh-add sites that don't need the strict ErrAlreadyMember signal — they're either guaranteed-new (group create) or already pre-filtered (invitation accept) or fine with idempotent semantics (AddGroupUsers). Switching them to SetGroupMemberRole makes the writes idempotent and removes a near-duplicate function. The "added" vs "role_changed" audit distinction is preserved by SetGroupMemberRole's own branching, so audit semantics are unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Replaces three legacy addGroupUsers call sites with the upsert setGroupMemberRole RPC, which adds the principal as a group member with the requested role. The app_group_member role UUID is resolved once from listRoles({scopes: ["app/group"]}) and passed down to each add-member component. - add-member-menu.tsx (views-new): receives roles from team-details-view, derives memberRoleId, swaps the addGroupUsers mutation for setGroupMemberRole with PERMISSIONS.UserPrincipal + memberRoleId. - team-members.tsx (legacy view): same swap; AddMemberDropdown now takes roles as a prop and derives memberRoleId internally. - invite-team-member-dialog.tsx: drops the two-step addGroupUsers + createPolicy workaround entirely. The dialog already collects the chosen role from the user; we now hand that role directly to setGroupMemberRole in one call. Depends on #1603 being deployed to production (SetGroupMemberRole upsert) — before that, the new SDK calls would hit the strict-set rejection for not-yet-members. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

vercel Bot deployed to Preview May 13, 2026 05:33 View deployment

whoAbhishekSah and others added 2 commits May 13, 2026 11:07

docs(membership): clarify SetGroupMemberRole upsert doc

e774125

Make explicit that both paths use the requested role: add with role on a new add, replace existing role with the requested role on a change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

whoAbhishekSah force-pushed the refactor/setgroupmemberrole-upsert branch from bb23f69 to e774125 Compare May 13, 2026 05:37

vercel Bot deployed to Preview May 13, 2026 05:38 View deployment

whoAbhishekSah requested a review from rohilsurana May 13, 2026 09:35

AmanGIT07 approved these changes May 13, 2026

View reviewed changes

whoAbhishekSah merged commit 0f0bc2d into main May 13, 2026
8 checks passed

whoAbhishekSah deleted the refactor/setgroupmemberrole-upsert branch May 13, 2026 11:00

whoAbhishekSah mentioned this pull request May 14, 2026

refactor(group): migrate AddGroupUsers handler to membership #1608

Merged

4 tasks

whoAbhishekSah mentioned this pull request May 14, 2026

refactor(sdk): migrate group add-member flows to setGroupMemberRole #1609

Merged

2 tasks

whoAbhishekSah mentioned this pull request May 14, 2026

chore(sdk-admin): list new group member webhook events #1613

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(membership): make SetGroupMemberRole an upsert#1603

refactor(membership): make SetGroupMemberRole an upsert#1603
whoAbhishekSah merged 2 commits into
mainfrom
refactor/setgroupmemberrole-upsert

whoAbhishekSah commented May 13, 2026 •

edited

Loading

Uh oh!

vercel Bot commented May 13, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented May 13, 2026 •

edited

Loading

Rate limit exceeded

Uh oh!

coveralls commented May 13, 2026 •

edited

Loading

Uh oh!

whoAbhishekSah commented May 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

whoAbhishekSah commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why

Test plan

Follow-ups

Uh oh!

vercel Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rate limit exceeded

Uh oh!

coveralls commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 25780601827

Coverage decreased (-0.003%) to 42.288%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

whoAbhishekSah commented May 13, 2026

Manual Testing Results - SetGroupMemberRole Upsert

Test Environment

New Upsert Add Behavior

Unhappy Path (Constraints Preserved)

Summary

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

whoAbhishekSah commented May 13, 2026 •

edited

Loading

vercel Bot commented May 13, 2026 •

edited

Loading

coderabbitai Bot commented May 13, 2026 •

edited

Loading

coveralls commented May 13, 2026 •

edited

Loading