Fixing advertisment #3557 #3293
Conversation
WalkthroughThe pull request updates the GraphQL schema and related components to support file attachments and organization-based advertisement queries. Specifically, a new attachments field is added to the update advertisement mutation and a new advertisementsByOrg query is introduced with its respective input type. In addition, corresponding input validations, asynchronous processing for file uploads, and improved error handling were implemented. Minor documentation updates and an expansion of the image MIME type enum were also applied. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant G as GraphQL Resolver (advertisementsByOrg)
participant A as Authentication Module
participant O as Organization Service
participant DB as Database
C->>G: Send advertisementsByOrg query with organizationId
G->>A: Verify client authentication
A-->>G: Authentication successful
G->>G: Parse input using QueryAdvertisementsByOrgInput schema
G->>O: Fetch organization by organizationId
O-->>G: Return organization details (or error if not found)
G->>DB: Query advertisements linked to organization
DB-->>G: Return advertisements list
G->>C: Respond with advertisements and attachments
sequenceDiagram
participant C as Client
participant G as GraphQL Resolver (updateAdvertisement)
participant A as Async Validator
participant DB as Database
C->>G: Send update advertisement mutation with attachments
G->>A: Validate input asynchronously (using safeParseAsync)
A-->>G: Return validated input including attachments
G->>DB: Update advertisement record and insert attachments if provided
DB-->>G: Return updated advertisement record
G->>C: Respond with updated advertisement
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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 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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Congratulations on making your first PR! 🎊 If you haven't already, check out our Contributing Guidelines and PR Reporting Guidelines to ensure that you are following our guidelines for contributing and creating PR.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3293 +/- ##
====================================================
- Coverage 48.36% 48.18% -0.19%
====================================================
Files 458 459 +1
Lines 34516 34727 +211
Branches 966 967 +1
====================================================
+ Hits 16695 16733 +38
- Misses 17821 17994 +173 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (1)
schema.graphql (1)
2841-2918: 🧹 Nitpick (assertive)Consider adding pagination support.
The
advertisementsByOrgquery returns an array of advertisements but lacks pagination support. For better scalability and performance, consider implementing pagination using the existing connection pattern used throughout the schema.- advertisementsByOrg(input: QueryAdvertisementsByOrgInput!): [Advertisement!] + advertisementsByOrg( + input: QueryAdvertisementsByOrgInput! + after: String + before: String + first: Int + last: Int + ): AdvertisementsByOrgConnection! +type AdvertisementsByOrgConnection { + edges: [AdvertisementsByOrgConnectionEdge] + pageInfo: PageInfo! +} +type AdvertisementsByOrgConnectionEdge { + cursor: String! + node: Advertisement +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
schema.graphql(3 hunks)src/drizzle/enums/imageMimeType.ts(1 hunks)src/drizzle/tables/advertisements.ts(1 hunks)src/graphql/inputs/MutationCreateAdvertisementInput.ts(1 hunks)src/graphql/inputs/MutationUpdateAdvertisementInput.ts(2 hunks)src/graphql/inputs/QueryAdvertisementsByOrgInput.ts(1 hunks)src/graphql/types/Mutation/createAdvertisement.ts(3 hunks)src/graphql/types/Mutation/updateAdvertisement.ts(4 hunks)src/graphql/types/Query/advertisement.ts(1 hunks)test/routes/graphql/gql.tada.d.ts(2 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 (12)
src/graphql/inputs/MutationCreateAdvertisementInput.ts (1)
7-8: 🧹 Nitpick (assertive)Fix grammatical error in the comment.
The comment has a minor grammatical error. "This file" should be used instead of "These file".
-//This file defines the input for the createAdvertisement mutation. It uses a Zod schema to validate and transform incoming data and then exposes it as a GraphQL input type. +// This file defines the input for the createAdvertisement mutation. It uses a Zod schema to validate and transform incoming data and then exposes it as a GraphQL input type.Likely an incorrect or invalid review comment.
src/graphql/inputs/QueryAdvertisementsByOrgInput.ts (1)
5-24: LGTM!The implementation follows best practices:
- Uses Zod schema for input validation
- Includes UUID format validation with a clear error message
- Properly documents the input type and fields
src/graphql/types/Mutation/createAdvertisement.ts (1)
67-67: LGTM!The comment clearly describes the purpose of the mutation field.
schema.graphql (1)
2032-2033: LGTM!The attachments field is properly defined with a clear description and appropriate type.
src/graphql/inputs/MutationUpdateAdvertisementInput.ts (1)
10-10: LGTM!Using
partial()is appropriate for an update mutation as it makes all fields optional.test/routes/graphql/gql.tada.d.ts (2)
97-97: Good addition for advertisement updates.
The newly introducedattachmentsfield inMutationUpdateAdvertisementInputis consistent with the rest of the schema and properly aligns with the updates for handling file uploads.
152-152: Query field looks properly introduced.
IncludingadvertisementsByOrgunderQueryis coherent with the schema changes.src/graphql/types/Query/advertisement.ts (1)
137-248: Robust membership checks.
TheadvertisementsByOrgresolver is well-structured, with appropriate authentication and authorization checks. The safe parsing logic ensures input validity, and error handling is consistent with the rest of the code.src/graphql/types/Mutation/updateAdvertisement.ts (4)
2-2: Check compatibility of FileUpload type.
Ensure thegraphql-upload-minimalversion aligns with the rest of the application and is regularly maintained.
3-3: Use of ULID is appropriate.
ULIDs provide a good level of uniqueness for distributed systems.
5-5: Import usage confirmed.
Referencing theadvertisementAttachmentMimeTypeEnumis consistent with your upload validation logic.
78-78: Asynchronous safe parsing is properly applied.
This approach correctly handles the async transformations, though confirm that partial validation failures are acceptable for your use case.
| /** | ||
| * Drizzle orm postgres table definition for advertisements. | ||
| * These file define how data is stored in your PostgreSQL database using Drizzle ORM. | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix grammatical error in the comment.
The comment has a similar grammatical error. "This file" should be used instead of "These file".
/**
- * These file define how data is stored in your PostgreSQL database using Drizzle ORM.
+ * This file defines how data is stored in your PostgreSQL database using Drizzle ORM.
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Drizzle orm postgres table definition for advertisements. | |
| * These file define how data is stored in your PostgreSQL database using Drizzle ORM. | |
| */ | |
| /** | |
| * This file defines how data is stored in your PostgreSQL database using Drizzle ORM. | |
| */ |
| export const imageMimeTypeEnum = z.enum([ | ||
| "image/avif", | ||
| "image/jpeg", | ||
| "image/jpg", | ||
| "image/png", | ||
| "image/webp", | ||
| ]); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider removing redundant MIME type.
"image/jpg" is redundant as "image/jpeg" is already included and is the official MIME type according to IANA standards. Having both might lead to inconsistencies in file handling.
export const imageMimeTypeEnum = z.enum([
"image/avif",
"image/jpeg",
- "image/jpg",
"image/png",
"image/webp",
]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const imageMimeTypeEnum = z.enum([ | |
| "image/avif", | |
| "image/jpeg", | |
| "image/jpg", | |
| "image/png", | |
| "image/webp", | |
| ]); | |
| export const imageMimeTypeEnum = z.enum([ | |
| "image/avif", | |
| "image/jpeg", | |
| "image/png", | |
| "image/webp", | |
| ]); |
| import { z } from "zod"; | ||
| import { builder } from "~/src/graphql/builder"; | ||
|
|
||
| //query to fetch multiple organuisation by organisationID |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix typo in the comment.
The comment has a typo: "organuisation" should be "organization".
-//query to fetch multiple organuisation by organisationID
+// Query to fetch multiple organizations by organizationID📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //query to fetch multiple organuisation by organisationID | |
| // Query to fetch multiple organizations by organizationID |
| @@ -12,14 +12,16 @@ | |||
| import { Advertisement } from "~/src/graphql/types/Advertisement/Advertisement"; | |||
| import { TalawaGraphQLError } from "~/src/utilities/TalawaGraphQLError"; | |||
|
|
|||
| //This file contains the logic for the createAdvertisement mutation.It Handles input validation, database insertion, file uploads to MinIO, and metadata storage for the createAdvertisement mutation. | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve comment formatting.
The comment provides good context but could be formatted better for readability.
-//This file contains the logic for the createAdvertisement mutation.It Handles input validation, database insertion, file uploads to MinIO, and metadata storage for the createAdvertisement mutation.
+// This file contains the logic for the createAdvertisement mutation. It handles:
+// - Input validation
+// - Database insertion
+// - File uploads to MinIO
+// - Metadata storage📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //This file contains the logic for the createAdvertisement mutation.It Handles input validation, database insertion, file uploads to MinIO, and metadata storage for the createAdvertisement mutation. | |
| // This file contains the logic for the createAdvertisement mutation. It handles: | |
| // - Input validation | |
| // - Database insertion | |
| // - File uploads to MinIO | |
| // - Metadata storage |
| @@ -161,7 +164,7 @@ | |||
| }, | |||
| }); | |||
| } | |||
|
|
|||
| //Prevents unauthorized users from creating advertisements | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove redundant comment.
The comment is redundant as the code block below it is self-documenting through the clear variable name currentUserOrganizationMembership and the role check.
-//Prevents unauthorized users from creating advertisements📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //Prevents unauthorized users from creating advertisements |
| 'QueryAdvertisementInput': { kind: 'INPUT_OBJECT'; name: 'QueryAdvertisementInput'; isOneOf: false; inputFields: [{ name: 'id'; type: { kind: 'NON_NULL'; name: never; ofType: { kind: 'SCALAR'; name: 'String'; ofType: null; }; }; defaultValue: null }]; }; | ||
| 'QueryAdvertisementsByOrgInput': { kind: 'INPUT_OBJECT'; name: 'QueryAdvertisementsByOrgInput'; isOneOf: false; inputFields: [{ name: 'organizationId'; type: { kind: 'NON_NULL'; name: never; ofType: { kind: 'SCALAR'; name: 'String'; ofType: null; }; }; defaultValue: null }]; }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider extra validation for organizationId.
While the schema is correct as-is, adding validation (e.g., non-empty checks) might help avoid edge cases with invalid IDs.
| @@ -127,3 +127,122 @@ | |||
| type: Advertisement, | |||
| }), | |||
| ); | |||
| //To Fetch All Advertisment for a particular organistation by user. | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor comment style suggestion.
Consider matching the spelling of "organization" consistently throughout the codebase.
| const queryAdvertisementsByOrgInputSchema = z.object({ | ||
| input: z.object({ | ||
| organizationId: z.string(), | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance input schema checks.
The current schema only requires organizationId to be a string. Consider verifying it’s not empty to handle potential misuse.
| input: mutationUpdateAdvertisementInputSchema.transform(async (arg, ctx) => { | ||
| let attachments: | ||
| | (FileUpload & { | ||
| mimetype: z.infer<typeof advertisementAttachmentMimeTypeEnum>; | ||
| })[] | ||
| | undefined; | ||
| console.log("Raw attachments:", arg.attachments); | ||
| if (arg.attachments !== undefined) { | ||
| const rawAttachments = await Promise.all(arg.attachments); | ||
| const { data, error, success } = advertisementAttachmentMimeTypeEnum | ||
| .array() | ||
| .safeParse(rawAttachments.map((attachment) => attachment.mimetype)); | ||
|
|
||
| if (!success) { | ||
| for (const issue of error.issues) { | ||
| if (typeof issue.path[0] === "number") { | ||
| ctx.addIssue({ | ||
| code: "custom", | ||
| path: ["attachments", issue.path[0]], | ||
| message: `Mime type "${rawAttachments[issue.path[0]]?.mimetype}" is not allowed.`, | ||
| }); | ||
| } | ||
| } | ||
| } else { | ||
| attachments = rawAttachments.map((attachment, index) => | ||
| Object.assign(attachment, { | ||
| mimetype: data[index], | ||
| }), | ||
| ); | ||
| } | ||
| } | ||
| return { | ||
| ...arg, | ||
| attachments, | ||
| }; | ||
| }), |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Console logging & attachment size validation.
While console logging can be helpful during development, consider replacing it with a more formal logging mechanism. Additionally, imposing size/quantity limits on attachments might prevent excessive resource usage.
| if (parsedArgs.input.attachments !== undefined) { | ||
| const attachments = parsedArgs.input.attachments; | ||
| const updatedAdvertisementAttachments = await ctx.drizzleClient | ||
| .insert(advertisementAttachmentsTable) | ||
| .values( | ||
| attachments.map((attachment) => ({ | ||
| advertisementId: updatedAdvertisement.id, | ||
| creatorId: currentUserId, | ||
| mimeType: attachment.mimetype, | ||
| name: ulid(), | ||
| })), | ||
| ) | ||
| .onConflictDoNothing() | ||
| .returning(); | ||
|
|
||
| if (Array.isArray(updatedAdvertisementAttachments)) { | ||
| await Promise.all( | ||
| updatedAdvertisementAttachments.map((attachment, index) => { | ||
| if (attachments[index] !== undefined) { | ||
| return ctx.minio.client.putObject( | ||
| ctx.minio.bucketName, | ||
| attachment.name, | ||
| attachments[index].createReadStream(), | ||
| undefined, | ||
| { | ||
| "content-type": attachments[index].mimetype, | ||
| }, | ||
| ); | ||
| } | ||
| }), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve error handling & concurrency control.
Inserting attachments and uploading to MinIO is solid. Consider handling partial failures in a transaction or limiting concurrency so that one failing upload doesn’t leave the database in an inconsistent state.
|
Okay I will try to cover this rabbit ai requested changes and add commits thanks |
|
|
Closing based on comments from @xoldd |
|
@palisadoes Sir but in this pr I also had worked on attachments to be fetched and uploaded succesfully. I know we dont want to do it from backend but still untill we solve the issue of uploading through frontend we may let it work? let me know your opinions please. |
|
Submit an updated PR based on the approach from @xoldd |
What kind of change does this PR introduce?
Code refactoring
Issue Number:
Fixes #3242
Snapshots/Videos:
https://www.loom.com/share/7988c722116945b892a7f7ddf8cd174b?sid=fed21822-8831-41ce-9637-707ba51a4a61
Summary
This PR enables the admin app to fetch multiple advertisement,storeimages and integrate with talawa admin app for advertisement section.
Does this PR introduce a breaking change?
No
Checklist
CodeRabbit AI Review
Test Coverage
Other information
Have you read the contributing guide?
Yes
Summary by CodeRabbit