Filter out inactive memberships in most spots #3489
Conversation
📝 WalkthroughWalkthroughThe changes refactor project membership logic across several modules. Boolean properties and filters for project membership are replaced with a richer Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (12)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/components/project/project-member/project-member.repository.ts (1)
326-343:⚠️ Potential issueFix the
activefilter – should beinactiveAt IS NULL, notIS NOT NULL- active: filter.isPropNotNull('inactiveAt'), + // A member is active when `inactiveAt` is NULL + active: filter.isPropNull('inactiveAt'),If
isPropNullhelper doesn’t exist, replace with:active: (q) => filter.isPropNotNull('inactiveAt')(q).negate(),Update every caller that relies on the previous (incorrect) behaviour.
🧹 Nitpick comments (1)
src/components/project/dto/project.dto.ts (1)
180-184: GraphQL exposure ofmembershipis unclearThe new
membershipfield lacks a@Field()decorator, so it is not exposed via GraphQL.
Confirm this is intentional; otherwise add the decorator (and possibly a dedicated object type) so clients can query roles & active state.If hiding from the schema is desired, document this to avoid confusion for future contributors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/components/progress-report/workflow/progress-report-workflow.repository.ts(2 hunks)src/components/project/dto/list-projects.dto.ts(4 hunks)src/components/project/dto/project.dto.ts(1 hunks)src/components/project/project-filters.query.ts(0 hunks)src/components/project/project-member/project-member.repository.ts(1 hunks)src/components/project/project.gel.repository.ts(2 hunks)src/components/project/project.repository.ts(3 hunks)src/components/project/project.resolver.ts(3 hunks)src/core/database/query/match-project-based-props.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/project/project-filters.query.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/components/progress-report/workflow/progress-report-workflow.repository.ts (1)
src/core/database/query/matching.ts (1)
ACTIVE(33-33)
src/components/project/dto/project.dto.ts (2)
src/core/resources/resource.loader.ts (1)
LinkTo(34-39)src/common/secured-property.ts (1)
UnsecuredDto(57-59)
src/core/database/query/match-project-based-props.ts (1)
src/core/database/query/matching.ts (1)
ACTIVE(33-33)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: Unit
- GitHub Check: Clean
- GitHub Check: Generate (head)
- GitHub Check: Generate (base)
- GitHub Check: lint
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
src/core/database/query/match-project-based-props.ts (1)
61-63: Double-check permission semantics after addingACTIVEAdding
ACTIVEhere filters out memberships whose relationship edge is inactive, but leaves memberships whose node hasinactiveAtset.
Given the PR note that permissions should still honour inactive memberships, this change might unintentionally remove permissions for users with inactive memberships that still have an active edge.Please verify expected permission behaviour end-to-end.
src/components/project/project.gel.repository.ts (1)
41-45:membership.activeused in filters but not loaded in shapeThe list filters rely on
project.membership.active, yet the hydrate shape only selectsid,roles, andinactiveAt.
Ifactiveis a computed property in EdgeDB that’s fine, but if not, extend the shape:membership: { id: true, roles: true, inactiveAt: true, + active: true, },Otherwise the subsequent
e.op(project.membership.active, '?=', true)may always evaluate to unknown.src/components/project/project.repository.ts (1)
95-107: Well-implemented membership hydration logicThe subquery correctly fetches active membership data for the current user, properly filtering by the ACTIVE relationship and returning the membership DTO. This aligns perfectly with the PR objective of filtering out inactive memberships.
src/components/project/project.resolver.ts (1)
160-171: Verify alignment with permission checking behaviorThe resolver correctly implements the membership check with optional inactive inclusion. However, the PR objectives mention that inactive memberships still count towards permissions, while Gel's
Project.isMemberdoesn't yet restrict to active memberships in permission checks.Please verify that this resolver's default behavior (excluding inactive memberships) aligns with the intended permission model across the system.
src/components/project/dto/list-projects.dto.ts (2)
97-103: Excellent backward compatibility implementationThe Transform decorator elegantly handles the transition from
mine/isMembertomembership.active, ensuring that old queries continue to work as expected while encouraging the use of the new membership-based filtering.
139-159: Well-designed deprecation strategyThe dynamic property definitions provide seamless backward compatibility while clearly communicating the deprecation path. The setters correctly translate the old boolean filters to the new membership-based approach, and the conditional logic prevents overriding explicit membership filters.
src/components/progress-report/workflow/progress-report-workflow.repository.ts
Outdated
Show resolved
Hide resolved
CarsonF
force-pushed
the
project-member/inactive-filters
branch
from
0f052c0 to
e411590
Compare
🗞 GraphQL SummaryView schema changes@@ -2312,9 +2312,12 @@
"""
internshipEngagements(input: EngagementListInput = {count: 25, order: ASC, page: 1, sort: "createdAt"}): SecuredInternshipEngagementList!
"""Is the requesting user a member of this project?"""
- isMember: Boolean!
+ isMember(
+ """Consider inactive memberships as well"""
+ includeInactive: Boolean
+ ): Boolean!
marketingLocation: SecuredLocation!
marketingRegionOverride: SecuredLocation!
modifiedAt: DateTime!
mouEnd: SecuredDateNullable!
@@ -2846,9 +2849,12 @@
id: ID!
initialMouEnd: SecuredDateNullable!
"""Is the requesting user a member of this project?"""
- isMember: Boolean!
+ isMember(
+ """Consider inactive memberships as well"""
+ includeInactive: Boolean
+ ): Boolean!
"""
Same as `engagements` field just typed as a list of concrete LanguageEngagements instead of the interface.
TranslationProjects will only have LanguageEngagements.
@@ -2969,9 +2975,12 @@
id: ID!
initialMouEnd: SecuredDateNullable!
"""Is the requesting user a member of this project?"""
- isMember: Boolean!
+ isMember(
+ """Consider inactive memberships as well"""
+ includeInactive: Boolean
+ ): Boolean!
"""
Same as `engagements` field just typed as a list of concrete LanguageEngagements instead of the interface.
TranslationProjects will only have LanguageEngagements.
@@ -4714,9 +4723,12 @@
id: ID!
initialMouEnd: SecuredDateNullable!
"""Is the requesting user a member of this project?"""
- isMember: Boolean!
+ isMember(
+ """Consider inactive memberships as well"""
+ includeInactive: Boolean
+ ): Boolean!
marketingLocation: SecuredLocation!
marketingRegionOverride: SecuredLocation!
modifiedAt: DateTime!
mouEnd: SecuredDateNullable!
@@ -4834,9 +4846,11 @@
input ProjectFilters {
"""Only projects created within this time range"""
createdAt: DateTimeFilter
- """Only projects that the requesting user is a member of"""
+ """
+ Only projects that the requesting user is an active member of. false does nothing.
+ """
isMember: Boolean
"""Only projects with _any_ members matching these filters"""
members: ProjectMemberFilters
@@ -7145,9 +7159,12 @@
id: ID!
initialMouEnd: SecuredDateNullable!
"""Is the requesting user a member of this project?"""
- isMember: Boolean!
+ isMember(
+ """Consider inactive memberships as well"""
+ includeInactive: Boolean
+ ): Boolean!
"""
Same as `engagements` field just typed as a list of concrete LanguageEngagements instead of the interface.
TranslationProjects will only have LanguageEngagements.
|
There was a problem hiding this comment.
Pull Request Overview
This PR restores the original behavior of only considering active project memberships across various database queries and the GraphQL API by filtering out inactive memberships by default.
- Added
ACTIVEfilters to membership relationships in core queries and repositories. - Replaced the standalone
isMemberfield with a richermembershipobject in the GraphQL schema, plus anincludeInactiveargument for opt-in. - Refactored input DTOs and filters to default
membership.active = trueand deprecated legacymine/isMemberflags.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/core/database/query/match-project-based-props.ts | Added ACTIVE to the member relation to ignore inactive users |
| src/components/project/project.resolver.ts | Introduced membership logic and includeInactive arg, removed DTO isMember |
| src/components/project/project.repository.ts | Subquery now fetches only active memberships and merges them |
| src/components/project/project.gel.repository.ts | Exposed membership fields and updated GEL filters |
| src/components/project/project-member/project-member.repository.ts | Applied active: true in projectMemberFilters |
| src/components/project/project-filters.query.ts | Removed legacy mine/isMember filters in favor of membership |
| src/components/project/dto/project.dto.ts | Replaced DTO isMember boolean with membership object |
| src/components/project/dto/list-projects.dto.ts | Added transform logic for membership, deprecated old flags |
| src/components/progress-report/workflow/progress-report-workflow.repository.ts | Added filter to skip inactive project members in workflow queries |
Comments suppressed due to low confidence (4)
src/components/progress-report/workflow/progress-report-workflow.repository.ts:147
- Using
pathto match theinactiveAtproperty as a relationship will never succeed. Properties likeinactiveAtaren’t stored as nodes or relationships—use awhereclause on the node property (e.g. filtermember.inactiveAt = null).
.where(path([
src/components/project/project.resolver.ts:88
- [nitpick] The class
IsMemberArgscontains anincludeInactiveargument; consider renaming the class toIncludeInactiveArgsor similar to better reflect its purpose.
class IsMemberArgs {
src/components/project/dto/list-projects.dto.ts:97
- The custom transform logic setting
value.activewhenmineorisMemberflags are used is non-trivial—ensure there are unit tests covering this behavior to prevent regressions.
@Transform(({ value, obj }) => {
src/components/project/project.resolver.ts:160
- The
isMemberresolver field is no longer present in the DTO schema, making this resolver unused. Consider removing it to avoid dead code.
@ResolveField(() => Boolean, {
Following up on #3444
This restores the previous functionality where only "active" memberships are considered.
Since inactive memberships was just introduced, we assumed before that they were just deleted.
Now that they are maintained, we need to ignore them in most spots.
It's worth noting that inactive memberships still count for permissions.
And Gel's
Project.isMemberdoes not yet restrict to only active, since this is currently used in permission checks.We'll need to do some more work here in the future. Which was already the case with isMember & in app policy checks.
Thinking more, it would probably make sense to invert the default filter. And default to only active memberships. That would've avoided the need add a filter for DOMO. I think the UI needs both so it's either way there.