Open
Conversation
Contributor
WalkthroughThe changes introduce a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant GraphQL API
participant Auth Service
participant Prisma ORM
participant Access Control
Client->>GraphQL API: Query adminJourneys(status, template, teamId, useLastActiveTeamId)
GraphQL API->>Auth Service: Authenticate & extract userId
alt User Authenticated
Auth Service-->>GraphQL API: Return userId & currentUser
GraphQL API->>Prisma ORM: Fetch journeyProfile (if useLastActiveTeamId)
alt useLastActiveTeamId true
alt Journey Profile Exists
Prisma ORM-->>GraphQL API: Return lastActiveTeamId
GraphQL API->>GraphQL API: Build filter with lastActiveTeamId
else Journey Profile Missing
Prisma ORM-->>GraphQL API: Not found
GraphQL API-->>Client: GraphQL Error (NOT_FOUND)
end
else useLastActiveTeamId false
GraphQL API->>GraphQL API: Use provided teamId or apply default ACL
end
GraphQL API->>Access Control: Get journeyReadAccessWhere(userId, currentUser)
Access Control-->>GraphQL API: Return access filter (teams, user journeys, published, publisher templates)
GraphQL API->>Prisma ORM: findMany with combined filters (access + status + template)
Prisma ORM-->>GraphQL API: Return Journey array
GraphQL API-->>Client: Return adminJourneys result
else User Not Authenticated
Auth Service-->>GraphQL API: Authentication failed
GraphQL API-->>Client: Authentication error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
View your CI Pipeline Execution ↗ for commit 3ebc9ff
☁️ Nx Cloud last updated this comment at |
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.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apis/api-journeys-modern/src/schema/journey/journey.acl.ts (1)
37-60: Extract these ACL predicates into a shared source of truth.
journeyReadAccessWherenow maintains the same role/template access matrix in a second place alongsidejourneyAcl(). Pulling the team/user/template predicates into shared helpers/constants will make the list filter and object-level checks much less likely to drift apart later.🤖 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/journey.acl.ts` around lines 37 - 60, The ACL predicates used in journeyReadAccessWhere are duplicated with journeyAcl; extract the common role/template predicates into a shared helper (e.g., export a constant or function like buildJourneyAccessPredicates or journeyAccessPredicates) that returns the array of OR conditions for team manager/member, team member, userJourneys owner/editor, template: true & published, and the optional template-only when isPublisher; then replace the inline OR array in journeyReadAccessWhere and the predicate list in journeyAcl to import and use that shared helper so both list filters and object-level checks derive from the same source of truth.apis/api-journeys-modern/src/schema/journey/adminJourneys.query.spec.ts (2)
34-53: Add astatusfilter case.
statusis part of the new query contract, but this suite never passes it, so a broken status predicate in the resolver would still go green.🤖 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.spec.ts` around lines 34 - 53, The test never exercises the new status argument of the ADMIN_JOURNEYS_QUERY, so add a case that passes a status variable into the adminJourneys GraphQL query to validate the resolver predicate; update the spec that uses ADMIN_JOURNEYS_QUERY to invoke the query with variables like { status: ['PUBLISHED'] } (or another JourneyStatus enum value used in fixtures), and assert the returned journeys match the expected filtered set (or that none are returned) so a broken status filter will fail the test.
193-197: Avoid asserting onAND[1] === {}.This couples the test to the resolver's current filter order and its use of a no-op placeholder. Assert on the absence of the personal-fallback predicate instead so harmless refactors do not break the test.
🤖 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.spec.ts` around lines 193 - 197, The test currently asserts andFilters[1] === {}, which couples it to filter ordering and a no-op placeholder; instead, update the assertion on prismaMock.journey.findMany (inspect lastFindManyCall?.where?.AND via andFilters) to ensure the resolver's personal-fallback predicate is absent: assert that no element in andFilters matches the personal-fallback predicate (e.g., no element is an empty object and none contain the specific personal-fallback key your resolver adds), using a check like Array.prototype.every to confirm no AND entry equals {} or contains the personal-fallback field rather than asserting at a fixed index.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apis/api-journeys-modern/src/schema/journey/adminJourneys.query.spec.ts`:
- Around line 240-273: The test "should allow publishers to read templates
regardless of status" uses mockJourney with status 'published', so change it to
an unpublished status (e.g., set mockJourney.status = 'draft' or use a dedicated
unpublished template fixture) before calling prismaMock.journey.findMany so the
resolved journey is an unpublished template; keep the template flag true and the
same assertions so the test demonstrates that publisher users
(mockPublisherUser) can read templates regardless of status (references:
mockJourney, mockPublisherUser, prismaMock.journey.findMany,
ADMIN_JOURNEYS_QUERY).
In `@apis/api-journeys-modern/src/schema/journey/adminJourneys.query.ts`:
- Around line 37-57: Reorder and tighten the team-id logic so explicit
args.teamId takes precedence: first check if args.teamId != null and set
filter.teamId = args.teamId (and skip any profile lookup), otherwise if
args.useLastActiveTeamId === true then call
prisma.journeyProfile.findUnique(...) and only set filter.teamId and
lastActiveApplied = true when profile != null and profile.lastActiveTeamId !=
null; do not flip lastActiveApplied when profile.lastActiveTeamId is null so the
subsequent owner/editor fallback (the branch guarded by !lastActiveApplied and
journeyReadAccessWhere(...)) still runs. Reference: args.teamId,
args.useLastActiveTeamId, prisma.journeyProfile.findUnique,
profile.lastActiveTeamId, lastActiveApplied, filter.teamId,
journeyReadAccessWhere.
---
Nitpick comments:
In `@apis/api-journeys-modern/src/schema/journey/adminJourneys.query.spec.ts`:
- Around line 34-53: The test never exercises the new status argument of the
ADMIN_JOURNEYS_QUERY, so add a case that passes a status variable into the
adminJourneys GraphQL query to validate the resolver predicate; update the spec
that uses ADMIN_JOURNEYS_QUERY to invoke the query with variables like { status:
['PUBLISHED'] } (or another JourneyStatus enum value used in fixtures), and
assert the returned journeys match the expected filtered set (or that none are
returned) so a broken status filter will fail the test.
- Around line 193-197: The test currently asserts andFilters[1] === {}, which
couples it to filter ordering and a no-op placeholder; instead, update the
assertion on prismaMock.journey.findMany (inspect lastFindManyCall?.where?.AND
via andFilters) to ensure the resolver's personal-fallback predicate is absent:
assert that no element in andFilters matches the personal-fallback predicate
(e.g., no element is an empty object and none contain the specific
personal-fallback key your resolver adds), using a check like
Array.prototype.every to confirm no AND entry equals {} or contains the
personal-fallback field rather than asserting at a fixed index.
In `@apis/api-journeys-modern/src/schema/journey/journey.acl.ts`:
- Around line 37-60: The ACL predicates used in journeyReadAccessWhere are
duplicated with journeyAcl; extract the common role/template predicates into a
shared helper (e.g., export a constant or function like
buildJourneyAccessPredicates or journeyAccessPredicates) that returns the array
of OR conditions for team manager/member, team member, userJourneys
owner/editor, template: true & published, and the optional template-only when
isPublisher; then replace the inline OR array in journeyReadAccessWhere and the
predicate list in journeyAcl to import and use that shared helper so both list
filters and object-level checks derive from the same source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2ca2e287-ba73-46ea-8aef-dc6d48907cc3
📒 Files selected for processing (7)
apis/api-gateway/schema.graphqlapis/api-journeys-modern/schema.graphqlapis/api-journeys-modern/src/schema/journey/adminJourneys.query.spec.tsapis/api-journeys-modern/src/schema/journey/adminJourneys.query.tsapis/api-journeys-modern/src/schema/journey/index.tsapis/api-journeys-modern/src/schema/journey/journey.acl.tsapis/api-journeys-modern/src/yoga.ts
Comment on lines
+37
to
+57
| if (args.useLastActiveTeamId === true) { | ||
| const profile = await prisma.journeyProfile.findUnique({ | ||
| where: { userId } | ||
| }) | ||
| if (profile == null) | ||
| throw new GraphQLError('journey profile not found', { | ||
| extensions: { code: 'NOT_FOUND' } | ||
| }) | ||
| lastActiveApplied = true | ||
| if (profile.lastActiveTeamId != null) { | ||
| filter.teamId = profile.lastActiveTeamId | ||
| } | ||
| } | ||
|
|
||
| if (args.teamId != null) { | ||
| filter.teamId = args.teamId | ||
| } else if ( | ||
| args.template !== true && | ||
| filter.teamId == null && | ||
| !lastActiveApplied | ||
| ) { |
Contributor
There was a problem hiding this comment.
Handle explicit teamId before the last-active lookup.
When both args are set, Lines 37-44 can still throw NOT_FOUND before Line 52 gives teamId precedence. Also, Line 45 flips lastActiveApplied even when profile.lastActiveTeamId is null, which skips the Lines 53-57 owner/editor fallback and can widen the query to whatever journeyReadAccessWhere(...) allows.
Possible fix if the intended precedence is teamId → last-active team → fallback filter
- if (args.useLastActiveTeamId === true) {
+ if (args.teamId != null) {
+ filter.teamId = args.teamId
+ } else if (args.useLastActiveTeamId === true) {
const profile = await prisma.journeyProfile.findUnique({
where: { userId }
})
if (profile == null)
throw new GraphQLError('journey profile not found', {
extensions: { code: 'NOT_FOUND' }
})
- lastActiveApplied = true
if (profile.lastActiveTeamId != null) {
filter.teamId = profile.lastActiveTeamId
+ lastActiveApplied = true
}
- }
-
- if (args.teamId != null) {
- filter.teamId = args.teamId
} else if (
args.template !== true &&
filter.teamId == null &&
!lastActiveApplied
) {Based on learnings, "journey data is public and does not require authorization checks for read access in GraphQL queries like journeySimpleGet."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (args.useLastActiveTeamId === true) { | |
| const profile = await prisma.journeyProfile.findUnique({ | |
| where: { userId } | |
| }) | |
| if (profile == null) | |
| throw new GraphQLError('journey profile not found', { | |
| extensions: { code: 'NOT_FOUND' } | |
| }) | |
| lastActiveApplied = true | |
| if (profile.lastActiveTeamId != null) { | |
| filter.teamId = profile.lastActiveTeamId | |
| } | |
| } | |
| if (args.teamId != null) { | |
| filter.teamId = args.teamId | |
| } else if ( | |
| args.template !== true && | |
| filter.teamId == null && | |
| !lastActiveApplied | |
| ) { | |
| if (args.teamId != null) { | |
| filter.teamId = args.teamId | |
| } else if (args.useLastActiveTeamId === true) { | |
| const profile = await prisma.journeyProfile.findUnique({ | |
| where: { userId } | |
| }) | |
| if (profile == null) | |
| throw new GraphQLError('journey profile not found', { | |
| extensions: { code: 'NOT_FOUND' } | |
| }) | |
| if (profile.lastActiveTeamId != null) { | |
| filter.teamId = profile.lastActiveTeamId | |
| lastActiveApplied = true | |
| } | |
| } else if ( | |
| args.template !== true && | |
| filter.teamId == null && | |
| !lastActiveApplied | |
| ) { |
🤖 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 37 - 57, Reorder and tighten the team-id logic so explicit args.teamId
takes precedence: first check if args.teamId != null and set filter.teamId =
args.teamId (and skip any profile lookup), otherwise if args.useLastActiveTeamId
=== true then call prisma.journeyProfile.findUnique(...) and only set
filter.teamId and lastActiveApplied = true when profile != null and
profile.lastActiveTeamId != null; do not flip lastActiveApplied when
profile.lastActiveTeamId is null so the subsequent owner/editor fallback (the
branch guarded by !lastActiveApplied and journeyReadAccessWhere(...)) still
runs. Reference: args.teamId, args.useLastActiveTeamId,
prisma.journeyProfile.findUnique, profile.lastActiveTeamId, lastActiveApplied,
filter.teamId, journeyReadAccessWhere.
|
I see you added the "on stage" label, I'll get this merged to the stage branch! |
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.
Summary by CodeRabbit
Release Notes