-
Notifications
You must be signed in to change notification settings - Fork 218
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 Message Reminders #3623
base: develop
Are you sure you want to change the base?
Add Message Reminders #3623
Conversation
Generated by 🚫 Danger |
func eventsController(_ controller: EventsController, didReceiveEvent event: Event) { | ||
if AppConfig.shared.demoAppConfig.isRemindersEnabled, | ||
let reminderDueEvent = event as? ReminderDueEvent { | ||
handleReminderDueEvent(reminderDueEvent) | ||
} | ||
} |
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.
I'm not sure if this is going to stay the same since the goal is for the backend to send a remote push notification.
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 doesn't feel complete without PN. Atm, we listen to WS event, and send local notification. But what if there's no active WS, do you know when push notifications would be supported?
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.
Yes the Push needs to come from the server, I will delete this one that is done 👍
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.
do you know when that would be ready?
SDK Size
|
var onLogout: (() -> Void)? | ||
var onDisconnect: (() -> Void)? | ||
|
||
private let currentUserController: CurrentChatUserController |
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.
At the moment, one controller is managing the different types of reminders. Although this works fine, we could also use a controller per reminder type. This way we would avoid having to many loadReminders requests. Since right now, whenever we switch the filter, we re-refetch the reminders. So yeah I might optimize this. WDYT?
Although this means we will have 4/5 currentUserControllers. Which is not very intuitive though 🤔 Maybe for the reminders feature it could be worth having a MessageReminderListController
instead of using the current user controller which will observer unread counts etc..
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.
I think it's fine to use one for the demo app - it shows how to use the API. It's up to customers to further optimize it, some might have different / less filters.
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 thing is that we should show them how to optimize their integration code. This means less calls, and less stress to our servers 🤔
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.
I wrote a longer reply to our Slack thread with a summary of supporting the idea of MessageReminderListController (mainly because reminders query has a filter)
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.
ok, that's also fine with me 👍
SDK Performance
|
SDK Size
|
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 great so far 👍 I think the most important thing is to check how / when push notifications will be supported - the due feature doesn't make much sense without it.
Additionally, apart from docs, we need a notion doc for other SDKs.
@@ -50,7 +52,8 @@ class AppConfig { | |||
isLocationAttachmentsEnabled: false, | |||
tokenRefreshDetails: nil, | |||
shouldShowConnectionBanner: false, | |||
isPremiumMemberFeatureEnabled: false | |||
isPremiumMemberFeatureEnabled: false, | |||
isRemindersEnabled: false |
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.
We should have it on by default
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 are disabled, and enabled by default using our Developers Scheme. Since the design is not approved by our Designers, it is probably not a good idea to make it enabled by default 🤔 Although that applies to drafts as well
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.
I would enable it for the default scheme as well. It helps with discoverability, and, let's be honest, you did a great job in the design part!
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.
Ahah thanks, okay I will enable it by default 👍
func eventsController(_ controller: EventsController, didReceiveEvent event: Event) { | ||
if AppConfig.shared.demoAppConfig.isRemindersEnabled, | ||
let reminderDueEvent = event as? ReminderDueEvent { | ||
handleReminderDueEvent(reminderDueEvent) | ||
} | ||
} |
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 doesn't feel complete without PN. Atm, we listen to WS event, and send local notification. But what if there's no active WS, do you know when push notifications would be supported?
var onLogout: (() -> Void)? | ||
var onDisconnect: (() -> Void)? | ||
|
||
private let currentUserController: CurrentChatUserController |
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.
I think it's fine to use one for the demo app - it shows how to use the API. It's up to customers to further optimize it, some might have different / less filters.
path: .reminders, | ||
method: .post, | ||
queryItems: nil, | ||
requiresConnectionId: false, |
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.
how do we receive reminder events if we don't require connection id?
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.
Yeah this might need to be true, will need to recheck
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.
Although, I think reminders will be receive events even if you don't query them. Since you might not have a reminders list. But still want to get reminded of your reminders. So this is not needed AFAIK
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.
ok, just checking - if it works without the connection id we can resolve this one.
@@ -382,3 +386,68 @@ public struct Command: Codable, Hashable { | |||
self.args = args | |||
} | |||
} | |||
|
|||
/// An object describing a reminder JSON payload. | |||
struct ReminderPayload: Decodable { |
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.
As mentioned on Slack, let's try using class here.
static var createdAt: FilterKey<Scope, Date> { .init(rawValue: "created_at", keyPathString: #keyPath(MessageReminderDTO.createdAt)) } | ||
} | ||
|
||
public extension Filter where Scope: AnyMessageReminderListFilterScope { |
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.
how did you come up with these filters?
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.
It is in the Notion's Backend Specs
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.
Ah, or do you mean the helpers? If you mean the helpers, it is because it will be the most common filters, so I think it should be useful for customers
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.
Copilot reviewed 35 out of 55 changed files in this pull request and generated no comments.
Files not reviewed (20)
- DemoApp/Screens/AppConfigViewController/AppConfigViewController.swift: Language not supported
- DemoApp/Screens/DemoAppTabBarController.swift: Language not supported
- DemoApp/Shared/DemoUsers.swift: Language not supported
- DemoApp/Shared/StreamChatWrapper.swift: Language not supported
- DemoApp/StreamChat/Components/DemoChatMessageActionsVC.swift: Language not supported
- DemoApp/StreamChat/Components/DemoChatMessageContentView.swift: Language not supported
- DemoApp/StreamChat/Components/DemoChatMessageLayoutOptionsResolver.swift: Language not supported
- DemoApp/StreamChat/DemoAppCoordinator+DemoApp.swift: Language not supported
- Sources/StreamChat/APIClient/Endpoints/EndpointPath+OfflineRequest.swift: Language not supported
- Sources/StreamChat/APIClient/Endpoints/EndpointPath.swift: Language not supported
- Sources/StreamChat/APIClient/Endpoints/MessageEndpoints.swift: Language not supported
- Sources/StreamChat/APIClient/Endpoints/Payloads/MessagePayloads.swift: Language not supported
- Sources/StreamChat/ChatClient+Environment.swift: Language not supported
- Sources/StreamChat/ChatClient.swift: Language not supported
- Sources/StreamChat/ChatClientFactory.swift: Language not supported
- Sources/StreamChat/Controllers/CurrentUserController/CurrentUserController.swift: Language not supported
- Sources/StreamChat/Controllers/MessageController/MessageController.swift: Language not supported
- Sources/StreamChat/Database/DTOs/MessageDTO.swift: Language not supported
- Sources/StreamChat/Database/DTOs/MessageReminderDTO.swift: Language not supported
- Sources/StreamChat/Database/DatabaseSession.swift: Language not supported
60495fe
to
1eca9b2
Compare
|
🔗 Issue Links
IOS-696
🎯 Goal
Add message reminders feature. This will allow users to save messages for later and get notified about them.
📝 Summary
New APIs:
Filter.isNil
to make it easier to filter nil valuesChatMessageController.createReminder()
ChatMessageController.updateReminder()
ChatMessageController.deleteReminder()
ChatCurrentUserController.loadReminders()
+loadMoreReminders()
Demo App new features:
Bonus:
Filter+ChatChannel.swift
toFilter+predicate
so that we can start using it for other data and not only channels.🛠 Implementation
TODO
🎨 Showcase
TODO
🧪 Manual Testing Notes
In order to test the feature, you need to change the API Key to Frankfurt C2 Staging:
TODO
☑️ Contributor Checklist
docs-content
repo