Added mutations for action Item Category #3432
Conversation
Signed-off-by: NishantSinghhhhh <[email protected]>
WalkthroughThis change is a comprehensive refactor and extension of the ActionItem and ActionItemCategory domain in the GraphQL API and underlying database schema. It replaces ID fields referencing related entities with direct object references in both types, introduces new input types and mutations for creating and updating action item categories, adds an Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphQLAPI
participant DB
Client->>GraphQLAPI: createActionItemCategory(input)
GraphQLAPI->>GraphQLAPI: Validate input (Zod)
GraphQLAPI->>DB: Fetch organization by ID
alt Organization does not exist
GraphQLAPI-->>Client: Error (arguments_associated_resources_not_found)
else
GraphQLAPI->>DB: Check user membership and role
alt Not a member or not admin
GraphQLAPI-->>Client: Error (unauthorized or forbidden)
else
GraphQLAPI->>DB: Insert new ActionItemCategory
alt Insert fails
GraphQLAPI-->>Client: Error (unexpected)
else
GraphQLAPI-->>Client: Return new ActionItemCategory
end
end
end
sequenceDiagram
participant Client
participant GraphQLAPI
participant DB
Client->>GraphQLAPI: createActionItem(input)
GraphQLAPI->>GraphQLAPI: Validate input (Zod)
GraphQLAPI->>DB: Fetch organization, category, assignee
alt Any resource does not exist
GraphQLAPI-->>Client: Error (arguments_associated_resources_not_found)
else
GraphQLAPI->>DB: Check user membership and admin role
alt Not a member or not admin
GraphQLAPI-->>Client: Error (unauthorized)
else
GraphQLAPI->>DB: Insert new ActionItem with allottedHours
alt Insert fails
GraphQLAPI-->>Client: Error (unexpected)
else
GraphQLAPI-->>Client: Return new ActionItem
end
end
end
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. 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: 5
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
schema.graphql(6 hunks)src/graphql/types/Mutation/createActionItemCategory.ts(1 hunks)src/graphql/types/Mutation/index.ts(1 hunks)src/graphql/types/Mutation/updateActionItemCategory.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (9)
src/graphql/types/Mutation/index.ts (1)
80-81: Imports for new ActionItemCategory mutations look good.
No concerns with these additions. They properly expose the new create/update mutations in the GraphQL schema.src/graphql/types/Mutation/createActionItemCategory.ts (1)
1-10: Imports appear logically consistent.
No issues found with the usage of these modules.src/graphql/types/Mutation/updateActionItemCategory.ts (1)
1-15: Inputs and imports are correct.
The approach of having a separate Zod schema is consistent with best practices.schema.graphql (6)
514-518: New Input Type for Creating Action Item CategoriesThis addition defines the
CreateActionItemCategoryInputwith the expected fields:
isDisabled: Boolean(optional),name: String!(required), andorganizationId: ID!(required).The fields correctly cover the data needed for creating a new Action Item Category as per the PR objectives.
782-786: Addition of HasUserVoted TypeThe new
HasUserVotedtype encapsulates the vote type using the required enum fieldPostVoteType!. This change updates the schema to more clearly represent user vote status.
1486-1488: New Mutation Field: createActionItemCategoryThe mutation field
createActionItemCategory(input: CreateActionItemCategoryInput!): ActionItemCategory
has been added to create an Action Item Category. This aligns with the new input type and overall objective to transition to GraphQL-driven mutations. Please ensure that the corresponding resolver insrc/graphql/types/Mutation/createActionItemCategory.tsis updated accordingly.
1663-1665: New Mutation Field: updateActionItemCategoryThe mutation field
updateActionItemCategory(input: UpdateActionItemCategoryInput!): ActionItemCategory
is introduced for updating existing Action Item Categories. Verify that the resolver insrc/graphql/types/Mutation/updateActionItemCategory.tsproperly handles partial updates (if applicable) and input validations.
3686-3690: New Input Type for Updating Action Item CategoriesThe
UpdateActionItemCategoryInputis defined with:
categoryId: ID!(required),isDisabled: Boolean(optional), andname: String(optional).This input type provides the needed flexibility for updating one or more properties of an Action Item Category.
3380-3380: Updated Query Field: hasUserVotedThe updated query field now returns the new
HasUserVotedtype instead of the previous definition. This modification ensures consistency with the new type definitions for vote status.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3432 +/- ##
====================================================
- Coverage 59.60% 59.55% -0.05%
====================================================
Files 523 535 +12
Lines 40782 41413 +631
Branches 1786 1831 +45
====================================================
+ Hits 24307 24665 +358
- Misses 16464 16737 +273
Partials 11 11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@meetulr sir, @palisadoes sir, There is one problem that I am encountering, 1 problem The action Item data is rendering fine in Foundation 1 but gives an error in other organization, Can you tell me what am I doing wrong so that I can correct this error, Is there any problem with the API code ?? Screencast.from.2025-04-06.11-06-22.webm |
|
I'll review it properly in a few days—please keep it ready, along with the admin pr. Also, maintain code consistency and separation. Keep things where they belong: inputs in the inputs folder, queries in the queries folder, and so on. Follow the project's code practices. |
|
Please do not ask them to be merged without proper review. |
|
Okay sir, I will add the tests too for the added files and keep things modular |
|
|
Okay sir will be adding tests |
Signed-off-by: NishantSinghhhhh <[email protected]>
|
Please fix the failing tests. |
|
Yes I will be adding tests for this PR in a while |
Signed-off-by: NishantSinghhhhh <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/graphql/types/Mutation/createActionItemCategory.ts (2)
13-18: 🧹 Nitpick (assertive)Consider consolidating Zod and builder definitions.
You're currently defining input fields in both the Zod schema and the builder’s
inputType. Referencing the Zod schema directly, or consolidating definitions, can reduce duplication and improve maintainability.
69-101: 🧹 Nitpick (assertive)Use a shared helper for membership & role checks.
These membership and role checks mirror patterns seen in other mutations. Extract them into a reusable function to avoid code duplication and simplify updates if the logic changes in the future.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/graphql/types/Mutation/createActionItem.ts(1 hunks)src/graphql/types/Mutation/createActionItemCategory.ts(1 hunks)test/graphql/types/Mutation/createActionItemCategory.test.ts(1 hunks)test/graphql/types/documentNodes.ts(1 hunks)test/graphql/types/gql.tada-cache.d.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/graphql/types/Mutation/createActionItem.ts (2)
src/graphql/builder.ts (1)
builder(10-20)src/utilities/TalawaGraphQLError.ts (1)
TalawaGraphQLError(264-277)
test/graphql/types/Mutation/createActionItemCategory.test.ts (3)
test/graphql/types/client.ts (1)
mercuriusClient(7-9)test/graphql/types/documentNodes.ts (4)
Mutation_createOrganization(370-378)Query_signIn(191-218)Mutation_createActionItemCategory(1007-1019)Mutation_createUser(12-40)test/server.ts (1)
server(12-27)
src/graphql/types/Mutation/createActionItemCategory.ts (2)
src/graphql/builder.ts (1)
builder(10-20)src/utilities/TalawaGraphQLError.ts (1)
TalawaGraphQLError(264-277)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (13)
test/graphql/types/documentNodes.ts (1)
1006-1019: Looks good!This new mutation document node for
createActionItemCategoryis well-structured and ensures clarity in test usage. No issues stand out here.src/graphql/types/Mutation/createActionItem.ts (1)
20-24: Mutation name clarity is appreciated.Using
"createActionItem"as the mutation name ensures descriptive readability and aligns with GraphQL best practices.src/graphql/types/Mutation/createActionItemCategory.ts (1)
14-14: Good job adding a minimum length constraint.The
z.string().min(1, "Category name cannot be empty")effectively prevents empty category names. This aligns with previous feedback on validating user inputs.test/graphql/types/gql.tada-cache.d.ts (1)
131-132: GraphQL mutation type definition correctly implemented.The new
CreateActionItemCategorymutation is properly defined with the appropriate input and return types. The input takes requiredorganizationIdandnamefields, plus an optionalisDisabledfield, which aligns well with the functionality being implemented.test/graphql/types/Mutation/createActionItemCategory.test.ts (9)
1-14: Well-structured imports with appropriate dependencies.The imports are properly organized, bringing in necessary testing utilities from Vitest, database tables, and GraphQL document nodes for the tests.
16-40: Well-implemented helper function for test setup.The
createOrganizationAndGetIdhelper function is well-structured and includes appropriate error handling and assertions. It creates an organization with unique test data for use in tests.
42-55: Clean utility function for test data setup.The
addMembershipfunction provides a clean way to add organization memberships for test users, which is reused across multiple tests.
57-71: Good authentication setup for tests.The code properly authenticates as an admin user to get a token for tests that require authentication. The assertions ensure that required data is available before proceeding with tests.
73-105: Comprehensive test for unauthenticated users.The test correctly verifies that unauthenticated requests are rejected with the appropriate error code. This ensures the mutation properly enforces authentication requirements.
107-140: Thorough validation of non-existent organization handling.The test effectively verifies that attempts to create a category for a non-existent organization are properly rejected with appropriate error details.
142-177: Well-implemented test for organization membership validation.This test correctly verifies that users who are not members of an organization cannot create categories for it, enforcing proper authorization boundaries.
179-245: Comprehensive test for role-based access control.This test suite thoroughly validates that non-administrator users cannot create categories even if they are members of the organization, enforcing proper role-based permissions.
1-294: Overall excellent test coverage for the new mutation.The test suite provides comprehensive coverage of the
createActionItemCategorymutation, testing authentication, authorization, input validation, and successful execution. The tests follow good practices with proper setup, teardown, and assertions.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (6)
src/graphql/types/Mutation/createActionItem.ts (1)
20-162: Membership & role check logic duplication.
You are repeating membership & role checks here, which have also appeared in other mutations. It may be beneficial to extract them into a shared helper for reusability.Reevaluate automatic
completionAtassignment.
At line 142, you setcompletionAttonew Date(), which marks the action item as completed at creation. Consider deferring this assignment until the action is truly completed or rename the field to avoid confusion.src/graphql/types/Mutation/createActionItemCategory.ts (1)
1-127: 🧹 Nitpick (assertive)Consider adding a max length or trimming for
name.
You already validate thatnameis non-empty. Enforcing a reasonable maximum length or trimming input can further prevent data inconsistencies.- name: z.string().min(1, "Category name cannot be empty"), + name: z.string().min(1, "Category name cannot be empty").max(50, "Name cannot exceed 50 characters"),Consolidate membership & role checks.
Similar membership & role checks exist in other mutations. A shared utility could keep repeated logic consistent and easier to maintain.Avoid duplicating Zod and builder definitions.
The input type validation is defined both in Zod and in the builder’sinputType. Centralizing or referencing a single schema helps prevent unintentional mismatches in future refactors.src/graphql/types/Mutation/updateActionItemCategory.ts (2)
8-14: Consider referencing the Zod schema to unify input definitions
This is a repeated suggestion from a previous review comment. You might consolidate the builder’s input fields with the same Zod schema used here to ensure consistency across mutations.
65-86: Well-structured membership checks
As noted previously, consider moving this membership and role-check logic into a shared utility to reduce duplication across create/update mutations.test/graphql/types/Mutation/createActionItemCategory.test.ts (2)
4-4: Consider removing or uncommenting the commented import.There's a commented import for
actionCategoriesTablewhich isn't being used in the code. If it's not needed, consider removing it entirely; if it will be needed, consider uncommenting it.-// import { actionCategoriesTable } from "~/src/drizzle/tables/actionCategories";
247-292: Verify mock parameters in the success case test.While the success test correctly mocks the database operation and asserts the response, it doesn't verify that the database insert was called with the expected parameters. Consider adding an assertion to verify the mock was called with the correct values.
server.drizzleClient.insert = vi.fn().mockReturnValueOnce({ values: vi.fn().mockReturnThis(), returning: vi.fn().mockResolvedValueOnce([ { id: "category-123", name: "Test Category", organizationId: orgId, creatorId: adminUserId, isDisabled: false, createdAt: new Date(), updatedAt: new Date(), }, ]), }); const result = await mercuriusClient.mutate( Mutation_createActionItemCategory, { headers: { authorization: `bearer ${authToken}` }, variables: { input: { name: "Test Category", organizationId: orgId, isDisabled: false, }, }, }, ); +// Verify insert was called with correct table and values +expect(server.drizzleClient.insert).toHaveBeenCalledWith(expect.anything()); +expect(server.drizzleClient.insert().values).toHaveBeenCalledWith( + expect.objectContaining({ + name: "Test Category", + organizationId: orgId, + isDisabled: false, + creatorId: adminUserId, + }) +); expect(result.errors).toBeUndefined();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
schema.graphql(6 hunks)src/createServer.ts(0 hunks)src/envConfigSchema.ts(0 hunks)src/graphql/types/Mutation/createActionItem.ts(1 hunks)src/graphql/types/Mutation/createActionItemCategory.ts(1 hunks)src/graphql/types/Mutation/index.ts(1 hunks)src/graphql/types/Mutation/updateActionItemCategory.ts(1 hunks)test/graphql/types/Mutation/createActionItemCategory.test.ts(1 hunks)test/graphql/types/documentNodes.ts(1 hunks)test/graphql/types/gql.tada-cache.d.ts(1 hunks)test/graphql/types/gql.tada.d.ts(5 hunks)
💤 Files with no reviewable changes (2)
- src/envConfigSchema.ts
- src/createServer.ts
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/graphql/types/Mutation/createActionItemCategory.ts (2)
src/graphql/builder.ts (1)
builder(10-20)src/utilities/TalawaGraphQLError.ts (1)
TalawaGraphQLError(264-277)
src/graphql/types/Mutation/createActionItem.ts (2)
src/graphql/builder.ts (1)
builder(10-20)src/utilities/TalawaGraphQLError.ts (1)
TalawaGraphQLError(264-277)
test/graphql/types/Mutation/createActionItemCategory.test.ts (3)
test/graphql/types/client.ts (1)
mercuriusClient(7-9)test/graphql/types/documentNodes.ts (4)
Mutation_createOrganization(370-378)Query_signIn(191-218)Mutation_createActionItemCategory(1007-1019)Mutation_createUser(12-40)test/server.ts (1)
server(12-27)
src/graphql/types/Mutation/updateActionItemCategory.ts (2)
src/graphql/builder.ts (1)
builder(10-20)src/utilities/TalawaGraphQLError.ts (1)
TalawaGraphQLError(264-277)
🔇 Additional comments (20)
test/graphql/types/documentNodes.ts (1)
1007-1019: Mutation registration looks good.
This newly addedcreateActionItemCategorymutation appears consistent with the existing pattern in this file. The input type and returned fields align correctly with the rest of the codebase.src/graphql/types/Mutation/index.ts (1)
80-81: Consistent import structure.
Importing and registering the newcreateActionItemCategoryandupdateActionItemCategorymutations here follows the existing convention of listing all mutation modules.src/graphql/types/Mutation/createActionItem.ts (1)
16-17: OptionalassignedAtfield integration is well-handled.
DefiningassignedAtas optional in the schema is a flexible approach. It’s clearly defaulted tonew Date()if omitted in the mutation.src/graphql/types/Mutation/updateActionItemCategory.ts (4)
32-45: Looks good
The authentication flow and argument parsing appear correct.
46-55: Straightforward retrieval
Fetching the existing category is clearly structured and typed properly.
56-63: Robust error handling
You correctly throw a TalawaGraphQLError when the category ID does not match any existing resource.
113-114: Returning updated data
Returning the updated record is straightforward and meets the mutation requirements.test/graphql/types/gql.tada.d.ts (4)
38-38: Well-structured input type for action item category creation.The
CreateActionItemCategoryInputdefinition correctly includes the required fields with appropriate nullability constraints. ThenameandorganizationIdfields are correctly marked as non-nullable, whileisDisabledcan be optional.
61-61: Good type definition for HasUserVoted.The
HasUserVotedobject type is well defined with a non-nullabletypefield that uses thePostVoteTypeenum. This makes the API response type-safe and prevents null values in critical fields.
73-73: Properly updated Mutation and Query objects.The Mutation and Query objects have been correctly updated to include the new fields
createActionItemCategoryandhasUserVoted, respectively, ensuring they're accessible through the GraphQL API.Also applies to: 187-187
224-224: Well-designed update input type.The
UpdateActionItemCategoryInputis properly structured with a requiredcategoryIdfield and optional update fields forisDisabledandname. This follows best practices for partial updates.test/graphql/types/Mutation/createActionItemCategory.test.ts (6)
16-40: Well-implemented organization creation helper function.The helper function is well-structured with appropriate error handling, assertions, and meaningful variable names. It properly creates an organization with unique names using faker to prevent test conflicts.
42-55: Good utility function for adding memberships.The
addMembershipfunction is concise and handles the database insertion correctly with typed role parameter.
79-106: Comprehensive authentication test.Your test for unauthenticated access properly verifies that the mutation returns the expected error code and response structure. This ensures unauthorized users cannot create action item categories.
107-140: Good validation for non-existent organization.The test correctly verifies that the mutation returns an appropriate error when attempting to create a category for a non-existent organization, with detailed assertion of the error structure.
142-177: Well-implemented membership validation.This test properly verifies that users who are not members of an organization cannot create categories for it, ensuring proper access control.
179-245: Thorough administrator role check.The test case for non-administrator users is comprehensive, creating a new user, adding appropriate membership, and verifying the correct error response when they attempt to create a category without admin rights.
schema.graphql (3)
782-785: New Type Definition: HasUserVotedThe new
HasUserVotedtype encapsulates the vote type information and replaces the previous scalar/boolean field. The field description is present and clear. Ensure that the resolver logic and client queries are updated accordingly to reflect this new type.
1486-1488: New Mutation Field: createActionItemCategoryThe mutation field for creating an Action Item Category is added with a clear description and proper input type. Verify that the corresponding resolver (in its separate file) includes the necessary input validation, authentication, and authorization checks.
1663-1665: New Mutation Field: updateActionItemCategoryThis mutation field for updating an existing Action Item Category is well defined. As with any update operation, ensure that the resolver enforces proper validation (e.g., at least one field to update is provided) and appropriate permission checks.
Signed-off-by: NishantSinghhhhh <[email protected]>
Signed-off-by: NishantSinghhhhh <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/graphql/types/Mutation/updateActionItemCategory.ts (4)
8-14: Consider referencing a shared Zod schema for category inputs.
This helps keep the definitions for create/update operations in sync and reduces duplication in the codebase.
22-28: Place input definitions in a dedicated folder.
Following the existing folder structure for inputs (e.g., theinputsfolder) can help maintain consistency and clarity.
65-86: Extract membership & role checks into a shared utility.
Moving membership and role validations to a shared utility promotes reusability across mutations.
110-121: Consider concurrency control and enhanced logging.
Row-level locking or version checks can help avoid conflicts if multiple users update the same category simultaneously. Logging the category ID in the error message can further aid debugging.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/graphql/types/Mutation/updateActionItemCategory.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/graphql/types/Mutation/updateActionItemCategory.ts (2)
src/graphql/builder.ts (1)
builder(10-20)src/utilities/TalawaGraphQLError.ts (1)
TalawaGraphQLError(264-277)
🪛 GitHub Actions: Pull request workflow
src/graphql/types/Mutation/updateActionItemCategory.ts
[error] 16-16: Formatter would have printed the following content: Code formatting issues detected. Please run the formatter to fix.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (3)
src/graphql/types/Mutation/updateActionItemCategory.ts (3)
32-45: Well-implemented authentication and validation.
The checks for user authentication and argument validation are clear and appropriately handled.
46-63: Straightforward existence check for the category.
Throwing an error for a missing category is a valid approach and keeps the logic simple.
102-109: Good approach to skip unnecessary updates.
Only performing an update when fields actually change helps reduce overhead and avoid needless writes.
I meant the file names in the tables, not in the types, it should be in pascal case like in the other types for the schema name. also, this remains:
Consistency across the code base. This need not be pointed out this many times. Look at how other tables are defined, how other types are defined, how other features are implemented... |
|
I apologize for the confusion between the file name and the table name, which is why I was not able to make the desired changes that you pointed out |
Signed-off-by: NishantSinghhhhh <[email protected]>
|
sir I have chnaged the table names as you said now the table actionItems is |
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (2)
src/graphql/types/Query/ActionItem.ts (1)
26-153: 🧹 Nitpick (assertive)Error handling could be simplified
The error handling logic is thorough but has some redundancy. There are multiple places checking for authorization issues with similar error generation. Consider extracting common error handling patterns into helper functions.
For example, you could create a helper function for authorization checks:
function ensureUserAuthorized( currentUser: { role?: string } | undefined, membership: any | undefined, resourcePath: string[] ) { if (!currentUser) { throw new TalawaGraphQLError({ extensions: { code: "unauthenticated", }, }); } if (currentUser.role !== "administrator" && !membership) { throw new TalawaGraphQLError({ extensions: { code: "unauthorized_action_on_arguments_associated_resources", issues: [{ argumentPath: resourcePath }], }, }); } }Then use it to simplify your code:
// Check authorization ensureUserAuthorized( currentUser, currentUserOrganizationMembership, ["input", "organizationId"] );src/graphql/types/ActionItem/categoryId.ts.ts (1)
1-55: 🧹 Nitpick (assertive)File name has duplicate extension
The file name
categoryId.ts.tshas a duplicate.tsextension which seems to be a mistake.Rename the file to
categoryId.tsto follow standard naming conventions.
♻️ Duplicate comments (22)
test/graphql/types/ActionItem/categoryId.test.ts (1)
43-65: Consider consolidating duplicate function calls in error tests.The test calls
resolveCategorytwice for the same inputs (lines 46-48 and 51). Consider restructuring to capture the error object once and run multiple assertions against it.- await expect(resolveCategory(parent, {}, ctx)).rejects.toBeInstanceOf( - TalawaGraphQLError, - ); - - try { - await resolveCategory(parent, {}, ctx); - } catch (err) { + try { + await resolveCategory(parent, {}, ctx); + fail("Expected function to throw an error"); + } catch (err) { + expect(err).toBeInstanceOf(TalawaGraphQLError); expect(err).toMatchObject({ message: "Category not found", extensions: { code: "arguments_associated_resources_not_found", issues: [{ argumentPath: ["categoryId"] }], }, });src/drizzle/schema.ts (1)
8-8:⚠️ Potential issueInconsistent naming pattern for relation export
The naming pattern for your relation exports should follow
<tableName>Relations. While you've properly renamed the table toactionItemsTable, the relation export isactionsItemRelationswhich is inconsistent.-export { actionItemsTable, actionsItemRelations } from "./tables/actionItems"; +export { actionItemsTable, actionItemsRelations } from "./tables/actionItems";This ensures you follow the established naming convention where the relation export name is derived from the table name.
src/graphql/types/ActionItem/actionItem.ts (1)
24-24: 🧹 Nitpick (assertive)Consider adding a comment explaining why updatedAt is nullable
The change to make updatedAt nullable isn't obvious. A comment would help explain the rationale.
Add a comment explaining why updatedAt can be null:
updatedAt: t.expose("updatedAt", { description: "Timestamp when the action item was last updated.", type: "DateTime", + // Nullable because new items might not have been updated yet nullable: true, }),src/graphql/types/Mutation/createActionItemCategory.ts (3)
81-85:⚠️ Potential issueUnsafe hand‑rolled SQL in
whereclause can break portability & safetyUsing raw template interpolation with SQL makes the statement harder to reason about and may bypass the query‑builder's parameterization guarantees.
A safer, clearer pattern is to rely on the logical helpers Drizzle already provides:
- where: (fields, operators) => - sql`${operators.eq(fields.memberId, currentUserId)} AND ${operators.eq( - fields.organizationId, - parsedArgs.input.organizationId, - )}`, + where: (fields, operators) => + operators.and( + operators.eq(fields.memberId, currentUserId), + operators.eq(fields.organizationId, parsedArgs.input.organizationId), + )
115-123: 🛠️ Refactor suggestionMissing updaterId in category creation
The resolver sets the
creatorIdbut not theupdaterIdfield, which should be set to maintain consistency in tracking who last modified the record.Add the updaterId field to the insert operation:
.values({ id: uuidv7(), name: parsedArgs.input.name, organizationId: parsedArgs.input.organizationId, creatorId: currentUserId, + updaterId: currentUserId, // If isDisabled is provided, use it; otherwise, default to false (i.e. enabled). isDisabled: parsedArgs.input.isDisabled ?? false, createdAt: new Date(), updatedAt: new Date(), })
111-132: 🛠️ Refactor suggestionCheck for duplicate category names before inserting
Nothing prevents two admins from inserting the same category twice for one organization.
Add a uniqueness guard before insertion:
+ // 5½. Ensure category name is unique within the organisation. + const existingCategory = await ctx.drizzleClient.query.actionItemCategoriesTable.findFirst({ + columns: { id: true }, + where: (fields, operators) => + operators.and( + operators.eq(fields.organizationId, parsedArgs.input.organizationId), + operators.eq(fields.name, parsedArgs.input.name), + ), + }); + if (existingCategory) { + throw new TalawaGraphQLError({ + extensions: { + code: "already_exists", + issues: [{ argumentPath: ["input", "name"], message: "Category name already exists" }], + }, + }); + } // 6. Insert the new category into the database.src/graphql/inputs/QueryActionCategoriesByOrganizationInput.ts (1)
33-36: 🧹 Nitpick (assertive)Clarify the purpose of the optional arguments wrapper
The comment "optional for consistency" doesn't clearly explain why this wrapper is needed or when it should be used.
/** - * 3️⃣ Arguments schema to wrap the input (optional for consistency) + * 3️⃣ Arguments schema to wrap the input + * This wrapper provides consistency with other query/mutation argument structures + * and allows validating the entire args object at once */src/graphql/types/Mutation/updateActionItemCategory.ts (3)
81-84:⚠️ Potential issueSecurity issue in SQL query and formatting inconsistency
There are two issues with this code segment:
- SQL query construction using string templates could lead to security issues
- The indentation on line 83 is inconsistent
Replace the raw SQL template with Drizzle's logical operators and fix the indentation:
- where: (fields, operators) => - sql`${operators.eq(fields.memberId, currentUserId)} - AND ${operators.eq(fields.organizationId, existingCategory.organizationId)}`, + where: (fields, operators) => + operators.and( + operators.eq(fields.memberId, currentUserId), + operators.eq(fields.organizationId, existingCategory.organizationId) + ),
104-107: 🛠️ Refactor suggestionMissing uniqueness check when updating name
When updating the category name, there's no check to prevent creating a duplicate name within the same organization.
Add a check for an existing category with the same name:
if (typeof name === "string") { + // Check if another category with the same name exists in this organization + const existingCategoryWithSameName = await ctx.drizzleClient.query.actionItemCategoriesTable.findFirst({ + columns: { id: true }, + where: (fields, operators) => + operators.and( + operators.eq(fields.organizationId, existingCategory.organizationId), + operators.eq(fields.name, name), + operators.ne(fields.id, categoryId), // Exclude the current category + ), + }); + + if (existingCategoryWithSameName) { + throw new TalawaGraphQLError({ + extensions: { + code: "invalid_arguments", + issues: [ + { + argumentPath: ["input", "name"], + message: "A category with this name already exists in the organization", + }, + ], + }, + }); + } updates.name = name; hasUpdates = true; }
115-119: 🛠️ Refactor suggestionMissing updaterId in update operation and empty else block
Two issues in this code segment:
- The code sets the
updatedAttimestamp but doesn't set theupdaterIdfield- There's an empty else block with just a comment
Fix both issues:
if (hasUpdates) { updates.updatedAt = new Date(); + updates.updaterId = currentUserId; - } else { - // Optionally handle scenario where nothing is provided to update. }src/drizzle/tables/actionItems.ts (5)
68-68: 🧹 Nitpick (assertive)Add comment explaining the allottedHours field
The allottedHours field was added to track time allocation for action items but lacks documentation about its purpose and usage.
- allottedHours: numeric("allotted_hours"), + // Number of hours estimated to complete this action item + allottedHours: numeric("allotted_hours"),
104-104: 🧹 Nitpick (assertive)Incorrect relation variable name
The relation variable should match the table name for consistency.
-export const actionsItemRelations = relations(actionItemsTable, ({ one }) => ({ +export const actionItemsRelations = relations(actionItemsTable, ({ one }) => ({
113-113:⚠️ Potential issueFix relationName to use the renamed tables
The relationName still references the old table names which doesn't match the renamed variables.
- relationName: "action_categories.id:actions.category_id", + relationName: "action_item_categories.id:action_items.category_id",
27-27: 🛠️ Refactor suggestionUpdate database column for assigneeId to match the TS field
The database column name
actor_iddoesn't match the field nameassigneeId, creating inconsistency between the code and database schema.- assigneeId: uuid("actor_id").references(() => usersTable.id, { + assigneeId: uuid("assignee_id").references(() => usersTable.id, {This will require a database migration to rename the column.
137-138: 🧹 Nitpick (assertive)Add export comment for clarity
The export comment could be improved to clarify the purpose of this schema.
-// ✅ Export the insert schema with the new field included +// ✅ Export the insert schema with the new field included +// This schema is used for validating insertions into the actionItems tablesrc/graphql/types/Mutation/createActionItem.ts (2)
12-14: 🛠️ Refactor suggestionIncorrect mutation export name
The exported mutation is named
createActionItemCategoryMutationbut it's actually implementingcreateActionItem. This naming mismatch could cause confusion.-export const createActionItemCategoryMutation = builder.mutationField( +export const createActionItemMutation = builder.mutationField(
145-145:⚠️ Potential issueRemove default completion date for incomplete items
Setting
completionAtto the current date on creation is misleading since the item is marked as incomplete (isCompleted: false). Completion date should only be set when an item is actually completed.- completionAt: new Date(), + completionAt: null,test/graphql/types/gql.tada.d.ts (5)
76-78:⚠️ Potential issueInput mismatch:
allottedHoursstillInt,assignedAtstillStringThis repeats an earlier review point and remains unresolved. Please update the underlying
MutationCreateActionItemInputdefinition as outlined above and regenerate the introspection file.
128-128: 🛠️ Refactor suggestionInconsistent id field name (
categoryIdvsid)Previous review requested aligning with the rest of the API (
id). Keeping a bespokecategoryIdforces client‑side special‑cases and harms DX.- t.id('categoryId', { required: true }) + t.id('id', { required: true })
129-129:⚠️ Potential issueUpdateActionItemInput – type & nullability issues still pending
allottedHoursisInt(should beFloat).isCompletedis non‑null, breaking PATCH‑style partial updates.assignedAtis missing entirely, so cannot be amended.These were flagged before and are still present.
189-191: 🛠️ Refactor suggestionScalar inconsistency for
organizationIdin query inputs
QueryActionCategoriesByOrganizationInputnow usesID✅, butQueryActionItemsByOrganizationInputstill usesString. UseIDeverywhere for identifier symmetry.- t.string('organizationId', { required: true }) + t.id('organizationId', { required: true })Update the builder file and regenerate introspection.
6-7: 🛠️ Refactor suggestion
⚠️ Potential issue
ActionItemfield type drift vs. input declarations
ActionItem.allottedHoursis now correctly exposed asFloatandassignedAtasDateTime, but their counterparts in the create / update input objects are stillIntandString(see lines 77 & 129). Clients will either lose precision (Float→Int) or fail validation (DateTime↔String).Action items can no longer be created/updated consistently until the input types and resolvers are synchronised.
Recommended fix (GraphQL builder definitions, not this generated file):
- t.int('allottedHours') + t.float('allottedHours') - t.string('assignedAt') + t.field('assignedAt', { type: 'DateTime' })After adjusting the builder code, re‑run code‑gen so the
.d.tsfile self‑heals.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (28)
drizzle_migrations/20250422113008_fast_prism.sql(1 hunks)drizzle_migrations/20250422113209_common_la_nuit.sql(1 hunks)drizzle_migrations/meta/_journal.json(1 hunks)scripts/dbManagement/helpers.ts(3 hunks)src/createServer.ts(0 hunks)src/drizzle/schema.ts(1 hunks)src/drizzle/tables/actionItemCategories.ts(2 hunks)src/drizzle/tables/actionItems.ts(2 hunks)src/drizzle/tables/organizations.ts(2 hunks)src/drizzle/tables/users.ts(2 hunks)src/envConfigSchema.ts(0 hunks)src/graphql/inputs/QueryActionCategoriesByOrganizationInput.ts(1 hunks)src/graphql/types/ActionItem/actionItem.ts(3 hunks)src/graphql/types/ActionItem/categoryId.ts.ts(1 hunks)src/graphql/types/ActionItem/index.ts(1 hunks)src/graphql/types/ActionItemCategory/actionItemCategory.ts(2 hunks)src/graphql/types/Mutation/createActionItem.ts(1 hunks)src/graphql/types/Mutation/createActionItemCategory.ts(1 hunks)src/graphql/types/Mutation/deleteActionItem.ts(3 hunks)src/graphql/types/Mutation/updateActionItem.ts(6 hunks)src/graphql/types/Mutation/updateActionItemCategory.ts(1 hunks)src/graphql/types/Query/ActionCategorybyOrg.ts(4 hunks)src/graphql/types/Query/ActionItem.ts(2 hunks)src/graphql/types/Query/index.ts(1 hunks)test/graphql/types/ActionItem/assignee.test.ts(0 hunks)test/graphql/types/ActionItem/categoryId.test.ts(1 hunks)test/graphql/types/gql.tada.d.ts(5 hunks)test/scripts/dbManagement/helpers.test.ts(4 hunks)
💤 Files with no reviewable changes (3)
- src/envConfigSchema.ts
- test/graphql/types/ActionItem/assignee.test.ts
- src/createServer.ts
🧰 Additional context used
🧠 Learnings (1)
test/graphql/types/gql.tada.d.ts (1)
Learnt from: GlenDsza
PR: PalisadoesFoundation/talawa-api#2615
File: src/typeDefs/inputs.ts:80-80
Timestamp: 2024-11-01T10:39:50.333Z
Learning: In `src/typeDefs/inputs.ts`, the `orgId` field in `ActionItemWhereInput` is intentionally optional (`ID`) and should not be changed to non-nullable (`ID!`).
🧬 Code Graph Analysis (10)
src/graphql/types/Mutation/deleteActionItem.ts (1)
src/drizzle/tables/actionItems.ts (1)
actionItemsTable(18-102)
src/drizzle/tables/users.ts (3)
src/drizzle/schema.ts (2)
actionItemsTable(8-8)actionItemCategoriesTable(5-5)src/drizzle/tables/actionItems.ts (1)
actionItemsTable(18-102)src/drizzle/tables/actionItemCategories.ts (1)
actionItemCategoriesTable(16-66)
test/graphql/types/ActionItem/categoryId.test.ts (3)
src/graphql/context.ts (1)
GraphQLContext(55-55)test/_Mocks_/drizzleClientMock.ts (1)
createMockDrizzleClient(30-50)src/utilities/TalawaGraphQLError.ts (1)
TalawaGraphQLError(264-277)
src/drizzle/tables/actionItemCategories.ts (3)
src/drizzle/tables/actionItems.ts (1)
actionItemsTable(18-102)src/drizzle/tables/users.ts (1)
usersTable(59-216)src/drizzle/tables/organizations.ts (1)
organizationsTable(31-128)
src/graphql/types/Mutation/updateActionItemCategory.ts (5)
src/graphql/builder.ts (1)
builder(10-20)src/graphql/types/ActionItemCategory/actionItemCategory.ts (2)
ActionItemCategory(6-8)ActionItemCategory(10-11)src/graphql/inputs/MutationUpdateActionItemCategory.ts (2)
MutationUpdateActionItemCategoryInput(18-41)mutationUpdateActionItemCategoryArgumentsSchema(46-48)src/utilities/TalawaGraphQLError.ts (1)
TalawaGraphQLError(264-277)src/drizzle/tables/actionItemCategories.ts (1)
actionItemCategoriesTable(16-66)
src/graphql/types/Mutation/updateActionItem.ts (1)
src/drizzle/tables/actionItems.ts (1)
actionItemsTable(18-102)
src/graphql/inputs/QueryActionCategoriesByOrganizationInput.ts (1)
src/graphql/builder.ts (1)
builder(10-20)
src/graphql/types/Query/ActionCategorybyOrg.ts (2)
src/utilities/TalawaGraphQLError.ts (1)
TalawaGraphQLError(264-277)src/graphql/inputs/QueryActionCategoriesByOrganizationInput.ts (1)
queryActionCategoriesByOrganizationArgumentsSchema(37-39)
src/graphql/types/ActionItem/categoryId.ts.ts (4)
src/graphql/context.ts (1)
GraphQLContext(55-55)src/graphql/types/ActionItemCategory/actionItemCategory.ts (2)
ActionItemCategory(6-8)ActionItemCategory(10-11)src/utilities/TalawaGraphQLError.ts (1)
TalawaGraphQLError(264-277)src/graphql/types/ActionItem/actionItem.ts (2)
ActionItem(4-4)ActionItem(6-6)
src/drizzle/tables/actionItems.ts (3)
src/drizzle/tables/users.ts (1)
usersTable(59-216)src/drizzle/tables/actionItemCategories.ts (1)
actionItemCategoriesTable(16-66)src/drizzle/tables/organizations.ts (1)
organizationsTable(31-128)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run tests for talawa api
🔇 Additional comments (35)
drizzle_migrations/meta/_journal.json (1)
11-59: Migration journal entries look goodThe journal entries for the migrations related to action items and categories have been properly added with sequential indices and appropriate tags. This correctly documents the database schema changes for tracking and versioning purposes.
I notice that the migration entries include operations for dropping and likely recreating the
action_categoriesandactionstables based on the migration file names (e.g., "fast_prism" and "common_la_nuit"). This aligns with the PR objective to migrate ActionItem Categories to GraphQL.src/graphql/types/Query/ActionItem.ts (2)
3-7: Import organization looks goodThe imports have been updated to be more organized and use absolute paths consistently. The consolidation of input imports and the consistent path structure for imports improve code maintainability.
66-66: Table naming change aligns with schema updatesThe table reference has been updated from
actionsTabletoactionItemsTable, which correctly aligns with the database schema changes introduced in the migrations. This ensures consistency between the GraphQL schema and the underlying database tables.src/drizzle/tables/organizations.ts (3)
14-15: Updated import statements to match renamed tablesThe imports have been correctly updated to reflect the renaming of action-related tables from
actionCategoriesTableandactionsTabletoactionItemCategoriesTableandactionItemsTable, ensuring proper reference to the renamed table modules.
136-138: Updated relation references to use renamed actionItems tableThe relation reference has been correctly updated from
actionsTabletoactionItemsTablewhile maintaining the original relation name, preserving the relationship logic while ensuring naming consistency.
142-144: Updated relation references to use renamed actionItemCategories tableThe relation reference has been correctly updated from
actionCategoriesTabletoactionItemCategoriesTablewhile preserving the original relation name string. This maintains consistency with the broader refactoring effort to standardize action item terminology.test/scripts/dbManagement/helpers.test.ts (4)
330-331: Updated test to use actionItemCategoriesTable referenceThe test has been properly updated to use the renamed
schema.actionItemCategoriesTablereference, aligning with the schema changes throughout the codebase.
344-345: Updated primary key reference to use actionItemCategoriesTableThe reference to the primary key has been correctly updated to use
schema.actionItemCategoriesTable.id, ensuring the test continues to function properly with the renamed tables.
364-369: Updated type inference for captured dataThe type inference has been correctly updated to use
schema.actionItemsTable.$inferInsert, ensuring proper type safety when working with the renamed table in the test.
383-386: Updated table and primary key references in test functionThe test case now correctly uses
schema.actionItemsTableand its ID field, maintaining consistency with the renamed tables throughout the codebase.src/drizzle/tables/users.ts (3)
22-23: Updated import statements to match renamed tablesThe imports have been correctly updated to reflect the renaming of action-related tables from
actionCategoriesTableandactionsTabletoactionItemCategoriesTableandactionItemsTable, maintaining consistency with changes across the codebase.
222-236: Updated user-action relationships to use renamed action tablesAll action-related relation definitions have been correctly updated to use
actionItemsTableinstead ofactionsTablewhile maintaining the original relation names. This ensures consistency with the broader refactoring of action item terminology while preserving the underlying relationship structure.
240-248: Updated user-actionCategory relationships to use renamed category tablesThe action category relation definitions have been correctly updated to use
actionItemCategoriesTableinstead ofactionCategoriesTablewhile preserving the original relation names. This maintains consistency with the codewide updates to action item terminology.src/graphql/types/Mutation/deleteActionItem.ts (3)
2-7: Updated imports and schema referencesThe imports have been updated to:
- Use the renamed
actionItemsTableinstead ofactionsTable- Fix the import path for
ActionItemto correctly useactionItem(lowercase)- Add an import for the shared
mutationDeleteActionItemArgumentsSchemaThese changes ensure consistency with the project's naming conventions and promote reuse of validation schemas.
58-64: Updated query to use renamed actionItemsTableThe database query now correctly references
actionItemsTableinstead of the previousactionsTable, aligning with the renamed table structure while maintaining the same query logic.
89-92: Updated delete operation to use renamed actionItemsTableThe delete operation now correctly uses
actionItemsTablein both the table reference and the where clause, maintaining consistency with the renamed table structure throughout the codebase.scripts/dbManagement/helpers.ts (2)
561-617: LGTM! Table references consistently updated.The changes appropriately update references from the old table names (
actionCategoriesTableandactionsTable) to the new names (actionItemCategoriesTableandactionItemsTable), aligning with the broader schema refactoring effort.
669-671: LGTM! Table references consistently updated in the tables array.Tables array entries have been correctly updated to use the new table names, maintaining consistency with the changes made elsewhere in the file.
src/graphql/types/Mutation/updateActionItem.ts (1)
198-201:❌ Incorrect review comment
Verify the handling of allottedHours as a string conversion.
The code converts
allottedHoursto a string before storing it, but the database column is defined asnumeric. While Postgres can handle string-to-numeric conversion automatically, it's generally better to maintain type consistency.
🏁 Script executed:
#!/bin/bash # Check how allottedHours is handled in other locations to ensure consistency echo "Checking allottedHours handling in other mutations:" rg --type ts "allottedHours.*toString\(\)" src/graphql/types/Mutation/ echo -e "\nChecking allottedHours column definition:" rg --type ts -A 3 "allottedHours" src/drizzle/tables/Length of output: 770
Remove incorrect concern about
allottedHoursconversionThe string conversion for
allottedHoursis intentional and consistent across bothcreateActionItem.tsandupdateActionItem.ts. In our Drizzle schema,numeric("allotted_hours")columns expect string values to preserve precision. No change needed here.Likely an incorrect or invalid review comment.
test/graphql/types/ActionItem/categoryId.test.ts (1)
1-79: LGTM! Comprehensive test coverage for the resolveCategory function.The test suite properly verifies all key scenarios for the resolveCategory function:
- Handling null categoryId
- Error behavior when category is not found
- Successful category resolution
The tests are well-structured with appropriate mocks and assertions.
src/drizzle/tables/actionItemCategories.ts (2)
12-12: Renaming looks goodThe renaming from action categories to action item categories is consistent and well-implemented throughout the file, including imports, table definition, and relations.
Also applies to: 16-16, 68-71
76-77: Field references correctly updatedThe field references in the relations definitions have been properly updated to use the new table name
actionItemCategoriesTable.Also applies to: 82-83, 88-89
src/graphql/types/Query/ActionCategorybyOrg.ts (4)
2-6: Import statement updates correctly reflect renamed typesThe import statements have been properly updated to use the renamed types and schemas.
35-40: Good refactoring to use shared Zod schemaReplacing the inline schema with a shared validation schema improves code reusability and maintainability.
63-68: Table reference and argument access properly updatedThe change from
actionCategoriesTabletoactionItemCategoriesTableis correct and consistent with the schema renaming. The organization ID is now properly accessed viaparsedArgs.input.organizationId.
29-32: Code quality improvementsAdded comments, improved error handling with specific error codes, and used null coalescing for the return value enhance code readability and robustness.
Also applies to: 56-57, 74-75, 94-94
src/graphql/types/ActionItem/categoryId.ts.ts (2)
6-43: Well-implemented resolver with appropriate error handlingThe resolver function is well-structured with:
- Null check for the categoryId
- Database query using the correct table
- Comprehensive error handling with specific error code and logging
This approach correctly implements the GraphQL pattern of resolving related objects.
45-54: Good implementation of ActionItem type extensionThe implementation properly extends the ActionItem type with a nullable category field that uses the resolver function. The field includes a descriptive comment that helps with API documentation.
src/graphql/types/ActionItem/actionItem.ts (1)
38-57: Well-implemented allottedHours field with robust type handlingThe implementation thoroughly handles different input types and edge cases when parsing the allottedHours value, including string parsing, NaN checking, and proper null handling.
src/graphql/types/ActionItemCategory/actionItemCategory.ts (3)
53-58: Good improvement in error handlingThe validation logic now properly uses
safeParseand provides more detailed error information in case of failure.
59-67: Improved robustness with null checksGood addition of null checks for the Drizzle client and table before attempting to query, preventing potential runtime errors.
69-71: Updated table reference correctlyThe query now correctly references
actionItemCategoriesTablein line with the schema changes throughout the codebase.src/graphql/types/Mutation/createActionItem.ts (2)
32-47: Good improvement in argument validationThe switch to
safeParseAsyncwith detailed error reporting is a significant improvement over the previous implementation. This provides better feedback to API consumers when arguments are invalid.
159-165: Improved error handling for empty resultGood addition of error logging and proper error response when the insert operation unexpectedly returns an empty array.
drizzle_migrations/20250422113209_common_la_nuit.sql (1)
43-43: Good indexing strategyThe unique index on
nameandorganization_idensures that category names are unique within each organization but can be reused across different organizations.
Signed-off-by: NishantSinghhhhh <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
drizzle_migrations/20250418120528_remarkable_molecule_man.sql (1)
22-22: No default foris_completed
"is_completed"isNOT NULLbut lacksDEFAULT false. Adding a default improves safety for ad‑hoc inserts and future migrations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
drizzle_migrations/20250406144036_curious_rattler.sql(3 hunks)drizzle_migrations/20250418120528_remarkable_molecule_man.sql(1 hunks)scripts/dbManagement/sample_data/action_items.json(165 hunks)
🔇 Additional comments (1)
scripts/dbManagement/sample_data/action_items.json (1)
5-11: Verify sample data matches the updated schemaIn the first object, the
"actorId"key has been correctly renamed to"assigneeId"(line 5) and a new"allottedHours": 5.5field was inserted (line 11). This change pattern repeats for every action item entry. Please ensure:
- All occurrences of
assigneeIdin this JSON align with your GraphQL input types and resolver mappings.- The new
allottedHoursfield is properly handled in your migrations, ORM models, and GraphQL schema (nullableFloat).Can you confirm that the backend migrations and resolvers have been updated to use
assigneeIdand exposeallottedHoursas expected?
Signed-off-by: NishantSinghhhhh <[email protected]>
Signed-off-by: NishantSinghhhhh <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (9)
drizzle_migrations/20250406144036_curious_rattler.sql (3)
15-15: Snake_case naming enforced for assignee_id
The column"assignee_id"correctly uses snake_case, replacing the previous camelCaseactorId/assigneeIdas per the project's conventions.
443-443: Foreign key updated forassignee_id
The FK constraint"actions_assignee_id_users_id_fk"now references the snake_case column, aligning with other user‐referencing FKs in the schema.
557-557: Index created forassignee_id
The index"actions_assignee_id_index"has been added on the new snake_case column, maintaining consistency with existing index naming patterns.drizzle_migrations/20250418120528_remarkable_molecule_man.sql (6)
12-38: Replace invalid SQL comment markersThe script uses
--> statement-breakpointwhich is not valid SQL. Standard SQL comments should use--. Please replace all occurrences:---> statement-breakpoint +-- statement-breakpoint
6-6: 🧹 Nitpick (assertive)Add DEFAULT for
is_disabledAdding
DEFAULT falsetois_disabledensures safe inserts and reduces boilerplate in future migrations.-"is_disabled" boolean NOT NULL, +"is_disabled" boolean NOT NULL DEFAULT false,
22-22: 🧹 Nitpick (assertive)Add DEFAULT for
is_completedSet a default for the completion flag to avoid forcing explicit values on insert.
-"is_completed" boolean NOT NULL, +"is_completed" boolean NOT NULL DEFAULT false,
1-11:⚠️ Potential issueInconsistent table naming with code conventions
Your migration creates
action_categories, but your ORM, GraphQL types, and application code use snake_caseaction_item_categories. Please rename the table and update all references to maintain consistency across the schema and resolvers.-CREATE TABLE "action_categories" ( +CREATE TABLE "action_item_categories" (
13-28:⚠️ Potential issueInconsistent table and column naming
Your migration defines
"actions"and"assignee_id", but your GraphQL layer and code expectaction_itemsandactor_id. Rename accordingly to avoid mapping errors.-CREATE TABLE "actions" ( +CREATE TABLE "action_items" ( ... -"assignee_id" uuid, +"actor_id" uuid,
39-49:⚠️ Potential issueUpdate indexes to use new table and column names
Ensure all index definitions reflect the renamed tables/columns. For example:
-CREATE INDEX "action_categories_created_at_index" ON "action_categories" USING btree ("created_at"); +CREATE INDEX "action_item_categories_created_at_index" ON "action_item_categories" USING btree ("created_at"); -CREATE INDEX "actions_assignee_id_index" ON "actions" USING btree ("assignee_id"); +CREATE INDEX "action_items_actor_id_index" ON "action_items" USING btree ("actor_id");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
drizzle_migrations/20250406144036_curious_rattler.sql(3 hunks)drizzle_migrations/20250418120528_remarkable_molecule_man.sql(1 hunks)drizzle_migrations/20250422113209_common_la_nuit.sql(1 hunks)src/envConfigSchema.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/envConfigSchema.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (1)
drizzle_migrations/20250422113209_common_la_nuit.sql (1)
31-39: Verify TypeScript table constants and relationshipsThe foreign key constraints and cascading rules look correct. Please ensure that your Drizzle ORM table constants and GraphQL resolvers have been updated to reference the exact table names
"action_categories"and"actions"(rather than any legacy constants likeactionItemCategoriesTableoractionItemsTable) to avoid mismatches at runtime.
|
@meetulr sir I have too many migration files what should I do about them ?? |
Signed-off-by: NishantSinghhhhh <[email protected]>
|
Why do the number of lines keep increasing so dramatically? Also, this and schema names, pascal case. |
Sir number of snapshots are increasing and due to that only I beleive the number of lines are increasing |
|
Should this be closed and raised again, after merging with the upstream? |
|
@NishantSinghhhhh you can drop the old migration files just keep the latest one |
Maybe yess as the number of commits are so much and the the drizzle migration files are also too much |
This will not work, if we delete any drizzle migration the server will crash |
|
Sir I am closing this PR and raising a new one @meetulr sir |
What kind of change does this PR introduce?
Feature: Migrates ActionItem Category handling from Express endpoints to GraphQL schema and resolvers.
Issue Number: #3320
Snapshots/Videos:
N/A (No UI changes; these changes affect backend GraphQL schemas and resolvers.)
If relevant, did you update the documentation?
I have added/updated relevant documentation within the code and README where needed.
Summary
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores