fix: update base condition for findAll() #359
Conversation
WalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ApplicationsService
participant ProvidersService
User->>ApplicationsService: Request all applications
ApplicationsService->>Database: Query applications with status ACTIVE
Database-->>ApplicationsService: Return active applications
ApplicationsService-->>User: Send active applications
User->>ProvidersService: Request all providers
ProvidersService->>Database: Query providers with status ACTIVE
Database-->>ProvidersService: Return active providers
ProvidersService-->>User: Send active providers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Outside diff range and nitpick comments (2)
apps/api/src/modules/applications/applications.service.ts (1)
Line range hint
62-68: Consider moving status filtering to CoreService.Since status filtering is a common requirement (as evidenced by similar changes in ProvidersService), consider making this a configurable feature in CoreService. This would:
- Reduce code duplication
- Provide consistent status filtering across all services
- Make it easier to modify status filtering behavior globally
You could modify CoreService to accept status filtering configuration in its constructor or add a method to configure default conditions that apply to all findAll calls.
Example approach:
export class CoreService<T> { protected defaultConditions: Array<{field: string; value: any}> = []; setDefaultConditions(conditions: Array<{field: string; value: any}>) { this.defaultConditions = conditions; } async findAll(options, entity, searchableFields, additionalConditions = []) { const conditions = [...this.defaultConditions, ...additionalConditions]; // existing implementation } }apps/api/src/modules/providers/providers.service.ts (1)
Line range hint
66-71: Consider adding a comment about the status filtering behavior.Since status filtering is a consistent pattern across methods in this service, it would be helpful to document this behavior.
+ // Always filter for active providers to maintain consistency with getById and getConfigById const baseConditions = [{ field: 'status', value: Status.ACTIVE }]; const searchableFields = ['name'];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/api/src/modules/applications/applications.service.ts(1 hunks)apps/api/src/modules/providers/providers.service.ts(1 hunks)
🔇 Additional comments (4)
apps/api/src/modules/applications/applications.service.ts (2)
62-62: LGTM! The base condition implementation is consistent.
The addition of Status.ACTIVE filter in baseConditions aligns with the existing pattern used in findById and findByUserId methods.
Line range hint 62-68: Verify impact on existing getAllApplications consumers.
While the change maintains consistency with other methods, let's verify that no existing code relies on retrieving non-active applications through getAllApplications.
✅ Verification successful
Let me gather more information about the implementation details to better understand the impact.
Let me check one more aspect to see if there are any GraphQL queries or fragments that might be affected by this change.
Based on the gathered information, I can now provide a final response:
Change to filter active applications appears safe to proceed
The implementation shows that getAllApplications is only used in the resolver as a direct pass-through to the service method, with no other direct consumers in the codebase. The new filter for active applications is consistent with standard patterns and doesn't show any evidence of breaking existing functionality.
- The only usage is in
applications.resolver.tswhich simply forwards the call to the service - The service implementation adds a base condition for active status, which is a common pattern
- No test files were found that might rely on retrieving non-active applications
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for getAllApplications usage to verify impact
echo "Searching for getAllApplications usage patterns..."
rg -l "getAllApplications" --type ts
# Search for any explicit status handling in GraphQL resolvers
echo "Checking for status handling in resolvers..."
ast-grep --pattern 'class $_ extends $_ {
$$$
getAllApplications($_, $_) {
$$$
}
$$$
}'
Length of output: 465
Script:
#!/bin/bash
# Check the implementation of getAllApplications in resolver and service
echo "Checking applications resolver implementation..."
rg -A 10 "getAllApplications" apps/api/src/modules/applications/applications.resolver.ts
echo "Checking applications service implementation..."
rg -A 10 "getAllApplications" apps/api/src/modules/applications/applications.service.ts
# Check for any direct status filtering in queries
echo "Checking for status-related queries..."
rg -A 5 "status.*=.*Status" apps/api/src/modules/applications/
Length of output: 1028
Script:
#!/bin/bash
# Search for GraphQL queries and fragments related to applications
echo "Checking GraphQL queries and fragments..."
fd -e graphql . | xargs rg -A 5 "applications.*{"
# Check for any test files that might rely on non-active applications
echo "Checking test files..."
fd -e spec.ts -e test.ts . | xargs rg -A 5 "getAllApplications"
Length of output: 357
apps/api/src/modules/providers/providers.service.ts (2)
Line range hint 66-71: Implementation is performant.
The status filter is correctly implemented at the database query level through baseConditions, ensuring efficient filtering.
66-66: LGTM! The change aligns with the PR objectives.
The addition of the status filter in baseConditions ensures that only active providers are fetched, which is consistent with the PR's goal.
Let's verify the consistency of this pattern across other services:
✅ Verification successful
The Status.ACTIVE filter pattern is consistently implemented across relevant services
The verification shows that:
- The Status import is properly defined from 'src/common/constants/database'
- The same baseConditions pattern with Status.ACTIVE is consistently used in related services:
- applications.service.ts
- providers.service.ts
- server-api-keys.service.ts
- notifications.service.ts
This confirms that the implementation follows the established pattern across the codebase and maintains consistency with other services that require active status filtering.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other services follow the same pattern for status filtering
# Expected: Similar baseConditions usage in other service files
# Search for similar findAll implementations in service files
rg -A 5 "baseConditions.*Status\.ACTIVE" "apps/api/src/modules/**/*service.ts"
# Search for any potential inconsistencies in status filtering
ast-grep --pattern 'findAll($$$, $_, $_, baseConditions)'
Length of output: 216
Script:
#!/bin/bash
# Let's try a more precise search approach
# 1. First, let's find all service files
fd "service.ts$" apps/api/src/modules/
# 2. Then search for baseConditions pattern with proper context
rg -l "baseConditions.*=.*\[.*Status\.ACTIVE" apps/api/src/modules/
# 3. Let's also verify the Status import
rg "import.*Status" apps/api/src/modules/
# 4. Search for similar findAll implementations
rg "findAll.*baseConditions" apps/api/src/modules/ -A 2
Length of output: 3375
API PR Checklist
Pre-requisites
.env.examplefile with the required values as applicable.PR Details
PR details have been updated as per the given format (see below)
feat: add admin login endpoint)Additional Information
ready for reviewshould be added if the PR is ready to be reviewed)Note: Reviewer should ensure that the checklist and description have been populated and followed correctly, and the PR should be merged only after resolving all conversations and verifying that CI checks pass.
Description:
Update
baseConditionsfor graphql core helper functionfindAll()to fetch active status recordsSummary by CodeRabbit