Repository navigation
feat(SCIM): Display deactivated org memberships - #8649
Conversation
Expose `is_organisation_membership_active` on the organisation users endpoint, and mark inactive members with an `Inactive` chip wherever organisation users are listed or picked in the dashboard. Closes #8648
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
github-actions
Bot
added
front-end
Docker builds report
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)📝 WalkthroughWalkthroughThe organisation users API now returns Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to The organisation-users API exposes the status under a different key than the one required by the issue, so clients expecting Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change displays existing membership state without granting access or changing activation rules. Organisation access checks remain in place, and inactive members remain administratively manageable. No introduced security issue was identified; concurrent membership changes and deployment compatibility were not fully verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8649 +/- ##
=======================================
Coverage 98.84% 98.84%
=======================================
Files 1659 1659
Lines 68541 68556 +15
=======================================
+ Hits 67751 67766 +15
Misses 790 790 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #21098 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-arm-16 — run #21099 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #21100 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #21100 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #21099 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #21098 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #21100 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #21098 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #21099 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #21100 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #21098 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #21099 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
talissoncosta
left a comment
talissoncosta
left a comment
There was a problem hiding this comment.
Had a look at the frontend side. Looks pretty good!
One thing I would change, and one to take or leave.
Spacing. The chip owning its own margin means a passed className replaces it rather than adds to it, so the mr-2 on the group member row loses the left margin. I'd move the spacing onto the parents instead:
<div className='d-flex align-items-center gap-2'>
<span>{`${first_name} ${last_name}`}</span>
<InactiveMembershipChip user={user} />
</div>I have it applied and checked across all five surfaces against a seeded org. Want me to push it to the branch?
Minor: UserSelect types users: any[], so the chip receives an untyped object. There is now a reason to narrow it to User[]. Easy to fold into the push above if you want it.
Drop the chip's built-in margin, and lay out name and chip with `d-flex align-items-center gap-2` on each parent, so spacing is consistent across surfaces. Narrow `UserSelect`'s `users` prop to `User[]`.
github-actions
Bot
added
docs
|
Thanks @talissoncosta! Fixed both in 6423d8d. |
github-actions
Bot
added
feature
github-actions
Bot
added
docs
talissoncosta
left a comment
talissoncosta
left a comment
There was a problem hiding this comment.
Looks good! There's still a detail I missed on my first pass.
However I realised that we are using a dead class that need to be fixed. So I will take it myself on a pr to re-introduce text-truncate
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Expose the required membership_active API field. · serializers.py:69-76
api/users/serializers.py:69-76
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winExpose the required
membership_activeAPI field.Issue
#8648explicitly requiresmembership_activein the organisation users API.UserListSerializeremits onlyis_organisation_membership_active, so clients that follow the issue contract cannot read the membership status.Rename the field and the same-PR frontend, test, and documentation references.
Suggested fix
- is_organisation_membership_active = serializers.SerializerMethodField( + membership_active = serializers.SerializerMethodField( ... - "is_organisation_membership_active", + "membership_active", ... - def get_is_organisation_membership_active(self, instance: FFAdminUser) -> bool: + def get_membership_active(self, instance: FFAdminUser) -> bool:
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 2c4d8443-c782-4d58-be50-b9cf59019bb5
📒 Files selected for processing (7)
api/tests/unit/users/test_unit_users_views.pyfrontend/web/components/EditPermissions.tsxfrontend/web/components/UserSelect.tsxfrontend/web/components/modals/CreateGroup.tsxfrontend/web/components/modals/CreateRole.tsxfrontend/web/components/users-permissions/InactiveMembershipChip.tsxfrontend/web/components/users-permissions/OrganisationUsersTable/components/OrganisationUsersTableRow.tsx
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
.text-truncate is defined in bootstrap/scss/helpers/_text-truncation.scss, and we import type, containers, grid and the rest but never helpers. So the class has never existed, and every call site using it silently does nothing. Three components already use it, each with the surrounding markup written correctly for it: AudienceSegmentList twice and CsvUpload once. They start truncating with an ellipsis instead of overflowing, which is what their markup was always asking for. Refs #8649 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Closes #8648
Follow-up to #8368 / #8370, which introduced deactivated organisation memberships via SCIM.
API
GET /api/v1/organisations/{organisation_pk}/users/(and theupdate-roleresponse) now exposeis_organisation_membership_active. It's read from the already-prefetcheduserorganisation_set, so the list query count is unchanged.Frontend
InactiveMembershipChipcomponent: anInactivechip (baseChip) with a tooltip explaining that the membership was deactivated by the identity provider, that the member can't access the organisation and doesn't count towards the seat limit, and that their role, permissions and groups are retained. It only renders when the field is explicitlyfalse.UserSelect(flag owners, project/environment admins, change request assignees, role members)Docs
Screenshots
1. Users and Permissions → Members (tooltip shown on hover)
2.
UserSelect— Create Project → Project administrators3. Group modal — "Add a user" dropdown and member rows
4. Project Settings → Permissions → Users
How did you test this code?
update-roleendpoints covering active and deactivated memberships; existing N+1 query-count test still passes.set_organisation_membership_active(..., is_active=False):Inactivechip; hovering shows the tooltip.UserSelect): chip shows in the picker.