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
[CIS-1181] Implement uploadFile API #1468
Conversation
@@ -33,6 +33,13 @@ public struct FileAttachmentPayload: AttachmentPayload { | |||
.flatMap { try? JSONEncoder.stream.encode($0) } | |||
.flatMap { try? JSONDecoder.stream.decode(T.self, from: $0) } | |||
} | |||
|
|||
public init(title: String?, assetURL: URL, file: AttachmentFile, extraData: [String: RawJSON]?) { |
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.
The public init
looks safe but it actually opens the door for a miss-use. When ***AttachmentPayload
is instantiated manually there's a chance it is passed to AnyAttachmentPayload.init
designed for custom non-uploadable attachments:
init<Payload: AttachmentPayload>(payload: Payload) {
self.init(
type: Payload.type,
payload: payload,
localFileURL: nil
)
}
instead of one designed for built-in uploadable attachments:
init(
localFileURL: URL,
attachmentType: AttachmentType,
extraData: Encodable? = nil
) { ... }
Attachment without localURL
are not picked up for uploading and will be sent to the backend as is (with local URLs) 🥲
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.
IMHO that's safe, developers don't need to create built-in payload instances unless they handle the uploads themselves.
override func addAttachmentToContent(from url: URL, type: AttachmentType) throws {
if pickingEncryptedMedia {
pickingEncryptedMedia = false
// Encrypt the attachment
// Upload the attachment
channelController?.uploadFile(type: type, localFileURL: url, completion: { result in
do {
let uploadedURL = try result.get()
// Append already uploaded attachment
let attachment = AnyAttachmentPayload(
payload: ImageAttachmentPayload(
title: nil,
imageURL: uploadedURL,
extraData: nil
)
)
DispatchQueue.main.async {
self.content.attachments.append(attachment)
}
} catch {
// Handle error
log.error("Failed uploading attachment: \(error)")
}
})
} else {
try super.addAttachmentToContent(from: url, type: type)
}
}
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.
They do not need but the API allows it, that's what I was saying 🙂
That's the correct way:
let attachment = try AnyAttachmentPayload(localFileURL: url, attachmentType: .image)
content.attachments.append(attachment)
That's the 💣
let attachment = AnyAttachmentPayload(
payload: ImageAttachmentPayload(
title: nil,
imageURL: url,
extraData: nil
)
)
content.attachments.append(attachment)
Codecov Report
@@ Coverage Diff @@
## main #1468 +/- ##
=======================================
Coverage 88.24% 88.25%
=======================================
Files 228 228
Lines 10364 10418 +54
=======================================
+ Hits 9146 9194 +48
- Misses 1218 1224 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Can you include code examples about this? It is not 100% obvious why this is not going via the CDN class
@tbarbugli It's actually going through the CDN class, just via |
e0957bc
to
b522f21
Compare
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.
Looks good 👍
It'll allow uploading a file to CDN without having to upload an attachment to a message
It'll allow uploading a file to CDN without having to upload an attachment to a message