-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add message contains keyword trigger #132
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
base: main
Are you sure you want to change the base?
feat: add message contains keyword trigger #132
Conversation
7724848 to
d98a20d
Compare
490aca2 to
c25c08d
Compare
AdamSelene
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.
I think it would be better to extend the existing "New message" trigger with an optional field, WDYT? We did the same for the Intercom "New reply" trigger
- if there is no value: no filtering
- if there is: apply filter
A second improvement (for another PR) could be to filter by author with a dropdown
9e236dc to
ba27866
Compare
AdamSelene
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 the changes, left a few more comments
| newUserTrigger, | ||
| newSavedMessageTrigger, | ||
| newTeamCustomEmojiTrigger, | ||
| messageContainsKeywordTrigger, |
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.
remove?
| ignoreBots: Property.Checkbox({ | ||
| displayName: 'Ignore Bot Messages ?', | ||
| required: true, | ||
| required: 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.
why change to 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.
I was thinking that since there is a default value provided, we can just use that and check the box only when needed!
| }), | ||
| matchWholeWord: Property.Checkbox({ | ||
| displayName: 'Match Whole Word Only', | ||
| description: 'If enabled, only matches the keyword as a complete word (e.g. "test" won\'t match "testing"). Leave empty if no keyword.', |
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.
| description: 'If enabled, only matches the keyword as a complete word (e.g. "test" won\'t match "testing"). Leave empty if no keyword.', | |
| description: 'If enabled, only matches the keyword as a complete word (e.g. "test" won\'t match "testing").', |
| sampleData: { | ||
| text: 'Hello world!', | ||
| channel: 'C1234567890', | ||
| user: 'U1234567890', | ||
| channel_type: 'channel', | ||
| ts: '1234567890.123456' | ||
| }, |
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.
| sampleData: { | |
| text: 'Hello world!', | |
| channel: 'C1234567890', | |
| user: 'U1234567890', | |
| channel_type: 'channel', | |
| ts: '1234567890.123456' | |
| }, | |
| sampleData: undefined, |
Otherwise you can't test with live data (click on "test trigger" then sending a test message in Slack) - it's a limitation from ActivePieces that you can't have both
| } | ||
| // check if it's channel message | ||
| if (!['channel','group'].includes(payloadBody.event.channel_type)) { | ||
| if (!includeThreads || !payloadBody.event.thread_ts) { |
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.
No need to add this extra check - "channel" and "group" messages are purely technical messages like "someone has joined the channel", etc. So they always need to be ignored
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.
Just to clarify, don't the channel and group messages not also include the messages sent by users and not just messages like "someone has joined the channel", etc. So in that case, we cannot always ignore all the channel and group messages right? Unless I have misunderstood something 😅
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.
Hmm I was mixing things with "subtypes" sorry for the confusion!
https://docs.slack.dev/reference/events/message/
Not sure where the channel_type comes from
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 the channel_type is to just indicate whether the message is coming from public or private channels or direct messages!
I have added a check with the subtype which will then ignore all the messages like "someone has joined the channel" or file uploads, will this be the desired behaviour?
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 so what I'm missing now is the extra if (!includeThreads || !payloadBody.event.thread_ts) check - why do you need it?
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.
initially it was to allow dm thread replies to be processed if we want to include threads, so if we don't want to include dm messages at all, we can remove that check?
packages/server/worker/package.json
Outdated
| "ai": "5.0.12", | ||
| "zod": "3.25.76", | ||
| "@activepieces/common-ai": "0.1.0" | ||
| "@activepieces/common-ai": "0.2.2" |
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.
Unwanted change?
| { | ||
| "name": "@activepieces/common-ai", | ||
| "version": "0.1.0", | ||
| "version": "0.2.2", |
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.
Unwanted changes on this file?
AdamSelene
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.
Aside from some cleanup, looking good
Left another comment to fully understand the implementation (not a blocker)
Did you test it locally?
| "@sinclair/typebox": "0.34.11", | ||
| "@activepieces/shared": "0.20.0", | ||
| "@activepieces/pieces-framework": "0.18.5", | ||
| "@activepieces/pieces-framework": "0.19.0", |
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.
Still unwanted change?
| import { newUserTrigger } from './lib/triggers/new-user'; | ||
| import { newSavedMessageTrigger } from './lib/triggers/new-saved-message'; | ||
| import { newTeamCustomEmojiTrigger } from './lib/triggers/new-team-custom-emoji'; | ||
| import { messageContainsKeywordTrigger } from './lib/triggers/message-contains-keyword'; |
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.
Remove?
| newUserTrigger, | ||
| newSavedMessageTrigger, | ||
| newTeamCustomEmojiTrigger, | ||
| messageContainsKeywordTrigger, |
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.
Remove?
|
@AdamSelene Hey! I have made the required changes, will you be available to review the changes? Thank you!! |
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.
You may need to bump the version again, I'll sync the repo with the upstream one
Edit: I confirm, see the conflict reported by Github
What does this PR do?
Adds a new Slack trigger called "Message Contains Keyword" that monitors all channels in a Slack workspace for messages containing specific keywords.
Explain How the Feature Works
Relevant User Scenarios
Fixes # (issue)