Add slack connector #115
Add slack connector #115
Conversation
…gle-drive-document
…elopers and users.
What's changedIgnored archived channels and bot messages How to test
Anything to be aware ofPlease check ReadMe |
src/providers/slack/chat-message.ts
Outdated
| protected async buildChatGroupList(): Promise<SchemaSocialChatGroup[]> { | ||
| const client = this.getSlackClient(); | ||
| let channelList: SchemaSocialChatGroup[] = []; | ||
| const types = ["im", "private_channel", "public_channel"]; |
There was a problem hiding this comment.
This should be using the channelTypes config options
src/providers/slack/chat-message.ts
Outdated
| sourceId: channel.id, | ||
| schema: CONFIG.verida.schemas.CHAT_GROUP, | ||
| sourceData: channel, | ||
| insertedAt: new Date().toISOString(), |
There was a problem hiding this comment.
Is therre a channel.created you can use here?
There was a problem hiding this comment.
channel.created indicates 1970 always, so used channel.updated instead
src/providers/slack/chat-message.ts
Outdated
| const types = ["im", "private_channel", "public_channel"]; | ||
|
|
||
| for (const type of types) { | ||
| const conversations = await client.conversations.list({ types: type }); |
There was a problem hiding this comment.
Is there a limit to how many conversations are returned? Is it necessary to iterate through multiple pages of conversation lists?
There was a problem hiding this comment.
Use MAX_BATCH_SIZE and removed iteration
src/providers/slack/chat-message.ts
Outdated
| let totalMessages = 0; | ||
| let chatHistory: SchemaSocialChatMessage[] = []; | ||
|
|
||
| for (let i = 0; i < mergedGroupList.length; i++) { |
There was a problem hiding this comment.
Use for (const group of mergedGroupList) {
There was a problem hiding this comment.
Use
for (const group of mergedGroupList) {
Okay
|
|
||
| let query: ConversationsHistoryArguments = { | ||
| channel: group.sourceId!, | ||
| limit: this.config.messagesPerGroupLimit, |
There was a problem hiding this comment.
What is the maximum limit supported by Slack API?
What happens if this.config.messagesPerGroupLimit is higher than that number?
There was a problem hiding this comment.
Slack has limit 1000, and if this.config.messagesPerGroupLimit is higher than 1000, will return maximum 1000 items
There was a problem hiding this comment.
This needs to be documented somewhere
| throw new Error("Invalid group or missing group sourceId"); | ||
| } | ||
|
|
||
| let items: SchemaSocialChatMessage[] = []; |
There was a problem hiding this comment.
See telegram implementation, this code should be wrapped in a loop to ensure the maximum number of results are returned, which will also automatically handle the backfill so that code can be removed.
There was a problem hiding this comment.
Slack supports cursor based pagination and I followed Google's item range tracking which is different from TG.
There was a problem hiding this comment.
My comment still stands, this needs to be refactored to match the Telegram impelementation.
This will only backfill once, however there could be multiple backfills required. As a result, the full batch size won't be filled.
There was a problem hiding this comment.
Actually, let's leave this for now.
Google has the same issue.
When we do the refactor we can fix this for all the handlers.
src/providers/slack/chat-message.ts
Outdated
| totalMessages: number, | ||
| groupCount: number | ||
| ) { | ||
| syncPosition.status = SyncHandlerStatus.SYNCING; |
There was a problem hiding this comment.
If totalMessages=0 the syncPosition.status should be ENABLED, not SYNCING.
src/schemas.ts
Outdated
| export interface SchemaSocialChatGroup extends SchemaRecord { | ||
| newestId?: string | ||
| syncData?: string | ||
| type?: string |
There was a problem hiding this comment.
Can you explain this?
Just tried to represent DM, private, or public. But not used. I will remove type
src/providers/slack/chat-message.ts
Outdated
| let totalMessages = 0; | ||
| let chatHistory: SchemaSocialChatMessage[] = []; | ||
|
|
||
| for (let i = 0; i < mergedGroupList.length; i++) { |
|
|
||
| let query: ConversationsHistoryArguments = { | ||
| channel: group.sourceId!, | ||
| limit: this.config.messagesPerGroupLimit, |
There was a problem hiding this comment.
This needs to be documented somewhere
| throw new Error("Invalid group or missing group sourceId"); | ||
| } | ||
|
|
||
| let items: SchemaSocialChatMessage[] = []; |
There was a problem hiding this comment.
My comment still stands, this needs to be refactored to match the Telegram impelementation.
This will only backfill once, however there could be multiple backfills required. As a result, the full batch size won't be filled.
| throw new Error("Invalid group or missing group sourceId"); | ||
| } | ||
|
|
||
| let items: SchemaSocialChatMessage[] = []; |
There was a problem hiding this comment.
Actually, let's leave this for now.
Google has the same issue.
When we do the refactor we can fix this for all the handlers.
| const firstGroupId = chatMessages[0].groupId; | ||
| const firstGroupMessages = chatMessages.filter(msg => msg.groupId === firstGroupId); | ||
| assert.equal(firstGroupMessages.length, handlerConfig.messagesPerGroupLimit, "Processed correct number of messages per group"); | ||
|
|
There was a problem hiding this comment.
Add a test to make sure most recent messages are processed first
There was a problem hiding this comment.
Add a test to make sure most recent messages are processed first
Done
|
|
||
| // Ensure second batch results are returned | ||
| assert.ok(secondBatchResults && secondBatchResults.length, "Have second batch results returned"); | ||
|
|
There was a problem hiding this comment.
Check second batch items aren't in the first batch
Check second batch first item is older than the first batch last item
There was a problem hiding this comment.
Check second batch items aren't in the first batch
Done
Check second batch first item is older than the first batch last item
A batch has multiple channels, can't do cross-channel ordering
| ); | ||
|
|
||
| // Check if synced every chat group correctly | ||
| const syncedGroup = (secondBatchChatGroups.filter(group => group.sourceId === secondBatchChatMessages[0].groupId))[0]; |
There was a problem hiding this comment.
This only checks one group, not all the groups
There was a problem hiding this comment.
This only checks one group, not all the groups
Done
| } | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Add tests to show that each type of chat group works:
SlackChatGroupType.IM,
SlackChatGroupType.PRIVATE_CHANNEL,
SlackChatGroupType.PUBLIC_CHANNEL
There was a problem hiding this comment.
Passing this test requires to have all kind of channels and messages in every channel.
Enabled DM only by default.
const groupTypes = {
[SlackChatGroupType.IM]: false, // enabled
[SlackChatGroupType.PRIVATE_CHANNEL]: true,
[SlackChatGroupType.PUBLIC_CHANNEL]: true,
};
Closes #98