feat(teams) support tiers team members#2175
Merged
whutchinson98 merged 16 commits intomainfrom Mar 26, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new PostgreSQL enum type Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/cloud-storage/teams/src/domain/team_service.rs (1)
340-348:⚠️ Potential issue | 🟠 MajorMirror the new tier grant in
delete_team.Joining now adds
RoleId::from(team_member.tier), butdelete_teamstill strips onlyTeamSubscriber. Non-owner members whose access came solely from this team will retainSubHaiku/SubSonnet/SubOpusafter the team is deleted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/teams/src/domain/team_service.rs` around lines 340 - 348, When deleting a team in delete_team, also remove the per-tier role that was granted on join (RoleId::from(team_member.tier)) in addition to RoleId::TeamSubscriber; for each non-owner team_member mirror the join logic that builds roles (the vec![RoleId::TeamSubscriber, RoleId::from(team_member.tier)] wrapped into non_empty::NonEmpty) and call the same user role service to remove those roles (use the complementary dangerous remove/upsert call you have for removals, e.g., the inverse of dangerous_upsert_roles_for_user) so members who only had access via the team no longer retain SubHaiku/SubSonnet/SubOpus after deletion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/roles_and_permissions/src/domain/model.rs`:
- Around line 64-85: The FromStr impl for RoleId (and the other FromStr
implementations referenced) should use anyhow::bail! for the error branch
instead of returning Err(anyhow::anyhow!(...)); replace the final arm `_ =>
Err(anyhow::anyhow!("unknown role id: {s}"))` with a call to
anyhow::bail!("unknown role id: {s}") in the FromStr for RoleId (and do the same
change in the other FromStr impls at the noted locations) so the function exits
with the idiomatic early-return macro.
In `@rust/cloud-storage/roles_and_permissions/src/domain/service/test.rs`:
- Around line 141-161: The test name claims delegation but only asserts returned
roles; either rename the test to reflect it verifies return values (e.g.,
test_get_user_roles_returns_repository_roles) or enhance the mock to assert
delegation: add a call-capture field and implementation for
MockUserRolesAndPermissionsRepository::get_user_roles (similar to
add_roles_to_user_calls), call
UserRolesAndPermissionsServiceImpl::get_user_roles with
MacroUserIdStr::parse_from_str(...), then assert the mock recorded exactly one
invocation and the captured user_id matches the passed ID; update assertions
accordingly.
In `@rust/cloud-storage/roles_and_permissions/src/outbound/pgpool.rs`:
- Around line 140-159: In get_user_roles replace the unsafe .parse().unwrap() on
role_id with safe handling: map the DB row to an Option<RoleId> by attempting
RoleId parsing and either filter_map to skip invalid entries while emitting a
warning via tracing::warn! (include the raw role string and user_id) or return a
descriptive UserRolesAndPermissionsError if you prefer strictness; ensure the
query mapping uses the same column alias ("role_id") and that any parsing
failure is converted into a logged warning or error rather than panicking so
RolesOnUsers inconsistencies don't crash the process.
In `@rust/cloud-storage/teams/src/domain/model.rs`:
- Around line 46-58: The current try_from_roles in TeamUserTier fails on legacy
paid roles (e.g., ProfessionalSubscriber, Corporate) and picks the lowest Sub*
when multiple roles exist; update try_from_roles to first normalize legacy roles
into their Sub* equivalents (map ProfessionalSubscriber/Corporate/etc. to
RoleId::SubOpus/SubSonnet/SubHaiku as appropriate), then compute the effective
tier by selecting the highest matching Sub* (prefer Opus over Sonnet over Haiku)
from the normalized HashSet<RoleId>; ensure the function returns
Ok(TeamUserTier::Opus|Sonnet|Haiku) when any normalized Sub* is present and only
bails if none match.
In `@rust/cloud-storage/teams/src/domain/team_service.rs`:
- Around line 236-243: The current logic uses is_user_member_of_team(user_id) to
decide removing tier-specific roles, which fails when a user leaves one team but
still belongs to other teams of different tiers; instead, after removing the
user from the team you must recompute the user's remaining highest team tier and
remove/downgrade roles accordingly. Update the block in team_service.rs
(referencing is_user_member_of_team, RoleId::TeamSubscriber, RoleId::from(tier),
and dangerous_remove_roles_from_user) to: fetch the user's remaining teams
(excluding the team being left), compute the highest remaining Tier enum, map
that to the set of roles the user should retain, then call
user_roles_and_permissions_service.dangerous_remove_roles_from_user with only
the roles that must be removed (or call the appropriate add/remove methods to
downgrade), and apply the same change in revoke_permissions_for_team_members to
recompute per-member tiers rather than relying on the membership boolean.
- Around line 354-403: The current get_team_subscription_id →
create_subscription → update_team_subscription flow races for first joins; fix
by doing a compare-and-set when persisting the subscription (use
team_repository.update_team_subscription CAS variant that updates WHERE id = $1
AND subscription_id IS NULL and return whether the update succeeded) and by
adding a team-scoped idempotency key to customer_repository.create_subscription
(eg. "team-{team_id}-create-subscription") so Stripe deduplicates concurrent
requests; implement logic in join flow to: re-check get_team_subscription_id, if
none call create_subscription with the idempotency key, then attempt the CAS
update and if the CAS fails fetch the existing subscription_id and call
increase_subscription_quantity instead of abandoning or creating another
subscription.
---
Outside diff comments:
In `@rust/cloud-storage/teams/src/domain/team_service.rs`:
- Around line 340-348: When deleting a team in delete_team, also remove the
per-tier role that was granted on join (RoleId::from(team_member.tier)) in
addition to RoleId::TeamSubscriber; for each non-owner team_member mirror the
join logic that builds roles (the vec![RoleId::TeamSubscriber,
RoleId::from(team_member.tier)] wrapped into non_empty::NonEmpty) and call the
same user role service to remove those roles (use the complementary dangerous
remove/upsert call you have for removals, e.g., the inverse of
dangerous_upsert_roles_for_user) so members who only had access via the team no
longer retain SubHaiku/SubSonnet/SubOpus after deletion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4c16dfb1-2872-4f14-aef1-5edd5f03211b
📒 Files selected for processing (28)
rust/cloud-storage/macro_db_client/migrations/20260325131311_team_user_tier.sqlrust/cloud-storage/macro_db_client/migrations/20260325154744_team_invite_tier.sqlrust/cloud-storage/roles_and_permissions/.sqlx/query-8717ae7a285b36ee3b82b3688a274b9734ecd81e4e1e1d0f7a1fc3a7d2c2c053.jsonrust/cloud-storage/roles_and_permissions/src/domain/model.rsrust/cloud-storage/roles_and_permissions/src/domain/port.rsrust/cloud-storage/roles_and_permissions/src/domain/port/test.rsrust/cloud-storage/roles_and_permissions/src/domain/service.rsrust/cloud-storage/roles_and_permissions/src/domain/service/test.rsrust/cloud-storage/roles_and_permissions/src/outbound/pgpool.rsrust/cloud-storage/roles_and_permissions/src/outbound/pgpool/test.rsrust/cloud-storage/teams/.sqlx/query-0f2057f8430788bb20f5d0f144445f28fba6434c9a2fe208e0a4a45b892dea2b.jsonrust/cloud-storage/teams/.sqlx/query-14ebe9e55b31c7dc4aec3553cff79e80942169ef55c62a990d3bbd54389e681a.jsonrust/cloud-storage/teams/.sqlx/query-1bf9b2bf67d4780b41454d87c4a1995677a162dd11af0b9d77df947b657cd651.jsonrust/cloud-storage/teams/.sqlx/query-4f26a2f6044225dd1019390579a3652eafdf1db2ae15212335ebd5ecbb3b4ebe.jsonrust/cloud-storage/teams/.sqlx/query-50332c40ef917c839c4539f12419035115d61a4c9e8146a894e41db59b0f0daa.jsonrust/cloud-storage/teams/.sqlx/query-a3a570d4857e91eb55d5e4299f9a856e70e71c0a070f0a1a005ba5e2aa7eabac.jsonrust/cloud-storage/teams/.sqlx/query-b36669573fe9b78417dfafd57157822e24246a4ad2f6fb4f2366aa5af643cf58.jsonrust/cloud-storage/teams/.sqlx/query-b470add8d87f6fff56d158c2aa9bf5dd871973fd3b9b6e1f25033f24a2cde3bd.jsonrust/cloud-storage/teams/.sqlx/query-bcb0dfd2aa420f102d8dd3484f65defd6cbcf8e801c4c4d43d5e0d1338e1ab04.jsonrust/cloud-storage/teams/.sqlx/query-d1f79f2a50de2167f06ac49f52b134ccc3b8eeb070e7fd710b18dbbfc783aa1d.jsonrust/cloud-storage/teams/.sqlx/query-d5ffa82610d978dab83826ea1dfe5c5fdba0126759ea99c27f91555adab1480d.jsonrust/cloud-storage/teams/.sqlx/query-d9a13a9704a1f510f984862914c0fda59b2d50ec3e26b9bf2659cc8b85e4f5a1.jsonrust/cloud-storage/teams/fixtures/teams.sqlrust/cloud-storage/teams/src/domain/model.rsrust/cloud-storage/teams/src/domain/team_repo.rsrust/cloud-storage/teams/src/domain/team_service.rsrust/cloud-storage/teams/src/outbound/team_repo.rsrust/cloud-storage/teams/src/outbound/team_repo/test.rs
💤 Files with no reviewable changes (4)
- rust/cloud-storage/teams/.sqlx/query-14ebe9e55b31c7dc4aec3553cff79e80942169ef55c62a990d3bbd54389e681a.json
- rust/cloud-storage/teams/.sqlx/query-1bf9b2bf67d4780b41454d87c4a1995677a162dd11af0b9d77df947b657cd651.json
- rust/cloud-storage/teams/.sqlx/query-bcb0dfd2aa420f102d8dd3484f65defd6cbcf8e801c4c4d43d5e0d1338e1ab04.json
- rust/cloud-storage/teams/.sqlx/query-b36669573fe9b78417dfafd57157822e24246a4ad2f6fb4f2366aa5af643cf58.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/cloud-storage/teams/src/domain/team_service.rs (1)
367-396:⚠️ Potential issue | 🔴 CriticalDon't consume the invite before the seat bump succeeds.
accept_team_invite, role upsert, and channel membership all happen before the Stripe increment. If Lines 387-396 fail, the invite is already consumed and the user keeps team access, but the subscription quantity stays unchanged with no clean retry path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/teams/src/domain/team_service.rs` around lines 367 - 396, Don't consume the invite before ensuring the Stripe seat bump: obtain the team/subscription identifier first (e.g., add or use a method to peek the invite/team like team_repository.get_team_invite or otherwise fetch team_id/subscription via get_team_subscription without calling accept_team_invite), parse it to stripe::SubscriptionId and call customer_repository.increase_subscription_quantity(&subscription_id, 1) and ensure it succeeds; only after the subscription quantity is incremented, call team_repository.accept_team_invite(team_invite_id, user_id) and then dangerous_upsert_roles_for_user and team_channels_repository.add_team_member_to_channels; if no existing peek method exists, add one so you can read team_id/subscription before calling accept_team_invite so failures in increase_subscription_quantity do not leave the invite consumed.
♻️ Duplicate comments (1)
rust/cloud-storage/teams/src/domain/team_service.rs (1)
275-281:⚠️ Potential issue | 🟠 MajorRecompute entitlements instead of keying off “any other team”.
RoleId::from(TeamUserTier)maps to the sameSubHaiku/SubSonnet/SubOpusroles used elsewhere inrust/cloud-storage/teams/src/domain/model.rs:17-60. These removal paths still only ask whether the user belongs to some other team, so they can leave stale higher-tier access behind, and they can also remove a valid personalSub*entitlement;delete_teamstill only stripsTeamSubscriber, so it has the same stale-role problem. Recompute the user's post-change role set from remaining teams plus any personal subscription before callingdangerous_remove_roles_from_user.Also applies to: 437-443
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/teams/src/domain/team_service.rs` around lines 275 - 281, The current removal logic in TeamService (the block calling is_user_member_of_team(...).await? and then dangerous_remove_roles_from_user(...)) and the similar block used by delete_team incorrectly keys off “any other team”; instead, compute the user's new entitlement set by querying their remaining team memberships and any personal subscription before removing roles. Update the code paths in team_service.rs (the is_user_member_of_team check and the delete_team flow) to: fetch the list of remaining teams for user_id, derive the correct RoleId set (including SubHaiku/SubSonnet/SubOpus from remaining teams plus any personal subscription), build a NonEmpty role list only for roles that should be removed, and then call dangerous_remove_roles_from_user with that recomputed set so you neither leave stale higher-tier roles nor remove valid personal Sub* entitlements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/teams/src/domain/customer_repo.rs`:
- Around line 9-21: The impl for CustomerRepositoryImpl is missing the trait
methods convert_subscription_to_team and get_subscription_id_for_customer; add
these two methods to the existing impl block for CustomerRepositoryImpl with
signatures matching the trait (returning impl Future<Output = Result<...,
CustomerError>> + Send), implement them following the pattern used by
create_subscription/increase_subscription_quantity (use the same DB/stripe
client fields, perform the necessary DB update and Stripe call where
applicable), map errors into CustomerError variants, and ensure the futures are
boxed/async as used elsewhere so the module compiles.
In `@rust/cloud-storage/teams/src/domain/team_service.rs`:
- Around line 137-145: The team subscription mapping is being persisted before
the Stripe/customer mutation; change the order so convert_subscription_to_team
is called first and only after it returns Ok do you call
team_repository.update_team_subscription; specifically, in team_service.rs swap
the two calls involving customer_repository.convert_subscription_to_team(...)
and team_repository.update_team_subscription(team_id, &customer_subscription_id)
so the subscription conversion (using customer_subscription_id and
team.team.owner_id) completes successfully before persisting the
team.subscription_id, ensuring failures do not leave the owner's personal
subscription mutated without the team mapping.
- Around line 201-208: The code incorrectly derives the new team's tier by
calling get_user_roles() and passing its aggregate result to
TeamUserTier::try_from_roles, which includes team-scoped Sub* roles; instead
fetch or compute only the user's global/organization-scoped roles (exclude
team-granted Sub* roles) before calling TeamUserTier::try_from_roles. Update the
call site around user_roles and team_user_tier to either call a dedicated method
that returns global roles (e.g., get_global_user_roles) or filter out any Sub* /
team-scoped roles from user_roles, and keep the same error mapping to
CreateTeamError::StorageLayerError on failure.
- Around line 324-334: The code currently calls get_team_subscription(team_id)
which bootstraps an owner subscription if none exists, risking cancelling the
owner's personal subscription; instead, fetch only the stored team subscription
(do not call get_team_subscription) — e.g., use the repository method that
returns an Option/nullable stored subscription id (reference:
get_team_subscription, cancel_subscription, DeleteTeamError) and if there is no
stored subscription simply skip cancellation; only parse to
stripe::SubscriptionId and call
customer_repository.cancel_subscription(&subscription_id) when a stored id is
present, mapping parse or repository errors to DeleteTeamError as appropriate.
---
Outside diff comments:
In `@rust/cloud-storage/teams/src/domain/team_service.rs`:
- Around line 367-396: Don't consume the invite before ensuring the Stripe seat
bump: obtain the team/subscription identifier first (e.g., add or use a method
to peek the invite/team like team_repository.get_team_invite or otherwise fetch
team_id/subscription via get_team_subscription without calling
accept_team_invite), parse it to stripe::SubscriptionId and call
customer_repository.increase_subscription_quantity(&subscription_id, 1) and
ensure it succeeds; only after the subscription quantity is incremented, call
team_repository.accept_team_invite(team_invite_id, user_id) and then
dangerous_upsert_roles_for_user and
team_channels_repository.add_team_member_to_channels; if no existing peek method
exists, add one so you can read team_id/subscription before calling
accept_team_invite so failures in increase_subscription_quantity do not leave
the invite consumed.
---
Duplicate comments:
In `@rust/cloud-storage/teams/src/domain/team_service.rs`:
- Around line 275-281: The current removal logic in TeamService (the block
calling is_user_member_of_team(...).await? and then
dangerous_remove_roles_from_user(...)) and the similar block used by delete_team
incorrectly keys off “any other team”; instead, compute the user's new
entitlement set by querying their remaining team memberships and any personal
subscription before removing roles. Update the code paths in team_service.rs
(the is_user_member_of_team check and the delete_team flow) to: fetch the list
of remaining teams for user_id, derive the correct RoleId set (including
SubHaiku/SubSonnet/SubOpus from remaining teams plus any personal subscription),
build a NonEmpty role list only for roles that should be removed, and then call
dangerous_remove_roles_from_user with that recomputed set so you neither leave
stale higher-tier roles nor remove valid personal Sub* entitlements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f58de6a-01a2-42bd-ae4c-c5948c871a97
📒 Files selected for processing (2)
rust/cloud-storage/teams/src/domain/customer_repo.rsrust/cloud-storage/teams/src/domain/team_service.rs
whutchinson98
force-pushed
the
hutch/macro-cyepJxjSo21faVp5NjtzN-support-tiers-team-members
branch
2 times, most recently
from
b384b0f to
a3dcb70
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
rust/cloud-storage/teams/src/outbound/team_repo.rs (1)
267-299:⚠️ Potential issue | 🔴 CriticalPersist the invite tier here instead of relying on the DB default.
accept_team_invite()now readsteam_invite.tierand copies it toteam_user, but this INSERT never writes thetiercolumn. Every new invite will therefore fall back to the default tier and members will join with the wrong entitlement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/teams/src/outbound/team_repo.rs` around lines 267 - 299, The INSERT into team_invite (in the block that produces invites in team_repo.rs) omits the tier column so new invites fall back to the DB default; update the SQL and the call to sqlx::query! to include the tier column (e.g., add tier to the INSERT column list, SELECT the tier from the UNNEST row tuple, and bind an additional parameter like &tier_strings[..] alongside &team_invite_ids[..], &email_strings[..], ¯o_user_ids[..]) so that team_invite.tier is persisted and accept_team_invite() will copy the correct tier into team_user.rust/cloud-storage/teams/src/domain/team_service.rs (2)
348-362:⚠️ Potential issue | 🟠 MajorDelete flow leaves the tier role behind.
join_team()grants bothTeamSubscriberandRoleId::from(team_member.tier), but this loop only removesTeamSubscriber. Users who lose their last team can retainSubHaiku/SubSonnet/SubOpusafter the team is deleted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/teams/src/domain/team_service.rs` around lines 348 - 362, The delete loop only removes RoleId::TeamSubscriber and misses removing each member's tier role granted in join_team(); for each member in members, build a NonEmpty list containing both RoleId::TeamSubscriber and the tier role (e.g., RoleId::from(member.tier) or whatever field represents the member's tier), then call self.user_roles_and_permissions_service.dangerous_remove_roles_from_user(&member.user_id, &roles). Use the same error mapping (DeleteTeamError::RemoveRolesFromUserError) and keep the is_user_member_of_team check before removing roles.
375-404:⚠️ Potential issue | 🔴 Critical
join_teamcan fail after the membership is already committed.
accept_team_invite()commits the DB changes and deletes the invite before roles, channels, and billing are updated. If any later step fails, the API returns an error even though the user is already in the team and cannot retry with the same invite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/teams/src/domain/team_service.rs` around lines 375 - 404, The bug is that accept_team_invite(...) commits and deletes the invite before later steps (dangerous_upsert_roles_for_user, team_channels_repository.add_team_member_to_channels, customer_repository.increase_subscription_quantity) can fail, leaving a partially-applied join; fix by making the accept-and-commit atomic: either (A) move the DB commit into a single transaction inside team_repository (provide a new accept_team_invite_atomic or add_team_member_transaction method) that performs creating the membership and deleting the invite only after roles, channels and billing are updated, or (B) implement compensating rollback in join_team: if dangerous_upsert_roles_for_user / add_team_member_to_channels / increase_subscription_quantity fails, call team_repository.remove_team_member_and_restore_invite or reverse the membership and restore the invite and revert billing and channel changes; update join_team to call get_team_subscription, increase_subscription_quantity, dangerous_upsert_roles_for_user, and add_team_member_to_channels inside that transactional/rollback flow so accept_team_invite no longer leaves the DB committed on failure. Ensure references to accept_team_invite, dangerous_upsert_roles_for_user, add_team_member_to_channels, get_team_subscription, increase_subscription_quantity, and team_repository are used to locate the changes.
♻️ Duplicate comments (4)
rust/cloud-storage/teams/src/domain/team_service.rs (4)
279-286:⚠️ Potential issue | 🟠 MajorRecompute the user's remaining tier before removing roles.
These branches only check whether the user is still in any team. If someone leaves an Opus team but still belongs to a Haiku team,
SubOpusis never downgraded; the same bug exists in the bulk revocation path.Also applies to: 445-452
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/teams/src/domain/team_service.rs` around lines 279 - 286, The current removal logic checks only whether the user is a member of any team; instead, before calling user_roles_and_permissions_service.dangerous_remove_roles_from_user (and in the bulk revocation branch), compute the user's remaining highest tier across all teams they still belong to (e.g. query team_repository for the user's teams or use an existing method to get remaining tier) and only remove/downgrade tier-related roles (RoleId::from(tier), RoleId::TeamSubscriber) if that remaining tier is lower than the tier being removed; update the branches around team_repository.is_user_member_of_team(...) and the bulk path (the block duplicated at lines ~445-452) to perform this recomputation and gate the role removal accordingly.
137-145:⚠️ Potential issue | 🔴 CriticalOnly store
subscription_idafter the Stripe conversion succeeds.
update_team_subscription()runs beforeconvert_subscription_to_team(). If the Stripe update fails, later calls will treat the stored id as a team subscription and can mutate or cancel what is still the owner's personal subscription.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/teams/src/domain/team_service.rs` around lines 137 - 145, The code stores the subscription as a team subscription before the Stripe-side conversion completes; move the call to TeamRepository::update_team_subscription so it runs only after CustomerRepository::convert_subscription_to_team succeeds. Specifically, call convert_subscription_to_team(&customer_subscription_id, team_id, &team.team.owner_id).await? first, check it returned Ok, and only then call update_team_subscription(team_id, &customer_subscription_id).await?; handle and propagate errors from convert_subscription_to_team without updating the DB to avoid marking an unchanged owner subscription as a team subscription.
331-341:⚠️ Potential issue | 🔴 CriticalDon't bootstrap a team subscription during deletion.
Calling
get_team_subscription()here can convert the owner's personal subscription when the team never stored one. Deleting such a team will end up canceling the owner's personal subscription instead of simply skipping cancellation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/teams/src/domain/team_service.rs` around lines 331 - 341, Don't call get_team_subscription() here because it bootstraps a subscription (possibly converting an owner's personal subscription); instead fetch the stored team subscription id without creating one, skip cancellation if none exists, and only parse-and-cancel when a stored id is present. Replace the get_team_subscription() call with a repository call that returns Option<String> (or use an existing method that reads the team record/subscription field), e.g., use team_repository.get_stored_subscription(team_id) or read the team record, then if Some(id) call stripe::SubscriptionId::from_str(&id).map_err(...) and customer_repository.cancel_subscription(&subscription_id).await.map_err(DeleteTeamError::CustomerError); if None, do nothing and proceed with deletion. Ensure you reference get_team_subscription, customer_repository.cancel_subscription, and DeleteTeamError::CustomerError when making the change.
203-210:⚠️ Potential issue | 🟠 MajorDon't derive the new team's tier from the caller's aggregate role set.
get_user_roles()includes team-grantedSub*roles, andTeamUserTier::try_from_roles()picks the first match. A user who already carries another team's roles can therefore create a new team with the wrong owner tier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/teams/src/domain/team_service.rs` around lines 203 - 210, The code currently calls user_roles_and_permissions_service.get_user_roles() and passes that aggregate to TeamUserTier::try_from_roles(), which incorrectly uses the caller's existing team-granted Sub* roles to pick the new team's tier; stop using get_user_roles() for the new team's tier determination. Instead, derive the tier from the roles that will be assigned on creation (e.g., the roles payload or the explicit owner role for the new team) and call TeamUserTier::try_from_roles() (or an appropriate TeamUserTier::from_owner/explicit method) on that new-team-specific role set; update the CreateTeam flow to use the create request's roles or an explicit Owner assignment rather than user_roles_and_permissions_service.get_user_roles().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@rust/cloud-storage/authentication_service/src/api/webhooks/user/stripe_webhook.rs`:
- Around line 373-376: The "active" case in stripe_webhook.rs currently only
calls track_stripe_subscription and does not restore team permissions, so users
deprovisioned in the "inactive" branch remain without access; modify the
"active" branch to call the same permission-restoration logic used elsewhere
when reactivating a subscription (e.g., invoke a restore_team_permissions or the
inverse of the revoke function) using the same identifiers (ctx,
subscription_id, tracking_data) so team membership/roles are re-granted and then
call track_stripe_subscription(ctx.analytics_client.clone(), subscription_id,
tracking_data) as before.
In `@rust/cloud-storage/teams/src/domain/team_service.rs`:
- Around line 261-277: The code drops per-member tier data before updating
billing, causing wrong Stripe line items to be adjusted; update the removal and
join flows (the block using remove_user_from_team -> tier and the join flow that
reads team_member.tier) to pass the member's tier (or resolve it to the correct
price/line_item id) into the customer_repository billing call instead of calling
generic decrease_subscription_quantity/increase_subscription_quantity with a
flat quantity. Concretely: use the returned tier from remove_user_from_team (and
team_member.tier in the join path) to look up the subscription's corresponding
line item (via get_team_subscription or a new helper) and call a tier-aware
customer_repository method (or extend
decrease_subscription_quantity/increase_subscription_quantity to accept a
tier/price_id) so the quantity change targets the correct Stripe line item for
that tier.
In `@rust/cloud-storage/teams/src/outbound/customer_repo.rs`:
- Around line 175-193: The current get_subscription_id_for_customer returns the
first active subscription for a customer which can accidentally pick an existing
team-bound subscription; modify get_subscription_id_for_customer to skip
subscriptions that are already marked as team subscriptions (i.e., check the
subscription metadata the same way convert_subscription_to_team marks them) by
iterating subscriptions.data and selecting the first sub where sub.metadata does
not contain the team marker (e.g., no "team_id" or "converted_to_team" key used
by convert_subscription_to_team); return that subscription.id or
CustomerError::SubscriptionNotActive if none match.
---
Outside diff comments:
In `@rust/cloud-storage/teams/src/domain/team_service.rs`:
- Around line 348-362: The delete loop only removes RoleId::TeamSubscriber and
misses removing each member's tier role granted in join_team(); for each member
in members, build a NonEmpty list containing both RoleId::TeamSubscriber and the
tier role (e.g., RoleId::from(member.tier) or whatever field represents the
member's tier), then call
self.user_roles_and_permissions_service.dangerous_remove_roles_from_user(&member.user_id,
&roles). Use the same error mapping (DeleteTeamError::RemoveRolesFromUserError)
and keep the is_user_member_of_team check before removing roles.
- Around line 375-404: The bug is that accept_team_invite(...) commits and
deletes the invite before later steps (dangerous_upsert_roles_for_user,
team_channels_repository.add_team_member_to_channels,
customer_repository.increase_subscription_quantity) can fail, leaving a
partially-applied join; fix by making the accept-and-commit atomic: either (A)
move the DB commit into a single transaction inside team_repository (provide a
new accept_team_invite_atomic or add_team_member_transaction method) that
performs creating the membership and deleting the invite only after roles,
channels and billing are updated, or (B) implement compensating rollback in
join_team: if dangerous_upsert_roles_for_user / add_team_member_to_channels /
increase_subscription_quantity fails, call
team_repository.remove_team_member_and_restore_invite or reverse the membership
and restore the invite and revert billing and channel changes; update join_team
to call get_team_subscription, increase_subscription_quantity,
dangerous_upsert_roles_for_user, and add_team_member_to_channels inside that
transactional/rollback flow so accept_team_invite no longer leaves the DB
committed on failure. Ensure references to accept_team_invite,
dangerous_upsert_roles_for_user, add_team_member_to_channels,
get_team_subscription, increase_subscription_quantity, and team_repository are
used to locate the changes.
In `@rust/cloud-storage/teams/src/outbound/team_repo.rs`:
- Around line 267-299: The INSERT into team_invite (in the block that produces
invites in team_repo.rs) omits the tier column so new invites fall back to the
DB default; update the SQL and the call to sqlx::query! to include the tier
column (e.g., add tier to the INSERT column list, SELECT the tier from the
UNNEST row tuple, and bind an additional parameter like &tier_strings[..]
alongside &team_invite_ids[..], &email_strings[..], ¯o_user_ids[..]) so that
team_invite.tier is persisted and accept_team_invite() will copy the correct
tier into team_user.
---
Duplicate comments:
In `@rust/cloud-storage/teams/src/domain/team_service.rs`:
- Around line 279-286: The current removal logic checks only whether the user is
a member of any team; instead, before calling
user_roles_and_permissions_service.dangerous_remove_roles_from_user (and in the
bulk revocation branch), compute the user's remaining highest tier across all
teams they still belong to (e.g. query team_repository for the user's teams or
use an existing method to get remaining tier) and only remove/downgrade
tier-related roles (RoleId::from(tier), RoleId::TeamSubscriber) if that
remaining tier is lower than the tier being removed; update the branches around
team_repository.is_user_member_of_team(...) and the bulk path (the block
duplicated at lines ~445-452) to perform this recomputation and gate the role
removal accordingly.
- Around line 137-145: The code stores the subscription as a team subscription
before the Stripe-side conversion completes; move the call to
TeamRepository::update_team_subscription so it runs only after
CustomerRepository::convert_subscription_to_team succeeds. Specifically, call
convert_subscription_to_team(&customer_subscription_id, team_id,
&team.team.owner_id).await? first, check it returned Ok, and only then call
update_team_subscription(team_id, &customer_subscription_id).await?; handle and
propagate errors from convert_subscription_to_team without updating the DB to
avoid marking an unchanged owner subscription as a team subscription.
- Around line 331-341: Don't call get_team_subscription() here because it
bootstraps a subscription (possibly converting an owner's personal
subscription); instead fetch the stored team subscription id without creating
one, skip cancellation if none exists, and only parse-and-cancel when a stored
id is present. Replace the get_team_subscription() call with a repository call
that returns Option<String> (or use an existing method that reads the team
record/subscription field), e.g., use
team_repository.get_stored_subscription(team_id) or read the team record, then
if Some(id) call stripe::SubscriptionId::from_str(&id).map_err(...) and
customer_repository.cancel_subscription(&subscription_id).await.map_err(DeleteTeamError::CustomerError);
if None, do nothing and proceed with deletion. Ensure you reference
get_team_subscription, customer_repository.cancel_subscription, and
DeleteTeamError::CustomerError when making the change.
- Around line 203-210: The code currently calls
user_roles_and_permissions_service.get_user_roles() and passes that aggregate to
TeamUserTier::try_from_roles(), which incorrectly uses the caller's existing
team-granted Sub* roles to pick the new team's tier; stop using get_user_roles()
for the new team's tier determination. Instead, derive the tier from the roles
that will be assigned on creation (e.g., the roles payload or the explicit owner
role for the new team) and call TeamUserTier::try_from_roles() (or an
appropriate TeamUserTier::from_owner/explicit method) on that new-team-specific
role set; update the CreateTeam flow to use the create request's roles or an
explicit Owner assignment rather than
user_roles_and_permissions_service.get_user_roles().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed908776-ea15-4980-a087-77e158c363a8
📒 Files selected for processing (5)
rust/cloud-storage/authentication_service/src/api/webhooks/user/stripe_webhook.rsrust/cloud-storage/teams/src/domain/customer_repo.rsrust/cloud-storage/teams/src/domain/team_service.rsrust/cloud-storage/teams/src/outbound/customer_repo.rsrust/cloud-storage/teams/src/outbound/team_repo.rs
rust/cloud-storage/authentication_service/src/api/webhooks/user/stripe_webhook.rs
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/cloud-storage/teams/src/outbound/customer_repo.rs (1)
54-74:⚠️ Potential issue | 🟠 MajorNew subscriptions are hardcoded to Haiku tier.
create_subscriptionalways usesself.stripe_price_ids.haiku(line 62) regardless of the team owner's actual tier. If a Sonnet or Opus subscriber creates a team, their subscription will be created with the wrong price.Consider either accepting a
TeamUserTierparameter or retrieving the appropriate tier fromCreateSubscriptionArgs.🐛 Suggested fix
async fn create_subscription( &self, args: CreateSubscriptionArgs, + tier: TeamUserTier, ) -> Result<stripe::SubscriptionId, CustomerError> { // Create the subscription let mut params = CreateSubscription::new(args.customer_id); params.items = Some(vec![CreateSubscriptionItems { - price: Some(self.stripe_price_ids.haiku.clone()), + price: Some(self.stripe_price_ids.price_id_for_tier(&tier).to_string()), quantity: Some(args.quantity), ..Default::default() }]);This requires updating the trait signature and all call sites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/teams/src/outbound/customer_repo.rs` around lines 54 - 74, create_subscription currently always uses self.stripe_price_ids.haiku; modify it to pick the correct price id based on the team's tier by adding a tier field to CreateSubscriptionArgs (e.g., TeamUserTier) or otherwise including the tier in args, update the trait signature for create_subscription and all its call sites to pass that tier, and inside create_subscription replace the hardcoded self.stripe_price_ids.haiku with a match on args.tier that selects self.stripe_price_ids.haiku/sonnet/opus (or returns an error for unknown tiers) before building CreateSubscription parameters.
♻️ Duplicate comments (5)
rust/cloud-storage/teams/src/domain/team_service.rs (4)
137-148:⚠️ Potential issue | 🔴 CriticalPersist the team mapping only after Stripe conversion succeeds.
The current order writes
subscription_idto the database (line 138-140) before callingconvert_subscription_to_teamon Stripe (line 143-145). If the Stripe call fails, subsequent calls return early at lines 117-118 with the persisted ID, but the subscription was never actually converted—leaving the owner's personal subscription in an inconsistent state.🐛 Proposed fix: swap the order
- // update the customers subscription to a team sub - self.team_repository - .update_team_subscription(team_id, &customer_subscription_id) - .await?; - // convert the customers subscription to a team subscription self.customer_repository .convert_subscription_to_team(&customer_subscription_id, team_id, &team.team.owner_id) .await?; + // update the customers subscription to a team sub + self.team_repository + .update_team_subscription(team_id, &customer_subscription_id) + .await?; + Ok(customer_subscription_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/teams/src/domain/team_service.rs` around lines 137 - 148, The code persists the team mapping via self.team_repository.update_team_subscription before performing the Stripe conversion self.customer_repository.convert_subscription_to_team, causing inconsistent state if the Stripe call fails; change the order so convert_subscription_to_team is called and awaited first, and only after it succeeds call update_team_subscription to persist the mapping, and return customer_subscription_id after both operations complete (ensure any ? propagation remains the same).
203-214:⚠️ Potential issue | 🟠 MajorTeam tier inference uses aggregate roles instead of personal subscription tier.
get_user_roles()returns all roles including team-grantedSub*roles.TeamUserTier::try_from_roles(seemodel.rs:49-60) returns the first match (SubHaikubeforeSubSonnet/SubOpus), so a user who is already a member of a Haiku team but has a personal Sonnet subscription will create a Haiku team instead of Sonnet.Additionally, users without any
Sub*roles (non-subscribers) will fail with "unable to extract team tier from user roles" instead of a clear error.Consider either:
- Fetching the user's personal subscription tier directly from Stripe/billing data
- Or filtering out team-granted roles before deriving the tier
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/teams/src/domain/team_service.rs` around lines 203 - 214, The current team tier is derived from aggregate roles returned by get_user_roles(), which includes team-granted Sub* roles so TeamUserTier::try_from_roles picks the wrong tier (and fails for users with no Sub*), causing create_team to create an incorrect or unclear outcome; fix by obtaining the user's personal subscription tier instead of using aggregated roles or by filtering out team-granted Sub* roles before calling TeamUserTier::try_from_roles — specifically, update the code path around get_user_roles() and the call to TeamUserTier::try_from_roles (used before invoking team_repository.create_team) to either call the billing/Stripe lookup for the user’s own subscription tier or to remove roles that originate from teams (team-granted roles) so only personal subscription roles are considered, and ensure a clear, explicit error is returned when no personal subscription tier is found.
279-286:⚠️ Potential issue | 🟠 MajorRecalculate remaining tier instead of conditional removal.
The check
is_user_member_of_teamis boolean and doesn't account for tier differences. If a user leaves an Opus team but remains in a Haiku team, this block is skipped entirely—the user retainsSubOpuswhen they should only haveSubHaiku.The fix requires recomputing the user's highest remaining team tier and adjusting roles accordingly, rather than all-or-nothing removal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/teams/src/domain/team_service.rs` around lines 279 - 286, The boolean check using team_repository.is_user_member_of_team(user_id) is too coarse; instead compute the user's remaining highest team tier after removing them from this team and then update roles to match that tier. Replace the current conditional block in team_service::remove_user_from_team with logic that queries the repository for the user's remaining teams (e.g., a method to list team tiers or counts), determine the highest remaining Tier enum/value, compute which RoleId(s) should be removed or kept (only remove roles for tiers higher than the recalculated highest tier, e.g., remove RoleId::TeamSubscriber variants above the remaining tier), and call user_roles_and_permissions_service.dangerous_remove_roles_from_user with the precise roles; map any errors to RemoveUserFromTeamError::RemoveRolesFromUserError as before.
327-367:⚠️ Potential issue | 🔴 CriticalDeletion should not bootstrap billing state; tier roles are not revoked.
Two issues:
Bootstrap on delete:
get_team_subscription(line 331) provisions the owner's personal subscription as team subscription if none exists. Deleting a team that never had a subscription will convert and then immediately cancel the owner's personal subscription.Missing tier role cleanup: Lines 349-350 only remove
TeamSubscriber, but not the tier-specific roles (SubHaiku/SubSonnet/SubOpus). Members retain elevated tier permissions after the team is deleted.🐛 Suggested approach
- let subscription_id = self.get_team_subscription(team_id).await?; + // Only fetch stored subscription - don't bootstrap + let subscription_id = self.team_repository.get_team_subscription_id(team_id).await?; - // Cancel subscription - let subscription_id = stripe::SubscriptionId::from_str(&subscription_id).map_err(|_| { - DeleteTeamError::StorageLayerError(anyhow::anyhow!("Invalid subscription id")) - })?; - - self.customer_repository - .cancel_subscription(&subscription_id) - .await - .map_err(DeleteTeamError::CustomerError)?; + // Only cancel if subscription exists + if let Some(subscription_id) = subscription_id { + let subscription_id = stripe::SubscriptionId::from_str(&subscription_id).map_err(|_| { + DeleteTeamError::StorageLayerError(anyhow::anyhow!("Invalid subscription id")) + })?; + self.customer_repository + .cancel_subscription(&subscription_id) + .await + .map_err(DeleteTeamError::CustomerError)?; + }For tier roles, include
RoleId::from(member.tier)in the roles vector inside the member loop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/teams/src/domain/team_service.rs` around lines 327 - 367, The delete_team flow currently calls get_team_subscription which bootstraps an owner’s personal subscription if none exists and then cancels it; change delete_team to fetch the team subscription only if it already exists (avoid calling the bootstraping path) by replacing the get_team_subscription call with a non-bootstrapping fetch (e.g. add/use a method like get_team_subscription_if_exists or add a flag to get_team_subscription to not create a subscription) and handle the None case by skipping cancellation; also fix role cleanup by expanding the roles removed per member in the loop—construct a roles list that includes RoleId::TeamSubscriber and the member’s tier role (use RoleId::from(member.tier) or equivalent), convert to non_empty::NonEmpty and pass that to dangerous_remove_roles_from_user, keeping the existing is_user_member_of_team check and error mapping.rust/cloud-storage/teams/src/outbound/customer_repo.rs (1)
247-267:⚠️ Potential issue | 🔴 CriticalReturns first active subscription without checking if it's already team-bound.
This method returns the first active subscription for a customer, but
convert_subscription_to_teammarks team subscriptions withteam_idmetadata. If an owner has an existing team subscription, this can return that subscription instead of their personal one, causing the bootstrap path inget_team_subscriptionto reassign an existing team's subscription to a new team.Filter out subscriptions that already have
team_idin metadata:subscriptions .data .into_iter() .find(|sub| { sub.metadata .as_ref() .map(|m| !m.contains_key("team_id")) .unwrap_or(true) }) .map(|sub| sub.id) .ok_or(CustomerError::SubscriptionNotActive)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/teams/src/outbound/customer_repo.rs` around lines 247 - 267, get_subscription_id_for_customer currently returns the first active subscription and can accidentally pick a subscription already bound to a team; update the selection logic in get_subscription_id_for_customer to skip subscriptions whose metadata contains "team_id" (i.e., find the first subscription where sub.metadata.as_ref().map(|m| !m.contains_key("team_id")).unwrap_or(true)) so personal subscriptions are chosen, ensuring convert_subscription_to_team and get_team_subscription won't reassign an existing team's subscription.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/teams/src/domain/team_service.rs`:
- Around line 395-404: get_team_subscription already returns a
stripe::SubscriptionId, so remove the redundant parse using
stripe::SubscriptionId::from_str and use the returned value directly: stop
shadowing subscription_id with a parsed result and pass the original
subscription_id from get_team_subscription into
customer_repository.increase_subscription_quantity (keep the call to
increase_subscription_quantity(&subscription_id, 1, team_member.tier) but ensure
subscription_id is the stripe::SubscriptionId returned by get_team_subscription
and not re-parsed).
In `@rust/cloud-storage/teams/src/outbound/customer_repo.rs`:
- Around line 125-131: The UpdateSubscription struct literals are misindented:
the fields (items, proration_behavior, ..Default::default()) should be indented
one level inside the UpdateSubscription { ... } block; update the three
occurrences that construct UpdateSubscription (the variables named update_params
in customer_repo.rs and the other two similar blocks) so each field line is
aligned/indented under the opening brace and the closing brace sits on its own
line, preserving existing fields like proration_behavior and items and keeping
..Default::default() last.
- Around line 77-81: The parameter increase_amount on
increase_subscription_quantity is never used; replace the hard-coded literal 1
used when changing the Stripe subscription quantity with the increase_amount
parameter so the function actually adjusts by the requested amount; locate the
quantity update calls inside increase_subscription_quantity (where quantity is
incremented/decremented) and substitute the literal 1 with increase_amount,
ensuring any arithmetic respects TeamUserTier logic and types.
- Around line 141-145: The decrease_subscription_quantity function currently
ignores the decrease_amount parameter and uses hardcoded 1s; update the logic in
decrease_subscription_quantity to use the provided decrease_amount: replace
comparisons against 1 with comparisons against decrease_amount (e.g., check if
current_quantity > decrease_amount), subtract decrease_amount from
current_quantity when reducing quantity (ensuring you clamp at zero or the
minimum allowed), and adjust any conditional branches (like setting
cancel_at_period_end or choosing update vs. cancel flows) to use the computed
new quantity rather than decrementing by 1; mirror the same parameter usage
pattern as in increase_subscription_quantity so the function respects the
decrease_amount argument.
---
Outside diff comments:
In `@rust/cloud-storage/teams/src/outbound/customer_repo.rs`:
- Around line 54-74: create_subscription currently always uses
self.stripe_price_ids.haiku; modify it to pick the correct price id based on the
team's tier by adding a tier field to CreateSubscriptionArgs (e.g.,
TeamUserTier) or otherwise including the tier in args, update the trait
signature for create_subscription and all its call sites to pass that tier, and
inside create_subscription replace the hardcoded self.stripe_price_ids.haiku
with a match on args.tier that selects self.stripe_price_ids.haiku/sonnet/opus
(or returns an error for unknown tiers) before building CreateSubscription
parameters.
---
Duplicate comments:
In `@rust/cloud-storage/teams/src/domain/team_service.rs`:
- Around line 137-148: The code persists the team mapping via
self.team_repository.update_team_subscription before performing the Stripe
conversion self.customer_repository.convert_subscription_to_team, causing
inconsistent state if the Stripe call fails; change the order so
convert_subscription_to_team is called and awaited first, and only after it
succeeds call update_team_subscription to persist the mapping, and return
customer_subscription_id after both operations complete (ensure any ?
propagation remains the same).
- Around line 203-214: The current team tier is derived from aggregate roles
returned by get_user_roles(), which includes team-granted Sub* roles so
TeamUserTier::try_from_roles picks the wrong tier (and fails for users with no
Sub*), causing create_team to create an incorrect or unclear outcome; fix by
obtaining the user's personal subscription tier instead of using aggregated
roles or by filtering out team-granted Sub* roles before calling
TeamUserTier::try_from_roles — specifically, update the code path around
get_user_roles() and the call to TeamUserTier::try_from_roles (used before
invoking team_repository.create_team) to either call the billing/Stripe lookup
for the user’s own subscription tier or to remove roles that originate from
teams (team-granted roles) so only personal subscription roles are considered,
and ensure a clear, explicit error is returned when no personal subscription
tier is found.
- Around line 279-286: The boolean check using
team_repository.is_user_member_of_team(user_id) is too coarse; instead compute
the user's remaining highest team tier after removing them from this team and
then update roles to match that tier. Replace the current conditional block in
team_service::remove_user_from_team with logic that queries the repository for
the user's remaining teams (e.g., a method to list team tiers or counts),
determine the highest remaining Tier enum/value, compute which RoleId(s) should
be removed or kept (only remove roles for tiers higher than the recalculated
highest tier, e.g., remove RoleId::TeamSubscriber variants above the remaining
tier), and call
user_roles_and_permissions_service.dangerous_remove_roles_from_user with the
precise roles; map any errors to
RemoveUserFromTeamError::RemoveRolesFromUserError as before.
- Around line 327-367: The delete_team flow currently calls
get_team_subscription which bootstraps an owner’s personal subscription if none
exists and then cancels it; change delete_team to fetch the team subscription
only if it already exists (avoid calling the bootstraping path) by replacing the
get_team_subscription call with a non-bootstrapping fetch (e.g. add/use a method
like get_team_subscription_if_exists or add a flag to get_team_subscription to
not create a subscription) and handle the None case by skipping cancellation;
also fix role cleanup by expanding the roles removed per member in the
loop—construct a roles list that includes RoleId::TeamSubscriber and the
member’s tier role (use RoleId::from(member.tier) or equivalent), convert to
non_empty::NonEmpty and pass that to dangerous_remove_roles_from_user, keeping
the existing is_user_member_of_team check and error mapping.
In `@rust/cloud-storage/teams/src/outbound/customer_repo.rs`:
- Around line 247-267: get_subscription_id_for_customer currently returns the
first active subscription and can accidentally pick a subscription already bound
to a team; update the selection logic in get_subscription_id_for_customer to
skip subscriptions whose metadata contains "team_id" (i.e., find the first
subscription where sub.metadata.as_ref().map(|m|
!m.contains_key("team_id")).unwrap_or(true)) so personal subscriptions are
chosen, ensuring convert_subscription_to_team and get_team_subscription won't
reassign an existing team's subscription.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cc371d28-b27a-43c8-8d87-0a5ece64c5f5
📒 Files selected for processing (4)
rust/cloud-storage/authentication_service/src/main.rsrust/cloud-storage/teams/src/domain/customer_repo.rsrust/cloud-storage/teams/src/domain/team_service.rsrust/cloud-storage/teams/src/outbound/customer_repo.rs
whutchinson98
force-pushed
the
hutch/macro-cyepJxjSo21faVp5NjtzN-support-tiers-team-members
branch
from
dfd67ef to
9d80435
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
rust/cloud-storage/authentication_service/src/api/webhooks/user/stripe_webhook.rs (1)
373-376:⚠️ Potential issue | 🟠 Major
"active"status still does not restore team permissions.The branch still only tracks analytics and leaves a TODO. If members were revoked in
"past_due" | "paused" | "unpaid" | "canceled", they remain deprovisioned after reactivation. Please implement permission reconciliation here before returningOk(()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/webhooks/user/stripe_webhook.rs` around lines 373 - 376, The "active" branch currently only calls track_stripe_subscription and leaves a TODO; implement permission reconciliation there by invoking the existing permission-restoration flow (e.g., call a function like reconcile_team_permissions or restore_team_permissions_for_subscription with ctx, subscription_id, and tracking_data) to fetch the affected team/users, re-evaluate their roles against the current plan, and grant back any permissions revoked during "past_due"|"paused"|"unpaid"|"canceled"; run this restoration before calling track_stripe_subscription and before returning Ok(()), and reuse existing helpers such as grant_permissions_to_user()/update_user_role() or the team permission service functions to avoid duplicating logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@rust/cloud-storage/authentication_service/src/api/webhooks/user/stripe_webhook.rs`:
- Around line 373-376: The "active" branch currently only calls
track_stripe_subscription and leaves a TODO; implement permission reconciliation
there by invoking the existing permission-restoration flow (e.g., call a
function like reconcile_team_permissions or
restore_team_permissions_for_subscription with ctx, subscription_id, and
tracking_data) to fetch the affected team/users, re-evaluate their roles against
the current plan, and grant back any permissions revoked during
"past_due"|"paused"|"unpaid"|"canceled"; run this restoration before calling
track_stripe_subscription and before returning Ok(()), and reuse existing
helpers such as grant_permissions_to_user()/update_user_role() or the team
permission service functions to avoid duplicating logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cd0627a4-00af-4eff-b30b-c266b7883e03
📒 Files selected for processing (2)
rust/cloud-storage/authentication_service/src/api/webhooks/user/stripe_webhook.rsrust/cloud-storage/authentication_service/src/main.rs
whutchinson98
deleted the
hutch/macro-cyepJxjSo21faVp5NjtzN-support-tiers-team-members
branch
This was referenced Mar 27, 2026
Merged
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
In a follow up PR I will expose an endpoint to actually modify a team members tier