Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

Revert "feat: add dm notify method (#660)" #662

Merged
merged 1 commit into from
Sep 11, 2023
Merged

Conversation

d-gubert
Copy link
Member

@d-gubert d-gubert commented Sep 11, 2023

This reverts commit dbb7e7c

It breaks quite a few conventions of the API - this makes the framework less intuitive, harder to find where things are (and it already is relatively difficult because we don't follow widespread dev patterns).

Having a method that sends a message on a Read accessor breaks the intended separation of concern of the API - and furthermore, the method has been added to the Notifier accessor, but it doesn't deal with a notification and rather sends a persistent message.

The IDirectMessage interface replaces the full room object with a three field object which does not conform to other points in the framework as well - and the concept of "Direct" in Rocket.Chat refers to a Room, not a Message.

A more flexible signature for the sendDirectMessage method would be to pass a message and a tuple of users. The method would then determine whether to create a new Direct room to send the message to or not.

To workaround the IPre... events limitation, we can start sending the IModify instance as a parameter to the event handler. It wasn't being sent before as a concern around performance, as this would give the app more surface to slow down whatever process they were being executed before (IPreMessageSent... potentially slowing down the process of sending messages, for instance). But considering the HTTP accessor is being passed as well, which apps can use to even call themselves in the server, we can argue on how effective the previous reasoning was.

@d-gubert d-gubert merged commit b2a842d into alpha Sep 11, 2023
8 checks passed
@d-gubert d-gubert deleted the revert-660-new/dmnotify branch September 11, 2023 18:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant