-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Create new RemoveAllByAction Worker #5690
Create new RemoveAllByAction Worker #5690
Conversation
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.
Good job @luchiago!
since we're here we could add a test to check what happens if the notifiable_type
is not in the included list and one to check if notifiable_ids
is not present :)
Right, @rhymes. I'll create the spec and send the changes. |
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.
Thought about some of the code that is unused. Thanks for continuing to help with this project!
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.
LGTM 🚀
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.
Thank you for the changes @luchiago!
What type of PR is this? (check all applicable)
Description
Related Tickets & Documents
#5305
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
Added to documentation?
[optional] What gif best describes this PR or how it makes you feel?