From 19d21f4b443c9e84d5925ca45c4eae383fd4db4a Mon Sep 17 00:00:00 2001 From: Sara Jackson Date: Mon, 21 Feb 2022 14:25:51 -0500 Subject: [PATCH 1/2] Paginate through slack channels If there are over 1000 slack channels in an organization, which is entirely possible with the ability to archive channels, the call method in loads_slack_channels would only retrieve the first 1000. This change adds a method to follow any pagination cursors and combine channel list pages together to ensure we have a complete channel list. This change also makes use of the exclude_archived argument for conversations_list which will (on Slack's side) throw out any archived channels. A snippet to keep in mind from Slack's docs: https://api.slack.com/methods/conversations.list "When paginating, any filters used in the request are applied after retrieving a virtual page's limit. For example. using exclude_archived=true when limit=20 on a virtual page that would contain 15 archived channels will return you the virtual page with only 5 results. Additional results are available from the next cursor valuePaginate through slack channels." --- app/services/slack/loads_slack_channels.rb | 14 +++++++-- spec/lib/slack/loads_slack_channels_spec.rb | 32 +++++++++++++++++---- 2 files changed, 39 insertions(+), 7 deletions(-) diff --git a/app/services/slack/loads_slack_channels.rb b/app/services/slack/loads_slack_channels.rb index fb4f674..2cf642e 100644 --- a/app/services/slack/loads_slack_channels.rb +++ b/app/services/slack/loads_slack_channels.rb @@ -11,10 +11,10 @@ class LoadsSlackChannels def call(types:) response = retry_when_rate_limited do - ClientWrapper.client.conversations_list(types: approved_types(types), limit: 1000) + ClientWrapper.client.conversations_list(types: approved_types(types), limit: 1000, exclude_archived: true) end - (response&.channels || []).reject { |ch| ch.is_archived } + get_paged_channels(response&.response_metadata&.next_cursor, response&.channels || []) end private @@ -22,5 +22,15 @@ def call(types:) def approved_types(types) types.split(",").select { |t| SLACK_CHANNEL_TYPES.include?(t) }.join(",") end + + def get_paged_channels(cursor, channels) + while cursor.present? + response = ClientWrapper.client.conversations_list(cursor: cursor, types: approved_types(types), limit: 1000, exclude_archived: true) + channels.append(response.channels) + cursor = response.response_metadata.next_cursor + end + + channels.flatten + end end end diff --git a/spec/lib/slack/loads_slack_channels_spec.rb b/spec/lib/slack/loads_slack_channels_spec.rb index 806238b..4b4abdb 100644 --- a/spec/lib/slack/loads_slack_channels_spec.rb +++ b/spec/lib/slack/loads_slack_channels_spec.rb @@ -18,17 +18,16 @@ is_mpim: true, user: Slack::Messages::Message.new(id: "USER_ID") ) - archived_slack_public_channel = Slack::Messages::Message.new( + Slack::Messages::Message.new( id: "PUBLIC_CHANNEL_ID_2", is_channel: true, is_archived: true ) - expect(@slack_client).to receive(:conversations_list).with(types: "public_channel,mpim", limit: 1000) { + expect(@slack_client).to receive(:conversations_list).with(types: "public_channel,mpim", limit: 1000, exclude_archived: true) { Slack::Messages::Message.new(ok: true, channels: [ slack_public_channel, - slack_mpim_channel, - archived_slack_public_channel + slack_mpim_channel ]) } @@ -37,13 +36,36 @@ expect(channels).to eq([slack_public_channel, slack_mpim_channel]) end + it "loads all active channels when requiring pagination" do + archived_slack_public_channel = Slack::Messages::Message.new( + id: "PUBLIC_CHANNEL_ID_2", + is_channel: true, + is_archived: true + ) + slack_public_channels = (0..1010).map do |ch| + Slack::Messages::Message.new( + id: "PUBLIC_CHANNEL_ID_#{ch}", + is_channel: true + ) + end + all_channels = slack_public_channels << archived_slack_public_channel + + expect(@slack_client).to receive(:conversations_list).with(types: "public_channel", limit: 1000, exclude_archived: true) { + Slack::Messages::Message.new(ok: true, channels: all_channels) + } + + channels = subject.call(types: "public_channel") + + expect(channels).to eq(slack_public_channels) + end + it "ignores unrecognized channel types" do slack_public_channel = Slack::Messages::Message.new( id: "PUBLIC_CHANNEL_ID_1", is_channel: true ) - expect(@slack_client).to receive(:conversations_list).with(types: "public_channel", limit: 1000) { + expect(@slack_client).to receive(:conversations_list).with(types: "public_channel", limit: 1000, exclude_archived: true) { Slack::Messages::Message.new(ok: true, channels: [slack_public_channel]) } From cc59fa660c51f018dab7eb1e875f837c25384bd3 Mon Sep 17 00:00:00 2001 From: Sara Jackson Date: Wed, 27 Apr 2022 11:09:45 -0400 Subject: [PATCH 2/2] Add keyword args, update spec, and bug Changing get_paged_chanels to take keyword args. Updating the loads_slack_channels spec for testing pagination to include the pagination cursor. Including types when calling get_paged_channels (bug fix) --- app/services/slack/loads_slack_channels.rb | 6 +++--- spec/lib/slack/loads_slack_channels_spec.rb | 12 +++++------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/app/services/slack/loads_slack_channels.rb b/app/services/slack/loads_slack_channels.rb index 2cf642e..8506c1b 100644 --- a/app/services/slack/loads_slack_channels.rb +++ b/app/services/slack/loads_slack_channels.rb @@ -14,7 +14,7 @@ def call(types:) ClientWrapper.client.conversations_list(types: approved_types(types), limit: 1000, exclude_archived: true) end - get_paged_channels(response&.response_metadata&.next_cursor, response&.channels || []) + get_paged_channels(cursor: response&.response_metadata&.next_cursor, channels: response&.channels || [], types: types) end private @@ -23,11 +23,11 @@ def approved_types(types) types.split(",").select { |t| SLACK_CHANNEL_TYPES.include?(t) }.join(",") end - def get_paged_channels(cursor, channels) + def get_paged_channels(cursor:, channels:, types:) while cursor.present? response = ClientWrapper.client.conversations_list(cursor: cursor, types: approved_types(types), limit: 1000, exclude_archived: true) channels.append(response.channels) - cursor = response.response_metadata.next_cursor + cursor = response.response_metadata&.next_cursor end channels.flatten diff --git a/spec/lib/slack/loads_slack_channels_spec.rb b/spec/lib/slack/loads_slack_channels_spec.rb index 4b4abdb..7075b50 100644 --- a/spec/lib/slack/loads_slack_channels_spec.rb +++ b/spec/lib/slack/loads_slack_channels_spec.rb @@ -37,21 +37,19 @@ end it "loads all active channels when requiring pagination" do - archived_slack_public_channel = Slack::Messages::Message.new( - id: "PUBLIC_CHANNEL_ID_2", - is_channel: true, - is_archived: true - ) slack_public_channels = (0..1010).map do |ch| Slack::Messages::Message.new( id: "PUBLIC_CHANNEL_ID_#{ch}", is_channel: true ) end - all_channels = slack_public_channels << archived_slack_public_channel + response_metadata = Slack::Messages::Message.new(next_cursor: "cursor") expect(@slack_client).to receive(:conversations_list).with(types: "public_channel", limit: 1000, exclude_archived: true) { - Slack::Messages::Message.new(ok: true, channels: all_channels) + Slack::Messages::Message.new(ok: true, response_metadata: response_metadata, channels: slack_public_channels[0..999]) + } + expect(@slack_client).to receive(:conversations_list).with(types: "public_channel", limit: 1000, exclude_archived: true, cursor: response_metadata.next_cursor) { + Slack::Messages::Message.new(ok: true, channels: slack_public_channels[1000..]) } channels = subject.call(types: "public_channel")