Removed ts-ignore statements from codebase and fixed the tests in faulty files #3253
Conversation
|
Warning Rate limit exceeded@yugal07 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 2 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 (1)
WalkthroughThis pull request introduces several modifications across multiple test files and a source file, focusing on updating import paths, enhancing type definitions, and refining mock implementations. The changes include the introduction of new types related to user roles and mock clients, which improve type safety and clarity in the tests. Additionally, adjustments to mock return values and test cases have been made to ensure consistency and accuracy in testing various scenarios, particularly concerning user authentication and authorization. Changes
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai full review |
|
I have added additional test cases and both the files are now at 100% coverage. @palisadoes PTAL |
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 (2)
test/graphql/types/Chat/updatedAt.test.ts(4 hunks)test/graphql/types/Event/updater.test.ts(9 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 (4)
test/graphql/types/Chat/updatedAt.test.ts (2)
2-33: Well-structured type definitions with proper type safety!The type definitions are well-organized, using zod for type inference and providing clear structures for mocks. This improves type safety and maintainability.
80-157: Excellent test coverage with comprehensive scenarios!The tests thoroughly cover:
- Authentication checks
- Authorization based on user roles
- Different permission levels (global admin, org admin, chat admin)
- Edge cases like user not found
test/graphql/types/Event/updater.test.ts (2)
37-44: Clean mock implementation with proper typing!The mock implementation is well-structured and properly typed using the MockDrizzleClient type.
70-251: Excellent test coverage with proper error handling and logging!The tests thoroughly cover:
- Authentication and authorization scenarios
- Data integrity checks
- Error cases with proper logging
- Different user role combinations
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 (1)
test/graphql/types/Event/updater.test.ts(9 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-02-23T15:57:18.082Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
test/graphql/types/Event/updater.test.ts (1)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Event/updater.test.ts:139-162
Timestamp: 2025-02-23T16:10:02.480Z
Learning: In test files, prefer keeping mock data separate for each test case instead of extracting common mock objects, as it provides better test isolation and makes future modifications easier without affecting other tests.
⏰ 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 (4)
test/graphql/types/Event/updater.test.ts (4)
2-7: LGTM! Import paths have been updated to use absolute paths.The changes improve maintainability by using absolute paths and adding proper type imports.
9-24: LGTM! Well-structured type definitions using zod.The type definitions are clear, well-documented, and properly use zod for type safety. The
MockDrizzleClienttype accurately represents the mock structure.
141-164: LGTM! Well-structured test case with isolated mock data.The test case follows best practices by keeping mock data separate and clearly documenting the test scenario.
72-252: LGTM! Comprehensive test coverage.The test suite:
- Covers authentication and authorization scenarios
- Handles edge cases (null updaterId, non-existent updater)
- Verifies behavior for different user roles
- Includes proper error handling and logging verification
| it("throws an unauthenticated error if the current user is not found", async () => { | ||
| // @ts-ignore | ||
| drizzleClientMock.query.usersTable.findFirst.mockReturnValue(undefined); | ||
| drizzleClientMock.query.usersTable.findFirst.mockResolvedValue(undefined); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider consistent mock implementation approach.
For consistency and better type safety, consider using mockImplementation instead of mockResolvedValue across all test cases:
- drizzleClientMock.query.usersTable.findFirst.mockResolvedValue(undefined);
+ drizzleClientMock.query.usersTable.findFirst.mockImplementation(() => Promise.resolve(undefined));Also applies to: 101-105, 122-126, 154-156, 173-173, 190-190, 213-215
| // Create a properly typed mock for drizzleClient | ||
| const drizzleClientMock = { | ||
| query: { | ||
| usersTable: { | ||
| findFirst: vi.fn(), | ||
| }, | ||
| }, | ||
| } as unknown as FastifyInstance["drizzleClient"]; | ||
| } as unknown as FastifyInstance["drizzleClient"] & MockDrizzleClient; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using mockImplementation for better type safety.
Based on our experience with complex types in Vitest, consider using mockImplementation instead of the direct mock function:
- findFirst: vi.fn(),
+ findFirst: vi.fn().mockImplementation(() => Promise.resolve(undefined)),📝 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.
| // Create a properly typed mock for drizzleClient | |
| const drizzleClientMock = { | |
| query: { | |
| usersTable: { | |
| findFirst: vi.fn(), | |
| }, | |
| }, | |
| } as unknown as FastifyInstance["drizzleClient"]; | |
| } as unknown as FastifyInstance["drizzleClient"] & MockDrizzleClient; | |
| // Create a properly typed mock for drizzleClient | |
| const drizzleClientMock = { | |
| query: { | |
| usersTable: { | |
| findFirst: vi.fn().mockImplementation(() => Promise.resolve(undefined)), | |
| }, | |
| }, | |
| } as unknown as FastifyInstance["drizzleClient"] & MockDrizzleClient; |
|
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3253 +/- ##
====================================================
+ Coverage 48.36% 48.80% +0.43%
====================================================
Files 458 458
Lines 34516 34494 -22
Branches 966 985 +19
====================================================
+ Hits 16695 16834 +139
+ Misses 17821 17660 -161 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
The src/graphql/types/Chat/updater.ts file is only showing 13% test code coverage. Please add more comprehensive and valid tests to get this as close to 100% as possible.
- Code coverage isn't reporting any significant change in the code health
- Here is the coverage for the file in question. (Scroll down)
|
The files i have worked on are src/graphql/types/Chat/updatedAt.ts and src/graphql/types/Event/updater.ts. Sure i will add tests for src/graphql/types/Chat/updater.ts, thanks for the review |
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 (2)
src/graphql/types/Chat/updater.ts(1 hunks)test/graphql/types/Chat/updater.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
test/graphql/types/Chat/updater.test.ts (1)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-02-23T15:57:18.082Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
⏰ 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 (11)
test/graphql/types/Chat/updater.test.ts (6)
1-16: Good job with comprehensive type definitions!The explicit type definitions using z.infer for UserRole, ChatMembershipRole, and OrganizationMembershipRole provide excellent type safety. This approach ensures that the test matches the actual type definitions used in the application.
18-33: Well-structured mock typesThe mock types are well-defined and closely match the shape of the actual database entities, which will make testing more reliable and maintainable.
98-109: Great test organization with comprehensive test casesThe test suite is well-structured with a beforeEach hook to reset mocks and clear tests for various authentication scenarios. This ensures reliable and isolated test execution.
140-154: Good edge case handlingTesting the null updaterId case is important as it represents a valid edge case in the application. The test properly verifies that null is returned when the updaterId is null.
195-215: Strong error handling verificationGood job testing the unexpected error case when an updaterId exists but the user is not found. This test ensures that the application correctly identifies and logs database inconsistency issues.
65-66: 🧹 Nitpick (assertive)Consider using mockImplementation consistently for type safety
Based on previous learnings in this codebase, it's preferable to use
mockImplementation(() => Promise.resolve(undefined))rather thanmockResolvedValue(undefined)for complex types to ensure better type safety.- findFirst: vi.fn().mockImplementation(() => Promise.resolve(undefined)), + findFirst: vi.fn().mockImplementation(() => Promise.resolve(undefined)),Likely an incorrect or invalid review comment.
src/graphql/types/Chat/updater.ts (5)
13-102: Excellent extraction of resolver logic into a separate functionThe
resolveUpdaterfunction cleanly encapsulates all the authentication, authorization, and data fetching logic. This separation makes the code more maintainable and testable.
18-24: Good authentication checkThe authentication check is properly implemented early in the function, ensuring that unauthenticated users cannot proceed further.
57-72: Well-structured permission checkingThe code clearly defines and checks for the three permission levels (global admin, organization admin, chat admin) that allow access to the updater information.
74-80: Efficient special case handlingThe function efficiently handles special cases like null updaterId and when the updater is the current user, avoiding unnecessary database queries.
108-109: Clean implementation with the extracted functionThe implementation is now much cleaner by using the extracted
resolveUpdaterfunction directly in the field definition.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/graphql/types/Chat/updater.test.ts (1)
145-145: 🧹 Nitpick (assertive)Apply a consistent mocking pattern.
The code uses bothmockImplementation(() => Promise.resolve(...))in some places andmockResolvedValueOnce(...)in others. Use one approach consistently to align with your codebase's conventions and the retrieved learnings on mocking complex return types.Also applies to: 185-185, 190-190, 192-192
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/graphql/types/Chat/updater.ts(1 hunks)test/graphql/types/Chat/updater.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
test/graphql/types/Chat/updater.test.ts (1)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-02-23T15:57:18.082Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Deployment to https://docs-api.talawa.io
🔇 Additional comments (20)
test/graphql/types/Chat/updater.test.ts (12)
1-9: Good import organization and setup.
These imports are coherent with Vitest, Zod, and the application’s modules. The separation of type imports from the local modules is clear and helps readability.
11-26: Type definitions appear solid.
Defining these type aliases allows strong typing for user roles and membership roles, promoting clarity throughout the tests.
27-33: Mocked drizzle client structure is well-defined.
Ensuring it closely matches the actual shape of the drizzle client fosters consistency between test and production environments.
35-47: Mock data follows best practices.
Storing reusable data objects (likemockCurrentUserandmockUpdaterUser) at the top keeps the tests concise.
49-61: Parent object initialization is adequately detailed.
Providing explicit sample fields ensures the resolver can be tested thoroughly for multiple properties.
62-96: Context mocks are straightforward.
The authenticated and unauthenticated contexts are clearly distinguished, improving test coverage for both valid and invalid client states.
98-109: Effective unauthenticated checks.
Both the “unauthenticated user” and “unfound user” paths are well-tested, ensuring the resolver handles these invalid states gracefully.Also applies to: 111-123
125-142: Unauthorized scenario is properly tested.
Verifying that regular users without admin privileges trigger an unauthorized action error covers critical permission checks.
144-159: Null updater scenario is tested carefully.
Confirming that a nullupdaterIdreturns null ensures robust handling of non-assigned updater references.
160-181: Current user as updater path is correctly handled.
This test confirms the correct return of the authenticated user when their ID matches theupdaterId.
183-197: Consider consolidating similar test cases.
Testing “returns updater user when found” alongside subsequent tests for global/organization/chat admins produce overlapping flows examining user resolution. Combine or differentiate them more distinctly to reduce repetition.
199-225: Properly tests unexpected error scenario.
When the updater is referenced in the chat but not found in the database, the error handling is validated thoroughly, covering a potential data corruption case.src/graphql/types/Chat/updater.ts (8)
13-24: Authentication check is clear.
Quickly throwing an unauthenticated error if the current client is not authenticated helps maintain security boundaries.
28-47: Fetching and joining membership data.
Using thewithproperty indrizzleClient.queryis an effective approach to gather chat and organization memberships in a single query, improving clarity of role checks.
49-55: Robust error for missing current user.
Throwing “unauthenticated” if the user is not found in the database closes loopholes for unauthorized access.
57-65: Variable extraction for membership roles is well-structured.
SeparatingisGlobalAdmin,isOrgAdmin, andisChatAdminfosters readability, especially when applying multiple role checks.
66-72: Unauthorized users are handled correctly.
This ensures only users with admin privileges can access the updater, in accordance with the domain logic.
74-80: Return null or current user for special cases.
Skipping queries ifupdaterIdis null or the same ascurrentUserIdis a solid optimization, preventing unnecessary database lookups.
82-99: Detailed business logic error handling.
Logging the error message and throwing a distinct “unexpected” exception helps administrators diagnose data inconsistencies promptly.
104-112: Resolver mapping is straightforward.
Assigningresolve: resolveUpdaterin the Chat type is clean and keeps the schema definition simple.
|
@palisadoes, I have implemented all of the suggestions from coderabbit and added test cases for src/graphql/types/Chat/updater.ts as per your request. hence this PR now also solves issue #3058 . Let me know if theres need for more changes |
palisadoes
merged commit 38a7f65
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
Bugfix
Issue Number:
Fixes #3228
Snapshots/Videos:
N/A
If relevant, did you update the documentation?
Auto-generated docs
Summary
Some files had ts-ignore statements which is considered to be bad practices, This PR fixes the faulty test files with proper tests and removes ts-ignore statements
Does this PR introduce a breaking change?
No
Checklist
CodeRabbit AI Review
Test Coverage
Have you read the contributing guide?
Yes
Summary by CodeRabbit
Chat.updaterresolver, validating behavior under various conditions.