From 00b42fbdfb675b9f24ed07ad30a863a79aaa2e0b Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Mon, 31 Aug 2026 16:44:25 +0200 Subject: [PATCH 1/9] feat(intercom): scaffold the datasource package First PR of lot 1 (PRD-1112): the package skeleton, nothing that talks to Intercom yet. Configuration, the Faraday client and the collections each follow in their own PR, so this one only has to prove the package is wired into rubocop, rspec, coverage and the release before any behaviour rests on it. Registered in the four places a package has to be declared, since a missing entry breaks CI or releases silently rather than loudly: version.rb in the exact format the release sed matches, the gemspec MFA opt-out plus its rubocop excludes, the three spots of .releaserc.js, and both the test matrix and the coverage file list of build.yml. Ships the error hierarchy the rest of the lot leans on: UnsupportedOperatorError descends from the toolkit's ValidationError so a filter Intercom cannot express exactly answers 400 with a message the operator can act on, and APIError carries Intercom's status and parsed body so a smart action can surface its reason instead of an opaque failure. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/build.yml | 3 +- .releaserc.js | 7 ++- .rubocop.yml | 3 + .../.gitignore | 8 +++ .../forest_admin_datasource_intercom/.rspec | 3 + .../forest_admin_datasource_intercom/Gemfile | 16 ++++++ .../Gemfile-test | 19 +++++++ .../forest_admin_datasource_intercom/Rakefile | 6 ++ .../forest_admin_datasource_intercom.gemspec | 36 ++++++++++++ .../lib/forest_admin_datasource_intercom.rb | 57 +++++++++++++++++++ .../datasource.rb | 24 ++++++++ .../version.rb | 3 + .../datasource_spec.rb | 17 ++++++ .../forest_admin_datasource_intercom_spec.rb | 49 ++++++++++++++++ .../spec/spec_helper.rb | 43 ++++++++++++++ 15 files changed, 291 insertions(+), 3 deletions(-) create mode 100644 packages/forest_admin_datasource_intercom/.gitignore create mode 100644 packages/forest_admin_datasource_intercom/.rspec create mode 100644 packages/forest_admin_datasource_intercom/Gemfile create mode 100644 packages/forest_admin_datasource_intercom/Gemfile-test create mode 100644 packages/forest_admin_datasource_intercom/Rakefile create mode 100644 packages/forest_admin_datasource_intercom/forest_admin_datasource_intercom.gemspec create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom.rb create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/version.rb create mode 100644 packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb create mode 100644 packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom_spec.rb create mode 100644 packages/forest_admin_datasource_intercom/spec/spec_helper.rb diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index f4fe543c2..a88277e4b 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -70,6 +70,7 @@ jobs: - forest_admin_datasource_snowflake - forest_admin_datasource_mambu_payments - forest_admin_datasource_graphql_hasura + - forest_admin_datasource_intercom services: mongodb: image: mongo:latest @@ -153,7 +154,7 @@ jobs: with: verbose: true oidc: true - files: ${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_agent/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_datasource_active_record/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_datasource_customizer/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_datasource_toolkit/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_rails/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_datasource_mongoid/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_rpc_agent/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_datasource_rpc/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_datasource_zendesk/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_datasource_snowflake/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_datasource_mambu_payments/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_datasource_graphql_hasura/coverage.json + files: ${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_agent/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_datasource_active_record/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_datasource_customizer/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_datasource_toolkit/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_rails/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_datasource_mongoid/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_rpc_agent/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_datasource_rpc/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_datasource_zendesk/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_datasource_snowflake/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_datasource_mambu_payments/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_datasource_graphql_hasura/coverage.json,${{ github.workspace }}/reports/${{ matrix.ruby-version }}-forest_admin_datasource_intercom/coverage.json deploy: name: Release package diff --git a/.releaserc.js b/.releaserc.js index 76e57c8f0..479152e69 100644 --- a/.releaserc.js +++ b/.releaserc.js @@ -31,7 +31,8 @@ module.exports = { 'sed -i \'s/VERSION = ".*"/VERSION = "${nextRelease.version}"/g\' packages/forest_admin_datasource_zendesk/lib/forest_admin_datasource_zendesk/version.rb; '+ 'sed -i \'s/VERSION = ".*"/VERSION = "${nextRelease.version}"/g\' packages/forest_admin_datasource_snowflake/lib/forest_admin_datasource_snowflake/version.rb; '+ 'sed -i \'s/VERSION = ".*"/VERSION = "${nextRelease.version}"/g\' packages/forest_admin_datasource_mambu_payments/lib/forest_admin_datasource_mambu_payments/version.rb; '+ - 'sed -i \'s/VERSION = ".*"/VERSION = "${nextRelease.version}"/g\' packages/forest_admin_datasource_graphql_hasura/lib/forest_admin_datasource_graphql_hasura/version.rb; ', + 'sed -i \'s/VERSION = ".*"/VERSION = "${nextRelease.version}"/g\' packages/forest_admin_datasource_graphql_hasura/lib/forest_admin_datasource_graphql_hasura/version.rb; '+ + 'sed -i \'s/VERSION = ".*"/VERSION = "${nextRelease.version}"/g\' packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/version.rb; ', successCmd: '( cd packages/forest_admin_agent && gem build && gem push forest_admin_agent-*.gem );' + '( cd packages/forest_admin_datasource_active_record && gem build && gem push forest_admin_datasource_active_record-*.gem );' + @@ -45,7 +46,8 @@ module.exports = { '( cd packages/forest_admin_datasource_zendesk && gem build && gem push forest_admin_datasource_zendesk-*.gem );' + '( cd packages/forest_admin_datasource_snowflake && gem build && gem push forest_admin_datasource_snowflake-*.gem );' + '( cd packages/forest_admin_datasource_mambu_payments && gem build && gem push forest_admin_datasource_mambu_payments-*.gem );' + - '( cd packages/forest_admin_datasource_graphql_hasura && gem build && gem push forest_admin_datasource_graphql_hasura-*.gem );' , + '( cd packages/forest_admin_datasource_graphql_hasura && gem build && gem push forest_admin_datasource_graphql_hasura-*.gem );' + + '( cd packages/forest_admin_datasource_intercom && gem build && gem push forest_admin_datasource_intercom-*.gem );' , }, ], [ @@ -68,6 +70,7 @@ module.exports = { 'packages/forest_admin_datasource_snowflake/lib/forest_admin_datasource_snowflake/version.rb', 'packages/forest_admin_datasource_mambu_payments/lib/forest_admin_datasource_mambu_payments/version.rb', 'packages/forest_admin_datasource_graphql_hasura/lib/forest_admin_datasource_graphql_hasura/version.rb', + 'packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/version.rb', 'package.json' ], }, diff --git a/.rubocop.yml b/.rubocop.yml index c6985f27d..657f0cc6c 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -42,6 +42,7 @@ Gemspec/RequireMFA: - 'packages/forest_admin_datasource_snowflake/forest_admin_datasource_snowflake.gemspec' - 'packages/forest_admin_datasource_mambu_payments/forest_admin_datasource_mambu_payments.gemspec' - 'packages/forest_admin_datasource_graphql_hasura/forest_admin_datasource_graphql_hasura.gemspec' + - 'packages/forest_admin_datasource_intercom/forest_admin_datasource_intercom.gemspec' # Offense count: 1 # This cop supports unsafe autocorrection (--autocorrect-all). @@ -133,6 +134,7 @@ Style/MutableConstant: - 'packages/forest_admin_datasource_zendesk/lib/forest_admin_datasource_zendesk/version.rb' - 'packages/forest_admin_datasource_snowflake/lib/forest_admin_datasource_snowflake/version.rb' - 'packages/forest_admin_datasource_mambu_payments/lib/forest_admin_datasource_mambu_payments/version.rb' + - 'packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/version.rb' # Offense count: 38 # This cop supports safe autocorrection (--autocorrect). @@ -217,6 +219,7 @@ Style/StringLiterals: - 'packages/forest_admin_datasource_zendesk/lib/forest_admin_datasource_zendesk/version.rb' - 'packages/forest_admin_datasource_snowflake/lib/forest_admin_datasource_snowflake/version.rb' - 'packages/forest_admin_datasource_mambu_payments/lib/forest_admin_datasource_mambu_payments/version.rb' + - 'packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/version.rb' # Offense count: 1 # This cop supports safe autocorrection (--autocorrect). diff --git a/packages/forest_admin_datasource_intercom/.gitignore b/packages/forest_admin_datasource_intercom/.gitignore new file mode 100644 index 000000000..06cfcfb83 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/.gitignore @@ -0,0 +1,8 @@ +*.gem +.bundle/ +Gemfile.lock +Gemfile-test.lock +coverage/ +pkg/ +tmp/ +.rspec_status diff --git a/packages/forest_admin_datasource_intercom/.rspec b/packages/forest_admin_datasource_intercom/.rspec new file mode 100644 index 000000000..34c5164d9 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/.rspec @@ -0,0 +1,3 @@ +--format documentation +--color +--require spec_helper diff --git a/packages/forest_admin_datasource_intercom/Gemfile b/packages/forest_admin_datasource_intercom/Gemfile new file mode 100644 index 000000000..c229ff1d5 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/Gemfile @@ -0,0 +1,16 @@ +source 'https://rubygems.org' + +gemspec + +gem 'forest_admin_datasource_customizer' +gem 'forest_admin_datasource_toolkit' +gem 'rake', '~> 13.0' +gem 'rubocop', '1.86.1' +gem 'rubocop-performance', '1.26.1' +gem 'rubocop-rspec', '3.9.0' + +group :development, :test do + gem 'rspec', '~> 3.0' + gem 'simplecov', '~> 0.22', require: false + gem 'webmock', '~> 3.0' +end diff --git a/packages/forest_admin_datasource_intercom/Gemfile-test b/packages/forest_admin_datasource_intercom/Gemfile-test new file mode 100644 index 000000000..8b433e2b5 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/Gemfile-test @@ -0,0 +1,19 @@ +source 'https://rubygems.org' + +# Specify your gem's dependencies in forest_admin_datasource_intercom.gemspec +gemspec + +gem 'rake', '~> 13.0' +gem 'rubocop', '1.86.1' +gem 'rubocop-performance', '1.26.1' +gem 'rubocop-rspec', '3.9.0' + +group :development, :test do + gem 'forest_admin_datasource_customizer', path: '../forest_admin_datasource_customizer' + gem 'forest_admin_datasource_toolkit', path: '../forest_admin_datasource_toolkit' + gem 'rspec', '~> 3.0' + gem 'simplecov', '~> 0.22', require: false + gem 'simplecov-html', '~> 0.12.3' + gem 'simplecov_json_formatter', '~> 0.1.4' + gem 'webmock', '~> 3.0' +end diff --git a/packages/forest_admin_datasource_intercom/Rakefile b/packages/forest_admin_datasource_intercom/Rakefile new file mode 100644 index 000000000..4c774a2bf --- /dev/null +++ b/packages/forest_admin_datasource_intercom/Rakefile @@ -0,0 +1,6 @@ +require 'bundler/gem_tasks' +require 'rspec/core/rake_task' + +RSpec::Core::RakeTask.new(:spec) + +task default: :spec diff --git a/packages/forest_admin_datasource_intercom/forest_admin_datasource_intercom.gemspec b/packages/forest_admin_datasource_intercom/forest_admin_datasource_intercom.gemspec new file mode 100644 index 000000000..14ce48bb4 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/forest_admin_datasource_intercom.gemspec @@ -0,0 +1,36 @@ +lib = File.expand_path('lib', __dir__) +$LOAD_PATH.unshift lib unless $LOAD_PATH.include?(lib) + +require_relative 'lib/forest_admin_datasource_intercom/version' + +Gem::Specification.new do |spec| + spec.name = 'forest_admin_datasource_intercom' + spec.version = ForestAdminDatasourceIntercom::VERSION + spec.authors = ['Forest Admin'] + spec.email = ['contact@forestadmin.com'] + spec.homepage = 'https://www.forestadmin.com' + spec.summary = 'Intercom datasource for Forest Admin Ruby agent.' + spec.description = 'Surface Intercom conversations, tickets, contacts and companies as Forest Admin collections.' + spec.license = 'GPL-3.0' + spec.required_ruby_version = '>= 3.0.0' + + spec.metadata['homepage_uri'] = spec.homepage + spec.metadata['source_code_uri'] = 'https://github.com/ForestAdmin/agent-ruby' + spec.metadata['changelog_uri'] = 'https://github.com/ForestAdmin/agent-ruby/blob/main/CHANGELOG.md' + spec.metadata['rubygems_mfa_required'] = 'false' + + spec.files = Dir.chdir(__dir__) do + `git ls-files -z`.split("\x0").reject do |f| + (File.expand_path(f) == __FILE__) || + f.start_with?(*%w[bin/ test/ spec/ features/ .git .circleci appveyor Gemfile]) + end + end + spec.bindir = 'exe' + spec.executables = spec.files.grep(%r{\Aexe/}) { |f| File.basename(f) } + spec.require_paths = ['lib'] + + spec.add_dependency 'activesupport', '>= 6.1' + spec.add_dependency 'faraday', '~> 2.0' + spec.add_dependency 'faraday-retry', '~> 2.0' + spec.add_dependency 'zeitwerk', '~> 2.3' +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom.rb new file mode 100644 index 000000000..32db16f9b --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom.rb @@ -0,0 +1,57 @@ +require_relative 'forest_admin_datasource_intercom/version' +require 'json' +require 'logger' +require 'set' +require 'uri' +require 'zeitwerk' +require 'faraday' +require 'faraday/retry' +require 'forest_admin_datasource_toolkit' + +loader = Zeitwerk::Loader.for_gem +loader.setup + +module ForestAdminDatasourceIntercom + class Error < StandardError; end + class ConfigurationError < Error; end + + # A filter Intercom cannot express exactly: an operator its search DSL refuses + # on that field, a tree deeper than the two levels it allows, or a group past + # its fifteen filters. It descends from the toolkit's ValidationError rather + # than from this package's Error so the agent answers 400 carrying the message + # instead of a 500 "Unexpected error" -- each one names something the operator + # set and can change, and the message is the only place they learn which. + # + # This datasource refuses rather than approximates: a result that looks + # filtered and is not is worse than an explicit refusal. + class UnsupportedOperatorError < ForestAdminDatasourceToolkit::Exceptions::ValidationError; end + + # Raised when an Intercom API call fails. Carries the HTTP status and the + # parsed response body so callers -- smart actions in particular -- can + # surface Intercom's own error message instead of a generic string. + class APIError < Error + attr_reader :status, :body + + def initialize(message, status: nil, body: nil) + super(message) + @status = status + @body = body + end + end + + class << self + attr_writer :logger + + def logger + @logger ||= default_logger + end + + private + + def default_logger + return Rails.logger if defined?(Rails) && Rails.respond_to?(:logger) && Rails.logger + + Logger.new($stderr).tap { |l| l.progname = 'forest_admin_datasource_intercom' } + end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb new file mode 100644 index 000000000..6a1695296 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb @@ -0,0 +1,24 @@ +module ForestAdminDatasourceIntercom + # Boot skeleton: it registers no collection yet. Configuration and the Faraday + # client come next, then the collections, each behind its own pull request -- + # so this one is what proves the package is wired into the monorepo (rubocop, + # rspec, coverage, release) before any behaviour depends on it. + class Datasource < ForestAdminDatasourceToolkit::Datasource + def initialize + super + register_collections + end + + # The datasource is what a Rails error page or a `logger.debug` is likeliest + # to print, and it will soon hold the client carrying the access token. + # Cutting the default dump here also spares the recursive walk of a + # datasource and its collections pointing at each other. + def inspect + "#<#{self.class.name} collections=#{collections.keys.inspect}>" + end + + private + + def register_collections; end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/version.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/version.rb new file mode 100644 index 000000000..bcf5c5876 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/version.rb @@ -0,0 +1,3 @@ +module ForestAdminDatasourceIntercom + VERSION = "0.1.0" +end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb new file mode 100644 index 000000000..da24e86f3 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb @@ -0,0 +1,17 @@ +module ForestAdminDatasourceIntercom + RSpec.describe Datasource do + subject(:datasource) { described_class.new } + + it 'boots without reaching Intercom' do + expect { datasource }.not_to raise_error + end + + it 'registers no collection yet' do + expect(datasource.collections).to be_empty + end + + it 'names the collections it holds when printed' do + expect(datasource.inspect).to eq('#') + end + end +end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom_spec.rb new file mode 100644 index 000000000..f38f4adfe --- /dev/null +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom_spec.rb @@ -0,0 +1,49 @@ +RSpec.describe ForestAdminDatasourceIntercom do + describe 'VERSION' do + # The release `sed` in .releaserc.js only matches `VERSION = "x.y.z"`, and a + # format it misses is a version that stays behind with no CI failure to say so. + it 'is a double-quoted semantic version' do + expect(described_class::VERSION).to match(/\A\d+\.\d+\.\d+\z/) + end + end + + describe '.logger' do + around do |example| + previous = described_class.logger + example.run + described_class.logger = previous + end + + it 'defaults to a logger named after the package' do + described_class.logger = nil + + expect(described_class.logger.progname).to eq('forest_admin_datasource_intercom') + end + + it 'takes the logger it is handed' do + logger = Logger.new(File::NULL) + described_class.logger = logger + + expect(described_class.logger).to be(logger) + end + end + + describe 'errors' do + it 'reports a filter Intercom cannot express as a validation error' do + expect(described_class::UnsupportedOperatorError.new('nope')) + .to be_a(ForestAdminDatasourceToolkit::Exceptions::ValidationError) + end + + it 'carries the status and parsed body of a failed call' do + error = described_class::APIError.new('boom', status: 429, body: { 'type' => 'error.list' }) + + expect(error).to have_attributes(message: 'boom', status: 429, body: { 'type' => 'error.list' }) + end + + it 'leaves the status and body unset when the call failed before answering' do + error = described_class::APIError.new('timeout') + + expect(error).to have_attributes(status: nil, body: nil) + end + end +end diff --git a/packages/forest_admin_datasource_intercom/spec/spec_helper.rb b/packages/forest_admin_datasource_intercom/spec/spec_helper.rb new file mode 100644 index 000000000..2c1c345be --- /dev/null +++ b/packages/forest_admin_datasource_intercom/spec/spec_helper.rb @@ -0,0 +1,43 @@ +require 'simplecov' +# JSON output is consumed by the qlty CI coverage step; HTML is for local +# inspection. simplecov-html and simplecov_json_formatter are required only +# in Gemfile-test, so guard the require for local Gemfile runs. +begin + require 'simplecov_json_formatter' + require 'simplecov-html' + SimpleCov.formatters = [SimpleCov::Formatter::JSONFormatter, SimpleCov::Formatter::HTMLFormatter] +rescue LoadError + # Local Gemfile run without the CI formatters; default text output is fine. +end + +SimpleCov.start do + add_filter '/spec/' + enable_coverage :branch + minimum_coverage 90 +end + +SimpleCov.coverage_dir 'coverage' + +require 'webmock/rspec' +require 'forest_admin_datasource_customizer' +require 'forest_admin_datasource_intercom' + +# Every payload the specs feed in is hand-written from the Intercom OpenAPI 2.16 +# spec, never captured from a workspace: a conversation body is personal data, +# and a fixture is read by everyone who clones the repo. +WebMock.disable_net_connect!(allow_localhost: true) + +RSpec.configure do |config| + config.expect_with :rspec do |c| + c.syntax = :expect + end + config.mock_with :rspec do |m| + m.verify_partial_doubles = true + end + config.disable_monkey_patching! + config.warnings = false + config.order = :random + Kernel.srand config.seed + + config.before { WebMock.reset! } +end From 225fa4913ff3952f891def6efefff2ec5be9793c Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Mon, 31 Aug 2026 17:01:47 +0200 Subject: [PATCH 2/9] feat(intercom): configure and pace the API client Second PR of lot 1 (PRD-1112). Everything a request needs before there is an endpoint to call: where it goes, which API version it asks for, how long it waits, and how it reacts to a refusal. The regional host is a first-class parameter rather than a constant. api.intercom.io does route to the right region, but a workspace under GDPR wants its requests reaching the European host and nothing else, and a base_url also points the client at a mock server or an egress proxy. The API version is pinned. Without the header a request follows the workspace's own default version, which an operator can change on Intercom's side -- and the payloads change shape under us. Intercom echoes the version it served, so the health check compares the two and says so when the pin was not honoured, rather than raising: running against a version we did not ask for still beats not running. Pacing is driven by the response headers, not by a table of budgets. Intercom allocates its quota in 10-second windows -- the measured x-ratelimit-limit is 1667, not the documented 10 000 a minute -- so what matters is the instantaneous rate, and every response says what is left of the current window and when it refills. The limiter waits that reset out; it also counts its own requests down, since several can be in flight before any of them answers. A reset further out than a window is a clock disagreement rather than a window emptying, so the request goes through and the log says why once per window. The 429 retry stays behind the limiter as the backstop for what this process cannot see: the workspace budget is shared with every other private app the customer runs. Only the verbs that change nothing are replayed, plus the 429 on any verb -- Intercom rejects it unprocessed, where a 502 on the way back from a POST it did perform would be replayed into a second reply on a conversation. per_page is bounded before it is sent: Intercom answers invalid_per_page past 150 instead of clamping, so the list view breaks rather than shrinks. Co-Authored-By: Claude Opus 5 (1M context) --- .rubocop.yml | 2 + .../client.rb | 176 +++++++++++++ .../configuration.rb | 112 +++++++++ .../datasource.rb | 24 +- .../rate_limiter.rb | 169 +++++++++++++ .../retry_policy.rb | 72 ++++++ .../throttle.rb | 19 ++ .../client_spec.rb | 237 ++++++++++++++++++ .../configuration_spec.rb | 98 ++++++++ .../datasource_spec.rb | 17 +- .../rate_limiter_spec.rb | 150 +++++++++++ .../retry_policy_spec.rb | 49 ++++ .../throttle_spec.rb | 40 +++ 13 files changed, 1155 insertions(+), 10 deletions(-) create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/client.rb create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/configuration.rb create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/rate_limiter.rb create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/retry_policy.rb create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/throttle.rb create mode 100644 packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/client_spec.rb create mode 100644 packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/configuration_spec.rb create mode 100644 packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/rate_limiter_spec.rb create mode 100644 packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/retry_policy_spec.rb create mode 100644 packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/throttle_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index 657f0cc6c..c6901590f 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -264,6 +264,7 @@ Metrics/ParameterLists: - 'packages/forest_admin_agent/lib/forest_admin_agent/audit_trail/capture.rb' - 'packages/forest_admin_agent/lib/forest_admin_agent/audit_trail/store.rb' - 'packages/forest_admin_datasource_graphql_hasura/lib/forest_admin_datasource_graphql_hasura/configuration.rb' + - 'packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/configuration.rb' - 'packages/forest_admin_datasource_zendesk/lib/forest_admin_datasource_zendesk/collections/base_collection.rb' - 'packages/forest_admin_datasource_snowflake/lib/forest_admin_datasource_snowflake/datasource.rb' - 'packages/forest_admin_agent/lib/forest_admin_agent/routes/query_handler.rb' @@ -299,6 +300,7 @@ Metrics/ModuleLength: - 'packages/forest_admin_datasource_mambu_payments/spec/**/*' - 'packages/forest_admin_rails/spec/**/*' - 'packages/forest_admin_rpc_agent/spec/**/*' + - 'packages/forest_admin_datasource_intercom/spec/**/*' - 'packages/forest_admin_datasource_mongoid/lib/forest_admin_datasource_mongoid/utils/helpers.rb' Metrics/MethodLength: diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/client.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/client.rb new file mode 100644 index 000000000..1f7b64292 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/client.rb @@ -0,0 +1,176 @@ +module ForestAdminDatasourceIntercom + # Every call to Intercom goes through here. Hand-written on Faraday rather + # than through the official `intercom` gem: that one maps the JSON onto + # objects, and what this datasource needs is the parts it hides -- the raw + # payload, because a custom attribute is a key nobody declared in advance; + # the quota headers, because the pacing is driven by them; and Intercom's own + # error body, because that text is what an operator reads when an action + # fails. + class Client + # `per_page=200` is refused with `invalid_per_page` -- "must be an integer + # between 0 and 150". There is no silent downgrade, so a page size is bounded + # before it is sent or the list view breaks rather than shrinks. + MAX_PER_PAGE = 150 + + def initialize(configuration) + @configuration = configuration + end + + # Health check: the admin the token belongs to, plus its workspace. Enough + # to prove the credentials are usable, and the one call that verifies the + # pinned API version was honoured -- Intercom echoes the version it served + # in a response header. + # + # `boot: true` runs it on the short-timeout connection, for a caller + # checking the token while the agent is still starting. + def me(boot: false) + must_succeed('me') do + response = get('me', boot: boot) + verify_pinned_version(response) + response.body + end + end + + # The page size Intercom accepts, whatever was asked for. + def self.bounded_per_page(size) + value = size.to_i + return 1 if value < 1 + + [value, MAX_PER_PAGE].min + end + + # The client holds the connections whose headers carry the access token in + # clear, and Faraday prints those headers on `inspect`. + def inspect + "#<#{self.class.name} url=#{@configuration.url.inspect}>" + end + + private + + # The raw response rather than its body: the quota headers are read by the + # throttle, and the version echo by `verify_pinned_version`. + def get(path, params = nil, boot: false) + (boot ? boot_connection : connection).get(path, params) + end + + # Intercom serves the version its workspace defaults to when the pin is not + # honoured, and the payloads differ between versions. The echo is the only + # way to notice, and noticing at boot is worth more than a schema that + # drifts silently -- so this reports rather than raises: the agent still runs + # against a version it did not ask for, which is better than not running. + def verify_pinned_version(response) + served = response.headers['intercom-version'] if response.respond_to?(:headers) + return if served.nil? || served.to_s == @configuration.api_version + + ForestAdminDatasourceIntercom.logger.warn( + "[forest_admin_datasource_intercom] asked Intercom for API version #{@configuration.api_version} " \ + "and it served #{served}. Payload shapes may differ from the ones this datasource expects; check the " \ + "workspace's default version in the Developer Hub." + ) + end + + def must_succeed(operation) + yield + rescue Faraday::Error => e + raise api_error(operation, e) + rescue StandardError => e + raise APIError, "Intercom API call failed: #{operation}: #{e.class}: #{e.message}" + end + + # Builds an APIError preserving the HTTP status and Intercom's own error body + # so a smart action can show the operator the real reason instead of + # "failed". + def api_error(operation, error) + response = error.respond_to?(:response) ? error.response : nil + status = response.is_a?(Hash) ? response[:status] : nil + body = parse_body(response.is_a?(Hash) ? response[:body] : nil) + detail = status ? "HTTP #{status} #{error_message(body)}".strip : "#{error.class}: #{error.message}" + + APIError.new("Intercom API call failed: #{operation}: #{detail}", status: status, body: body) + end + + # Intercom answers a failure with `{ "type": "error.list", "request_id": + # "...", "errors": [{ "code": ..., "message": ... }] }`. The request id is + # what its support asks for first, so it is appended after the truncation + # rather than being what a long body pushes out. + def error_message(parsed) + return parsed.to_s[0, 500] unless parsed.is_a?(Hash) + + message = join_errors(parsed['errors']) + message = parsed.to_json if message.empty? + + append_request_id(message[0, 500], parsed['request_id']) + end + + def join_errors(errors) + Array(errors).filter_map do |error| + next error unless error.is_a?(Hash) + + [error['code'], error['message']].compact.join(': ') + end.join('; ') + end + + def append_request_id(message, request_id) + return message unless request_id + + "#{message} (request_id: #{request_id})" + end + + def parse_body(body) + return body unless body.is_a?(String) && !body.empty? + + JSON.parse(body) + rescue JSON::ParserError + body + end + + def connection + @connection ||= build_connection( + retry_policy: @configuration.retry_policy, + timeout: @configuration.timeout, + open_timeout: @configuration.open_timeout + ) + end + + # For what is read while the datasource is being constructed -- the + # custom-attribute introspection above all: short timeouts and one quick + # retry, so a slow Intercom cannot turn a Rails boot into minutes of + # waiting. Memoized separately from `connection`, which keeps the patience + # every later request is entitled to. + def boot_connection + @boot_connection ||= build_connection( + retry_policy: @configuration.boot_retry_policy, + timeout: @configuration.boot_timeout, + open_timeout: @configuration.boot_open_timeout + ) + end + + # Middleware order is deliberate: `raise_error` sits outside the JSON parser + # so it raises with an already-parsed body, and `retry` sits innermost so it + # inspects raw statuses -- behind `raise_error` it would never see a 429. + # + # The throttle goes inside `retry`, which is what makes a replay wait for + # the window like a first attempt, and what lets the 429's own headers reach + # the limiter: outside it, the middleware would run once for a request that + # reached Intercom three times. + # + # How long a request may take is the caller's to state; everything else is + # the same on every connection this builds, the limiter included -- a second + # limiter would meter in a window of its own and spend the budget twice. + def build_connection(retry_policy:, timeout:, open_timeout:) + Faraday.new(url: @configuration.url) do |f| + f.request :json + f.response :raise_error + f.response :json + f.request :retry, **retry_policy.to_faraday_options + f.use Throttle, limiter: @configuration.rate_limiter if @configuration.rate_limiter + f.headers['Authorization'] = "Bearer #{@configuration.access_token}" + f.headers['Accept'] = 'application/json' + f.headers['Intercom-Version'] = @configuration.api_version + f.headers['User-Agent'] = "forest_admin_datasource_intercom/#{VERSION}" + f.options.open_timeout = open_timeout + f.options.timeout = timeout + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/configuration.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/configuration.rb new file mode 100644 index 000000000..65a3013c8 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/configuration.rb @@ -0,0 +1,112 @@ +module ForestAdminDatasourceIntercom + class Configuration + # A workspace is hosted in one region and answers in that region only. The + # host is therefore a configuration parameter rather than a constant: + # `api.intercom.io` does route to the right region, but a customer under + # GDPR wants its requests to reach the European host and nothing else. + REGION_HOSTS = { + us: 'https://api.intercom.io', + eu: 'https://api.eu.intercom.io', + au: 'https://api.au.intercom.io' + }.freeze + + DEFAULT_REGION = :us + + # Without an explicit version a request follows the workspace's own default, + # which an operator can change on Intercom's side -- and the payloads change + # shape under us. Pinned to what the spike ran against; 2.14 and 2.16 both + # answered, and the response echoes the version back, so `Client#me` + # verifies at boot that the pin was honoured. + DEFAULT_API_VERSION = '2.16'.freeze + + attr_reader :access_token, :region, :base_url, :api_version, :open_timeout, :timeout, + :retry_policy, :rate_limiter, :boot_open_timeout, :boot_timeout, :boot_retry_policy + + # `rate_limiter: nil` takes the pacing out of the stack, leaving the 429 + # retry as the only rate-limit handling. For a deployment that meters on its + # own side, or one that would rather see the 429. + # + # The `boot_` trio governs what the datasource reads while it is being + # constructed -- the custom-attribute introspection above all -- where the + # wait is a Rails boot the operator sits through rather than a request that + # has already returned a page. + def initialize(access_token:, region: nil, base_url: nil, api_version: DEFAULT_API_VERSION, + open_timeout: 5, timeout: 30, retry_policy: RetryPolicy.new, + rate_limiter: RateLimiter.new, boot_open_timeout: 3, boot_timeout: 10, + boot_retry_policy: RetryPolicy.boot) + @access_token = access_token + @region = (region || DEFAULT_REGION).to_s.downcase.to_sym + @base_url = base_url + @api_version = api_version.to_s + @open_timeout = open_timeout + @timeout = timeout + @retry_policy = retry_policy + @rate_limiter = rate_limiter + @boot_open_timeout = boot_open_timeout + @boot_timeout = boot_timeout + @boot_retry_policy = boot_retry_policy + validate! + end + + # An explicit `base_url` wins over the region: it is what points the client + # at a mock server or an egress proxy, neither of which is a region. + def url + @url ||= (@base_url || REGION_HOSTS.fetch(@region)).chomp('/') + end + + # Whatever precedes the endpoint in the path, for a base url mounted under a + # subpath. Empty against the API itself. + def base_path + @base_path ||= URI.parse(url).path + end + + # `access_token` is a bearer credential, and nothing prints a Configuration + # on purpose: what reaches an `inspect` is a Rails error page, or a + # `logger.debug` of something holding one. The default would put the token + # in clear there. `Client` and `Datasource` mask their own for the same + # reason -- together they cut every path from an object this package hands + # out to the credential. + def inspect + "#<#{self.class.name} url=#{url.inspect} api_version=#{@api_version.inspect} access_token=[FILTERED]>" + end + + private + + def validate! + raise ConfigurationError, 'ForestAdminDatasourceIntercom missing required config: access_token' if + blank?(@access_token) + + validate_region! + validate_base_url! + raise ConfigurationError, 'ForestAdminDatasourceIntercom api_version cannot be empty' if blank?(@api_version) + end + + def validate_region! + return if @base_url || REGION_HOSTS.key?(@region) + + raise ConfigurationError, + "ForestAdminDatasourceIntercom unknown region #{@region.inspect}: " \ + "expected one of #{REGION_HOSTS.keys.map(&:inspect).join(", ")}, or an explicit base_url." + end + + # A base url that is not absolute makes Faraday resolve every path against + # the process's working directory instead of Intercom, which surfaces much + # later as a connection failure naming nothing. + def validate_base_url! + return if @base_url.nil? + + uri = URI.parse(@base_url) + return if uri.is_a?(URI::HTTP) && !blank?(uri.host) + + raise ConfigurationError, + "ForestAdminDatasourceIntercom base_url must be an absolute http(s) url, got #{@base_url.inspect}" + rescue URI::InvalidURIError + raise ConfigurationError, + "ForestAdminDatasourceIntercom base_url is not a valid url: #{@base_url.inspect}" + end + + def blank?(value) + value.nil? || value.to_s.strip.empty? + end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb index 6a1695296..6b8b07315 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb @@ -1,18 +1,24 @@ module ForestAdminDatasourceIntercom - # Boot skeleton: it registers no collection yet. Configuration and the Faraday - # client come next, then the collections, each behind its own pull request -- - # so this one is what proves the package is wired into the monorepo (rubocop, - # rspec, coverage, release) before any behaviour depends on it. + # Boot skeleton: it configures a client and registers no collection yet. The + # collections follow in their own pull requests, each one bringing the + # endpoints it reads. class Datasource < ForestAdminDatasourceToolkit::Datasource - def initialize - super + attr_reader :client, :configuration + + def initialize(access_token:, **options) + super() + @configuration = Configuration.new(access_token: access_token, **options) + @client = Client.new(@configuration) + register_collections end # The datasource is what a Rails error page or a `logger.debug` is likeliest - # to print, and it will soon hold the client carrying the access token. - # Cutting the default dump here also spares the recursive walk of a - # datasource and its collections pointing at each other. + # to print, and it holds the client whose connections carry the access token. + # Every collection will reach that token the same way, through the + # `@datasource` the toolkit's Collection keeps, so cutting the chain here + # covers them too -- and spares the recursive dump the default `inspect` + # walks into, a datasource and its collections pointing at each other. def inspect "#<#{self.class.name} collections=#{collections.keys.inspect}>" end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/rate_limiter.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/rate_limiter.rb new file mode 100644 index 000000000..6b37a5794 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/rate_limiter.rb @@ -0,0 +1,169 @@ +module ForestAdminDatasourceIntercom + # Paces requests on what Intercom says is left of the current window, so the + # budget is spent rather than exceeded. + # + # Intercom meters the app and, above it, the whole workspace -- 25 000 + # requests a minute shared with every other private app the customer runs -- + # and it allocates that budget in 10-second windows: the measured + # `x-ratelimit-limit` is 1667, not 10 000. A burst of 3 000 requests in two + # seconds therefore takes a 429 while the minute's budget is barely touched, + # which is why what matters here is the instantaneous rate and not a volume + # per minute. + # + # Unlike an API that only documents its budgets, Intercom reports the state of + # the window on every response, so this is driven by those headers rather than + # by a table: what is left, and when it refills. This sits in front of the 429 + # retry rather than replacing it -- the retry stays the backstop for the part + # of the workspace budget spent by traffic this process never sees. + # + # One limiter per Configuration, hence per token, since that is what Intercom + # meters. + class RateLimiter + # Intercom's allocation window. Only used as the ceiling below: the reset + # instant itself always comes from the response. + WINDOW = 10.0 + + # How far past the reset a request may be held. A little over one window, so + # a full window can be waited out, and no more: past this the wait is not + # Intercom's window emptying but a clock disagreeing. + DEFAULT_MAX_WAIT = 12.0 + + attr_reader :max_wait + + def initialize(max_wait: DEFAULT_MAX_WAIT, now: nil, sleeper: nil) + @max_wait = max_wait.to_f + # Wall clock rather than monotonic on purpose: `X-RateLimit-Reset` is an + # absolute epoch second on Intercom's clock, so the two have to be + # comparable. `clamp_wait` is what keeps a skewed clock from turning that + # comparison into a long sleep. + @now = now || -> { Time.now.to_f } + @sleeper = sleeper || ->(seconds) { sleep(seconds) } + @mutex = Mutex.new + @remaining = nil + @reset_at = nil + @limit = nil + @warned_at = nil + end + + # Blocks until the current window has room, then returns. Called once per + # attempt, retries included: a replayed request spends the budget a first + # one did. + def acquire + wait, declined = @mutex.synchronize { plan_wait } + + warn_saturated(declined) if declined + return if wait <= 0 + + @sleeper.call(wait) + end + + # What a response says about the window it was answered in. Called on every + # response, the 429 included -- that one carries the most useful reset of + # all. + def observe(headers) + limit = integer_header(headers, 'x-ratelimit-limit') + remaining = integer_header(headers, 'x-ratelimit-remaining') + reset_at = integer_header(headers, 'x-ratelimit-reset') + return if remaining.nil? && reset_at.nil? + + @mutex.synchronize { record(limit, remaining, reset_at) } + end + + private + + # A local decrement per request on top of what the headers report: several + # requests can be in flight before any of them comes back, and a `remaining` + # that only ever moves on a response lets all of them through on the same + # stale figure. + # + # Returns the wait the caller owes and, when the reset is too far out to be + # waited for, the wait that was declined -- nil otherwise. Both are settled + # here, the second reading shared state like the first: it is nil on every + # bypass but the first of a window, so the log line is not repeated. + def plan_wait + @remaining -= 1 if @remaining + return [0, nil] unless exhausted? + + wait = clamp_wait(@reset_at - @now.call) + return [0, nil] if wait <= 0 + return [0, first_warning? ? wait : nil] if wait > @max_wait + + [wait, nil] + end + + # Nothing left in a window that has not refilled yet. An unknown state -- + # before the first response -- is not exhaustion: the first request is what + # discovers the budget. + def exhausted? + !@remaining.nil? && @remaining <= 0 && !@reset_at.nil? + end + + # Intercom's reset is a timestamp from its clock, and the two clocks can + # disagree by more than the window is long. A wait longer than a window plus + # its own slack is that disagreement rather than a window emptying, so it is + # cut back to something a caller can afford to wait. + def clamp_wait(seconds) + return 0.0 if seconds <= 0 + + [seconds, WINDOW + @max_wait].min + end + + # A window is adopted whole. A response answered in an older window than the + # one already recorded is ignored: replies come back out of order, and one + # from the previous window would otherwise resurrect a budget already spent. + # + # Within the same window the smaller `remaining` wins, so the local + # decrements of in-flight requests are not undone by a response that left + # Intercom before they were made. + def record(limit, remaining, reset_at) + return if reset_at && @reset_at && reset_at < @reset_at + + new_window = reset_at && (@reset_at.nil? || reset_at > @reset_at) + @reset_at = reset_at if reset_at + @limit = limit if limit + return if remaining.nil? + + @remaining = new_window || @remaining.nil? ? remaining : [remaining, @remaining].min + end + + # One line per window. What the warning reports is a saturation that lasts, + # so a line per request puts one on every request it describes -- hundreds + # of them, burying the first, which is the only one the operator needed. + def first_warning? + now = @now.call + return false if @warned_at && now - @warned_at < WINDOW + + @warned_at = now + true + end + + def warn_saturated(wait) + ForestAdminDatasourceIntercom.logger.warn( + "[forest_admin_datasource_intercom] the Intercom rate-limit window is spent (limit #{@limit || "unknown"} " \ + "per #{WINDOW.round}s) and its reset is #{wait.round(1)}s out, past the #{@max_wait.round(1)}s this waits. " \ + 'Letting the request through -- Intercom may answer 429, which the client retries. A reset this far out ' \ + "usually means this host's clock disagrees with Intercom's. Reported once per #{WINDOW.round}s." + ) + end + + def integer_header(headers, name) + value = header_value(headers, name) + return nil if value.nil? || value.to_s.strip.empty? + + Integer(value.to_s.strip, exception: false) + end + + # Faraday hands over headers that look themselves up case-insensitively, but + # what reaches here is whatever the middleware was given -- a plain hash + # included -- and HTTP header names are case-insensitive on the wire. + def header_value(headers, name) + return nil if headers.nil? + + direct = headers[name] + return direct unless direct.nil? + return nil unless headers.respond_to?(:find) + + headers.find { |key, _value| key.to_s.downcase == name }&.last + end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/retry_policy.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/retry_policy.rb new file mode 100644 index 000000000..249b74eb2 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/retry_policy.rb @@ -0,0 +1,72 @@ +module ForestAdminDatasourceIntercom + # Everything governing how the client reacts to a failed request, in one + # place: which statuses and exceptions are worth another attempt, on which + # verbs, and how long to wait. + class RetryPolicy + # Intercom allocates its quota in 10-second windows, so a 429 is recovered + # from within one of them -- unlike an API metering by the minute. The cap + # still has to cover a whole window: faraday-retry gives up outright when + # Retry-After exceeds max_interval, which would turn the 429 retry into an + # immediate give-up exactly when it matters. + DEFAULT_MAX_INTERVAL = 12 + + STATUSES = [429, 500, 502, 503, 504].freeze + + # faraday-retry's defaults plus ConnectionFailed: a dropped connection is + # exactly the transient failure a resilient client should absorb, and it is + # not retried out of the box. + EXCEPTIONS = [ + Errno::ETIMEDOUT, 'Timeout::Error', Faraday::TimeoutError, + Faraday::RetriableResponse, Faraday::ConnectionFailed + ].freeze + + # The verbs that change nothing, so any transient failure is worth another + # attempt. Narrower than faraday-retry's idempotent default: a 502 or a + # dropped connection on the way back from a POST Intercom did perform would + # be replayed into a second reply on the conversation, or a second ticket. + # + # A 429 stays safe to retry on any verb, Intercom having rejected the + # request before processing it, and travels through retry_if rather than + # through this list: faraday-retry ORs the two, so `methods` can only widen + # the set, never restrict it. + RETRYABLE_METHODS = %i[get head options].freeze + RETRY_IF = ->(env, _exception) { env[:status] == 429 } + + # The cap for a call that must not hold the boot, deliberately below a + # rate-limit window where DEFAULT_MAX_INTERVAL sits above it: a Retry-After + # past the cap makes faraday-retry abandon outright, which is what turns a + # 429 at boot into an immediate give-up rather than a window of waiting per + # attempt. + BOOT_MAX_INTERVAL = 2 + + BACKOFF_FACTOR = 2 + + attr_reader :max_retries, :interval, :max_interval + + # One retry rather than none, for what is read once and never revisited: a + # transient failure there costs its result for the whole life of the + # process, and half a second absorbs the hiccup without waiting a 429 out. + def self.boot + new(max_retries: 1, interval: 0.5, max_interval: BOOT_MAX_INTERVAL) + end + + def initialize(max_retries: 3, interval: 0.5, max_interval: DEFAULT_MAX_INTERVAL) + @max_retries = max_retries + @interval = interval + @max_interval = max_interval + end + + def to_faraday_options + { + max: @max_retries, + interval: @interval, + max_interval: @max_interval, + backoff_factor: BACKOFF_FACTOR, + retry_statuses: STATUSES, + exceptions: EXCEPTIONS, + methods: RETRYABLE_METHODS, + retry_if: RETRY_IF + } + end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/throttle.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/throttle.rb new file mode 100644 index 000000000..fa8fdad3b --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/throttle.rb @@ -0,0 +1,19 @@ +module ForestAdminDatasourceIntercom + # Holds a request until the rate-limit window has room, and feeds the window + # back what the response says about it. A middleware rather than a call in + # each client method: there is one code path for every request here, where the + # client has one per endpoint, and this one also covers the requests the client + # never issues itself -- the replays `retry` performs. + class Throttle < Faraday::Middleware + def initialize(app, limiter:) + super(app) + @limiter = limiter + end + + def call(env) + @limiter.acquire + + @app.call(env).on_complete { |response_env| @limiter.observe(response_env[:response_headers]) } + end + end +end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/client_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/client_spec.rb new file mode 100644 index 000000000..a450db87d --- /dev/null +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/client_spec.rb @@ -0,0 +1,237 @@ +module ForestAdminDatasourceIntercom + RSpec.describe Client do + subject(:client) { described_class.new(configuration) } + + let(:retry_policy) { RetryPolicy.new(max_retries: 2, interval: 0) } + let(:configuration) { Configuration.new(access_token: 's3cr3t', retry_policy: retry_policy, rate_limiter: nil) } + let(:base) { configuration.url } + + def json(payload, status = 200, headers = {}) + { status: status, + body: payload.is_a?(String) ? payload : payload.to_json, + headers: { 'Content-Type' => 'application/json' }.merge(headers) } + end + + describe 'authentication and version pinning' do + before { stub_request(:get, "#{base}/me").to_return(json({ 'type' => 'admin' })) } + + it 'sends the access token as a bearer token' do + client.me + + expect(WebMock).to have_requested(:get, "#{base}/me") + .with(headers: { 'Authorization' => 'Bearer s3cr3t', 'Accept' => 'application/json' }) + end + + # Without the header the request follows the workspace's own default + # version, which an operator can change on Intercom's side. + it 'pins the API version on every request' do + client.me + + expect(WebMock).to have_requested(:get, "#{base}/me").with(headers: { 'Intercom-Version' => '2.16' }) + end + + it 'advertises a versioned user agent' do + client.me + + expect(WebMock).to have_requested(:get, "#{base}/me") + .with(headers: { 'User-Agent' => "forest_admin_datasource_intercom/#{VERSION}" }) + end + end + + describe '#me' do + it 'returns the admin the token belongs to' do + stub_request(:get, "#{base}/me").to_return(json('type' => 'admin', 'id' => '1', 'email' => 'a@b.test')) + + expect(client.me).to include('id' => '1', 'email' => 'a@b.test') + end + + it 'reaches the regional host it was configured for' do + eu = described_class.new(Configuration.new(access_token: 's3cr3t', region: :eu, rate_limiter: nil)) + stub_request(:get, 'https://api.eu.intercom.io/me').to_return(json('type' => 'admin')) + + eu.me + + expect(WebMock).to have_requested(:get, 'https://api.eu.intercom.io/me') + end + end + + describe 'the version Intercom actually served' do + # Intercom echoes the version it served. A mismatch means the payloads may + # not be the ones this datasource expects, which is worth saying out loud + # -- and worth saying rather than raising: running against a version we + # did not ask for beats not running. + it 'warns when it differs from the pinned one' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + stub_request(:get, "#{base}/me").to_return(json({ 'type' => 'admin' }, 200, 'intercom-version' => '2.14')) + + client.me + + expect(ForestAdminDatasourceIntercom.logger).to have_received(:warn).with(/asked.*2\.16.*served 2\.14/m) + end + + it 'stays quiet when the pin was honoured' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + stub_request(:get, "#{base}/me").to_return(json({ 'type' => 'admin' }, 200, 'intercom-version' => '2.16')) + + client.me + + expect(ForestAdminDatasourceIntercom.logger).not_to have_received(:warn) + end + + it 'stays quiet when Intercom echoes nothing' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + stub_request(:get, "#{base}/me").to_return(json('type' => 'admin')) + + client.me + + expect(ForestAdminDatasourceIntercom.logger).not_to have_received(:warn) + end + end + + describe 'failures' do + it "carries Intercom's status, parsed body and error text" do + body = { 'type' => 'error.list', 'request_id' => 'req_1', + 'errors' => [{ 'code' => 'unauthorized', 'message' => 'Access Token Invalid' }] } + stub_request(:get, "#{base}/me").to_return(json(body, 401)) + + expect { client.me }.to raise_error(APIError) { |error| + expect(error.message).to eq('Intercom API call failed: me: HTTP 401 unauthorized: ' \ + 'Access Token Invalid (request_id: req_1)') + expect(error.status).to eq(401) + expect(error.body).to eq(body) + } + end + + it 'joins the several errors one response can carry' do + body = { 'errors' => [{ 'code' => 'parameter_invalid', 'message' => 'per_page' }, + { 'code' => 'parameter_invalid', 'message' => 'starting_after' }] } + stub_request(:get, "#{base}/me").to_return(json(body, 400)) + + expect { client.me }.to raise_error(APIError, /per_page; parameter_invalid: starting_after/) + end + + it 'falls back to the whole body when the shape is not the documented one' do + stub_request(:get, "#{base}/me").to_return(json({ 'oops' => true }, 500)) + + expect { client.me }.to raise_error(APIError, /\{"oops":true\}/) + end + + it 'keeps a body that is not JSON at all, which is what a gateway answers' do + stub_request(:get, "#{base}/me").to_return(status: 502, body: 'bad gateway') + + expect { client.me }.to raise_error(APIError) { |error| + expect(error.status).to eq(502) + expect(error.body).to eq('bad gateway') + } + end + + # No status to report: the request never reached Intercom, so there is + # nothing of its to surface. + it 'reports a dropped connection without a status' do + stub_request(:get, "#{base}/me").to_raise(Faraday::ConnectionFailed.new('closed')) + + expect { client.me }.to raise_error(APIError) { |error| + expect(error.message).to include('Faraday::ConnectionFailed') + expect(error.status).to be_nil + } + end + + it 'replays a 429 rather than surfacing it' do + stub_request(:get, "#{base}/me") + .to_return(json({ 'errors' => [{ 'code' => 'rate_limit_exceeded' }] }, 429)) + .then.to_return(json('type' => 'admin')) + + expect(client.me).to eq('type' => 'admin') + end + + it 'gives up on a 429 that outlasts the retries, saying which endpoint' do + stub_request(:get, "#{base}/me").to_return(json({ 'errors' => [{ 'code' => 'rate_limit_exceeded' }] }, 429)) + + expect { client.me }.to raise_error(APIError, /me: HTTP 429 rate_limit_exceeded/) + end + + # Whatever else goes wrong on the way, a caller of this client only ever + # has to rescue APIError -- and the message names the operation, since a + # failure with no endpoint in it is a failure nobody can place. + it 'still names the operation when the failure is not one it expected' do + stub_request(:get, "#{base}/me").to_raise(ArgumentError.new('unexpected')) + + expect { client.me }.to raise_error(APIError, /me: ArgumentError: unexpected/) + end + end + + describe '.bounded_per_page' do + # Intercom answers `invalid_per_page` past 150 instead of clamping, so a + # page size is bounded before it is sent or the list view breaks. + it 'caps a page size at what Intercom accepts' do + expect(described_class.bounded_per_page(200)).to eq(150) + end + + it 'leaves an acceptable size alone' do + expect(described_class.bounded_per_page(50)).to eq(50) + end + + it 'asks for one record rather than none, an empty page being no answer' do + expect([described_class.bounded_per_page(0), described_class.bounded_per_page(-5)]).to eq([1, 1]) + end + end + + describe 'the boot connection' do + # What is read while the datasource is being constructed waits far less + # than a request that already has a page on screen: the wait there is + # minutes of Rails boot the operator sits through. + it 'honours the configured boot timeouts' do + booted = described_class.new(Configuration.new(access_token: 's3cr3t', boot_open_timeout: 1, boot_timeout: 2)) + conn = booted.send(:boot_connection) + + expect(conn.options).to have_attributes(open_timeout: 1, timeout: 2) + end + + it 'keeps the patience of a regular request on the regular connection' do + expect(client.send(:connection).options).to have_attributes(open_timeout: 5, timeout: 30) + end + + it 'reads through it when asked to' do + stub_request(:get, "#{base}/me").to_return(json('type' => 'admin')) + + expect(client.me(boot: true)).to eq('type' => 'admin') + end + end + + describe 'pacing' do + let(:limiter) { instance_double(RateLimiter, acquire: nil, observe: nil) } + let(:paced) do + described_class.new(Configuration.new(access_token: 's3cr3t', retry_policy: retry_policy, + rate_limiter: limiter)) + end + + it 'asks the limiter for room, and feeds it the window back' do + stub_request(:get, "#{base}/me").to_return(json({ 'type' => 'admin' }, 200, + 'x-ratelimit-remaining' => '1666')) + + paced.me + + expect(limiter).to have_received(:acquire) + expect(limiter).to have_received(:observe).with(hash_including('x-ratelimit-remaining' => '1666')) + end + + # The throttle sits inside the retry, so a replay waits for the window + # like a first attempt rather than going out on a budget already spent. + it 'asks again for every replay, not once per call' do + stub_request(:get, "#{base}/me") + .to_return(json({}, 429)).then.to_return(json('type' => 'admin')) + + paced.me + + expect(limiter).to have_received(:acquire).twice + end + end + + describe '#inspect' do + it 'never prints the token its connections carry' do + expect(client.inspect).to include(base) + expect(client.inspect).not_to include('s3cr3t') + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/configuration_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/configuration_spec.rb new file mode 100644 index 000000000..64b9079a1 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/configuration_spec.rb @@ -0,0 +1,98 @@ +module ForestAdminDatasourceIntercom + RSpec.describe Configuration do + subject(:configuration) { described_class.new(access_token: 's3cr3t') } + + it 'defaults to the US host, since that is where a workspace lands unasked' do + expect(configuration.url).to eq('https://api.intercom.io') + end + + it 'pins the API version the spike ran against' do + expect(configuration.api_version).to eq('2.16') + end + + it 'points at the regional host it is given' do + expect(described_class.new(access_token: 's3cr3t', region: :eu).url).to eq('https://api.eu.intercom.io') + end + + it 'takes the region as a string too' do + expect(described_class.new(access_token: 's3cr3t', region: 'AU').url).to eq('https://api.au.intercom.io') + end + + it 'lets an explicit base_url win over the region, for a proxy or a mock server' do + configured = described_class.new(access_token: 's3cr3t', region: :eu, base_url: 'https://intercom.test/api/') + + expect(configured.url).to eq('https://intercom.test/api') + end + + it 'reports the subpath a base_url is mounted under' do + configured = described_class.new(access_token: 's3cr3t', base_url: 'https://intercom.test/api') + + expect(configured.base_path).to eq('/api') + end + + it 'reports no subpath against the API itself' do + expect(configuration.base_path).to eq('') + end + + describe 'validation' do + it 'refuses a missing access token' do + expect { described_class.new(access_token: nil) } + .to raise_error(ConfigurationError, /missing required config: access_token/) + end + + it 'refuses a blank access token' do + expect { described_class.new(access_token: ' ') } + .to raise_error(ConfigurationError, /access_token/) + end + + it 'names the regions it knows when handed one it does not' do + expect { described_class.new(access_token: 's3cr3t', region: :moon) } + .to raise_error(ConfigurationError, /unknown region :moon.*:us, :eu, :au/m) + end + + # A relative base_url makes Faraday resolve paths against the working + # directory, which surfaces much later as a failure naming nothing. + it 'refuses a base_url that is not absolute' do + expect { described_class.new(access_token: 's3cr3t', base_url: 'api.intercom.io') } + .to raise_error(ConfigurationError, /must be an absolute http\(s\) url/) + end + + it 'refuses a base_url that is not a url at all' do + expect { described_class.new(access_token: 's3cr3t', base_url: 'http://[bad') } + .to raise_error(ConfigurationError, /not a valid url/) + end + + it 'refuses an empty api_version, which would let the workspace default decide' do + expect { described_class.new(access_token: 's3cr3t', api_version: '') } + .to raise_error(ConfigurationError, /api_version cannot be empty/) + end + end + + describe 'defaults' do + it 'paces requests and retries unless told otherwise' do + expect(configuration).to have_attributes(rate_limiter: an_instance_of(RateLimiter), + retry_policy: an_instance_of(RetryPolicy)) + end + + it 'is patient on a request and impatient on the boot' do + expect(configuration).to have_attributes(timeout: 30, open_timeout: 5, boot_timeout: 10, + boot_open_timeout: 3) + end + + it 'takes the pacing out of the stack when handed no limiter' do + expect(described_class.new(access_token: 's3cr3t', rate_limiter: nil).rate_limiter).to be_nil + end + end + + describe '#inspect' do + it 'never prints the bearer token' do + expect(configuration.inspect).to include('[FILTERED]') + expect(configuration.inspect).not_to include('s3cr3t') + end + + it 'still names the host and version, which is what one inspects it for' do + expect(configuration.inspect).to include('https://api.intercom.io', '2.16') + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb index da24e86f3..70bc76281 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb @@ -1,6 +1,6 @@ module ForestAdminDatasourceIntercom RSpec.describe Datasource do - subject(:datasource) { described_class.new } + subject(:datasource) { described_class.new(access_token: 's3cr3t') } it 'boots without reaching Intercom' do expect { datasource }.not_to raise_error @@ -10,8 +10,23 @@ module ForestAdminDatasourceIntercom expect(datasource.collections).to be_empty end + it 'configures a client from the options it is handed' do + configured = described_class.new(access_token: 's3cr3t', region: :eu) + + expect(configured.configuration.url).to eq('https://api.eu.intercom.io') + expect(configured.client).to be_a(Client) + end + + it 'refuses to boot on a configuration it cannot use' do + expect { described_class.new(access_token: nil) }.to raise_error(ConfigurationError) + end + it 'names the collections it holds when printed' do expect(datasource.inspect).to eq('#') end + + it 'never prints the token the client carries' do + expect(datasource.inspect).not_to include('s3cr3t') + end end end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/rate_limiter_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/rate_limiter_spec.rb new file mode 100644 index 000000000..78c234aa6 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/rate_limiter_spec.rb @@ -0,0 +1,150 @@ +module ForestAdminDatasourceIntercom + RSpec.describe RateLimiter do + # Sleeping moves the clock, the way it does outside a spec: a window waited + # out is a window that has refilled by the time the next request asks. + subject(:limiter) { build_limiter } + + let(:time) { [1_000.0] } + let(:slept) { [] } + + def build_limiter(**options) + sleeper = lambda do |seconds| + slept << seconds + time[0] += seconds + end + + described_class.new(now: -> { time[0] }, sleeper: sleeper, **options) + end + + # What Intercom answers with: the limit of the 10-second window, what is + # left of it, and the epoch second it refills at. + def headers(remaining:, reset_in: 4, limit: 1667) + { 'x-ratelimit-limit' => limit.to_s, + 'x-ratelimit-remaining' => remaining.to_s, + 'x-ratelimit-reset' => (time[0] + reset_in).to_i.to_s } + end + + it 'lets the first request through: the budget is what that request discovers' do + limiter.acquire + + expect(slept).to be_empty + end + + it 'lets a request through while the window still has room' do + limiter.observe(headers(remaining: 5)) + limiter.acquire + + expect(slept).to be_empty + end + + it 'waits for the reset once the window is spent' do + limiter.observe(headers(remaining: 0, reset_in: 4)) + limiter.acquire + + expect(slept).to eq([4.0]) + end + + it 'lets requests through again once the reset has passed' do + limiter.observe(headers(remaining: 0, reset_in: -1)) + limiter.acquire + + expect(slept).to be_empty + end + + # Several requests can be in flight before any of them answers, and a + # `remaining` that only moves on a response lets all of them through on the + # same stale figure. + it 'counts its own requests down rather than trusting the last response' do + limiter.observe(headers(remaining: 2, reset_in: 3)) + 3.times { limiter.acquire } + + expect(slept).to eq([3.0]) + end + + it 'ignores a response from a window older than the one it knows' do + limiter.observe(headers(remaining: 0, reset_in: 5)) + limiter.observe(headers(remaining: 100, reset_in: -10)) + limiter.acquire + + expect(slept).to eq([5.0]) + end + + it 'adopts a new window whole, generous remaining included' do + limiter.observe(headers(remaining: 0, reset_in: 2)) + limiter.observe(headers(remaining: 50, reset_in: 12)) + limiter.acquire + + expect(slept).to be_empty + end + + # A response that left Intercom before the requests now in flight were made + # must not hand their budget back. + it 'keeps the smaller remaining inside one window' do + limiter.observe(headers(remaining: 2, reset_in: 3)) + limiter.acquire + limiter.observe(headers(remaining: 99, reset_in: 3)) + 2.times { limiter.acquire } + + expect(slept).to eq([3.0]) + end + + it 'reads the headers whatever their case' do + limiter.observe('X-RateLimit-Remaining' => '0', 'X-RateLimit-Reset' => (time[0] + 6).to_i.to_s) + limiter.acquire + + expect(slept).to eq([6.0]) + end + + it 'ignores a response carrying no rate-limit headers at all' do + limiter.observe('content-type' => 'application/json') + limiter.acquire + + expect(slept).to be_empty + end + + it 'ignores a remaining that is not a number' do + limiter.observe('x-ratelimit-remaining' => 'many', 'x-ratelimit-reset' => (time[0] + 3).to_i.to_s) + limiter.acquire + + expect(slept).to be_empty + end + + it 'sleeps a reset out when it fits within the bound it was given' do + capped = build_limiter(max_wait: 2.0) + capped.observe(headers(remaining: 0, reset_in: 1)) + capped.acquire + + expect(slept).to eq([1.0]) + end + + describe 'a reset further out than one window' do + # Intercom's reset is a timestamp from its clock. One that far out is the + # two clocks disagreeing, not a window emptying, so the request goes + # through and the log says why -- waiting an hour would read as a hang. + before do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + limiter.observe(headers(remaining: 0, reset_in: 3_600)) + end + + it 'lets the request through instead of waiting it out' do + limiter.acquire + + expect(slept).to be_empty + end + + it 'says so once, not once per request' do + 3.times { limiter.acquire } + + expect(ForestAdminDatasourceIntercom.logger) + .to have_received(:warn).once.with(/rate-limit window is spent.*limit 1667/m) + end + end + + it 'sleeps for real when handed no sleeper' do + real = described_class.new(now: -> { 999.96 }) + real.observe('x-ratelimit-remaining' => '0', 'x-ratelimit-reset' => '1000') + + expect { real.acquire }.not_to raise_error + end + end +end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/retry_policy_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/retry_policy_spec.rb new file mode 100644 index 000000000..060565df4 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/retry_policy_spec.rb @@ -0,0 +1,49 @@ +module ForestAdminDatasourceIntercom + RSpec.describe RetryPolicy do + describe '#to_faraday_options' do + subject(:options) { described_class.new.to_faraday_options } + + it 'retries the statuses worth another attempt' do + expect(options[:retry_statuses]).to eq([429, 500, 502, 503, 504]) + end + + # A 502 on the way back from a POST Intercom did perform would be replayed + # into a second reply on the conversation. + it 'only replays the verbs that change nothing' do + expect(options[:methods]).to eq(%i[get head options]) + end + + it 'replays a 429 whatever the verb, Intercom having rejected it unprocessed' do + expect(options[:retry_if].call({ status: 429 }, nil)).to be(true) + end + + it 'leaves any other status to the methods list' do + expect(options[:retry_if].call({ status: 502 }, nil)).to be(false) + end + + # faraday-retry abandons outright when Retry-After exceeds max_interval, + # so the cap has to cover Intercom's whole 10-second window. + it 'waits out a full rate-limit window' do + expect(options[:max_interval]).to be > RateLimiter::WINDOW + end + + it 'absorbs a dropped connection, which faraday-retry does not by default' do + expect(options[:exceptions]).to include(Faraday::ConnectionFailed) + end + end + + describe '.boot' do + subject(:options) { described_class.boot.to_faraday_options } + + it 'retries once: a boot read is never revisited, and never worth a long wait' do + expect(options[:max]).to eq(1) + end + + # Below a rate-limit window on purpose: past the cap faraday-retry gives + # up at once, which is what keeps a 429 from holding the Rails boot. + it 'gives up rather than waiting a 429 out' do + expect(options[:max_interval]).to be < RateLimiter::WINDOW + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/throttle_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/throttle_spec.rb new file mode 100644 index 000000000..a2209a2a7 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/throttle_spec.rb @@ -0,0 +1,40 @@ +module ForestAdminDatasourceIntercom + RSpec.describe Throttle do + let(:limiter) { instance_double(RateLimiter, acquire: nil, observe: nil) } + let(:connection) do + Faraday.new(url: 'https://api.intercom.test') do |f| + f.use described_class, limiter: limiter + end + end + + before do + stub_request(:get, 'https://api.intercom.test/me') + .to_return(status: 200, body: '{}', + headers: { 'Content-Type' => 'application/json', 'x-ratelimit-remaining' => '7' }) + end + + it 'asks for room before the request leaves' do + connection.get('me') + + expect(limiter).to have_received(:acquire) + end + + it 'hands the window back what the response says about it' do + connection.get('me') + + expect(limiter).to have_received(:observe).with(hash_including('x-ratelimit-remaining' => '7')) + end + + # The 429 carries the most useful reset of all, so the observation cannot be + # limited to the responses that succeeded. + it 'observes a rejected response too' do + stub_request(:get, 'https://api.intercom.test/me') + .to_return(status: 429, body: '{}', + headers: { 'Content-Type' => 'application/json', 'x-ratelimit-remaining' => '0' }) + + connection.get('me') + + expect(limiter).to have_received(:observe).with(hash_including('x-ratelimit-remaining' => '0')) + end + end +end From da59bc0bd4eced431bb943cdd3f42e1cd85908d5 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Mon, 31 Aug 2026 17:39:49 +0200 Subject: [PATCH 3/9] feat(intercom): walk cursor pages into a Forest window Third step of lot 1 (PRD-1112). Forest sends an offset/limit window; Intercom only hands out the page after a cursor and documents that jumping to page N is not supported. The walker bridges the two: it follows cursors until the window is covered, then slices it out. The walk is capped at 50 pages of 150 records -- not for the quota, which is generous, but for what an operator is willing to wait for and because page 200 of a list view answers no real question. Every truncation is logged naming the window it stopped in: a page that looks like the whole answer and is not is the failure this datasource exists to avoid. Records are deduplicated by id inside the walk. Intercom documents that a dataset modified between two paginated requests yields duplicates or missed records, and conversations move constantly -- two rows carrying one id is what a list view renders as two identical lines and a count that never adds up. The missed counterpart is inherent to cursor pagination and gets documented rather than papered over. The client gains the page envelope: records, the cursor the next page advertises, and the exact total_count that will feed Forest's record counter. An advertised next page whose cursor cannot be read is refused rather than taken for the last page -- including the url shape an older API version serves, whose query string is read instead. A `data` that is absent or is not a list is refused too: read as an empty page, it would hand the collection rows built out of envelope keys. Co-Authored-By: Claude Opus 5 (1M context) --- .../client.rb | 91 ++++++++- .../pagination/cursor_walker.rb | 126 +++++++++++++ .../client_spec.rb | 143 +++++++++++++++ .../pagination/cursor_walker_spec.rb | 173 ++++++++++++++++++ 4 files changed, 532 insertions(+), 1 deletion(-) create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/pagination/cursor_walker.rb create mode 100644 packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/pagination/cursor_walker_spec.rb diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/client.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/client.rb index 1f7b64292..f03faf2b8 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/client.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/client.rb @@ -6,12 +6,20 @@ module ForestAdminDatasourceIntercom # the quota headers, because the pacing is driven by them; and Intercom's own # error body, because that text is what an operator reads when an action # fails. - class Client + # Long by line count only: the public surface is one method per endpoint, and + # the rest is the envelope and error handling every one of them shares. + class Client # rubocop:disable Metrics/ClassLength # `per_page=200` is refused with `invalid_per_page` -- "must be an integer # between 0 and 150". There is no silent downgrade, so a page size is bounded # before it is sent or the list view breaks rather than shrinks. MAX_PER_PAGE = 150 + # `next_cursor` is nil as soon as Intercom stops advertising a next page, so + # callers never have to know how the absence is spelled on the wire. + # `total_count` is exact, filter included, which is what makes Forest's + # record counter and its "number of" charts one request each. + Page = Struct.new(:records, :next_cursor, :total_count, keyword_init: true) + def initialize(configuration) @configuration = configuration end @@ -31,6 +39,18 @@ def me(boot: false) end end + # One page of a cursor-paginated listing. `starting_after` is what the + # previous page advertised; nil asks for the first one. + # + # `CursorWalker` is what turns the offset/limit window a list view asks for + # into a sequence of these. + def list_page(path, per_page:, starting_after: nil, params: {}, boot: false) + query = params.merge('per_page' => self.class.bounded_per_page(per_page)) + query['starting_after'] = starting_after unless blank?(starting_after) + + must_succeed(path) { to_page(get(path, query, boot: boot).body, path) } + end + # The page size Intercom accepts, whatever was asked for. def self.bounded_per_page(size) value = size.to_i @@ -69,10 +89,79 @@ def verify_pinned_version(response) ) end + # Intercom wraps a listing in `{ "type": "list", "data": [...], + # "total_count": N, "pages": { "next": { "starting_after": "..." } } }`. + def to_page(body, operation) + Page.new(records: extract_list(body, operation), + next_cursor: next_cursor(body, operation), + total_count: extract_count(body)) + end + + # A listing whose `data` is absent or is not a list broke the contract. It + # is refused rather than read as an empty page: `Array()` would turn the + # envelope into `[key, value]` pairs the collection would serialize into + # rows holding nothing -- a page that looks answered and is empty. + def extract_list(body, operation) + data = body.is_a?(Hash) ? body['data'] : nil + return data if data.is_a?(Array) + + refuse_body_shape(operation, "'data' is not a list") + end + + # Absent on the last page, which is how the walk knows it is done. An older + # API version spells it as a url instead of an object -- and one can be + # served despite the pin, which is what the version echo warns about -- so + # the cursor is read out of its query string rather than the page being + # taken for the last one. + # + # Anything else is refused, an advertised page whose cursor cannot be read + # included: taking it for the last page would truncate the answer silently, + # which is worse than a failure naming what it could not read. + def next_cursor(body, operation) + advertised = body.is_a?(Hash) && body['pages'].is_a?(Hash) ? body['pages']['next'] : nil + return nil if advertised.nil? + + cursor = case advertised + when Hash then advertised['starting_after'] + when String then cursor_from_url(advertised) + end + + presence(cursor) || refuse_body_shape(operation, "'pages.next' carries no cursor this can follow") + end + + def cursor_from_url(url) + Faraday::Utils.parse_query(URI.parse(url).query.to_s)['starting_after'] + rescue URI::InvalidURIError + nil + end + + # nil rather than 0 when Intercom sends no count: zero is an answer, and + # this is the absence of one. + def extract_count(body) + count = body['total_count'] if body.is_a?(Hash) + count.is_a?(Numeric) ? count.to_i : nil + end + + def refuse_body_shape(operation, detail) + raise APIError.new("Intercom API call failed: #{operation}: unexpected response shape, #{detail}", status: nil) + end + + def presence(value) + blank?(value) ? nil : value + end + + def blank?(value) + value.nil? || value.to_s.empty? + end + def must_succeed(operation) yield rescue Faraday::Error => e raise api_error(operation, e) + rescue APIError + # Already mapped, with its status intact; re-wrapping would erase it -- + # a 404 read as "no such record" rather than as a failure, above all. + raise rescue StandardError => e raise APIError, "Intercom API call failed: #{operation}: #{e.class}: #{e.message}" end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/pagination/cursor_walker.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/pagination/cursor_walker.rb new file mode 100644 index 000000000..5132f0d8a --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/pagination/cursor_walker.rb @@ -0,0 +1,126 @@ +module ForestAdminDatasourceIntercom + module Pagination + # Forest asks for an offset/limit window; Intercom only knows how to hand + # out the page after a cursor, and documents that jumping to page N is not + # supported. Bridging the two means walking pages until the window is + # covered, then slicing it out. Reaching page 20 therefore costs 20 + # requests -- sequential ones, a cursor only being known once the page + # before it came back. + # + # The walk is capped for that reason rather than for the quota's: 10 000 + # requests a minute is generous enough that the caps below are about what an + # operator is willing to wait for, and about the fact that page 200 of a + # list view answers no real question (R9). Every truncation is logged -- + # never silent, since a page that looks like the whole answer and is not is + # the failure this datasource exists to avoid. + class CursorWalker + # 50 pages of 150 records. Intercom's quota lets these be generous: the + # walk is bounded by patience, and by the point past which a list view is + # not being read but scraped. + MAX_PAGES = 50 + MAX_RECORDS = 7_500 + + def initialize(max_pages: MAX_PAGES, max_records: MAX_RECORDS) + @max_pages = max_pages + @max_records = max_records + end + + # Yields `(per_page, cursor)` and expects a Client::Page back. + # + # A nil limit asks for every record past the offset: the walk then runs + # until Intercom says there is no page left, or until a cap stops it. That + # distinction is the whole point of accepting nil rather than a huge limit + # standing in for "everything": a walk told to collect a thousand records + # stops at a thousand having covered the window it was given, and reports + # nothing, while a walk told to collect everything and stopped by a cap + # knows it is handing back less than it was asked for, and says so. + def walk(offset:, limit:, &page_source) + offset = offset.to_i.clamp(0, nil) + limit = limit&.to_i + return [] if limit && !limit.positive? + + records = collect(offset, limit, &page_source) + + limit ? (records[offset, limit] || []) : records.drop(offset) + end + + private + + # The walk itself: pages are collected until the window is covered, the + # source says there is nothing left, or a cap stops it. The slicing is + # `walk`'s; this only decides how far to go. + def collect(offset, limit) + needed = limit && (offset + limit) + records = [] + cursor = nil + seen_ids = Set.new + seen_cursors = Set.new + pages = 0 + + loop do + page = yield(batch_size(needed, records.size), cursor) + records.concat(fresh(page.records, seen_ids)) + pages += 1 + + break if stop?(page, seen_cursors) + break if needed && records.size >= needed + + if capped?(pages, records.size) + log_truncation(offset: offset, limit: limit, pages: pages, collected: records.size) + break + end + + cursor = page.next_cursor + end + + records + end + + # Intercom documents that "if items are modified between paginated + # requests it is possible to see duplicate or missed records" -- and + # conversations move constantly, so a deep walk over them will see the + # same record twice. A duplicate is dropped here rather than being served + # as two rows carrying one id, which is what a list view would render as + # two identical lines and a record count that never adds up. The missing + # counterpart is inherent to cursor pagination and is documented instead. + # + # A record with no id is kept: it is not this walk's business to decide + # that a payload it does not recognise is not a record. + def fresh(records, seen_ids) + records.select { |record| record['id'].nil? || seen_ids.add?(record['id']) } + end + + # An empty page, a cursor that does not move and a cursor already followed + # all stop the walk. Intercom does none of the three today -- `pages.next` + # is simply absent on the last page -- but a walk driven by a remote value + # stops on its own terms rather than on the caps only: a cycle wider than + # one page would otherwise collect the same pages until a cap cut it + # short. + def stop?(page, seen_cursors) + page.next_cursor.nil? || page.records.empty? || !seen_cursors.add?(page.next_cursor) + end + + def capped?(pages, collected) + pages >= @max_pages || collected >= @max_records + end + + # Bounded by the record budget left, and by the window still missing when + # there is one, so the walk never collects past @max_records. `Client` + # bounds it again to what Intercom accepts. + def batch_size(needed, collected) + budget = @max_records - collected + budget = [needed - collected, budget].min if needed + Client.bounded_per_page(budget) + end + + def log_truncation(offset:, limit:, pages:, collected:) + window = limit ? "offset=#{offset} limit=#{limit}" : "every record past offset=#{offset}" + ForestAdminDatasourceIntercom.logger.warn( + "[forest_admin_datasource_intercom] Stopped paginating after #{pages} page(s) / " \ + "#{collected} record(s) while fetching #{window}; results are truncated. " \ + 'Narrow the filter to reach records past this point.' + ) + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/client_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/client_spec.rb index a450db87d..fede1d9dd 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/client_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/client_spec.rb @@ -160,6 +160,149 @@ def json(payload, status = 200, headers = {}) end end + describe '#list_page' do + def list_body(data, next_page: nil, total: 2) + body = { 'type' => 'list', 'data' => data, 'total_count' => total, + 'pages' => { 'type' => 'pages', 'page' => 1, 'per_page' => 50 } } + body['pages']['next'] = next_page unless next_page.nil? + body + end + + it 'reads the records, the next cursor and the exact count off one response' do + body = list_body([{ 'id' => '1' }], next_page: { 'starting_after' => 'cursor_2' }) + stub_request(:get, "#{base}/conversations").with(query: { 'per_page' => '150' }).to_return(json(body)) + + page = client.list_page('conversations', per_page: 150) + + expect(page.records).to eq([{ 'id' => '1' }]) + expect(page.next_cursor).to eq('cursor_2') + expect(page.total_count).to eq(2) + end + + it 'sends the cursor the previous page advertised' do + stub_request(:get, "#{base}/conversations") + .with(query: { 'per_page' => '50', 'starting_after' => 'cursor_2' }) + .to_return(json(list_body([]))) + + client.list_page('conversations', per_page: 50, starting_after: 'cursor_2') + + expect(WebMock).to have_requested(:get, "#{base}/conversations") + .with(query: { 'per_page' => '50', 'starting_after' => 'cursor_2' }) + end + + it 'bounds the page size before sending it, Intercom refusing rather than clamping' do + stub_request(:get, "#{base}/conversations").with(query: { 'per_page' => '150' }) + .to_return(json(list_body([]))) + + client.list_page('conversations', per_page: 500) + + expect(WebMock).to have_requested(:get, "#{base}/conversations").with(query: { 'per_page' => '150' }) + end + + it 'carries the parameters an endpoint of its own needs' do + stub_request(:get, "#{base}/conversations") + .with(query: { 'per_page' => '150', 'display_as' => 'plaintext' }).to_return(json(list_body([]))) + + client.list_page('conversations', per_page: 150, params: { 'display_as' => 'plaintext' }) + + expect(WebMock).to have_requested(:get, "#{base}/conversations") + .with(query: hash_including('display_as' => 'plaintext')) + end + + # The last page simply carries no `pages.next`, which is what stops a walk. + it 'reports no next cursor on the last page' do + stub_request(:get, "#{base}/conversations").with(query: hash_including({})) + .to_return(json(list_body([{ 'id' => '1' }]))) + + expect(client.list_page('conversations', per_page: 150).next_cursor).to be_nil + end + + # An older API version spells `pages.next` as a url, and one can be served + # despite the pin -- reading the cursor out of it beats taking the page for + # the last one and truncating the answer. + it 'reads the cursor out of a next page spelled as a url' do + url = "#{base}/conversations?per_page=50&starting_after=cursor_9" + stub_request(:get, "#{base}/conversations").with(query: hash_including({})) + .to_return(json(list_body([], next_page: url))) + + expect(client.list_page('conversations', per_page: 50).next_cursor).to eq('cursor_9') + end + + # An advertised page taken for the last one is a silently truncated + # answer, so every unreadable shape is refused rather than dropped. + it 'refuses a next-page url carrying no cursor' do + body = list_body([], next_page: "#{base}/conversations?per_page=50") + stub_request(:get, "#{base}/conversations").with(query: hash_including({})).to_return(json(body)) + + expect { client.list_page('conversations', per_page: 50) } + .to raise_error(APIError, /pages\.next' carries no cursor/) + end + + it 'refuses a next-page url it cannot parse' do + stub_request(:get, "#{base}/conversations").with(query: hash_including({})) + .to_return(json(list_body([], next_page: 'http://[bad'))) + + expect { client.list_page('conversations', per_page: 50) } + .to raise_error(APIError, /pages\.next' carries no cursor/) + end + + it 'refuses a next page it can read neither way, rather than truncating silently' do + stub_request(:get, "#{base}/conversations").with(query: hash_including({})) + .to_return(json(list_body([], next_page: 42))) + + expect { client.list_page('conversations', per_page: 50) } + .to raise_error(APIError, /unexpected response shape.*pages\.next/m) + end + + # `Array()` on the envelope would hand the collection rows built out of + # [key, value] pairs: a page that looks answered and holds nothing. + it 'refuses a response whose data is not a list' do + stub_request(:get, "#{base}/conversations").with(query: hash_including({})) + .to_return(json({ 'type' => 'list', 'data' => { 'id' => '1' } })) + + expect { client.list_page('conversations', per_page: 50) } + .to raise_error(APIError, /unexpected response shape.*'data' is not a list/m) + end + + it 'refuses a response carrying no data at all' do + stub_request(:get, "#{base}/conversations").with(query: hash_including({})) + .to_return(json({ 'type' => 'list', 'total_count' => 0 })) + + expect { client.list_page('conversations', per_page: 50) }.to raise_error(APIError, /'data' is not a list/) + end + + it 'serves an empty page as an empty page, zero being an answer' do + stub_request(:get, "#{base}/conversations").with(query: hash_including({})) + .to_return(json(list_body([], total: 0))) + + expect(client.list_page('conversations', per_page: 50)) + .to have_attributes(records: [], next_cursor: nil, total_count: 0) + end + + # nil rather than 0: zero is an answer, and this is the absence of one. + it 'reports no count when Intercom sends none' do + stub_request(:get, "#{base}/conversations").with(query: hash_including({})) + .to_return(json({ 'type' => 'list', 'data' => [] })) + + expect(client.list_page('conversations', per_page: 50).total_count).to be_nil + end + + it 'reads a page through the boot connection when asked to' do + stub_request(:get, "#{base}/ticket_types").with(query: hash_including({})) + .to_return(json(list_body([{ 'id' => '1' }]))) + + expect(client.list_page('ticket_types', per_page: 50, boot: true).records.size).to eq(1) + end + + it 'names the endpoint when the read fails' do + stub_request(:get, "#{base}/conversations").with(query: hash_including({})) + .to_return(json({ 'errors' => [{ 'code' => 'not_found' }] }, 404)) + + expect { client.list_page('conversations', per_page: 50) } + .to raise_error(APIError, /conversations: HTTP 404 not_found/) + end + end + describe '.bounded_per_page' do # Intercom answers `invalid_per_page` past 150 instead of clamping, so a # page size is bounded before it is sent or the list view breaks. diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/pagination/cursor_walker_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/pagination/cursor_walker_spec.rb new file mode 100644 index 000000000..70529cc32 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/pagination/cursor_walker_spec.rb @@ -0,0 +1,173 @@ +module ForestAdminDatasourceIntercom + module Pagination + RSpec.describe CursorWalker do + subject(:walker) { described_class.new } + + let(:asked) { [] } + + def record(id) + { 'id' => id } + end + + def page(records, next_cursor: nil) + Client::Page.new(records: records, next_cursor: next_cursor, total_count: nil) + end + + # A page source Intercom's own shape: each page advertises the cursor of + # the next one, and the last advertises nothing. + def source(*pages) + queue = pages.dup + + lambda do |per_page, cursor| + asked << [per_page, cursor] + queue.shift || page([]) + end + end + + def ids(records) + records.map { |r| r['id'] } + end + + it 'serves a window one page already covers' do + records = walker.walk(offset: 0, limit: 2, &source(page([record('a'), record('b')], next_cursor: 'c1'))) + + expect(ids(records)).to eq(%w[a b]) + end + + it 'asks only for the records the window still needs' do + walker.walk(offset: 0, limit: 3, &source(page([record('a'), record('b'), record('c')]))) + + expect(asked).to eq([[3, nil]]) + end + + it 'walks pages until the window is covered, then slices the offset out' do + pages = source(page([record('a'), record('b')], next_cursor: 'c1'), + page([record('c'), record('d')], next_cursor: 'c2')) + + records = walker.walk(offset: 2, limit: 2, &pages) + + expect(ids(records)).to eq(%w[c d]) + expect(asked).to eq([[4, nil], [2, 'c1']]) + end + + it 'follows the cursor each page advertises' do + walker.walk(offset: 0, limit: 4, &source(page([record('a')], next_cursor: 'c1'), + page([record('b')], next_cursor: 'c2'), + page([record('c')]))) + + expect(asked.map(&:last)).to eq([nil, 'c1', 'c2']) + end + + it 'stops where Intercom stops advertising a next page' do + records = walker.walk(offset: 0, limit: 10, &source(page([record('a')]))) + + expect(ids(records)).to eq(%w[a]) + expect(asked.size).to eq(1) + end + + it 'stops on an empty page' do + records = walker.walk(offset: 0, limit: 10, &source(page([], next_cursor: 'c1'))) + + expect(records).to be_empty + end + + # None of this happens against Intercom today, but a walk driven by a + # remote value stops on its own terms rather than on the caps only. + it 'stops on a cursor it has already followed' do + pages = source(page([record('a')], next_cursor: 'loop'), + page([record('b')], next_cursor: 'loop'), + page([record('c')], next_cursor: 'loop')) + + walker.walk(offset: 0, limit: 10, &pages) + + expect(asked.size).to eq(2) + end + + # Intercom documents duplicates on a dataset that moves between two + # paginated requests, and conversations move constantly. Two rows carrying + # one id is what a list view renders as two identical lines. + it 'drops a record a previous page already served' do + pages = source(page([record('a'), record('b')], next_cursor: 'c1'), + page([record('b'), record('c')])) + + records = walker.walk(offset: 0, limit: 10, &pages) + + expect(ids(records)).to eq(%w[a b c]) + end + + it 'keeps records carrying no id rather than deciding they are not records' do + anonymous = source(page([{ 'email' => 'a@b.test' }, { 'email' => 'c@d.test' }])) + records = walker.walk(offset: 0, limit: 10, &anonymous) + + expect(records.size).to eq(2) + end + + it 'returns nothing, and asks nothing, for a limit of zero' do + records = walker.walk(offset: 0, limit: 0, &source(page([record('a')]))) + + expect(records).to be_empty + expect(asked).to be_empty + end + + it 'reads an offset past the end as an empty window rather than an error' do + records = walker.walk(offset: 50, limit: 10, &source(page([record('a')]))) + + expect(records).to be_empty + end + + it 'treats a negative offset as the beginning' do + records = walker.walk(offset: -5, limit: 1, &source(page([record('a')]))) + + expect(ids(records)).to eq(%w[a]) + end + + describe 'a limit of nil, which asks for everything past the offset' do + it 'walks to the end' do + pages = source(page([record('a')], next_cursor: 'c1'), page([record('b')])) + + expect(ids(walker.walk(offset: 0, limit: nil, &pages))).to eq(%w[a b]) + end + + it 'asks for the largest page Intercom accepts' do + walker.walk(offset: 0, limit: nil, &source(page([record('a')]))) + + expect(asked.first.first).to eq(Client::MAX_PER_PAGE) + end + end + + describe 'caps' do + before { allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) } + + it 'stops after the page it is allowed, and says the result is truncated' do + capped = described_class.new(max_pages: 2) + pages = source(page([record('a')], next_cursor: 'c1'), + page([record('b')], next_cursor: 'c2'), + page([record('c')], next_cursor: 'c3')) + + capped.walk(offset: 0, limit: nil, &pages) + + expect(asked.size).to eq(2) + expect(ForestAdminDatasourceIntercom.logger).to have_received(:warn).with(/truncated/) + end + + it 'stops on the record budget, and never asks for more than it has left' do + capped = described_class.new(max_records: 3) + pages = source(page([record('a'), record('b')], next_cursor: 'c1'), + page([record('c'), record('d')], next_cursor: 'c2')) + + capped.walk(offset: 0, limit: nil, &pages) + + expect(asked).to eq([[3, nil], [1, 'c1']]) + end + + # A walk that covered the window it was given hands back exactly that, + # and has nothing to report -- unlike one a cap cut short. + it 'stays quiet when the window was covered' do + walker.walk(offset: 0, limit: 1, &source(page([record('a')], next_cursor: 'c1'))) + + expect(ForestAdminDatasourceIntercom.logger).not_to have_received(:warn) + end + end + end + end +end From 59ab8cc1295dd5def1652059ea47356c44e9fd3b Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Mon, 31 Aug 2026 18:10:29 +0200 Subject: [PATCH 4/9] feat(intercom): publish the reference collections Fourth step of lot 1 (PRD-1112). Admins, teams, ticket types and ticket states: the collections that turn an assignee id into a teammate and a state id into a label, which is what the conversation and ticket rows of the next steps need to be readable at all. Their endpoints answer whole -- no pagination parameter, no filter, no sort -- which paradoxically makes this the most capable tier of the datasource. Filtering, sorting, paging and counting them in memory is exact rather than approximate, because the records in hand are every record Intercom holds: a window cut out of them carries the rows a server-side query would have returned. It is the one place where an in-memory pass does not risk the thing refused everywhere else, a result that looks filtered without being filtered, which only arises when what one holds is a page of something larger. So these are the only countable and groupable collections of the lot, and the cost is bandwidth. A condition the tier cannot evaluate is refused rather than applied: `match` answers nil for an operator with no in-memory equivalence and `apply` reads that as "no match", so it would otherwise hand back an empty page an operator cannot tell from a real answer. The schema advertises no such operator -- every advertised one is re-checked against the toolkit rather than trusted -- but a scope, a segment or a customizer can still send one. Two Intercom particulars, both measured rather than assumed: /admins and /teams put their records under their own key where /ticket_types uses the `data` envelope, so the collection names its key and `data` is the fallback; and a team id is a string on the team and a number inside `admin_ids`, so ids are stringified or a filter value from Forest would never match. `fetch_all` also follows a cursor if one is advertised, since no pagination parameter in the specification is not a promise that a large workspace answers in one response. Co-Authored-By: Claude Opus 5 (1M context) --- .../client.rb | 64 +++++ .../collections/admin.rb | 52 ++++ .../collections/base_collection.rb | 71 +++++ .../collections/fetch_all_collection.rb | 191 ++++++++++++++ .../collections/team.rb | 36 +++ .../collections/ticket_state.rb | 40 +++ .../collections/ticket_type.rb | 47 ++++ .../datasource.rb | 15 +- .../client_spec.rb | 79 ++++++ .../collections/admin_spec.rb | 61 +++++ .../collections/fetch_all_collection_spec.rb | 246 ++++++++++++++++++ .../collections/team_spec.rb | 41 +++ .../collections/ticket_state_spec.rb | 38 +++ .../collections/ticket_type_spec.rb | 45 ++++ .../datasource_spec.rb | 15 +- 15 files changed, 1034 insertions(+), 7 deletions(-) create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/admin.rb create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/base_collection.rb create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/fetch_all_collection.rb create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/team.rb create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket_state.rb create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket_type.rb create mode 100644 packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/admin_spec.rb create mode 100644 packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/fetch_all_collection_spec.rb create mode 100644 packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_spec.rb create mode 100644 packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_state_spec.rb create mode 100644 packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_type_spec.rb diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/client.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/client.rb index f03faf2b8..9bef4e18f 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/client.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/client.rb @@ -14,6 +14,12 @@ class Client # rubocop:disable Metrics/ClassLength # before it is sent or the list view breaks rather than shrinks. MAX_PER_PAGE = 150 + # Bounds `fetch_all`, which asks for a whole reference collection rather than + # a window: those endpoints answer in one response, so reaching this many + # pages means Intercom started paginating on its own and the read is spending + # more than the answer is worth. + MAX_COLLECTED_PAGES = 10 + # `next_cursor` is nil as soon as Intercom stops advertising a next page, so # callers never have to know how the absence is spelled on the wire. # `total_count` is exact, filter included, which is what makes Forest's @@ -51,6 +57,23 @@ def list_page(path, per_page:, starting_after: nil, params: {}, boot: false) must_succeed(path) { to_page(get(path, query, boot: boot).body, path) } end + # Every record of an endpoint that answers in one response: the reference + # collections -- admins, teams, ticket types, ticket states -- whose paths + # declare no pagination parameter at all. + # + # `list_key` is the key the endpoint puts its records under. Intercom is not + # consistent about it: `/admins` answers `{"type": "admin.list", "admins": + # [...]}` where `/ticket_types` answers the `data` envelope every paginated + # listing uses, so the collection names its own and `data` is the fallback. + # + # A cursor is followed if one is advertised, defensively: no pagination + # parameter in the specification is not a promise that a large workspace + # answers in one response, and a truncated reference collection would show + # an operator a state list missing its last states. + def fetch_all(path, list_key: 'data', boot: false) + must_succeed(path) { collect_pages(path, list_key: list_key, boot: boot) } + end + # The page size Intercom accepts, whatever was asked for. def self.bounded_per_page(size) value = size.to_i @@ -89,6 +112,47 @@ def verify_pinned_version(response) ) end + def collect_pages(path, list_key:, boot:) + records = [] + cursor = nil + pages = 0 + + loop do + body = get(path, cursor.nil? ? nil : { 'starting_after' => cursor }, boot: boot).body + records.concat(extract_entities(body, path, list_key)) + pages += 1 + cursor = next_cursor(body, path) + break if cursor.nil? + + if pages >= MAX_COLLECTED_PAGES + log_collection_cap(path, pages, records.size) + break + end + end + + records + end + + # The records under the key the endpoint uses, or under `data`. Anything + # else is refused: a reference collection silently read as empty is a + # ticket-state column with no values and an assignee shown as a raw id. + def extract_entities(body, operation, list_key) + return [] if body.nil? || body == '' + + entities = body.is_a?(Hash) ? (body[list_key] || body['data']) : nil + return entities if entities.is_a?(Array) + + refuse_body_shape(operation, "neither '#{list_key}' nor 'data' is a list") + end + + def log_collection_cap(path, pages, collected) + ForestAdminDatasourceIntercom.logger.warn( + "[forest_admin_datasource_intercom] Stopped reading #{path} after #{pages} page(s) / " \ + "#{collected} record(s); the rest is left out. This endpoint is read whole on purpose, so a workspace " \ + 'this large needs the collection bounded rather than listed.' + ) + end + # Intercom wraps a listing in `{ "type": "list", "data": [...], # "total_count": N, "pages": { "next": { "starting_after": "..." } } }`. def to_page(body, operation) diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/admin.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/admin.rb new file mode 100644 index 000000000..45704ea82 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/admin.rb @@ -0,0 +1,52 @@ +module ForestAdminDatasourceIntercom + module Collections + # The teammates of the workspace: who a conversation or a ticket is assigned + # to. Without this collection an assignee is a raw id on every row. + class Admin < FetchAllCollection + def initialize(datasource) + super(datasource, 'IntercomAdmin') + end + + protected + + # `/admins` puts its records under `admins`, not under the `data` envelope + # the paginated listings use. + def fetch_all + client.fetch_all('admins', list_key: 'admins') + end + + def serialize(admin) + attrs = admin.is_a?(Hash) ? admin : {} + + { 'id' => stringify_id(attrs['id']), + 'name' => attrs['name'], + 'email' => attrs['email'], + 'job_title' => attrs['job_title'], + 'away_mode_enabled' => attrs['away_mode_enabled'], + 'away_mode_reassign' => attrs['away_mode_reassign'], + 'has_inbox_seat' => attrs['has_inbox_seat'], + 'team_ids' => Array(attrs['team_ids']).map { |id| stringify_id(id) } } + end + + private + + def define_schema + add_column('id', 'String', is_primary_key: true) + add_column('name', 'String') + add_column('email', 'String') + add_column('job_title', 'String') + # Whether the teammate is away, and whether their conversations get + # reassigned while they are: the two an ops lead looks at before + # assigning anything. + add_column('away_mode_enabled', 'Boolean') + add_column('away_mode_reassign', 'Boolean') + add_column('has_inbox_seat', 'Boolean') + # A list, so neither filterable nor sortable. It stays a plain column + # rather than a relation: Intercom carries the membership on the admin + # and on the team both, so declaring it twice would give the schema two + # sides of a many-to-many with no join collection to hold it. + add_column('team_ids', 'Json') + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/base_collection.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/base_collection.rb new file mode 100644 index 000000000..3cdf5f549 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/base_collection.rb @@ -0,0 +1,71 @@ +module ForestAdminDatasourceIntercom + module Collections + # What every Intercom collection shares: how a schema is declared, how a + # record is narrowed to the projection asked for, and how a window is cut + # out of records already in hand. + # + # Read-only for now. The writes and the business actions arrive with lot 3, + # and the relations with lot 4, once Contacts and Companies exist -- a + # relation whose target collection is missing is a schema the agent refuses + # to boot on. + class BaseCollection < ForestAdminDatasourceToolkit::Collection + ColumnSchema = ForestAdminDatasourceToolkit::Schema::ColumnSchema + Operators = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators + Equivalent = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::ConditionTreeEquivalent + + def initialize(datasource, name) + super + define_schema + end + + def client + datasource.client + end + + protected + + def define_schema = raise(NotImplementedError, "#{self.class} did not implement define_schema") + + # A record narrowed to what was asked for. A projection naming a field the + # record does not carry yields nil rather than nothing at all: the agent + # asked for a column, and an absent key would read as a record missing it. + def project(record, projection) + fields = Array(projection) + return record if fields.empty? + + fields.to_h { |field| [field, record[field]] } + end + + # The window a list view asked for, cut out of records already in hand. + # + # A filter with no page -- or a page naming no limit -- asks for every + # record it matched, and there is nothing to cut. How far the read that + # collected them went is a different question, answered by the walker and + # its caps. + def page_window(records, filter) + page = filter&.page + return records if page.nil? + + offset = page.offset.to_i.clamp(0, nil) + limit = page.limit.to_i + return records.drop(offset) unless limit.positive? + + records[offset, limit] || [] + end + + # The timezone in-memory date comparisons are evaluated in. The caller's, + # since that is whose "today" the filter was written against. + def timezone_for(caller) + caller.respond_to?(:timezone) ? caller.timezone : nil + end + + # Ids reach this datasource as strings -- a filter value from Forest, a + # segment, a url -- while Intercom types them inconsistently: a team id is + # a string, the same team's id inside `admin_ids` is a number. Left as it + # comes, an integer id would never match the string the filter carries. + def stringify_id(value) + value&.to_s + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/fetch_all_collection.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/fetch_all_collection.rb new file mode 100644 index 000000000..66d33feca --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/fetch_all_collection.rb @@ -0,0 +1,191 @@ +module ForestAdminDatasourceIntercom + module Collections + # Base for the reference collections Intercom hands back whole in a single + # response: admins, teams, ticket types, ticket states. Their endpoints + # declare no pagination parameter, no filter and no sort. + # + # Paradoxically this is the most capable tier of the datasource. Filtering, + # sorting, paginating and counting that response in memory is *exact* rather + # than approximate, because the records in hand are every record Intercom + # holds: a window cut out of them carries the rows a server-side query would + # have returned. It is the one place where an in-memory pass does not risk + # the thing this datasource refuses everywhere else -- a result that looks + # filtered without being filtered -- which only arises when what one holds + # is a single page of something larger. The cost is bandwidth, not + # correctness. + # + # Each read re-reads the endpoint, so an operator sees what Intercom holds + # now rather than what it held when the process booted. One request per list + # against a 10 000-a-minute budget is not a figure any list view approaches. + class FetchAllCollection < BaseCollection + # The filters a column may advertise, per column type. Restricted to what + # the toolkit can evaluate in memory, since the in-memory pass is the only + # pass there is here: an operator with no equivalence makes `match` answer + # nil, which `apply` reads as "no match" and would empty the page instead + # of filtering it. + OPERATOR_CANDIDATES = { + 'String' => [Operators::EQUAL, Operators::NOT_EQUAL, Operators::IN, Operators::NOT_IN, + Operators::PRESENT, Operators::BLANK, Operators::CONTAINS, Operators::I_CONTAINS, + Operators::NOT_CONTAINS, Operators::STARTS_WITH, Operators::ENDS_WITH], + 'Boolean' => [Operators::EQUAL, Operators::NOT_EQUAL, Operators::IN, Operators::NOT_IN, + Operators::PRESENT, Operators::BLANK] + }.freeze + + # The operators `ConditionTreeLeaf#match` evaluates natively; anything else + # needs an equivalence for the column's type to be evaluable at all. + IN_MEMORY_OPERATORS = [Operators::IN, Operators::EQUAL, Operators::LESS_THAN, Operators::GREATER_THAN, + Operators::MATCH, Operators::STARTS_WITH, Operators::ENDS_WITH, + Operators::LONGER_THAN, Operators::SHORTER_THAN, Operators::INCLUDES_ALL, + Operators::NOT_IN, Operators::NOT_EQUAL, Operators::NOT_CONTAINS].freeze + + # Candidates are re-checked against the toolkit rather than trusted, so an + # equivalence it stops providing takes the filter out of the schema instead + # of turning every page using it into an empty one. + def self.operators_for(column_type) + Array(OPERATOR_CANDIDATES[column_type]).select do |operator| + Equivalent.equivalent_tree?(operator, IN_MEMORY_OPERATORS, column_type) + end + end + + # Countable, unlike the cursor-paginated collections: the count answered + # here is taken over every record Intercom holds rather than over the pages + # a walk happened to collect. + def initialize(datasource, name) + super + enable_count + end + + def list(caller, filter, projection) + records = sort_in_memory(filtered_records(caller, filter), filter&.sort) + + page_window(records, filter).map { |record| project(record, projection) } + end + + # Exact, like the filter and the sort above it, which is why these columns + # stay groupable. + # + # Rows come back keyed with strings because that is how the agent reads + # them, while `Aggregation#apply` hands them back keyed with symbols. + def aggregate(caller, filter, aggregation, limit = nil) + aggregation.apply(filtered_records(caller, filter), timezone_for(caller), limit) + .map { |row| { 'group' => row[:group], 'value' => row[:value] } } + end + + protected + + # Scalar columns are sortable and groupable, the in-memory pass honouring + # anything asked of them. A Json column is neither, nor filterable: it + # holds a list, and what a filter on it would mean has no in-memory + # counterpart. + def add_column(name, type, is_primary_key: false) + add_field(name, ColumnSchema.new(column_type: type, + filter_operators: self.class.operators_for(type), + is_primary_key: is_primary_key, + is_sortable: type != 'Json', + is_groupable: type != 'Json')) + end + + # Every record of the collection, straight from its endpoint. + def fetch_all = raise(NotImplementedError, "#{self.class} did not implement fetch_all") + + # One Intercom entity flattened into a record matching the schema. + def serialize(_entity) = raise(NotImplementedError, "#{self.class} did not implement serialize") + + private + + # The complete dataset, serialized and narrowed to the rows the filter + # keeps: what `list` pages and what `aggregate` counts are the same rows. + def filtered_records(caller, filter) + records = fetch_all.map { |entity| serialize(entity) } + tree = filter&.condition_tree + return records if tree.nil? + + refuse_unevaluable!(tree) + tree.apply(records, self, timezone_for(caller)) + end + + # A condition this collection cannot evaluate is refused rather than + # applied. `match` answers nil for an operator with no in-memory + # equivalence and `apply` reads that as "no match", so an unevaluable + # condition would hand back an empty page that looks like a filter + # matching nothing -- indistinguishable, to the operator, from a real + # answer. The schema advertises no such operator; a scope, a segment or a + # customizer can still send one. + def refuse_unevaluable!(tree) + offender = nil + tree.some_leaf do |leaf| + offender = leaf unless evaluable?(leaf) + !offender.nil? + end + return if offender.nil? + + raise UnsupportedOperatorError, + "#{name} cannot filter '#{offender.field}' with '#{offender.operator}': it is read whole from " \ + 'Intercom and filtered in memory, which supports only the operators its columns advertise. ' \ + 'Change the condition, or the scope or segment carrying it.' + end + + def evaluable?(leaf) + schema = fields[leaf.field] + schema.is_a?(ColumnSchema) && schema.filter_operators.include?(leaf.operator) + end + + # Neither Ruby's `sort` nor the toolkit's `Sort#apply` can be used as is: + # `sort` is not stable, and `<=>` answers nil on a null, on two booleans + # and on mixed types, which leaves the comparator undefined and the order + # arbitrary. Ties therefore fall back to the position Intercom returned the + # record in. + # + # Every requested order is honoured, the ascending primary-key sort the + # agent injects when a request names none included, so there is no + # unsortable order to report here -- unlike the cursor collections, where + # Intercom ignores a sort without saying so. + def sort_in_memory(records, sort) + clauses = sort_clauses(sort) + return records if clauses.empty? + + records.each_with_index.sort do |(left, left_index), (right, right_index)| + compare_clauses(left, right, clauses).nonzero? || (left_index <=> right_index) + end.map(&:first) + end + + # A sort clause naming a field this collection does not carry is dropped: + # ordering by a column that is not there would compare nil to nil on every + # row and leave the order to the tie-break. + def sort_clauses(sort) + Array(sort).filter_map do |clause| + field = clause[:field] || clause['field'] + next unless fields.key?(field) + + # `key?` rather than `||`: a descending clause carries `false`, which an + # `||` fallback would read as "absent" and turn back into ascending. + ascending = clause.key?(:ascending) ? clause[:ascending] : clause['ascending'] + [field, ascending != false] + end + end + + def compare_clauses(left, right, clauses) + clauses.each do |field, ascending| + comparison = compare_values(left[field], right[field]) + next if comparison.zero? + + return ascending ? comparison : -comparison + end + + 0 + end + + # Nulls sort last ascending and first descending, the way a database orders + # them; values `<=>` cannot compare -- two booleans, for one -- are + # compared through their string form rather than left undefined, which puts + # `false` before `true`, again like a database. + def compare_values(left, right) + return 0 if left.nil? && right.nil? + return 1 if left.nil? + return -1 if right.nil? + + (left <=> right) || (left.to_s <=> right.to_s) + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/team.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/team.rb new file mode 100644 index 000000000..2286bfa46 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/team.rb @@ -0,0 +1,36 @@ +module ForestAdminDatasourceIntercom + module Collections + # The inbox teams a conversation can be assigned to, rather than a single + # teammate. + class Team < FetchAllCollection + def initialize(datasource) + super(datasource, 'IntercomTeam') + end + + protected + + # Like `/admins`, `/teams` uses its own key instead of the `data` envelope. + def fetch_all + client.fetch_all('teams', list_key: 'teams') + end + + def serialize(team) + attrs = team.is_a?(Hash) ? team : {} + + { 'id' => stringify_id(attrs['id']), + 'name' => attrs['name'], + 'admin_ids' => Array(attrs['admin_ids']).map { |id| stringify_id(id) } } + end + + private + + def define_schema + add_column('id', 'String', is_primary_key: true) + add_column('name', 'String') + # Intercom types these as numbers here and as strings on the admin + # itself; they are stringified so both sides carry the same id. + add_column('admin_ids', 'Json') + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket_state.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket_state.rb new file mode 100644 index 000000000..f9845d7e7 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket_state.rb @@ -0,0 +1,40 @@ +module ForestAdminDatasourceIntercom + module Collections + # The ticket states of the workspace. A ticket carries its state as an id, so + # without this collection a support queue reads as a column of numbers. + class TicketState < FetchAllCollection + def initialize(datasource) + super(datasource, 'IntercomTicketState') + end + + protected + + def fetch_all + client.fetch_all('ticket_states') + end + + # Two labels rather than one: `internal_label` is what the support team + # sees, `external_label` what the customer is shown. An operator reading a + # queue needs the first, and needs to know what the second says. + def serialize(ticket_state) + attrs = ticket_state.is_a?(Hash) ? ticket_state : {} + + { 'id' => stringify_id(attrs['id']), + 'category' => attrs['category'], + 'internal_label' => attrs['internal_label'], + 'external_label' => attrs['external_label'], + 'archived' => attrs['archived'] } + end + + private + + def define_schema + add_column('id', 'String', is_primary_key: true) + add_column('category', 'String') + add_column('internal_label', 'String') + add_column('external_label', 'String') + add_column('archived', 'Boolean') + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket_type.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket_type.rb new file mode 100644 index 000000000..7b6367198 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket_type.rb @@ -0,0 +1,47 @@ +module ForestAdminDatasourceIntercom + module Collections + # The ticket types the workspace defines. They are what makes a ticket's type + # readable, and they are also where the ticket attributes are declared -- + # which is what the ticket collection reads them for. + class TicketType < FetchAllCollection + def initialize(datasource) + super(datasource, 'IntercomTicketType') + end + + protected + + def fetch_all + client.fetch_all('ticket_types') + end + + # `ticket_type_attributes` is deliberately left out: it is a nested list of + # attribute definitions, useful to the ticket collection and meaningless as + # a column. An attribute of the same name carries a different id from one + # type to the next (measured), which is exactly why the ticket collection + # has to read the definitions rather than assume them. + def serialize(ticket_type) + attrs = ticket_type.is_a?(Hash) ? ticket_type : {} + + { 'id' => stringify_id(attrs['id']), + 'name' => attrs['name'], + 'description' => attrs['description'], + 'category' => attrs['category'], + 'icon' => attrs['icon'], + 'archived' => attrs['archived'] } + end + + private + + def define_schema + add_column('id', 'String', is_primary_key: true) + add_column('name', 'String') + add_column('description', 'String') + # `request` / `task` / `tracker` on the wire, not the labels the Intercom + # interface shows -- the same mismatch the ticket filter has to respect. + add_column('category', 'String') + add_column('icon', 'String') + add_column('archived', 'Boolean') + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb index 6b8b07315..c9e082676 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb @@ -1,7 +1,4 @@ module ForestAdminDatasourceIntercom - # Boot skeleton: it configures a client and registers no collection yet. The - # collections follow in their own pull requests, each one bringing the - # endpoints it reads. class Datasource < ForestAdminDatasourceToolkit::Datasource attr_reader :client, :configuration @@ -25,6 +22,16 @@ def inspect private - def register_collections; end + # The reference collections first: they are what turns an assignee id into a + # teammate and a state id into a label, and nothing else in the schema points + # at them yet. Conversations and Tickets follow, and no request is made here + # -- each collection reads its endpoint when it is listed, so a datasource + # boots whatever Intercom is doing. + def register_collections + add_collection(Collections::Admin.new(self)) + add_collection(Collections::Team.new(self)) + add_collection(Collections::TicketType.new(self)) + add_collection(Collections::TicketState.new(self)) + end end end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/client_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/client_spec.rb index fede1d9dd..39e924ad7 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/client_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/client_spec.rb @@ -303,6 +303,85 @@ def list_body(data, next_page: nil, total: 2) end end + describe '#fetch_all' do + it 'reads the records under the key the endpoint uses' do + stub_request(:get, "#{base}/admins") + .to_return(json('type' => 'admin.list', 'admins' => [{ 'id' => '1' }, { 'id' => '2' }])) + + expect(client.fetch_all('admins', list_key: 'admins').size).to eq(2) + end + + # Intercom is not consistent about it: /admins and /teams use their own + # key, /ticket_types the `data` envelope every paginated listing uses. + it 'falls back to the data envelope' do + stub_request(:get, "#{base}/ticket_types").to_return(json('type' => 'list', 'data' => [{ 'id' => '1' }])) + + expect(client.fetch_all('ticket_types')).to eq([{ 'id' => '1' }]) + end + + it 'asks for no page: these endpoints answer whole' do + stub_request(:get, "#{base}/teams").to_return(json('teams' => [])) + + client.fetch_all('teams', list_key: 'teams') + + expect(WebMock).to have_requested(:get, "#{base}/teams").with(query: {}) + end + + # A reference collection read as empty is a state column with no values and + # an assignee shown as a raw id -- worse than a failure naming the shape. + it 'refuses a response holding neither key' do + stub_request(:get, "#{base}/admins").to_return(json('type' => 'admin.list', 'admins' => { 'id' => '1' })) + + expect { client.fetch_all('admins', list_key: 'admins') } + .to raise_error(APIError, /neither 'admins' nor 'data' is a list/) + end + + it 'reads an empty body as no record' do + stub_request(:get, "#{base}/admins").to_return(status: 200, body: '') + + expect(client.fetch_all('admins', list_key: 'admins')).to eq([]) + end + + # No pagination parameter in the specification is not a promise that a + # large workspace answers in one response, and a truncated reference + # collection would show an operator a state list missing its last states. + it 'follows a cursor if one is advertised anyway' do + stub_request(:get, "#{base}/tags").with(query: {}) + .to_return(json('data' => [{ 'id' => '1' }], + 'pages' => { 'next' => { 'starting_after' => 'c2' } })) + stub_request(:get, "#{base}/tags").with(query: { 'starting_after' => 'c2' }) + .to_return(json('data' => [{ 'id' => '2' }])) + + expect(client.fetch_all('tags').map { |tag| tag['id'] }).to eq(%w[1 2]) + end + + it 'stops at its page cap and says what it left out' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + stub_request(:get, "#{base}/tags").with(query: hash_including({})) + .to_return(json('data' => [{ 'id' => '1' }], + 'pages' => { 'next' => { 'starting_after' => 'c' } })) + + client.fetch_all('tags') + + expect(WebMock).to have_requested(:get, "#{base}/tags") + .with(query: hash_including({})).times(described_class::MAX_COLLECTED_PAGES) + expect(ForestAdminDatasourceIntercom.logger).to have_received(:warn).with(/Stopped reading tags/) + end + + it 'reads through the boot connection when asked to' do + stub_request(:get, "#{base}/ticket_types").to_return(json('data' => [])) + + expect(client.fetch_all('ticket_types', boot: true)).to eq([]) + end + + it 'names the endpoint when the read fails' do + stub_request(:get, "#{base}/admins").to_return(json({ 'errors' => [{ 'code' => 'forbidden' }] }, 403)) + + expect { client.fetch_all('admins', list_key: 'admins') } + .to raise_error(APIError, /admins: HTTP 403 forbidden/) + end + end + describe '.bounded_per_page' do # Intercom answers `invalid_per_page` past 150 instead of clamping, so a # page size is bounded before it is sent or the list view breaks. diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/admin_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/admin_spec.rb new file mode 100644 index 000000000..a98cfd112 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/admin_spec.rb @@ -0,0 +1,61 @@ +module ForestAdminDatasourceIntercom + RSpec.describe Collections::Admin do + subject(:collection) { described_class.new(datasource) } + + let(:datasource) { Datasource.new(access_token: 's3cr3t', rate_limiter: nil) } + let(:base) { datasource.configuration.url } + + def filter + ForestAdminDatasourceToolkit::Components::Query::Filter.new + end + + def stub_admins(*admins) + stub_request(:get, "#{base}/admins") + .to_return(status: 200, body: { 'type' => 'admin.list', 'admins' => admins }.to_json, + headers: { 'Content-Type' => 'application/json' }) + end + + it 'is named IntercomAdmin' do + expect(collection.name).to eq('IntercomAdmin') + end + + it 'exposes the columns an ops lead reads before assigning anything' do + expect(collection.fields.keys) + .to eq(%w[id name email job_title away_mode_enabled away_mode_reassign has_inbox_seat team_ids]) + end + + it 'declares id as the primary key' do + expect(collection.fields['id']).to have_attributes(is_primary_key: true, column_type: 'String') + end + + # `/admins` puts its records under `admins` rather than under the `data` + # envelope the paginated listings use. + it 'reads the endpoint and flattens the teammate' do + stub_admins('type' => 'admin', 'id' => '1', 'name' => 'Alice', 'email' => 'alice@acme.test', + 'job_title' => 'Support', 'away_mode_enabled' => true, 'away_mode_reassign' => false, + 'has_inbox_seat' => true, 'team_ids' => [814_865]) + + expect(collection.list(nil, filter, nil)) + .to eq([{ 'id' => '1', 'name' => 'Alice', 'email' => 'alice@acme.test', 'job_title' => 'Support', + 'away_mode_enabled' => true, 'away_mode_reassign' => false, 'has_inbox_seat' => true, + 'team_ids' => %w[814865] }]) + end + + # Intercom types a team id as a number here and as a string on the team + # itself; a filter value from Forest always arrives as a string. + it 'stringifies the ids so both sides of the membership match' do + stub_admins('id' => 493_881, 'team_ids' => [814_865, 814_866]) + + row = collection.list(nil, filter, nil).first + + expect(row['id']).to eq('493881') + expect(row['team_ids']).to eq(%w[814865 814866]) + end + + it 'reads a teammate with no team as one with no team, not as one with a null' do + stub_admins('id' => '1', 'team_ids' => nil) + + expect(collection.list(nil, filter, nil).first['team_ids']).to eq([]) + end + end +end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/fetch_all_collection_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/fetch_all_collection_spec.rb new file mode 100644 index 000000000..e6e56f7ee --- /dev/null +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/fetch_all_collection_spec.rb @@ -0,0 +1,246 @@ +module ForestAdminDatasourceIntercom + # The in-memory tier is exercised through Admin, a real collection carrying one + # column of each kind it has to handle: strings, booleans and a list. + RSpec.describe Collections::FetchAllCollection do + subject(:collection) { Collections::Admin.new(datasource) } + + let(:datasource) { Datasource.new(access_token: 's3cr3t', rate_limiter: nil) } + let(:base) { datasource.configuration.url } + let(:operators) { ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators } + + def leaf(field, operator, value = nil) + ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf + .new(field, operator, value) + end + + def filter(condition_tree: nil, page: nil, sort: nil) + ForestAdminDatasourceToolkit::Components::Query::Filter.new(condition_tree: condition_tree, page: page, + sort: sort) + end + + def page(offset, limit) + ForestAdminDatasourceToolkit::Components::Query::Page.new(offset: offset, limit: limit) + end + + def sort(*clauses) + ForestAdminDatasourceToolkit::Components::Query::Sort.new(clauses) + end + + def aggregation(operation, field: nil, groups: []) + ForestAdminDatasourceToolkit::Components::Query::Aggregation.new(operation: operation, field: field, + groups: groups) + end + + def admin(id, overrides = {}) + { 'type' => 'admin', 'id' => id, 'name' => "Admin #{id}", 'email' => "#{id}@acme.test", + 'away_mode_enabled' => false, 'has_inbox_seat' => true, 'team_ids' => [] }.merge(overrides) + end + + def stub_admins(*admins) + stub_request(:get, "#{base}/admins") + .to_return(status: 200, body: { 'type' => 'admin.list', 'admins' => admins }.to_json, + headers: { 'Content-Type' => 'application/json' }) + end + + def ids(records) + records.map { |record| record['id'] } + end + + # An operator is evaluable in memory when `ConditionTreeLeaf#match` handles + # it natively or the toolkit can rewrite it into operators that it does. + def evaluable?(operator, column_type) + ForestAdminDatasourceToolkit::Components::Query::ConditionTree::ConditionTreeEquivalent + .equivalent_tree?(operator, described_class::IN_MEMORY_OPERATORS, column_type) + end + + # The count and the group are taken over every record Intercom holds, not + # over a page of them, which is what makes them exact. + it 'is countable' do + expect(collection.is_countable?).to be(true) + end + + describe 'columns' do + it 'declares a scalar column filterable, sortable and groupable' do + expect(collection.fields['name']) + .to have_attributes(is_sortable: true, is_groupable: true, is_read_only: false) + expect(collection.fields['name'].filter_operators).to include(operators::EQUAL) + end + + # A list has no in-memory counterpart for any of the three. + it 'declares a Json column neither filterable nor sortable' do + expect(collection.fields['team_ids']) + .to have_attributes(column_type: 'Json', is_sortable: false, is_groupable: false, filter_operators: []) + end + + # A filter the UI offers and the collection then answers by emptying the + # page is the failure this whole datasource is built to avoid. + it 'advertises only operators it can actually evaluate' do + advertised = collection.fields.flat_map do |_name, column| + column.filter_operators.map { |operator| [operator, column.column_type] } + end + + expect(advertised.reject { |operator, type| evaluable?(operator, type) }).to be_empty + end + end + + describe '#list' do + it 'reads every record of the endpoint and serializes it' do + stub_admins(admin('1'), admin('2')) + + expect(ids(collection.list(nil, filter, nil))).to eq(%w[1 2]) + end + + it 'narrows the record to the projection' do + stub_admins(admin('1')) + + expect(collection.list(nil, filter, %w[id email])).to eq([{ 'id' => '1', 'email' => '1@acme.test' }]) + end + + # Freshness over rate-limit thrift: nothing is kept from the previous list, + # so an operator sees the teammates the workspace has now. + it 'reads the endpoint again on the next list' do + stub_admins(admin('1')) + + 2.times { collection.list(nil, filter, nil) } + + expect(WebMock).to have_requested(:get, "#{base}/admins").twice + end + + it 'propagates a failure rather than answering with no record' do + stub_request(:get, "#{base}/admins").to_return(status: 500, body: '{}', + headers: { 'Content-Type' => 'application/json' }) + + expect { collection.list(nil, filter, nil) }.to raise_error(APIError) + end + end + + describe '#list with a filter' do + before { stub_admins(admin('1', 'name' => 'Alice'), admin('2', 'name' => 'Bob', 'has_inbox_seat' => false)) } + + def filtered(field, operator, value = nil) + ids(collection.list(nil, filter(condition_tree: leaf(field, operator, value)), nil)) + end + + it 'keeps the rows a string condition names' do + expect(filtered('name', operators::EQUAL, 'Alice')).to eq(%w[1]) + end + + it 'keeps the rows a boolean condition names' do + expect(filtered('has_inbox_seat', operators::EQUAL, false)).to eq(%w[2]) + end + + it 'answers an operator it advertises through an equivalence' do + expect(filtered('name', operators::I_CONTAINS, 'ali')).to eq(%w[1]) + end + + it 'answers nothing when nothing matches, rather than everything' do + expect(filtered('name', operators::EQUAL, 'Nobody')).to be_empty + end + + # `match` answers nil for an operator with no equivalence and `apply` reads + # that as "no match", so this would otherwise be an empty page an operator + # cannot tell from a real answer. The schema advertises no such operator; a + # scope or a segment can still send one. + it 'refuses a condition it cannot evaluate instead of emptying the page' do + expect { filtered('team_ids', operators::EQUAL, 'x') } + .to raise_error(UnsupportedOperatorError, /cannot filter 'team_ids' with 'equal'/) + end + + it 'refuses a condition on a column it does not carry' do + expect { filtered('unknown', operators::EQUAL, 'x') } + .to raise_error(UnsupportedOperatorError, /cannot filter 'unknown'/) + end + + it 'names a refusal after the operator, so the message says what to change' do + expect { filtered('name', operators::LESS_THAN, 'x') } + .to raise_error(UnsupportedOperatorError, /'less_than'/) + end + end + + describe '#list with a sort' do + before do + stub_admins(admin('2', 'name' => 'Bob'), admin('1', 'name' => 'Alice'), admin('3', 'name' => nil)) + end + + it 'orders on the column asked for' do + expect(ids(collection.list(nil, filter(sort: sort({ field: 'name', ascending: true })), nil))) + .to eq(%w[1 2 3]) + end + + # Nulls last ascending, first descending, the way a database orders them. + it 'puts a null first on a descending order' do + expect(ids(collection.list(nil, filter(sort: sort({ field: 'name', ascending: false })), nil))) + .to eq(%w[3 2 1]) + end + + it 'keeps the order Intercom returned for rows the sort cannot separate' do + rows = collection.list(nil, filter(sort: sort({ field: 'away_mode_enabled', ascending: true })), nil) + + expect(ids(rows)).to eq(%w[2 1 3]) + end + + it 'drops a clause naming a column it does not carry' do + rows = collection.list(nil, filter(sort: sort({ field: 'unknown', ascending: true })), nil) + + expect(ids(rows)).to eq(%w[2 1 3]) + end + end + + describe '#list with a page' do + before { stub_admins(admin('1'), admin('2'), admin('3')) } + + it 'cuts the window out of the records in hand' do + expect(ids(collection.list(nil, filter(page: page(1, 1)), nil))).to eq(%w[2]) + end + + it 'reads a page with no limit as every record past the offset' do + expect(ids(collection.list(nil, filter(page: page(1, 0)), nil))).to eq(%w[2 3]) + end + + it 'answers an offset past the end with no record' do + expect(collection.list(nil, filter(page: page(50, 10)), nil)).to be_empty + end + end + + describe '#aggregate' do + before { stub_admins(admin('1', 'name' => 'Alice'), admin('2', 'name' => 'Bob', 'has_inbox_seat' => false)) } + + it 'counts every record, exactly' do + expect(collection.aggregate(nil, filter, aggregation('Count'))) + .to eq([{ 'group' => {}, 'value' => 2 }]) + end + + it 'counts the rows a filter keeps' do + filtered = filter(condition_tree: leaf('has_inbox_seat', operators::EQUAL, true)) + + expect(collection.aggregate(nil, filtered, aggregation('Count')).first['value']).to eq(1) + end + + it 'groups on a column, which is exact for the same reason' do + rows = collection.aggregate(nil, filter, aggregation('Count', groups: [{ field: 'has_inbox_seat' }])) + + expect(rows.sum { |row| row['value'] }).to eq(2) + expect(rows.size).to eq(2) + end + end + + describe 'the hooks a collection has to implement' do + let(:incomplete) do + Class.new(described_class) do + def initialize(datasource) + super(datasource, 'Incomplete') + end + + def define_schema + add_column('id', 'String', is_primary_key: true) + end + end + end + + it 'says which one is missing rather than failing obscurely' do + expect { incomplete.new(datasource).list(nil, filter, nil) } + .to raise_error(NotImplementedError, /did not implement fetch_all/) + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_spec.rb new file mode 100644 index 000000000..91f52dce4 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_spec.rb @@ -0,0 +1,41 @@ +module ForestAdminDatasourceIntercom + RSpec.describe Collections::Team do + subject(:collection) { described_class.new(datasource) } + + let(:datasource) { Datasource.new(access_token: 's3cr3t', rate_limiter: nil) } + let(:base) { datasource.configuration.url } + + def filter + ForestAdminDatasourceToolkit::Components::Query::Filter.new + end + + def stub_teams(*teams) + stub_request(:get, "#{base}/teams") + .to_return(status: 200, body: { 'type' => 'team.list', 'teams' => teams }.to_json, + headers: { 'Content-Type' => 'application/json' }) + end + + it 'is named IntercomTeam' do + expect(collection.name).to eq('IntercomTeam') + end + + it 'exposes the team and its membership' do + expect(collection.fields.keys).to eq(%w[id name admin_ids]) + end + + # Intercom carries the membership on the team and on the admin both. Left as + # a plain list it stays readable on either side; declared as a relation it + # would give the schema two halves of a many-to-many with no join collection. + it 'keeps the membership a list rather than a relation' do + expect(collection.fields['admin_ids']) + .to have_attributes(type: 'Column', column_type: 'Json', filter_operators: []) + end + + it 'reads the endpoint under its own key and stringifies the ids' do + stub_teams('type' => 'team', 'id' => '814865', 'name' => 'Support', 'admin_ids' => [493_881]) + + expect(collection.list(nil, filter, nil)) + .to eq([{ 'id' => '814865', 'name' => 'Support', 'admin_ids' => %w[493881] }]) + end + end +end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_state_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_state_spec.rb new file mode 100644 index 000000000..88ab24ded --- /dev/null +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_state_spec.rb @@ -0,0 +1,38 @@ +module ForestAdminDatasourceIntercom + RSpec.describe Collections::TicketState do + subject(:collection) { described_class.new(datasource) } + + let(:datasource) { Datasource.new(access_token: 's3cr3t', rate_limiter: nil) } + let(:base) { datasource.configuration.url } + + def filter + ForestAdminDatasourceToolkit::Components::Query::Filter.new + end + + def stub_ticket_states(*states) + stub_request(:get, "#{base}/ticket_states") + .to_return(status: 200, body: { 'type' => 'list', 'data' => states }.to_json, + headers: { 'Content-Type' => 'application/json' }) + end + + it 'is named IntercomTicketState' do + expect(collection.name).to eq('IntercomTicketState') + end + + # Two labels rather than one: what the support team sees, and what the + # customer is shown. + it 'exposes both labels of a state' do + expect(collection.fields.keys).to eq(%w[id category internal_label external_label archived]) + end + + it 'reads the endpoint and serializes a state' do + stub_ticket_states('type' => 'ticket_state', 'id' => '3', 'category' => 'submitted', + 'internal_label' => 'Waiting on triage', 'external_label' => 'We are on it', + 'archived' => false) + + expect(collection.list(nil, filter, nil)) + .to eq([{ 'id' => '3', 'category' => 'submitted', 'internal_label' => 'Waiting on triage', + 'external_label' => 'We are on it', 'archived' => false }]) + end + end +end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_type_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_type_spec.rb new file mode 100644 index 000000000..f34ed10ba --- /dev/null +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_type_spec.rb @@ -0,0 +1,45 @@ +module ForestAdminDatasourceIntercom + RSpec.describe Collections::TicketType do + subject(:collection) { described_class.new(datasource) } + + let(:datasource) { Datasource.new(access_token: 's3cr3t', rate_limiter: nil) } + let(:base) { datasource.configuration.url } + + def filter + ForestAdminDatasourceToolkit::Components::Query::Filter.new + end + + def stub_ticket_types(*types) + stub_request(:get, "#{base}/ticket_types") + .to_return(status: 200, body: { 'type' => 'list', 'data' => types }.to_json, + headers: { 'Content-Type' => 'application/json' }) + end + + it 'is named IntercomTicketType' do + expect(collection.name).to eq('IntercomTicketType') + end + + it 'exposes what makes a ticket type readable' do + expect(collection.fields.keys).to eq(%w[id name description category icon archived]) + end + + # This endpoint uses the `data` envelope, unlike /admins and /teams. + it 'reads the endpoint through the data envelope' do + stub_ticket_types('type' => 'ticket_type', 'id' => '1', 'name' => 'Bug', 'description' => 'A bug', + 'category' => 'request', 'icon' => '🐛', 'archived' => false) + + expect(collection.list(nil, filter, nil)) + .to eq([{ 'id' => '1', 'name' => 'Bug', 'description' => 'A bug', 'category' => 'request', + 'icon' => '🐛', 'archived' => false }]) + end + + # The attribute definitions nested here are what the ticket collection reads + # to build its columns -- an attribute of the same name carries a different + # id from one type to the next -- and they are meaningless as a column. + it 'leaves the nested attribute definitions out of the schema' do + stub_ticket_types('id' => '1', 'ticket_type_attributes' => { 'type' => 'list', 'data' => [{ 'id' => '9' }] }) + + expect(collection.list(nil, filter, nil).first.keys).not_to include('ticket_type_attributes') + end + end +end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb index 70bc76281..b904efe50 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb @@ -6,8 +6,17 @@ module ForestAdminDatasourceIntercom expect { datasource }.not_to raise_error end - it 'registers no collection yet' do - expect(datasource.collections).to be_empty + # The reference collections come first: they are what turns an assignee id + # into a teammate and a state id into a label. + it 'publishes the reference collections' do + expect(datasource.collections.keys) + .to eq(%w[IntercomAdmin IntercomTeam IntercomTicketType IntercomTicketState]) + end + + it 'reaches Intercom only when a collection is listed, never while booting' do + datasource + + expect(WebMock).not_to have_requested(:get, /intercom/) end it 'configures a client from the options it is handed' do @@ -22,7 +31,7 @@ module ForestAdminDatasourceIntercom end it 'names the collections it holds when printed' do - expect(datasource.inspect).to eq('#') + expect(datasource.inspect).to include('IntercomAdmin', 'IntercomTicketState') end it 'never prints the token the client carries' do From 129ec6494a988de056f52c58c47e0516c57a489b Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Mon, 31 Aug 2026 20:07:07 +0200 Subject: [PATCH 5/9] feat(intercom): read conversations, thread included Fifth step of lot 1 (PRD-1112). The collection an ops team actually works in: what is open, assigned to whom, since when, and what the customer wrote. Cursor collections get their own tier, the opposite of the reference one in every respect: what is in hand is a page of something far larger, so nothing may be filtered, sorted or counted in memory without answering a fraction as if it were the whole. Three routes and no fourth -- no condition walks the listing, `id equals X` reads the record through its own endpoint, and anything else is refused with a message naming the lot that will answer it. Every column ships unfilterable and unsortable for that reason; only the primary key advertises operators, and it is answered by the record endpoint rather than by a filter. Counting is the exception that costs nothing: `total_count` is exact on every response, so the record counter is one request. A group-by is refused rather than computed over the pages a walk collected, which would look exact while answering a fraction. The order Intercom silently drops is reported. A sort sent to these endpoints raises nothing and changes nothing -- measured -- so an operator who asked for an order and did not get one learns it here or nowhere. The timeline opens on `source`, not on the parts: the message that started the conversation lives there, and a thread built from the parts alone loses exactly the one nobody opens a conversation without wanting to read. Every entry keeps its `part_type`, an assignment and a reply being different events. Since Intercom returns the parts only when retrieving a single conversation, a record read gets its timeline for free while a list view pays a request per row: bounded to ten, and the rows past it keep a nil that reads as unknown rather than an empty thread. Contact identity is denormalized onto the row by one bulk read per page rather than a lookup per row, and a failure there costs the two columns instead of the page. It stays a pair of columns rather than a relation: the Contacts collection arrives in lot 4, and a relation whose target is missing is a schema the agent refuses to boot on. Two Intercom particulars handled on the way: the records come under `conversations` rather than the `data` envelope, so `list_page` now takes a `list_key` like `fetch_all` does, and dates travel as epoch seconds, read back as ISO8601 in UTC -- where Intercom stores and truncates, and where a local rendering would hide the shift that makes a day-granular date filter wrong. Co-Authored-By: Claude Opus 5 (1M context) --- .rubocop.yml | 1 + .../lib/forest_admin_datasource_intercom.rb | 1 + .../client.rb | 60 ++- .../collections/base_collection.rb | 12 + .../collections/conversation.rb | 192 +++++++ .../collections/conversation/serializer.rb | 101 ++++ .../collections/conversation/timeline.rb | 73 +++ .../collections/cursor_collection.rb | 240 +++++++++ .../datasource.rb | 9 +- .../collections/conversation_spec.rb | 474 ++++++++++++++++++ .../datasource_spec.rb | 7 +- 11 files changed, 1145 insertions(+), 25 deletions(-) create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation.rb create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation/serializer.rb create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation/timeline.rb create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb create mode 100644 packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/conversation_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index c6901590f..b2cc222be 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -265,6 +265,7 @@ Metrics/ParameterLists: - 'packages/forest_admin_agent/lib/forest_admin_agent/audit_trail/store.rb' - 'packages/forest_admin_datasource_graphql_hasura/lib/forest_admin_datasource_graphql_hasura/configuration.rb' - 'packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/configuration.rb' + - 'packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/client.rb' - 'packages/forest_admin_datasource_zendesk/lib/forest_admin_datasource_zendesk/collections/base_collection.rb' - 'packages/forest_admin_datasource_snowflake/lib/forest_admin_datasource_snowflake/datasource.rb' - 'packages/forest_admin_agent/lib/forest_admin_agent/routes/query_handler.rb' diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom.rb index 32db16f9b..88371a7f6 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom.rb @@ -2,6 +2,7 @@ require 'json' require 'logger' require 'set' +require 'time' require 'uri' require 'zeitwerk' require 'faraday' diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/client.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/client.rb index 9bef4e18f..d9362403b 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/client.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/client.rb @@ -50,11 +50,38 @@ def me(boot: false) # # `CursorWalker` is what turns the offset/limit window a list view asks for # into a sequence of these. - def list_page(path, per_page:, starting_after: nil, params: {}, boot: false) + # + # `list_key` is the key the endpoint puts its records under, and it is not + # `data` everywhere: `/tickets/search` answers under `tickets` (measured), so + # a caller names its own and `data` stays the fallback. + def list_page(path, per_page:, starting_after: nil, params: {}, list_key: 'data', boot: false) query = params.merge('per_page' => self.class.bounded_per_page(per_page)) query['starting_after'] = starting_after unless blank?(starting_after) - must_succeed(path) { to_page(get(path, query, boot: boot).body, path) } + must_succeed(path) { to_page(get(path, query, boot: boot).body, path, list_key) } + end + + # One page of a search endpoint. The query is written by the caller rather + # than translated from a Forest filter -- that translation is lot 2 -- so + # what goes on the wire is what the caller asked for. + def search_page(path, query:, per_page:, starting_after: nil, list_key: 'data') + pagination = { 'per_page' => self.class.bounded_per_page(per_page) } + pagination['starting_after'] = starting_after unless blank?(starting_after) + body = { 'query' => query, 'pagination' => pagination } + + must_succeed(path) { to_page(post(path, body).body, path, list_key) } + end + + # One record from its own endpoint. Raises on a 404 like on any other + # failure: what a missing record means -- a stale link, a record outside the + # token's scope, a deletion -- is the caller's to decide, not the client's. + def fetch_record(path, id, params: {}, boot: false) + operation = "#{path}/#{id}" + + must_succeed(operation) do + body = get("#{path}/#{Faraday::Utils.escape(id)}", params, boot: boot).body + body.is_a?(Hash) ? body : refuse_body_shape(operation, 'the response is not a record') + end end # Every record of an endpoint that answers in one response: the reference @@ -96,6 +123,10 @@ def get(path, params = nil, boot: false) (boot ? boot_connection : connection).get(path, params) end + def post(path, body, boot: false) + (boot ? boot_connection : connection).post(path, body) + end + # Intercom serves the version its workspace defaults to when the pin is not # honoured, and the payloads differ between versions. The echo is the only # way to notice, and noticing at boot is worth more than a schema that @@ -134,15 +165,19 @@ def collect_pages(path, list_key:, boot:) end # The records under the key the endpoint uses, or under `data`. Anything - # else is refused: a reference collection silently read as empty is a - # ticket-state column with no values and an assignee shown as a raw id. + # else is refused rather than read as an empty page: `Array()` would turn the + # envelope into `[key, value]` pairs a collection would serialize into rows + # holding nothing -- a page that looks answered and is empty -- and a + # reference collection silently read as empty is a state column with no + # values and an assignee shown as a raw id. def extract_entities(body, operation, list_key) return [] if body.nil? || body == '' entities = body.is_a?(Hash) ? (body[list_key] || body['data']) : nil return entities if entities.is_a?(Array) - refuse_body_shape(operation, "neither '#{list_key}' nor 'data' is a list") + detail = list_key == 'data' ? "'data' is not a list" : "neither '#{list_key}' nor 'data' is a list" + refuse_body_shape(operation, detail) end def log_collection_cap(path, pages, collected) @@ -155,23 +190,12 @@ def log_collection_cap(path, pages, collected) # Intercom wraps a listing in `{ "type": "list", "data": [...], # "total_count": N, "pages": { "next": { "starting_after": "..." } } }`. - def to_page(body, operation) - Page.new(records: extract_list(body, operation), + def to_page(body, operation, list_key) + Page.new(records: extract_entities(body, operation, list_key), next_cursor: next_cursor(body, operation), total_count: extract_count(body)) end - # A listing whose `data` is absent or is not a list broke the contract. It - # is refused rather than read as an empty page: `Array()` would turn the - # envelope into `[key, value]` pairs the collection would serialize into - # rows holding nothing -- a page that looks answered and is empty. - def extract_list(body, operation) - data = body.is_a?(Hash) ? body['data'] : nil - return data if data.is_a?(Array) - - refuse_body_shape(operation, "'data' is not a list") - end - # Absent on the last page, which is how the walk knows it is done. An older # API version spells it as a url instead of an object -- and one can be # served despite the pin, which is what the version echo warns about -- so diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/base_collection.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/base_collection.rb index 3cdf5f549..0fc1f4835 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/base_collection.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/base_collection.rb @@ -12,6 +12,7 @@ class BaseCollection < ForestAdminDatasourceToolkit::Collection ColumnSchema = ForestAdminDatasourceToolkit::Schema::ColumnSchema Operators = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators Equivalent = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::ConditionTreeEquivalent + Leaf = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf def initialize(datasource, name) super @@ -66,6 +67,17 @@ def timezone_for(caller) def stringify_id(value) value&.to_s end + + # Intercom dates travel as epoch seconds; Forest reads a Date column as an + # ISO8601 string, and a filter carries one too, so comparing the two is the + # ordering itself. UTC deliberately: that is where Intercom stores and + # truncates, and rendering a local time here would hide the very shift that + # makes a day-granular date filter wrong. + def stamp(seconds) + return nil unless seconds.is_a?(Numeric) && seconds.positive? + + Time.at(seconds).utc.iso8601 + end end end end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation.rb new file mode 100644 index 000000000..4bc978585 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation.rb @@ -0,0 +1,192 @@ +module ForestAdminDatasourceIntercom + module Collections + # The conversations of the workspace: what a support team actually works on. + # + # Read through `GET /conversations`, whose records Intercom puts under + # `conversations` rather than under the `data` envelope -- `/tickets/search` + # does the same with `tickets`, so the key is named rather than assumed. + # + # `display_as=plaintext` on every read: the bodies are HTML written by end + # customers, and rendering third-party HTML inside Forest is neither safe nor + # useful (R10). + # Long by line count only: most of it declares the columns, one call each. + class Conversation < CursorCollection # rubocop:disable Metrics/ClassLength + include Conversation::Serializer + include Conversation::Timeline + + # How many conversations of one page may have their timeline read. The + # parts are absent from the listing response -- Intercom returns them only + # when retrieving a single conversation -- so a timeline asked for in a + # list view costs one request per row. Bounded rather than turned into a + # page the operator waits half a minute for; rows past the cap are left at + # nil, which reads as "unknown", never as "this conversation is empty". + MAX_TIMELINE_READS = 10 + + # An `id in [...]` read of contacts is one request per chunk, against the + # whole page rather than per row. + CONTACT_CHUNK = 100 + + def initialize(datasource) + super(datasource, 'IntercomConversation') + end + + protected + + def list_endpoint = 'conversations' + def list_key = 'conversations' + def read_params = { 'display_as' => 'plaintext' } + + # The contact identity and the timeline, each read only when the projection + # names it: neither is on the conversation payload, and a page that never + # asked for them must not pay for them. + def enrich(records, rows, projection) + wanted = Array(projection).map(&:to_s) + + embed_contact_identity(records, rows, wanted) + embed_timeline(records, rows, wanted) + end + + private + + def define_schema + add_column('id', 'String', is_primary_key: true) + add_column('title', 'String') + # Left plain strings rather than enums: the values a workspace really + # serves are worth measuring before the interface offers them as a + # closed list, and nothing filters on them in this lot anyway. + add_column('state', 'String') + add_column('priority', 'String') + add_column('open', 'Boolean') + add_column('read', 'Boolean') + add_column('created_at', 'Date') + add_column('updated_at', 'Date') + add_column('waiting_since', 'Date') + add_column('snoozed_until', 'Date') + add_column('admin_assignee_id', 'String') + add_column('team_assignee_id', 'String') + # The conversation carries its company as a whole object, so the account + # name is free here -- unlike on a ticket, which carries the id alone. + add_column('company_id', 'String') + add_column('company_name', 'String') + define_contact_columns + define_source_columns + define_statistics_columns + add_column('tag_names', 'Json') + add_column('ai_agent_participated', 'Boolean') + add_column('timeline', 'Json') + end + + # The contact identity is denormalized onto the row rather than declared as + # a relation: the Contacts collection arrives in lot 4, and a relation whose + # target collection is missing is a schema the agent refuses to boot on. + # + # A group conversation has several contacts; the row carries the first and + # says how many there are, rather than pretending there is one. + def define_contact_columns + add_column('contact_ids', 'Json') + add_column('contact_count', 'Number') + add_column('contact_name', 'String') + add_column('contact_email', 'String') + end + + # The message that opened the conversation lives in `source`, not in the + # parts. A timeline built from the parts alone loses it, which is the one + # message nobody opens a conversation without wanting to read. + def define_source_columns + add_column('source_type', 'String') + add_column('source_subject', 'String') + add_column('source_body', 'String') + add_column('source_author_name', 'String') + add_column('source_author_email', 'String') + add_column('source_delivered_as', 'String') + end + + # Intercom keeps the lifecycle of a conversation in `statistics`, which is + # where the closure date and the reply timestamps come from. Flattened onto + # the row: they cost nothing, they are exact, and they are what an ops lead + # reads a queue for. + def define_statistics_columns + add_column('closed_at', 'Date') + add_column('first_closed_at', 'Date') + add_column('closed_by_id', 'String') + add_column('first_contact_reply_at', 'Date') + add_column('last_contact_reply_at', 'Date') + add_column('last_admin_reply_at', 'Date') + add_column('reopen_count', 'Number') + add_column('part_count', 'Number') + end + + def embed_contact_identity(records, rows, projection) + return unless (%w[contact_name contact_email] & projection).any? + + identities = contact_identities(records) + records.each_with_index do |record, index| + identity = identities[first_contact_id(record)] || {} + rows[index]['contact_name'] = identity['name'] if rows[index].key?('contact_name') + rows[index]['contact_email'] = identity['email'] if rows[index].key?('contact_email') + end + end + + # One read per chunk of ids for the whole page, never one per row. A + # failure costs the two columns and nothing else: an identity that could + # not be read is not a page that could not be served. + def contact_identities(records) + ids = records.filter_map { |record| first_contact_id(record) }.uniq + return {} if ids.empty? + + ids.each_slice(CONTACT_CHUNK).with_object({}) do |chunk, indexed| + page = client.search_page('contacts/search', per_page: chunk.size, + query: { 'field' => 'id', 'operator' => 'IN', + 'value' => chunk }) + page.records.each { |contact| indexed[contact['id'].to_s] = contact } + end + rescue APIError => e + ForestAdminDatasourceIntercom.logger.warn( + "[forest_admin_datasource_intercom] #{name} could not read the contacts of this page (HTTP " \ + "#{e.status || "-"}); the name and e-mail columns are left empty for it." + ) + {} + end + + # A record read through the record endpoint already carries its parts, so + # its timeline is free; one read from the listing does not, and pays a + # request. Rows past the cap keep the nil the projection put there. + def embed_timeline(records, rows, projection) + return unless projection.include?('timeline') + + budget = MAX_TIMELINE_READS + missing = 0 + + records.each_with_index do |record, index| + if parts_of(record) + rows[index]['timeline'] = build_timeline(record) + elsif budget.positive? + budget -= 1 + detail = read_detail(record['id']) + rows[index]['timeline'] = detail && build_timeline(detail) + else + missing += 1 + end + end + + warn_truncated_timelines(missing) if missing.positive? + end + + def read_detail(id) + client.fetch_record(list_endpoint, id, params: read_params) + rescue APIError => e + raise unless e.status == 404 + + nil + end + + def warn_truncated_timelines(missing) + ForestAdminDatasourceIntercom.logger.warn( + "[forest_admin_datasource_intercom] #{name} left the timeline of #{missing} row(s) unread: Intercom " \ + 'returns the parts only when retrieving one conversation, so a list view pays a request per row and ' \ + "this reads at most #{MAX_TIMELINE_READS}. Those rows show no timeline rather than an empty one." + ) + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation/serializer.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation/serializer.rb new file mode 100644 index 000000000..69c9459c4 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation/serializer.rb @@ -0,0 +1,101 @@ +module ForestAdminDatasourceIntercom + module Collections + class Conversation < CursorCollection + # One Intercom conversation flattened into the row the schema declares. + # Nothing here reads a sub-resource: every value comes from the payload the + # listing already returned. + module Serializer + protected + + def serialize(conversation) + attrs = conversation.is_a?(Hash) ? conversation : {} + + native(attrs) + .merge(contacts_of(attrs)) + .merge(source_of(attrs['source'])) + .merge(statistics_of(attrs['statistics'])) + end + + private + + def native(attrs) + company = attrs['company'].is_a?(Hash) ? attrs['company'] : {} + + { + 'id' => stringify_id(attrs['id']), + 'title' => attrs['title'], + 'state' => attrs['state'], + 'priority' => attrs['priority'], + 'open' => attrs['open'], + 'read' => attrs['read'], + 'created_at' => stamp(attrs['created_at']), + 'updated_at' => stamp(attrs['updated_at']), + 'waiting_since' => stamp(attrs['waiting_since']), + 'snoozed_until' => stamp(attrs['snoozed_until']), + 'admin_assignee_id' => stringify_id(attrs['admin_assignee_id']), + 'team_assignee_id' => stringify_id(attrs['team_assignee_id']), + 'company_id' => stringify_id(company['id']), + 'company_name' => company['name'], + 'tag_names' => nested_list(attrs['tags'], 'tags').filter_map { |tag| tag['name'] if tag.is_a?(Hash) }, + 'ai_agent_participated' => attrs['ai_agent_participated'] + } + end + + # A group conversation carries several contacts. The row names the first + # and counts them, rather than presenting one of several as the one. + def contacts_of(attrs) + ids = nested_list(attrs['contacts'], 'contacts').filter_map { |contact| stringify_id(contact['id']) } + + { 'contact_ids' => ids, 'contact_count' => ids.size, + # Filled by the bulk read of `enrich`, and left nil when the + # projection did not ask for them. + 'contact_name' => nil, 'contact_email' => nil } + end + + def source_of(source) + attrs = source.is_a?(Hash) ? source : {} + author = attrs['author'].is_a?(Hash) ? attrs['author'] : {} + + { 'source_type' => attrs['type'], + 'source_subject' => attrs['subject'], + # Plaintext, because `display_as=plaintext` rides on every read: the + # bodies are HTML written by end customers. + 'source_body' => attrs['body'], + 'source_author_name' => author['name'], + 'source_author_email' => author['email'], + 'source_delivered_as' => attrs['delivered_as'] } + end + + # `statistics` is null on a conversation Intercom has computed nothing + # for yet; every column then reads as absent rather than as zero. + def statistics_of(statistics) + attrs = statistics.is_a?(Hash) ? statistics : {} + + { 'closed_at' => stamp(attrs['last_close_at']), + 'first_closed_at' => stamp(attrs['first_close_at']), + 'closed_by_id' => stringify_id(attrs['last_closed_by_id']), + 'first_contact_reply_at' => stamp(attrs['first_contact_reply_at']), + 'last_contact_reply_at' => stamp(attrs['last_contact_reply_at']), + 'last_admin_reply_at' => stamp(attrs['last_admin_reply_at']), + 'reopen_count' => attrs['count_reopens'], + 'part_count' => attrs['count_conversation_parts'] } + end + + # Intercom nests its lists twice -- `{"type": "contact.list", "contacts": + # [...]}` -- and answers a null instead of an empty list when there is + # nothing. + def nested_list(container, key) + return [] unless container.is_a?(Hash) + + list = container[key] + list.is_a?(Array) ? list : [] + end + + def first_contact_id(record) + contact = nested_list((record || {})['contacts'], 'contacts').first + contact.is_a?(Hash) ? stringify_id(contact['id']) : nil + end + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation/timeline.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation/timeline.rb new file mode 100644 index 000000000..9b757f6c8 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation/timeline.rb @@ -0,0 +1,73 @@ +module ForestAdminDatasourceIntercom + module Collections + class Conversation < CursorCollection + # The thread of a conversation, as a structured list the record view can + # render: who said what, when, and through which kind of event. + # + # Two things this exists to get right. The opening message lives in + # `source`, not in the parts -- a timeline built from the parts alone opens + # on the first reply and loses what the customer actually asked. And + # `part_type` is kept on every entry: an assignment, a note and a reply are + # not the same event, and a thread that flattens them reads as a + # conversation that never happened the way it did. + # + # Intercom caps a conversation at its 500 most recent parts; the entry + # count is therefore what is in hand, not necessarily what exists. + module Timeline + # The pseudo type of the opening entry. Not an Intercom part type: it is + # the source, and calling it `comment` would make it indistinguishable + # from the replies that follow. + SOURCE_PART_TYPE = 'conversation_started'.freeze + + private + + def build_timeline(conversation) + attrs = conversation.is_a?(Hash) ? conversation : {} + entries = [source_entry(attrs)].compact + + entries + (parts_of(attrs) || []).map { |part| part_entry(part) } + end + + # nil rather than an empty list when the payload carries no parts at all: + # a listing response has none, and reading that as "this conversation is + # empty" is exactly the answer that looks complete without being it. + def parts_of(conversation) + container = (conversation || {})['conversation_parts'] + return nil unless container.is_a?(Hash) + + parts = container['conversation_parts'] + parts.is_a?(Array) ? parts : nil + end + + def source_entry(attrs) + source = attrs['source'] + return nil unless source.is_a?(Hash) + + entry(part_type: SOURCE_PART_TYPE, created_at: attrs['created_at'], author: source['author'], + body: source['body'], attachments: source['attachments']) + .merge('id' => stringify_id(source['id'])) + end + + def part_entry(part) + attrs = part.is_a?(Hash) ? part : {} + + entry(part_type: attrs['part_type'], created_at: attrs['created_at'], author: attrs['author'], + body: attrs['body'], attachments: attrs['attachments']) + .merge('id' => stringify_id(attrs['id']), 'redacted' => attrs['redacted']) + end + + def entry(part_type:, created_at:, author:, body:, attachments:) + writer = author.is_a?(Hash) ? author : {} + + { 'part_type' => part_type, + 'created_at' => stamp(created_at), + 'author_type' => writer['type'], + 'author_name' => writer['name'], + 'author_email' => writer['email'], + 'body' => body, + 'attachment_count' => Array(attachments).size } + end + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb new file mode 100644 index 000000000..8d6251943 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb @@ -0,0 +1,240 @@ +module ForestAdminDatasourceIntercom + module Collections + # Base for the collections Intercom paginates by cursor: conversations and + # tickets. The opposite tier of `FetchAllCollection` in every way -- what is + # in hand is a page of something far larger, so nothing may be filtered, + # sorted or counted in memory without answering a fraction as if it were the + # whole. + # + # Three routes, and no fourth: + # + # * no condition at all -- a list view -- walks the listing endpoint; + # * `id equals X` reads the record through its own endpoint, which is what a + # record detail is; + # * anything else is **refused**. Translating a Forest condition tree into + # Intercom's search DSL is lot 2, and until it exists a filter that cannot + # be honoured has to say so: an unfiltered page served in answer to a + # filter is the one failure this datasource is built to avoid. + # + # Counting is the exception that costs nothing: `total_count` is exact on + # every response, filter included, so the record counter is one request. + # Long by line count only: half of it is the refusals, and a refusal that + # does not say what to do instead is a refusal an operator cannot act on. + class CursorCollection < BaseCollection # rubocop:disable Metrics/ClassLength + Aggregation = ForestAdminDatasourceToolkit::Components::Query::Aggregation + + # How many records an `id in [...]` read may fetch. One request per id -- + # Intercom has no "read these records" endpoint -- so the fan-out is + # bounded rather than turned into a rate limit halfway through a page. + MAX_ID_READS = 25 + + # Countable, and exactly: unlike the pages a walk collected, `total_count` + # is the whole dataset the filter names. + def initialize(datasource, name) + super + enable_count + end + + def list(_caller, filter, projection) + warn_ignored_sort(filter&.sort) + + records = fetch_records(filter) + rows = records.map { |record| project(serialize(record), projection) } + enrich(records, rows, projection) + rows + end + + # Count only, and never a group: Intercom exposes no aggregate endpoint, + # and grouping over the pages a walk happened to collect would look exact + # while answering a fraction. Refused here rather than through the + # contract's NotImplementedError, which reads as an oversight. + def aggregate(_caller, filter, aggregation, _limit = nil) + refuse_unsupported_aggregation!(aggregation) + + [{ 'group' => {}, 'value' => count_records(filter) }] + end + + protected + + # The listing endpoint, its record key, and the parameters every read of + # this collection carries. + def list_endpoint = raise(NotImplementedError, "#{self.class} did not implement list_endpoint") + def record_endpoint = list_endpoint + def list_key = 'data' + def read_params = {} + + # One Intercom entity flattened into a record matching the schema. + def serialize(_entity) = raise(NotImplementedError, "#{self.class} did not implement serialize") + + # Hook for what a row needs beyond its own payload. Left empty here: what + # it costs is the collection's business, not this base's. + def enrich(_records, _rows, _projection); end + + # Bounded per collection rather than by the API maximum: Intercom offers no + # field selection, so a collection whose rows carry their whole timeline + # pays for it by the page. See Ticket. + def max_page_size = Client::MAX_PER_PAGE + + # A column of this tier advertises no filter and no sort, because the + # collection can honour neither -- except on the primary key, which is + # answered by the record endpoint rather than by a filter. A schema that + # advertised more would put filters in the interface that the read then + # refuses. + def add_column(name, type, is_primary_key: false) + operators = is_primary_key ? [Operators::EQUAL, Operators::IN] : [] + add_field(name, ColumnSchema.new(column_type: type, + filter_operators: operators, + is_primary_key: is_primary_key, + is_sortable: false, + is_groupable: false)) + end + + def walker + @walker ||= Pagination::CursorWalker.new + end + + private + + def fetch_records(filter) + ids = id_lookup(filter) + return records_by_ids(ids) if ids + + refuse_filter!(filter) unless browsing?(filter) + + listed_records(filter) + end + + def browsing?(filter) + filter.nil? || (filter.condition_tree.nil? && blank_search?(filter)) + end + + def blank_search?(filter) + search = filter.respond_to?(:search) ? filter.search : nil + search.nil? || search.to_s.strip.empty? + end + + # A record detail is `id equals X`, and a bulk read of related records is + # `id in [...]`. Only a bare leaf on the primary key takes this route: an + # `and` also carrying a scope names a narrower set than the ids do, and + # answering it with the ids alone would serve records the scope excludes. + def id_lookup(filter) + tree = filter&.condition_tree + return nil unless tree.is_a?(Leaf) && tree.field.to_s == primary_key + return nil unless blank_search?(filter) + + case tree.operator + when Operators::EQUAL then [tree.value].compact.map(&:to_s) + when Operators::IN then Array(tree.value).compact.map(&:to_s) + end + end + + def primary_key + @primary_key ||= fields.find do |_name, field| + field.respond_to?(:is_primary_key) && field.is_primary_key + end&.first + end + + # A record the operator can no longer reach -- deleted, or outside the + # token's scope -- reads as "no record" rather than as a failed page. + def records_by_ids(ids) + wanted = ids.first(MAX_ID_READS) + warn_truncated_ids(ids.size) if ids.size > wanted.size + + wanted.filter_map do |id| + client.fetch_record(record_endpoint, id, params: read_params) + rescue APIError => e + raise unless e.status == 404 + + nil + end + end + + def listed_records(filter) + offset, limit = translate_page(filter&.page) + + walker.walk(offset: offset, limit: limit) do |per_page, cursor| + client.list_page(list_endpoint, per_page: [per_page, max_page_size].min, + starting_after: cursor, params: read_params, list_key: list_key) + end + end + + # A filter with no page asks for every record it matched; the walker reads + # that as the nil limit it bounds with its own caps. + def translate_page(page) + return [0, nil] if page.nil? + + limit = page.limit.to_i + [page.offset.to_i.clamp(0, nil), limit.positive? ? limit : nil] + end + + # Exact, and one request: `total_count` counts what the filter names, not + # what a page happened to hold. An id lookup counts the records it found, + # which is cheaper still. + def count_records(filter) + ids = id_lookup(filter) + return records_by_ids(ids).size if ids + + refuse_filter!(filter) unless browsing?(filter) + + page = client.list_page(list_endpoint, per_page: 1, params: read_params, list_key: list_key) + return page.total_count if page.total_count + + raise UnsupportedOperatorError, + "#{name} cannot be counted: Intercom answered this listing without a total_count, and counting the " \ + 'pages the agent walked would answer a fraction of the collection as if it were the whole of it.' + end + + def refuse_unsupported_aggregation!(aggregation) + return if aggregation.is_a?(Aggregation) && aggregation.operation.to_s.casecmp('count').zero? && + Array(aggregation.groups).empty? && aggregation.field.nil? + + raise UnsupportedOperatorError, + "#{name} can only be counted: Intercom exposes no aggregate endpoint, and grouping or summing the " \ + 'pages the agent walked would answer a fraction of the collection as if it were the whole of it. ' \ + 'Chart it on a collection read whole, or wait for the bounded group-by of the reporting lot.' + end + + def refuse_filter!(filter) + detail = if filter&.condition_tree + 'a condition on this collection' + else + 'a free-text search' + end + + raise UnsupportedOperatorError, + "#{name} cannot answer #{detail} yet: it reads Intercom's listing endpoint, which takes no filter. " \ + 'Server-side filtering goes through the search endpoint and arrives with the filter translation. ' \ + 'Until then, remove the condition, the scope or the segment carrying it rather than being served a ' \ + 'page that would look filtered without being it.' + end + + # Intercom accepts a `sort` on these endpoints and ignores it without a + # word -- measured -- so an order the operator asked for and did not get + # has to be reported here or nowhere. The ascending primary-key sort the + # agent injects when a request names none is not one of those. + def warn_ignored_sort(sort) + clauses = Array(sort) + return if clauses.empty? || default_pk_sort?(clauses) + + ForestAdminDatasourceIntercom.logger.warn( + "[forest_admin_datasource_intercom] #{name} was asked to sort on " \ + "#{clauses.map { |clause| clause[:field] || clause["field"] }.join(", ")}, and Intercom ignores a sort on " \ + 'this endpoint without reporting it. The rows come back in the order the API imposes.' + ) + end + + def default_pk_sort?(clauses) + clauses.size == 1 && + (clauses.first[:field] || clauses.first['field']).to_s == primary_key && + (clauses.first[:ascending] || clauses.first['ascending']) != false + end + + def warn_truncated_ids(asked) + ForestAdminDatasourceIntercom.logger.warn( + "[forest_admin_datasource_intercom] #{name} was asked for #{asked} records by id and read the first " \ + "#{MAX_ID_READS}: Intercom reads them one request each. The result is truncated." + ) + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb index c9e082676..601022e13 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb @@ -23,15 +23,16 @@ def inspect private # The reference collections first: they are what turns an assignee id into a - # teammate and a state id into a label, and nothing else in the schema points - # at them yet. Conversations and Tickets follow, and no request is made here - # -- each collection reads its endpoint when it is listed, so a datasource - # boots whatever Intercom is doing. + # teammate and a state id into a label. No request is made here -- each + # collection reads its endpoint when it is listed, so a datasource boots + # whatever Intercom is doing, and a workspace the token cannot read costs + # rows rather than the agent. def register_collections add_collection(Collections::Admin.new(self)) add_collection(Collections::Team.new(self)) add_collection(Collections::TicketType.new(self)) add_collection(Collections::TicketState.new(self)) + add_collection(Collections::Conversation.new(self)) end end end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/conversation_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/conversation_spec.rb new file mode 100644 index 000000000..e5ddbeb6d --- /dev/null +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/conversation_spec.rb @@ -0,0 +1,474 @@ +module ForestAdminDatasourceIntercom + RSpec.describe Collections::Conversation do + subject(:collection) { datasource.get_collection('IntercomConversation') } + + let(:datasource) { Datasource.new(access_token: 's3cr3t', rate_limiter: nil) } + let(:base) { datasource.configuration.url } + let(:operators) { ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators } + + def filter(condition_tree: nil, page: nil, sort: nil) + ForestAdminDatasourceToolkit::Components::Query::Filter.new(condition_tree: condition_tree, page: page, + sort: sort) + end + + def leaf(field, operator, value = nil) + ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf + .new(field, operator, value) + end + + def json(payload, status = 200) + { status: status, body: payload.to_json, headers: { 'Content-Type' => 'application/json' } } + end + + # Hand-written from the OpenAPI 2.16 spec, never captured from a workspace: + # a conversation body is personal data. + def conversation(id, overrides = {}) + { + 'type' => 'conversation', 'id' => id, 'title' => "Facture #{id}", 'state' => 'closed', + 'priority' => 'priority', 'open' => false, 'read' => true, + 'created_at' => 1_700_000_000, 'updated_at' => 1_700_003_600, + 'waiting_since' => nil, 'snoozed_until' => nil, + 'admin_assignee_id' => 493_881, 'team_assignee_id' => 814_865, + 'company' => { 'type' => 'company', 'id' => '696dd52099f73812610d9c7b', 'name' => 'Acme' }, + 'contacts' => { 'type' => 'contact.list', + 'contacts' => [{ 'type' => 'contact', 'id' => 'c1' }, { 'type' => 'contact', 'id' => 'c2' }] }, + 'tags' => { 'type' => 'tag.list', 'tags' => [{ 'id' => 't1', 'name' => 'billing' }] }, + 'ai_agent_participated' => true, + 'source' => { 'type' => 'conversation', 'id' => 's1', 'delivered_as' => 'customer_initiated', + 'subject' => 'Ma facture', 'body' => 'Bonjour, ou est ma facture ?', + 'author' => { 'type' => 'user', 'id' => 'c1', 'name' => 'Camille', + 'email' => 'camille@acme.test' }, + 'attachments' => [] }, + 'statistics' => { 'type' => 'conversation_statistics', 'first_close_at' => 1_700_002_000, + 'last_close_at' => 1_700_003_000, 'last_closed_by_id' => '493881', + 'first_contact_reply_at' => 1_700_000_050, 'last_contact_reply_at' => 1_700_001_000, + 'last_admin_reply_at' => 1_700_002_500, 'count_reopens' => 1, + 'count_conversation_parts' => 4 } + }.merge(overrides) + end + + def parts(*entries) + { 'conversation_parts' => { 'type' => 'conversation_part.list', 'conversation_parts' => entries } } + end + + def part(part_type, overrides = {}) + { 'type' => 'conversation_part', 'id' => 'p1', 'part_type' => part_type, 'body' => 'Je regarde.', + 'created_at' => 1_700_002_500, 'redacted' => false, 'attachments' => [], + 'author' => { 'type' => 'admin', 'id' => '493881', 'name' => 'Alice', + 'email' => 'alice@acme.test' } }.merge(overrides) + end + + # Intercom puts the records under `conversations`, not under the `data` + # envelope -- measured on `/tickets/search`, and the listings follow the same + # habit. + def stub_list(*records, next_cursor: nil, total: nil, query: hash_including({})) + body = { 'type' => 'conversation.list', 'conversations' => records, + 'total_count' => total || records.size, 'pages' => { 'type' => 'pages', 'page' => 1 } } + body['pages']['next'] = { 'starting_after' => next_cursor } if next_cursor + + stub_request(:get, "#{base}/conversations").with(query: query).to_return(json(body)) + end + + def stub_record(id, payload, status = 200) + stub_request(:get, "#{base}/conversations/#{id}").with(query: hash_including({})).to_return(json(payload, status)) + end + + def ids(rows) + rows.map { |row| row['id'] } + end + + describe 'schema' do + it 'is named IntercomConversation' do + expect(collection.name).to eq('IntercomConversation') + end + + # Intercom ignores a sort on this endpoint without a word and filters + # nothing on the listing, so a column advertising either would put in the + # interface what the read then refuses. + it 'declares every column unsortable and unfilterable, except the primary key' do + others = collection.fields.except('id') + + expect(others.values.map(&:is_sortable).uniq).to eq([false]) + expect(others.values.map(&:filter_operators).flatten.uniq).to be_empty + end + + # The record detail is `id equals X`, answered by the record endpoint + # rather than by a filter. + it 'answers the primary key with equal and in' do + expect(collection.fields['id'].filter_operators).to eq(%w[equal in]) + end + + it 'is countable, since total_count is exact' do + expect(collection.is_countable?).to be(true) + end + + # No aggregate endpoint, so no group-by may be offered. + it 'declares no column groupable' do + expect(collection.fields.values.map(&:is_groupable).uniq).to eq([false]) + end + end + + describe '#list' do + it 'reads the listing endpoint as plain text and pages by cursor' do + stub_list(conversation('1')) + + collection.list(nil, filter, nil) + + expect(WebMock).to have_requested(:get, "#{base}/conversations") + .with(query: hash_including('display_as' => 'plaintext')) + end + + it 'flattens the payload into the row the schema declares' do + stub_list(conversation('1')) + + row = collection.list(nil, filter, nil).first + + expect(row).to include('id' => '1', 'title' => 'Facture 1', 'state' => 'closed', 'open' => false, + 'company_id' => '696dd52099f73812610d9c7b', 'company_name' => 'Acme', + 'admin_assignee_id' => '493881', 'team_assignee_id' => '814865', + 'tag_names' => %w[billing], 'ai_agent_participated' => true) + end + + # Epoch seconds are what Intercom sends; a Date column and a date filter + # both read ISO8601, and UTC is where Intercom truncates. + it 'reads the dates as ISO8601 in UTC' do + row = (stub_list(conversation('1')) && collection.list(nil, filter, nil)).first + + expect(row['created_at']).to eq('2023-11-14T22:13:20Z') + end + + it 'flattens the lifecycle Intercom keeps in statistics' do + stub_list(conversation('1')) + + expect(collection.list(nil, filter, nil).first) + .to include('closed_at' => '2023-11-14T23:03:20Z', 'closed_by_id' => '493881', + 'last_admin_reply_at' => '2023-11-14T22:55:00Z', 'reopen_count' => 1, 'part_count' => 4) + end + + # A conversation Intercom has computed nothing for yet answers a null + # statistics; the columns then read as absent rather than as zero. + it 'reads a missing statistics block as absent, not as zero' do + stub_list(conversation('1', 'statistics' => nil)) + + expect(collection.list(nil, filter, nil).first) + .to include('closed_at' => nil, 'reopen_count' => nil) + end + + # A group conversation has several contacts: the row names how many rather + # than presenting one of them as the one. + it 'carries the contact ids and their count' do + stub_list(conversation('1')) + + expect(collection.list(nil, filter, nil).first) + .to include('contact_ids' => %w[c1 c2], 'contact_count' => 2) + end + + it 'narrows the row to the projection' do + stub_list(conversation('1')) + + expect(collection.list(nil, filter, %w[id state])).to eq([{ 'id' => '1', 'state' => 'closed' }]) + end + + it 'walks the cursor until the window is covered' do + first = { 'conversations' => [conversation('1'), conversation('2')], + 'pages' => { 'next' => { 'starting_after' => 'c2' } } } + stub_request(:get, "#{base}/conversations").with(query: hash_including('per_page' => '3')) + .to_return(json(first)) + stub_request(:get, "#{base}/conversations").with(query: hash_including('starting_after' => 'c2')) + .to_return(json('conversations' => [conversation('3')])) + + page = ForestAdminDatasourceToolkit::Components::Query::Page.new(offset: 2, limit: 1) + + expect(ids(collection.list(nil, filter(page: page), %w[id]))).to eq(%w[3]) + end + end + + describe '#list of one record' do + # What a record detail is. It goes to the record endpoint rather than to + # the listing, which is also what brings the parts along. + it 'reads id equals X through the record endpoint' do + stub_record('1', conversation('1')) + + expect(ids(collection.list(nil, filter(condition_tree: leaf('id', operators::EQUAL, '1')), %w[id]))) + .to eq(%w[1]) + end + + # A stale link, or a record outside the token's scope: no record, not a + # failed page. + it 'reads a 404 as no record' do + stub_record('gone', { 'errors' => [{ 'code' => 'not_found' }] }, 404) + + expect(collection.list(nil, filter(condition_tree: leaf('id', operators::EQUAL, 'gone')), %w[id])).to eq([]) + end + + it 'still raises on a failure that is not a missing record' do + stub_record('1', { 'errors' => [{ 'code' => 'forbidden' }] }, 403) + + expect { collection.list(nil, filter(condition_tree: leaf('id', operators::EQUAL, '1')), %w[id]) } + .to raise_error(APIError) + end + end + + describe '#list of several records by id' do + # What a pointing collection asks for when it reads related records in + # bulk. Intercom has no "read these records" endpoint, so it is one + # request per id -- and therefore bounded. + it 'reads each id through the record endpoint' do + %w[1 2].each do |id| + stub_record(id, conversation(id)) + end + + rows = collection.list(nil, filter(condition_tree: leaf('id', operators::IN, %w[1 2])), %w[id]) + + expect(ids(rows)).to eq(%w[1 2]) + end + + it 'reads the first of too many and says the result is truncated' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + asked = (1..(Collections::CursorCollection::MAX_ID_READS + 3)).map(&:to_s) + stub_request(:get, %r{/conversations/\d+}).to_return(json(conversation('1'))) + + rows = collection.list(nil, filter(condition_tree: leaf('id', operators::IN, asked)), %w[id]) + + expect(rows.size).to eq(Collections::CursorCollection::MAX_ID_READS) + expect(ForestAdminDatasourceIntercom.logger).to have_received(:warn).with(/read the first 25/) + end + end + + describe 'a filter it cannot honour' do + # Translating a Forest tree into Intercom's search DSL is the next lot. + # Until then a page that looks filtered without being it is the one answer + # this datasource must not give. + it 'refuses a condition on anything but the primary key' do + expect { collection.list(nil, filter(condition_tree: leaf('state', operators::EQUAL, 'open')), %w[id]) } + .to raise_error(UnsupportedOperatorError, /cannot answer a condition/) + end + + it 'refuses a free-text search' do + searched = ForestAdminDatasourceToolkit::Components::Query::Filter.new(search: 'facture') + + expect { collection.list(nil, searched, %w[id]) } + .to raise_error(UnsupportedOperatorError, /cannot answer a free-text search/) + end + + it 'says where the filtering will come from, so the message is actionable' do + expect { collection.list(nil, filter(condition_tree: leaf('state', operators::EQUAL, 'open')), %w[id]) } + .to raise_error(UnsupportedOperatorError, /search endpoint.*filter translation/m) + end + end + + describe 'a sort Intercom ignores' do + before { allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) } + + # Measured: a sort sent to this endpoint raises nothing and changes + # nothing, so an order the operator asked for and did not get can only be + # reported here. + it 'reports the order it did not get' do + stub_list(conversation('1')) + sort = ForestAdminDatasourceToolkit::Components::Query::Sort.new([{ field: 'created_at', ascending: false }]) + + collection.list(nil, filter(sort: sort), %w[id]) + + expect(ForestAdminDatasourceIntercom.logger).to have_received(:warn).with(/ignores a sort/) + end + + it 'stays quiet on the primary-key order the agent injects by default' do + stub_list(conversation('1')) + sort = ForestAdminDatasourceToolkit::Components::Query::Sort.new([{ field: 'id', ascending: true }]) + + collection.list(nil, filter(sort: sort), %w[id]) + + expect(ForestAdminDatasourceIntercom.logger).not_to have_received(:warn) + end + end + + describe '#aggregate' do + def aggregation(operation, field: nil, groups: []) + ForestAdminDatasourceToolkit::Components::Query::Aggregation.new(operation: operation, field: field, + groups: groups) + end + + # One request, and exact on the whole collection rather than on the page + # the walk happened to read. + it 'counts through total_count' do + stub_list(conversation('1'), total: 81_142, query: hash_including('per_page' => '1')) + + expect(collection.aggregate(nil, filter, aggregation('Count'))) + .to eq([{ 'group' => {}, 'value' => 81_142 }]) + end + + it 'counts the records an id lookup found' do + stub_record('1', conversation('1')) + + expect(collection.aggregate(nil, filter(condition_tree: leaf('id', operators::EQUAL, '1')), + aggregation('Count')).first['value']).to eq(1) + end + + # Grouping over the pages a walk collected would look exact while + # answering a fraction. + it 'refuses a group-by' do + expect { collection.aggregate(nil, filter, aggregation('Count', groups: [{ field: 'state' }])) } + .to raise_error(UnsupportedOperatorError, /can only be counted/) + end + + it 'refuses a sum' do + expect { collection.aggregate(nil, filter, aggregation('Sum', field: 'reopen_count')) } + .to raise_error(UnsupportedOperatorError, /can only be counted/) + end + + it 'refuses a condition it could not honour on the list either' do + expect do + collection.aggregate(nil, filter(condition_tree: leaf('state', operators::EQUAL, 'open')), + aggregation('Count')) + end.to raise_error(UnsupportedOperatorError, /cannot answer a condition/) + end + + # Counting the pages a walk collected would answer a fraction as if it + # were the whole, so a listing with no total_count is a listing this + # cannot count. + it 'refuses to count a listing Intercom answered without a total_count' do + stub_request(:get, "#{base}/conversations").with(query: hash_including({})) + .to_return(json('conversations' => [], + 'pages' => { 'type' => 'pages' })) + + expect { collection.aggregate(nil, filter, aggregation('Count')) } + .to raise_error(UnsupportedOperatorError, /without a total_count/) + end + end + + describe 'the contact identity' do + before do + stub_list(conversation('1')) + stub_request(:post, "#{base}/contacts/search") + .to_return(json('type' => 'list', + 'data' => [{ 'id' => 'c1', 'name' => 'Camille', 'email' => 'camille@acme.test' }])) + end + + # Denormalized rather than declared as a relation: the Contacts collection + # arrives in lot 4, and a relation whose target is missing is a schema the + # agent refuses to boot on. + it 'reads the identity of the page in one request and puts it on the row' do + row = collection.list(nil, filter, %w[id contact_name contact_email]).first + + expect(row).to include('contact_name' => 'Camille', 'contact_email' => 'camille@acme.test') + expect(WebMock).to have_requested(:post, "#{base}/contacts/search").once + end + + it 'asks for the contacts of the page by id' do + collection.list(nil, filter, %w[id contact_name]) + + expect(WebMock).to have_requested(:post, "#{base}/contacts/search") + .with(body: hash_including('query' => { 'field' => 'id', 'operator' => 'IN', 'value' => %w[c1] })) + end + + # A page that never asked for the identity must not pay for it. + it 'reads nothing when the projection does not name it' do + collection.list(nil, filter, %w[id state]) + + expect(WebMock).not_to have_requested(:post, "#{base}/contacts/search") + end + + # An identity that could not be read is not a page that could not be + # served: it costs the two columns. + it 'degrades to empty columns when the read fails' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + stub_request(:post, "#{base}/contacts/search").to_return(json({ 'errors' => [] }, 403)) + + row = collection.list(nil, filter, %w[id contact_name]).first + + expect(row['contact_name']).to be_nil + expect(ForestAdminDatasourceIntercom.logger).to have_received(:warn).with(/could not read the contacts/) + end + end + + describe 'the timeline' do + let(:conversation_with_parts) do + conversation('1').merge(parts(part('assignment', 'id' => 'p1', 'body' => nil, 'created_at' => 1_700_000_100), + part('comment', 'id' => 'p2', 'created_at' => 1_700_002_500))) + end + + # The message that opened the conversation lives in `source`, not in the + # parts: a timeline built from the parts alone loses what the customer + # actually asked. + it 'opens on the source message' do + stub_record('1', conversation_with_parts) + + timeline = collection.list(nil, filter(condition_tree: leaf('id', operators::EQUAL, '1')), + %w[id timeline]).first['timeline'] + + expect(timeline.first).to include('part_type' => 'conversation_started', + 'body' => 'Bonjour, ou est ma facture ?', + 'author_name' => 'Camille', 'created_at' => '2023-11-14T22:13:20Z') + end + + # An assignment, a note and a reply are not the same event; a thread that + # flattens them reads as a conversation that never happened that way. + it 'keeps the part type of every entry' do + stub_record('1', conversation_with_parts) + + timeline = collection.list(nil, filter(condition_tree: leaf('id', operators::EQUAL, '1')), + %w[timeline]).first['timeline'] + + expect(timeline.map { |entry| entry['part_type'] }).to eq(%w[conversation_started assignment comment]) + end + + it 'costs no request on a record read, the parts riding along with it' do + stub_record('1', conversation_with_parts) + + collection.list(nil, filter(condition_tree: leaf('id', operators::EQUAL, '1')), %w[timeline]) + + expect(WebMock).to have_requested(:get, "#{base}/conversations/1") + .with(query: hash_including({})).once + end + + # Intercom returns the parts only when retrieving one conversation, so a + # list view pays a request per row. + it 'reads the record when a listed row has no parts' do + stub_list(conversation('1')) + stub_record('1', conversation_with_parts) + + rows = collection.list(nil, filter, %w[id timeline]) + + expect(rows.first['timeline'].size).to eq(3) + end + + it 'reads nothing when the projection does not name it' do + stub_list(conversation('1')) + + collection.list(nil, filter, %w[id state]) + + expect(WebMock).not_to have_requested(:get, "#{base}/conversations/1").with(query: hash_including({})) + end + + # A conversation deleted between the page and the read of its timeline: + # the row keeps a nil timeline rather than failing the whole page. + it 'leaves the timeline unread when the record has gone' do + stub_list(conversation('1')) + stub_record('1', { 'errors' => [{ 'code' => 'not_found' }] }, 404) + + expect(collection.list(nil, filter, %w[id timeline]).first['timeline']).to be_nil + end + + it 'still raises when the timeline read fails for another reason' do + stub_list(conversation('1')) + stub_record('1', { 'errors' => [{ 'code' => 'forbidden' }] }, 403) + + expect { collection.list(nil, filter, %w[id timeline]) }.to raise_error(APIError) + end + + # Rows past the cap keep a nil timeline -- unknown -- rather than an empty + # list, which would read as "this conversation has no message". + it 'bounds the fan-out and says what it left unread' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + listed = (1..(described_class::MAX_TIMELINE_READS + 2)).map { |index| conversation(index.to_s) } + stub_list(*listed) + stub_request(:get, %r{/conversations/\d+}).to_return(json(conversation_with_parts)) + + rows = collection.list(nil, filter, %w[id timeline]) + + expect(rows.count { |row| row['timeline'].nil? }).to eq(2) + expect(ForestAdminDatasourceIntercom.logger).to have_received(:warn).with(/left the timeline of 2 row/) + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb index b904efe50..97a95c6fd 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb @@ -7,10 +7,11 @@ module ForestAdminDatasourceIntercom end # The reference collections come first: they are what turns an assignee id - # into a teammate and a state id into a label. - it 'publishes the reference collections' do + # into a teammate and a state id into a label. Conversations follow, Tickets + # next. + it 'publishes the collections of the lot' do expect(datasource.collections.keys) - .to eq(%w[IntercomAdmin IntercomTeam IntercomTicketType IntercomTicketState]) + .to eq(%w[IntercomAdmin IntercomTeam IntercomTicketType IntercomTicketState IntercomConversation]) end it 'reaches Intercom only when a collection is listed, never while booting' do From 36999b71b22bf8a9b355cd289b870f1eee207cc9 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Mon, 31 Aug 2026 20:36:08 +0200 Subject: [PATCH 6/9] feat(intercom): read tickets, closure and last reply derived Sixth step of lot 1 (PRD-1112), and the one the measurements on a real workspace of 81 142 tickets reshaped. There is no GET /tickets at all, so even an unfiltered list view goes through POST /tickets/search with a predicate matching everything, and its records come back under `tickets` rather than the `data` envelope. The cursor tier grew a `read_page` hook for that: walking a page is the base's business, which endpoint answers it is the collection's. The page size is bounded at 25, far below the 150 the API accepts. Not caution: a ticket carries its whole timeline in the search response and Intercom offers no field selection, so a page of 150 would move some 23 000 part objects. The figure is provisional until measured against real response sizes on the customer's workspace. That same payload is what makes the two derived columns defensible. A ticket has no `statistics` block -- measured, confirming the specification -- so neither a closure date nor a last responder exists as a field, but both are in the parts, which are paid for whether or not anything asks: * `closed_at` and `closed_by_name` come from the last transition into a state of category `resolved`. Matched on the `ticket_state_updated` prefix rather than the `_by_admin` variant the sample showed, since this workspace runs workflows and a closure done by automation would otherwise be invisible; transitions whose target equals the previous state are ignored, because they exist. * `last_reply_at`, `last_responder_name` and `last_responder_type` come from the last `comment` part, notes excluded: an internal note is a touch, not an answer to the person waiting. Both ship display-only, and that is not temporary: the search endpoint filters neither and ignores a sort without reporting it, so a column advertising either would put in the interface what the read cannot honour. When a resolved ticket's transition fell past Intercom's 500-part ceiling the value is unknown rather than absent -- detected by comparing the parts in hand with their total, and reported in a log since a Date column cannot say it. The ticket-type attributes are introspected once at boot and published as the union of every type's, keyed by name the way the payload is. The id each type gives the same name is kept even though nothing uses it yet: it is what the filter translation will need, and re-reading it would cost a second boot round trip. A token without that permission costs the attribute columns, never the boot. An attribute whose name a native column already carries is skipped rather than overwriting it. The state arrives embedded as a whole object, so its labels cost nothing, and the contact identity denormalization moved to a module both collections now share. Co-Authored-By: Claude Opus 5 (1M context) --- .../collections/base_collection.rb | 10 + .../collections/contact_identity.rb | 78 +++++ .../collections/conversation.rb | 39 +-- .../collections/conversation/serializer.rb | 28 +- .../collections/cursor_collection.rb | 15 +- .../collections/ticket.rb | 131 ++++++++ .../collections/ticket/derived_columns.rb | 127 ++++++++ .../collections/ticket/serializer.rb | 77 +++++ .../datasource.rb | 10 + .../schema/ticket_attributes_introspector.rb | 92 ++++++ .../collections/ticket_spec.rb | 283 ++++++++++++++++++ .../collections/ticket_type_spec.rb | 8 +- .../datasource_spec.rb | 25 +- .../ticket_attributes_introspector_spec.rb | 114 +++++++ .../spec/spec_helper.rb | 18 +- 15 files changed, 977 insertions(+), 78 deletions(-) create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/contact_identity.rb create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket.rb create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket/derived_columns.rb create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket/serializer.rb create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/schema/ticket_attributes_introspector.rb create mode 100644 packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_spec.rb create mode 100644 packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/schema/ticket_attributes_introspector_spec.rb diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/base_collection.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/base_collection.rb index 0fc1f4835..9c4731f27 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/base_collection.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/base_collection.rb @@ -68,6 +68,16 @@ def stringify_id(value) value&.to_s end + # Intercom nests its lists twice -- `{"type": "contact.list", "contacts": + # [...]}` -- and answers a null instead of an empty list when there is + # nothing. + def nested_list(container, key) + return [] unless container.is_a?(Hash) + + list = container[key] + list.is_a?(Array) ? list : [] + end + # Intercom dates travel as epoch seconds; Forest reads a Date column as an # ISO8601 string, and a filter carries one too, so comparing the two is the # ordering itself. UTC deliberately: that is where Intercom stores and diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/contact_identity.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/contact_identity.rb new file mode 100644 index 000000000..2bc4fab54 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/contact_identity.rb @@ -0,0 +1,78 @@ +module ForestAdminDatasourceIntercom + module Collections + # The contact of a conversation or of a ticket, denormalized onto the row. + # + # Intercom nests only the ids -- `{"type": "contact.list", "contacts": + # [{"id": "..."}]}` -- so a name and an e-mail cost a read. That read is done + # once per page, for every row at once, and never per row: a page of 25 rows + # is one request, not 25. + # + # It stays a pair of columns rather than a relation because the Contacts + # collection arrives in lot 4, and a relation whose target collection is + # missing is a schema the agent refuses to boot on. + module ContactIdentity + COLUMNS = %w[contact_name contact_email].freeze + + # How many ids one `id in [...]` read carries. A page holds fewer than this + # in practice; the chunk keeps the request bounded if it ever does not. + CONTACT_CHUNK = 100 + + private + + def define_contact_columns + add_column('contact_ids', 'Json') + add_column('contact_count', 'Number') + add_column('contact_name', 'String') + add_column('contact_email', 'String') + end + + # A group conversation, or a ticket opened for several people, has more + # than one contact: the row names the first and counts them, rather than + # presenting one of several as the one. + def contact_columns_for(attrs) + ids = nested_list(attrs['contacts'], 'contacts').filter_map { |contact| stringify_id(contact['id']) } + + { 'contact_ids' => ids, 'contact_count' => ids.size, + # Filled by the bulk read below, and left nil when the projection did + # not ask for them. + 'contact_name' => nil, 'contact_email' => nil } + end + + def first_contact_id(record) + contact = nested_list((record || {})['contacts'], 'contacts').first + contact.is_a?(Hash) ? stringify_id(contact['id']) : nil + end + + def embed_contact_identity(records, rows, projection) + return unless (COLUMNS & projection).any? + + identities = contact_identities(records) + records.each_with_index do |record, index| + identity = identities[first_contact_id(record)] || {} + rows[index]['contact_name'] = identity['name'] if rows[index].key?('contact_name') + rows[index]['contact_email'] = identity['email'] if rows[index].key?('contact_email') + end + end + + # A failure costs the two columns and nothing else: an identity that could + # not be read is not a page that could not be served. + def contact_identities(records) + ids = records.filter_map { |record| first_contact_id(record) }.uniq + return {} if ids.empty? + + ids.each_slice(CONTACT_CHUNK).with_object({}) do |chunk, indexed| + page = client.search_page('contacts/search', per_page: chunk.size, + query: { 'field' => 'id', 'operator' => 'IN', + 'value' => chunk }) + page.records.each { |contact| indexed[contact['id'].to_s] = contact } + end + rescue APIError => e + ForestAdminDatasourceIntercom.logger.warn( + "[forest_admin_datasource_intercom] #{name} could not read the contacts of this page (HTTP " \ + "#{e.status || "-"}); the name and e-mail columns are left empty for it." + ) + {} + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation.rb index 4bc978585..9c0d13fc6 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation.rb @@ -10,7 +10,8 @@ module Collections # customers, and rendering third-party HTML inside Forest is neither safe nor # useful (R10). # Long by line count only: most of it declares the columns, one call each. - class Conversation < CursorCollection # rubocop:disable Metrics/ClassLength + class Conversation < CursorCollection + include ContactIdentity include Conversation::Serializer include Conversation::Timeline @@ -22,10 +23,6 @@ class Conversation < CursorCollection # rubocop:disable Metrics/ClassLength # nil, which reads as "unknown", never as "this conversation is empty". MAX_TIMELINE_READS = 10 - # An `id in [...]` read of contacts is one request per chunk, against the - # whole page rather than per row. - CONTACT_CHUNK = 100 - def initialize(datasource) super(datasource, 'IntercomConversation') end @@ -116,38 +113,6 @@ def define_statistics_columns add_column('part_count', 'Number') end - def embed_contact_identity(records, rows, projection) - return unless (%w[contact_name contact_email] & projection).any? - - identities = contact_identities(records) - records.each_with_index do |record, index| - identity = identities[first_contact_id(record)] || {} - rows[index]['contact_name'] = identity['name'] if rows[index].key?('contact_name') - rows[index]['contact_email'] = identity['email'] if rows[index].key?('contact_email') - end - end - - # One read per chunk of ids for the whole page, never one per row. A - # failure costs the two columns and nothing else: an identity that could - # not be read is not a page that could not be served. - def contact_identities(records) - ids = records.filter_map { |record| first_contact_id(record) }.uniq - return {} if ids.empty? - - ids.each_slice(CONTACT_CHUNK).with_object({}) do |chunk, indexed| - page = client.search_page('contacts/search', per_page: chunk.size, - query: { 'field' => 'id', 'operator' => 'IN', - 'value' => chunk }) - page.records.each { |contact| indexed[contact['id'].to_s] = contact } - end - rescue APIError => e - ForestAdminDatasourceIntercom.logger.warn( - "[forest_admin_datasource_intercom] #{name} could not read the contacts of this page (HTTP " \ - "#{e.status || "-"}); the name and e-mail columns are left empty for it." - ) - {} - end - # A record read through the record endpoint already carries its parts, so # its timeline is free; one read from the listing does not, and pays a # request. Rows past the cap keep the nil the projection put there. diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation/serializer.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation/serializer.rb index 69c9459c4..e241b168c 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation/serializer.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation/serializer.rb @@ -11,7 +11,7 @@ def serialize(conversation) attrs = conversation.is_a?(Hash) ? conversation : {} native(attrs) - .merge(contacts_of(attrs)) + .merge(contact_columns_for(attrs)) .merge(source_of(attrs['source'])) .merge(statistics_of(attrs['statistics'])) end @@ -41,17 +41,6 @@ def native(attrs) } end - # A group conversation carries several contacts. The row names the first - # and counts them, rather than presenting one of several as the one. - def contacts_of(attrs) - ids = nested_list(attrs['contacts'], 'contacts').filter_map { |contact| stringify_id(contact['id']) } - - { 'contact_ids' => ids, 'contact_count' => ids.size, - # Filled by the bulk read of `enrich`, and left nil when the - # projection did not ask for them. - 'contact_name' => nil, 'contact_email' => nil } - end - def source_of(source) attrs = source.is_a?(Hash) ? source : {} author = attrs['author'].is_a?(Hash) ? attrs['author'] : {} @@ -80,21 +69,6 @@ def statistics_of(statistics) 'reopen_count' => attrs['count_reopens'], 'part_count' => attrs['count_conversation_parts'] } end - - # Intercom nests its lists twice -- `{"type": "contact.list", "contacts": - # [...]}` -- and answers a null instead of an empty list when there is - # nothing. - def nested_list(container, key) - return [] unless container.is_a?(Hash) - - list = container[key] - list.is_a?(Array) ? list : [] - end - - def first_contact_id(record) - contact = nested_list((record || {})['contacts'], 'contacts').first - contact.is_a?(Hash) ? stringify_id(contact['id']) : nil - end end end end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb index 8d6251943..f2a252b4a 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb @@ -75,6 +75,14 @@ def enrich(_records, _rows, _projection); end # pays for it by the page. See Ticket. def max_page_size = Client::MAX_PER_PAGE + # One page of the collection. A listing for conversations, a search for + # tickets -- Intercom exposes no `GET /tickets` at all -- so the endpoint + # and its shape belong to the collection, while walking it does not. + def read_page(per_page:, cursor:) + client.list_page(list_endpoint, per_page: [per_page, max_page_size].min, + starting_after: cursor, params: read_params, list_key: list_key) + end + # A column of this tier advertises no filter and no sort, because the # collection can honour neither -- except on the primary key, which is # answered by the record endpoint rather than by a filter. A schema that @@ -152,10 +160,7 @@ def records_by_ids(ids) def listed_records(filter) offset, limit = translate_page(filter&.page) - walker.walk(offset: offset, limit: limit) do |per_page, cursor| - client.list_page(list_endpoint, per_page: [per_page, max_page_size].min, - starting_after: cursor, params: read_params, list_key: list_key) - end + walker.walk(offset: offset, limit: limit) { |per_page, cursor| read_page(per_page: per_page, cursor: cursor) } end # A filter with no page asks for every record it matched; the walker reads @@ -176,7 +181,7 @@ def count_records(filter) refuse_filter!(filter) unless browsing?(filter) - page = client.list_page(list_endpoint, per_page: 1, params: read_params, list_key: list_key) + page = read_page(per_page: 1, cursor: nil) return page.total_count if page.total_count raise UnsupportedOperatorError, diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket.rb new file mode 100644 index 000000000..0239cc0f8 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket.rb @@ -0,0 +1,131 @@ +module ForestAdminDatasourceIntercom + module Collections + # The tickets of the workspace. + # + # Read through `POST /tickets/search`: Intercom exposes no `GET /tickets` at + # all, so even an unfiltered list view goes through the search endpoint with + # a predicate that matches everything. Its records come back under `tickets` + # rather than under the `data` envelope -- measured. + # + # The response carries the whole timeline of every ticket, and there is no + # way to ask it not to: Intercom offers no field selection. Measured, one + # ticket carried 155 parts, so a page of 150 would move some 23 000 part + # objects, customer message bodies included. Two consequences run through + # this class: the page size is bounded far below what the API accepts, and + # everything derived from those parts is free, since they are paid for + # whether or not anything asks. + class Ticket < CursorCollection + include ContactIdentity + include Ticket::Serializer + include Ticket::DerivedColumns + + # Intercom accepts 150. This is not that: it is what keeps one page of + # tickets, timelines included, a response an agent can hold and an operator + # can wait for. Provisional until measured against real response sizes on + # the customer's workspace. + MAX_TICKETS_PER_PAGE = 25 + + # `/tickets/search` demands a query, so a list view sends the least noisy + # predicate that matches everything. Every ticket has a creation date, and + # a bound at the epoch keeps whatever the day-granular truncation does to + # it harmless. + MATCH_EVERY_TICKET = { 'field' => 'created_at', 'operator' => '>', 'value' => '0' }.freeze + + def initialize(datasource, attributes: []) + @attributes = attributes + super(datasource, 'IntercomTicket') + end + + protected + + def list_endpoint = 'tickets/search' + def record_endpoint = 'tickets' + def list_key = 'tickets' + def max_page_size = MAX_TICKETS_PER_PAGE + + # A search rather than a listing, which is the whole reason this hook + # exists. + def read_page(per_page:, cursor:) + client.search_page(list_endpoint, query: MATCH_EVERY_TICKET, list_key: list_key, + per_page: [per_page, max_page_size].min, starting_after: cursor) + end + + def enrich(records, rows, projection) + wanted = Array(projection).map(&:to_s) + + embed_contact_identity(records, rows, wanted) + embed_derived_columns(records, rows, wanted) + end + + private + + def define_schema + add_column('id', 'String', is_primary_key: true) + # The number the support team says out loud, next to the id the API + # answers by. + add_column('ticket_id', 'String') + # `request` / `task` / `tracker` on the wire, never the labels the + # Intercom interface shows -- the same mismatch a filter on it will have + # to respect. + add_column('category', 'String') + add_column('open', 'Boolean') + add_column('is_shared', 'Boolean') + add_column('created_at', 'Date') + add_column('updated_at', 'Date') + add_column('admin_assignee_id', 'String') + add_column('team_assignee_id', 'String') + # The ticket carries its company as an id alone, unlike a conversation + # which carries the whole object: the account name would cost a request + # per row, so it is not offered here. Measured: the id is Intercom's own, + # not the customer's external one, which is what a relation will have to + # target in lot 4. + add_column('company_id', 'String') + define_state_columns + define_type_columns + define_contact_columns + define_derived_columns + add_column('part_count', 'Number') + register_attribute_columns + end + + # The state arrives embedded as a whole object, so its labels cost nothing. + # `IntercomTicketState` remains a collection of its own -- it is the list of + # what a state can be -- but a row does not depend on it to be readable. + def define_state_columns + add_column('state_id', 'String') + add_column('state_category', 'String') + add_column('state_label', 'String') + add_column('state_external_label', 'String') + add_column('previous_state_id', 'String') + end + + def define_type_columns + add_column('ticket_type_id', 'String') + add_column('ticket_type_name', 'String') + end + + # The attribute columns of every ticket type, in union. Read at boot by + # `TicketAttributesIntrospector`; an attribute whose name is already a + # column of this collection is skipped rather than silently overwriting it. + def register_attribute_columns + @attribute_columns = @attributes.reject { |attribute| collides?(attribute) } + @attribute_columns.each { |attribute| add_column(attribute.name, attribute.column_type) } + end + + def collides?(attribute) + return false unless fields.key?(attribute.name) + + ForestAdminDatasourceIntercom.logger.warn( + "[forest_admin_datasource_intercom] #{name} skips the ticket attribute '#{attribute.name}': a native " \ + 'column already carries that name, and overwriting it would show the attribute where the operator ' \ + 'expects the ticket field.' + ) + true + end + + def attribute_columns + @attribute_columns || [] + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket/derived_columns.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket/derived_columns.rb new file mode 100644 index 000000000..41911f280 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket/derived_columns.rb @@ -0,0 +1,127 @@ +module ForestAdminDatasourceIntercom + module Collections + class Ticket < CursorCollection + # The two columns a support queue is read for and that Intercom does not + # carry: when the ticket was closed, and who spoke last. + # + # A ticket has no `statistics` block -- measured against a workspace of 81 + # 142 tickets, confirming the specification -- so neither exists as a + # field. Both are derived from the parts, and the parts arrive with the + # search response whether or not anything asks for them, so both cost + # nothing: this is the one place where deriving a column is cheaper than + # reading one. + # + # Display only, and that is not a temporary state: `/tickets/search` + # filters on neither and ignores a sort without reporting it, so a column + # advertising either would put in the interface what the read cannot + # honour. + module DerivedColumns + # A ticket is not "closed" on Intercom, it enters a state whose category + # is resolved. + RESOLVED = 'resolved'.freeze + + # Matched on the prefix, never on the full `ticket_state_updated_by_admin` + # the sample showed: a workspace running workflows closes tickets through + # other variants of the same event, and a closure nobody can see is worse + # than a column nobody offers. + STATE_CHANGE_PREFIX = 'ticket_state_updated'.freeze + + # A reply to the customer. A `note` is an internal touch, not an answer: + # counting it would name as "last responder" someone who never wrote to + # the person waiting. + REPLY_PART = 'comment'.freeze + + private + + def define_derived_columns + add_column('closed_at', 'Date') + add_column('closed_by_name', 'String') + add_column('last_reply_at', 'Date') + add_column('last_responder_name', 'String') + # `admin` or `contact`: whether the last word came from the team or + # from the customer is what tells a queue who owes the next one. + add_column('last_responder_type', 'String') + end + + def derived_columns_for(attrs) + parts = parts_of(attrs) + closure = last_closure(parts) + reply = last_reply(parts) + + { 'closed_at' => stamp(closure&.dig('created_at')), + 'closed_by_name' => author_of(closure)['name'], + 'last_reply_at' => stamp(reply&.dig('created_at')), + 'last_responder_name' => author_of(reply)['name'], + 'last_responder_type' => author_of(reply)['type'] } + end + + # The hook of the base's `enrich`: nothing to read here, since + # `serialize` already derived everything. What is left is telling the + # operator when a value is missing because the timeline was truncated + # rather than because the event never happened. + def embed_derived_columns(records, _rows, projection) + return unless (%w[closed_at closed_by_name] & projection).any? + + unknown = records.count { |record| closure_unknown?(record) } + warn_unknown_closures(unknown) if unknown.positive? + end + + # A resolved ticket with no closure in hand, on a timeline Intercom + # truncated: the date is *unknown*, not absent. A Date column cannot say + # that, so the log does. + def closure_unknown?(record) + state = record['ticket_state'].is_a?(Hash) ? record['ticket_state'] : {} + return false unless state['category'] == RESOLVED + + last_closure(parts_of(record)).nil? && truncated?(record) + end + + # Intercom keeps the 500 most recent parts of a ticket. Past that, the + # transition that closed it may have fallen out of the window. + def truncated?(record) + total = parts_total(record) + total ? total > parts_of(record).size : false + end + + def last_closure(parts) + closures = parts.select { |part| state_change?(part) && part['ticket_state'] == RESOLVED } + + closures.max_by { |part| part['created_at'].to_i } + end + + # A part can record a transition to the state the ticket was already in + # -- measured -- and that is not an event. + def state_change?(part) + part['part_type'].to_s.start_with?(STATE_CHANGE_PREFIX) && + part['ticket_state'] != part['previous_ticket_state'] + end + + def last_reply(parts) + parts.select { |part| part['part_type'] == REPLY_PART }.max_by { |part| part['created_at'].to_i } + end + + def author_of(part) + author = (part || {})['author'] + author.is_a?(Hash) ? author : {} + end + + def parts_of(record) + nested_list((record || {})['ticket_parts'], 'ticket_parts') + end + + def parts_total(record) + container = (record || {})['ticket_parts'] + container.is_a?(Hash) ? container['total_count'] : nil + end + + def warn_unknown_closures(unknown) + ForestAdminDatasourceIntercom.logger.warn( + "[forest_admin_datasource_intercom] #{name}: #{unknown} resolved ticket(s) of this page show no " \ + 'closure date because Intercom truncated their timeline at 500 parts, not because they were never ' \ + 'closed. The column is unknown for those rows.' + ) + end + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket/serializer.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket/serializer.rb new file mode 100644 index 000000000..0ba95a2a4 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket/serializer.rb @@ -0,0 +1,77 @@ +module ForestAdminDatasourceIntercom + module Collections + class Ticket < CursorCollection + # One Intercom ticket flattened into the row the schema declares. Nothing + # here reads a sub-resource: the state, the type and the attributes all + # travel with the ticket. + module Serializer + protected + + def serialize(ticket) + attrs = ticket.is_a?(Hash) ? ticket : {} + + native(attrs) + .merge(state_of(attrs)) + .merge(type_of(attrs['ticket_type'])) + .merge(contact_columns_for(attrs)) + .merge(attribute_values_of(attrs['ticket_attributes'])) + .merge(derived_columns_for(attrs)) + end + + private + + def native(attrs) + { 'id' => stringify_id(attrs['id']), + 'ticket_id' => stringify_id(attrs['ticket_id']), + 'category' => attrs['category'], + 'open' => attrs['open'], + 'is_shared' => attrs['is_shared'], + 'created_at' => stamp(attrs['created_at']), + 'updated_at' => stamp(attrs['updated_at']), + 'admin_assignee_id' => stringify_id(attrs['admin_assignee_id']), + 'team_assignee_id' => stringify_id(attrs['team_assignee_id']), + 'company_id' => stringify_id(attrs['company_id']), + 'part_count' => parts_total(attrs) } + end + + def state_of(attrs) + state = attrs['ticket_state'].is_a?(Hash) ? attrs['ticket_state'] : {} + + { 'state_id' => stringify_id(state['id']), + 'state_category' => state['category'], + 'state_label' => state['internal_label'], + 'state_external_label' => state['external_label'], + 'previous_state_id' => stringify_id(attrs['previous_ticket_state_id']) } + end + + def type_of(ticket_type) + attrs = ticket_type.is_a?(Hash) ? ticket_type : {} + + { 'ticket_type_id' => stringify_id(attrs['id']), 'ticket_type_name' => attrs['name'] } + end + + # Intercom keys the values by attribute **name**, which is what lets a + # single collection display the union of every type's attributes -- and + # what stops it from filtering on them, since the filter is written by id + # and the id differs from one type to the next. + # + # A ticket of another type simply does not carry the key: the column + # reads as absent rather than as empty. + def attribute_values_of(values) + held = values.is_a?(Hash) ? values : {} + + attribute_columns.to_h { |attribute| [attribute.name, coerce(held[attribute.name], attribute)] } + end + + # A date attribute comes back as epoch seconds like every other Intercom + # date; the rest is handed over as it came. + def coerce(value, attribute) + return nil if value.nil? + return stamp(value) if attribute.column_type == 'Date' && value.is_a?(Numeric) + + value + end + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb index 601022e13..f9ccf242a 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb @@ -33,6 +33,16 @@ def register_collections add_collection(Collections::TicketType.new(self)) add_collection(Collections::TicketState.new(self)) add_collection(Collections::Conversation.new(self)) + # The one boot-time read of the datasource: the attributes a workspace + # defines on its ticket types, which are columns of the Tickets collection + # and cannot be discovered from a ticket payload -- a ticket carries the + # values of its own type only. It degrades to no attribute column rather + # than to a failed boot. + add_collection(Collections::Ticket.new(self, attributes: ticket_attributes)) + end + + def ticket_attributes + Schema::TicketAttributesIntrospector.new(@client).attributes end end end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/schema/ticket_attributes_introspector.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/schema/ticket_attributes_introspector.rb new file mode 100644 index 000000000..bfc1f7499 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/schema/ticket_attributes_introspector.rb @@ -0,0 +1,92 @@ +module ForestAdminDatasourceIntercom + module Schema + # The attributes a workspace defines on its ticket types, read once while the + # datasource is being constructed. + # + # They are declared **per ticket type**, so a single Tickets collection can + # only carry their union -- and that union is for display. Measured on a real + # workspace: two types share the names `_default_title_` and + # `_default_description_` while carrying different attribute ids (14162161 + # against 14162165), and Intercom filters an attribute by id + # (`ticket_attribute.{id}`), never by name. A union column therefore has no + # single id to translate to unless the type of the row is known, which is why + # these ship unfilterable and why filtering on one means a collection per + # ticket type (R7). + # + # The ids are kept per type all the same: they are exactly what the filter + # translation of the next lot will need, and reading them again would cost a + # second boot-time round trip. + class TicketAttributesIntrospector + # Intercom's attribute data types, mapped onto what Forest can render. A + # `list` is a single choice among values the workspace defined, so it reads + # as a string rather than as a Json blob; `files` is a list of attachments + # and has no scalar form at all. + COLUMN_TYPES = { + 'string' => 'String', 'list' => 'String', 'integer' => 'Number', 'decimal' => 'Number', + 'boolean' => 'Boolean', 'datetime' => 'Date', 'date' => 'Date', 'files' => 'Json' + }.freeze + + DEFAULT_COLUMN_TYPE = 'String'.freeze + + Attribute = Struct.new(:name, :column_type, :data_type, :ids_by_ticket_type, keyword_init: true) + + def initialize(client) + @client = client + end + + # The union, one entry per attribute name. Degrades to nothing rather than + # to a failure: a token without the ticket-types permission costs the + # attribute columns, never the boot of the agent. + def attributes + @attributes ||= build + rescue APIError => e + ForestAdminDatasourceIntercom.logger.warn( + "[forest_admin_datasource_intercom] could not read the ticket types (HTTP #{e.status || "-"}); " \ + 'the Tickets collection boots without its attribute columns.' + ) + @attributes = [] + end + + private + + def build + # Read on the boot connection: this happens while Rails is starting, and + # a slow Intercom must not turn that into minutes the operator sits + # through. + @client.fetch_all('ticket_types', boot: true) + .each_with_object({}) { |ticket_type, union| collect(ticket_type, union) } + .values + end + + def collect(ticket_type, union) + type_id = ticket_type['id'].to_s + definitions(ticket_type).each do |definition| + name = definition['name'].to_s + # An archived attribute is not offered any more, and a nameless one has + # nothing to be a column of. + next if name.empty? || definition['archived'] + + entry = union[name] ||= Attribute.new(name: name, column_type: column_type_for(definition), + data_type: definition['data_type'], ids_by_ticket_type: {}) + entry.ids_by_ticket_type[type_id] = definition['id'].to_s + end + end + + def definitions(ticket_type) + return [] unless ticket_type.is_a?(Hash) + + container = ticket_type['ticket_type_attributes'] + return [] unless container.is_a?(Hash) + + list = container['data'] + list.is_a?(Array) ? list : [] + end + + # An unknown data type reads as a string rather than being dropped: showing + # the value Intercom sent beats hiding a column because its type is new. + def column_type_for(definition) + COLUMN_TYPES.fetch(definition['data_type'].to_s, DEFAULT_COLUMN_TYPE) + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_spec.rb new file mode 100644 index 000000000..aef9619da --- /dev/null +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_spec.rb @@ -0,0 +1,283 @@ +module ForestAdminDatasourceIntercom + RSpec.describe Collections::Ticket do + subject(:collection) { described_class.new(datasource, attributes: attributes) } + + let(:datasource) { Datasource.new(access_token: 's3cr3t', rate_limiter: nil) } + # Not read off the datasource: that would build it, and boot the ticket-type + # introspection before the stub of it exists. + let(:base) { Configuration::REGION_HOSTS[:us] } + let(:operators) { ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators } + let(:attributes) do + [Schema::TicketAttributesIntrospector::Attribute.new(name: '_default_title_', column_type: 'String', + data_type: 'string', + ids_by_ticket_type: { '1' => '14162161' }), + Schema::TicketAttributesIntrospector::Attribute.new(name: 'Due', column_type: 'Date', data_type: 'datetime', + ids_by_ticket_type: { '2' => '9002' })] + end + + def json(payload, status = 200) + { status: status, body: payload.to_json, headers: { 'Content-Type' => 'application/json' } } + end + + def filter(condition_tree: nil, page: nil) + ForestAdminDatasourceToolkit::Components::Query::Filter.new(condition_tree: condition_tree, page: page) + end + + def leaf(field, operator, value = nil) + ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf + .new(field, operator, value) + end + + # Hand-written from the shape measured on a real workspace: the state comes + # embedded, the company as a bare id, and the parts ride along. + def ticket(id, overrides = {}) + { 'type' => 'ticket', 'id' => id, 'ticket_id' => "1#{id}", 'category' => 'request', + 'open' => true, 'is_shared' => false, 'created_at' => 1_700_000_000, 'updated_at' => 1_700_003_600, + 'admin_assignee_id' => 493_881, 'team_assignee_id' => 0, + 'company_id' => '696dd52099f73812610d9c7b', + 'ticket_state' => { 'type' => 'ticket_state', 'id' => '19', 'category' => 'in_progress', + 'internal_label' => 'En cours Tech', 'external_label' => 'Investigation en cours' }, + 'previous_ticket_state_id' => '14', + 'ticket_type' => { 'type' => 'ticket_type', 'id' => '1', 'name' => 'Bug' }, + 'contacts' => { 'type' => 'contact.list', 'contacts' => [{ 'type' => 'contact', 'id' => 'c1' }] }, + 'ticket_attributes' => { '_default_title_' => 'Facture manquante' }, + 'ticket_parts' => { 'type' => 'ticket_part.list', 'total_count' => 0, 'ticket_parts' => [] } } + .merge(overrides) + end + + def parts(*entries, total: nil) + { 'ticket_parts' => { 'type' => 'ticket_part.list', 'total_count' => total || entries.size, + 'ticket_parts' => entries } } + end + + def state_change(to, from: 'in_progress', at: 1_700_002_000, by: 'Alice', part_type: nil) + { 'type' => 'ticket_part', 'id' => "s#{at}", 'part_type' => part_type || 'ticket_state_updated_by_admin', + 'ticket_state' => to, 'previous_ticket_state' => from, 'created_at' => at, + 'author' => { 'type' => 'admin', 'id' => '1', 'name' => by, 'email' => 'alice@acme.test' } } + end + + def comment(at:, by: 'Alice', type: 'admin', part_type: 'comment') + { 'type' => 'ticket_part', 'id' => "c#{at}", 'part_type' => part_type, 'body' => 'Je regarde.', + 'created_at' => at, 'author' => { 'type' => type, 'id' => '1', 'name' => by } } + end + + def stub_search(*records, total: nil, body: nil) + answer = { 'type' => 'ticket.list', 'tickets' => records, 'total_count' => total || records.size, + 'pages' => { 'type' => 'pages', 'page' => 1 } } + + request = stub_request(:post, "#{base}/tickets/search") + request = request.with(body: hash_including(body)) if body + request.to_return(json(answer)) + end + + def rows(projection = nil, **options) + collection.list(nil, filter(**options), projection) + end + + describe 'schema' do + it 'is named IntercomTicket' do + expect(collection.name).to eq('IntercomTicket') + end + + # The state travels embedded, so its labels cost nothing and the row does + # not depend on IntercomTicketState to be readable. + it 'flattens the embedded state into its labels' do + expect(collection.fields.keys) + .to include('state_id', 'state_category', 'state_label', 'state_external_label', 'previous_state_id') + end + + it 'carries the attributes of every ticket type in union' do + expect(collection.fields.keys).to include('_default_title_', 'Due') + expect(collection.fields['Due'].column_type).to eq('Date') + end + + # `/tickets/search` filters none of these and ignores a sort without + # saying so, so nothing but the primary key may advertise anything. + it 'declares every column unfilterable and unsortable, except the primary key' do + others = collection.fields.except('id') + + expect(others.values.map(&:filter_operators).flatten.uniq).to be_empty + expect(others.values.map(&:is_sortable).uniq).to eq([false]) + end + + # An attribute overwriting a native column would show the attribute where + # the operator expects the ticket field. + it 'skips an attribute whose name a native column already carries' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + clashing = Schema::TicketAttributesIntrospector::Attribute.new(name: 'category', column_type: 'String', + data_type: 'string', ids_by_ticket_type: {}) + + collection = described_class.new(datasource, attributes: [clashing]) + + expect(collection.fields['category'].column_type).to eq('String') + expect(ForestAdminDatasourceIntercom.logger).to have_received(:warn).with(/skips the ticket attribute/) + end + end + + describe '#list' do + # There is no GET /tickets at all: even an unfiltered list view goes + # through the search endpoint with a predicate that matches everything. + it 'reads the search endpoint with a predicate matching every ticket' do + stub_search(ticket('1')) + + rows(%w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/tickets/search") + .with(body: hash_including('query' => { 'field' => 'created_at', 'operator' => '>', 'value' => '0' })) + end + + # Not the 150 the API accepts: a page carries every ticket's whole + # timeline, and there is no way to ask Intercom for less. + it 'asks for far fewer tickets than the API would allow' do + stub_search(ticket('1')) + + rows(%w[id]) + + pagination = hash_including('per_page' => described_class::MAX_TICKETS_PER_PAGE) + + expect(WebMock).to have_requested(:post, "#{base}/tickets/search") + .with(body: hash_including('pagination' => pagination)) + end + + it 'reads the records under the tickets key' do + stub_search(ticket('1'), ticket('2')) + + expect(rows(%w[id]).map { |row| row['id'] }).to eq(%w[1 2]) + end + + it 'flattens the ticket into the row the schema declares' do + stub_search(ticket('1')) + + expect(rows.first) + .to include('id' => '1', 'ticket_id' => '11', 'category' => 'request', 'open' => true, + 'state_id' => '19', 'state_category' => 'in_progress', 'state_label' => 'En cours Tech', + 'previous_state_id' => '14', 'ticket_type_name' => 'Bug', + 'company_id' => '696dd52099f73812610d9c7b', 'admin_assignee_id' => '493881') + end + + # Intercom keys the values by attribute name, which is what lets one + # collection display the union -- and what stops it from filtering on them. + it 'reads an attribute value by its name' do + stub_search(ticket('1')) + + expect(rows.first['_default_title_']).to eq('Facture manquante') + end + + it 'leaves an attribute of another ticket type absent rather than empty' do + stub_search(ticket('1')) + + expect(rows.first['Due']).to be_nil + end + + it 'reads a date attribute as ISO8601 like every other Intercom date' do + stub_search(ticket('1', 'ticket_attributes' => { 'Due' => 1_700_000_000 })) + + expect(rows.first['Due']).to eq('2023-11-14T22:13:20Z') + end + + # A record detail goes to its own endpoint, which is not the search one. + it 'reads one ticket through the record endpoint' do + stub_request(:get, "#{base}/tickets/1").to_return(json(ticket('1'))) + + expect(rows(%w[id], condition_tree: leaf('id', operators::EQUAL, '1')).map { |row| row['id'] }).to eq(%w[1]) + end + + it 'refuses a condition it cannot honour' do + expect { rows(%w[id], condition_tree: leaf('state_category', operators::EQUAL, 'resolved')) } + .to raise_error(UnsupportedOperatorError, /cannot answer a condition/) + end + end + + describe '#aggregate' do + it 'counts through the total_count of the search, exactly' do + stub_search(ticket('1'), total: 81_142) + aggregation = ForestAdminDatasourceToolkit::Components::Query::Aggregation.new(operation: 'Count') + + expect(collection.aggregate(nil, filter, aggregation)).to eq([{ 'group' => {}, 'value' => 81_142 }]) + end + end + + describe 'the derived columns' do + # A ticket has no statistics block -- measured on 81 142 tickets -- so the + # closure date exists nowhere but in the parts, which ride along anyway. + it 'reads the closure from the last transition into a resolved state' do + stub_search(ticket('1', **parts(state_change('in_progress', from: 'submitted', at: 1_700_001_000), + state_change('resolved', at: 1_700_002_000, by: 'Alice')))) + + expect(rows.first).to include('closed_at' => '2023-11-14T22:46:40Z', 'closed_by_name' => 'Alice') + end + + # This workspace runs workflows: a closure done by automation carries + # another variant of the same event, and matching the admin one in full + # would make it invisible. + it 'reads a closure whatever the variant of the state-change event' do + stub_search(ticket('1', **parts(state_change('resolved', at: 1_700_002_000, + part_type: 'ticket_state_updated_by_workflow')))) + + expect(rows.first['closed_at']).to eq('2023-11-14T22:46:40Z') + end + + # Measured: a part can record a transition to the state the ticket was + # already in, and that is not an event. + it 'ignores a transition that changed nothing' do + stub_search(ticket('1', **parts(state_change('resolved', from: 'resolved', at: 1_700_002_000)))) + + expect(rows.first['closed_at']).to be_nil + end + + it 'keeps the last closure of a ticket that was reopened' do + stub_search(ticket('1', **parts(state_change('resolved', at: 1_700_001_000), + state_change('in_progress', from: 'resolved', at: 1_700_001_500), + state_change('resolved', at: 1_700_002_000)))) + + expect(rows.first['closed_at']).to eq('2023-11-14T22:46:40Z') + end + + it 'names the last responder and which side they are on' do + stub_search(ticket('1', **parts(comment(at: 1_700_001_000, by: 'Alice'), + comment(at: 1_700_002_000, by: 'Camille', type: 'contact')))) + + expect(rows.first) + .to include('last_reply_at' => '2023-11-14T22:46:40Z', 'last_responder_name' => 'Camille', + 'last_responder_type' => 'contact') + end + + # An internal note is a touch, not an answer: it would name as last + # responder someone who never wrote to the person waiting. + it 'ignores an internal note' do + stub_search(ticket('1', **parts(comment(at: 1_700_001_000, by: 'Alice'), + comment(at: 1_700_002_000, by: 'Bob', part_type: 'note')))) + + expect(rows.first['last_responder_name']).to eq('Alice') + end + + it 'leaves both columns empty on a ticket nothing happened to' do + stub_search(ticket('1')) + + expect(rows.first).to include('closed_at' => nil, 'last_responder_name' => nil) + end + + # Intercom keeps the 500 most recent parts. A resolved ticket whose + # transition fell out of that window has an *unknown* closure date, not an + # absent one -- a Date column cannot say it, so the log does. + it 'reports a resolved ticket whose timeline was truncated' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + resolved = { 'ticket_state' => { 'id' => '20', 'category' => 'resolved', 'internal_label' => 'Resolu' } } + stub_search(ticket('1', **resolved, **parts(comment(at: 1_700_001_000), total: 500))) + + rows(%w[id closed_at]) + + expect(ForestAdminDatasourceIntercom.logger).to have_received(:warn).with(/truncated their timeline/) + end + + it 'stays quiet when the closure is simply absent from a complete timeline' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + stub_search(ticket('1', **parts(comment(at: 1_700_001_000)))) + + rows(%w[id closed_at]) + + expect(ForestAdminDatasourceIntercom.logger).not_to have_received(:warn) + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_type_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_type_spec.rb index f34ed10ba..38d1f490d 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_type_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_type_spec.rb @@ -9,7 +9,7 @@ def filter ForestAdminDatasourceToolkit::Components::Query::Filter.new end - def stub_ticket_types(*types) + def stub_types(*types) stub_request(:get, "#{base}/ticket_types") .to_return(status: 200, body: { 'type' => 'list', 'data' => types }.to_json, headers: { 'Content-Type' => 'application/json' }) @@ -25,8 +25,8 @@ def stub_ticket_types(*types) # This endpoint uses the `data` envelope, unlike /admins and /teams. it 'reads the endpoint through the data envelope' do - stub_ticket_types('type' => 'ticket_type', 'id' => '1', 'name' => 'Bug', 'description' => 'A bug', - 'category' => 'request', 'icon' => '🐛', 'archived' => false) + stub_types('type' => 'ticket_type', 'id' => '1', 'name' => 'Bug', 'description' => 'A bug', + 'category' => 'request', 'icon' => '🐛', 'archived' => false) expect(collection.list(nil, filter, nil)) .to eq([{ 'id' => '1', 'name' => 'Bug', 'description' => 'A bug', 'category' => 'request', @@ -37,7 +37,7 @@ def stub_ticket_types(*types) # to build its columns -- an attribute of the same name carries a different # id from one type to the next -- and they are meaningless as a column. it 'leaves the nested attribute definitions out of the schema' do - stub_ticket_types('id' => '1', 'ticket_type_attributes' => { 'type' => 'list', 'data' => [{ 'id' => '9' }] }) + stub_types('id' => '1', 'ticket_type_attributes' => { 'type' => 'list', 'data' => [{ 'id' => '9' }] }) expect(collection.list(nil, filter, nil).first.keys).not_to include('ticket_type_attributes') end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb index 97a95c6fd..c5387e059 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb @@ -11,17 +11,34 @@ module ForestAdminDatasourceIntercom # next. it 'publishes the collections of the lot' do expect(datasource.collections.keys) - .to eq(%w[IntercomAdmin IntercomTeam IntercomTicketType IntercomTicketState IntercomConversation]) + .to eq(%w[IntercomAdmin IntercomTeam IntercomTicketType IntercomTicketState IntercomConversation + IntercomTicket]) end - it 'reaches Intercom only when a collection is listed, never while booting' do + # The one read a boot performs: the attributes a workspace declares on its + # ticket types are columns of the Tickets collection, and a ticket payload + # carries the values of its own type only, so they cannot be discovered from + # the records. + it 'introspects the ticket-type attributes while registering, and reads nothing else' do datasource - expect(WebMock).not_to have_requested(:get, /intercom/) + expect(WebMock).to have_requested(:get, /ticket_types/).once + expect(WebMock).not_to have_requested(:get, /conversations|admins|teams/) + end + + # A token without that permission costs the attribute columns, never the + # agent. + it 'boots without the attribute columns when the introspection is refused' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + stub_request(:get, /ticket_types/).to_return(status: 403, body: '{}', + headers: { 'Content-Type' => 'application/json' }) + + expect(datasource.get_collection('IntercomTicket').fields.keys).not_to include('_default_title_') end it 'configures a client from the options it is handed' do - configured = described_class.new(access_token: 's3cr3t', region: :eu) + stub_ticket_types(base: 'https://api.eu.intercom.io') + configured = described_class.new(access_token: 's3cr3t', region: :eu, rate_limiter: nil) expect(configured.configuration.url).to eq('https://api.eu.intercom.io') expect(configured.client).to be_a(Client) diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/schema/ticket_attributes_introspector_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/schema/ticket_attributes_introspector_spec.rb new file mode 100644 index 000000000..cb862a5eb --- /dev/null +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/schema/ticket_attributes_introspector_spec.rb @@ -0,0 +1,114 @@ +module ForestAdminDatasourceIntercom + module Schema + RSpec.describe TicketAttributesIntrospector do + subject(:introspector) { described_class.new(Client.new(configuration)) } + + let(:configuration) { Configuration.new(access_token: 's3cr3t', rate_limiter: nil) } + let(:base) { configuration.url } + + def json(payload, status = 200) + { status: status, body: payload.to_json, headers: { 'Content-Type' => 'application/json' } } + end + + def attribute(name, id, data_type: 'string', archived: false) + { 'id' => id, 'name' => name, 'data_type' => data_type, 'archived' => archived } + end + + def ticket_type(id, name, *attributes) + { 'type' => 'ticket_type', 'id' => id, 'name' => name, + 'ticket_type_attributes' => { 'type' => 'list', 'data' => attributes } } + end + + def stub_types(*types) + stub_request(:get, "#{base}/ticket_types").to_return(json('type' => 'list', 'data' => types)) + end + + it 'reads one entry per attribute name' do + stub_types(ticket_type('1', 'Bug', attribute('Severity', '9001')), + ticket_type('2', 'Task', attribute('Due', '9002', data_type: 'datetime'))) + + expect(introspector.attributes.map(&:name)).to contain_exactly('Severity', 'Due') + end + + # Measured: two ticket types share the names `_default_title_` and + # `_default_description_` while carrying different attribute ids. A union + # column has no single id to be filtered by, which is why these ship + # unfilterable -- and why the ids are kept, since that is what a filter + # per ticket type will need. + it 'keeps the id each ticket type gives the same attribute name' do + stub_types(ticket_type('1', 'Bug', attribute('_default_title_', '14162161')), + ticket_type('2', 'Task', attribute('_default_title_', '14162165'))) + + expect(introspector.attributes.map(&:ids_by_ticket_type)) + .to eq([{ '1' => '14162161', '2' => '14162165' }]) + end + + it 'maps the Intercom data types onto what Forest renders' do + stub_types(ticket_type('1', 'Bug', attribute('n', '1', data_type: 'integer'), + attribute('d', '2', data_type: 'decimal'), + attribute('b', '3', data_type: 'boolean'), + attribute('t', '4', data_type: 'datetime'), + attribute('l', '5', data_type: 'list'), + attribute('f', '6', data_type: 'files'))) + + expect(introspector.attributes.map(&:column_type)).to eq(%w[Number Number Boolean Date String Json]) + end + + # Showing the value Intercom sent beats hiding a column because its type + # is one this datasource has not met yet. + it 'reads an unknown data type as a string rather than dropping the column' do + stub_types(ticket_type('1', 'Bug', attribute('x', '1', data_type: 'quantum'))) + + expect(introspector.attributes.map(&:column_type)).to eq(%w[String]) + end + + it 'leaves out an archived attribute, which is not offered any more' do + stub_types(ticket_type('1', 'Bug', attribute('Gone', '1', archived: true), attribute('Here', '2'))) + + expect(introspector.attributes.map(&:name)).to eq(%w[Here]) + end + + it 'leaves out an attribute with no name to be a column of' do + stub_types(ticket_type('1', 'Bug', attribute('', '1'))) + + expect(introspector.attributes).to be_empty + end + + it 'reads a ticket type declaring no attribute as declaring none' do + stub_types({ 'id' => '1', 'name' => 'Bug' }) + + expect(introspector.attributes).to be_empty + end + + # It runs while Rails is starting, so it waits far less than a request + # that already has a page on screen. + it 'reads through the boot connection' do + slow = Configuration.new(access_token: 's3cr3t', rate_limiter: nil, boot_timeout: 2, timeout: 30) + client = Client.new(slow) + stub_types + + described_class.new(client).attributes + + expect(client.send(:boot_connection).options.timeout).to eq(2) + end + + it 'reads once and remembers, a schema being built once' do + stub_types(ticket_type('1', 'Bug', attribute('Severity', '9001'))) + + 2.times { introspector.attributes } + + expect(WebMock).to have_requested(:get, "#{base}/ticket_types").once + end + + # A token without the ticket-types permission costs the attribute columns, + # never the boot of the agent. + it 'degrades to no attribute when the read is refused' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + stub_request(:get, "#{base}/ticket_types").to_return(json({ 'errors' => [{ 'code' => 'forbidden' }] }, 403)) + + expect(introspector.attributes).to eq([]) + expect(ForestAdminDatasourceIntercom.logger).to have_received(:warn).with(/boots without its attribute/) + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/spec/spec_helper.rb b/packages/forest_admin_datasource_intercom/spec/spec_helper.rb index 2c1c345be..056ee440e 100644 --- a/packages/forest_admin_datasource_intercom/spec/spec_helper.rb +++ b/packages/forest_admin_datasource_intercom/spec/spec_helper.rb @@ -27,7 +27,20 @@ # and a fixture is read by everyone who clones the repo. WebMock.disable_net_connect!(allow_localhost: true) +# A datasource introspects the ticket-type attributes while it registers its +# collections, so every spec building one issues that read. The base url is not +# taken from the datasource on purpose: reading it would build the datasource, +# and boot the very read this stubs. +module IntercomBootStubs + def stub_ticket_types(*types, base: ForestAdminDatasourceIntercom::Configuration::REGION_HOSTS[:us]) + stub_request(:get, "#{base}/ticket_types") + .to_return(status: 200, body: { 'type' => 'list', 'data' => types }.to_json, + headers: { 'Content-Type' => 'application/json' }) + end +end + RSpec.configure do |config| + config.include IntercomBootStubs config.expect_with :rspec do |c| c.syntax = :expect end @@ -39,5 +52,8 @@ config.order = :random Kernel.srand config.seed - config.before { WebMock.reset! } + config.before do + WebMock.reset! + stub_ticket_types + end end From 974badb0f8546c0fa32bc15dfb3f32739d82b418 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Tue, 1 Sep 2026 08:45:45 +0200 Subject: [PATCH 7/9] docs(intercom): document the datasource Seventh and last step of lot 1 (PRD-1112): the README, and the one hardening the review of the logs turned up. A response that fails to parse used to travel into the error message, and a JSON parser opens its message with the characters it choked on. On a 4xx those are Intercom's error text, which is what an operator needs; on a 200 they are the payload -- a conversation body, most of the time. The failure is now named rather than quoted, at both levels: the Faraday parsing error, and a parser raising on its own, which would otherwise have reached the catch-all whose message is the exception's. The rest of the log review came back clean -- operations, counts, statuses and Intercom request ids, never content. The README documents what this datasource refuses and why, since that is the part an operator meets first: no offset pagination, a sort accepted and ignored, no aggregate endpoint, `per_page` refused past 150 and bounded at 25 for tickets, no `GET /tickets` at all, and an envelope key that is not always `data`. Then the two tiers and why they behave differently, the derived ticket columns with the 500-part ceiling that makes a closure date unknown rather than absent, the rate-limit windows, the privacy rules, and the single read a boot performs. Every figure in it was checked against the constants rather than remembered. Co-Authored-By: Claude Opus 5 (1M context) --- .../README.md | 235 ++++++++++++++++++ .../client.rb | 49 +++- .../client_spec.rb | 28 +++ 3 files changed, 306 insertions(+), 6 deletions(-) create mode 100644 packages/forest_admin_datasource_intercom/README.md diff --git a/packages/forest_admin_datasource_intercom/README.md b/packages/forest_admin_datasource_intercom/README.md new file mode 100644 index 000000000..3505508e0 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/README.md @@ -0,0 +1,235 @@ +# Forest — Intercom datasource + +Surface [Intercom](https://www.intercom.com) conversations, tickets, teammates, teams, ticket types +and ticket states as Forest collections. + +This is **lot 1: read only**. Rows, record details, exact counts and the conversation thread work; +server-side filtering, writes, business actions, contacts and companies arrive in the lots after it +— see "What is not here yet". + +## Installation + +```ruby +# Gemfile +gem 'forest_admin_datasource_intercom' +``` + +## Usage + +```ruby +# app/lib/forest_admin_rails/create_agent.rb +ForestAdminAgent::Builder::AgentFactory.instance.add_datasource( + ForestAdminDatasourceIntercom::Datasource.new( + access_token: ENV['INTERCOM_ACCESS_TOKEN'], + region: :eu # :us (default), :eu or :au + ) +) +``` + +The token is the access token of a private app, created in Intercom's Developer Hub under +*Configure › Authentication*. OAuth is out of scope: it belongs to a control plane distributing a +connector, not to an agent reading one workspace. + +`Client#me` is the health check — it returns the admin the token belongs to, and is the one call +that verifies the pinned API version was honoured. + +### Configuration + +| Option | Default | What it is for | +| --- | --- | --- | +| `access_token` | — | Required. The private app's bearer token. | +| `region` | `:us` | `:us`, `:eu`, `:au`. A workspace answers in its own region only. | +| `base_url` | from `region` | Wins over `region`. For an egress proxy or a mock server. | +| `api_version` | `'2.16'` | Sent as `Intercom-Version` on every request. | +| `open_timeout` / `timeout` | `5` / `30` | A request that already has a page on screen. | +| `boot_open_timeout` / `boot_timeout` | `3` / `10` | The one read performed while the agent starts. | +| `retry_policy` | `RetryPolicy.new` | Statuses, verbs and backoff. | +| `boot_retry_policy` | `RetryPolicy.boot` | One quick retry; gives up rather than waiting a 429 out. | +| `rate_limiter` | `RateLimiter.new` | `nil` takes the pacing out of the stack. | + +**Pin the region explicitly.** `api.intercom.io` does route to the right one, but a workspace under +GDPR wants its requests reaching the European host and nothing else. + +**The version is pinned on purpose.** Without the header a request follows the workspace's own +default version, which an operator can change on Intercom's side — and the payloads change shape +underneath. Intercom echoes the version it served, so `me` compares the two and logs a warning when +the pin was not honoured, rather than raising: running against a version we did not ask for still +beats not running. + +### Token permissions + +A read-only token is enough, and is what to recommend for this lot. A permission the token lacks +costs **columns or a collection, never the boot of the agent**: the ticket-type introspection +degrades to no attribute column, and a collection whose endpoint answers 403 fails its own page. + +## Collections + +| Collection | Endpoint | Paginated | Countable | +| --- | --- | --- | --- | +| `IntercomConversation` | `GET /conversations`, `GET /conversations/{id}` | cursor | yes, exactly | +| `IntercomTicket` | `POST /tickets/search`, `GET /tickets/{id}` | cursor | yes, exactly | +| `IntercomAdmin` | `GET /admins` | read whole | yes, exactly | +| `IntercomTeam` | `GET /teams` | read whole | yes, exactly | +| `IntercomTicketType` | `GET /ticket_types` | read whole | yes, exactly | +| `IntercomTicketState` | `GET /ticket_states` | read whole | yes, exactly | + +Two tiers, and they behave differently on purpose. + +**Read whole** — admins, teams, ticket types, ticket states. Their endpoints answer in one response, +so filtering, sorting, paging and counting them in memory is *exact*: the records in hand are every +record Intercom holds. These are the only collections that can be filtered, sorted and grouped in +this lot, and the only ones a chart may group by. The cost is bandwidth, not correctness. + +**Cursor** — conversations and tickets. What is in hand is a page of something far larger, so +nothing is filtered or sorted in memory. Three routes and no fourth: no condition walks the listing, +`id equals X` reads the record through its own endpoint, and **anything else is refused** with a +message naming the lot that will answer it. + +## What the API cannot do, and what this does about it + +Where Forest asks for something Intercom has no equivalent for, this datasource **refuses with a +message naming the reason** rather than answering something that looks right and is not. Those +arrive as a 400 carrying the text. + +- **No offset pagination.** Intercom hands out the page after a cursor and documents that jumping to + page N is unsupported, so reaching page 20 costs 20 sequential requests. The walk is capped at 50 + pages / 7 500 records and every truncation is logged, naming the window it stopped in. +- **Duplicates on a moving dataset.** Intercom documents that records modified between two paginated + requests can be served twice; the walk deduplicates by id. The missed counterpart is inherent to + cursor pagination and cannot be repaired — it is documented rather than papered over. +- **A sort is accepted and ignored.** Measured: `sort` on these endpoints raises nothing and changes + nothing. Since the lack of support is undetectable at runtime, no column is declared sortable and + a requested order is reported in the log. The rows come back in the order the API imposes. +- **No aggregate endpoint.** Counting is free and exact — `total_count` counts what the query names, + not what a page held — so the record counter is one request. Anything beyond a count is refused on + the cursor collections: grouping over the pages a walk collected would look exact while answering + a fraction. +- **`per_page` is refused past 150**, with `invalid_per_page` and no silent downgrade, so the page + size is bounded before the request leaves. Tickets are bounded far lower still: **25**, because + the search response carries the whole timeline of every ticket and Intercom offers no field + selection. Provisional, pending measurement against real response sizes. +- **No `GET /tickets` at all.** Even an unfiltered ticket list goes through `POST /tickets/search` + with a predicate matching everything. +- **The envelope key is not always `data`.** Measured: `/tickets/search` answers under `tickets`, + `/admins` under `admins`, `/teams` under `teams`. A response carrying neither the expected key nor + `data` is refused rather than read as an empty page. + +## Conversations + +The row carries what a queue is read for: state, priority, assignee and team ids, the company, the +tags, and the lifecycle Intercom keeps in `statistics` — `closed_at`, `closed_by_id`, +`first_contact_reply_at`, `last_contact_reply_at`, `last_admin_reply_at`, `reopen_count`. + +**The timeline opens on `source`, not on the parts.** The message that started the conversation +lives there; a thread built from the parts alone opens on the first reply and loses what the +customer actually asked. Every entry keeps its `part_type` — an assignment, a note and a reply are +different events. + +Intercom returns the parts **only when retrieving a single conversation**, so: + +- a record detail gets its timeline for free; +- a list view asking for the `timeline` column pays one request per row, bounded to 10. The rows past + that keep a `nil`, which reads as *unknown* — never as an empty thread. + +A conversation is capped at its **500 most recent parts**; a very long thread is therefore partial, +and says so nowhere but here. + +Contact name and e-mail are denormalized onto the row by **one bulk read per page**, not one per +row, and only when the projection names them. A failure there costs those two columns, not the page. + +## Tickets + +A ticket carries **no `statistics` block** — measured against a workspace of 81 142 tickets — so +neither a closure date nor a last responder exists as a field. Both are derived from the parts, +which ride along in the search response whether or not anything asks for them, and therefore cost +nothing: + +| Column | Derived from | +| --- | --- | +| `closed_at`, `closed_by_name` | the last transition into a state of category `resolved` | +| `last_reply_at`, `last_responder_name`, `last_responder_type` | the last `comment` part | + +Four things to know about them: + +- a ticket is not "closed" on Intercom, it enters a **resolved** state; +- the state-change event is matched on its **prefix**, not on `ticket_state_updated_by_admin`: a + workspace running workflows closes tickets through other variants, and an invisible closure is + worse than an absent column; +- a transition whose target equals the previous state is ignored — measured, they exist; +- **a resolved ticket showing no closure date may have been closed all the same**: past the 500-part + ceiling the transition falls out of the window. That case is detected and logged, since a Date + column cannot say "unknown". + +Both columns are **display only**, and not temporarily: `/tickets/search` filters on neither and +ignores a sort, so neither advertises an operator. + +The attributes a workspace declares on its ticket types are introspected once at boot and published +as the **union** of every type's, keyed by name the way the payload is. Filtering one is a different +matter: Intercom filters an attribute by id (`ticket_attribute.{id}`), and the same name carries a +different id from one ticket type to the next — measured, `_default_title_` is `14162161` on one +type and `14162165` on another. A union column has no single id to translate to, so filtering on a +ticket attribute means one collection per ticket type. The ids are kept per type for the lot that +will need them. + +## Rate limits + +Intercom meters the app and, above it, the whole workspace — 25 000 requests a minute shared with +every other private app the customer runs — and allocates that budget in **10-second windows**: the +measured `x-ratelimit-limit` is 1667, not 10 000. A burst therefore takes a 429 while the minute's +budget is barely touched, which is why what matters is the instantaneous rate. + +The limiter is driven by the headers Intercom returns on every response rather than by a table: it +waits out the reset when the window is spent, and counts its own in-flight requests down so several +of them do not go out on the same stale figure. A reset further out than a window is a clock +disagreement rather than a window emptying — the request goes through and the log says so, once per +window. + +It sits **in front of** the 429 retry, not instead of it: the retry remains the defence against the +part of the workspace budget spent by traffic this process cannot see. Pass `rate_limiter: nil` to +meter on your own side instead. + +## Privacy + +The body of a conversation is raw personal data, and this datasource is built on that assumption. + +- **Nothing logs a body.** Logs carry the operation, the counts and Intercom's request id — never + content. A response that fails to parse is reported by name, never quoted: a JSON parser opens its + message with the characters it choked on, and on a 200 those are the payload. +- **`display_as=plaintext` on every conversation read.** The bodies are HTML written by end + customers; rendering third-party HTML inside Forest is neither safe nor useful. +- **The regional host is configurable** so a workspace's data stays in its region. +- Ticket list pages carry customer message bodies whether or not anything asks for them — Intercom + offers no field selection. Restrict the body columns with Forest's field-level permissions where + that matters. + +## Boot-time introspection + +Constructing the datasource performs exactly **one** read: `GET /ticket_types`, for the attribute +columns of `IntercomTicket`. It runs on the boot connection — short timeouts, one quick retry — so a +slow Intercom cannot turn a Rails boot into minutes the operator sits through, and it degrades to no +attribute column rather than to a failed boot. + +Everything else is read when a collection is listed, so an agent boots whatever Intercom is doing. + +## What is not here yet + +| Lot | What it brings | +| --- | --- | +| 2 | Filter translation into Intercom's search DSL, free-text search, per-endpoint operator tables, UTC date bounds | +| 3 | Writes and business actions: reply, close, snooze, reopen, assign, tag, convert | +| 4 | Contacts and companies, and the relations promoted from today's denormalized columns | +| 5 | Notes, tags, segments | +| 6 | Bounded group-by and the reporting export | + +## Development + +```bash +cd packages/forest_admin_datasource_intercom +BUNDLE_GEMFILE=Gemfile-test bundle install +BUNDLE_GEMFILE=Gemfile-test bundle exec rspec +bundle exec rubocop # from the repository root +``` + +Specs stub the HTTP layer with WebMock. Every payload they feed in is **hand-written from the +OpenAPI 2.16 specification**, never captured from a workspace: a conversation body is personal data, +and a fixture is read by everyone who clones the repository. diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/client.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/client.rb index d9362403b..e9b278e3a 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/client.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/client.rb @@ -242,9 +242,12 @@ def blank?(value) value.nil? || value.to_s.empty? end + # `JSON::ParserError` alongside Faraday's own errors: on its own it would + # reach the catch-all below, whose message is the exception's -- and a JSON + # parser opens its message with what it choked on. def must_succeed(operation) yield - rescue Faraday::Error => e + rescue Faraday::Error, JSON::ParserError => e raise api_error(operation, e) rescue APIError # Already mapped, with its status intact; re-wrapping would erase it -- @@ -258,20 +261,54 @@ def must_succeed(operation) # so a smart action can show the operator the real reason instead of # "failed". def api_error(operation, error) + response = response_of(error) + status = response[:status] + body = parse_body(response[:body]) + + APIError.new("Intercom API call failed: #{operation}: #{failure_detail(error, status, body)}", + status: status, body: body) + end + + # Faraday hands the status and the body back in a plain hash on most errors + # and in its own `Env` on a parsing error; both answer `[]`. + def response_of(error) response = error.respond_to?(:response) ? error.response : nil - status = response.is_a?(Hash) ? response[:status] : nil - body = parse_body(response.is_a?(Hash) ? response[:body] : nil) - detail = status ? "HTTP #{status} #{error_message(body)}".strip : "#{error.class}: #{error.message}" + return { status: nil, body: nil } unless response.respond_to?(:[]) + + { status: response[:status], body: response[:body] } + end + + # A body that could not be parsed is named, never quoted: the parser's own + # message opens with the characters it choked on, and on a 200 those are the + # payload -- a conversation body, most of the time (R10). + def failure_detail(error, status, body) + return unreadable_detail(status) if parse_failure?(error) + return "#{error.class}: #{error.message}" unless status - APIError.new("Intercom API call failed: #{operation}: #{detail}", status: status, body: body) + "HTTP #{status} #{error_message(body)}".strip + end + + def unreadable_detail(status) + detail = 'the response could not be read as JSON' + status ? "#{detail} (HTTP #{status})" : detail + end + + def parse_failure?(error) + error.is_a?(Faraday::ParsingError) || error.is_a?(JSON::ParserError) end # Intercom answers a failure with `{ "type": "error.list", "request_id": # "...", "errors": [{ "code": ..., "message": ... }] }`. The request id is # what its support asks for first, so it is appended after the truncation # rather than being what a long body pushes out. + # + # A body of any other shape is *not* echoed here. This message travels into + # the interface and into whatever collects the agent's errors, and the body + # of a response that failed to parse is a payload rather than an error -- + # conversation bodies included (R10). Its size is reported instead, and the + # body itself stays on the exception for whoever inspects one. def error_message(parsed) - return parsed.to_s[0, 500] unless parsed.is_a?(Hash) + return "(unreadable body, #{parsed.to_s.bytesize} bytes)" unless parsed.is_a?(Hash) message = join_errors(parsed['errors']) message = parsed.to_json if message.empty? diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/client_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/client_spec.rb index 39e924ad7..65a9c9d56 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/client_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/client_spec.rb @@ -150,6 +150,34 @@ def json(payload, status = 200, headers = {}) expect { client.me }.to raise_error(APIError, /me: HTTP 429 rate_limit_exceeded/) end + # A body that failed to parse is a payload, not an error: on a 200 it is + # customer content, and this message is shown in the interface and + # collected by whatever watches the agent (R10). + it 'names a body it could not read rather than quoting it' do + stub_request(:get, "#{base}/me") + .to_return(json('Bonjour, voici mon RIB FR76 3000 4000 0500 0012 3456 789')) + + expect { client.me }.to raise_error(APIError) { |error| + expect(error.message).to eq('Intercom API call failed: me: the response could not be read as JSON') + # Faraday hands a parsing error an unfinished response, so there is no + # body to keep here -- which suits this one: the point is that the + # payload does not travel with the error. + expect(error.body).to be_nil + } + end + + # The same guard, one level down: a parser raising on its own would + # otherwise reach the catch-all, whose message is the exception's -- and + # a JSON parser quotes what it choked on. + it 'says as little when a parser raises outside Faraday' do + stub_request(:get, "#{base}/me").to_raise(JSON::ParserError.new("unexpected token 'mon RIB FR76'")) + + expect { client.me }.to raise_error(APIError) { |error| + expect(error.message).to include('could not be read as JSON') + expect(error.message).not_to include('RIB') + } + end + # Whatever else goes wrong on the way, a caller of this client only ever # has to rescue APIError -- and the message names the operation, since a # failure with no endpoint in it is a failure nobody can place. From e1d06852e0bc3eb12e6876151e85cba9900cd6b1 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Tue, 1 Sep 2026 10:38:47 +0200 Subject: [PATCH 8/9] fix(intercom): publish ticket attributes under a name Forest can carry Found on a real workspace: the ticket list answered 400 in five milliseconds, before any call to Intercom. Forest lists the fields of a request in a comma-separated query parameter, and a workspace names its ticket attributes in free text. One of them is called "ID de l'objet en question (immo, facture, user)": the agent splits that on the commas, gets three fields no collection has, and rejects the projection. Nothing was wrong with the read -- the count, which sends no field list, answered 200 the whole time. The introspector now derives a column name from the workspace's own: the commas and colons that are separators in Forest's protocol become spaces, and the HTML escaping Intercom hands back -- `j'ai` -- is undone, since that is an artefact of where the name was typed rather than part of it. The payload key stays the workspace's name, because that is what `ticket_attributes` is keyed by; only the schema sees the derived one. Two attributes reading as the same column would share an entry and the second's values would be read under the first's name, which is worse than missing them: the second is left out with a log line naming both. Co-Authored-By: Claude Opus 5 (1M context) --- .../lib/forest_admin_datasource_intercom.rb | 1 + .../collections/ticket.rb | 15 +++-- .../collections/ticket/serializer.rb | 6 +- .../schema/ticket_attributes_introspector.rb | 64 +++++++++++++++++-- .../collections/ticket_spec.rb | 42 +++++++++--- .../ticket_attributes_introspector_spec.rb | 50 +++++++++++++++ 6 files changed, 154 insertions(+), 24 deletions(-) diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom.rb index 88371a7f6..43d8fe9c9 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom.rb @@ -1,4 +1,5 @@ require_relative 'forest_admin_datasource_intercom/version' +require 'cgi/escape' require 'json' require 'logger' require 'set' diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket.rb index 0239cc0f8..69dcf6953 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket.rb @@ -105,20 +105,21 @@ def define_type_columns end # The attribute columns of every ticket type, in union. Read at boot by - # `TicketAttributesIntrospector`; an attribute whose name is already a - # column of this collection is skipped rather than silently overwriting it. + # `TicketAttributesIntrospector`, which is also where a workspace's own + # name is turned into one a Forest query string can carry. An attribute + # landing on a native column is skipped rather than overwriting it. def register_attribute_columns @attribute_columns = @attributes.reject { |attribute| collides?(attribute) } - @attribute_columns.each { |attribute| add_column(attribute.name, attribute.column_type) } + @attribute_columns.each { |attribute| add_column(attribute.column_name, attribute.column_type) } end def collides?(attribute) - return false unless fields.key?(attribute.name) + return false unless fields.key?(attribute.column_name) ForestAdminDatasourceIntercom.logger.warn( - "[forest_admin_datasource_intercom] #{name} skips the ticket attribute '#{attribute.name}': a native " \ - 'column already carries that name, and overwriting it would show the attribute where the operator ' \ - 'expects the ticket field.' + "[forest_admin_datasource_intercom] #{name} skips the ticket attribute #{attribute.name.inspect}: a " \ + "native column already carries the name #{attribute.column_name.inspect}, and overwriting it would show " \ + 'the attribute where the operator expects the ticket field.' ) true end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket/serializer.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket/serializer.rb index 0ba95a2a4..3fb91030a 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket/serializer.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket/serializer.rb @@ -55,12 +55,16 @@ def type_of(ticket_type) # what stops it from filtering on them, since the filter is written by id # and the id differs from one type to the next. # + # The value is read under the name the workspace gave it and written + # under the column name the schema publishes; the two differ whenever the + # first could not travel through a Forest query string. + # # A ticket of another type simply does not carry the key: the column # reads as absent rather than as empty. def attribute_values_of(values) held = values.is_a?(Hash) ? values : {} - attribute_columns.to_h { |attribute| [attribute.name, coerce(held[attribute.name], attribute)] } + attribute_columns.to_h { |attribute| [attribute.column_name, coerce(held[attribute.name], attribute)] } end # A date attribute comes back as epoch seconds like every other Intercom diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/schema/ticket_attributes_introspector.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/schema/ticket_attributes_introspector.rb index bfc1f7499..85f597e5f 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/schema/ticket_attributes_introspector.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/schema/ticket_attributes_introspector.rb @@ -28,7 +28,20 @@ class TicketAttributesIntrospector DEFAULT_COLUMN_TYPE = 'String'.freeze - Attribute = Struct.new(:name, :column_type, :data_type, :ids_by_ticket_type, keyword_init: true) + # What a column name may not contain, and it has nothing to do with + # Intercom: Forest lists the fields of a request in a **comma-separated** + # query parameter, and uses a colon to name a field through a relation. + # A workspace names its ticket attributes in free text -- measured, one is + # called `ID de l'objet en question (immo, facture, user)` -- and a comma + # in there splits the projection into fields no collection has, which the + # agent rejects as a 400 before the page is ever read. + UNSAFE_IN_A_COLUMN_NAME = /[,:]/ + + # `name` is the key the payload uses, `column_name` the one the schema + # publishes; they differ whenever the workspace's own name cannot travel + # through Forest's query string. + Attribute = Struct.new(:name, :column_name, :column_type, :data_type, :ids_by_ticket_type, + keyword_init: true) def initialize(client) @client = client @@ -61,17 +74,54 @@ def build def collect(ticket_type, union) type_id = ticket_type['id'].to_s definitions(ticket_type).each do |definition| - name = definition['name'].to_s - # An archived attribute is not offered any more, and a nameless one has - # nothing to be a column of. - next if name.empty? || definition['archived'] + entry = entry_for(definition, union) + next if entry.nil? - entry = union[name] ||= Attribute.new(name: name, column_type: column_type_for(definition), - data_type: definition['data_type'], ids_by_ticket_type: {}) entry.ids_by_ticket_type[type_id] = definition['id'].to_s end end + # The union is keyed by column name rather than by the workspace's own, + # since that is what has to be unique in a schema. Two different attributes + # landing on one column would otherwise share an entry, and the second's + # values would be read under the first's name -- wrong values rather than + # missing ones, which is worse. + def entry_for(definition, union) + name = definition['name'].to_s + # An archived attribute is not offered any more, and a nameless one has + # nothing to be a column of. + return nil if name.empty? || definition['archived'] + + column = column_name_for(name) + return nil if column.empty? + + entry = union[column] + return union[column] = attribute_from(name, column, definition) if entry.nil? + return entry if entry.name == name + + warn_collision(name, entry.name, column) + nil + end + + def attribute_from(name, column, definition) + Attribute.new(name: name, column_name: column, column_type: column_type_for(definition), + data_type: definition['data_type'], ids_by_ticket_type: {}) + end + + # Intercom hands these back HTML-escaped -- `Ce que j'ai vérifié` -- + # which is an artefact of where they were typed, not part of the name. + def column_name_for(name) + CGI.unescapeHTML(name).gsub(UNSAFE_IN_A_COLUMN_NAME, ' ').squeeze(' ').strip + end + + def warn_collision(name, kept, column) + ForestAdminDatasourceIntercom.logger.warn( + "[forest_admin_datasource_intercom] the ticket attribute #{name.inspect} is left out: it reads as the " \ + "column #{column.inspect}, which #{kept.inspect} already carries. Rename one of them in Intercom to " \ + 'publish both.' + ) + end + def definitions(ticket_type) return [] unless ticket_type.is_a?(Hash) diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_spec.rb index aef9619da..55c13c07a 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_spec.rb @@ -7,12 +7,15 @@ module ForestAdminDatasourceIntercom # introspection before the stub of it exists. let(:base) { Configuration::REGION_HOSTS[:us] } let(:operators) { ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators } - let(:attributes) do - [Schema::TicketAttributesIntrospector::Attribute.new(name: '_default_title_', column_type: 'String', - data_type: 'string', - ids_by_ticket_type: { '1' => '14162161' }), - Schema::TicketAttributesIntrospector::Attribute.new(name: 'Due', column_type: 'Date', data_type: 'datetime', - ids_by_ticket_type: { '2' => '9002' })] + let(:attributes) { [attribute('_default_title_'), attribute('Due', column_type: 'Date')] } + + # `column_name` is what the schema publishes and `name` the key the payload + # uses; they differ when the workspace's own name cannot travel through a + # Forest query string. + def attribute(name, column_name: nil, column_type: 'String') + Schema::TicketAttributesIntrospector::Attribute.new(name: name, column_name: column_name || name, + column_type: column_type, data_type: 'string', + ids_by_ticket_type: { '1' => '9001' }) end def json(payload, status = 200) @@ -104,10 +107,8 @@ def rows(projection = nil, **options) # the operator expects the ticket field. it 'skips an attribute whose name a native column already carries' do allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) - clashing = Schema::TicketAttributesIntrospector::Attribute.new(name: 'category', column_type: 'String', - data_type: 'string', ids_by_ticket_type: {}) - collection = described_class.new(datasource, attributes: [clashing]) + collection = described_class.new(datasource, attributes: [attribute('category')]) expect(collection.fields['category'].column_type).to eq('String') expect(ForestAdminDatasourceIntercom.logger).to have_received(:warn).with(/skips the ticket attribute/) @@ -163,6 +164,29 @@ def rows(projection = nil, **options) expect(rows.first['_default_title_']).to eq('Facture manquante') end + # Forest lists the fields of a request in a comma-separated query + # parameter, so a column name carrying one splits the projection into + # fields no collection has -- a 400 before the page is ever read. The + # introspector renames such an attribute; the value is still read under the + # name Intercom keys it by. + it 'reads a renamed attribute under the name the payload uses' do + renamed = attribute('ID de l\'objet (immo, facture)', column_name: "ID de l'objet (immo facture)") + collection = described_class.new(datasource, attributes: [renamed]) + stub_search(ticket('1', 'ticket_attributes' => { 'ID de l\'objet (immo, facture)' => 'immo_42' })) + + row = collection.list(nil, filter, nil).first + + expect(row["ID de l'objet (immo facture)"]).to eq('immo_42') + end + + # The invariant behind the rename, asserted on the whole schema rather than + # on one column. + it 'publishes no column name a Forest query string could not carry' do + collection = described_class.new(datasource, attributes: [attribute('Scope', column_name: 'Scope')]) + + expect(collection.fields.keys.grep(/[,:]/)).to be_empty + end + it 'leaves an attribute of another ticket type absent rather than empty' do stub_search(ticket('1')) diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/schema/ticket_attributes_introspector_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/schema/ticket_attributes_introspector_spec.rb index cb862a5eb..a6119eaea 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/schema/ticket_attributes_introspector_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/schema/ticket_attributes_introspector_spec.rb @@ -43,6 +43,56 @@ def stub_types(*types) .to eq([{ '1' => '14162161', '2' => '14162165' }]) end + # Forest lists the fields of a request in a comma-separated query + # parameter: a comma in a column name splits the projection into fields no + # collection has, and the agent rejects the page with a 400 before reading + # anything. Measured on a real workspace, several attributes carry one. + it 'takes the commas out of a column name, keeping the name the payload uses' do + stub_types(ticket_type('1', 'Bug', attribute("ID de l'objet (immo, facture, user)", '9001'))) + + expect(introspector.attributes.first) + .to have_attributes(name: "ID de l'objet (immo, facture, user)", + column_name: "ID de l'objet (immo facture user)") + end + + # A colon is how Forest names a field through a relation. + it 'takes a colon out too' do + stub_types(ticket_type('1', 'Bug', attribute('Scope: mobile', '9001'))) + + expect(introspector.attributes.first.column_name).to eq('Scope mobile') + end + + # Intercom hands the names back HTML-escaped, which is an artefact of where + # they were typed rather than part of the name. + it 'unescapes what Intercom escaped' do + stub_types(ticket_type('1', 'Bug', attribute('Ce que j'ai vérifié & validé', '9001'))) + + expect(introspector.attributes.first.column_name).to eq("Ce que j'ai vérifié & validé") + end + + it 'leaves a name that needs nothing alone' do + stub_types(ticket_type('1', 'Bug', attribute('Severity', '9001'))) + + expect(introspector.attributes.first).to have_attributes(name: 'Severity', column_name: 'Severity') + end + + # Two different attributes landing on one column would otherwise share an + # entry, and the second's values would be read under the first's name -- + # wrong values rather than missing ones. + it 'leaves out a second attribute that reads as an existing column' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + stub_types(ticket_type('1', 'Bug', attribute('Scope, mobile', '9001'), attribute('Scope mobile', '9002'))) + + expect(introspector.attributes.map(&:name)).to eq(['Scope, mobile']) + expect(ForestAdminDatasourceIntercom.logger).to have_received(:warn).with(/is left out/) + end + + it 'leaves out an attribute whose name is nothing but separators' do + stub_types(ticket_type('1', 'Bug', attribute(' , : ', '9001'))) + + expect(introspector.attributes).to be_empty + end + it 'maps the Intercom data types onto what Forest renders' do stub_types(ticket_type('1', 'Bug', attribute('n', '1', data_type: 'integer'), attribute('d', '2', data_type: 'decimal'), From a51bd3d4e8a04a7f2ee27f840c374c9901b2c406 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Tue, 1 Sep 2026 16:28:11 +0200 Subject: [PATCH 9/9] fix(intercom): publish every column read-only A lot that writes nothing was publishing editable columns: neither add_column set is_read_only, so the six collections offered a Save and a Delete in the interface, both reaching the update and delete the collections do not implement -- a 500 where the schema should simply not have offered the button. The two tiers already refuse on the read side what Intercom cannot honour; this is the same rule on the write side. Co-Authored-By: Claude Opus 5 (1M context) --- .../collections/cursor_collection.rb | 3 ++- .../collections/fetch_all_collection.rb | 5 +++++ .../collections/conversation_spec.rb | 6 ++++++ .../collections/fetch_all_collection_spec.rb | 8 +++++++- 4 files changed, 20 insertions(+), 2 deletions(-) diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb index f2a252b4a..eb8fa4721 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb @@ -87,12 +87,13 @@ def read_page(per_page:, cursor:) # collection can honour neither -- except on the primary key, which is # answered by the record endpoint rather than by a filter. A schema that # advertised more would put filters in the interface that the read then - # refuses. + # refuses. Read-only for the same reason, on the write side. def add_column(name, type, is_primary_key: false) operators = is_primary_key ? [Operators::EQUAL, Operators::IN] : [] add_field(name, ColumnSchema.new(column_type: type, filter_operators: operators, is_primary_key: is_primary_key, + is_read_only: true, is_sortable: false, is_groupable: false)) end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/fetch_all_collection.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/fetch_all_collection.rb index 66d33feca..ceeefe5f5 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/fetch_all_collection.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/fetch_all_collection.rb @@ -77,10 +77,15 @@ def aggregate(caller, filter, aggregation, limit = nil) # anything asked of them. A Json column is neither, nor filterable: it # holds a list, and what a filter on it would mean has no in-memory # counterpart. + # + # Every column is read-only: this lot writes nothing, and an editable + # column would offer a Save that reaches an `update` the collection does + # not implement. def add_column(name, type, is_primary_key: false) add_field(name, ColumnSchema.new(column_type: type, filter_operators: self.class.operators_for(type), is_primary_key: is_primary_key, + is_read_only: true, is_sortable: type != 'Json', is_groupable: type != 'Json')) end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/conversation_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/conversation_spec.rb index e5ddbeb6d..eb4a24ad2 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/conversation_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/conversation_spec.rb @@ -106,6 +106,12 @@ def ids(rows) it 'declares no column groupable' do expect(collection.fields.values.map(&:is_groupable).uniq).to eq([false]) end + + # This lot writes nothing: an editable column would offer a Save that + # reaches an `update` the collection does not implement. + it 'declares every column read-only' do + expect(collection.fields.values.map(&:is_read_only).uniq).to eq([true]) + end end describe '#list' do diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/fetch_all_collection_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/fetch_all_collection_spec.rb index e6e56f7ee..09b5cefc0 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/fetch_all_collection_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/fetch_all_collection_spec.rb @@ -62,10 +62,16 @@ def evaluable?(operator, column_type) describe 'columns' do it 'declares a scalar column filterable, sortable and groupable' do expect(collection.fields['name']) - .to have_attributes(is_sortable: true, is_groupable: true, is_read_only: false) + .to have_attributes(is_sortable: true, is_groupable: true) expect(collection.fields['name'].filter_operators).to include(operators::EQUAL) end + # This lot writes nothing: an editable column would offer a Save that + # reaches an `update` the collection does not implement. + it 'declares every column read-only' do + expect(collection.fields.values.map(&:is_read_only).uniq).to eq([true]) + end + # A list has no in-memory counterpart for any of the three. it 'declares a Json column neither filterable nor sortable' do expect(collection.fields['team_ids'])