Skip to content
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-2262] Allow changing attachment info with custom CDN #2369

Merged

Conversation

nuno-vieira
Copy link
Member

@nuno-vieira nuno-vieira commented Nov 9, 2022

🔗 Issue Links

CIS-2262

🎯 Goal

Currently, our CDNClient protocol expects always to return a single URL. This is very limited since a customer might want to update the attachment info using their file uploader API, like changing the preview URL or adding extra data.

The goal of this PR is to improve this and allow the customer to change the final attachment when uploading to their CDN.

🛠 Implementation

  • Introduces a new AttachmentUploader and renames the old one to AttachmentQueueUploader.
  • channelController.uploadFile() and channelController.uploadImage() are now deprecated in favour of channelController.uploadAttachment()

The CDNClient can be customized if you are only interested in changing the CDN Url and do not want to update the attachment payload.

The AtttachmentUploader can be customized for more fine-grain control, in case you want to update the final attachment with additional information.

🧪 Manual Testing Notes

Probably worth doing smoke testing related to loading images.

☑️ Contributor Checklist

  • I have signed the Stream CLA (required)
  • This change follows zero ⚠️ policy (required)
  • This change should be manually QAed
  • Changelog is updated with client-facing changes
  • New code is covered by unit tests
  • Comparison screenshots added for visual changes
  • Affected documentation updated (docusaurus, tutorial, CMS)

@nuno-vieira nuno-vieira added 🌐 SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK ✨ Enhancement A request to improve the SDK labels Nov 9, 2022
@Stream-iOS-Bot
Copy link
Collaborator

Stream-iOS-Bot commented Nov 9, 2022

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@nuno-vieira nuno-vieira added the 💥 Breaking Changes A PR that contains breaking changes label Nov 9, 2022
@nuno-vieira nuno-vieira force-pushed the add/CIS-2262-allow-changing-attachment-info-with-custom-cdn branch 2 times, most recently from 1c4d1ba to d84d98e Compare November 9, 2022 01:06
@nuno-vieira nuno-vieira removed the 💥 Breaking Changes A PR that contains breaking changes label Nov 9, 2022
@nuno-vieira nuno-vieira force-pushed the add/CIS-2262-allow-changing-attachment-info-with-custom-cdn branch from 57a0ed5 to a4590d0 Compare November 9, 2022 12:38
@emerge-tools
Copy link

emerge-tools bot commented Nov 10, 2022

📏 Size Analysis

Total install size 10.6 MB | This change: ⬆️ +18.3 kB (+0.172%)

Image of diff

🗂 See size breakdown
Item Install size
➕ StreamChat.framework/StreamChat.AnyAttachmentUpdater.update(forPayload) ⬆️ 5.0 kB
➕ StreamChat.framework/StreamChat.UploadedAttachment.value witness ⬆️ 3.7 kB
➖ StreamChat.framework/StreamChat.ChatChannelController.uploadFile(localFileURL,progress,completion) ⬇️ 3.6 kB
➖ StreamChat.framework/StreamChat.AttachmentDTO.update(uploadedFileURL) ⬇️ 3.6 kB
➖ StreamChat.framework/StreamChat.ChatChannelController.uploadImage(localFileURL,progress,completion) ⬇️ 3.2 kB
➕ StreamChat.framework/StreamChat.AttachmentQueueUploader.updateAttachmentIfNeeded(attachmentId,uploadedAttachment,newState,completion) ⬆️ 3.2 kB
➕ StreamChat.framework/StreamChat.ChatChannelController.uploadAttachment(localFileURL,type,progress,completion) ⬆️ 2.9 kB
➕ StreamChat.framework/StreamChat.AttachmentQueueUploader.uploadNextAttachment ⬆️ 2.8 kB
➖ StreamChat.framework/StreamChat.AttachmentUploader.uploadNextAttachment ⬇️ 2.6 kB
➖ StreamChat.framework/StreamChat.AttachmentUploader.updateAttachmentIfNeeded(newState,attachmentUpdates,completion) ⬇️ 2.1 kB
➕ StreamChat.framework/StreamChat.UploadedFile.value witness ⬆️ 1.6 kB
➕ StreamChat.framework/StreamChat.AttachmentQueueUploader.startObserving ⬆️ 1.6 kB
➖ StreamChat.framework/StreamChat.AttachmentUploader.startObserving ⬇️ 1.6 kB
StreamChat.framework/DYLD.Exports ⬆️ 1.3 kB
➕ StreamChat.framework/StreamChat.APIClient.init(sessionConfiguration,requestEncoder,requestDecoder,attachmentUploader,tokenRefresher,queueOfflineRequest) ⬆️ 1.1 kB
➖ StreamChat.framework/StreamChat.APIClient.init(sessionConfiguration,requestEncoder,requestDecoder,CDNClient,tokenRefresher,queueOfflineRequest) ⬇️ 1.1 kB
StreamChat.framework/StreamChat.FileUploadPayload.value witness ⬆️ 1.1 kB
StreamChat.framework/StreamChat.ImageAttachmentPayload.value witness 740 B
➕ StreamChat.framework/StreamChat.StreamAttachmentUploader.upload(progress,completion) 660 B
StreamChat.framework/Code Signature 520 B

🔎 See the full size analysis (fb4e383) merging into develop (d6c94f3)

Provide a baseSha and pullRequestNumber with your upload to ensure diffs are correct. Read more in the docs

@nuno-vieira nuno-vieira force-pushed the add/CIS-2262-allow-changing-attachment-info-with-custom-cdn branch from fb4e383 to 48219c5 Compare November 10, 2022 00:56
@nuno-vieira nuno-vieira marked this pull request as ready for review November 10, 2022 01:02
@nuno-vieira nuno-vieira requested a review from a team as a code owner November 10, 2022 01:02
@nuno-vieira nuno-vieira force-pushed the add/CIS-2262-allow-changing-attachment-info-with-custom-cdn branch 2 times, most recently from 0da3d14 to fec0039 Compare November 10, 2022 17:57
Copy link
Contributor

@martinmitrevski martinmitrevski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in general. I'm only worried about this line:
let previewUrl = try container.decodeIfPresent(URL.self, forKey: .thumbURL) ?? imageURL.
It seems we were previously decoding the thumbURL and setting the previewURL after all. Now this logic is gone, which might be a breaking change.

@nuno-vieira
Copy link
Member Author

Looks good in general. I'm only worried about this line:

let previewUrl = try container.decodeIfPresent(URL.self, forKey: .thumbURL) ?? imageURL.

It seems we were previously decoding the thumbURL and setting the previewURL after all. Now this logic is gone, which might be a breaking change.

This does not exist, our API does not return any thumb_url for Images. Also, our ImageAttachmentPayload does not encode that value, so it would never be sent to the server even if has a customer you updated the previewUrl. So this is fine to remove

@nuno-vieira nuno-vieira force-pushed the add/CIS-2262-allow-changing-attachment-info-with-custom-cdn branch from fec0039 to 6975ff4 Compare November 11, 2022 14:48
@nuno-vieira nuno-vieira force-pushed the add/CIS-2262-allow-changing-attachment-info-with-custom-cdn branch from 8da7bb0 to 9626c96 Compare November 11, 2022 14:59
Copy link
Contributor

@martinmitrevski martinmitrevski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work @nuno-vieira 👏

@sonarcloud
Copy link

sonarcloud bot commented Nov 14, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 10 Code Smells

86.8% 86.8% Coverage
0.0% 0.0% Duplication

@nuno-vieira nuno-vieira merged commit 1309c15 into develop Nov 14, 2022
@nuno-vieira nuno-vieira deleted the add/CIS-2262-allow-changing-attachment-info-with-custom-cdn branch November 14, 2022 14:48
This was referenced Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Enhancement A request to improve the SDK 🌐 SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants