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

Add support for sending system messages client-side #3477

Merged
merged 10 commits into from
Oct 30, 2024

Conversation

nuno-vieira
Copy link
Member

@nuno-vieira nuno-vieira commented Oct 30, 2024

🔗 Issue Links

Resolves https://stream-io.atlassian.net/browse/PBE-4505

🎯 Goal

Add support for sending system messages from the SDK.

📝 Summary

  • Adds support for sending system messages from the SDK
  • Makes sure that when sending system messages with skip the last message date enabled, it does not update the lastMessageAt locally

🎨 Showcase

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-10-30.at.15.48.53.mp4

🧪 Manual Testing Notes

Regular Case:
1- Open the Demo App
2- Open the Channel Actions
3- Tap on Send a system message
4- A system message should be rendered on the channel
5- The channel list order should be updated

Skipping System lastMessageAt Case:
1- Open the Demo App
2- Login with Leia Organa
3- Send a system message in the SKipppy channel
4- The channel list order should NOT be updated

☑️ Contributor Checklist

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

@nuno-vieira nuno-vieira requested a review from a team as a code owner October 30, 2024 15:51
@Stream-SDK-Bot
Copy link
Collaborator

Stream-SDK-Bot commented Oct 30, 2024

SDK Size

title develop branch diff status
StreamChat 6.92 MB 6.92 MB 0 KB 🟢
StreamChatUI 4.95 MB 4.95 MB 0 KB 🟢

@Stream-SDK-Bot
Copy link
Collaborator

SDK Performance

target metric benchmark branch performance status
MessageList Hitches total duration 10 ms 1.67 ms 83.3% 🔼 🟢
Duration 2.6 s 2.54 s 2.31% 🔼 🟢
Hitch time ratio 4 ms per s 0.65 ms per s 83.75% 🔼 🟢
Frame rate 75 fps 78.46 fps 4.61% 🔼 🟢
Number of hitches 1 0.2 80.0% 🔼 🟢

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. I added one suggestion for the db handling, let me know what you think.
Additionally, we need some usage docs before we merge it.

@@ -38,6 +38,10 @@ class MessageDTO: NSManagedObject {
@NSManaged var replyCount: Int32
@NSManaged var extraData: Data?
@NSManaged var isSilent: Bool

// Used for creating a message as a system message from the client SDK.
@NSManaged var isSystem: Bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it better to use the optional type directly here? If we need to use it for some other type, we would need to use another helper var like this one.

Copy link
Member Author

Choose a reason for hiding this comment

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

@martinmitrevski I've removed the isSystem prop. The reason I added this, was because when using the message for the HTTP Request, we should only send the type if it is a system message, but now I check if the type is system, and if yes, then we send it. So the isSystem is not really needed 👍

Sources/StreamChat/Database/DTOs/MessageDTO.swift Outdated Show resolved Hide resolved
@nuno-vieira
Copy link
Member Author

Looks good. I added one suggestion for the db handling, let me know what you think. Additionally, we need some usage docs before we merge it.

@martinmitrevski We probably should only update the CMS docs, I don't think updating the UI docs is very valuable 🤔 Also, where would we put this on the UI docs?

@nuno-vieira nuno-vieira force-pushed the add/support-sending-system-messages branch from cb17cb1 to 6d4febe Compare October 30, 2024 17:23
@nuno-vieira nuno-vieira force-pushed the add/support-sending-system-messages branch from 6d4febe to e5c1ed7 Compare October 30, 2024 17:24
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.

LGTM! About the docs, yes, only the CMS docs. I think Android has this already, so you can follow the same format.

@Stream-SDK-Bot
Copy link
Collaborator

Stream-SDK-Bot commented Oct 30, 2024

SDK Size

title develop branch diff status
StreamChat 6.92 MB 6.92 MB 0 KB 🟢
StreamChatUI 4.95 MB 4.95 MB 0 KB 🟢

@nuno-vieira nuno-vieira enabled auto-merge (squash) October 30, 2024 17:39
Copy link

sonarcloud bot commented Oct 30, 2024

@nuno-vieira nuno-vieira merged commit 1555103 into develop Oct 30, 2024
15 checks passed
@nuno-vieira nuno-vieira deleted the add/support-sending-system-messages branch October 30, 2024 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants