Fixed3244 #3284
Fixed3244 #3284
Conversation
|
Warning Rate limit exceeded@igennova has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 40 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 a new GraphQL query field named Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant G as GraphQL Server
participant V as Validation (QueryOrganizationInput)
participant DB as Organizations Table
participant Auth as Authorization
C->>G: Query organizations(input)
G->>Auth: isAuthorized()
Auth-->>G: true
G->>V: Validate input.id
alt Validation fails (e.g., invalid UUID)
V-->>G: Error ("Invalid arguments")
G-->>C: Return GraphQL Error
else Validation passes
alt Input includes id
G->>DB: Query organization by id
alt Organization found
DB-->>G: Organization data
G-->>C: Return [Organization]
else Organization not found
DB-->>G: Not found
G-->>C: Error ("Resource not found")
end
else No id provided
G->>DB: Query list of organizations (limit 20)
DB-->>G: List Organizations
G-->>C: Return [Organization]
end
end
Possibly related PRs
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 #3284 +/- ##
====================================================
- Coverage 48.12% 48.09% -0.04%
====================================================
Files 455 457 +2
Lines 34036 34112 +76
Branches 908 911 +3
====================================================
+ Hits 16381 16406 +25
- Misses 17655 17706 +51 ☔ View full report in Codecov by Sentry. |
|
@coderabbit.ai full review |
|
@igennova I'll perform a comprehensive review of all the changes in this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
schema.graphql(1 hunks)src/graphql/inputs/QueryOrganizationInput.ts(1 hunks)src/graphql/types/Query/index.ts(1 hunks)src/graphql/types/Query/organizations.ts(1 hunks)src/utilities/auth.ts(1 hunks)test/routes/graphql/Query/organizations.test.ts(1 hunks)test/routes/graphql/documentNodes.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/utilities/auth.ts
[warning] 7-7: src/utilities/auth.ts#L7
Added line #L7 was not covered by tests
[warning] 10-11: src/utilities/auth.ts#L10-L11
Added lines #L10 - L11 were not covered by tests
src/graphql/types/Query/organizations.ts
[warning] 29-29: src/graphql/types/Query/organizations.ts#L29
Added line #L29 was not covered by tests
[warning] 31-36: src/graphql/types/Query/organizations.ts#L31-L36
Added lines #L31 - L36 were not covered by tests
[warning] 39-49: src/graphql/types/Query/organizations.ts#L39-L49
Added lines #L39 - L49 were not covered by tests
[warning] 52-57: src/graphql/types/Query/organizations.ts#L52-L57
Added lines #L52 - L57 were not covered by tests
[warning] 60-67: src/graphql/types/Query/organizations.ts#L60-L67
Added lines #L60 - L67 were not covered by tests
[warning] 70-72: src/graphql/types/Query/organizations.ts#L70-L72
Added lines #L70 - L72 were not covered by tests
[warning] 75-78: src/graphql/types/Query/organizations.ts#L75-L78
Added lines #L75 - L78 were not covered by tests
[warning] 80-80: src/graphql/types/Query/organizations.ts#L80
Added line #L80 was not covered by tests
[warning] 83-89: src/graphql/types/Query/organizations.ts#L83-L89
Added lines #L83 - L89 were not covered by tests
🔇 Additional comments (6)
src/graphql/inputs/QueryOrganizationInput.ts (1)
6-13: Validate the interplay between nullable, optional, and transform.Marking
idas both.nullable()and.optional()but then throwing an error when it isnullcan be confusing. Ensure that this logic accurately reflects your intended behavior:
• Ifidcan be omitted,.optional()with a default might be more suitable.
• Ifidmust always be non-null when provided, you might drop.nullable().Confirm that consumers of this input align with your constraints.
src/graphql/types/Query/index.ts (1)
22-22: LGTM!The import is correctly placed and follows the established pattern.
test/routes/graphql/documentNodes.ts (1)
468-479: LGTM!The query document is well-structured and selects appropriate fields for organization listing during registration.
test/routes/graphql/Query/organizations.test.ts (3)
11-14: LGTM!The authorization mock is correctly implemented and documented.
16-26: LGTM!The GraphQL query mock follows the schema definition and includes necessary fields.
28-98: LGTM!The mock client implementation thoroughly handles different scenarios with appropriate error responses.
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/routes/graphql/Query/organizations.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 (5)
test/routes/graphql/Query/organizations.test.ts (5)
1-10: LGTM! Well-structured imports.The imports are comprehensive and properly organized, including all necessary testing utilities, type definitions, and mock dependencies.
11-14: LGTM! Clear authorization mock.The authorization mock is simple and clear, always returning true for test purposes.
16-26: LGTM! Well-structured GraphQL query mock.The query mock clearly defines the expected structure with proper indentation.
100-176: LGTM! Comprehensive error handling test suites.The test suites for invalid arguments and resource not found scenarios are well-structured with thorough assertions.
177-214: Add test case for empty input object.While the current test cases are good, one suggested scenario from the previous review is still missing.
Add a test case for querying with an empty input object:
test("returns a list of organizations when empty input object is provided.", async () => { const result = await mercuriusClient.query(Query_organizations, { variables: { input: {} } }); expect(result.errors).toBeUndefined(); expect(result.data.organizations).toEqual( expect.arrayContaining([ expect.objectContaining({ id: expect.any(String), name: expect.any(String) }) ]) ); });
|
@coderabbit.ai full review |
|
@igennova I'll review all the changes in this PR. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/graphql/types/Query/organizations.ts (1)
136-146: 🛠️ Refactor suggestionImprove error handling with specific error codes.
The current implementation uses a generic "unauthorized_action" error code for all errors. Consider:
- Using specific error codes based on the error type
- Using the context logger instead of console.error
} catch (error) { - console.error("Error in organizations query:", error); + ctx.log.error("Error in organizations query:", error); + if (error instanceof TalawaGraphQLError) { + throw error; + } + throw new TalawaGraphQLError({ extensions: { - code: "unauthorized_action", + code: "internal_server_error", + message: "An unexpected error occurred", }, }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/graphql/types/Query/organizations.ts(1 hunks)test/routes/graphql/Query/organizations.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/graphql/types/Query/organizations.ts (1)
Learnt from: GlenDsza
PR: PalisadoesFoundation/talawa-api#2615
File: tests/resolvers/Query/actionItemsByOrganization.spec.ts:51-82
Timestamp: 2024-11-12T10:41:04.351Z
Learning: When reviewing `tests/resolvers/Query/actionItemsByOrganization.spec.ts`, if the existing test cases already provide 100% coverage, avoid suggesting additional test cases.
🪛 Biome (1.9.4)
test/routes/graphql/Query/organizations.test.ts
[error] 19-19: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 3-3: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
src/graphql/types/Query/organizations.ts
[error] 15-15: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 16-16: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 41-41: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 41-41: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
🪛 GitHub Actions: Pull request workflow
test/routes/graphql/Query/organizations.test.ts
[error] 19-19: lint/correctness/noUnreachable: This code will never be reached because this statement will return from the function beforehand.
[error] 3-3: lint/style/useImportType: All these imports are only used as types. Use import type for 'GraphQLContext'.
[error] 2-5: organizeImports: Import statements could be sorted.
[error] 19-19: format: Formatter would have printed the following content: return mockContext as unknown as GraphQLContext;
src/graphql/types/Query/organizations.ts
[error] 15-15: lint/suspicious/noExplicitAny: Unexpected any. Specify a different type.
[error] 16-16: lint/suspicious/noExplicitAny: Unexpected any. Specify a different type.
[error] 41-41: lint/suspicious/noExplicitAny: Unexpected any. Specify a different type.
[error] 41-41: lint/suspicious/noExplicitAny: Unexpected any. Specify a different type.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (3)
test/routes/graphql/Query/organizations.test.ts (2)
29-49: LGTM! Input validation test is well-structured.The test case effectively validates the error handling for invalid UUID format input.
51-115: LGTM! Organization fetching tests are comprehensive.The test cases effectively cover:
- Fetching a single organization by ID
- Handling non-existent organization
- Fetching multiple organizations (default behavior)
src/graphql/types/Query/organizations.ts (1)
72-85: LGTM! Query field is well-documented.The query field definition includes clear descriptions for both the field and its input argument.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/graphql/types/Query/organizations.ts (2)
75-77: 🧹 Nitpick (assertive)Implement pagination and configurable limit.
The current implementation has a hard-coded limit of 20 records. Consider:
- Making the limit configurable via input
- Implementing proper pagination (cursor-based or offset-based)
- Adding sorting options
return await ctx.drizzleClient.query.organizationsTable.findMany({ - limit: 20, + limit: args.input?.limit ?? 20, + offset: args.input?.offset, + orderBy: args.input?.orderBy, });
78-89: 🛠️ Refactor suggestionImprove error handling granularity.
The current implementation throws a generic "unauthorized_action" error for all exceptions. Consider:
- Differentiating between error types (e.g., database errors, validation errors)
- Preserving the original error context
- Adding specific error codes for different scenarios
} catch (error) { console.error("Error in organizations query:", error); + if (error instanceof TalawaGraphQLError) { + throw error; + } + if (error.code === 'P2002') { + throw new TalawaGraphQLError({ + extensions: { + code: "database_error", + message: "Database constraint violation", + }, + }); + } throw new TalawaGraphQLError({ extensions: { - code: "unauthorized_action", + code: "internal_server_error", + message: "An unexpected error occurred", }, }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/graphql/types/Query/organizations.ts(1 hunks)test/routes/graphql/Query/organizations.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/graphql/types/Query/organizations.ts (1)
Learnt from: GlenDsza
PR: PalisadoesFoundation/talawa-api#2615
File: tests/resolvers/Query/actionItemsByOrganization.spec.ts:51-82
Timestamp: 2024-11-12T10:41:04.351Z
Learning: When reviewing `tests/resolvers/Query/actionItemsByOrganization.spec.ts`, if the existing test cases already provide 100% coverage, avoid suggesting additional test cases.
🪛 Biome (1.9.4)
test/routes/graphql/Query/organizations.test.ts
[error] 103-103: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 119-119: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 120-120: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Actions: Pull request workflow
test/routes/graphql/Query/organizations.test.ts
[error] 103-103: lint/style/noNonNullAssertion: Forbidden non-null assertion. Unsafe fix: Replace with optional chain operator ?.
[error] 119-119: lint/style/noNonNullAssertion: Forbidden non-null assertion. Unsafe fix: Replace with optional chain operator ?.
[error] 120-120: lint/complexity/useOptionalChain: Change to an optional chain.
[error] 6-8: Import statements could be sorted.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (1)
src/graphql/types/Query/organizations.ts (1)
14-27: Consider incorporating authorization checks.Your
organizationsquery currently lacks authorization checks. This means any user could potentially query these records. Incorporate yourisAuthorizedlogic here to ensure only permitted users can access organization data.
| // Assuming we have a known organization ID in the test database | ||
| const knownOrganizationId = "known-org-id"; // Replace with actual test data |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Replace hard-coded test data with proper test fixtures.
Consider using a test fixture or factory function to generate test data instead of hard-coding the organization ID.
const createTestOrganization = () => ({
id: faker.string.uuid(),
name: faker.company.name(),
// Add other required fields
});| expect(result.data.organizations![0]).toEqual( | ||
| expect.objectContaining({ | ||
| id: knownOrganizationId, | ||
| // Add other expected organization fields | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Fix non-null assertions and improve type safety.
Replace non-null assertions with proper null checks and add missing field assertions.
-expect(result.data.organizations![0]).toEqual(
+expect(result.data.organizations?.[0]).toEqual(
expect.objectContaining({
id: knownOrganizationId,
- // Add other expected organization fields
+ name: expect.any(String),
+ description: expect.any(String),
+ createdAt: expect.any(String),
}),
);
-expect(result.data.organizations!.length).toBeLessThanOrEqual(20);
-expect(result.data.organizations && result.data.organizations[0]).toEqual(
+expect(result.data.organizations?.length).toBeLessThanOrEqual(20);
+expect(result.data.organizations?.[0]).toEqual(
expect.objectContaining({
id: expect.any(String),
- // Add other expected organization fields
+ name: expect.any(String),
+ description: expect.any(String),
+ createdAt: expect.any(String),
}),
);Also applies to: 119-125
🧰 Tools
🪛 Biome (1.9.4)
[error] 103-103: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
🪛 GitHub Actions: Pull request workflow
[error] 103-103: lint/style/noNonNullAssertion: Forbidden non-null assertion. Unsafe fix: Replace with optional chain operator ?.
| suite("handles unauthorized access", () => { | ||
| test("returns unauthorized_action error when user lacks permissions", async () => { | ||
| // Simulate an unauthorized request (implementation depends on your auth setup) | ||
| const result = await mercuriusClient.query(Query_organizations, { | ||
| headers: { | ||
| authorization: "bearer invalid_token", | ||
| }, | ||
| variables: {}, | ||
| }); | ||
|
|
||
| expect(result.data.organizations).toEqual(null); | ||
| expect(result.errors).toEqual( | ||
| expect.arrayContaining<TalawaGraphQLFormattedError>([ | ||
| expect.objectContaining<TalawaGraphQLFormattedError>({ | ||
| extensions: expect.objectContaining<UnauthorizedActionExtensions>({ | ||
| code: "unauthorized_action", | ||
| }), | ||
| message: expect.any(String), | ||
| path: ["organizations"], | ||
| }), | ||
| ]), | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance authorization test coverage.
Consider adding more authorization test scenarios:
- Different user roles
- Valid token but insufficient permissions
- Expired token
test('returns unauthorized_action error when token is expired', async () => {
const expiredToken = generateExpiredToken();
const result = await mercuriusClient.query(Query_organizations, {
headers: {
authorization: `bearer ${expiredToken}`,
},
variables: {},
});
// Add assertions
});
test('returns unauthorized_action error when user has insufficient permissions', async () => {
const tokenWithInsufficientPerms = generateTokenWithRole('basic_user');
const result = await mercuriusClient.query(Query_organizations, {
headers: {
authorization: `bearer ${tokenWithInsufficientPerms}`,
},
variables: {},
});
// Add assertions
});
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/routes/graphql/Query/organizations.test.ts (1)
129-152:⚠️ Potential issueReplace hardcoded token and enhance authorization tests.
The current implementation has two issues:
- Hardcoded bearer token is a security concern
- Limited coverage of authorization scenarios
Apply this diff to improve the test suite:
+const generateTestToken = (type: 'invalid' | 'expired' | 'insufficient_perms'): string => { + switch (type) { + case 'invalid': + return faker.string.alphanumeric(32); + case 'expired': + // Generate expired JWT token + return 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyLCJleHAiOjE1MTYyMzkwMjJ9'; + case 'insufficient_perms': + // Generate valid token with basic permissions + return 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwicm9sZSI6ImJhc2ljX3VzZXIiLCJpYXQiOjE1MTYyMzkwMjJ9'; + } +}; suite("handles unauthorized access", () => { - test("returns unauthorized_action error when user lacks permissions", async () => { + test.each([ + ['invalid token', 'invalid'], + ['expired token', 'expired'], + ['insufficient permissions', 'insufficient_perms'], + ])('returns unauthorized_action error when using %s', async (_, tokenType) => { const result = await mercuriusClient.query(Query_organizations, { headers: { - authorization: "bearer invalid_token", + authorization: `bearer ${generateTestToken(tokenType as 'invalid' | 'expired' | 'insufficient_perms')}`, }, variables: {}, }); expect(result.data.organizations).toEqual(null); expect(result.errors).toEqual( expect.arrayContaining<TalawaGraphQLFormattedError>([ expect.objectContaining<TalawaGraphQLFormattedError>({ extensions: expect.objectContaining<UnauthorizedActionExtensions>({ code: "unauthorized_action", + reason: expect.any(String), }), message: expect.any(String), path: ["organizations"], }), ]), ); }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/routes/graphql/Query/organizations.test.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pull request workflow
test/routes/graphql/Query/organizations.test.ts
[error] 104-106: Formatter would have printed the following content: expected object structure does not match.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (1)
test/routes/graphql/Query/organizations.test.ts (1)
52-84: LGTM! Good use of faker for test data generation.The test case effectively validates the handling of non-existent resources and uses faker to generate random ULIDs, which is a good practice for test data generation.
| test("input validation fails with invalid ID format", async () => { | ||
| const result = await mercuriusClient.query(Query_organizations, { | ||
| variables: { | ||
| input: { | ||
| id: "invalid-id-format", | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(result.data.organizations).toEqual(null); | ||
| expect(result.errors).toEqual( | ||
| expect.arrayContaining<TalawaGraphQLFormattedError>([ | ||
| expect.objectContaining<TalawaGraphQLFormattedError>({ | ||
| extensions: expect.objectContaining<InvalidArgumentsExtensions>({ | ||
| code: "invalid_arguments", | ||
| issues: expect.arrayContaining< | ||
| InvalidArgumentsExtensions["issues"][number] | ||
| >([ | ||
| { | ||
| argumentPath: ["input", "id"], | ||
| message: expect.any(String), | ||
| }, | ||
| ]), | ||
| }), | ||
| message: expect.any(String), | ||
| path: ["organizations"], | ||
| }), | ||
| ]), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add test cases for additional invalid input scenarios.
Consider adding test cases for:
- Empty string ID
- Numeric ID
- Special characters in ID
- Very long ID string
Example test case:
test("input validation fails with empty string ID", async () => {
const result = await mercuriusClient.query(Query_organizations, {
variables: {
input: {
id: "",
},
},
});
expect(result.data.organizations).toEqual(null);
expect(result.errors).toEqual(
expect.arrayContaining<TalawaGraphQLFormattedError>([
expect.objectContaining<TalawaGraphQLFormattedError>({
extensions: expect.objectContaining<InvalidArgumentsExtensions>({
code: "invalid_arguments",
issues: expect.arrayContaining([
{
argumentPath: ["input", "id"],
message: expect.any(String),
},
]),
}),
}),
]),
);
});| test("returns a single organization when ID is provided", async () => { | ||
| // Assuming we have a known organization ID in the test database | ||
| const knownOrganizationId = ""; // Replace with actual test data | ||
| const result = await mercuriusClient.query(Query_organizations, { | ||
| variables: { | ||
| input: { | ||
| id: knownOrganizationId, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(result.errors).toBeUndefined(); | ||
| expect(result.data.organizations).toHaveLength(1); | ||
| expect(result.data.organizations).not.toBeNull(); | ||
| expect(result.data.organizations?.[0]).toEqual( | ||
| expect.objectContaining({ | ||
| id: knownOrganizationId, | ||
| }) | ||
| ); | ||
|
|
There was a problem hiding this comment.
Enhance test data and assertions for organization retrieval.
The test case needs improvement in several areas:
- Empty organization ID needs to be replaced with actual test data
- Assertions should verify more organization fields
Apply this diff to improve the test:
-const knownOrganizationId = ""; // Replace with actual test data
+const mockOrganization = {
+ id: faker.string.uuid(),
+ name: faker.company.name(),
+ description: faker.company.catchPhrase(),
+ createdAt: faker.date.past().toISOString(),
+};
+
+// Setup: Create the organization in test database
+
const result = await mercuriusClient.query(Query_organizations, {
variables: {
input: {
- id: knownOrganizationId,
+ id: mockOrganization.id,
},
},
});
expect(result.errors).toBeUndefined();
expect(result.data.organizations).toHaveLength(1);
expect(result.data.organizations).not.toBeNull();
expect(result.data.organizations?.[0]).toEqual(
expect.objectContaining({
- id: knownOrganizationId,
+ id: mockOrganization.id,
+ name: mockOrganization.name,
+ description: mockOrganization.description,
+ createdAt: expect.any(String),
})
);📝 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.
| test("returns a single organization when ID is provided", async () => { | |
| // Assuming we have a known organization ID in the test database | |
| const knownOrganizationId = ""; // Replace with actual test data | |
| const result = await mercuriusClient.query(Query_organizations, { | |
| variables: { | |
| input: { | |
| id: knownOrganizationId, | |
| }, | |
| }, | |
| }); | |
| expect(result.errors).toBeUndefined(); | |
| expect(result.data.organizations).toHaveLength(1); | |
| expect(result.data.organizations).not.toBeNull(); | |
| expect(result.data.organizations?.[0]).toEqual( | |
| expect.objectContaining({ | |
| id: knownOrganizationId, | |
| }) | |
| ); | |
| test("returns a single organization when ID is provided", async () => { | |
| // Assuming we have a known organization ID in the test database | |
| const mockOrganization = { | |
| id: faker.string.uuid(), | |
| name: faker.company.name(), | |
| description: faker.company.catchPhrase(), | |
| createdAt: faker.date.past().toISOString(), | |
| }; | |
| // Setup: Create the organization in test database | |
| const result = await mercuriusClient.query(Query_organizations, { | |
| variables: { | |
| input: { | |
| id: mockOrganization.id, | |
| }, | |
| }, | |
| }); | |
| expect(result.errors).toBeUndefined(); | |
| expect(result.data.organizations).toHaveLength(1); | |
| expect(result.data.organizations).not.toBeNull(); | |
| expect(result.data.organizations?.[0]).toEqual( | |
| expect.objectContaining({ | |
| id: mockOrganization.id, | |
| name: mockOrganization.name, | |
| description: mockOrganization.description, | |
| createdAt: expect.any(String), | |
| }) | |
| ); | |
| }); |
🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] 104-106: Formatter would have printed the following content: expected object structure does not match.
| test("returns unauthorized_action error when user lacks permissions", async () => { | ||
| const result = (await mercuriusClient.query(Query_organizations, { | ||
| headers: { | ||
| authorization: "bearer invalid_token", |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical test
What kind of change does this PR introduce?
Feature - Added an API to fetch organizations before user registration.
Issue Number
Fixes #3244
Snapshots/Videos
If relevant, did you update the documentation?
Summary
This PR introduces a new API in the
develop-postgresbranch to fetch organizations before user registration.organizationsAPI is in thedevelopbranch, so I created a separate API indevelop-postgresfor this functionality.Does this PR introduce a breaking change?
No.
Checklist
CodeRabbit AI Review
Test Coverage
Other information
Have you read the contributing guide?
Yes.
Summary by CodeRabbit
New Features
Tests