feat(user): get a user's questions, answers and his followed questions #92
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Warning Rate Limit Exceeded@andylizf has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 3 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. WalkthroughThe recent updates encompass significant architectural improvements in a Q&A platform, focusing on enhancing user interactions and data flow. Changes include method parameter reordering, Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (20)
- src/answer/answer.controller.ts (2 hunks)
- src/answer/answer.module.ts (3 hunks)
- src/answer/answer.service.ts (8 hunks)
- src/attitude/attitude.module.ts (1 hunks)
- src/attitude/attitude.service.ts (2 hunks)
- src/common/pipe/snake-case-to-camel-case.pipe.spec.ts (1 hunks)
- src/common/pipe/snake-case-to-camel-case.pipe.ts (1 hunks)
- src/groups/groups.module.ts (1 hunks)
- src/questions/questions.controller.ts (3 hunks)
- src/questions/questions.module.ts (2 hunks)
- src/questions/questions.service.ts (9 hunks)
- src/users/DTO/get-answered-answers.dto.ts (1 hunks)
- src/users/DTO/get-asked-questions.dto.ts (1 hunks)
- src/users/DTO/get-followed-questions.dto.ts (1 hunks)
- src/users/users.controller.ts (6 hunks)
- src/users/users.module.ts (3 hunks)
- src/users/users.service.ts (8 hunks)
- src/users/users.spec.ts (1 hunks)
- test/answer.e2e-spec.ts (6 hunks)
- test/question.e2e-spec.ts (5 hunks)
Additional comments: 40
src/users/DTO/get-answered-answers.dto.ts (1)
- 1-10: The
GetAnsweredAnswersRespondDtois well-structured and follows best practices for DTO design in NestJS. Ensure that theAnswerDtoandPageRespondDtoare correctly implemented and consistently used across the application.src/users/DTO/get-asked-questions.dto.ts (1)
- 1-10: The
GetAskedQuestionsRespondDtois consistently structured and adheres to best practices for DTO design in NestJS. Ensure that theQuestionDtoandPageRespondDtoare correctly implemented and consistently used across the application.src/users/DTO/get-followed-questions.dto.ts (1)
- 1-10: The
GetFollowedQuestionsRespondDtois consistently structured and adheres to best practices for DTO design in NestJS. Ensure that theQuestionDtoandPageRespondDtoare correctly implemented and consistently used across the application.src/attitude/attitude.module.ts (1)
- 9-15: The use of
forwardRefto address circular dependencies with theUsersModuleis correctly implemented. Ensure to test the application thoroughly to verify that no unintended side effects have been introduced.src/groups/groups.module.ts (1)
- 36-36: The use of
forwardRefto address circular dependencies with theUsersModule,QuestionsModule, andAnswerModuleis correctly implemented. Ensure to test the application thoroughly to verify that no unintended side effects have been introduced.src/answer/answer.module.ts (1)
- 31-31: The removal of the
@Entity()decorator and the use offorwardReffor addressing circular dependencies are correctly implemented. Ensure to test the application thoroughly to verify that no unintended side effects have been introduced.src/common/pipe/snake-case-to-camel-case.pipe.ts (1)
- 1-39: The
SnakeCaseToCamelCasePipeis well-implemented, providing flexibility with optional prefix and suffix ignore patterns. Ensure to test this pipe thoroughly, especially in edge cases, and verify its usage across the application.src/users/users.module.ts (1)
- 46-47: The use of
forwardRefforAnswerModuleandQuestionsModule, and the addition ofUsersPermissionService, are correctly implemented. Ensure to test the integration ofUsersPermissionServicethoroughly and verify that no unintended side effects have been introduced.src/questions/questions.module.ts (4)
- 12-12: The addition of
AnswerModuleis appropriate for enabling interactions between questions and answers, aligning with the PR's objectives.- 43-43: Utilizing
forwardRefforUsersModuleis a suitable approach to resolve circular dependencies, aligning with the PR's refactoring goals.- 45-46: The application of
forwardReffor bothAttitudeModuleandGroupsModuleeffectively addresses circular dependencies, enhancing the codebase's maintainability.- 47-47: Incorporating
AnswerModulewithforwardRefis a prudent measure to tackle circular dependencies, supporting the PR's aim for a more maintainable codebase.src/common/pipe/snake-case-to-camel-case.pipe.spec.ts (1)
- 1-52: The tests for
SnakeCaseToCamelCasePipeprovide comprehensive coverage, ensuring the utility behaves as expected across a variety of scenarios. This is a good practice for maintaining the robustness of utility functions.src/attitude/attitude.service.ts (1)
- 25-25: The use of
InjectwithforwardRefforUsersServiceis a standard and effective approach to resolve circular dependencies in NestJS, aligning with the PR's objectives of improving code maintainability.src/answer/answer.controller.ts (2)
- 63-63: Moving the
userAgentparameter for consistency or method signature requirements is sensible. Ensure all calls to this method are updated accordingly to prevent runtime errors.Verification successful
To ensure a thorough verification, let's broaden the search to include TypeScript files, as the initial search might have missed occurrences due to file type restrictions.
The search results confirm that the
getQuestionAnswersmethod's parameter order change has been correctly implemented in its calls within the codebase. This aligns with the expectations set by the review comment, ensuring consistency and preventing runtime errors due to parameter misalignment.* 123-123: Adjusting the `userAgent` parameter order is a good practice for consistency. Verify that all method calls are updated to match the new order to avoid runtime issues.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all calls to getQuestionAnswers have been updated to reflect the new parameter order. rg "getQuestionAnswers\(" --type jsLength of output: 37
Script:
#!/bin/bash # Broaden the search to include TypeScript files to verify if all calls to getQuestionAnswers have been updated. rg "getQuestionAnswers\(" --type tsLength of output: 259
Verification successful
--- <!-- [task review_new_context_2/2] Analyze the updated context provided by the shell script execution. [observations] The updated script searched for calls to `getAnswerDetail(` in TypeScript files and found only the declaration of the method itself in `src/answer/answer.controller.ts`. [analysis] The fact that only the method declaration was found and no calls to this method were detected suggests that either the method is not being called anywhere within the TypeScript files of the project, or the method calls are dynamically constructed in a way that the search could not detect. [actions] Given the information at hand, it's reasonable to conclude that there are no statically detectable calls to `getAnswerDetail` that need updating in the TypeScript files. This does not rule out the possibility of dynamic calls or calls from files in other languages or frameworks that were not searched. However, based on the evidence, the specific concern about updating calls to match the new parameter order in TypeScript files can be considered addressed. --> Based on the search results, there are no statically detectable calls to `getAnswerDetail` in TypeScript files that need updating to match the new parameter order. Ensure to consider the possibility of dynamic calls or calls from other languages or frameworks not covered by this search. </details> <details> <summary>Scripts Executed</summary> The following scripts were executed for the analysis: --- Script: ```shell #!/bin/bash # Verify that all calls to getAnswerDetail have been updated to reflect the new parameter order. rg "getAnswerDetail\(" --type jsLength of output: 34
Script:
#!/bin/bash # Broaden the search to include TypeScript files and not specify the file type, to ensure all calls to getAnswerDetail are found. rg "getAnswerDetail\(" --type tsLength of output: 91
src/questions/questions.controller.ts (3)
- 36-36: The import of
SnakeCaseToCamelCasePipeis appropriate for its intended use in transforming query parameters, supporting the PR's objectives.- 326-326: Modifying the
sortquery parameter to useSnakeCaseToCamelCasePipeenhances API usability by allowing consistent parameter formatting. This is a thoughtful improvement.- 344-344: Changing the response message to 'OK' in various endpoints is a good practice for consistency across the API, improving the developer experience.
src/users/users.controller.ts (4)
- 16-16: The use of
@InjectwithforwardRef()for injectingAnswerServiceandQuestionsServiceis a valid approach to resolve circular dependencies in NestJS. However, it's beneficial to periodically review the application's architecture to see if there are opportunities to minimize these dependencies for a cleaner design.Also applies to: 27-27, 30-43
- 451-561: The
getUserAskedQuestionsmethod is well-implemented with proper pagination and error handling. Ensure that thequestionsService.getUserAskedQuestionsmethod has appropriate handling for cases where the user ID does not exist, potentially returning an empty list or a specific error message.- 488-524: The
getUserAnsweredAnswersmethod follows best practices for pagination and error handling. As withgetUserAskedQuestions, ensure that theanswerService.getUserAnsweredAnswersAcrossQuestionsmethod appropriately handles cases where the user ID does not exist.- 526-561: The
getFollowedQuestionsmethod is implemented consistently with the other user-related methods, including proper pagination and error handling. Ensure that thequestionsService.getFollowedQuestionsmethod has appropriate handling for cases where the user ID does not exist.src/answer/answer.service.ts (3)
- 42-54: Injecting services using
@InjectwithforwardRef()and making service propertiesreadonlyare good practices for handling circular dependencies and ensuring immutability. However, consider reviewing the application's architecture to minimize these dependencies for a cleaner design.- 159-208: The
getUserAnsweredAnswersAcrossQuestionsmethod is well-implemented with proper pagination and user existence check. Ensure that there's clear documentation or comments on how non-existent users are handled, potentially returning an empty list or a specific error message.- 444-507: The
getFavoriteAnswersmethod follows best practices for pagination and user existence check. As withgetUserAnsweredAnswersAcrossQuestions, ensure that there's clear documentation or comments on how non-existent users are handled.src/users/users.service.ts (1)
- 70-73: The use of
@Inject(forwardRef(() => AnswerService))and@Inject(forwardRef(() => QuestionsService))is a good practice to resolve circular dependencies. However, ensure that the circular dependency cannot be eliminated through architectural changes, as resolving circular dependencies at the root can lead to a more maintainable codebase.src/questions/questions.service.ts (7)
- 19-19: The import of
AnswerServiceis correctly added to support the new functionalities related to answers. Ensure thatAnswerServiceis properly implemented and tested, as it's now a dependency ofQuestionsService.- 69-70: The injection of
AnswerServiceusingforwardRef()is appropriate for handling circular dependencies. This change aligns with the PR objectives of improving service injection and addressing circular dependencies. Ensure that all services injected withforwardRef()are correctly handled in their respective modules.- 258-258: The method
getAnswerIdOfCreatedBybeing moved toAnswerServiceand called here is a good example of separating concerns and enhancing modularity. This change should improve maintainability and testability of the code related to answer management. Verify that the method is correctly implemented inAnswerServiceand covered by tests.- 533-586: The method
getFollowedQuestionsis a significant addition, enabling users to retrieve questions they have followed. This method appears to be well-implemented, with proper pagination and error handling. However, consider optimizing the database queries to minimize the number of calls, especially in the pagination logic, to enhance performance.Consider optimizing the database queries within
getFollowedQuestionsto reduce the number of calls and improve performance, especially for pagination.
- 663-663: The use of
getAnswerIdOfCreatedByininviteUsersToAnswerQuestionto check if a user has already answered a question before inviting them is a good practice. It prevents redundant invitations and ensures data integrity. Ensure that the error handling for already answered and already invited scenarios is user-friendly and informative.- 882-885: The method
isQuestionAnsweredBycorrectly utilizesgetAnswerIdOfCreatedByto determine if a question has been answered by a specific user. This is a clean way to encapsulate the logic withinAnswerService. Ensure that this method is used consistently across the service where similar checks are needed.- 906-962: The methods
getQuestionCountandgetUserAskedQuestionsare new additions that align with the PR objectives of enhancing user functionalities. These methods are well-structured and include pagination and error handling. However, ensure that the database queries are optimized for performance, especially when dealing with large datasets.Review and optimize the database queries in
getQuestionCountandgetUserAskedQuestionsfor performance, particularly for large datasets.test/answer.e2e-spec.ts (5)
- 30-30: Adding a new array
auxUserAskedAnswerIdsto track answers asked by auxiliary users is a good approach for testing purposes. However, ensure that this array is properly reset or managed between tests to avoid test data leakage.- 192-199: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [182-196]
The modification of the
createAnswerfunction to accept auserIdparameter and updateauxUserAskedAnswerIdsaccordingly is a significant change. It's important to ensure that all calls to this function have been updated to include the new parameter. Additionally, consider adding a check to ensureuserIdis valid to prevent potential issues.Verification successful
The verification process confirms that all calls to
createAnswerintest/answer.e2e-spec.tshave been updated to include the newuserIdparameter, as per the function's updated signature. This addresses the concern raised in the review comment.* 208-213: The loop to create answers for the first five questions is correctly updated to include the `userId` and `auxToken` parameters. This ensures that the answers are associated with the auxiliary user, which is crucial for the tests that follow. * 229-229: The loop for creating answers for the sixth question and updating `specialAnswerIds` is correctly implemented. It uses the `userIdTokenPairList` to create answers from different users, which is useful for testing various scenarios. * 509-583: The addition of tests under the 'Get Answers By Asker ID' section is a valuable enhancement to the test suite. These tests verify the functionality of retrieving answers asked by a specific user. It's important to ensure that the pagination logic (e.g., `page_start`, `page_size`, `has_prev`, `has_more`) is correctly implemented and tested in the application code to match the expectations set in these tests.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all calls to createAnswer include the new userId parameter. ast-grep --lang typescript --pattern $'createAnswer($_, $_, $_, $_)'Length of output: 751
test/question.e2e-spec.ts (1)
- 606-648: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [393-646]
The
search questiondescribe block tests the functionality of searching questions with various parameters. It's important to also test the accuracy of the search results, especially when using filters likeqfor query strings. Consider adding assertions to verify that the returned questions match the search criteria.Would you like assistance in adding more detailed assertions to these tests?
andylizf
requested changes
Mar 21, 2024
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- src/questions/DTO/question-invitation.dto.ts (1 hunks)
- src/questions/questions.service.ts (11 hunks)
- test/question.e2e-spec.ts (6 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/questions/questions.service.ts
- test/question.e2e-spec.ts
Additional comments: 1
src/questions/DTO/question-invitation.dto.ts (1)
- 5-9: The changes from camelCase to snake_case in property names (
questionIdtoquestion_id,createdAttocreated_at,updatedAttoupdated_at, andisAnsweredtois_answered) are noted. Ensure these changes are consistent with the project's coding standards and consider the following:
- Verify that serialization/deserialization processes are correctly handled with the new property names.
- Check for potential impacts on other parts of the codebase where these DTO properties are referenced, to ensure that all necessary updates are made.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- src/answer/answer.service.ts (8 hunks)
- src/questions/questions.service.ts (11 hunks)
- src/users/users.service.ts (8 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/answer/answer.service.ts
- src/questions/questions.service.ts
- src/users/users.service.ts
Nictheboy
force-pushed
the
patch-user
branch
2 times, most recently
from
51777d4 to
e5b6586
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- src/answer/answer.service.ts (8 hunks)
- src/questions/questions.service.ts (11 hunks)
- src/users/users.service.ts (8 hunks)
- src/users/users.spec.ts (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- src/answer/answer.service.ts
- src/questions/questions.service.ts
- src/users/users.service.ts
- src/users/users.spec.ts
andylizf
approved these changes
Mar 22, 2024
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (8)
- src/answer/answer.controller.ts (2 hunks)
- src/answer/answer.service.ts (8 hunks)
- src/questions/questions.controller.ts (3 hunks)
- src/questions/questions.service.ts (11 hunks)
- src/users/users.controller.ts (6 hunks)
- src/users/users.service.ts (8 hunks)
- src/users/users.spec.ts (1 hunks)
- test/question.e2e-spec.ts (6 hunks)
Files skipped from review as they are similar to previous changes (6)
- src/answer/answer.controller.ts
- src/answer/answer.service.ts
- src/questions/questions.controller.ts
- src/questions/questions.service.ts
- src/users/users.service.ts
- src/users/users.spec.ts
Additional comments: 8
src/users/users.controller.ts (1)
- 25-48: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [16-79]
The use of
forwardRefforAnswerServiceandQuestionsServiceinjections is appropriate for resolving circular dependencies, and the injections are correctly implemented. Good job addressing the circular dependency issue as mentioned in the PR objectives.test/question.e2e-spec.ts (7)
- 169-176: Consider parallelizing the creation of questions using
Promise.allto improve test execution time. However, ensure that the sequential order ofquestionIdsis not a requirement for your tests.- 307-414: The tests for getting questions asked by a user are comprehensive, covering various scenarios including error handling and pagination. Ensure that the pagination logic aligns with the application's requirements and that edge cases are adequately tested.
- 630-653: Adding tests for following multiple questions enhances the coverage for the follow functionality. This is a good practice as it ensures the feature behaves correctly under different scenarios.
- 655-672: The test for getting followed questions assumes a single followed question. Consider expanding this test to cover scenarios where a user follows multiple questions, to ensure correct pagination and data retrieval.
- 1094-1236: The tests for invitations are well-structured and cover a range of scenarios including successful invitations, error handling, and pagination. Ensure that the pagination tests accurately reflect the expected behavior, especially in scenarios with a large number of invitations.
- 1311-1311: Ensure that the test for getting invitation details correctly verifies all relevant fields in the response. This includes checking the
is_answeredfield if applicable, to ensure that the invitation status is accurately represented.Verification successful
The verification process confirms that the
is_answeredfield is correctly tested in the invitation details test, as it checks for the field's existence and verifies its data type as boolean. This aligns with the intent of the review comment to ensure comprehensive testing of the invitation details, including the invitation status.* 307-414: Overall, the tests for user interactions with questions (asking, following, and inviting to answer) are comprehensive. Consider reviewing the consistency of error messages and status codes across these tests to ensure a uniform user experience.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the `is_answered` field is correctly tested in the invitation details test. grep -C3 'is_answered' test/question.e2e-spec.tsLength of output: 482
Also applies to: 655-672, 1094-1236
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (21)
- src/answer/answer.controller.ts (2 hunks)
- src/answer/answer.module.ts (3 hunks)
- src/answer/answer.service.ts (8 hunks)
- src/attitude/attitude.module.ts (1 hunks)
- src/attitude/attitude.service.ts (2 hunks)
- src/common/pipe/snake-case-to-camel-case.pipe.spec.ts (1 hunks)
- src/common/pipe/snake-case-to-camel-case.pipe.ts (1 hunks)
- src/groups/groups.module.ts (1 hunks)
- src/questions/DTO/question-invitation.dto.ts (1 hunks)
- src/questions/questions.controller.ts (3 hunks)
- src/questions/questions.module.ts (2 hunks)
- src/questions/questions.service.ts (11 hunks)
- src/users/DTO/get-answered-answers.dto.ts (1 hunks)
- src/users/DTO/get-asked-questions.dto.ts (1 hunks)
- src/users/DTO/get-followed-questions.dto.ts (1 hunks)
- src/users/users.controller.ts (6 hunks)
- src/users/users.module.ts (3 hunks)
- src/users/users.service.ts (8 hunks)
- src/users/users.spec.ts (1 hunks)
- test/answer.e2e-spec.ts (6 hunks)
- test/question.e2e-spec.ts (6 hunks)
Files skipped from review as they are similar to previous changes (19)
- src/answer/answer.controller.ts
- src/answer/answer.module.ts
- src/answer/answer.service.ts
- src/attitude/attitude.module.ts
- src/attitude/attitude.service.ts
- src/common/pipe/snake-case-to-camel-case.pipe.spec.ts
- src/common/pipe/snake-case-to-camel-case.pipe.ts
- src/groups/groups.module.ts
- src/questions/DTO/question-invitation.dto.ts
- src/questions/questions.controller.ts
- src/questions/questions.module.ts
- src/users/DTO/get-answered-answers.dto.ts
- src/users/DTO/get-asked-questions.dto.ts
- src/users/DTO/get-followed-questions.dto.ts
- src/users/users.module.ts
- src/users/users.service.ts
- src/users/users.spec.ts
- test/answer.e2e-spec.ts
- test/question.e2e-spec.ts
Additional comments: 7
src/users/users.controller.ts (4)
- 30-36: The imports for
AnswerServiceandQuestionsServiceare correctly added to support the new functionalities related to user interactions with questions and answers. This aligns with the PR objectives to enhance user experience by enabling them to retrieve their asked questions, answers, and followed questions.- 73-76: The injection of
AnswerServiceandQuestionsServiceusing@Inject(forwardRef(() => ...))is a good practice to resolve circular dependency issues. This approach enhances the modularity and maintainability of the codebase, aligning with the PR's objectives for structural improvements.- 381-383: Updating method signatures for pagination parameters in
getFollowersandgetFolloweeswith optionalParseIntPipeis a good practice for ensuring that the input is correctly parsed as an integer. This change improves the robustness of the pagination feature.Also applies to: 418-420
- 451-486: The methods
getUserAskedQuestions,getUserAnsweredAnswers, andgetFollowedQuestionsimplement the new functionalities as described in the PR objectives. Each method correctly handles pagination and viewer ID retrieval. However, as previously suggested and declined, extracting common logic such as pagination handling and viewer ID retrieval into reusable private methods would have enhanced code maintainability and readability. Since the suggestion was previously declined, we'll respect the decision and not insist on this change.Also applies to: 488-524, 526-561
src/questions/questions.service.ts (3)
- 69-70: The injection of
AnswerServiceusingforwardRef()is a good practice to resolve circular dependencies. However, ensure that the circular dependency is unavoidable and consider restructuring the code to eliminate it if possible.- 663-663: It's good to see error handling for cases where the user has already answered or been invited to answer a question. This prevents redundant data and ensures the integrity of invitations. Keep up with this practice of thorough error checking in all relevant methods.
- 882-885: The method
isQuestionAnsweredBycorrectly checks if a question has been answered by a specific user. This is a crucial check for maintaining the integrity of user interactions with questions. However, consider adding a comment explaining the return statement for clarity, especially for developers unfamiliar with the codebase.
Comment on lines
+533
to
+586
| async getFollowedQuestions( | ||
| followerId: number, | ||
| firstQuestionId: number | undefined, // if from start | ||
| pageSize: number, | ||
| viewerId?: number, // optional | ||
| ip?: string, // optional | ||
| userAgent?: string, // optional | ||
| ): Promise<[QuestionDto[], PageRespondDto]> { | ||
| if ((await this.userService.isUserExists(followerId)) == false) | ||
| throw new UserIdNotFoundError(followerId); | ||
| if (firstQuestionId == undefined) { | ||
| const relations = await this.questionFollowRelationRepository.find({ | ||
| where: { followerId }, | ||
| take: pageSize + 1, | ||
| order: { questionId: 'ASC' }, | ||
| }); | ||
| const DTOs = await Promise.all( | ||
| relations.map((r) => { | ||
| return this.getQuestionDto(r.questionId, viewerId, ip, userAgent); | ||
| }), | ||
| ); | ||
| return PageHelper.PageStart(DTOs, pageSize, (item) => item.id); | ||
| } else { | ||
| const prevPromise = this.questionFollowRelationRepository.find({ | ||
| where: { | ||
| followerId, | ||
| questionId: LessThan(firstQuestionId), | ||
| }, | ||
| take: pageSize, | ||
| order: { questionId: 'DESC' }, | ||
| }); | ||
| const currPromise = this.questionFollowRelationRepository.find({ | ||
| where: { | ||
| followerId, | ||
| questionId: MoreThanOrEqual(firstQuestionId), | ||
| }, | ||
| take: pageSize + 1, | ||
| order: { questionId: 'ASC' }, | ||
| }); | ||
| const [prev, curr] = await Promise.all([prevPromise, currPromise]); | ||
| const currDTOs = await Promise.all( | ||
| curr.map((record) => | ||
| this.getQuestionDto(record.questionId, viewerId, ip, userAgent), | ||
| ), | ||
| ); | ||
| return PageHelper.PageMiddle( | ||
| prev, | ||
| currDTOs, | ||
| pageSize, | ||
| (i) => i.questionId, | ||
| (i) => i.id, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
The method getFollowedQuestions is well-implemented with clear logic for pagination. However, consider abstracting the pagination logic into a reusable component or service to reduce code duplication across different methods that require similar pagination functionality.
src/questions/questions.service.ts
Outdated
Comment on lines
906
to
961
| async getQuestionCount(userId: number | undefined): Promise<number> { | ||
| if (userId == undefined) return 0; | ||
| return await this.questionRepository.countBy({ createdById: userId }); | ||
| } | ||
|
|
||
| async getUserAskedQuestions( | ||
| userId: number, | ||
| pageStart: number | undefined, | ||
| pageSize: number, | ||
| viewerId?: number, | ||
| userAgent?: string, | ||
| ip?: string, | ||
| ): Promise<[QuestionDto[], PageRespondDto]> { | ||
| if ((await this.userService.isUserExists(userId)) == false) | ||
| throw new UserIdNotFoundError(userId); | ||
| if (!pageStart) { | ||
| const currPage = await this.questionRepository.find({ | ||
| where: { createdById: userId }, | ||
| order: { id: 'ASC' }, | ||
| take: pageSize + 1, | ||
| }); | ||
| const currDto = await Promise.all( | ||
| currPage.map(async (entity) => { | ||
| return this.getQuestionDto(entity.id, viewerId, userAgent, ip); | ||
| }), | ||
| ); | ||
| return PageHelper.PageStart(currDto, pageSize, (answer) => answer.id); | ||
| } else { | ||
| const prevPage = await this.questionRepository.find({ | ||
| where: { | ||
| createdById: userId, | ||
| id: LessThan(pageStart), | ||
| }, | ||
| order: { id: 'DESC' }, | ||
| take: pageSize, | ||
| }); | ||
| const currPage = await this.questionRepository.find({ | ||
| where: { | ||
| createdById: userId, | ||
| id: MoreThanOrEqual(pageStart), | ||
| }, | ||
| order: { id: 'ASC' }, | ||
| take: pageSize + 1, | ||
| }); | ||
| const currDto = await Promise.all( | ||
| currPage.map(async (entity) => { | ||
| return this.getQuestionDto(entity.id, viewerId, userAgent, ip); | ||
| }), | ||
| ); | ||
| return PageHelper.PageMiddle( | ||
| prevPage, | ||
| currDto, | ||
| pageSize, | ||
| (answer) => answer.id, | ||
| (answer) => answer.id, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The method getUserAskedQuestions effectively retrieves questions asked by a specific user with pagination support. As with getFollowedQuestions, consider abstracting the pagination logic to reduce code duplication and improve maintainability.
andylizf
force-pushed
the
patch-user
branch
2 times, most recently
from
0c60313 to
bf22a8a
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (7)
design/API.ymlis excluded by:!**/*.ymldocker-compose-prod.ymlis excluded by:!**/*.ymldocker-compose.dev.ymlis excluded by:!**/*.ymldocker-compose.ymlis excluded by:!**/*.ymlpackage.jsonis excluded by:!**/*.jsonpnpm-lock.yamlis excluded by:!**/*.yamlprisma/migrations/migration_lock.tomlis excluded by:!**/*.toml
Files selected for processing (21)
- .github/dependabot.yml (1 hunks)
- .github/workflows/build-docker-prod.yml (1 hunks)
- .github/workflows/build-test-docker-dev.yml (2 hunks)
- .github/workflows/test-coverage.yml (1 hunks)
- .github/workflows/test.yml (1 hunks)
- Dockerfile (2 hunks)
- prisma/migrations/20240321040602_init/migration.sql (1 hunks)
- prisma/schema.prisma (1 hunks)
- src/answer/answer.controller.ts (4 hunks)
- src/answer/answer.service.ts (8 hunks)
- src/avatars/avatars.controller.ts (1 hunks)
- src/comments/comment.controller.ts (2 hunks)
- src/common/interceptor/token-validate.interceptor.ts (1 hunks)
- src/groups/groups.controller.ts (2 hunks)
- src/questions/questions.controller.ts (6 hunks)
- src/questions/questions.service.ts (11 hunks)
- src/topics/topics.controller.ts (1 hunks)
- src/users/users.controller.ts (6 hunks)
- src/users/users.service.ts (8 hunks)
- src/users/users.spec.ts (1 hunks)
- test/question.e2e-spec.ts (8 hunks)
Files skipped from review as they are similar to previous changes (6)
- src/answer/answer.controller.ts
- src/answer/answer.service.ts
- src/questions/questions.controller.ts
- src/users/users.service.ts
- src/users/users.spec.ts
- test/question.e2e-spec.ts
Additional comments: 26
.github/dependabot.yml (1)
- 1-16: The Dependabot configuration looks good and follows best practices for keeping dependencies up to date on a weekly basis.
Dockerfile (2)
- 1-4: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-8]
Updating to
node:21and introducingPNPM_HOMEwith corepack enabled are good practices for leveraging the latest Node.js features and improving package management.
- 39-39: The addition of
opensslin the production image is justified for SSL/TLS functionality. Ensure it's necessary for your application to keep the image size and attack surface minimal.Verification successful
The addition of
opensslin the Docker image is justified and necessary for the application's database interactions, as indicated by its direct usage in theprisma/schema.prismafile for secure connections. This confirms the necessity ofopensslfor the application's functionality.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if openssl is directly used by the application. This is a placeholder for actual verification steps, which might include checking the application's documentation or source code. echo "Check application documentation or source code for direct usage of openssl."Length of output: 160
Script:
#!/bin/bash # Search for direct mentions of openssl in the source code and configuration files. rg "openssl" --hidden --ignore-caseLength of output: 183
src/common/interceptor/token-validate.interceptor.ts (1)
- 38-43: The interceptor correctly verifies the access token if present. Consider adding error handling for cases where token verification fails, to ensure a clear response is sent back to the client instead of an unhandled exception.
src/avatars/avatars.controller.ts (1)
- 9-27: The addition of
UseFilters,UsePipes,ValidationPipe,BaseErrorExceptionFilter, andTokenValidateInterceptordecorators enhances request handling, validation, and error management in line with NestJS best practices.src/topics/topics.controller.ts (1)
- 21-36: The addition of
TokenValidateInterceptorand the adjustments in the usage ofUsePipesandUseFiltersdecorators enhance the functionality and robustness of theTopicsController. These changes align with NestJS best practices for handling requests, validation, and error management..github/workflows/test-coverage.yml (1)
- 80-88: Updating the versions of GitHub Actions used for repository checkout, Node.js setup, and pnpm installation to v4 is a good practice, ensuring access to the latest features and security patches.
.github/workflows/test.yml (3)
- 80-80: The update to
actions/checkout@v4is a good practice for ensuring the workflow benefits from the latest features and security patches.- 83-83: Updating
actions/setup-node@v4ensures the workflow uses the most recent features and security improvements for Node.js setup.- 88-88: The update to
pnpm/action-setup@v3ensures compatibility and access to the latest features for pnpm within the workflow.src/comments/comment.controller.ts (2)
- 22-22: Adding
TokenValidateInterceptorensures that all routes within theCommentsControllerare protected by token validation, enhancing security.- 13-13: The organization and updates to the import statements, including the addition of
TokenValidateInterceptor, are appropriate for the controller's functionality.src/groups/groups.controller.ts (1)
- 30-30: Integrating
TokenValidateInterceptorin theGroupsControllerensures that all endpoints are subject to token validation, which is crucial for protecting routes that require user authentication.src/users/users.controller.ts (3)
- 31-45: The addition of
AnswerService,QuestionsService, and related DTOs is necessary for the new functionalities introduced in this controller, enabling it to handle user-related questions and answers effectively.- 76-79: The injection of
AnswerServiceandQuestionsServiceusing@Inject(forwardRef(() => Service))resolves circular dependency issues and allows theUsersControllerto utilize these services for the new endpoints.- 381-389: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [384-423]
The updated method signatures for
getFollowersandgetFolloweeswith optional pagination parameters follow best practices for APIs, improving usability and performance.prisma/schema.prisma (1)
- 12-12: The addition of
binaryTargetswith Debian OpenSSL versions enhances compatibility with different deployment environments. Ensure that your deployment environment(s) use or are compatible with these OpenSSL versions.src/questions/questions.service.ts (5)
- 69-70: The injection of
AnswerServiceusingforwardRef()is correctly implemented to resolve the circular dependency issue. This is a good practice in NestJS when dealing with modules that are interdependent.- 533-586: The implementation of
getFollowedQuestionsmethod correctly handles pagination and retrieves questions followed by a user. As previously suggested, consider abstracting the pagination logic into a reusable component or service to enhance code maintainability and reduce duplication.- 910-962: The
getUserAskedQuestionsmethod effectively retrieves questions asked by a specific user with pagination support. As withgetFollowedQuestions, abstracting the pagination logic into a reusable component or service would reduce code duplication and improve maintainability.- 906-908: The
getQuestionCountmethod is straightforward and efficiently retrieves the count of questions created by a user. This method follows best practices for simplicity and performance.- 882-885: The
isQuestionAnsweredBymethod correctly checks if a question has been answered by a specific user. The use ofgetAnswerIdOfCreatedByfromAnswerServiceto determine if an answer exists is a good example of leveraging service layer abstraction.prisma/migrations/20240321040602_init/migration.sql (4)
- 1-11: The creation of enums for
attitudable_type,attitude_type,attitude_type_not_undefined, andcomment_commentabletype_enumis clear and follows a consistent naming convention. However, ensure that the removal of 'UNDEFINED' fromattitude_typein the creation ofattitude_type_not_undefinedis intentional and aligns with the application logic.- 14-273: The table creation statements are well-structured, with appropriate data types and default values. Pay special attention to the use of
TIMESTAMP(6)for date columns and ensure this precision is required for the application's functionality. Also, consider adding comments to the SQL file to describe the purpose of each table, enhancing maintainability.- 452-621: The creation of indexes is comprehensive, covering a wide range of fields across different tables. This should significantly improve query performance. However, review the necessity of each index to ensure it's justified by the query patterns used by the application, as excessive indexing can slow down write operations.
- 629-770: The foreign key constraints are correctly defined, ensuring referential integrity across the database. It's good practice to specify the
ON DELETEandON UPDATEactions explicitly, as done here. However, ensure that the chosen actions (NO ACTION,CASCADE,SET NULL,RESTRICT) align with the application's data management and integrity requirements.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By the way, I have added tests for 'GET /questions/{id}/invitations' and fixed a small bug in it.
Summary by CodeRabbit
forwardReffor service injections to enhance modularity.QuestionsControllerfor clarity.