Implementing Attachments #18
Conversation
mliao95
force-pushed
the
mikeliao/attachments
branch
from
2b6090a to
2afa0a1
Compare
| } | ||
| } | ||
|
|
||
| self.httpClient.putJson((response.uploadMetadata?.url)!, headersToInclude, fileData) { |
There was a problem hiding this comment.
Shall we move this entire API calling logic to a separate file? ApiProtocol + ApiClient?
The reason i am thinking is because we are intending chatService to act as a router file and making actual call here might be misleading when we are expanding. We can also put the metrics uploading api there?
There was a problem hiding this comment.
Created APIProtocol and APIClient
| switch result { | ||
| case .success(let response): | ||
| if let url = URL(string: response.url!) { | ||
| self.downloadFile(url: url, filename: filename) { (localUrl, error) in |
There was a problem hiding this comment.
Shall we do this or instead let client take care of it?
Reason: They might wanna show progress/notification for downloading large files etc.
There was a problem hiding this comment.
I would imagine downloading would fall more under business logic than UI logic, so I think the right place for it would still be the SDK. This might be more of a niche case considering attachments are capped at 20MB. We can consider exposing GetAttachment API in addition to this one in the future in case customers want to trigger and manage their own download.
There was a problem hiding this comment.
That is true, we can do that. My worry was for large files and user not knowing about how is it being downloading after they hit getAttachment. Like these two operations could take significant time based on n/w. Shall we consider a callback for progress update or similar?
There was a problem hiding this comment.
Added getAttachmentDownloadUrl for customers who want to handle the download themselves
| let tempFilePathUrl = tempDirectory.appendingPathComponent(filename) | ||
|
|
||
|
|
||
| if FileManager.default.fileExists(atPath: tempFilePathUrl.path) { |
There was a problem hiding this comment.
Are we requesting any sort of permission to access it? If thats the case then we should let client take care of it imo
There was a problem hiding this comment.
FileManager.default.temporaryDirectory lives within the app's sandbox and doesn't persist across sessions/apps, so no need to request additional permissions.
| chatService.downloadAttachment(attachmentId: attachmentId, filename: filename) { result in | ||
| switch result { | ||
| case .success(let localUrl): | ||
| print("Local URL: \(String(describing: localUrl))") |
There was a problem hiding this comment.
nit: was it intentional? I am looking out for any app sec flags
| case doc = "application/msword" | ||
| case docx = "application/vnd.openxmlformats-officedocument.wordprocessingml.document" | ||
| case heic = "image/heic" | ||
| case jpg = "image/jpeg" |
There was a problem hiding this comment.
is there a difference between jpeg and jpg? also do we need to add jfif?
There was a problem hiding this comment.
.jpg , .jpeg , .jpe , .jif , .jfif all map to image/jpeg MIME type.
| /// - completion: Completion handler to handle the success status or error. | ||
| func completeAttachmentUpload(connectionToken: String, attachmentIds: [String], completion: @escaping (Result<AWSConnectParticipantCompleteAttachmentUploadResponse, Error>) -> Void) | ||
|
|
||
| func getAttachment(connectionToken: String, attachmentId: String, completion: @escaping (Result<AWSConnectParticipantGetAttachmentResponse, Error>) -> Void) |
| return nil | ||
| } | ||
| let time = CommonUtils().formatTime(innerJson["AbsoluteTime"] as! String)! | ||
| switch type { |
There was a problem hiding this comment.
The guard statement above will execute the else block if either typeString or type is nil (null).
guard let typeString = innerJson["Type"] as? String, let type = WebSocketMessageType(rawValue: typeString) else {
print("Unknown websocket message type: \(String(describing: innerJson["Type"]))")
return nil
}
| return nil | ||
| } | ||
|
|
||
| let message = Message( |
There was a problem hiding this comment.
For this function, if an attachment message comes through the websocket, we translate the data into a Message object for the customer to then parse in the UI.
Pretty much instead of letting the customer parse the JSON themselves, they can access message properties via message.text or message.timeStamp
| @@ -7,6 +7,6 @@ import AWSCore | |||
| // Temporary using this file untill we figure out how to separate sender and reciever based on thier name or role. | |||
|
|
|||
| class Config { | |||
| let isDevMode = false // TODO: Set up isDevMode | |||
| let isDevMode = true // TODO: Set up isDevMode | |||
| } | ||
| } | ||
|
|
||
| // Print the entire request for debugging |
There was a problem hiding this comment.
are we going to remove this in the future? or at least redact sensitive info like connection token??
There was a problem hiding this comment.
Yeah we will remove this after pen-testing concludes. They want all logging enabled for now
mliao95
force-pushed
the
mikeliao/attachments
branch
from
2c54205 to
e46a6ae
Compare
Issue #, if available:
Description of changes:
sendAttachmentStartAttachmentUpload, PUT request to S3 bucket,CompleteAttachmentUploadgetAttachmentDownloadUrl:GetAttachmentConnectParticipantAPI and returns URL for user to download attachmentdownloadAttachment:getAttachmentDownloadUrlto get download link, downloads file to temporary local storage, returns file URL of temporarily stored file.gitignoreto directory rootBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.