Added Required Queries #3360
Added Required Queries #3360
Conversation
WalkthroughThe changes introduce new filtering functionality in the GraphQL schema. A new field Changes
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 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
|
JXP25
changed the title
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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/graphql/types/User/index.ts (1)
24-24: Consider maintaining alphabetical ordering for importsThe new import for "createdOrganizations" has been added at the end of the import list, but it appears that imports in this file are organized alphabetically. For consistency with the existing code style, consider placing this import in alphabetical order (between "creator" and "educationGrade").
import "./countryCode"; import "./creator"; +import "./createdOrganizations"; import "./educationGrade"; import "./emailAddress";src/graphql/types/User/createdOrganizations.ts (2)
6-16: Consider adding pagination supportFor scalability, it would be helpful to implement pagination for the
createdOrganizationsfield. As users might create many organizations over time, returning all of them in a single query could potentially lead to performance issues.You could add pagination parameters like this:
createdOrganizations: t.field({ type: [Organization], description: "Organizations created by the user", + args: { + limit: t.arg.int({ defaultValue: 10 }), + offset: t.arg.int({ defaultValue: 0 }), + }, - resolve: async (parent, _args, ctx) => { + resolve: async (parent, args, ctx) => { // Query the organizations table where the creator_id equals the user's id. return await ctx.drizzleClient.query.organizationsTable.findMany({ where: (fields, operators) => operators.eq(fields.creatorId, parent.id), + limit: args.limit, + offset: args.offset, }); }, }),
9-15: Consider adding error handlingThe resolver doesn't include any error handling. In a production environment, it's good practice to catch and handle potential database errors to prevent them from cascading up to the GraphQL response.
resolve: async (parent, _args, ctx) => { // Query the organizations table where the creator_id equals the user's id. - return await ctx.drizzleClient.query.organizationsTable.findMany({ - where: (fields, operators) => - operators.eq(fields.creatorId, parent.id), - }); + try { + return await ctx.drizzleClient.query.organizationsTable.findMany({ + where: (fields, operators) => + operators.eq(fields.creatorId, parent.id), + }); + } catch (error) { + console.error("Error fetching created organizations:", error); + throw new Error("Failed to fetch created organizations"); + } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/graphql/types/User/createdOrganizations.ts(1 hunks)src/graphql/types/User/index.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)
src/graphql/types/User/createdOrganizations.ts (1)
1-18: Implementation looks good!The new field for retrieving organizations created by a user is well-implemented. You've properly:
- Added appropriate imports
- Implemented the User type extension
- Defined the field with correct return type and description
- Implemented a resolver that queries the organizations table with the user's ID
The code structure follows the project's patterns and the resolver logic is clear and straightforward.
|
@palisadoes PR is ready for review (coderabbit parsing error solved in #3356, it is really simple fix but don't want to duplicate it in every PR) |
|
@pranshugupta54, @GlenDsza, Please review this PR. |
|
@JaiPannu-IITI Please fix the coderabbit issue, so that its comments are documented as addressed in this PR. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3360 +/- ##
====================================================
+ Coverage 52.25% 52.61% +0.35%
====================================================
Files 466 467 +1
Lines 35059 35092 +33
Branches 1182 1208 +26
====================================================
+ Hits 18321 18462 +141
+ Misses 16736 16628 -108
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@Cioppolo14 Please add ignore sensitive file label for coderabbit fix. |
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 (3)
src/graphql/types/Query/organizations.ts(2 hunks)src/graphql/types/User/createdOrganizations.ts(1 hunks)src/graphql/types/User/organizationsWhereMember.ts(4 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 (5)
src/graphql/types/Query/organizations.ts (1)
2-24: Filter implementation looks goodThe addition of filtering capability to the organizations query is well-implemented. The code maintains the existing error handling and follows consistent patterns with other parts of the codebase.
I particularly like:
- The clear update to the description to mention optional filtering
- The use of
sql\TRUE`` as a default condition when no filter is provided- The maintained limit of 20 organizations, which prevents excessive load
- The consistent error handling approach
src/graphql/types/User/organizationsWhereMember.ts (4)
1-7: Initial setup for filter looks goodThe import additions and schema extension for adding the filter argument are well-implemented. The transformation of arguments correctly includes the filter.
Also applies to: 19-21, 43-44
61-67: Connection definition with filter parameter is well-structuredThe updated connection definition with the filter argument is clear and follows GraphQL best practices by making the filter optional.
116-152: Query construction with filtering looks solidThe implementation correctly:
- Extracts the filter from parsed arguments
- Constructs a direct join between membership and organization tables
- Applies the filter condition using case-insensitive matching
- Maintains the existing cursor-based pagination logic
The comment on line 129 is helpful for understanding the join approach.
197-218: Result transformation correctly handles the joined dataThe transformation logic properly handles the data structure resulting from the join operation. The cursor creation and node extraction are appropriately implemented.
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)
src/graphql/types/User/createdOrganizations.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 (2)
src/graphql/types/User/createdOrganizations.ts (2)
1-3: Imports look good!All imported functions and types are utilized appropriately. No concerns here.
5-12: Field definition is well-structured.Defining a list of
Organizationwith the optional string filter is a clean approach. The description is clear, and the argument configuration is straightforward.
|
@Cioppolo14 Also sensitive files are changed to allow code rabbit review this PR, it is small bug in yaml file I have already resolved in my previous PR's too, they are yet to be merged. |
@JaiPannu-IITI Please add tests to covered the modified lines as well. Covering tests in separate PRs wont be ideal. It would be better to add new features along with testcases |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/graphql/types/User/createdOrganizations.test.ts (2)
1-8: 🧹 Nitpick (assertive)Sort import statements alphabetically.
The import statements should be sorted alphabetically for better organization and maintainability.
import { beforeEach, describe, expect, test, vi } from "vitest"; import type { ExplicitGraphQLContext, ImplicitMercuriusContext, } from "~/src/graphql/context"; import type { User } from "~/src/graphql/types/User/User"; -import { resolveCreatedOrganizations } from "~/src/graphql/types/User/createdOrganizations"; -import { TalawaGraphQLError } from "~/src/utilities/TalawaGraphQLError"; +import { resolveCreatedOrganizations } from "~/src/graphql/types/User/createdOrganizations"; +import { TalawaGraphQLError } from "~/src/utilities/TalawaGraphQLError";
78-95: 🧹 Nitpick (assertive)Enhance filter test to verify filter behavior.
The test confirms that the
wherefunction exists but doesn't verify its actual filtering behavior. Consider adding assertions to test the filter function with sample data.expect(typeof findManyArgs.where).toBe("function"); + + // Test the where function with sample data + const whereFunction = findManyArgs.where; + const matchingOrg = { name: "Filtered Test" }; + const nonMatchingOrg = { name: "Other Organization" }; + expect(whereFunction(matchingOrg)).toBe(true); + expect(whereFunction(nonMatchingOrg)).toBe(false);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
test/graphql/types/Query/organizations.test.ts(1 hunks)test/graphql/types/User/createdOrganizations.test.ts(1 hunks)
🔇 Additional comments (5)
test/graphql/types/User/createdOrganizations.test.ts (2)
29-32: Good type definition for the mock user.Properly typing the mock user as
Userensures type safety throughout the tests.
34-116: Well-structured test suite with comprehensive scenarios.The test suite effectively covers authentication, happy path, filtering, and error handling scenarios. This ensures robust testing of the new resolver functionality.
test/graphql/types/Query/organizations.test.ts (3)
28-109: Well-structured tests covering important scenarios.The test suite effectively validates authentication, successful queries, filtering, and error handling. This comprehensive approach ensures the reliability of the new resolver functionality.
1-8: Clean imports and proper organization.The imports are properly organized and include all necessary dependencies for the tests.
89-92: Good approach for testing the where clause.Using mock.calls to extract and verify the dynamically created filter function is a good technique. The variable naming is clear and the approach is appropriate for testing this type of functionality.
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 (1)
test/graphql/types/User/organizationsWhereMember.test.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pull request workflow
test/graphql/types/User/organizationsWhereMember.test.ts
[error] 6-6: 'Organization' is declared but its value is never read.
[error] 9-9: 'TalawaGraphQLError' is declared but its value is never read.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
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 (1)
test/graphql/types/Query/organizations.test.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pull request workflow
test/graphql/types/Query/organizations.test.ts
[error] 146-164: 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 (1)
test/graphql/types/Query/organizations.test.ts (1)
69-94: Improve filter test to verify actual filtering behavior.The test confirms that the
wherefunction exists, but doesn't verify how it actually filters organizations. Adding assertions for the filter function's behavior would make this test more robust.expect(typeof whereArg).toBe("function"); + + // Test the where function with sample data + const matchingOrg = { name: "Filtered Test" }; + const nonMatchingOrg = { name: "Other Organization" }; + expect(whereArg(matchingOrg)).toBe(true); + expect(whereArg(nonMatchingOrg)).toBe(false);
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
test/graphql/types/Query/organizations.test.ts (4)
69-94: 🧹 Nitpick (assertive)Enhance filter test with validation of filtering behavior.
The test verifies that the filter is applied by checking that the
whereargument exists, but it doesn't validate how the filter actually works in practice. Consider adding assertions to verify the filter function's actual behavior with sample data.expect(typeof whereArg).toBe("function"); + + // Test the where function with sample data + const matchingOrg = { name: "Filtered Test" }; + const nonMatchingOrg = { name: "Other Organization" }; + expect(whereArg(matchingOrg)).toBe(true); + expect(whereArg(nonMatchingOrg)).toBe(false);
111-130: 🧹 Nitpick (assertive)Clarify test description to better reflect filter condition testing.
The test description could be more specific about what aspect of the filter condition is being tested.
-test("where clause returns correct condition when filter is provided", async () => { +test("where clause generates ilike condition when filter is provided", async () => {
132-145:⚠️ Potential issueFix inconsistency between test description and implementation.
This test description indicates it's testing when a filter is not provided, but the code is actually using a non-null filter value "Filtered". This creates confusion about what's being tested.
-test("where clause returns sql`TRUE` when filter is not provided", async () => { - mockDrizzleClient.query.organizationsTable.findMany.mockResolvedValue([]); - const filterValue = "Filtered"; - await resolveOrganizations(null, { filter: filterValue }, baseMockCtx); +test("where clause returns sql`TRUE` when filter is null", async () => { + mockDrizzleClient.query.organizationsTable.findMany.mockResolvedValue([]); + await resolveOrganizations(null, { filter: null }, baseMockCtx);
147-161: 🛠️ Refactor suggestionRemove duplicate test for clarity.
This test is functionally a duplicate of the previous test, causing confusion. Since both tests verify the behavior when no filter is effectively applied, consider removing one and making the remaining test clear about its purpose.
-test("where clause returns sql`TRUE` when filter is not provided", async () => { +test("where clause returns sql`TRUE` when filter is undefined", async () => {If you keep both tests, one should test with
nulland one withundefinedto cover both cases, and the test descriptions should clearly differentiate between them.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
test/graphql/types/Query/organizations.test.ts(1 hunks)test/graphql/types/User/createdOrganizations.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (7)
test/graphql/types/User/createdOrganizations.test.ts (7)
1-9: Sort import statements alphabeticallyImport statements are not sorted alphabetically, which may cause issues with your linting or CI pipeline. This was flagged in a previous review comment as well.
import { and, ilike, sql } from "drizzle-orm"; import { beforeEach, describe, expect, test, vi } from "vitest"; import type { ExplicitGraphQLContext, ImplicitMercuriusContext, } from "~/src/graphql/context"; import type { User } from "~/src/graphql/types/User/User"; -import { resolveCreatedOrganizations } from "~/src/graphql/types/User/createdOrganizations"; -import { TalawaGraphQLError } from "~/src/utilities/TalawaGraphQLError"; +import { resolveCreatedOrganizations } from "~/src/graphql/types/User/createdOrganizations"; +import { TalawaGraphQLError } from "~/src/utilities/TalawaGraphQLError";
29-32: LGTM: User mock setup looks goodThe mock user parent correctly matches the context user data, which is important for testing authentication flows.
39-52: Good authentication test coverageThis test properly verifies that the resolver rejects unauthenticated requests, which is an important security aspect to test.
78-95: Enhance filter test with specific assertionsThe test confirms that
whereis a function, but doesn't verify how the filter is actually applied. This was suggested in a previous review comment.expect(findManyArgs.limit).toBe(20); expect(findManyArgs.offset).toBe(0); expect(typeof findManyArgs.where).toBe("function"); + +// Test the where function with a mock table row +const mockRow = { name: "Test Org" }; +const whereFunction = findManyArgs.where; +const dummyOperators = { + eq: (field, value) => true, + ilike: (field, pattern) => field.includes(filterValue) +}; +expect(whereFunction({ name: sql`Test Org`, creatorId: sql`user123` }, dummyOperators)).toBeTruthy();
97-115: Good error handling testThe test properly verifies that errors from the database are logged and a user-friendly error message is thrown.
117-154: Well-structured test for filter conditionThis test thoroughly verifies the SQL condition construction when a filter is applied. The use of dummy fields and operators is a good technique for testing the where clause.
156-185: Thorough testing of unfiltered query conditionThe test properly verifies that when no filter is provided, the where clause only includes the creator ID condition and a SQL TRUE condition, which is the expected behavior.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
test/graphql/types/User/organizationsWhereMember.test.ts (3)
112-138: 🛠️ Refactor suggestionImplement test for successful retrieval of organizations.
The commented-out test for successfully retrieving organizations is important for ensuring the main functionality works correctly. With the enhanced mock implementation proposed earlier, this test can be uncommented and implemented.
Per the previous review comments, it's important to test the success path where organizations are properly returned. This test should be uncommented and implemented to ensure full coverage.
-// test("returns organizations where the user is a member", async () => { -// mockDrizzleClient.query.usersTable.findFirst.mockResolvedValue({ -// id: "user123", -// role: "member", -// }); - -// const organizationsMockData = [ -// { -// membershipCreatedAt: new Date(), -// membershipOrganizationId: "org1", -// organization: { id: "org1", name: "Test Organization" }, -// }, -// ]; - -// mockDrizzleClient.select.mockResolvedValue(organizationsMockData); - -// const result = await resolveOrganizationsWhereMember(mockUserParent, { filter: undefined }, baseMockCtx); - -// expect(result).toEqual( -// transformToDefaultGraphQLConnection({ -// createCursor: expect.any(Function), -// createNode: expect.any(Function), -// parsedArgs: expect.any(Object), -// rawNodes: organizationsMockData, -// }), -// ); -// }); +test("returns organizations where the user is a member", async () => { + mockDrizzleClient.query.usersTable.findFirst.mockResolvedValue({ + id: "user123", + role: "member", + }); + + const organizationsMockData = [ + { + membershipCreatedAt: new Date(), + membershipOrganizationId: "org1", + organization: { id: "org1", name: "Test Organization" }, + }, + ]; + + // With the enhanced mock implementation, this should work + mockDrizzleClient.select.mockReturnValue({ + from: vi.fn().mockReturnValue({ + innerJoin: vi.fn().mockReturnValue({ + where: vi.fn().mockReturnValue({ + orderBy: vi.fn().mockReturnValue({ + limit: vi.fn().mockResolvedValue(organizationsMockData) + }) + }) + }) + }) + }); + + const result = await resolveOrganizationsWhereMember(mockUserParent, { filter: undefined }, baseMockCtx); + + expect(result).toEqual( + transformToDefaultGraphQLConnection({ + createCursor: expect.any(Function), + createNode: expect.any(Function), + parsedArgs: expect.any(Object), + rawNodes: organizationsMockData, + }), + ); +});
140-157: 🛠️ Refactor suggestionImplement test for filter functionality.
The commented-out test for filter functionality is crucial as it tests a core feature mentioned in the PR objectives. This test should be uncommented and implemented.
Per the previous review comments, testing the filter functionality is critical since it's a core feature of this PR. This ensures that the filter functionality works correctly.
-// test("applies filter when provided", async () => { -// mockDrizzleClient.query.usersTable.findFirst.mockResolvedValue({ -// id: "user123", -// role: "member", -// }); - -// mockDrizzleClient.select.mockResolvedValue([ -// { -// membershipCreatedAt: new Date(), -// membershipOrganizationId: "org1", -// organization: { id: "org1", name: "Filtered Org" }, -// }, -// ]); - -// await resolveOrganizationsWhereMember(mockUserParent, { filter: "Filtered" }, baseMockCtx); - -// expect(mockDrizzleClient.where).toHaveBeenCalledWith(expect.any(Function)); -// }); +test("applies filter when provided", async () => { + mockDrizzleClient.query.usersTable.findFirst.mockResolvedValue({ + id: "user123", + role: "member", + }); + + const filteredData = [ + { + membershipCreatedAt: new Date(), + membershipOrganizationId: "org1", + organization: { id: "org1", name: "Filtered Org" }, + }, + ]; + + // Set up chainable mocks that can be spied on + const whereMock = vi.fn().mockReturnValue({ + orderBy: vi.fn().mockReturnValue({ + limit: vi.fn().mockResolvedValue(filteredData) + }) + }); + + mockDrizzleClient.select.mockReturnValue({ + from: vi.fn().mockReturnValue({ + innerJoin: vi.fn().mockReturnValue({ + where: whereMock + }) + }) + }); + + const result = await resolveOrganizationsWhereMember( + mockUserParent, + { filter: "Filtered" }, + baseMockCtx + ); + + // Verify filter was applied + expect(whereMock).toHaveBeenCalled(); + expect(result.edges[0].node.name).toBe("Filtered Org"); +});
77-95: 🧹 Nitpick (assertive)Verify mock implementation in unauthorized user test.
Similar to the previous test, this test doesn't explicitly verify that the
findFirstmock is called with the correct parameters. Add an assertion to ensure the code path is functioning as expected.test("throws an unauthorized error if a non-administrator queries another user's organizations", async () => { mockDrizzleClient.query.usersTable.findFirst.mockResolvedValue({ id: "user123", role: "member", }); const anotherUserParent = { id: "anotherUser", role: "member", } as unknown as User; await expect( resolveOrganizationsWhereMember( anotherUserParent, { filter: undefined }, baseMockCtx, ), ).rejects.toThrow(TalawaGraphQLError); + + expect(mockDrizzleClient.query.usersTable.findFirst).toHaveBeenCalledWith({ + where: expect.objectContaining({ id: "user123" }) + }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/graphql/types/User/organizationsWhereMember.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/User/organizationsWhereMember.test.ts (1)
12-12: Consider removing the unused TalawaGraphQLError import.The
TalawaGraphQLErrorimport is used in the assertions but not directly instantiated. If you're only checking that an error is thrown of this type, it's fine to keep. However, if you modify your tests to check for specific error codes as suggested, you might not need this import at all.#!/bin/bash # Check if TalawaGraphQLError is used in assertions or instantiated in this file # Count occurrences of TalawaGraphQLError beyond the import statement grep -n "TalawaGraphQLError" test/graphql/types/User/organizationsWhereMember.test.ts | grep -v "import"
|
Not ready yet, will add more test cases |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@JaiPannu-IITI I'll help you write more tests for the @JaiPannu-IITI Let me examine the current test coverage and implementation details so I can provide comprehensive tests for the @JaiPannu-IITI Based on my analysis of the file structure and the implementation, here are additional test cases for import { describe, it, vi, expect, beforeEach } from "vitest";
import { resolveOrganizationsWhereMember } from "../../../../src/graphql/types/User/organizationsWhereMember";
import { TalawaGraphQLError } from "../../../../src/utils/errors";
import { organizationMembershipsTable, organizationsTable } from "../../../../src/db/schema";
import { inArray, ilike, sql } from "drizzle-orm";
import { encode } from "../../../../src/utils/base64";
// Mocking necessary dependencies
vi.mock("../../../../src/db/schema", () => ({
organizationMembershipsTable: {
userId: "userId",
organizationId: "organizationId",
},
organizationsTable: {
id: "id",
name: "name",
},
}));
// Mock base context for authenticated user
const mockContext = {
currentUser: {
isAuthenticated: true,
userId: "user-id-1",
currentMember: {
organizationId: "org-id-1",
},
},
currentMember: {
organizationId: "org-id-1",
},
dbClient: {
query: vi.fn().mockReturnValue({
findFirst: vi.fn(),
select: vi.fn().mockReturnThis(),
from: vi.fn().mockReturnThis(),
where: vi.fn().mockReturnThis(),
innerJoin: vi.fn().mockReturnThis(),
as: vi.fn().mockReturnThis(),
offset: vi.fn().mockReturnThis(),
limit: vi.fn().mockReturnThis(),
execute: vi.fn(),
}),
},
logger: {
debug: vi.fn(),
error: vi.fn(),
info: vi.fn(),
warn: vi.fn(),
},
};
// Mock parent user
const mockUser = {
id: "user-id-1",
};
describe("resolveOrganizationsWhereMember", () => {
beforeEach(() => {
vi.clearAllMocks();
});
it("throws an error when user is not authenticated", async () => {
const context = {
...mockContext,
currentUser: {
...mockContext.currentUser,
isAuthenticated: false,
},
};
await expect(
resolveOrganizationsWhereMember(mockUser, {}, context as any)
).rejects.toThrow(TalawaGraphQLError);
});
it("throws an error when current user doesn't exist", async () => {
const context = {
...mockContext,
currentUser: {
...mockContext.currentUser,
userId: undefined,
},
};
await expect(
resolveOrganizationsWhereMember(mockUser, {}, context as any)
).rejects.toThrow(TalawaGraphQLError);
});
it("throws an error when a non-admin user tries to fetch organizations for another user", async () => {
const differentUser = {
id: "different-user-id",
};
await expect(
resolveOrganizationsWhereMember(differentUser, {}, mockContext as any)
).rejects.toThrow(TalawaGraphQLError);
});
it("successfully returns organizations where user is a member", async () => {
const mockOrgs = [
{
organizations: {
id: "org-id-1",
name: "Organization 1",
description: "Description 1",
},
},
{
organizations: {
id: "org-id-2",
name: "Organization 2",
description: "Description 2",
},
},
];
mockContext.dbClient.query().execute.mockResolvedValue(mockOrgs);
const result = await resolveOrganizationsWhereMember(
mockUser,
{},
mockContext as any
);
expect(result.edges.length).toBe(2);
expect(result.edges[0].node).toEqual(mockOrgs[0].organizations);
expect(result.edges[1].node).toEqual(mockOrgs[1].organizations);
expect(result.pageInfo.hasNextPage).toBe(false);
expect(mockContext.dbClient.query().limit).toHaveBeenCalledWith(11);
});
it("applies filter correctly when provided", async () => {
const mockOrgs = [
{
organizations: {
id: "org-id-1",
name: "Test Organization",
description: "Description for test org",
},
},
];
mockContext.dbClient.query().execute.mockResolvedValue(mockOrgs);
const result = await resolveOrganizationsWhereMember(
mockUser,
{ filter: "Test" },
mockContext as any
);
expect(result.edges.length).toBe(1);
expect(result.edges[0].node).toEqual(mockOrgs[0].organizations);
// Verify the where function was called
const whereFunction = mockContext.dbClient.query().where.mock.calls[0][0];
// Test that the where function generates an ilike condition when filter is provided
const whereClauseWithFilter = whereFunction();
expect(ilike).toHaveBeenCalledWith(organizationsTable.name, "%Test%");
});
it("handles cursor-based pagination correctly", async () => {
const mockOrgs = [
{
organizations: {
id: "org-id-3",
name: "Organization 3",
description: "Description 3",
},
},
{
organizations: {
id: "org-id-4",
name: "Organization 4",
description: "Description 4",
},
},
];
mockContext.dbClient.query().execute.mockResolvedValue(mockOrgs);
// Create a valid cursor for testing
const cursor = encode(JSON.stringify({ id: "org-id-2" }));
const result = await resolveOrganizationsWhereMember(
mockUser,
{ after: cursor },
mockContext as any
);
expect(result.edges.length).toBe(2);
expect(result.pageInfo.hasNextPage).toBe(false);
expect(result.pageInfo.startCursor).toBe(encode(JSON.stringify({ id: "org-id-3" })));
expect(result.pageInfo.endCursor).toBe(encode(JSON.stringify({ id: "org-id-4" })));
});
it("correctly indicates hasNextPage when more results are available", async () => {
// Create 11 mock orgs (limit is 10 + 1 to check for next page)
const mockOrgs = Array.from({ length: 11 }, (_, i) => ({
organizations: {
id: `org-id-${i + 1}`,
name: `Organization ${i + 1}`,
description: `Description ${i + 1}`,
},
}));
mockContext.dbClient.query().execute.mockResolvedValue(mockOrgs);
const result = await resolveOrganizationsWhereMember(
mockUser,
{},
mockContext as any
);
// Should return only 10 edges even though we had 11 results
expect(result.edges.length).toBe(10);
expect(result.pageInfo.hasNextPage).toBe(true);
});
it("throws a friendly error message when cursor is invalid", async () => {
const invalidCursor = "invalid-cursor";
await expect(
resolveOrganizationsWhereMember(
mockUser,
{ after: invalidCursor },
mockContext as any
)
).rejects.toThrow(TalawaGraphQLError);
expect(mockContext.logger.error).toHaveBeenCalled();
});
it("throws a friendly error message when database query fails", async () => {
mockContext.dbClient.query().execute.mockRejectedValue(new Error("Database error"));
await expect(
resolveOrganizationsWhereMember(
mockUser,
{},
mockContext as any
)
).rejects.toThrow("Failed to retrieve organizations where user is a member");
expect(mockContext.logger.error).toHaveBeenCalled();
});
it("returns empty edges when no organizations are found", async () => {
mockContext.dbClient.query().execute.mockResolvedValue([]);
const result = await resolveOrganizationsWhereMember(
mockUser,
{},
mockContext as any
);
expect(result.edges).toEqual([]);
expect(result.pageInfo.hasNextPage).toBe(false);
expect(result.pageInfo.startCursor).toBeNull();
expect(result.pageInfo.endCursor).toBeNull();
});
});These tests cover:
Please review the tests and adjust them if necessary to match the exact implementation details of your resolver function. Let me know if you need any clarification or have any questions about these test cases. 🏁 Script executed: Length of output: 13401 🏁 Script executed: Length of output: 13401 @JaiPannu-IITI Based on examining both files, I can see you need tests for the success cases and various functional aspects of the import { and, ilike, sql, eq, or, gt, lt } from "drizzle-orm";
import { organizationMembershipsTable } from "~/src/drizzle/tables/organizationMemberships";
import { organizationsTable } from "~/src/drizzle/tables/organizations";
import { transformToDefaultGraphQLConnection } from "~/src/utilities/defaultGraphQLConnection";
// Uncomment these imports at the top of the file
test("returns organizations where the user is a member", async () => {
mockDrizzleClient.query.usersTable.findFirst.mockResolvedValue({
id: "user123",
role: "member",
});
const organizationsMockData = [
{
membershipCreatedAt: new Date("2023-01-01"),
membershipOrganizationId: "org1",
organization: { id: "org1", name: "Test Organization 1" },
},
{
membershipCreatedAt: new Date("2023-01-02"),
membershipOrganizationId: "org2",
organization: { id: "org2", name: "Test Organization 2" },
},
];
mockDrizzleClient.select.mockImplementation(() => ({
from: () => ({
innerJoin: () => ({
where: () => ({
limit: () => ({
orderBy: () => Promise.resolve(organizationsMockData),
}),
}),
}),
}),
}));
const result = await resolveOrganizationsWhereMember(
mockUserParent,
{ filter: undefined },
baseMockCtx,
);
expect(result.edges.length).toBe(2);
expect(result.edges[0].node).toEqual(organizationsMockData[0].organization);
expect(result.edges[1].node).toEqual(organizationsMockData[1].organization);
expect(result.pageInfo.hasNextPage).toBe(false);
expect(result.pageInfo.hasPreviousPage).toBe(false);
});
test("applies filter when provided", async () => {
mockDrizzleClient.query.usersTable.findFirst.mockResolvedValue({
id: "user123",
role: "member",
});
const organizationsMockData = [
{
membershipCreatedAt: new Date("2023-01-01"),
membershipOrganizationId: "org1",
organization: { id: "org1", name: "Filtered Org" },
},
];
mockDrizzleClient.select.mockImplementation(() => ({
from: () => ({
innerJoin: () => ({
where: (whereFn) => {
// Call the where function to trigger filter application
const whereCondition = whereFn();
// Test that ilike is applied with the filter
expect(whereCondition).toBeDefined();
return {
limit: () => ({
orderBy: () => Promise.resolve(organizationsMockData),
}),
};
},
}),
}),
}));
const result = await resolveOrganizationsWhereMember(
mockUserParent,
{ filter: "Filtered" },
baseMockCtx,
);
expect(result.edges.length).toBe(1);
expect(result.edges[0].node.name).toBe("Filtered Org");
});
test("applies cursor-based pagination correctly", async () => {
mockDrizzleClient.query.usersTable.findFirst.mockResolvedValue({
id: "user123",
role: "member",
});
const organizationsMockData = [
{
membershipCreatedAt: new Date("2024-01-02T12:00:00Z"),
membershipOrganizationId: "org2",
organization: { id: "org2", name: "Org B" },
},
{
membershipCreatedAt: new Date("2024-01-03T12:00:00Z"),
membershipOrganizationId: "org3",
organization: { id: "org3", name: "Org C" },
},
];
mockDrizzleClient.select.mockImplementation(() => ({
from: () => ({
innerJoin: () => ({
where: () => ({
limit: () => ({
orderBy: () => Promise.resolve(organizationsMockData),
}),
}),
}),
}),
}));
const cursor = Buffer.from(
JSON.stringify({ createdAt: "2024-01-01T12:00:00Z", organizationId: "org1" }),
).toString("base64url");
const result = await resolveOrganizationsWhereMember(
mockUserParent,
{ after: cursor },
baseMockCtx,
);
expect(result.edges.length).toBe(2);
expect(result.edges[0].node.id).toBe("org2");
expect(result.edges[1].node.id).toBe("org3");
expect(result.pageInfo.startCursor).toBeDefined();
expect(result.pageInfo.endCursor).toBeDefined();
});
test("handles empty result set correctly", async () => {
mockDrizzleClient.query.usersTable.findFirst.mockResolvedValue({
id: "user123",
role: "member",
});
mockDrizzleClient.select.mockImplementation(() => ({
from: () => ({
innerJoin: () => ({
where: () => ({
limit: () => ({
orderBy: () => Promise.resolve([]),
}),
}),
}),
}),
}));
const result = await resolveOrganizationsWhereMember(
mockUserParent,
{ filter: undefined },
baseMockCtx,
);
expect(result.edges).toEqual([]);
expect(result.pageInfo.hasNextPage).toBe(false);
expect(result.pageInfo.hasPreviousPage).toBe(false);
expect(result.pageInfo.startCursor).toBeNull();
expect(result.pageInfo.endCursor).toBeNull();
});
test("correctly indicates hasNextPage when more results are available", async () => {
mockDrizzleClient.query.usersTable.findFirst.mockResolvedValue({
id: "user123",
role: "member",
});
// Create array with 11 items (10 + 1 for checking if there's a next page)
const organizationsMockData = Array.from({ length: 11 }, (_, i) => ({
membershipCreatedAt: new Date(`2023-01-${i + 1}`),
membershipOrganizationId: `org${i + 1}`,
organization: { id: `org${i + 1}`, name: `Organization ${i + 1}` },
}));
mockDrizzleClient.select.mockImplementation(() => ({
from: () => ({
innerJoin: () => ({
where: () => ({
limit: () => ({
orderBy: () => Promise.resolve(organizationsMockData),
}),
}),
}),
}),
}));
const result = await resolveOrganizationsWhereMember(
mockUserParent,
{ filter: undefined },
baseMockCtx,
);
expect(result.edges.length).toBe(10); // Should return 10 even though 11 were retrieved
expect(result.pageInfo.hasNextPage).toBe(true);
});
test("throws and logs error when database query fails", async () => {
mockDrizzleClient.query.usersTable.findFirst.mockResolvedValue({
id: "user123",
role: "member",
});
const dbError = new Error("Database error");
mockDrizzleClient.select.mockImplementation(() => ({
from: () => ({
innerJoin: () => ({
where: () => ({
limit: () => ({
orderBy: () => Promise.reject(dbError),
}),
}),
}),
}),
}));
await expect(
resolveOrganizationsWhereMember(
mockUserParent,
{ filter: undefined },
baseMockCtx,
),
).rejects.toThrow();
expect(baseMockCtx.log.error).toHaveBeenCalled();
});
test("administrator can query other user's organizations", async () => {
// Mock current user as admin
mockDrizzleClient.query.usersTable.findFirst.mockResolvedValue({
id: "admin123",
role: "administrator",
});
const adminCtx = {
...baseMockCtx,
currentClient: {
isAuthenticated: true,
user: { id: "admin123", role: "administrator" },
},
} as unknown as ContextType;
const organizationsMockData = [
{
membershipCreatedAt: new Date("2023-01-01"),
membershipOrganizationId: "org1",
organization: { id: "org1", name: "Test Organization" },
},
];
mockDrizzleClient.select.mockImplementation(() => ({
from: () => ({
innerJoin: () => ({
where: () => ({
limit: () => ({
orderBy: () => Promise.resolve(organizationsMockData),
}),
}),
}),
}),
}));
// Different user than the admin
const differentUserParent = {
id: "user456",
role: "member",
} as unknown as User;
// This should not throw because admin can access other user's data
const result = await resolveOrganizationsWhereMember(
differentUserParent,
{ filter: undefined },
adminCtx,
);
expect(result.edges.length).toBe(1);
});
test("returns expected cursor format for GraphQL connection", async () => {
mockDrizzleClient.query.usersTable.findFirst.mockResolvedValue({
id: "user123",
role: "member",
});
const testDate = new Date("2023-01-01T12:00:00Z");
const organizationsMockData = [
{
membershipCreatedAt: testDate,
membershipOrganizationId: "org1",
organization: { id: "org1", name: "Test Organization" },
},
];
mockDrizzleClient.select.mockImplementation(() => ({
from: () => ({
innerJoin: () => ({
where: () => ({
limit: () => ({
orderBy: () => Promise.resolve(organizationsMockData),
}),
}),
}),
}),
}));
const result = await resolveOrganizationsWhereMember(
mockUserParent,
{ filter: undefined },
baseMockCtx,
);
// Check cursor format
const expectedCursor = Buffer.from(
JSON.stringify({
createdAt: testDate.toISOString(),
organizationId: "org1",
}),
).toString("base64url");
expect(result.edges[0].cursor).toBe(expectedCursor);
expect(result.pageInfo.startCursor).toBe(expectedCursor);
expect(result.pageInfo.endCursor).toBe(expectedCursor);
});These tests cover:
Remember to uncomment the imports at the top of the file as indicated. This comprehensive test suite should satisfy the requirements for thorough testing of the |
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/User/organizationsWhereMember.ts(3 hunks)test/graphql/types/User/organizationsWhereMember.test.ts(1 hunks)
🔇 Additional comments (3)
test/graphql/types/User/organizationsWhereMember.test.ts (1)
24-49: Mock chaining approach looks correct.
Your chainable mock implementation formockWhere,mockFrom, andmockSelectis a neat approach to replicating Drizzle’s fluent API style. This setup provides a solid foundation for verifying how queries are built and executed.src/graphql/types/User/organizationsWhereMember.ts (2)
77-124: Authentication and authorization checks look solid.
You properly guard the resolver for both authentication and user ownership/administrator access, throwing tailoredTalawaGraphQLErrors when needed. This alignment with the rest of your codebase’s security model appears correct.
235-248: GraphQL field definition aligns with updated resolver.
Your addition of thefilterargument and the connection-based return type inorganizationsWhereMemberclosely matches the updatedresolveOrganizationsWhereMemberfunction. This ensures end-to-end consistency and clarity for consumers of the GraphQL API.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
test/graphql/types/User/organizationsWhereMember.test.ts (2)
181-205: 🧹 Nitpick (assertive)Enhance cursor-based pagination test.
The current test only verifies that
mockWherewas called but doesn't validate that the cursor is correctly decoded and used to build the pagination query. This was previously mentioned in a review comment.test("applies cursor-based pagination correctly", async () => { mockDrizzleClient.query.usersTable.findFirst.mockResolvedValue({ id: "user123", role: "member", }); const cursorArgs = { ...globalArgs, cursor: Buffer.from( JSON.stringify({ createdAt: "2024-01-01T12:00:00Z", organizationId: "org1", }), ).toString("base64url"), }; await resolveOrganizationsWhereMember( mockUserParent, cursorArgs, baseMockCtx, ); expect(mockWhere).toHaveBeenCalled(); + // Verify the cursor parameters are used correctly + const whereArg = mockWhere.mock.calls[0]?.[0]; + // Check that the whereArg contains conditions based on the cursor values + const whereString = whereArg.toString(); + expect(whereString).toContain("2024-01-01T12:00:00Z"); + expect(whereString).toContain("org1"); });
91-98: 🧹 Nitpick (assertive)Enhance error test assertions with specific error code verification.
The test correctly checks that a TalawaGraphQLError is thrown, but should also verify the specific error code to ensure the correct type of error is being thrown.
await expect( resolveOrganizationsWhereMember( mockUserParent, globalArgs, unauthenticatedCtx, ), -).rejects.toThrow(TalawaGraphQLError); +).rejects.toThrowError( + expect.objectContaining({ + extensions: { code: "unauthenticated" } + }) +);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/graphql/types/User/organizationsWhereMember.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)
|
@palisadoes PR is ready for review Summary:
|
palisadoes
merged commit 6152d31
into
PalisadoesFoundation:develop-postgres
Overview
This PR is necessary for PalisadoesFoundation/talawa-admin#3838
Parent Issue: #3230
Please merge together.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
resolveOrganizationsWhereMemberfunction to ensure proper error handling and filtering.resolveCreatedOrganizationsfunction to validate its behavior across multiple scenarios.