Feat/add get organisation user vouch count by source #145
Conversation
WalkthroughThe changes introduce a new method Changes
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/server-extension/types.ts (2)
42-49: Consider renamingattestorIdto be more descriptive.While the implementation is correct, consider renaming
attestorIdto something more descriptive likeuserIdorvoucherIdto better reflect its purpose in the context of vouch counting.
51-58: LGTM! Consider adding JSDoc comments.The implementation is well-structured and follows the existing patterns. To improve maintainability, consider adding JSDoc comments to describe the purpose of this result type and its fields.
Example documentation:
+/** + * Represents the result of counting vouches grouped by user + */ @ObjectType() export class VouchCountByUserResult { + /** Total number of vouches across all users */ @Field(() => Int) totalVouches: number = 0; + /** Breakdown of vouch counts per user */ @Field(() => [VouchCountByUser]) vouchCountByUser: VouchCountByUser[] = []; }src/server-extension/organization-resolver.ts (3)
116-135: Consider performance optimizations for the SQL queryA few suggestions to optimize the query:
- Consider using LEFT JOIN instead of INNER JOIN if you need to include attestors with no vouches
- Add pagination to handle large result sets
- Consider adding appropriate indexes for the WHERE clause conditions
- SELECT - attestor_organisation.attestor_id AS attestor_id, - COUNT(*) AS total_count - FROM - project_attestation - JOIN attestor_organisation - ON project_attestation.attestor_organisation_id = attestor_organisation.id - JOIN project - ON project_attestation.project_id = project.id - WHERE - attestor_organisation.organisation_id = $1 - AND project.source = $2 - AND project_attestation.vouch = true - AND project_attestation.attest_timestamp BETWEEN $3 AND $4 - GROUP BY - attestor_organisation.attestor_id - ORDER BY - total_count DESC; + SELECT + attestor_organisation.attestor_id AS attestor_id, + COUNT(*) AS total_count + FROM + project_attestation + LEFT JOIN attestor_organisation + ON project_attestation.attestor_organisation_id = attestor_organisation.id + LEFT JOIN project + ON project_attestation.project_id = project.id + WHERE + attestor_organisation.organisation_id = $1 + AND project.source = $2 + AND project_attestation.vouch = true + AND project_attestation.attest_timestamp BETWEEN $3 AND $4 + GROUP BY + attestor_organisation.attestor_id + ORDER BY + total_count DESC + LIMIT $5 + OFFSET $6;
156-161: Enhance error handling with specific error typesThe current error handling catches all errors and wraps them in a generic Error. Consider:
- Creating specific error types for different failure scenarios
- Adding more context in error messages (e.g., including the organisationId)
- } catch (error: any) { - console.error("Error fetching attestor vouch count by source:", error); - throw new Error( - `Failed to fetch attestor vouch count by source: ${error.message}` - ); + } catch (error: any) { + console.error( + `Error fetching attestor vouch count by source for organisation ${organisationId}:`, + error + ); + if (error.code === '23503') { // Foreign key violation + throw new Error(`Organisation ${organisationId} not found`); + } + throw new Error( + `Failed to fetch attestor vouch count by source for organisation ${organisationId}: ${error.message}` + );
93-162: Consider adding unit tests for the new functionalityThe new query method should be thoroughly tested, including:
- Happy path with various date ranges
- Edge cases for date validation
- Error scenarios
- Empty result handling
Would you like me to help generate comprehensive unit tests for this new functionality?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/server-extension/organization-resolver.ts(2 hunks)src/server-extension/types.ts(1 hunks)
🔇 Additional comments (2)
src/server-extension/types.ts (1)
41-58: Verify consistent naming across the codebase.
Let's verify that the naming conventions used in these new types are consistent with the rest of the codebase.
✅ Verification successful
Based on the search results, I can now generate the final response:
Naming conventions are consistent across the codebase
The naming conventions used in these new types are consistent with the existing codebase patterns:
- The term "attestor" is consistently used throughout (not "attester")
- The suffix "Result" for response types follows the established pattern
- The camelCase naming for properties is consistent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check naming conventions for similar types and fields
# Check for other uses of "attestor" vs "user" terminology
rg -i "attestor|vouch.*user" --type ts
# Check for similar result type patterns
ast-grep --pattern '@ObjectType()
export class $_Result {
$$$
}'
Length of output: 10727
src/server-extension/organization-resolver.ts (1)
3-8: LGTM: Import changes are appropriate
The new type imports align with the added functionality and follow the existing import pattern.
MohammadPCh
deleted the
feat/add-getOrganisationUserVouchCountBySource
branch
Summary by CodeRabbit
New Features
Bug Fixes
Documentation