addedSupport for edit of AdvertisementAttachment #3333
Conversation
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
|
WalkthroughThe changes add a new Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant G as GraphQL Endpoint
participant R as Mutation Resolver
participant A as Attachment Processor
participant S as Storage Service
C->>G: Send updateAdvertisement mutation with attachments
G->>R: Parse input via mutationUpdateAdvertisementInputSchema
R->>A: Asynchronously validate and process attachments
A-->>R: Return validated attachment files
R->>S: Upload attachments to storage
S-->>R: Confirm uploads
R->>G: Return updated advertisement with attachments
G-->>C: Response 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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3333 +/- ##
====================================================
- Coverage 49.45% 49.38% -0.08%
====================================================
Files 462 462
Lines 34776 34856 +80
Branches 1038 1038
====================================================
+ Hits 17199 17212 +13
- Misses 17577 17644 +67 ☔ 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: 5
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
schema.graphql(1 hunks)src/graphql/inputs/MutationUpdateAdvertisementInput.ts(2 hunks)src/graphql/types/Mutation/updateAdvertisement.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Deployment to https://docs-api.talawa.io
🔇 Additional comments (10)
src/graphql/inputs/MutationUpdateAdvertisementInput.ts (4)
1-2: Imports look coherent
The usage ofFileUploadfrom"graphql-upload-minimal"andzfrom"zod"aligns well with the rest of the codebase.
10-10: Confirm correctness of making all fields optional
Using.partial()makes every advertisement field optional. Verify that this is intentional, especially if some fields should remain mandatory.
39-41: Time range validation
CheckingendAt > startAtcorrectly prevents invalid time ranges. If your setup involves different time zones, confirm this logic remains valid across them.
45-45: Clearer error message
The updated error message precisely states thatendAtmust be greater thanstartAt. This improves clarity for end users.src/graphql/types/Mutation/updateAdvertisement.ts (6)
2-3: New import usage
AddingFileUploadandulidreferences is aligned with the new attachment-handling workflow.
5-6: Additional advertisement attachment references
ImportingadvertisementAttachmentMimeTypeEnumandadvertisementAttachmentsTableis crucial for validating and storing attachments. Confirm that the enum covers all acceptable file types.
17-17: Asynchronous transform for input
Applying an async transform is a clean way to validate and normalize attachments before proceeding with the remaining logic.
77-77: Asynchronous input parsing
UsingsafeParseAsyncmatches your async validation approach. This ensures the final parsed object is accurate before proceeding.
249-252: Handle unexpected database behavior
Logging and throwing an error when the update unexpectedly returns an empty array helps with debugging.
293-293: Final advertisement return
Returning the updated advertisement immediately provides clients with the latest data, ensuring they can confirm their changes.
| """Attachments of the advertisement.""" | ||
| attachments: [Upload!] | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add clarifying documentation on optional attachments
Currently, the schema indicates a list of non-null Uploads, yet it is also treated as an optional field. Consider making it explicit in the docstring or adjusting the schema to clarify how and when attachments may be omitted entirely.
| id: advertisementsTableInsertSchema.shape.id.unwrap(), | ||
| attachments: z | ||
| .custom<Promise<FileUpload>>() | ||
| .array() | ||
| .min(1) | ||
| .max(20) | ||
| .optional(), |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Ensure alignment of ID unwrapping and attachments constraints
The id field is now mandatory (unwrap()), while attachments is optional but must contain at least one entry when provided. This approach is consistent. However, you might also consider specifying file size or other limits here (as opposed to solely downstream).
| .superRefine(({ attachments, ...remainingArgs }, ctx) => { | ||
| if (Array.isArray(attachments) && attachments.length === 0) { | ||
| attachments = undefined; | ||
| } | ||
| if ( | ||
| !Object.values(remainingArgs).some((value) => value !== undefined) && | ||
| attachments === undefined | ||
| ) { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Validate behavior for empty updates
Your superRefine requires at least one non-empty field, preventing no-op updates. However, this also prevents passing an empty attachments array (e.g., for removals). If removing attachments is desirable, consider a dedicated strategy for that.
| let attachments: | ||
| | (FileUpload & { | ||
| mimetype: z.infer<typeof advertisementAttachmentMimeTypeEnum>; | ||
| })[] | ||
| | undefined; | ||
| 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.
🛠️ Refactor suggestion
Asynchronous attachment normalization
Gathering and validating the attachments’ MIME types in a single pass is effective. Consider adding safeguards for:
- Large file sizes (resource exhaustion).
- Partial success scenarios (e.g., if only some attachments fail).
| 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.
🛠️ Refactor suggestion
Parallel object uploads
While uploading attachments in parallel is typically faster, consider:
- Handling partial failures if individual uploads fail.
- Checking resource limits (e.g., large files).
- Logging errors from each upload to improve observability.
|
Please fix the first comment so that each issue listed automatically closes. The PR_GUIDELINES.md file has details. Please also fill in the template for the pull request as completely as you can. It greatly helps others searching for code changes in future and helps others understand the summarized rationale for the work you have done Also make sure the title is easily searchable for terms related to what the PR is about. The related issue also needs to be stated. |
What kind of change does this PR introduce?
refactoring
Issue Number:
In response to updatedPR #3293
Snapshots/Videos:
https://www.loom.com/share/7988c722116945b892a7f7ddf8cd174b?sid=fed21822-8831-41ce-9637-707ba51a4a61
If relevant, did you update the documentation?
No
Summary
Earlier we were not able to edit advertisement due to no minio client in updateAdvertisment.ts
I added the necessary changes for people to edit advertsement.
Does this PR introduce a breaking change?
No
Checklist
CodeRabbit AI Review
Test Coverage
Other information
Have you read the contributing guide?
Summary by CodeRabbit