Add endpoint for analytics dashboard #134
Conversation
WalkthroughThe pull request introduces a new GraphQL resolver, Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
src/server-extension/types.ts (1)
24-37: LGTM! Consider using a more specific type for thedatefield.The
VouchCountPerMonthclass is well-defined with appropriate decorators and field types. The use of default values is a good practice for preventing null/undefined errors.However, consider using a more specific type for the
datefield instead of a string. GraphQL has a built-inDateTimescalar type which might be more appropriate for representing dates.You could modify the
datefield as follows:- @Field() - date: string = ""; + @Field(() => GraphQLISODateTime) + date: Date = new Date();Don't forget to import
GraphQLISODateTimefromtype-graphqlat the top of the file:import { ..., GraphQLISODateTime } from "type-graphql";This change would provide better type safety and automatic date parsing/serialization.
src/server-extension/organization-resolver.ts (1)
7-7: Inconsistent spelling: 'Organisation' vs 'Organization'The class is named
OrganisationResolver, but the file is namedorganization-resolver.ts, using different spellings of "organization". For consistency and to prevent confusion, it's recommended to use the same spelling throughout the codebase.Consider renaming the class to
OrganizationResolveror renaming the file toorganisation-resolver.ts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/server-extension/organization-resolver.ts (1 hunks)
- src/server-extension/resolvers/index.ts (1 hunks)
- src/server-extension/types.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
src/server-extension/resolvers/index.ts (1)
1-1: LGTM. Verify the new import path.The import path for
ProjectResolverhas been updated, which is likely part of a project structure reorganization. This change looks good, but it's important to ensure that the new path is correct.Let's verify that the new import path is correct:
✅ Verification successful
Verified: The new import path for
ProjectResolverexists and there are no remaining imports from the old path.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the project-resolver file exists fd -t f project-resolver.ts -X echo "project-resolver.ts exists" # Check for any remaining imports from the old path rg --type typescript '"../resolver"'Length of output: 210
Script:
#!/bin/bash # Verify that the project-resolver file exists fd -t f project-resolver.ts -X echo "project-resolver.ts exists" # Check for any remaining imports from the old path using a glob pattern rg --glob '*.ts' '"../resolver"'Length of output: 171
src/server-extension/types.ts (1)
15-22: LGTM! Well-structured class definition.The
VouchCountResultclass is well-defined with appropriate decorators and field types. The use of default values is a good practice for preventing null/undefined errors.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/server-extension/helper.ts (1 hunks)
- src/server-extension/organization-resolver.ts (1 hunks)
🧰 Additional context used
🪛 Biome
src/server-extension/helper.ts
[error] 20-20: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (6)
src/server-extension/organization-resolver.ts (6)
1-9: LGTM: Imports and class declaration are well-structured.The imports are appropriate for the functionality, and the class declaration with its constructor is correctly implemented.
11-27: Great job on input validation!The input validation for date formats and range is well-implemented, addressing the previous concern about potential errors due to invalid date formats. The error messages are clear and informative.
1-74: Overall, good implementation with room for improvementThe
OrganisationResolverclass is well-structured and implements the required functionality for fetching organization vouch counts. The input validation and error handling are particularly well done. However, there are a few areas where the code could be further improved:
- Consider using TypeORM's QueryBuilder instead of raw SQL for better maintainability and security.
- Enhance type safety by avoiding the use of
anytypes, especially in the result mapping function.- Include more detailed error information when rethrowing exceptions to aid in debugging.
Implementing these suggestions will further improve the code quality, maintainability, and robustness of this resolver.
28-54: 🛠️ Refactor suggestionConsider using TypeORM QueryBuilder for improved maintainability
While the current SQL query is well-structured, using TypeORM's QueryBuilder can enhance code readability, maintainability, and security. It provides better integration with TypeORM's features and helps prevent SQL injection vulnerabilities.
As suggested in a previous review, consider refactoring the query using QueryBuilder. Here's an example of how it could be implemented:
const resultPerMonth = await manager .getRepository('project_attestation') .createQueryBuilder('project_attestation') .select("TO_CHAR(project_attestation.attest_timestamp, 'YYYY-MM')", 'date') .addSelect('COUNT(*)', 'total_count') .addSelect( "SUM(CASE WHEN project_attestation.comment IS NOT NULL AND project_attestation.comment != '' THEN 1 ELSE 0 END)", 'count_with_comments' ) .leftJoin('attestor_organisation', 'attestor_organisation', 'project_attestation.attestor_organisation_id = attestor_organisation.id') .where('attestor_organisation.organisation_id = :organisationId', { organisationId }) .andWhere('project_attestation.vouch = true') .andWhere('project_attestation.attest_timestamp BETWEEN :fromDate AND :toDate', { fromDate, toDate }) .groupBy("TO_CHAR(project_attestation.attest_timestamp, 'YYYY-MM')") .orderBy('date') .getRawMany();This approach would align better with TypeORM best practices and improve overall code quality.
56-68: 🛠️ Refactor suggestionEnhance type safety by avoiding 'any' type
To improve type safety and prevent potential runtime errors, consider defining a specific interface or type for the query result instead of using
any. This addresses a previous review comment and aligns with TypeScript best practices.Here's a suggested implementation:
interface VouchCountRow { date: string; total_count: string; count_with_comments: string; } const totalPerMonth: VouchCountPerMonth[] = resultPerMonth.map( (row: VouchCountRow) => ({ date: row.date, totalCount: Number(row.total_count), countWithComments: Number(row.count_with_comments), countWithoutComments: Number(row.total_count) - Number(row.count_with_comments), }) );This approach provides better type checking and makes the code more robust and self-documenting.
69-73: 🛠️ Refactor suggestionImprove error handling by including original error details
The current error handling approach of catching, logging, and rethrowing errors is good. However, to further aid in debugging, consider including the original error message in the thrown exception, as suggested in a previous review.
You can modify the error handling as follows:
} catch (error: unknown) { console.error("Error fetching vouch count by date:", error); if (error instanceof Error) { throw new Error(`Failed to fetch vouch count by date: ${error.message}`); } else { throw new Error("Failed to fetch vouch count by date: Unknown error"); } }This change will provide more context about the specific error that occurred, making it easier to diagnose and fix issues in production.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation