fix(condo): DOMA-12708 added ticket visibility logic to include observers #7090
Conversation
📝 WalkthroughWalkthroughAdded ticket observer support: fetched observers via new GraphQL hook, threaded observers and the Changes
Sequence Diagram(s)sequenceDiagram
participant Page as Ticket Page
participant GQL as GraphQL (GetTicketObserversByTicketId)
participant Context as TicketVisibilityContext
participant Flags as FeatureFlagsService
Page->>GQL: fetch observers (ticketId)
GQL-->>Page: observers data
Page->>Flags: read TICKET_OBSERVERS flag
Flags-->>Page: isTicketObserversEnabled
Page->>Context: canEmployeeReadTicket(ticket, observers, flag)
Context->>Context: evaluate visibility (assignee/executor/property/specialization + observer check)
Context-->>Page: boolean (canRead)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fca4669998
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| { | ||
| assignee: { id: userId }, | ||
| executor: { id: userId }, | ||
| observers_some: { user: { id: userId } }, |
There was a problem hiding this comment.
The new observers_some condition is added to the same OR item as assignee and executor. In Keystone where objects, fields inside one OR item are ANDed, so this now requires a ticket to have the user as assignee, executor, and observer to match. That is stricter than before and will hide tickets where the user is assigned/executor but not explicitly added as an observer (especially in the property/specialization visibility paths that rely on assignedTicketFiltersQuery). If the intent is to include observers, observers_some needs to be a separate OR entry instead of an additional field in the same object.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/condo/pages/ticket/[id]/index.tsx (1)
952-961: Consider simplifying the chained filters.The two
.filter()calls can be combined into a single pass for slightly better readability and efficiency.♻️ Suggested simplification
- const observers = useMemo(() => observersData?.observers?.filter(Boolean)?.filter(observer => observer?.user?.id) || [], [observersData?.observers]) + const observers = useMemo( + () => observersData?.observers?.filter(observer => observer && observer?.user?.id) || [], + [observersData?.observers] + )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/condo/domains/ticket/contexts/TicketVisibilityContext.tsxapps/condo/pages/ticket/[id]/index.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Use 4 spaces for indentation in JavaScript/TypeScript
Use single quotes for strings and JSX attributes
Never use semicolons
Always use spaces inside curly braces: { foo }
Always use trailing commas for multiline arrays, objects, and imports
Always require space before function parentheses
Enforce import order groups: builtin → external → @open-condo → internal → sibling → parent with newlines between groups
Alphabetize imports within each group in case-insensitive ascending order
Use specific lodash imports (e.g., import get from 'lodash/get') instead of named imports from the main lodash package
Don't use jspdf - use pdfmake instead
Don't import from @open-keystone/fields* or @open-condo/keystone/fields
Use async/await for promises instead of .then() chains
Use nosemgrep: comments to suppress Semgrep false positives with explanations for why the code is safe
Files:
apps/condo/domains/ticket/contexts/TicketVisibilityContext.tsxapps/condo/pages/ticket/[id]/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript version 5.8.3 or higher with strict mode recommended and @typescript-eslint/parser
Use 'interface' over 'type' for defining object shapes in TypeScript
Files:
apps/condo/domains/ticket/contexts/TicketVisibilityContext.tsxapps/condo/pages/ticket/[id]/index.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{tsx,jsx}: Use @open-condo/ui components for GUI elements instead of creating custom UI elements
Import UI components and types from @open-condo/ui package
Import icons from @open-condo/icons instead of other icon libraries
Use Space component for layout spacing instead of custom margins
Use Typography components for all text rendering
Import styles from @open-condo/ui/dist/styles.min.css in root component
Use useBreakpoints hook from @open-condo/ui/hooks for responsive design
Use useContainerSize hook from @open-condo/ui/hooks for container dimensions
Files:
apps/condo/domains/ticket/contexts/TicketVisibilityContext.tsxapps/condo/pages/ticket/[id]/index.tsx
**/*.{tsx,jsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Import colors from @open-condo/ui/colors for consistent theming in JavaScript/TypeScript
Files:
apps/condo/domains/ticket/contexts/TicketVisibilityContext.tsxapps/condo/pages/ticket/[id]/index.tsx
🧬 Code graph analysis (1)
apps/condo/pages/ticket/[id]/index.tsx (1)
apps/condo/gql/index.ts (1)
useGetTicketObserversByTicketIdQuery(7562-7565)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build open-source part (ubuntu-22.04, 24.x, 3.14)
- GitHub Check: [review] Deploy
- GitHub Check: Lint source code (24.x)
- GitHub Check: Check dependencies integrity (24.x)
- GitHub Check: Semgrep vulnerabilities check
🔇 Additional comments (8)
apps/condo/pages/ticket/[id]/index.tsx (2)
13-14: LGTM!The import addition follows the established pattern and is correctly placed within the existing import group from
@app/condo/gql.
984-994: LGTM!The loading state correctly includes
observersLoadingto prevent premature access denial, and thecanEmployeeReadTicketcall properly passes the observers data as required by the updated signature.apps/condo/domains/ticket/contexts/TicketVisibilityContext.tsx (6)
7-8: LGTM!The import is correctly placed and follows the established import order.
149-152: LGTM!The observer check is correctly implemented and follows the same pattern as the existing assignee/executor check.
164-197: LGTM!The observer visibility check is correctly integrated into all visibility type branches. The OR logic (
|| isUserIsTicketObserver) properly extends access to observers in the same way as assignees and executors.
28-28: LGTM!The interface correctly types the
observersparameter using the query result type.
281-291: LGTM!The callback correctly passes the observers parameter through to
isEmployeeCanReadTicket, and the dependency array appropriately excludesobserverssince it's a function parameter, not an external dependency.
44-52: Verify query filter semantics and test visibility behavior.The structure places
assignee,executor, andobservers_someas sibling fields within a single object in the OR array. In standard GraphQL filtering, this means all three conditions must match (AND semantics), not any one of them (OR semantics).If the intent is to allow access when the user is the assignee OR executor OR observer, separate objects in the OR array would be more semantically correct:
Suggested alternative structure
const assignedTicketFiltersQuery = { OR: [ { assignee: { id: userId } }, { executor: { id: userId } }, { observers_some: { user: { id: userId } } }, ], }This pattern existed before the observers_some addition, so confirm whether the current behavior matches your intended visibility logic. Consider adding tests for the visibility filtering to ensure it works as expected.
… visibility logic
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @apps/condo/domains/ticket/contexts/TicketVisibilityContext.tsx:
- Around line 47-55: assignedTicketFiltersQuery currently places assignee,
executor and observer checks inside a single object in the OR array which
creates an AND between those fields; change it so each condition is its own
object in the OR array (i.e., OR: [ { assignee: { id: userId } }, { executor: {
id: userId } }, ...(isTicketObserversEnabled ? [{ observers_some: { user: { id:
userId } } }] : []) ]) so the query uses logical OR across
assignee/executor/observer; update the creation of assignedTicketFiltersQuery
accordingly, keeping the isTicketObserversEnabled conditional but returning an
array element rather than merging into one object.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/condo/domains/ticket/contexts/TicketVisibilityContext.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Use 4 spaces for indentation in JavaScript/TypeScript
Use single quotes for strings and JSX attributes
Never use semicolons
Always use spaces inside curly braces: { foo }
Always use trailing commas for multiline arrays, objects, and imports
Always require space before function parentheses
Enforce import order groups: builtin → external → @open-condo → internal → sibling → parent with newlines between groups
Alphabetize imports within each group in case-insensitive ascending order
Use specific lodash imports (e.g., import get from 'lodash/get') instead of named imports from the main lodash package
Don't use jspdf - use pdfmake instead
Don't import from @open-keystone/fields* or @open-condo/keystone/fields
Use async/await for promises instead of .then() chains
Use nosemgrep: comments to suppress Semgrep false positives with explanations for why the code is safe
Files:
apps/condo/domains/ticket/contexts/TicketVisibilityContext.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript version 5.8.3 or higher with strict mode recommended and @typescript-eslint/parser
Use 'interface' over 'type' for defining object shapes in TypeScript
Files:
apps/condo/domains/ticket/contexts/TicketVisibilityContext.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{tsx,jsx}: Use @open-condo/ui components for GUI elements instead of creating custom UI elements
Import UI components and types from @open-condo/ui package
Import icons from @open-condo/icons instead of other icon libraries
Use Space component for layout spacing instead of custom margins
Use Typography components for all text rendering
Import styles from @open-condo/ui/dist/styles.min.css in root component
Use useBreakpoints hook from @open-condo/ui/hooks for responsive design
Use useContainerSize hook from @open-condo/ui/hooks for container dimensions
Files:
apps/condo/domains/ticket/contexts/TicketVisibilityContext.tsx
**/*.{tsx,jsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Import colors from @open-condo/ui/colors for consistent theming in JavaScript/TypeScript
Files:
apps/condo/domains/ticket/contexts/TicketVisibilityContext.tsx
🧬 Code graph analysis (1)
apps/condo/domains/ticket/contexts/TicketVisibilityContext.tsx (6)
apps/condo/gql/index.ts (1)
GetTicketByIdQueryResult(6280-6280)apps/condo/gql/operation.types.ts (1)
GetTicketObserversByTicketIdQuery(1184-1184)apps/condo/domains/ticket/access/TicketObserver.js (1)
observer(52-52)apps/condo/domains/organization/constants/common.js (1)
ASSIGNED_TICKET_VISIBILITY(4-4)packages/featureflags/FeatureFlagsContext.tsx (1)
useFeatureFlags(231-231)apps/condo/domains/common/constants/featureflags.js (1)
TICKET_OBSERVERS(44-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build open-source part (ubuntu-22.04, 24.x, 3.14)
- GitHub Check: [review] Deploy
- GitHub Check: Lint source code (24.x)
- GitHub Check: Check dependencies integrity (24.x)
- GitHub Check: Semgrep vulnerabilities check
- GitHub Check: Build Docker Image
🔇 Additional comments (4)
apps/condo/domains/ticket/contexts/TicketVisibilityContext.tsx (4)
1-24: LGTM!The new imports are properly organized following the established pattern (external → @open-condo → internal) and all imported items are utilized in the code.
152-200: LGTM!The observer logic is correctly integrated across all visibility branches:
- Proper null-safety with
Array.isArray(observers)check and optional chaining (observer?.user?.id)- Feature flag check ensures backward compatibility when
isTicketObserversEnabledis false- Observer check consistently added alongside assignee/executor checks in all relevant branches
220-222: LGTM!Feature flag integration follows the established pattern using
useFeatureFlagshook.
289-300: LGTM!The callback correctly:
- Accepts
observersas a parameter (aligned with updated interface at line 30)- Includes
isTicketObserversEnabledin the dependency array as it's a closure variable- Properly excludes
observersfrom deps since it's a function parameter, not a closure capture
| const assignedTicketFiltersQuery = { | ||
| OR: [ | ||
| { | ||
| assignee: { id: userId }, | ||
| executor: { id: userId }, | ||
| ...(isTicketObserversEnabled ? { observers_some: { user: { id: userId } } } : {}), | ||
| }, | ||
| ], | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the actual file to understand the context better
head -70 apps/condo/domains/ticket/contexts/TicketVisibilityContext.tsx | tail -30Repository: open-condo-software/condo
Length of output: 1070
🏁 Script executed:
# Search for similar filter patterns to understand OR/AND logic conventions
rg -n -A8 'OR:\s*\[' apps/condo/domains/ --type ts --type tsx | head -100Repository: open-condo-software/condo
Length of output: 96
🏁 Script executed:
# Look for the isEmployeeCanReadTicket function referenced in the comment
rg -n 'isEmployeeCanReadTicket' apps/condo/domains/ticket/contexts/TicketVisibilityContext.tsx -B2 -A15Repository: open-condo-software/condo
Length of output: 1566
🏁 Script executed:
# Search for Keystone filter documentation or examples with multiple field conditions
rg -n 'assignee.*executor' apps/condo/domains/ --type ts --type tsx | head -20Repository: open-condo-software/condo
Length of output: 96
🏁 Script executed:
# Find where assignedTicketFiltersQuery is used
rg -n 'assignedTicketFiltersQuery' apps/condo/domains/ticket/contexts/TicketVisibilityContext.tsx -B2 -A10Repository: open-condo-software/condo
Length of output: 2549
🏁 Script executed:
# Look for other examples of OR filters with multiple conditions in one object
rg -n 'OR.*\[' apps/condo/domains/ticket/ | head -20Repository: open-condo-software/condo
Length of output: 2591
🏁 Script executed:
# Search for how filters are constructed elsewhere - look for field patterns
rg -n 'assignee.*executor' apps/condo/domains/ticket/ | head -30Repository: open-condo-software/condo
Length of output: 6811
🏁 Script executed:
# Check the file for imports and understand what filter API is being used
head -40 apps/condo/domains/ticket/contexts/TicketVisibilityContext.tsxRepository: open-condo-software/condo
Length of output: 1587
🏁 Script executed:
# Look at the GraphQL query patterns to understand the filter semantics
rg -n "OR.*assignee.*executor" apps/condo/domains/ticket/ -B2 -A2Repository: open-condo-software/condo
Length of output: 983
🏁 Script executed:
# Check test files to see if the filter logic is tested and what the expected behavior is
rg -n "assignedTicketFiltersQuery\|assignee.*executor.*observer" apps/condo/domains/ticket/ --type ts -A5 -B2Repository: open-condo-software/condo
Length of output: 51
🏁 Script executed:
# Check the Keystone/Apollo filter schema to understand the intended semantics
cat apps/condo/schema.graphql 2>/dev/null | grep -A 10 "TicketWhereInput" | head -30 || rg -n "input TicketWhereInput" apps/ -A 15 | head -40Repository: open-condo-software/condo
Length of output: 871
🏁 Script executed:
# Look for tests that verify the filter behavior
find apps/condo/domains/ticket -name "*.spec.ts" -o -name "*.spec.tsx" | xargs grep -l "assignedTicketFiltersQuery\|isEmployeeCanReadTicket" 2>/dev/null | head -5Repository: open-condo-software/condo
Length of output: 51
🏁 Script executed:
# Check if there are tests for the filter behavior in TicketVisibilityContext
find apps/condo/domains/ticket -name "*TicketVisibility*.spec.*" -o -name "*TicketVisibility*.test.*" 2>/dev/null | head -5Repository: open-condo-software/condo
Length of output: 51
🏁 Script executed:
# Search more broadly for test files that test the visibility filters
rg -l "getTicketsQueryByTicketVisibilityType\|assignedTicketFiltersQuery" apps/condo/ --type tsRepository: open-condo-software/condo
Length of output: 51
🏁 Script executed:
# Look at the actual usage of assignedTicketFiltersQuery to understand how it combines with other filters
rg -n "assignedTicketFiltersQuery" apps/condo/domains/ticket/contexts/TicketVisibilityContext.tsx -B3 -A3 | head -80Repository: open-condo-software/condo
Length of output: 1571
🏁 Script executed:
# Check if there's documentation or comments explaining the filter logic
rg -n "AND|OR" apps/condo/domains/ticket/contexts/TicketVisibilityContext.tsx | head -30Repository: open-condo-software/condo
Length of output: 572
🏁 Script executed:
# Check the exact semantic meaning by looking at how GraphQL OR works in KeystoneJS
rg -n "OR.*\[" apps/condo/domains/ticket/queries/Ticket.graphql -A5 | head -50Repository: open-condo-software/condo
Length of output: 51
🏁 Script executed:
# Let's understand the semantics by checking if there are any comments or tests about field combination
rg -n "AND.*OR\|field.*combine" apps/condo/domains/ticket/contexts/ -iRepository: open-condo-software/condo
Length of output: 51
🏁 Script executed:
# Check if the issue was identified before or if there are related discussions
rg -n "assignee.*executor.*observer" apps/condo/ --type ts -B2 -A8 | head -80Repository: open-condo-software/condo
Length of output: 6026
🏁 Script executed:
# Let's verify the semantics by checking how Keystone/Prisma interprets OR with multiple fields
# Look for documentation or examples in the codebase comments
rg -n "OR.*assignee\|executor.*OR" apps/condo/domains/ -i -B3 -A3 | head -100Repository: open-condo-software/condo
Length of output: 51
🏁 Script executed:
# Check if the filter actually causes issues by searching for bug reports or issues related to visibility
rg -n "visibility.*filter\|filter.*visibility" apps/condo/domains/ticket/ -i -B2 -A5 | head -50Repository: open-condo-software/condo
Length of output: 51
🏁 Script executed:
# Most importantly, let's see how the .graphql files use OR - they use shorthand
# which should expand to separate objects, not a single object
cat apps/condo/domains/ticket/queries/Ticket.graphql | grep -A 10 "OR:"Repository: open-condo-software/condo
Length of output: 683
🏁 Script executed:
# Now let's verify the Keystone/Prisma semantics more carefully by looking at actual usage
# Check how OR arrays with single vs multiple fields are used elsewhere
rg -n 'OR:\s*\[' apps/condo/domains/ticket/ -A10 | head -150Repository: open-condo-software/condo
Length of output: 13445
🏁 Script executed:
# Let's also check the GraphQL schema definition for OR to confirm the semantics
rg -n 'OR.*TicketWhereInput\]' apps/ -B5 -A5Repository: open-condo-software/condo
Length of output: 51
Fix filter logic to use OR instead of AND for assignee/executor/observer conditions.
The assignedTicketFiltersQuery incorrectly combines all conditions in a single object within the OR array. In Keystone's filter semantics, multiple fields in one object represent AND logic. This means a ticket would need to match ALL conditions simultaneously (assignee AND executor AND observer), which is not achievable.
The codebase pattern shows that separate OR conditions should use distinct objects in the array. This is evident in tables.utils.spec.ts where { OR: [ { field1 }, { field2 } ] } produces OR logic between conditions, and confirmed by isEmployeeCanReadTicket (line 155) which correctly uses boolean OR: ticket?.assignee?.id === userId || ticket?.executor?.id === userId.
Change the filter structure to:
const assignedTicketFiltersQuery = {
OR: [
{ assignee: { id: userId } },
{ executor: { id: userId } },
...(isTicketObserversEnabled ? [{ observers_some: { user: { id: userId } } }] : []),
],
}🤖 Prompt for AI Agents
In @apps/condo/domains/ticket/contexts/TicketVisibilityContext.tsx around lines
47 - 55, assignedTicketFiltersQuery currently places assignee, executor and
observer checks inside a single object in the OR array which creates an AND
between those fields; change it so each condition is its own object in the OR
array (i.e., OR: [ { assignee: { id: userId } }, { executor: { id: userId } },
...(isTicketObserversEnabled ? [{ observers_some: { user: { id: userId } } }] :
[]) ]) so the query uses logical OR across assignee/executor/observer; update
the creation of assignedTicketFiltersQuery accordingly, keeping the
isTicketObserversEnabled conditional but returning an array element rather than
merging into one object.
|
tolmachev21
deleted the
fix/condo/DOMA-12708/ticket-observers-visibility
branch
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.