Add organization sort #133
Conversation
|
@aminlatifi can you review it? It's not finished but I really appreciate if you take a look and approve the way that I handle it. thanks |
WalkthroughThe changes introduce a new function Changes
Possibly related PRs
Poem
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: 6
🧹 Outside diff range and nitpick comments (5)
src/server-extension/helper.ts (1)
1-16: Overall assessment: Useful addition with room for improvement.The
getSelectedFieldsfunction provides a valuable utility for extracting selected fields from GraphQL queries. However, to enhance its reliability and maintainability, consider implementing the suggested improvements:
- Strengthen type safety by replacing 'any' with more specific types.
- Add comprehensive error handling for potential edge cases.
- Increase flexibility by making the 'project' prefix configurable.
These enhancements will make the function more robust and easier to use across different contexts in the project.
src/server-extension/types.ts (2)
12-22: LGTM:OrganisationProjectTypeis well-structured, but needs clarification.The
OrganisationProjectTypeclass is correctly defined with appropriate fields and types. However, the purpose of thevouchfield is not immediately clear from the context.Could you please provide a brief comment or documentation explaining the purpose and usage of the
vouchfield? This would improve the code's self-documentation and make it easier for other developers to understand and use this type correctly.
24-34: LGTM:ProjectTypeis well-structured, but consider some improvements.The
ProjectTypeclass is correctly defined. However, there are a couple of points to consider:
The
titlefield is nullable. Is this intentional? If so, it might be helpful to add a brief comment explaining why a project can have a null title.There's a commented-out field
attestedOrganisations. Is this a planned feature? If so, consider adding a TODO comment explaining the intent and any pending work.These additions would improve the code's self-documentation and make future development easier.
src/server-extension/resolver.ts (2)
16-17: Remove Unnecessary Nullability forlimitandoffsetThe arguments
limitandoffsetare marked as nullable with{ nullable: true }, but default values are provided (limit: number = 10,offset: number = 0). This can cause confusion and potential issues ifnullis explicitly passed, as it would override the default values.Consider removing the
nullable: trueoption to enforce that these arguments always have numeric values:- @Arg("limit", () => Number, { nullable: true }) limit: number = 10, - @Arg("offset", () => Number, { nullable: true }) offset: number = 0, + @Arg("limit", () => Number) limit: number = 10, + @Arg("offset", () => Number) offset: number = 0,
54-55: Use a Logging Framework for Error HandlingUsing
console.erroris not recommended for production applications. It’s better to use a dedicated logging framework that can handle different log levels and output logs appropriately.Consider using a logging library like
winstonorpino:- console.error("Error fetching and sorting projects:", error); + logger.error("Error fetching and sorting projects:", error);Make sure to import and configure the logger at the beginning of your file:
import { logger } from './logger'; // Assuming you have a logger setup
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/server-extension/helper.ts (1 hunks)
- src/server-extension/resolver.ts (1 hunks)
- src/server-extension/resolvers/index.ts (1 hunks)
- src/server-extension/types.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/server-extension/resolvers/index.ts
🧰 Additional context used
🪛 Biome
src/server-extension/resolver.ts
[error] 23-23: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
🔇 Additional comments (5)
src/server-extension/helper.ts (1)
1-1: LGTM: Import statement is correct.The import of
GraphQLResolveInfofrom the 'graphql' package is appropriate for the function's parameter type.src/server-extension/types.ts (3)
1-1: LGTM: Imports are appropriate and concise.The import statement correctly includes all necessary decorators and types from 'type-graphql' that are used in the file.
3-10: LGTM:OrganisationTypeis well-structured.The
OrganisationTypeclass is correctly defined with appropriate fields:
idas a non-nullable ID typenameas a non-nullable stringThis structure provides a clear and concise representation of an organization in the GraphQL schema.
1-34: Overall, the GraphQL types are well-defined and structured.This file introduces clear and concise GraphQL object types using the
type-graphqllibrary. The types are well-structured and follow good practices. The suggested improvements are minor and mainly relate to enhancing documentation for better clarity and maintainability.Great job on implementing these GraphQL types!
src/server-extension/resolver.ts (1)
50-52: Map Raw Query Results toProjectTypeInstancesThe
manager.query()method returns raw results that may not automatically conform to theProjectType. This can lead to issues if the data types or field names don't match exactly.Consider mapping the raw results to instances of
ProjectType:return rawProjects.map(raw => Object.assign(new ProjectType(), raw));Ensure that the field names in the raw results match the properties of
ProjectType.
|
@aminlatifi Thanks for your great suggestions! I agree with you, and I'll limit it to just the projects' IDs. My initial goal was to make the resolver dynamic, but I realized it’s more efficient to retrieve only the projects' IDs from this endpoint and then make a separate request for the project details. Thanks again for your input! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/server-extension/resolver.ts (1)
49-52: Enhance error handling for better debuggingWhen re-throwing the error, including the original error message can provide more context and aid in debugging. This ensures that the calling function receives detailed information about what went wrong.
Apply this change to include the original error message:
catch (error) { console.error("Error fetching and sorting projects:", error); - throw new Error("Failed to fetch and sort projects"); + throw new Error(`Failed to fetch and sort projects: ${error.message}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/server-extension/resolver.ts (1 hunks)
- src/server-extension/types.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server-extension/types.ts
🧰 Additional context used
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/server-extension/resolver.ts (2)
14-27: LGTM:getSortByfunction is well-structuredThe
getSortByfunction effectively mapsEProjectSortvalues to sorting criteria. It covers all enum cases and provides a default, which is good practice.Consider adding a return type annotation to enhance type safety:
const getSortBy = (sortBy: EProjectSort): { sortBy: string; order: 'ASC' | 'DESC' } => { // ... existing implementation ... }
33-40: LGTM: Query method declaration is well-structured, but contains an unused parameterThe
getProjectsSortedByVouchOrFlagmethod is properly decorated and has a well-structured signature for a GraphQL query. The default values forlimitandoffsetare a nice touch.The
@Info() info: GraphQLResolveInfoparameter is not used in the method implementation. Consider removing it if it's not needed:async getProjectsSortedByVouchOrFlag( @Arg("orgIds", () => [String]) orgIds: string[], @Arg("sortBy", () => String) sortBy: EProjectSort, @Arg("limit", () => Number, { nullable: true }) limit: number = 10, @Arg("offset", () => Number, { nullable: true }) offset: number = 0 ): Promise<ProjectsSortedByVouchOrFlagType[]> { // ... existing implementation ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/server-extension/resolver.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
src/server-extension/resolver.ts (3)
1-12: LGTM: Imports and enum declaration look goodThe imports are appropriate for a GraphQL resolver, and the
EProjectSortenum provides a clear set of sorting options for projects. This structure enhances code readability and maintainability.
29-31: LGTM:ProjectResolverclass declaration is well-structuredThe
ProjectResolverclass is properly decorated with@Resolver()for use with TypeGraphQL. The constructor's use of a transaction function is a good practice for managing database connections, promoting better control over database operations.
41-80: LGTM: Method implementation is secure and functional, with room for minor improvementsThe
getProjectsSortedByVouchOrFlagmethod implementation uses a parameterized SQL query, which addresses previous SQL injection concerns. The query logic appears correct, joining the necessary tables and applying sorting based on the input.Consider the following improvements:
Error handling: Provide more specific error messages to aid in debugging.
catch (error) { console.error("Error in getProjectsSortedByVouchOrFlag:", error); throw new Error(`Failed to fetch and sort projects: ${error.message}`); }Type safety: Consider mapping the raw query results to
ProjectsSortedByVouchOrFlagTypeobjects before returning.return rawProjects.map(project => ({ id: project.id, totalCount: parseInt(project.total_count, 10) }));Performance: If
ProjectsSortedByVouchOrFlagTypeincludes more fields than justidandtotalCount, consider fetching those fields in the SQL query to avoid N+1 query issues.To ensure the SQL query is correctly constructed for different sort orders, run the following verification script:
Summary by CodeRabbit
New Features
ProjectResolverfor handling project-related queries.OrganisationType,OrganisationProjectType,ProjectType, andProjectsSortedByVouchOrFlagType.Bug Fixes
Documentation
ProjectResolverthrough exports for easier integration.