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

feature/delete-message #428

Open
wants to merge 4 commits into
base: 3.0-mountable
Choose a base branch
from

Conversation

emorissettegregoire
Copy link
Contributor

@emorissettegregoire emorissettegregoire commented Dec 23, 2024

Allow message deletion by passing the message timestamp to the delete_message method

Copy link
Member

@matthewblack matthewblack left a comment

Choose a reason for hiding this comment

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

At this point, we should be looking at all this code as the extendable version of Stealth. Even though, we don't have to build the actual functionality we shouldn't be making code that we will have to peal back later once we get to SMS/Bandwidth.

Hopefully that makes sense!

app/controllers/stealth/controller/replies.rb Outdated Show resolved Hide resolved
app/controllers/stealth/controller/replies.rb Outdated Show resolved Hide resolved
@emorissettegregoire
Copy link
Contributor Author

Yes absolutely, my bad. Will do! Thanks

@kladaFOX
Copy link

DIdn't get the idea, so if I want to delete message I should call smth like this:
say(nil, thread_id: 'THREAD_ID', delete_message: 'MESSAGE_TS') right?

@emorissettegregoire
Copy link
Contributor Author

emorissettegregoire commented Dec 24, 2024

DIdn't get the idea, so if I want to delete message I should call smth like this:

say(nil, thread_id: 'THREAD_ID', delete_message: 'MESSAGE_TS') right?

In my use case I delete a mssg as soon as I send another one.. so I pass the mssg content as well. Check PR on Mav

@kladaFOX
Copy link

yeah I get it you are calling
say("blablabla", thread_id: , delete_message:)
But why not implement delete_message(message_id)?
And your case would be more readable. You will call say and then delete or vice versa.
I also think that having the delete_message method would be better for future use cases

@matthewblack
Copy link
Member

matthewblack commented Dec 24, 2024 via email

@emorissettegregoire
Copy link
Contributor Author

Yes, thank you guys. I agree. Will check it with fresh eyes and see if we need both.

@matthewblack
Copy link
Member

Hey @emorissettegregoire - Sorry for the confusion earlier. I took a deeper look what you're doing with this delete_message argument via the stealth-slack client.

I was initially under the impression that this was one API call to slack and that's how they handled "replacing a message". However, it looks like you are sending two API requests - one for creating the message, and then another for deleting. This violates the Single Responsibility Principle (SRP) and I agree with Ilya that we should create a separate reply handler called delete and inside our Stealth app, we should call both.

The goal in Stealth is to make each function modular which makes it easier to maintain. Moreover, separating concerns makes it easier to test, debug, and extend functionality. For example, if we ever decide to introduce new deletion-related behaviors or parameters, we wouldn’t have to modify the say method and risk affecting unrelated logic.

Let me know if you want to chat through any implementation together! Happy Holidays!

@emorissettegregoire
Copy link
Contributor Author

Hey @emorissettegregoire - Sorry for the confusion earlier. I took a deeper look what you're doing with this delete_message argument via the stealth-slack client.

I was initially under the impression that this was one API call to slack and that's how they handled "replacing a message". However, it looks like you are sending two API requests - one for creating the message, and then another for deleting. This violates the Single Responsibility Principle (SRP) and I agree with Ilya that we should create a separate reply handler called delete and inside our Stealth app, we should call both.

The goal in Stealth is to make each function modular which makes it easier to maintain. Moreover, separating concerns makes it easier to test, debug, and extend functionality. For example, if we ever decide to introduce new deletion-related behaviors or parameters, we wouldn’t have to modify the say method and risk affecting unrelated logic.

Let me know if you want to chat through any implementation together! Happy Holidays!

@matthewblack on it. Thanks!

Copy link

@kladaFOX kladaFOX left a comment

Choose a reason for hiding this comment

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

Lovely!

Copy link
Member

@matthewblack matthewblack left a comment

Choose a reason for hiding this comment

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

Hell yeah! Great work Emilie.

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