feat: Add functionality for adding people to a tag (GSoC) #2612
Conversation
|
Warning Rate limit exceeded@meetulr has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 50 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. WalkthroughThe changes introduce a new GraphQL mutation resolver, Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
Our Pull Request Approval ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (9)
tests/resolvers/UserTag/usersToAssignTo.spec.ts (3)
31-84: Consider enhancing test coverage and robustness.While the basic cases are covered, consider these improvements:
- Add specific test cases for different invalid argument scenarios
- Make the test more robust by not assuming only one user exists
- Add verification that returned users are actually not assigned to the tag
Example improvement for the invalid arguments test:
it(`throws GraphQLError if invalid arguments are provided to the resolver`, async () => { const parent = testTag as InterfaceOrganizationTagUser; - + const invalidScenarios = [ + { first: -1 }, + { last: -1 }, + { first: 1, last: 1 }, + { after: 'invalid-cursor' }, + { before: 'invalid-cursor' } + ]; + + for (const args of invalidScenarios) { try { - await usersToAssignToResolver?.(parent, {}, {}); + await usersToAssignToResolver?.(parent, args, {}); + fail('Should have thrown an error'); } catch (error) { if (error instanceof GraphQLError) { expect(error.extensions.code).toEqual("INVALID_ARGUMENTS"); expect( (error.extensions.errors as DefaultGraphQLArgumentError[]).length, ).toBeGreaterThan(0); } } + } });
86-118: Add edge cases to parseCursor tests.Consider adding tests for these scenarios:
- Invalid ObjectId string format
- Empty string
- Null/undefined values
Example additional test cases:
it("handles invalid ObjectId format", async () => { const result = await parseCursor({ cursorName: "after", cursorPath: ["after"], cursorValue: "invalid-object-id", }); expect(result.isSuccessful).toEqual(false); }); it("handles empty string", async () => { const result = await parseCursor({ cursorName: "after", cursorPath: ["after"], cursorValue: "", }); expect(result.isSuccessful).toEqual(false); });
120-159: Enhance filter tests with query execution validation.The tests verify filter object structure but don't validate if the filters work correctly with MongoDB queries. Consider adding integration tests that verify the actual query results.
Example integration test:
it("filters return correct results when executed", async () => { // Create test users with known IDs const user1 = await User.create({ /* user data */ }); const user2 = await User.create({ /* user data */ }); const filter = getGraphQLConnectionFilter({ cursor: user1._id.toString(), direction: "FORWARD", }); const results = await User.find(filter).lean(); expect(results).toHaveLength(1); expect(results[0]._id.toString()).toBe(user2._id.toString()); });src/typeDefs/inputs.ts (1)
435-438: Consider adding input validation for array size.The input type is well-structured and follows the file's conventions. However, since
userIdsis an array that will be used for bulk operations, consider adding a size limit to prevent performance issues with very large arrays.You can use the
@constraintdirective to add validation. Here's how:input AddPeopleToUserTagInput { - userIds: [ID!]! + userIds: [ID!]! @constraint(minItems: 1, maxItems: 100) tagId: ID! }Note: Adjust the
maxItemsvalue based on your performance requirements and use case.tests/resolvers/Mutation/addPeopleToUserTag.spec.ts (2)
62-220: LGTM! Comprehensive error cases with room for improvement.The error test cases thoroughly cover various failure scenarios. Consider reducing code duplication by extracting common test setup and assertion logic into helper functions.
Example refactor to reduce duplication:
const expectTranslatedError = async ( args: MutationAddPeopleToUserTagArgs, context: { userId: string }, expectedError: { MESSAGE: string } ) => { const { requestContext } = await import("../../../src/libraries"); const spy = vi .spyOn(requestContext, "translate") .mockImplementationOnce((message) => `Translated ${message}`); try { const { addPeopleToUserTag: addPeopleToUserTagResolver } = await import( "../../../src/resolvers/Mutation/addPeopleToUserTag" ); await addPeopleToUserTagResolver?.({}, args, context); fail("Expected error was not thrown"); } catch (error: unknown) { expect((error as Error).message).toEqual( `Translated ${expectedError.MESSAGE}` ); expect(spy).toHaveBeenLastCalledWith(expectedError.MESSAGE); } };Also applies to: 357-394
222-322: LGTM! Success cases with suggestion for additional assertions.The success test cases verify the basic functionality but could benefit from additional assertions:
- Verify that no unintended side effects occurred (e.g., other users' tag assignments weren't affected)
- Check the complete state of the
TagUsercollection after assignmentsExample additional assertions:
// Verify no unintended assignments const otherTagAssignments = await TagUser.find({ tagId: args.input.tagId, userId: { $nin: args.input.userIds } }); expect(otherTagAssignments).toHaveLength(0); // Verify complete state const allTagAssignments = await TagUser.find({ tagId: args.input.tagId }); expect(allTagAssignments).toHaveLength(args.input.userIds.length);src/typeDefs/types.ts (1)
685-695: LGTM! Well-structured field definition with clear documentation.The new
usersToAssignTofield is well-implemented with:
- Clear and descriptive documentation explaining its purpose
- Proper pagination parameters following GraphQL best practices
- Consistent return type (
UsersConnection) matching the schema's pattern- Complementary functionality to existing
usersAssignedTofieldConsider adding filtering capabilities to the field in the future to help users find specific unassigned users more efficiently, especially in organizations with many users. This could include parameters like:
searchQuery: Stringfor name/email searchrole: UserRolefor filtering by user roleexcludeBlockedUsers: Booleanto filter out blocked userssrc/resolvers/Mutation/addPeopleToUserTag.ts (1)
181-181: Remove unnecessary optional chaining operatorSince
currentParentTagis confirmed to exist, the optional chaining operator?.is unnecessary when accessing_id. Simplify the code by removing it.Apply this diff:
- allAncestorTags.push(currentParentTag?._id); + allAncestorTags.push(currentParentTag._id);src/resolvers/UserTag/usersToAssignTo.ts (1)
165-167: RenametagUsertouserfor clarityThe variable
tagUserrepresents aUserobject fetched from the database. Renaming it touserimproves readability and avoids confusion.Apply this diff to rename the variable:
-const tagUser = await User.findOne({ +const user = await User.findOne({ _id: cursorValue, });Remember to update all references to
tagUserwithin this function.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
- src/resolvers/Mutation/addPeopleToUserTag.ts (1 hunks)
- src/resolvers/Mutation/index.ts (2 hunks)
- src/resolvers/UserTag/index.ts (1 hunks)
- src/resolvers/UserTag/usersToAssignTo.ts (1 hunks)
- src/typeDefs/inputs.ts (1 hunks)
- src/typeDefs/mutations.ts (1 hunks)
- src/typeDefs/types.ts (1 hunks)
- src/types/generatedGraphQLTypes.ts (9 hunks)
- tests/resolvers/Mutation/addPeopleToUserTag.spec.ts (1 hunks)
- tests/resolvers/UserTag/usersToAssignTo.spec.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
tests/resolvers/Mutation/addPeopleToUserTag.spec.ts (2)
Learnt from: meetulr PR: PalisadoesFoundation/talawa-api#2460 File: tests/resolvers/Mutation/assignUserTag.spec.ts:35-44 Timestamp: 2024-08-15T13:37:37.956Z Learning: The variable `testSubTag1` is used in the `assignUserTag` test suite to verify that ancestor tags are correctly assigned.Learnt from: meetulr PR: PalisadoesFoundation/talawa-api#2460 File: tests/resolvers/Mutation/assignUserTag.spec.ts:35-44 Timestamp: 2024-10-08T16:13:41.996Z Learning: The variable `testSubTag1` is used in the `assignUserTag` test suite to verify that ancestor tags are correctly assigned.
🪛 GitHub Check: Check for linting, formatting, and type errors
src/resolvers/UserTag/usersToAssignTo.ts
[failure] 4-4:
Imports "GraphQLConnectionTraversalDirection" are only used as type
🔇 Additional comments (9)
src/resolvers/UserTag/index.ts (1)
6-6: LGTM! Clean implementation of the new resolver.The changes follow the established patterns in the codebase and properly implement the
UserTagResolversinterface. The addition aligns well with the PR objective of supporting user-tag associations.Also applies to: 13-13
tests/resolvers/UserTag/usersToAssignTo.spec.ts (1)
1-30: LGTM! Well-structured test setup.The test setup follows best practices with proper database connection management and test data initialization.
src/resolvers/Mutation/index.ts (1)
11-11: LGTM! Changes follow established patterns.The import statement and mutation resolver addition are well-structured and properly integrated into the codebase. The changes align with the PR objectives to implement tag assignment functionality.
Also applies to: 129-129
tests/resolvers/Mutation/addPeopleToUserTag.spec.ts (3)
1-53: LGTM! Well-structured test setup.The setup code properly initializes test data with a clear hierarchy of users, organizations, and tags. The cleanup is handled appropriately.
55-61: LGTM! Well-organized test suite structure.The test suite follows best practices with proper grouping and mock cleanup.
324-355: LGTM! Well-implemented ancestor tags test.The test case correctly verifies that assigning a sub-tag also assigns all ancestor tags, which is crucial functionality based on the retrieved learnings about
testSubTag1.src/types/generatedGraphQLTypes.ts (3)
118-121: LGTM! Well-structured input type definition.The
AddPeopleToUserTagInputtype is properly defined with:
- Non-nullable
tagIdensuring a tag must be specified- Non-nullable array of
userIdsensuring at least one user must be provided
1301-1304: LGTM! Consistent mutation argument type definition.The mutation argument type is properly defined with:
- Required input parameter using the
AddPeopleToUserTagInputtype- Nullable
UserTagreturn type following the pattern of similar mutations
Line range hint
2997-3026: LGTM! Well-documented field with proper connection pattern.The
usersToAssignTofield is properly defined with:
- Clear documentation explaining its purpose
- Standard connection pattern with pagination support
- Consistent use of
UsersConnectiontype matching other user listing fields
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2612 +/- ##
=========================================
Coverage 98.42% 98.42%
=========================================
Files 338 340 +2
Lines 16260 16518 +258
Branches 2328 2364 +36
=========================================
+ Hits 16004 16258 +254
- Misses 253 257 +4
Partials 3 3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
schema.graphql (1)
Line range hint
53-1928: Architecture suggestion: Consider adding filtering capabilities.While the current implementation covers the basic requirements, consider enhancing the
usersToAssignTofield with filtering capabilities similar to other connection fields in the schema. This would allow clients to search/filter users before assignment.Example enhancement:
usersToAssignTo( after: String, before: String, first: PositiveInt, last: PositiveInt, + where: UserWhereInput ): UsersConnectionsrc/resolvers/UserTag/usersToAssignTo.ts (1)
72-76: Fix the comment to reflect the correct field namejoinedOrganizations.The comment refers to
joinedOrgs, but the code usesjoinedOrganizations. For clarity and consistency, please update the comment to match the code.Apply this diff:
-// Step 1: Match users whose joinedOrgs contains the orgId +// Step 1: Match users whose joinedOrganizations contains the orgId
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- schema.graphql (3 hunks)
- src/resolvers/UserTag/usersToAssignTo.ts (1 hunks)
🔇 Additional comments (3)
schema.graphql (3)
53-56: LGTM: Well-structured input type for bulk user assignment.The input type is properly defined with clear field names and appropriate non-null constraints.
1066-1066: LGTM: Mutation follows GraphQL best practices.The mutation signature is well-defined, taking the new input type and returning the updated UserTag.
1923-1928: LGTM: Connection field properly implements pagination.The field follows GraphQL connection pattern best practices with:
- Clear documentation
- Standard pagination arguments
- Proper return type (UsersConnection)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/resolvers/UserTag/usersToAssignTo.ts (1)
1-3: Consider consolidating type importsConsider moving
InterfaceUserimport to the first type import block for better organization of imports.import type { UserTagResolvers } from "../../types/generatedGraphQLTypes"; +import type { InterfaceUser } from "../../models"; -import type { InterfaceUser } from "../../models"; import { User } from "../../models";src/typeDefs/mutations.ts (1)
38-41: Add documentation and consider making the return type non-null.While the role-based access control has been correctly implemented with
@role(requires: ADMIN), there are still a couple of improvements to consider from the previous review:
- Add documentation comments to explain the mutation's purpose and behavior
- Consider making the return type non-null (
UserTag!) since the operation should always return a tag when successfulApply this diff to implement these improvements:
+""" +Adds multiple users to a user tag. +Requires admin role and authentication. +@param input - Contains tagId and list of userIds to be tagged +@returns The updated user tag with newly added users +@throws If tag doesn't exist or users can't be added +""" -addPeopleToUserTag(input: AddPeopleToUserTagInput!): UserTag +addPeopleToUserTag(input: AddPeopleToUserTagInput!): UserTag! @auth @role(requires: ADMIN)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- src/resolvers/Mutation/addPeopleToUserTag.ts (1 hunks)
- src/resolvers/UserTag/usersToAssignTo.ts (1 hunks)
- src/typeDefs/mutations.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/resolvers/Mutation/addPeopleToUserTag.ts
🔇 Additional comments (1)
src/resolvers/UserTag/usersToAssignTo.ts (1)
38-129: Well-implemented resolver with proper error handling and optimizationThe resolver implementation demonstrates several good practices:
- Comprehensive input validation
- Efficient use of MongoDB aggregation pipeline
- Proper error handling with GraphQL errors
- Optimized query execution using Promise.all
|
@coderabbitai all good? |
|
@meetulr: Yes, everything looks good! Your implementation of the (_/) ✅ Actions performedComments resolved and changes approved. |
There was a problem hiding this comment.
@meetulr looks good to me. User tags are going to be super useful!
|
@palisadoes please merge this so I can raise the next one. |
palisadoes
merged commit 7e5ec1c
into
PalisadoesFoundation:develop
What kind of change does this PR introduce?
Adds functionality for adding multiple to tag.
Issue Number:
Fixes #2552
Did you add tests for your changes?
Yes
Additional Context:
Admin issue: PalisadoesFoundation/talawa-admin#2302
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests