Test: src/graphql/types/Community/updater.ts #3110
Conversation
Signed-off-by: NishantSinghhhhh <nishantsingh_230137@aitpune.edu.in>
Signed-off-by: NishantSinghhhhh <nishantsingh_230137@aitpune.edu.in>
|
Warning Rate limit exceeded@NishantSinghhhhh has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request introduces a new GraphQL resolver for the Community type. A new type, Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as CommunityResolver.updater
participant DB as Database (usersTable)
participant CTX as GraphQLContext
C->>R: Request updater for Community
R->>CTX: Check if user is authenticated
alt Unauthenticated
R-->>C: Throw TalawaGraphQLError
else Authenticated
R->>R: Check if parent has updaterId
alt No updaterId
R-->>C: Return null
else updaterId exists
R->>DB: Query for user with updaterId
alt User not found
R->>R: Log missing user message
R-->>C: Return null
else User found
R->>DB: Query again for updater details
R-->>C: Return fetched user
end
end
end
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3110 +/- ##
====================================================
+ Coverage 39.63% 39.70% +0.06%
====================================================
Files 454 454
Lines 33477 33528 +51
Branches 397 407 +10
====================================================
+ Hits 13270 13311 +41
- Misses 20207 20217 +10 ☔ View full report in Codecov by Sentry. |
![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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/graphql/context.ts(1 hunks)src/graphql/types/Community/Community.ts(1 hunks)test/graphql/types/Community/updater.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (3)
src/graphql/context.ts (1)
17-17: LGTM! Enhanced type safety with role information.Adding the 'role' field to the user type in
ExplicitAuthenticationTokenPayloadimproves type safety and makes authorization requirements explicit.src/graphql/types/Community/Community.ts (1)
22-38: LGTM! Robust authentication and authorization checks.The implementation properly validates user authentication and authorization with clear error messages.
test/graphql/types/Community/updater.test.ts (1)
150-183: LGTM! Comprehensive test coverage.The test suite thoroughly covers the core functionality, edge cases, and error handling scenarios.
Signed-off-by: NishantSinghhhhh <nishantsingh_230137@aitpune.edu.in>
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
|
@palisadoes , there are type errors coming from files that i did not even touch |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/graphql/context.ts (1)
Line range hint
5-21: Fix formatting issues flagged by biome check.The formatter has detected issues with:
- Empty line at line 5
- Type declaration formatting (lines 6-8)
- Closing bracket indentation (line 21)
Apply the following formatting fixes:
- -export type AuthTokenUserFields = Pick<typeof usersTable.$inferSelect, "id" | "role">; - +export type AuthTokenUserFields = Pick< + typeof usersTable.$inferSelect, + "id" | "role" +>; export type ExplicitAuthenticationTokenPayload = { user: AuthTokenUserFields; - }; +};
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/graphql/context.ts(2 hunks)test/graphql/types/Community/updater.test.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pull request workflow
src/graphql/context.ts
[error] 5-21: File formatting issues detected by biome check. The formatter would have printed changes for the 'AuthTokenUserFields' type declaration (lines 5-8) and for the closing bracket in 'ExplicitAuthenticationTokenPayload' (line 21). Please apply the suggested formatting changes.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (7)
src/graphql/context.ts (1)
6-6: LGTM! Type safety improvements.The changes enhance type safety by:
- Creating a reusable
AuthTokenUserFieldstype- Updating
ExplicitAuthenticationTokenPayloadto include the user's roleAlso applies to: 20-20
test/graphql/types/Community/updater.test.ts (6)
24-33: LGTM! Well-structured test context interface.The
TestContextinterface properly extendsGraphQLContextwhile overriding the necessary types for testing.
84-148: LGTM! Comprehensive test setup.The test setup is thorough and includes:
- Mock user with admin role
- Mock community with all required fields
- Complete test context with all necessary dependencies
150-156: LGTM! Core functionality tests.Good coverage of the main success paths:
- Fetching updater user
- Handling null updaterId
Also applies to: 158-170
172-183: LGTM! Authentication and authorization tests.Thorough testing of security checks:
- Unauthenticated user handling
- Unauthorized (non-admin) user handling
Also applies to: 185-202
225-242: LGTM! Error handling test.Good coverage of database error scenario with proper error logging verification.
244-281: LGTM! Edge case handling.Comprehensive test for fetching different user with proper verification of:
- Multiple database calls
- Correct updaterId usage
|
Did you merge with the latest upstream? The sign up file was fixed a few days ago |
Signed-off-by: NishantSinghhhhh <nishantsingh_230137@aitpune.edu.in>
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
Yes I did , My code was up to date |
Signed-off-by: NishantSinghhhhh <nishantsingh_230137@aitpune.edu.in>
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/graphql/context.ts(1 hunks)src/graphql/types/Community/Community.ts(1 hunks)test/graphql/types/Community/updater.test.ts(1 hunks)test/utilities/mockLogger.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (6)
src/graphql/types/Community/Community.ts (2)
11-17: Type definition looks good.
No issues observed with theCommunityResolversdefinition.
35-39: Optimize database queries by removing redundant user lookup.
This is the same suggestion from a previous review. You can reuse the result from the first query instead of querying again.Also applies to: 45-49
test/utilities/mockLogger.ts (1)
1-29: Mock logger implementation looks solid.
All relevant log methods are mocked for thorough testing, and the flexible child logger approach is a good design.test/graphql/types/Community/updater.test.ts (3)
18-28: LGTM! Well-structured test context interface.The TestContext interface is well-designed, properly extending GraphQLContext while adding type-safe mock implementations for the database client and logger.
29-44: LGTM! Type-safe mock PubSub implementation.The mock PubSub implementation is well-structured with proper typing and handles all necessary operations including publish, subscribe, and asyncIterator.
51-114: LGTM! Comprehensive test setup with proper mocking.The test setup is thorough with proper type safety and reset of mocks between tests. Good use of the mock logger utility.
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/graphql/types/Community/updater.test.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pull request workflow
test/graphql/types/Community/updater.test.ts
[error] 143-143: Formatter would have printed the following content: unexpected empty line before line 143.
[error] Some errors were emitted while running checks.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (1)
test/graphql/types/Community/updater.test.ts (1)
51-114: LGTM! Well-structured test setup.The test setup is comprehensive and follows best practices:
- Proper test isolation through beforeEach
- Complete mocking of dependencies
- Realistic test data
🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] Some errors were emitted while running checks.
Signed-off-by: NishantSinghhhhh <nishantsingh_230137@aitpune.edu.in>
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
|
@palisadoes , I will be adding more tests in this PR , Do not merge it now , there aree lot many test cases that are left to be covered |
Signed-off-by: NishantSinghhhhh <nishantsingh_230137@aitpune.edu.in>
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/graphql/types/Community/updater.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (1)
test/graphql/types/Community/updater.test.ts (1)
29-44: 🧹 Nitpick (assertive)Enhance type safety in PubSub mock implementation.
The current implementation could be improved with stronger type checking for the event topic and payload.
const createMockPubSub = () => ({ publish: vi.fn().mockImplementation( ( - event: { - topic: keyof PubSubEvents; - payload: PubSubEvents[keyof PubSubEvents]; + event: { + topic: K extends keyof PubSubEvents ? K : never; + payload: K extends keyof PubSubEvents ? PubSubEvents[K] : never; }, callback?: () => void, ) => { if (callback) callback(); return; }, ), subscribe: vi.fn(), asyncIterator: vi.fn(), });Likely invalid or redundant comment.
Signed-off-by: NishantSinghhhhh <nishantsingh_230137@aitpune.edu.in>
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/graphql/types/Community/updater.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (6)
test/graphql/types/Community/updater.test.ts (6)
1-317: LGTM! Comprehensive test coverage.The test suite provides thorough coverage of the Community updater resolver, including:
- Authentication checks
- Database error handling
- Success cases
- Edge cases
- Proper error messages and logging
13-16: Consider expanding PubSubEvents type.The
PubSubEventstype appears incomplete as it only includesCOMMUNITY_CREATEDandPOST_CREATEDevents. Consider adding other relevant events that might be used in the Community context.Let's check for other event types in the codebase:
61-79: 🧹 Nitpick (assertive)Simplify mock community object creation.
The mock community object contains many null fields that could be simplified using object spread.
mockCommunity = { id: "community-123", name: "Test Community", createdAt: new Date(), updatedAt: new Date(), updaterId: "456", - facebookURL: null, - githubURL: null, - inactivityTimeoutDuration: null, - instagramURL: null, - linkedinURL: null, - logoMimeType: null, - logoName: null, - redditURL: null, - slackURL: null, - websiteURL: null, - xURL: null, - youtubeURL: null, + ...Object.fromEntries( + [ + 'facebookURL', 'githubURL', 'inactivityTimeoutDuration', + 'instagramURL', 'linkedinURL', 'logoMimeType', 'logoName', + 'redditURL', 'slackURL', 'websiteURL', 'xURL', 'youtubeURL' + ].map(key => [key, null]) + ) };Likely invalid or redundant comment.
53-59: 🧹 Nitpick (assertive)Use type-safe role constant.
Consider using a type-safe constant for the role value to prevent typos and ensure consistency.
+const USER_ROLES = { + ADMIN: 'administrator', + USER: 'user' +} as const; mockUser = { id: "123", name: "John Doe", - role: "administrator", + role: USER_ROLES.ADMIN, createdAt: new Date(), updatedAt: null, };Likely invalid or redundant comment.
187-205: 🛠️ Refactor suggestionRemove duplicate test case.
This test case duplicates the functionality already tested in the "should make correct database queries with expected parameters" test at lines 143-164. Consider merging these tests to reduce redundancy.
- it("should correctly query the database for current user and updater user", async () => { - const updaterUser = { - id: "456", - name: "Jane Updater", - role: "user", - createdAt: new Date(), - updatedAt: null, - }; - - ctx.drizzleClient.query.usersTable.findFirst - .mockResolvedValueOnce(mockUser) // First call for current user - .mockResolvedValueOnce(updaterUser); // Second call for updater user - - await CommunityResolver.updater(mockCommunity, {}, ctx); - - expect(ctx.drizzleClient.query.usersTable.findFirst).toHaveBeenCalledTimes( - 2, - ); - });Likely invalid or redundant comment.
11-11: 🧹 Nitpick (assertive)Remove unnecessary type alias.
The
DeepPartialtype is just an alias for the built-inPartialtype. Consider removing this type alias and usingPartialdirectly.-type DeepPartial<T> = Partial<T>;Likely invalid or redundant comment.
Signed-off-by: NishantSinghhhhh <nishantsingh_230137@aitpune.edu.in>
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/utilities/mockLogger.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (1)
test/utilities/mockLogger.ts (1)
1-4: LGTM! Well-structured imports and function signature.The imports and function signature are appropriate for creating a mock logger.
Signed-off-by: NishantSinghhhhh <nishantsingh_230137@aitpune.edu.in>
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
|
@palisadoes , there is 1 type errror that was coming in this PR related to uuid , for that I have installed a new dependency
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
test/graphql/types/Community/updater.test.ts(1 hunks)test/utilities/mockLogger.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (6)
test/utilities/mockLogger.ts (5)
4-7: LGTM!The interface is well-defined with optional properties that allow flexible configuration of the mock logger.
9-40: 🧹 Nitpick (assertive)Consider improving type safety and preventing infinite recursion.
The implementation is solid but could be improved in two areas:
- The type assertion could be replaced with a type guard.
- The child method could potentially cause infinite recursion.
Consider this improved implementation:
export const createMockLogger = ( config?: MockLoggerConfig, + depth: number = 0, ): FastifyBaseLogger => { const level = config?.level ?? "info"; const enabledLevels = config?.enabledLevels ?? new Set(["error", "warn", "info", "debug", "trace", "fatal", "silent"]); const logger = { error: vi.fn(), warn: vi.fn(), info: vi.fn(), debug: vi.fn(), trace: vi.fn(), fatal: vi.fn(), silent: vi.fn(), - child: () => createMockLogger(config), + child: () => depth < 10 ? createMockLogger(config, depth + 1) : logger, level, isLevelEnabled: (l: string) => enabledLevels.has(l), bindings: vi.fn().mockReturnValue({}), flush: vi.fn(), isFatalEnabled: () => enabledLevels.has("fatal"), isErrorEnabled: () => enabledLevels.has("error"), isWarnEnabled: () => enabledLevels.has("warn"), isInfoEnabled: () => enabledLevels.has("info"), isDebugEnabled: () => enabledLevels.has("debug"), isTraceEnabled: () => enabledLevels.has("trace"), isSilentEnabled: () => enabledLevels.has("silent"), }; + // Type guard to ensure all required properties are implemented + const isComplete = (l: typeof logger): l is FastifyBaseLogger => { + const required: (keyof FastifyBaseLogger)[] = [ + 'error', 'warn', 'info', 'debug', 'trace', 'fatal', + 'child', 'level', 'isLevelEnabled' + ]; + return required.every(prop => prop in l); + }; + + if (!isComplete(logger)) { + throw new Error('Mock logger is missing required properties'); + } - return logger as unknown as FastifyBaseLogger; + return logger; };Likely invalid or redundant comment.
39-40: Type casting implementation looks good.The current implementation maintains type compatibility while keeping the code simple, as discussed in previous reviews.
1-40: Overall implementation is well-structured and comprehensive.The mock logger implementation provides all necessary functionality for testing purposes, with good defaults and configuration options. The code is clean and maintainable.
25-25: 🧹 Nitpick (assertive)Optimize child logger creation.
The current implementation creates a new logger instance for each child, which could lead to memory issues in tests with deep chains of child loggers. Consider reusing the parent logger instance.
-child: () => createMockLogger(config), +child: () => logger,Likely invalid or redundant comment.
test/graphql/types/Community/updater.test.ts (1)
60-78: 🧹 Nitpick (assertive)Simplify mock object creation.
The mock community object contains many null fields that could be simplified.
mockCommunity = { id: "community-123", name: "Test Community", createdAt: new Date(), updatedAt: new Date(), updaterId: "456", + ...Object.fromEntries( + [ + 'facebookURL', 'githubURL', 'inactivityTimeoutDuration', + 'instagramURL', 'linkedinURL', 'logoMimeType', 'logoName', + 'redditURL', 'slackURL', 'websiteURL', 'xURL', 'youtubeURL' + ].map(key => [key, null]) + ) - facebookURL: null, - githubURL: null, - inactivityTimeoutDuration: null, - instagramURL: null, - linkedinURL: null, - logoMimeType: null, - logoName: null, - redditURL: null, - slackURL: null, - websiteURL: null, - xURL: null, - youtubeURL: null, };Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/graphql/types/Community/Community.ts(1 hunks)test/graphql/types/Community/updater.test.ts(1 hunks)test/utilities/mockLogger.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/graphql/types/Community/Community.ts
[warning] 53-53: src/graphql/types/Community/Community.ts#L53
Added line #L53 was not covered by tests
[warning] 57-64: src/graphql/types/Community/Community.ts#L57-L64
Added lines #L57 - L64 were not covered by tests
🔇 Additional comments (5)
src/graphql/types/Community/Community.ts (4)
3-5: All good with the imports.
The added imports forTalawaGraphQLError,GraphQLContext, andUserare appropriate and enhance type safety and error handling.
11-17: Type definitions look solid.
DefiningCommunityResolverswith a clear return type ofPromise<User | null>helps ensure that the resolver’s contract is properly enforced.
35-40: Duplicate DB queries for the same user.
The code performs two queries for the sameupdaterId. Reuse the first query result to avoid an unnecessary second lookup.Also applies to: 51-55
53-53: Add test coverage for the second query’s null or undefined results.
Some lines remain untested (e.g., lines 53, 57–64). Although certain error paths are covered, consider adding a test where the second query specifically returns undefined or triggers theisNullcondition in the query.Also applies to: 57-64
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 53-53: src/graphql/types/Community/Community.ts#L53
Added line #L53 was not covered by teststest/graphql/types/Community/updater.test.ts (1)
53-53: Confirm coverage of the second query condition.
Ensure there is a test case whereparent.updaterIdleads to theisNullcheck in the second query, or confirm that this code path is unreachable.
|
@palisadoes , CodeRabbit is asking me to change the backend code , as it states that adding role based authorization , which will require changing a lot many backend files so I am not adding those changes |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Signed-off-by: NishantSinghhhhh <nishantsingh_230137@aitpune.edu.in>
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
Signed-off-by: NishantSinghhhhh <nishantsingh_230137@aitpune.edu.in>
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
|
@palisadoes, I have completed this PR and added as many test cases as possible. I also included the
Additionally, I have incorporated most of the genuine changes requested by coderabbitai. also , I have tried doing full review by coderabbit a lot many times , but it is not responding anything If there are any other important modifications you'd like me to make, please let me know, and I'll address them right away. Otherwise, I'll proceed with working on the next component. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/graphql/types/Community/Community.ts (1)
36-56: 🛠️ Refactor suggestionConsolidate redundant user lookups.
This code executes two separate queries (
existingUserandupdater) against the same table and effectively checks the same condition. You can unify the logic into a single query while retaining or adjusting the error checks, thereby improving performance and clarity.const updater = await context.drizzleClient.query.usersTable.findFirst({ where: (users, { eq }) => eq(users.id, updaterId), }); if (!updater) { context.log.warn(`No user found for updaterId: ${updaterId}`); throw new TalawaGraphQLError({ message: "Updater user not found", extensions: { code: "arguments_associated_resources_not_found", issues: [{ argumentPath: ["updaterId"] }], }, }); } return updater;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
package.json(2 hunks)src/graphql/types/Community/Community.ts(1 hunks)test/graphql/types/Community/updater.test.ts(1 hunks)test/utilities/mockLogger.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
test/graphql/types/Community/updater.test.ts (1)
Learnt from: NishantSinghhhhh
PR: PalisadoesFoundation/talawa-api#3110
File: test/graphql/types/Community/updater.test.ts:278-301
Timestamp: 2025-02-02T08:24:26.022Z
Learning: In Talawa API, for invalid input validation errors, use TalawaGraphQLError with code "invalid_arguments" and include the required "issues" array containing argumentPath and message. Don't use generic error codes like "bad_user_input".
🪛 GitHub Actions: Pull request workflow
src/graphql/types/Community/Community.ts
[error] 32-32: Formatter would have printed the following content indicating formatting issues.
test/graphql/types/Community/updater.test.ts
[error] 298-298: Formatter would have printed the following content indicating formatting issues.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (3)
src/graphql/types/Community/Community.ts (1)
69-75: Good error handling approach.Catching and logging database errors here is a solid practice. It ensures troubleshooting information is retained. The usage of
TalawaGraphQLErrorfor rethrowing maintains consistency with the rest of the codebase.test/utilities/mockLogger.ts (1)
16-51: 🧹 Nitpick (assertive)Prevent potential infinite recursion in child loggers.
child: () => createMockLogger(config)can recursively create nested loggers. While this may not necessarily cause issues in short test runs, adding a depth limit or returning the same logger instance helps avoid corner cases.child: (() => { let depth = 0; return () => { + if (depth >= 3) { + return logger as FastifyBaseLogger; + } depth++; return createMockLogger(config); }; })(),Likely invalid or redundant comment.
package.json (1)
42-42: @types/uuid Type Definitions EnhancementIntroducing
"@types/uuid": "^10.0.0"as a dev dependency improves type safety when working with UUIDs. However, note that modern versions of theuuidlibrary (such as v11) may already include built-in TypeScript definitions. Please verify that adding this dependency is necessary and does not introduce any conflicts or redundant type definitions.
Signed-off-by: NishantSinghhhhh <nishantsingh_230137@aitpune.edu.in>
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
|
@palisadoes, I have completed this PR and added as many test cases as possible. Can you tell me what more things I can add to make it more mergeable |
palisadoes
changed the title
palisadoes
merged commit 8e74cfe
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
Adding tests for updater.ts
Issue Number:
Fixes: #3085
Snapshots/Videos:
Screencast from 2025-02-01 04-25-01.webm
If relevant, did you update the documentation?
N/A
Summary
This PR improves the updater.ts file by:
Replacing any types with more specific TypeScript types to improve type safety.
Added tests to verify the updater.ts logic, including database error handling, user retrieval, and authentication/authorization checks.
Does this PR introduce a breaking change?
No
Checklist
CodeRabbit AI Review
I have reviewed and addressed all critical issues flagged by CodeRabbit AI
I have implemented or provided justification for each non-critical suggestion
I have documented my reasoning in the PR comments where CodeRabbit AI suggestions were not implemented
Test Coverage
I have written tests for all new changes/features
I have verified that test coverage meets or exceeds 95%
I have run the test suite locally and all tests pass
Other Information
Have you read the contributing guide?
Yes ✅
Summary by CodeRabbit
New Features
Tests