-
Notifications
You must be signed in to change notification settings - Fork 0
[TM-2769] add bulk upload media #478
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| import { ApiProperty } from "@nestjs/swagger"; | ||
| import { JsonApiDto } from "@terramatch-microservices/common/decorators"; | ||
|
|
||
| @JsonApiDto({ type: "mediaBulkResponse" }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resource types should be plural: mediaBulkResponses
| return this.entitiesService.mediaDto(media, { entityType: media.modelType as EntityType, entityUuid: model.uuid }); | ||
| } | ||
|
|
||
| @Post("/site/:siteUuid/bulkUpload") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be sites. In the end though, I don't love how specific this is to just sites photo collections. It would be a lot better to have the current uploadFile method be able to accept a bulk body in addition to the single upload body. A bulkUpload is not a resource - it's an action, so this endpoint is constructed like an RPC method, not a REST endpoint. There are a few other places (like in the form data provider) where we handle bulk uploads by issuing multiple requests at the same time.
I would say this bulk upload should either be made generic so that it can be done for any entity / collection, or this photo upload bulk process should use that same client side pattern being used in the link above.
If you decide to make this endpoint generic for all entities and want help getting the body parameter to work correctly with two different possible types, let me know. I imagine that will be a little tricky to wire up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh... I just remembered this is for Flority. Does this ticket need to be in the current release? I want to make the current endpoint more adaptable (perhaps it only accepts "bulk" uploads, but sometimes the client only sends a single file, which might be simpler), but I think that's going to be enough additional work that it will delay the release if we try to include it.
I'm going to hold off on reviewing the rest of this PR for now.
Task Link