-
Notifications
You must be signed in to change notification settings - Fork 6k
Added .delete() shortcut for business messages #4869
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
Added .delete() shortcut for business messages #4869
Conversation
|
Hi @jainamoswal may I ask what's your status here? do you need help with anything? |
|
I'm still figuring out about adding the tests regarding the code I added. @Bibo-Joshi |
|
So there are a couple of things we'd want to test here.
The way i'd approach this is to Then we'd also need to have two test message objects, possibly as a parameterized fixture, you can also search the codebase or shoot me any questions if some parts aren't clear. |
|
@jainamoswal are you still interested in working on this? Do you need additional support with the tests? |
|
I will take this as a no. @aelkheir would you like to continue work on this, possibly after the api update is finished? :) |
|
Apologies @jainamoswal, something went wrong my side and i had to force push. |
Bibo-Joshi
left a comment
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.
Thanks for supporting @aelkheir ! If there is nothing left from your side I'd say this is good to merge :)
closes #4867