Open
Conversation
- Added `adminJourneys` query to fetch journeys based on user roles and filters. - Implemented `getJourneyProfile` query to retrieve user-specific journey profiles. - Created corresponding test files for both queries to ensure functionality and access control. - Updated journey schema index to include new queries.
- Removed deprecated `adminJourneys` and `getJourneyProfile` queries from the legacy schema. - Updated the modern schema to include refined `adminJourneys` and `getJourneyProfile` queries with improved access control. - Adjusted resolver logic to enhance filtering and user role checks for journey access. - Cleaned up related test cases to reflect the changes in query structure and functionality.
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:
WalkthroughRelocates two public GraphQL query fields—adminJourneys and getJourneyProfile—from the legacy api-journeys API to the api-journeys-modern API; legacy schema and resolver exposures removed, and equivalent schema entries, resolvers, and tests added in the modern API. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ModernAPI as Modern API (GraphQL)
participant Auth as Auth Service
participant Profile as JourneyProfile (Prisma)
participant Journeys as Journeys (Prisma)
Client->>ModernAPI: adminJourneys(status, template, teamId, useLastActiveTeamId)
ModernAPI->>Auth: validate token / get userId
Auth-->>ModernAPI: userId
alt useLastActiveTeamId == true
ModernAPI->>Profile: findUnique(where: { userId })
Profile-->>ModernAPI: journeyProfile (or null)
end
ModernAPI->>Journeys: findMany(where: ACL ∧ filters)
Journeys-->>ModernAPI: [Journey]
ModernAPI-->>Client: [Journey]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
|
View your CI Pipeline Execution ↗ for commit eb0116d
☁️ Nx Cloud last updated this comment at |
github-actions
bot
temporarily deployed
to
Preview - journeys-admin
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apis/api-journeys-modern/src/schema/journeyProfile/getJourneyProfile.query.spec.ts (1)
15-24: Add an explicit type for the mockUser fixture.The test fixture includes a
rolesproperty not present in the standardUserinterface from@core/yoga/firebaseClient, so a local type keeps the fixture type-safe and prevents drift.Suggested refactor
+type MockUser = { + id: string + email: string + emailVerified: boolean + firstName: string + lastName: string + imageUrl: string | null + roles: string[] +} + - const mockUser = { + const mockUser: MockUser = { id: 'userId', email: 'test@example.com', emailVerified: true, firstName: 'Test', lastName: 'User', imageUrl: null, roles: [] }Per coding guidelines:
**/*.{ts,tsx}: Define a type if possible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-journeys-modern/src/schema/journeyProfile/getJourneyProfile.query.spec.ts` around lines 15 - 24, The test fixture mockUser in the getJourneyProfile suite lacks an explicit type and includes a roles property not present on the standard User type; define a local interface or type alias (e.g., MockUser or TestUser) that describes the exact shape used in the test (including id, email, emailVerified, firstName, lastName, imageUrl, and roles) and annotate the mockUser constant with that type so the fixture is type-safe and cannot drift from expected fields; update the mockUser declaration in the describe('getJourneyProfile') block to use that new type.apis/api-journeys-modern/src/schema/journey/adminJourneys.query.ts (2)
114-119: No pagination onfindMany— unbounded result set.If a user has access to a large number of journeys (e.g., a team manager of a prolific team), this query returns all of them in a single response. If this matches the original
api-journeysbehavior, it's fine for this migration PR, but worth adding pagination (take/cursor) as a follow-up to protect against performance degradation at scale.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-journeys-modern/src/schema/journey/adminJourneys.query.ts` around lines 114 - 119, The prisma.journey.findMany call currently returns an unbounded result set (using accessibleJourneys and filter merged into where), so add pagination parameters (e.g., accept and pass through take/skip or cursor-based args) to limit results and support cursor paging; modify the call site that builds query (the object referenced by query) or the findMany invocation to include take and cursor (and a stable orderBy, e.g., id or createdAt) so callers can request paged slices and the server enforces a sane default limit when none provided.
64-111: Consider consolidating duplicate ACL branches.The team manager/member and journey owner/editor conditions differ only by role and could each be merged using
in:♻️ Suggested consolidation
const accessibleJourneys: Prisma.JourneyWhereInput = { OR: [ - // user is a team manager + // user is a team manager or member { team: { userTeams: { some: { userId, - role: UserTeamRole.manager - } - } - } - }, - // user is a team member - { - team: { - userTeams: { - some: { - userId, - role: UserTeamRole.member + role: { in: [UserTeamRole.manager, UserTeamRole.member] } } } } }, - // user is a journey owner + // user is a journey owner or editor { userJourneys: { some: { userId, - role: UserJourneyRole.owner - } - } - }, - // user is a journey editor - { - userJourneys: { - some: { - userId, - role: UserJourneyRole.editor + role: { in: [UserJourneyRole.owner, UserJourneyRole.editor] } } } },That said, if this mirrors the original
api-journeysimplementation for migration fidelity, keeping it as-is is fine. Based on learnings, the migration to api-journeys-modern should preserve the same structure as the original api-journeys implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-journeys-modern/src/schema/journey/adminJourneys.query.ts` around lines 64 - 111, The ACL query in accessibleJourneys duplicates nearly identical branches for team roles and journey roles; consolidate them by replacing the separate manager/member branches with a single team.userTeams.some condition using role: { in: [...] } for UserTeamRole and likewise merge the userJourneys owner/editor branches into one using role: { in: [...] } for UserJourneyRole, keeping the rest of the Prisma.JourneyWhereInput structure (including the published template branch with PrismaJourneyStatus.published) intact so the logic and symbols accessibleJourneys, UserTeamRole, UserJourneyRole remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apis/api-journeys-modern/src/schema/journey/adminJourneys.query.ts`:
- Around line 114-119: The prisma.journey.findMany call currently returns an
unbounded result set (using accessibleJourneys and filter merged into where), so
add pagination parameters (e.g., accept and pass through take/skip or
cursor-based args) to limit results and support cursor paging; modify the call
site that builds query (the object referenced by query) or the findMany
invocation to include take and cursor (and a stable orderBy, e.g., id or
createdAt) so callers can request paged slices and the server enforces a sane
default limit when none provided.
- Around line 64-111: The ACL query in accessibleJourneys duplicates nearly
identical branches for team roles and journey roles; consolidate them by
replacing the separate manager/member branches with a single team.userTeams.some
condition using role: { in: [...] } for UserTeamRole and likewise merge the
userJourneys owner/editor branches into one using role: { in: [...] } for
UserJourneyRole, keeping the rest of the Prisma.JourneyWhereInput structure
(including the published template branch with PrismaJourneyStatus.published)
intact so the logic and symbols accessibleJourneys, UserTeamRole,
UserJourneyRole remain unchanged.
In
`@apis/api-journeys-modern/src/schema/journeyProfile/getJourneyProfile.query.spec.ts`:
- Around line 15-24: The test fixture mockUser in the getJourneyProfile suite
lacks an explicit type and includes a roles property not present on the standard
User type; define a local interface or type alias (e.g., MockUser or TestUser)
that describes the exact shape used in the test (including id, email,
emailVerified, firstName, lastName, imageUrl, and roles) and annotate the
mockUser constant with that type so the fixture is type-safe and cannot drift
from expected fields; update the mockUser declaration in the
describe('getJourneyProfile') block to use that new type.
- Added `ExecutionResult` type to the admin journeys query tests for better type safety. - Removed the mock for anonymous user as it was not utilized in the tests. - Updated test cases to ensure consistent handling of query results and improved readability.
github-actions
bot
temporarily deployed
to
Preview - journeys-admin
…injourneys-modern
github-actions
bot
temporarily deployed
to
Preview - journeys-admin
github-actions
bot
temporarily deployed
to
Preview - journeys-admin
github-actions
bot
temporarily deployed
to
Preview - journeys-admin
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
3 participants
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.
Summary by CodeRabbit
New Features
Chores
Bug Fixes / API Changes
Tests