diff --git a/packages/forest_admin_datasource_intercom/README.md b/packages/forest_admin_datasource_intercom/README.md index 38503e773..43ce826e8 100644 --- a/packages/forest_admin_datasource_intercom/README.md +++ b/packages/forest_admin_datasource_intercom/README.md @@ -60,7 +60,15 @@ beats not running. 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. +degrades to no attribute column, a collection whose endpoint answers 403 fails its own page, and a +token that cannot read `/admins` or `/teams` leaves the `admin_names` / `team_names` column empty +rather than failing the page it is on. + +A **relation is the exception**, and it is worth knowing before scoping a token: resolving one reads +the target endpoint, and that read is not guarded the way the names above are. A token denied +`/admins` fails any page projecting `admin_assignee:name`, and fails the related list behind +`IntercomTeam#admins` — the failure lands on the collection being read, not on the one that was +denied. Scope the token to the endpoints in the table below, or to none of them. ## Collections @@ -70,12 +78,13 @@ degrades to no attribute column, and a collection whose endpoint answers 403 fai | `IntercomTicket` | `POST /tickets/search`, `GET /tickets/{id}` | cursor | yes, exactly | | `IntercomAdmin` | `GET /admins` | read whole | yes, exactly | | `IntercomTeam` | `GET /teams` | read whole | yes, exactly | +| `IntercomTeamMembership` | `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, +**Read whole** — admins, teams, team memberships, 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. @@ -86,6 +95,66 @@ nothing is filtered or sorted in memory. Three routes and no fourth: no conditio Intercom's search DSL and walked through the search endpoint. What the translation cannot express is **refused by name** — see [Filtering](#filtering). +## Relations + +Intercom joins nothing: a ticket carries an assignee id, and the teammate behind it is a second read +of a second endpoint. What makes eight relations affordable is that every collection on the far end +is read whole in one request — so a relation resolves for a **whole page at the price of one read**, +never one read per row. The price is per target *collection*, not per relation: a ticket's `state` +and `previous_state` are one read of `/ticket_states`, over the ids both of them name. + +*Exactly*, with one bound worth naming: "read whole" is what the endpoint answers, and `fetch_all` +stops after [`MAX_COLLECTED_PAGES`](lib/forest_admin_datasource_intercom/client.rb) pages if Intercom +paginates one of these on its own — it logs when it does. A workspace whose `/admins` or `/teams` +runs past that cap resolves the relations pointing at the records it dropped as empty. The figure is +sized for reference collections, which is what every target here is. + +| Collection | Relation | Target | Filterable through | +| --- | --- | --- | --- | +| `IntercomConversation` | `admin_assignee`, `closed_by` | `IntercomAdmin` | yes | +| `IntercomConversation` | `team_assignee` | `IntercomTeam` | yes | +| `IntercomTicket` | `admin_assignee` | `IntercomAdmin` | yes | +| `IntercomTicket` | `team_assignee` | `IntercomTeam` | yes | +| `IntercomTicket` | `ticket_type` | `IntercomTicketType` | yes | +| `IntercomTicket` | `state`, `previous_state` | `IntercomTicketState` | **no** — read and navigate only | +| `IntercomTeam` | `admins` | `IntercomAdmin` | no (many-to-many) | +| `IntercomAdmin` | `teams` | `IntercomTeam` | no (many-to-many) | +| `IntercomTeamMembership` | `team`, `admin` | `IntercomTeam`, `IntercomAdmin` | yes | + +Every one of them is **read-only**: this lot writes nothing, and Intercom exposes no endpoint that +writes a team membership at all. + +**`IntercomTeamMembership` exists because Intercom's does not.** The workspace carries the +membership on the team (`admin_ids`) and on the teammate (`team_ids`) both and exposes no resource +for the pair, while a many-to-many needs a collection to travel through. It is synthesized from +`GET /teams`, one record per pair, keyed `teamId:adminId`. Without it, both sides read as an array of +ids nobody can click. + +Two consequences of travelling through it are worth knowing. A **related list of teammates is +ordered by the membership, not by the teammate**: the agent hands the through collection the columns +of the collection the relation reaches, so an order on `name` or `email` cannot be resolved there and +is logged rather than silently dropped. And a `admin_ids` entry naming a teammate `/admins` does not +answer -- one who left, one outside the token's reach -- **drops out of the related list** instead of +appearing as an empty row. + +Alongside it, a team names its teammates (`admin_names`) and a teammate its teams (`team_names`) on +the row itself, so a list view reads without a join. **Those replace the arrays of ids** the first +lots published: one readable form plus a relation to navigate, rather than two ways to read one fact. +They are read only when a projection asks for them, and a token that cannot read the other side +costs the column and nothing else — never the page, and never the relation. + +The same rule settled the ticket labels: `state_label` and `ticket_type_name` stay on the row, +`state_category` and `state_external_label` are gone — they are a hop away, on the `state` relation, +and neither was ever filterable, so no segment, scope or saved filter could rest on them. + +A relation reads its target **undecorated**, so a permission scope or a segment defined on the target +does not narrow what a relation resolves — the same way a native datasource joins a table without +applying the scopes of the collection mapped to it. + +One semantic worth stating plainly: **a row whose foreign key is null matches no relation filter**, +the way a join drops it, negated filters included. A ticket with no assignee is not "assigned to +someone other than Marie". + ## 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 @@ -200,6 +269,9 @@ refuses a search by name. `last_responder_name`, `last_responder_type`. They exist nowhere in Intercom; `/tickets/search` filters none of them and ignores a sort on them without a word; - **the account of a ticket** — `company_id`, refused by the endpoint itself with `invalid_field`; +- **the state of a ticket** — the measured table carries no filter on a state id, so `state_id`, + `previous_state_id` and the `state` relation are read and navigated rather than filtered. Whether + the endpoint filters one at all is one of the probe's open questions; - **the ticket attributes** — filtered as `ticket_attribute.{id}`, and the same attribute carries a different id per ticket type, so a union column has no single id to translate to. See [Tickets](#tickets); @@ -212,6 +284,39 @@ refuses a search by name. - **group-by**, on either cursor collection: there is no aggregate endpoint, and grouping over the pages a walk collected would look exact while answering a fraction. +### Through a relation + +A relation is published filterable as soon as *any* column of its target is — the agent decides that, +not this datasource — so the interface offers `admin_assignee:name` the moment the relation exists. +What Intercom is really filtered on is the foreign key: the **target says which of its records +match**, over every record it holds rather than over a page, and the ids it names become the +condition the search carries. + +That is exact, and it has three visible edges: + +- Intercom takes no membership operator on these fields, so several matches become **one equality per + match**, inside an `OR` — which counts against the fifteen conditions a group allows. A relation + condition matching more records than that is refused by name rather than sent and answered with a + 400 naming neither the limit nor the filter that hit it. That `OR` is **inlined into a parent that + aggregates the same way**, so it costs no level of nesting where it does not have to: the two + levels Intercom allows are spent on the filter that was written, not on the expansion of a + relation. Where inlining it would take the parent past fifteen conditions it stays nested, width + being the scarcer of the two. +- A condition the target matched **no record** with names no row, and the DSL cannot say so: the + search is skipped entirely rather than sent as a filter that would come back with everything. +- A relation whose foreign key the endpoint does not filter — the ticket `state` — is refused with a + message saying which of the two it is: the relation is there to be read and navigated. Whether + `/tickets/search` filters a state id at all is one of the probe's open questions; the answer lands + in the table, not in an assumption. + +On the collections read whole the same condition costs nothing: they filter in memory, so the ids go +in as a plain membership and none of the DSL's limits apply. + +A **many-to-many is published unfilterable** — `admins` and `teams` — and a condition written on one +anyway, in a scope or a segment, is refused before it reaches this datasource: the agent's own +validator answers a 400 naming the field and its type. Filter on a column of the collection next +door instead. + ### The limits of a search, checked before the request leaves Intercom nests a search **two levels** deep and takes **fifteen conditions per group**. Past either @@ -327,7 +432,7 @@ Everything else is read when a collection is listed, so an agent boots whatever | Lot | What it brings | | --- | --- | | 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 | +| 4 | Contacts and companies, and the relations towards them promoted from today's denormalized columns | | 5 | Notes, tags, segments | | 6 | Bounded group-by and the reporting export | 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 index 45704ea82..9cba4a2d3 100644 --- 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 @@ -25,11 +25,37 @@ def serialize(admin) '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) } } + # Not a column: it is what the names below are read from, and what the + # membership collection turns into the relation. + 'team_ids' => Array(attrs['team_ids']).map { |id| stringify_id(id) }, + 'team_names' => nil } + end + + # The teams the teammate belongs to, by name, and only when a projection + # asked for them: one read of `/teams` for the whole page. A token that + # cannot read them costs the column and nothing else. + def enrich(records, rows, projection) + return unless column_asked?(projection, 'team_names') + + names = team_names + records.each_with_index do |record, index| + rows[index]['team_names'] = Array(record['team_ids']).filter_map { |id| names[id] } + end end private + def team_names + client.fetch_all('teams', list_key: 'teams') + .to_h { |team| [stringify_id(team['id']), team['name']] } + rescue APIError => e + ForestAdminDatasourceIntercom.logger.warn( + "[forest_admin_datasource_intercom] #{name} could not read the teams of the workspace (HTTP " \ + "#{e.status || "-"}); the names are left empty. The relation to IntercomTeam is unaffected." + ) + {} + end + def define_schema add_column('id', 'String', is_primary_key: true) add_column('name', 'String') @@ -41,11 +67,14 @@ def define_schema 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') + # A list, so neither filterable nor sortable -- as the array of ids it + # replaces was. It reads the teammate without a join; the relation below + # is what navigates it, through the membership collection that gives the + # many-to-many the join Intercom does not expose. + add_column('team_names', 'Json') + add_many_to_many('teams', foreign_collection: 'IntercomTeam', + through_collection: 'IntercomTeamMembership', + origin_key: 'admin_id', foreign_key: 'team_id') 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 index 9c4731f27..30d11d7e7 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 @@ -4,11 +4,15 @@ module Collections # 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. + # Read-only for now: the writes and the business actions arrive with lot 3. + # The relations towards Contacts and Companies wait for lot 4, those two + # collections not existing yet -- a relation whose target collection is + # missing is a schema the agent refuses to boot on. The relations between the + # collections this datasource already serves are declared and answered here, + # through `Relations`. class BaseCollection < ForestAdminDatasourceToolkit::Collection + include Relations + ColumnSchema = ForestAdminDatasourceToolkit::Schema::ColumnSchema Operators = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators Equivalent = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::ConditionTreeEquivalent @@ -27,14 +31,34 @@ def client 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. + # A record narrowed to the columns that were 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. + # + # A path through a relation is not a column and is skipped here: it is + # answered by `embed_relations`, which nests a whole row under the relation + # name once the page is in hand. + # + # No projection at all asks for every column -- which is not the same as + # every key a serialized record happens to carry: a couple of them hold + # what a column is read *from*, the ids behind a membership for one, and + # publishing those would show the operator the plumbing. def project(record, projection) - fields = Array(projection) - return record if fields.empty? + asked = Array(projection).map(&:to_s) + return record.slice(*column_names) if asked.empty? + + asked.reject { |field| field.include?(':') }.to_h { |field| [field, record[field]] } + end + + # Whether a projection asks for a column. No projection at all asks for + # every declared column, which is how `project` reads it -- an enrichment + # guarded on the column being named would leave nil the very column the + # projection publishes. + def column_asked?(projection, column) + asked = Array(projection).map(&:to_s) - fields.to_h { |field| [field, record[field]] } + asked.empty? || asked.include?(column) end # The window a list view asked for, cut out of records already in hand. @@ -54,6 +78,10 @@ def page_window(records, filter) records[offset, limit] || [] end + def column_names + @column_names ||= fields.select { |_, field| field.is_a?(ColumnSchema) }.keys + 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) 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 dbe8a53d0..380caeec9 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,7 @@ 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 + class Conversation < CursorCollection # rubocop:disable Metrics/ClassLength include ContactIdentity include Conversation::Serializer include Conversation::Timeline @@ -69,18 +69,38 @@ def define_schema 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_assignment_columns define_contact_columns define_source_columns define_statistics_columns add_column('tag_names', 'Json') add_column('ai_agent_participated', 'Boolean') add_column('timeline', 'Json') + define_relations + end + + # Who the conversation sits with, and who closed it. All three targets are + # read whole in one request, and `/conversations/search` takes a filter on + # each of the three keys -- so these relations can be read, navigated and + # filtered through alike. + # + # No relation towards the company: a conversation carries its account as a + # whole object, so the name is already on the row, and the Companies + # collection arrives with lot 4. + def define_relations + add_many_to_one('admin_assignee', foreign_collection: 'IntercomAdmin', foreign_key: 'admin_assignee_id') + add_many_to_one('team_assignee', foreign_collection: 'IntercomTeam', foreign_key: 'team_assignee_id') + add_many_to_one('closed_by', foreign_collection: 'IntercomAdmin', foreign_key: 'closed_by_id') + end + + # Who the conversation sits with, and which account it belongs to. 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. + def define_assignment_columns + add_column('admin_assignee_id', 'String') + add_column('team_assignee_id', 'String') + add_column('company_id', 'String') + add_column('company_name', 'String') end # The contact identity is denormalized onto the row rather than declared as 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 17c7d3c39..62545008f 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 @@ -16,6 +16,10 @@ module Collections # refuses by name: an unfiltered page served in answer to a filter is the # one failure this datasource is built to avoid. # + # A condition through a relation is resolved before any of that: the target + # collection is asked which of its records match, and what reaches Intercom + # is a condition on the foreign key. See `Relations`. + # # 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 @@ -39,8 +43,14 @@ def list(caller, filter, projection) warn_ignored_sort(filter&.sort) records = fetch_records(caller, filter) - rows = records.map { |record| project(serialize(record), projection) } + # Serialized whole and projected afterwards rather than the other way + # round: a projection reaching through a relation names no foreign key, + # and the key is where the relation is read from. + serialized = records.map { |record| serialize(record) } + rows = serialized.map { |record| project(record, projection) } + enrich(records, rows, projection) + embed_relations(caller, serialized, rows, projection) rows end @@ -146,7 +156,13 @@ def fetch_records(caller, filter) # having been dropped by the truncation before the window was applied. return records_by_ids(page_window(ids, filter)) if ids - listed_records(filter, translate(caller, filter)) + query = translate(caller, filter) + # A condition through a relation the target matched no record with names + # no row, and Intercom's DSL cannot say so: the read is skipped rather + # than sent as a filter that would come back with everything. + return [] if query == NOTHING + + listed_records(filter, query) end # The Intercom query a filter comes down to, or nil for a list view, which @@ -155,12 +171,64 @@ def fetch_records(caller, filter) # one tree, it is checked against the nesting Intercom allows like every # other condition, instead of adding a level nothing counted. def translate(caller, filter) - tree = combined_tree(filter) + tree = rewrite_relation_conditions(caller, combined_tree(filter)) do |key, ids, leaf| + relation_group(key, ids, leaf) + end + return NOTHING if tree == NOTHING Query::ConditionTreeTranslator.call(tree, endpoint: search_endpoint, collection: name, timezone: timezone_for(caller)) end + # The ids the target matched, written as the filter Intercom does take on + # the foreign key: a group of equalities, its DSL offering no membership + # operator on these fields. + # + # That group counts against the fifteen conditions Intercom allows, so a + # relation condition matching more records than that is refused rather than + # sent -- and refused here, where the message can name the relation the + # operator filtered on rather than the key it resolved to. + # + # It also costs a level of nesting, which is why it is built as the group + # `Relations` can tell from one the operator wrote: inlined into a parent + # that aggregates the same way, it costs none. + def relation_group(key, ids, leaf) + refuse_fan_out!(leaf, key, ids) if ids.size > Query::ConditionTreeTranslator::MAX_GROUP_SIZE + return Leaf.new(key, Operators::EQUAL, ids.first) if ids.size == 1 + + RelationBranch.new('Or', ids.map { |id| Leaf.new(key, Operators::EQUAL, id) }, leaf.field) + end + + # Inlining a relation group into its parent trades a level of nesting for + # width, and Intercom bounds both. Past the conditions a group may hold, + # the nested form is the one it answers. + def absorb_relation_group?(size) + size <= Query::ConditionTreeTranslator::MAX_GROUP_SIZE + end + + # A relation this endpoint filters nothing through. It is navigable all the + # same -- the read costs nothing, the target being read whole -- and saying + # which of the two it is, is the whole point of the message. + def check_relation_filterable!(leaf, relation) + key = relation.foreign_key + refuse_unfilterable_key!(leaf, key) if search_endpoint.field(key).nil? + end + + def refuse_unfilterable_key!(leaf, key) + raise UnsupportedOperatorError, + "#{name} cannot filter #{leaf.field.inspect}: the relation resolves to #{key.inspect}, on " \ + "which #{search_endpoint.path} takes no filter. The relation is there to be read and " \ + "navigated; filter on one of: #{search_endpoint.filterable_columns.join(", ")}." + end + + def refuse_fan_out!(leaf, key, ids) + raise UnsupportedOperatorError, + "#{name} cannot filter #{leaf.field.inspect}: it names #{ids.size} records, " \ + "#{search_endpoint.path} answers #{key.inspect} one value at a time, and Intercom takes " \ + "#{Query::ConditionTreeTranslator::MAX_GROUP_SIZE} conditions per group. Narrow the condition " \ + "on the relation, or filter on #{key.inspect} itself." + end + def combined_tree(filter) conditions = [filter&.condition_tree, search_condition(filter)].compact return conditions.first if conditions.size < 2 @@ -244,7 +312,10 @@ def count_records(caller, filter) ids = id_lookup(filter) return records_by_ids(ids).size if ids - page = read_page(per_page: 1, cursor: nil, query: translate(caller, filter)) + query = translate(caller, filter) + return 0 if query == NOTHING + + page = read_page(per_page: 1, cursor: nil, query: query) return page.total_count if page.total_count raise UnsupportedOperatorError, 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 ceeefe5f5..2cb9d8b30 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 @@ -17,7 +17,7 @@ module Collections # 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 + class FetchAllCollection < BaseCollection # rubocop:disable Metrics/ClassLength # 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 @@ -57,8 +57,12 @@ def initialize(datasource, name) def list(caller, filter, projection) records = sort_in_memory(filtered_records(caller, filter), filter&.sort) + window = page_window(records, filter) + rows = window.map { |record| project(record, projection) } - page_window(records, filter).map { |record| project(record, projection) } + enrich(window, rows, projection) + embed_relations(caller, window, rows, projection) + rows end # Exact, like the filter and the sort above it, which is why these columns @@ -90,6 +94,12 @@ def add_column(name, type, is_primary_key: false) is_groupable: type != 'Json')) end + # Hook for what a row needs beyond the endpoint this collection reads -- + # a name held on the other side of a membership, say. Called with the rows + # of the page only, and with the projection, so a column nobody asked for + # costs no request. + def enrich(_records, _rows, _projection); end + # Every record of the collection, straight from its endpoint. def fetch_all = raise(NotImplementedError, "#{self.class} did not implement fetch_all") @@ -105,6 +115,13 @@ def filtered_records(caller, filter) tree = filter&.condition_tree return records if tree.nil? + # A condition through a relation becomes a membership on the foreign key: + # this tier filters in memory, where a list of ids costs no more than one + # id and none of Intercom's group limits apply -- they bound its search + # DSL, which nothing here goes through. + tree = rewrite_relation_conditions(caller, tree) { |key, ids, _| Leaf.new(key, Operators::IN, ids) } + return [] if tree == NOTHING + refuse_unevaluable!(tree) tree.apply(records, self, timezone_for(caller)) end @@ -156,19 +173,36 @@ def sort_in_memory(records, sort) # 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. + # row and leave the order to the tie-break. Reported when it happens, + # rather than dropped in silence -- an order asked for and not honoured is + # reported everywhere else in this datasource, and there is one route that + # reaches this tier with a clause it cannot resolve: a related list through + # a many-to-many is served by the collection it travels through, and + # `Filter#nest` prefixes the condition tree without prefixing the sort, so + # the membership is handed the columns of the collection it reaches. def sort_clauses(sort) - Array(sort).filter_map do |clause| - field = clause[:field] || clause['field'] - next unless fields.key?(field) + known, unknown = Array(sort).partition { |clause| fields.key?(sort_field(clause)) } + warn_unsortable(unknown) unless unknown.empty? + known.map do |clause| # `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] + [sort_field(clause), ascending != false] end end + def sort_field(clause) = clause[:field] || clause['field'] + + def warn_unsortable(clauses) + ForestAdminDatasourceIntercom.logger.warn( + "[forest_admin_datasource_intercom] #{name} was asked to sort on " \ + "#{clauses.map { |clause| sort_field(clause).inspect }.join(", ")}, which it does not carry; the rows " \ + 'come back in the order Intercom returned them. A related list through a many-to-many is ordered by the ' \ + 'columns of the collection it travels through, not by those of the collection it reaches.' + ) + end + def compare_clauses(left, right, clauses) clauses.each do |field, ascending| comparison = compare_values(left[field], right[field]) diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/relations.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/relations.rb new file mode 100644 index 000000000..bcb7dc03b --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/relations.rb @@ -0,0 +1,266 @@ +module ForestAdminDatasourceIntercom + module Collections + # How this datasource declares a relation, and how it answers one. + # + # Intercom joins nothing: a ticket carries an assignee id, and the teammate + # behind it is a second read of a second endpoint. What makes that affordable + # -- and exact -- is that every collection on the far end of these relations + # is read whole in one request, the tier `FetchAllCollection` serves. + # + # Declaring a relation obliges both halves below, and the second is not + # optional: a many-to-one is published filterable as soon as *any* column of + # its target is (`GeneratorField#build_many_to_one_schema`), so the interface + # offers `assignee:name` the moment the relation exists. Left unanswered, + # that filter reaches a translator refusing every traversing field -- the + # interface offering a filter the datasource then refuses, which is the one + # thing this package is built not to do. + # + # * a projection through a relation (`assignee:name`) is answered by reading + # the target once per page and nesting its row under the relation name; + # * a filter through a relation is answered by asking the target which of its + # records match, and filtering Intercom on the ids it names. + # + # The second is exact rather than approximate -- the target answers over + # every record it holds, not over a page -- with one semantic worth stating + # plainly: a row whose foreign key is null matches no relation filter, the + # way a join drops it, negated filters included. A ticket with no assignee is + # not "assigned to someone other than Marie". + module Relations # rubocop:disable Metrics/ModuleLength + ManyToOneSchema = ForestAdminDatasourceToolkit::Schema::Relations::ManyToOneSchema + ManyToManySchema = ForestAdminDatasourceToolkit::Schema::Relations::ManyToManySchema + Filter = ForestAdminDatasourceToolkit::Components::Query::Filter + Projection = ForestAdminDatasourceToolkit::Components::Query::Projection + Operators = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators + Branch = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeBranch + Leaf = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf + + # What a relation condition comes to when the target matched no record: no + # row can satisfy it. It is a value rather than an empty condition because + # the two tiers spell "match nothing" differently -- in memory an `in []` + # says it, while Intercom's search DSL has no way to. + NOTHING = :matches_nothing + + # The group a relation condition expands into, told apart from a group the + # operator wrote. Two things hang on the difference: this one may be + # inlined into a parent aggregating the same way -- the level it adds is + # one nobody budgeted for -- and a tier refusing it can name the relation + # rather than a shape the operator never wrote. + class RelationBranch < Branch + attr_reader :relation_field + + def initialize(aggregator, conditions, relation_field) + @relation_field = relation_field + super(aggregator, conditions) + end + end + + protected + + # Every relation of this package points at a collection keyed by `id` and + # is read-only: nothing in this lot writes, and an editable relation would + # offer an association the collection cannot perform. + def add_many_to_one(name, foreign_collection:, foreign_key:) + add_field(name, ManyToOneSchema.new(foreign_collection: foreign_collection, + foreign_key: foreign_key, + foreign_key_target: 'id', + is_read_only: true)) + end + + def add_many_to_many(name, foreign_collection:, through_collection:, origin_key:, foreign_key:) + add_field(name, ManyToManySchema.new(foreign_collection: foreign_collection, + through_collection: through_collection, + origin_key: origin_key, origin_key_target: 'id', + foreign_key: foreign_key, foreign_key_target: 'id', + is_read_only: true)) + end + + # The rows of one page, with the relations the projection named nested onto + # them. `records` are the same rows before projection: that is where the + # foreign keys are read, since a projection naming `assignee:name` does not + # have to name `admin_assignee_id`. + # + # One request per target *collection* per page -- never one per row, and + # never twice for two relations that point at the same collection: a + # ticket's `state` and `previous_state` are one read of `/ticket_states`, + # over the ids both of them name. + def embed_relations(caller, records, rows, projection) + asked = many_to_one_asked(projection) + return if asked.empty? + + indexed = indexed_targets(caller, records, asked) + + asked.each do |name, relation, wanted| + rows_of_target = indexed[relation.foreign_collection] + + # Nil rather than absent when an id names no record: a teammate who + # left the workspace reads as no teammate, not as a broken row. Sliced + # back to what this relation asked for, the read having been widened to + # the union of what every relation on that collection did. + records.each_with_index do |record, index| + target_row = rows_of_target[record[relation.foreign_key]] + rows[index][name] = target_row&.slice(*wanted) + end + end + end + + # The tree this tier can filter on, every `relation:field` leaf traded for + # a condition on the foreign key. The block is handed that key and the ids + # the target matched, and answers the node the tier wants: an `in` where + # the filtering is done in memory, a group of equalities where Intercom's + # DSL takes no membership operator. + # + # An `and` carrying a leaf that matches nothing matches nothing itself; an + # `or` drops that leaf and keeps its siblings. + def rewrite_relation_conditions(caller, node, &builder) + case node + when Branch then rewrite_branch(caller, node, &builder) + when Leaf then relation_leaf?(node) ? rewrite_relation_leaf(caller, node, &builder) : node + else node + end + end + + private + + def relation_leaf?(node) + node.is_a?(Leaf) && node.field.to_s.include?(':') + end + + def relations_asked(projection) + Projection.new(Array(projection).map(&:to_s)).relations + end + + # The many-to-one relations the projection named, each with the columns it + # asked of its target. The target's own key travels with them whether or + # not it was asked for: it is what the rows are indexed by here, and what + # makes the nested row a link rather than a label in the interface. + def many_to_one_asked(projection) + relations_asked(projection).filter_map do |name, sub_projection| + relation = fields[name] + next unless relation.is_a?(ManyToOneSchema) + + [name, relation, Array(sub_projection).map(&:to_s).union([relation.foreign_key_target])] + end + end + + def indexed_targets(caller, records, asked) + asked.group_by { |_, relation, _| relation.foreign_collection } + .transform_values { |group| target_rows(caller, records, group) } + end + + # One read per target collection, over the ids every relation pointing at + # it names and the union of the columns they asked for. + def target_rows(caller, records, group) + relation = group.first[1] + ids = group.flat_map { |_, rel, _| records.filter_map { |record| record[rel.foreign_key] } }.uniq + return {} if ids.empty? + + target = relation.foreign_key_target + wanted = Projection.new(group.flat_map { |_, _, columns| columns }.uniq) + filter = Filter.new(condition_tree: Leaf.new(target, Operators::IN, ids)) + + foreign_collection(relation).list(caller, filter, wanted).to_h { |row| [row[target], row] } + end + + def rewrite_branch(caller, branch, &builder) + rewritten = Array(branch.conditions).map { |node| rewrite_relation_conditions(caller, node, &builder) } + + if branch.aggregator.to_s.casecmp('or').zero? + kept = rewritten.reject { |node| node == NOTHING } + kept.empty? ? NOTHING : Branch.new(branch.aggregator, absorb_groups(branch.aggregator, kept)) + elsif rewritten.include?(NOTHING) + NOTHING + else + Branch.new(branch.aggregator, absorb_groups(branch.aggregator, rewritten)) + end + end + + # A relation group aggregating the way its parent does is inlined into it: + # `or(x, or(a, b))` is `or(x, a, b)` and one level shallower. The level it + # saves is one nothing budgeted for -- what a tier measured its nesting + # limit against is the tree the operator wrote, not the equalities a + # relation expands into afterwards. + # + # Only a relation group is inlined. A group the operator wrote is what + # their filter means, and flattening it would spend on one group the + # conditions two groups were holding. + def absorb_groups(aggregator, nodes) + absorbed = nodes.flat_map do |node| + inlinable_group?(node, aggregator) ? Array(node.conditions) : [node] + end + + absorb_relation_group?(absorbed.size) ? absorbed : nodes + end + + def inlinable_group?(node, aggregator) + node.is_a?(RelationBranch) && node.aggregator.to_s.casecmp(aggregator.to_s).zero? + end + + # Hook for a tier that bounds how many conditions a group may hold: past + # that, the nested form is the one that fits, and the level it costs is + # spent rather than the width. + def absorb_relation_group?(_size) = true + + def rewrite_relation_leaf(caller, leaf) + name, path = leaf.field.to_s.split(':', 2) + relation = fields[name] + refuse_unknown_relation!(leaf, name) unless relation.is_a?(ManyToOneSchema) + refuse_two_hops!(leaf) if path.include?(':') + + check_relation_filterable!(leaf, relation) + + ids = matching_ids(caller, relation, Leaf.new(path, leaf.operator, leaf.value)) + return NOTHING if ids.empty? + + yield(relation.foreign_key, ids, leaf) + end + + # Hook for a tier that cannot filter on every foreign key it declares a + # relation on. Answered before the target is read: a refusal that spends a + # request first costs exactly what it refuses to do. + def check_relation_filterable!(_leaf, _relation); end + + # Which records of the target the condition names, asked of the target + # itself: it owns what its columns can be filtered with, and it answers + # over every record Intercom holds rather than over a page of them. + def matching_ids(caller, relation, leaf) + target = relation.foreign_key_target + + foreign_collection(relation) + .list(caller, Filter.new(condition_tree: leaf), Projection.new([target])) + .filter_map { |row| row[target] } + .uniq + end + + # The target as the datasource holds it, undecorated -- so a permission + # scope or a segment defined on the target does not narrow what a relation + # resolves. That is how a native datasource behaves too: it joins the table + # without applying the scopes of the collection mapped to it. + def foreign_collection(relation) + datasource.get_collection(relation.foreign_collection) + end + + # Either a name this collection carries no relation under -- a condition + # from a scope or a segment written against another schema -- or a + # many-to-many, which is published unfilterable: resolving one would mean + # reading the collection it travels through once per value. + def refuse_unknown_relation!(leaf, relation_name) + filterable = fields.select { |_, field| field.is_a?(ManyToOneSchema) }.keys + + raise UnsupportedOperatorError, + "#{name} cannot filter #{leaf.field.inspect}: #{relation_name.inspect} is not a relation it filters " \ + "through. #{filterable.empty? ? "It has none." : "Those it does: #{filterable.join(", ")}."} Filter " \ + 'on a column of the collection next door instead.' + end + + # Two hops would mean resolving a relation of a relation, one read per + # level, and nothing in this datasource publishes a filter that deep -- a + # many-to-many is published unfilterable. It is refused by name rather than + # half-answered. + def refuse_two_hops!(leaf) + raise UnsupportedOperatorError, + "#{name} cannot filter #{leaf.field.inspect}: it reaches through two relations, and this datasource " \ + 'resolves one. Filter on a column of the collection next door instead.' + 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 index 2286bfa46..a0e111bcf 100644 --- 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 @@ -19,17 +19,49 @@ def serialize(team) { 'id' => stringify_id(attrs['id']), 'name' => attrs['name'], - 'admin_ids' => Array(attrs['admin_ids']).map { |id| stringify_id(id) } } + # Not a column: it is what the names below are read from, and what the + # membership collection turns into the relation. Intercom types these + # as numbers here and as strings on the admin itself, so they are + # stringified for both sides to carry the same id. + 'admin_ids' => Array(attrs['admin_ids']).map { |id| stringify_id(id) }, + 'admin_names' => nil } + end + + # The teammates of the team, by name, and only when a projection asked for + # them: one read of `/admins` for the whole page, never one per team. A + # token that cannot read the teammates costs the column and nothing else. + def enrich(records, rows, projection) + return unless column_asked?(projection, 'admin_names') + + names = admin_names + records.each_with_index do |record, index| + rows[index]['admin_names'] = Array(record['admin_ids']).filter_map { |id| names[id] } + end end private + def admin_names + client.fetch_all('admins', list_key: 'admins') + .to_h { |admin| [stringify_id(admin['id']), admin['name']] } + rescue APIError => e + ForestAdminDatasourceIntercom.logger.warn( + "[forest_admin_datasource_intercom] #{name} could not read the teammates of the workspace (HTTP " \ + "#{e.status || "-"}); the names are left empty. The relation to IntercomAdmin is unaffected." + ) + {} + end + 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') + # A list, so neither filterable nor sortable -- as the array of ids it + # replaces was. It reads the team without a join; the relation below is + # what navigates it, and filtering happens on the Admins collection. + add_column('admin_names', 'Json') + add_many_to_many('admins', foreign_collection: 'IntercomAdmin', + through_collection: 'IntercomTeamMembership', + origin_key: 'team_id', foreign_key: 'admin_id') end end end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/team_membership.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/team_membership.rb new file mode 100644 index 000000000..9a1d662b0 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/team_membership.rb @@ -0,0 +1,61 @@ +module ForestAdminDatasourceIntercom + module Collections + # The membership of the inbox teams: one record per team-and-teammate pair. + # + # It exists because Intercom's does not. The workspace carries the membership + # on the team (`admin_ids`) and on the teammate (`team_ids`) both, and offers + # no resource for the pair -- while a many-to-many needs a collection to + # travel through, whose two many-to-one relations are what + # `Utils::Collection.get_through_target` looks for. Declared here, Teams and + # Admins navigate to each other in both directions; left undeclared, both + # sides read as an array of ids. + # + # Read from `/teams` alone. The admin side names the same pairs, so reading + # it too would spend a request confirming what the first answer already said. + # Read-only, like the relations it carries: Intercom exposes no endpoint that + # writes a membership, and an editable relation would offer an association + # that could only fail. + class TeamMembership < FetchAllCollection + def initialize(datasource) + super(datasource, 'IntercomTeamMembership') + end + + protected + + def fetch_all + client.fetch_all('teams', list_key: 'teams').flat_map { |team| pairs_of(team) } + end + + # Keyed by both ids rather than by a counter: a related list is read over + # two requests, and a key that changed between them would move the rows + # under the operator. + def serialize(pair) + { 'id' => "#{pair["team_id"]}:#{pair["admin_id"]}", + 'team_id' => pair['team_id'], + 'admin_id' => pair['admin_id'] } + end + + private + + def pairs_of(team) + return [] unless team.is_a?(Hash) + + team_id = stringify_id(team['id']) + return [] if team_id.nil? + + Array(team['admin_ids']).filter_map do |admin_id| + id = stringify_id(admin_id) + { 'team_id' => team_id, 'admin_id' => id } unless id.nil? + end + end + + def define_schema + add_column('id', 'String', is_primary_key: true) + add_column('team_id', 'String') + add_column('admin_id', 'String') + add_many_to_one('team', foreign_collection: 'IntercomTeam', foreign_key: 'team_id') + add_many_to_one('admin', foreign_collection: 'IntercomAdmin', foreign_key: 'admin_id') + end + end + end +end 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 2569e4552..f071cd37c 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 @@ -85,17 +85,23 @@ def define_schema define_contact_columns define_derived_columns add_column('part_count', 'Number') + # Before the attribute columns rather than after: a workspace attribute + # whose name lands on a relation is then skipped with a warning, the way + # one landing on a column already is. Declared after, it would collide + # and take the boot with it. + define_relations 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. + # The state arrives embedded as a whole object, so its label costs nothing: + # a queue reads without a join. One label and not three -- the category and + # the customer-facing label are read through the `state` relation, which is + # where every field of a state lives. Neither of the two was ever + # filterable, so nothing that could be saved in a segment or a scope + # depended on them. 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 @@ -104,6 +110,23 @@ def define_type_columns add_column('ticket_type_name', 'String') end + # The four reference collections a ticket points at. Every target is read + # whole in one request, so a relation resolves for a page at the price of a + # single read. + # + # Only two of them can be filtered *through*: `/tickets/search` takes a + # filter on `admin_assignee_id`, `team_assignee_id` and `ticket_type_id`, + # and none on a state id -- which the refusal names when a filter reaches + # for it, rather than letting the interface offer what the endpoint drops. + def define_relations + add_many_to_one('admin_assignee', foreign_collection: 'IntercomAdmin', foreign_key: 'admin_assignee_id') + add_many_to_one('team_assignee', foreign_collection: 'IntercomTeam', foreign_key: 'team_assignee_id') + add_many_to_one('state', foreign_collection: 'IntercomTicketState', foreign_key: 'state_id') + add_many_to_one('previous_state', foreign_collection: 'IntercomTicketState', + foreign_key: 'previous_state_id') + add_many_to_one('ticket_type', foreign_collection: 'IntercomTicketType', foreign_key: 'ticket_type_id') + end + # The attribute columns of every ticket type, in union. Read at boot by # `TicketAttributesIntrospector`, which is also where a workspace's own # name is turned into one a Forest query string can carry. An attribute @@ -118,8 +141,8 @@ def collides?(attribute) ForestAdminDatasourceIntercom.logger.warn( "[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.' + "native column or relation 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 3fb91030a..d547fefb0 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 @@ -37,10 +37,10 @@ def native(attrs) def state_of(attrs) state = attrs['ticket_state'].is_a?(Hash) ? attrs['ticket_state'] : {} + # `internal_label` is what the support team reads. The category and the + # customer-facing label are a hop away, on the `state` relation. { '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 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 f9ccf242a..c2a4c2bf0 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 @@ -30,6 +30,10 @@ def inspect def register_collections add_collection(Collections::Admin.new(self)) add_collection(Collections::Team.new(self)) + # The join Intercom does not expose: without it the membership of a team is + # an array of ids on either side, since a many-to-many needs a collection + # to travel through. + add_collection(Collections::TeamMembership.new(self)) add_collection(Collections::TicketType.new(self)) add_collection(Collections::TicketState.new(self)) add_collection(Collections::Conversation.new(self)) diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/query/condition_tree_translator.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/query/condition_tree_translator.rb index 3418a7951..aa33b085d 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/query/condition_tree_translator.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/query/condition_tree_translator.rb @@ -65,7 +65,7 @@ def translate_branch(branch, depth) # enough that a wrapper around nothing is a level worth not spending. return translate(conditions.first, depth) if conditions.size == 1 - refuse_too_deep!(depth) if depth > MAX_DEPTH + refuse_too_deep!(branch, depth) if depth > MAX_DEPTH refuse_too_wide!(branch, conditions.size) if conditions.size > MAX_GROUP_SIZE { 'operator' => operator, 'value' => conditions.map { |condition| translate(condition, depth + 1) } } @@ -75,11 +75,27 @@ def translate_branch(branch, depth) # message names the shape rather than a number, since the tree an operator # can act on is the segment and the scope they wrote, not the one the agent # assembled out of them. - def refuse_too_deep!(depth) + def refuse_too_deep!(branch, depth) raise UnsupportedOperatorError, "#{@collection} cannot answer this filter: Intercom nests a search #{MAX_DEPTH} levels deep and this " \ - "one reaches #{depth}. A group inside a group inside a group is one level too many -- flatten the " \ - 'segment, the scope or the filter carrying the innermost one.' + "one reaches #{depth}. #{deepening_cause(branch)}" + end + + # A group the operator wrote is theirs to flatten. A group a relation + # expanded into is not: it is the several records the relation matched, + # written one equality each because Intercom takes no membership operator + # on a foreign key -- and telling them to flatten a nesting they never + # wrote is a refusal they cannot act on. + def deepening_cause(branch) + field = branch.respond_to?(:relation_field) ? branch.relation_field : nil + + if field.nil? + 'A group inside a group inside a group is one level too many -- flatten the segment, the scope or the ' \ + 'filter carrying the innermost one.' + else + "The innermost group is what #{field.inspect} expanded into, one equality per record it matched: narrow " \ + 'that condition until it names a single record, or lift it out of the groups nesting it.' + end end # Fifteen is reached without trying: a scope, a segment and a filter add up, 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 index a98cfd112..9ef97e218 100644 --- 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 @@ -9,10 +9,16 @@ def filter ForestAdminDatasourceToolkit::Components::Query::Filter.new end + def json(payload, status = 200) + { status: status, body: payload.to_json, headers: { 'Content-Type' => 'application/json' } } + 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' }) + stub_request(:get, "#{base}/admins").to_return(json('type' => 'admin.list', 'admins' => admins)) + end + + def stub_teams(*teams) + stub_request(:get, "#{base}/teams").to_return(json('type' => 'team.list', 'teams' => teams)) end it 'is named IntercomAdmin' do @@ -21,7 +27,7 @@ def stub_admins(*admins) 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]) + .to eq(%w[id name email job_title away_mode_enabled away_mode_reassign has_inbox_seat team_names teams]) end it 'declares id as the primary key' do @@ -34,28 +40,80 @@ def stub_admins(*admins) 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]) + stub_teams('id' => '814865', 'name' => 'Support') 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] }]) + 'team_names' => ['Support'] }]) + end + + # A projection naming nothing asks for every declared column, which is how + # `project` reads it -- so the enrichment has to read it that way too, or the + # one column the projection publishes comes back nil. + it 'fills the derived names when the projection names no column at all' do + stub_admins('id' => '1', 'name' => 'Alice', 'team_ids' => [814_865]) + stub_teams('id' => '814865', 'name' => 'Support') + + expect(collection.list(nil, filter, nil).first['team_names']).to eq(['Support']) 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. + # itself; a filter value from Forest always arrives as a string. Both sides + # of the membership therefore carry the same id, without which the relation + # would resolve to nothing rather than to an error. 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 + stub_admins('id' => 493_881, 'team_ids' => [814_865]) + stub_teams('id' => '814865', 'name' => 'Support') - expect(row['id']).to eq('493881') - expect(row['team_ids']).to eq(%w[814865 814866]) + expect(collection.list(nil, filter, %w[id team_names]).first) + .to eq({ 'id' => '493881', 'team_names' => ['Support'] }) 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) + describe 'the teams of a teammate' do + it 'names them for the whole page in one read, rather than one read per row' do + stub_admins({ 'id' => '1', 'team_ids' => [814_865] }, { 'id' => '2', 'team_ids' => [814_865, 814_866] }) + stub_teams({ 'id' => '814865', 'name' => 'Support' }, { 'id' => '814866', 'name' => 'Billing' }) + + expect(collection.list(nil, filter, %w[id team_names])) + .to eq([{ 'id' => '1', 'team_names' => ['Support'] }, + { 'id' => '2', 'team_names' => %w[Support Billing] }]) + expect(WebMock).to have_requested(:get, "#{base}/teams").once + end + + # A column nobody asked for costs no request. + it 'reads nothing when no projection asks for the names' do + stub_admins('id' => '1', 'team_ids' => [814_865]) + + collection.list(nil, filter, %w[id name]) + + expect(WebMock).not_to have_requested(:get, "#{base}/teams") + end + + # A missing permission costs the column, never the page -- and never the + # relation, which reads the teams from the other side. + it 'leaves the names empty when the teams cannot be read' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + stub_admins('id' => '1', 'team_ids' => [814_865]) + stub_request(:get, "#{base}/teams").to_return(json({ 'type' => 'error.list' }, 403)) + + expect(collection.list(nil, filter, %w[id team_names]).first) + .to eq({ 'id' => '1', 'team_names' => [] }) + 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) + stub_teams('id' => '814865', 'name' => 'Support') + + expect(collection.list(nil, filter, %w[team_names]).first['team_names']).to eq([]) + end - expect(collection.list(nil, filter, nil).first['team_ids']).to eq([]) + it 'is a many-to-many through the membership collection, read-only' do + expect(collection.fields['teams']) + .to have_attributes(type: 'ManyToMany', foreign_collection: 'IntercomTeam', + through_collection: 'IntercomTeamMembership', + origin_key: 'admin_id', foreign_key: 'team_id', is_read_only: true) + end 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 index 3b562221b..a6cfe8db5 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 @@ -25,6 +25,12 @@ def json(payload, status = 200) { status: status, body: payload.to_json, headers: { 'Content-Type' => 'application/json' } } end + # The columns alone: a relation carries neither operators nor an order, and + # what it may be filtered through is asserted on its own below. + def columns + collection.fields.select { |_, field| field.type == 'Column' } + end + # Hand-written from the OpenAPI 2.16 spec, never captured from a workspace: # a conversation body is personal data. def conversation(id, overrides = {}) @@ -98,7 +104,7 @@ def ids(rows) # Neither search endpoint takes a sort, and Intercom ignores the one it is # sent without a word, so no column of this tier may advertise one. it 'declares every column unsortable' do - expect(collection.fields.values.map(&:is_sortable).uniq).to eq([false]) + expect(columns.values.map(&:is_sortable).uniq).to eq([false]) end # Derived from the measured table, never written by hand: a column @@ -135,7 +141,7 @@ def ids(rows) # 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]) + expect(columns.values.map(&:is_groupable).uniq).to eq([false]) end # This lot writes nothing: an editable column would offer a Save that @@ -145,6 +151,54 @@ def ids(rows) end end + # All three targets are read whole in one request, and + # `/conversations/search` takes a filter on each of the three keys -- so + # these relations can be read, navigated and filtered through alike, unlike + # the state of a ticket. + describe 'the relations to the reference collections' do + it 'points every id at the collection that reads it' do + expect(collection.fields['admin_assignee']) + .to have_attributes(type: 'ManyToOne', foreign_collection: 'IntercomAdmin', + foreign_key: 'admin_assignee_id', foreign_key_target: 'id', is_read_only: true) + expect(collection.fields['team_assignee']) + .to have_attributes(foreign_collection: 'IntercomTeam', foreign_key: 'team_assignee_id') + expect(collection.fields['closed_by']) + .to have_attributes(foreign_collection: 'IntercomAdmin', foreign_key: 'closed_by_id') + end + + # The account is on the payload as a whole object, so the name is already a + # column; the Companies collection arrives with lot 4, and a relation whose + # target is missing is a schema the agent refuses to boot on. + it 'declares no relation towards the account' do + expect(collection.fields.keys).not_to include('company') + end + + it 'nests the teammate who closed it, reading the teammates once for the page' do + stub_list(conversation('1'), conversation('2')) + stub_request(:get, "#{base}/admins") + .to_return(json('type' => 'admin.list', 'admins' => [{ 'id' => '493881', 'name' => 'Alice' }])) + + rows = collection.list(nil, filter, ['id', 'closed_by:name']) + + expect(rows).to eq([{ 'id' => '1', 'closed_by' => { 'name' => 'Alice', 'id' => '493881' } }, + { 'id' => '2', 'closed_by' => { 'name' => 'Alice', 'id' => '493881' } }]) + expect(WebMock).to have_requested(:get, "#{base}/admins").once + end + + it 'filters on the foreign key the target resolved to' do + stub_request(:get, "#{base}/admins") + .to_return(json('type' => 'admin.list', 'admins' => [{ 'id' => '493881', 'name' => 'Alice' }])) + stub_search(conversation('1')) + + collection.list(nil, filter(condition_tree: leaf('admin_assignee:name', operators::EQUAL, 'Alice')), %w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/conversations/search") + .with(query: hash_including({}), + body: hash_including('query' => { 'field' => 'admin_assignee_id', 'operator' => '=', + 'value' => '493881' })) + end + end + describe '#list' do it 'reads the listing endpoint as plain text and pages by cursor' do stub_list(conversation('1')) 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 09b5cefc0..94d03933c 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 @@ -53,6 +53,14 @@ def evaluable?(operator, column_type) .equivalent_tree?(operator, described_class::IN_MEMORY_OPERATORS, column_type) end + # A projection naming nothing asks for every declared column, `team_names` + # among them, so a list with no projection reads the teams as well. + before do + stub_request(:get, "#{base}/teams") + .to_return(status: 200, body: { 'type' => 'team.list', 'teams' => [] }.to_json, + headers: { 'Content-Type' => 'application/json' }) + 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 @@ -74,14 +82,14 @@ def evaluable?(operator, column_type) # 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']) + expect(collection.fields['team_names']) .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| + advertised = collection.fields.select { |_, field| field.type == 'Column' }.flat_map do |_name, column| column.filter_operators.map { |operator| [operator, column.column_type] } end @@ -185,10 +193,29 @@ def filtered(field, operator, value = nil) expect(ids(rows)).to eq(%w[2 1 3]) end - it 'drops a clause naming a column it does not carry' do + # An order asked for and not honoured is reported everywhere else in this + # datasource, and a related list through a many-to-many reaches this tier + # with the columns of the collection it *reaches* rather than the one it + # travels through -- `Filter#nest` prefixes the condition tree and not the + # sort. Dropped in silence, that is the one order nothing would report. + it 'drops a clause naming a column it does not carry, and says so' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + rows = collection.list(nil, filter(sort: sort({ field: 'unknown', ascending: true })), nil) expect(ids(rows)).to eq(%w[2 1 3]) + expect(ForestAdminDatasourceIntercom.logger) + .to have_received(:warn).with(/sort on "unknown", which it does not carry/) + end + + it 'keeps the clauses it does carry alongside the one it drops' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + + rows = collection.list( + nil, filter(sort: sort({ field: 'unknown', ascending: true }, { field: 'name', ascending: true })), nil + ) + + expect(ids(rows)).to eq(%w[1 2 3]) end end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_membership_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_membership_spec.rb new file mode 100644 index 000000000..656301c2a --- /dev/null +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_membership_spec.rb @@ -0,0 +1,206 @@ +module ForestAdminDatasourceIntercom + RSpec.describe Collections::TeamMembership do + subject(:collection) { datasource.get_collection('IntercomTeamMembership') } + + 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) + ForestAdminDatasourceToolkit::Components::Query::Filter.new(condition_tree: condition_tree) + end + + def leaf(field, operator, value = nil) + ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf + .new(field, operator, value) + end + + def branch(aggregator, *conditions) + ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeBranch + .new(aggregator, conditions) + end + + def json(payload, status = 200) + { status: status, body: payload.to_json, headers: { 'Content-Type' => 'application/json' } } + end + + def stub_teams(*teams) + stub_request(:get, "#{base}/teams").to_return(json('type' => 'team.list', 'teams' => teams)) + end + + def stub_admins(*admins) + stub_request(:get, "#{base}/admins").to_return(json('type' => 'admin.list', 'admins' => admins)) + end + + it 'is named IntercomTeamMembership' do + expect(collection.name).to eq('IntercomTeamMembership') + end + + it 'exposes the pair and a relation to either side of it' do + expect(collection.fields.keys).to eq(%w[id team_id admin_id team admin]) + end + + # One record per pair, keyed by both ids: a related list is read over two + # requests, and a key that changed in between would move the rows under the + # operator. + it 'reads one record per pair from the team side alone' do + stub_teams({ 'id' => '814865', 'name' => 'Support', 'admin_ids' => [493_881, 493_882] }, + { 'id' => '814866', 'name' => 'Billing', 'admin_ids' => [493_882] }) + + expect(collection.list(nil, filter, nil)) + .to eq([{ 'id' => '814865:493881', 'team_id' => '814865', 'admin_id' => '493881' }, + { 'id' => '814865:493882', 'team_id' => '814865', 'admin_id' => '493882' }, + { 'id' => '814866:493882', 'team_id' => '814866', 'admin_id' => '493882' }]) + expect(WebMock).not_to have_requested(:get, "#{base}/admins") + end + + it 'holds no record for a team nobody belongs to' do + stub_teams({ 'id' => '814865', 'admin_ids' => [] }, { 'id' => '814866', 'admin_ids' => nil }) + + expect(collection.list(nil, filter, nil)).to be_empty + end + + # Intercom types the id as a number inside `admin_ids` and as a string on the + # teammate itself: the pair carries the form the other side answers by, or + # the relation resolves to nothing. + it 'stringifies both ids' do + stub_teams('id' => 814_865, 'admin_ids' => [493_881]) + + expect(collection.list(nil, filter, nil).first) + .to eq({ 'id' => '814865:493881', 'team_id' => '814865', 'admin_id' => '493881' }) + end + + describe 'a projection through a relation' do + it 'nests the teammate under the relation, reading them once for the page' do + stub_teams('id' => '814865', 'admin_ids' => [493_881, 493_882]) + stub_admins({ 'id' => '493881', 'name' => 'Alice' }, { 'id' => '493882', 'name' => 'Bruno' }) + + expect(collection.list(nil, filter, ['id', 'admin:name'])) + .to eq([{ 'id' => '814865:493881', 'admin' => { 'name' => 'Alice', 'id' => '493881' } }, + { 'id' => '814865:493882', 'admin' => { 'name' => 'Bruno', 'id' => '493882' } }]) + expect(WebMock).to have_requested(:get, "#{base}/admins").once + end + + # A teammate who left the workspace is still named by the membership until + # Intercom drops the pair. The row reads as having no teammate rather than + # as a broken record. + it 'nests nothing when the id names no record' do + stub_teams('id' => '814865', 'admin_ids' => [493_881]) + stub_admins('id' => '493882', 'name' => 'Bruno') + + expect(collection.list(nil, filter, ['id', 'admin:name']).first) + .to eq({ 'id' => '814865:493881', 'admin' => nil }) + end + + it 'reads no relation the projection did not name' do + stub_teams('id' => '814865', 'admin_ids' => [493_881]) + + collection.list(nil, filter, %w[id team_id]) + + expect(WebMock).not_to have_requested(:get, "#{base}/admins") + end + end + + # This tier filters in memory, over every record Intercom holds, so a + # condition through a relation becomes a plain membership on the foreign key + # -- none of the limits of Intercom's search DSL apply here, nothing going + # through it. + describe 'a condition through a relation' do + it 'keeps the pairs whose teammate the target matched' do + stub_teams('id' => '814865', 'admin_ids' => [493_881, 493_882]) + stub_admins({ 'id' => '493881', 'name' => 'Alice' }, { 'id' => '493882', 'name' => 'Bruno' }) + + rows = collection.list(nil, filter(condition_tree: leaf('admin:name', operators::EQUAL, 'Alice')), %w[id]) + + expect(rows).to eq([{ 'id' => '814865:493881' }]) + end + + it 'answers nothing when the target matched no record, rather than everything' do + stub_teams('id' => '814865', 'admin_ids' => [493_881]) + stub_admins('id' => '493881', 'name' => 'Alice') + + rows = collection.list(nil, filter(condition_tree: leaf('admin:name', operators::EQUAL, 'Zoe')), %w[id]) + + expect(rows).to be_empty + end + + it 'counts the pairs a relation condition keeps, exactly' do + stub_teams('id' => '814865', 'admin_ids' => [493_881, 493_882]) + stub_admins({ 'id' => '493881', 'name' => 'Alice' }, { 'id' => '493882', 'name' => 'Bruno' }) + aggregation = ForestAdminDatasourceToolkit::Components::Query::Aggregation.new(operation: 'Count') + + rows = collection.aggregate(nil, filter(condition_tree: leaf('admin:name', operators::EQUAL, 'Alice')), + aggregation) + + expect(rows).to eq([{ 'group' => {}, 'value' => 1 }]) + end + + # The shape the agent really builds: a scope, then a segment, then the + # operator's own filter, one branch at a time. + it 'keeps a relation condition standing next to a condition on its own column' do + stub_teams({ 'id' => '814865', 'admin_ids' => [493_881] }, { 'id' => '814866', 'admin_ids' => [493_881] }) + stub_admins('id' => '493881', 'name' => 'Alice') + tree = branch('And', leaf('team_id', operators::EQUAL, '814866'), + leaf('admin:name', operators::EQUAL, 'Alice')) + + expect(collection.list(nil, filter(condition_tree: tree), %w[id])) + .to eq([{ 'id' => '814866:493881' }]) + end + + # An `and` carrying a condition nothing can satisfy matches nothing itself. + it 'answers nothing when a relation condition inside an and matches nothing' do + stub_teams('id' => '814865', 'admin_ids' => [493_881]) + stub_admins('id' => '493881', 'name' => 'Alice') + tree = branch('And', leaf('team_id', operators::EQUAL, '814865'), + leaf('admin:name', operators::EQUAL, 'Zoe')) + + expect(collection.list(nil, filter(condition_tree: tree), %w[id])).to be_empty + end + + # An `or` drops it and keeps its siblings: what the others name is still + # named. + it 'keeps the siblings of a relation condition inside an or' do + stub_teams({ 'id' => '814865', 'admin_ids' => [493_881] }, { 'id' => '814866', 'admin_ids' => [493_882] }) + stub_admins({ 'id' => '493881', 'name' => 'Alice' }, { 'id' => '493882', 'name' => 'Bruno' }) + tree = branch('Or', leaf('admin:name', operators::EQUAL, 'Zoe'), + leaf('team_id', operators::EQUAL, '814866')) + + expect(collection.list(nil, filter(condition_tree: tree), %w[id])) + .to eq([{ 'id' => '814866:493882' }]) + end + + it 'answers nothing when every branch of an or matches nothing' do + stub_teams('id' => '814865', 'admin_ids' => [493_881]) + stub_admins('id' => '493881', 'name' => 'Alice') + tree = branch('Or', leaf('admin:name', operators::EQUAL, 'Zoe'), + leaf('admin:name', operators::EQUAL, 'Yann')) + + expect(collection.list(nil, filter(condition_tree: tree), %w[id])).to be_empty + end + + # The operators a relation condition may carry are the target's own: it is + # the one that evaluates them, and the one whose refusal is worth reading. + it 'refuses an operator the target cannot evaluate, naming it' do + stub_teams('id' => '814865', 'admin_ids' => [493_881]) + stub_admins('id' => '493881', 'name' => 'Alice') + + expect { collection.list(nil, filter(condition_tree: leaf('admin:team_names', operators::EQUAL, 'x')), nil) } + .to raise_error(UnsupportedOperatorError, /IntercomAdmin cannot filter 'team_names'/) + end + + it 'refuses a relation it does not declare, naming the ones it has' do + stub_teams('id' => '814865', 'admin_ids' => [493_881]) + + expect { collection.list(nil, filter(condition_tree: leaf('owner:name', operators::EQUAL, 'x')), nil) } + .to raise_error(UnsupportedOperatorError, /"owner" is not a relation it filters through.*team, admin/m) + end + + it 'refuses a path reaching through two relations' do + stub_teams('id' => '814865', 'admin_ids' => [493_881]) + + expect { collection.list(nil, filter(condition_tree: leaf('admin:teams:name', operators::EQUAL, 'x')), nil) } + .to raise_error(UnsupportedOperatorError, /reaches through two relations/) + 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 index 91f52dce4..54c01b75a 100644 --- 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 @@ -1,41 +1,172 @@ module ForestAdminDatasourceIntercom RSpec.describe Collections::Team do - subject(:collection) { described_class.new(datasource) } + subject(:collection) { datasource.get_collection('IntercomTeam') } let(:datasource) { Datasource.new(access_token: 's3cr3t', rate_limiter: nil) } let(:base) { datasource.configuration.url } - def filter - ForestAdminDatasourceToolkit::Components::Query::Filter.new + def filter(condition_tree: nil) + ForestAdminDatasourceToolkit::Components::Query::Filter.new(condition_tree: condition_tree) + end + + def admins_named(name) + operators = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators + ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf + .new('admins:name', operators::EQUAL, name) + end + + def json(payload, status = 200) + { status: status, body: payload.to_json, headers: { 'Content-Type' => 'application/json' } } 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' }) + stub_request(:get, "#{base}/teams").to_return(json('type' => 'team.list', 'teams' => teams)) end - it 'is named IntercomTeam' do - expect(collection.name).to eq('IntercomTeam') + def stub_admins(*admins) + stub_request(:get, "#{base}/admins").to_return(json('type' => 'admin.list', 'admins' => admins)) end - it 'exposes the team and its membership' do - expect(collection.fields.keys).to eq(%w[id name admin_ids]) + it 'is named IntercomTeam' do + expect(collection.name).to eq('IntercomTeam') 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: []) + it 'exposes the team, its teammates by name, and the relation to them' do + expect(collection.fields.keys).to eq(%w[id name admin_names admins]) 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] }]) + expect(collection.list(nil, filter, %w[id name])).to eq([{ 'id' => '814865', 'name' => 'Support' }]) + end + + describe 'the teammates of a team' do + it 'names them for the whole page in one read, rather than one read per row' do + stub_teams({ 'id' => '814865', 'name' => 'Support', 'admin_ids' => [493_881, 493_882] }, + { 'id' => '814866', 'name' => 'Billing', 'admin_ids' => [493_882] }) + stub_admins({ 'id' => '493881', 'name' => 'Alice' }, { 'id' => '493882', 'name' => 'Bruno' }) + + expect(collection.list(nil, filter, %w[id admin_names])) + .to eq([{ 'id' => '814865', 'admin_names' => %w[Alice Bruno] }, + { 'id' => '814866', 'admin_names' => %w[Bruno] }]) + expect(WebMock).to have_requested(:get, "#{base}/admins").once + end + + it 'names them when the projection names no column at all' do + stub_teams('id' => '814865', 'name' => 'Support', 'admin_ids' => [493_881]) + stub_admins('id' => '493881', 'name' => 'Alice') + + expect(collection.list(nil, filter, nil).first['admin_names']).to eq(['Alice']) + end + + it 'reads nothing when no projection asks for the names' do + stub_teams('id' => '814865', 'name' => 'Support', 'admin_ids' => [493_881]) + + collection.list(nil, filter, %w[id name]) + + expect(WebMock).not_to have_requested(:get, "#{base}/admins") + end + + # A missing permission costs the column, never the page -- and never the + # relation, which reads the teammates from the other side. + it 'leaves the names empty when the teammates cannot be read' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + stub_teams('id' => '814865', 'admin_ids' => [493_881]) + stub_request(:get, "#{base}/admins").to_return(json({ 'type' => 'error.list' }, 403)) + + expect(collection.list(nil, filter, %w[id admin_names]).first) + .to eq({ 'id' => '814865', 'admin_names' => [] }) + end + end + + # Intercom carries the membership on the team and on the teammate both and + # exposes no resource for the pair, so the many-to-many travels through the + # membership collection this datasource synthesizes. + describe 'the relation to the teammates' do + it 'is a many-to-many through the membership collection' do + expect(collection.fields['admins']) + .to have_attributes(type: 'ManyToMany', foreign_collection: 'IntercomAdmin', + through_collection: 'IntercomTeamMembership', + origin_key: 'team_id', foreign_key: 'admin_id') + end + + # Intercom exposes no endpoint that writes a membership, and an editable + # relation would offer an association that could only fail. + it 'is read-only' do + expect(collection.fields['admins'].is_read_only).to be(true) + end + + # What the toolkit looks for on the collection a many-to-many travels + # through. Without the two, a related list falls back to a filter on the + # foreign collection that nothing there can answer. + it 'is reachable from both ends of the membership' do + utils = ForestAdminDatasourceToolkit::Utils::Collection + + expect(utils.get_through_target(collection, 'admins')).to eq('admin') + expect(utils.get_through_origin(collection, 'admins')).to eq('team') + end + + # A many-to-many is published unfilterable, so nothing the interface offers + # reaches here -- a scope or a segment still can, and it is refused rather + # than resolved by reading the membership once per value. + it 'refuses a condition through the membership' do + stub_teams('id' => '814865', 'admin_ids' => [493_881]) + + expect { collection.list(nil, filter(condition_tree: admins_named('Alice')), nil) } + .to raise_error(UnsupportedOperatorError, /"admins" is not a relation it filters through. It has none/) + end + + # The path a related list really takes: through the membership, whose own + # relation towards the teammate is what carries the rows back. + it 'lists the teammates of one team, through the membership' do + stub_teams({ 'id' => '814865', 'admin_ids' => [493_881] }, { 'id' => '814866', 'admin_ids' => [493_882] }) + stub_admins({ 'id' => '493881', 'name' => 'Alice' }, { 'id' => '493882', 'name' => 'Bruno' }) + query = ForestAdminDatasourceToolkit::Components::Query + + rows = ForestAdminDatasourceToolkit::Utils::Collection.list_relation( + collection, %w[814865], 'admins', nil, query::Filter.new, query::Projection.new(%w[id name]) + ) + + expect(rows).to eq([{ 'id' => '493881', 'name' => 'Alice' }]) + end + + # `admin_ids` names a teammate `/admins` does not answer -- one who left + # the workspace, one outside the token's reach, one past the page cap of a + # large workspace. The membership nests a nil under the relation, which is + # right on a row an operator reads; unwrapped into a related list it would + # be a row holding nothing, and the serializer reads a row by key. + it 'drops a teammate the workspace no longer answers, rather than listing an empty row' do + stub_teams('id' => '814865', 'admin_ids' => [493_881, 999_999]) + stub_admins('id' => '493881', 'name' => 'Alice') + query = ForestAdminDatasourceToolkit::Components::Query + + rows = ForestAdminDatasourceToolkit::Utils::Collection.list_relation( + collection, %w[814865], 'admins', nil, query::Filter.new, query::Projection.new(%w[id name]) + ) + + expect(rows).to eq([{ 'id' => '493881', 'name' => 'Alice' }]) + end + + # The membership is handed the columns of the collection the relation + # reaches, `Filter#nest` prefixing the condition tree and not the sort. It + # cannot order on them, and says so rather than answering an order it did + # not honour. + it 'reports the order it cannot honour on a related list' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + stub_teams('id' => '814865', 'admin_ids' => [493_881, 493_882]) + stub_admins({ 'id' => '493881', 'name' => 'Zoe' }, { 'id' => '493882', 'name' => 'Alice' }) + query = ForestAdminDatasourceToolkit::Components::Query + + ForestAdminDatasourceToolkit::Utils::Collection.list_relation( + collection, %w[814865], 'admins', nil, + query::Filter.new(sort: query::Sort.new([{ field: 'name', ascending: true }])), + query::Projection.new(%w[id name]) + ) + + expect(ForestAdminDatasourceIntercom.logger) + .to have_received(:warn).with(/sort on "name", which it does not carry/) + 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 index bf1b8fddb..3e5530c25 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 @@ -31,6 +31,11 @@ def leaf(field, operator, value = nil) .new(field, operator, value) end + def branch(aggregator, *conditions) + ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeBranch + .new(aggregator, conditions) + 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 = {}) @@ -64,6 +69,14 @@ def comment(at:, by: 'Alice', type: 'admin', part_type: 'comment') 'created_at' => at, 'author' => { 'type' => type, 'id' => '1', 'name' => by } } end + def stub_admins(*admins) + stub_request(:get, "#{base}/admins").to_return(json('type' => 'admin.list', 'admins' => admins)) + end + + def stub_ticket_states(*states) + stub_request(:get, "#{base}/ticket_states").to_return(json('type' => 'list', 'data' => states)) + 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 } } @@ -77,16 +90,24 @@ def rows(projection = nil, **options) collection.list(nil, filter(**options), projection) end + # The columns alone: a relation carries neither operators nor an order. + def columns + collection.fields.select { |_, field| field.type == 'Column' } + 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') + # The state travels embedded, so its label costs nothing and the row does + # not depend on IntercomTicketState to be readable. One label and not + # three: the category and the customer-facing label are a hop away, on the + # relation, and neither was ever filterable -- so no segment or scope could + # rest on them. + it 'flattens the embedded state into one label, the rest being a hop away' do + expect(collection.fields.keys).to include('state_id', 'state_label', 'previous_state_id') + expect(collection.fields.keys).not_to include('state_category', 'state_external_label') end it 'carries the attributes of every ticket type in union' do @@ -96,7 +117,7 @@ def rows(projection = nil, **options) # `/tickets/search` ignores a sort without saying so, on every column. it 'declares every column unsortable' do - expect(collection.fields.values.map(&:is_sortable).uniq).to eq([false]) + expect(columns.values.map(&:is_sortable).uniq).to eq([false]) end it 'advertises the filters the search endpoint answers, and only those' do @@ -180,7 +201,7 @@ def rows(projection = nil, **options) 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', + 'state_id' => '19', 'state_label' => 'En cours Tech', 'previous_state_id' => '14', 'ticket_type_name' => 'Bug', 'company_id' => '696dd52099f73812610d9c7b', 'admin_assignee_id' => '493881') end @@ -265,6 +286,253 @@ def rows(projection = nil, **options) end end + # Every target is read whole in one request, so a relation resolves for a + # whole page at the price of a single read -- which is what makes eight of + # them affordable at all. + describe 'the relations to the reference collections' do + it 'points every id at the collection that reads it' do + expect(collection.fields['admin_assignee']) + .to have_attributes(type: 'ManyToOne', foreign_collection: 'IntercomAdmin', + foreign_key: 'admin_assignee_id', foreign_key_target: 'id', is_read_only: true) + expect(collection.fields['team_assignee']) + .to have_attributes(foreign_collection: 'IntercomTeam', foreign_key: 'team_assignee_id') + expect(collection.fields['state']) + .to have_attributes(foreign_collection: 'IntercomTicketState', foreign_key: 'state_id') + expect(collection.fields['previous_state']) + .to have_attributes(foreign_collection: 'IntercomTicketState', foreign_key: 'previous_state_id') + expect(collection.fields['ticket_type']) + .to have_attributes(foreign_collection: 'IntercomTicketType', foreign_key: 'ticket_type_id') + end + + # Declared before the attribute columns, so a workspace attribute whose + # name lands on a relation is skipped with a warning the way one landing on + # a column is. Declared after, it would collide and take the boot with it. + it 'keeps the relation when a ticket attribute carries its name' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + + collection = described_class.new(datasource, attributes: [attribute('state')]) + + expect(collection.fields['state'].type).to eq('ManyToOne') + end + + it 'nests the assignee under the relation, reading the teammates once for the page' do + stub_search(ticket('1'), ticket('2')) + stub_admins('id' => '493881', 'name' => 'Alice', 'email' => 'alice@acme.test') + + rows = collection.list(nil, filter, ['id', 'admin_assignee:name']) + + expect(rows).to eq([{ 'id' => '1', 'admin_assignee' => { 'name' => 'Alice', 'id' => '493881' } }, + { 'id' => '2', 'admin_assignee' => { 'name' => 'Alice', 'id' => '493881' } }]) + expect(WebMock).to have_requested(:get, "#{base}/admins").once + end + + # A teammate who left the workspace: the row reads as unassigned rather + # than as a page that could not be served. + it 'nests nothing when the id names no teammate' do + stub_search(ticket('1')) + stub_admins('id' => '493882', 'name' => 'Bruno') + + expect(collection.list(nil, filter, ['id', 'admin_assignee:name']).first) + .to eq({ 'id' => '1', 'admin_assignee' => nil }) + end + + it 'reads no relation the projection did not name' do + stub_search(ticket('1')) + + collection.list(nil, filter, %w[id state_label]) + + expect(WebMock).not_to have_requested(:get, "#{base}/admins") + end + + # The price of a relation is one read per target *collection*, not one per + # relation: `state` and `previous_state` name the same endpoint, and it is + # read once, over the ids both of them point at. + it 'reads a collection two relations point at once, not once per relation' do + stub_search(ticket('1')) + stub_ticket_states({ 'id' => '19', 'internal_label' => 'En cours Tech' }, + { 'id' => '14', 'internal_label' => 'Recu' }) + + rows = collection.list(nil, filter, ['id', 'state:internal_label', 'previous_state:internal_label']) + + expect(rows.first).to eq({ 'id' => '1', + 'state' => { 'internal_label' => 'En cours Tech', 'id' => '19' }, + 'previous_state' => { 'internal_label' => 'Recu', 'id' => '14' } }) + expect(WebMock).to have_requested(:get, "#{base}/ticket_states").once + end + + # One read for two relations means one projection for two, and each of them + # still gets the columns it asked for rather than the union. + it 'nests under each relation only the columns that relation asked for' do + stub_search(ticket('1')) + stub_ticket_states({ 'id' => '19', 'category' => 'in_progress', 'internal_label' => 'En cours Tech' }, + { 'id' => '14', 'category' => 'submitted', 'internal_label' => 'Recu' }) + + rows = collection.list(nil, filter, ['id', 'state:category', 'previous_state:internal_label']) + + expect(rows.first).to eq({ 'id' => '1', + 'state' => { 'category' => 'in_progress', 'id' => '19' }, + 'previous_state' => { 'internal_label' => 'Recu', 'id' => '14' } }) + end + end + + # A relation is published filterable as soon as any column of its target is, + # so the interface offers `admin_assignee:name` the moment the relation + # exists. What Intercom is really filtered on is the foreign key: the target + # says which of its records match -- over every record it holds, not over a + # page -- and the ids it names are what the search carries. + describe 'a condition through a relation' do + it 'filters on the foreign key the target resolved to' do + stub_admins('id' => '493881', 'name' => 'Alice') + stub_search(ticket('1')) + + rows(%w[id], condition_tree: leaf('admin_assignee:name', operators::EQUAL, 'Alice')) + + expect(WebMock).to have_requested(:post, "#{base}/tickets/search") + .with(body: hash_including('query' => { 'field' => 'admin_assignee_id', 'operator' => '=', + 'value' => '493881' })) + end + + # Intercom takes no membership operator on these fields, so several matches + # become several conditions -- which is also why the group has a ceiling. + it 'writes a group of equalities when the target matched several records' do + stub_admins({ 'id' => '493881', 'name' => 'Alice' }, { 'id' => '493882', 'name' => 'Alice' }) + stub_search(ticket('1')) + + rows(%w[id], condition_tree: leaf('admin_assignee:name', operators::EQUAL, 'Alice')) + + expect(WebMock).to have_requested(:post, "#{base}/tickets/search") + .with(body: hash_including('query' => { + 'operator' => 'OR', + 'value' => [{ 'field' => 'admin_assignee_id', 'operator' => '=', + 'value' => '493881' }, + { 'field' => 'admin_assignee_id', 'operator' => '=', + 'value' => '493882' }] + })) + end + + # No row can satisfy it, and Intercom's DSL has no way of saying so: the + # search is skipped rather than sent as a filter that would come back with + # every ticket. + it 'answers nothing, and reads nothing, when the target matched no record' do + stub_admins('id' => '493881', 'name' => 'Alice') + + expect(rows(%w[id], condition_tree: leaf('admin_assignee:name', operators::EQUAL, 'Zoe'))).to be_empty + expect(WebMock).not_to have_requested(:post, "#{base}/tickets/search") + end + + it 'counts none of them either, without a request' do + stub_admins('id' => '493881', 'name' => 'Alice') + aggregation = ForestAdminDatasourceToolkit::Components::Query::Aggregation.new(operation: 'Count') + + counted = collection.aggregate(nil, filter(condition_tree: leaf('admin_assignee:name', + operators::EQUAL, 'Zoe')), aggregation) + + expect(counted).to eq([{ 'group' => {}, 'value' => 0 }]) + expect(WebMock).not_to have_requested(:post, "#{base}/tickets/search") + end + + # A relation group nested inside the tree the agent assembled: a scope, a + # segment and the operator's own filter, and the group counts as one of + # them. + it 'nests the group inside the condition it was written next to' do + stub_admins({ 'id' => '493881', 'name' => 'Alice' }, { 'id' => '493882', 'name' => 'Alice' }) + stub_search(ticket('1')) + tree = branch('And', leaf('category', operators::EQUAL, 'request'), + leaf('admin_assignee:name', operators::EQUAL, 'Alice')) + + collection.list(nil, filter(condition_tree: tree), %w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/tickets/search") + .with(body: hash_including('query' => { + 'operator' => 'AND', + 'value' => [{ 'field' => 'category', 'operator' => '=', + 'value' => 'request' }, + { 'operator' => 'OR', + 'value' => [{ 'field' => 'admin_assignee_id', + 'operator' => '=', 'value' => '493881' }, + { 'field' => 'admin_assignee_id', + 'operator' => '=', 'value' => '493882' }] }] + })) + end + + # Intercom nests a search two levels deep, and the group a relation expands + # into is a level nobody wrote. Inlined into a parent aggregating the same + # way, it costs none -- without which a scope plus a "match any" filter + # carrying one relation condition would be refused for a nesting the + # operator cannot find in their own filter. + it 'inlines the group into a parent that aggregates the same way' do + stub_admins({ 'id' => '493881', 'name' => 'Alice' }, { 'id' => '493882', 'name' => 'Alice' }) + stub_search(ticket('1')) + tree = branch('And', leaf('category', operators::EQUAL, 'request'), + branch('Or', leaf('open', operators::EQUAL, true), + leaf('admin_assignee:name', operators::EQUAL, 'Alice'))) + + collection.list(nil, filter(condition_tree: tree), %w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/tickets/search") + .with(body: hash_including('query' => { + 'operator' => 'AND', + 'value' => [{ 'field' => 'category', 'operator' => '=', + 'value' => 'request' }, + { 'operator' => 'OR', + 'value' => [{ 'field' => 'open', 'operator' => '=', + 'value' => true }, + { 'field' => 'admin_assignee_id', + 'operator' => '=', 'value' => '493881' }, + { 'field' => 'admin_assignee_id', + 'operator' => '=', 'value' => '493882' }] }] + })) + end + + # Inlining trades a level of nesting for width, and Intercom bounds both. + # Past fifteen conditions the nested form is the one that fits, so the + # group stays where it was rather than emptying the budget it was spared. + it 'leaves the group nested when inlining it would pass fifteen conditions' do + stub_admins(*(1..3).map { |index| { 'id' => index.to_s, 'name' => 'Alice' } }) + stub_search(ticket('1')) + others = (1..14).map { |index| leaf('category', operators::NOT_EQUAL, "c#{index}") } + tree = branch('Or', *others, leaf('admin_assignee:name', operators::EQUAL, 'Alice')) + + collection.list(nil, filter(condition_tree: tree), %w[id]) + + expect(WebMock).to(have_requested(:post, "#{base}/tickets/search").with do |request| + query = JSON.parse(request.body)['query'] + query['value'].size == 15 && query['value'].last['operator'] == 'OR' && + query['value'].last['value'].size == 3 + end) + end + + it 'reads nothing when a relation condition inside an and matches nothing' do + stub_admins('id' => '493881', 'name' => 'Alice') + tree = branch('And', leaf('category', operators::EQUAL, 'request'), + leaf('admin_assignee:name', operators::EQUAL, 'Zoe')) + + expect(collection.list(nil, filter(condition_tree: tree), %w[id])).to be_empty + expect(WebMock).not_to have_requested(:post, "#{base}/tickets/search") + end + + # Fifteen conditions per group is Intercom's limit, and a relation reaches + # it without trying. Refused by name rather than sent and answered with a + # 400 naming neither the limit nor the filter that hit it. + it 'refuses a relation condition matching more records than a group holds' do + stub_admins(*(1..16).map { |index| { 'id' => index.to_s, 'name' => 'Alice' } }) + + expect { rows(%w[id], condition_tree: leaf('admin_assignee:name', operators::EQUAL, 'Alice')) } + .to raise_error(UnsupportedOperatorError, /names 16 records.*15 conditions per group/m) + end + + # `/tickets/search` filters no state id -- the table carries none -- so the + # state relation is there to be read and navigated, and the message says + # which of the two it is rather than naming a column the operator never + # wrote. Refused before the target is read, a refusal that spends a request + # costing exactly what it refuses. + it 'refuses a condition through a relation the endpoint filters nothing on' do + expect { rows(%w[id], condition_tree: leaf('state:category', operators::EQUAL, 'in_progress')) } + .to raise_error(UnsupportedOperatorError, %r{resolves to "state_id", on which tickets/search takes no}) + expect(WebMock).not_to have_requested(:get, /ticket_states/) + end + end + describe '#aggregate' do it 'counts a filtered collection through the total_count of its search' do stub_search(total: 12, body: { 'query' => { 'field' => 'open', 'operator' => '=', 'value' => true } }) 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 c5387e059..91ad2f84f 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 @@ -8,11 +8,12 @@ module ForestAdminDatasourceIntercom # The reference collections come first: they are what turns an assignee id # into a teammate and a state id into a label. Conversations follow, Tickets - # next. + # next. The membership sits with the two collections it joins: a many-to-many + # needs a collection to travel through, and Intercom exposes none. it 'publishes the collections of the lot' do expect(datasource.collections.keys) - .to eq(%w[IntercomAdmin IntercomTeam IntercomTicketType IntercomTicketState IntercomConversation - IntercomTicket]) + .to eq(%w[IntercomAdmin IntercomTeam IntercomTeamMembership IntercomTicketType IntercomTicketState + IntercomConversation IntercomTicket]) end # The one read a boot performs: the attributes a workspace declares on its diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/query/condition_tree_translator_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/query/condition_tree_translator_spec.rb index b252c3867..797efa12a 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/query/condition_tree_translator_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/query/condition_tree_translator_spec.rb @@ -134,7 +134,21 @@ def leaves(count) branch('And', *leaves(2)))) expect { translate(tree) } - .to raise_error(UnsupportedOperatorError, /nests a search 2 levels deep and this one reaches 3/) + .to raise_error(UnsupportedOperatorError, + /nests a search 2 levels deep and this one reaches 3.*flatten the segment/m) + end + + # The innermost group is sometimes not one the operator wrote: a relation + # expands into one equality per record it matched, and telling them to + # flatten a nesting they never wrote is a refusal they cannot act on. + it 'names the relation when the innermost group is what one expanded into' do + relation_group = Collections::Relations::RelationBranch.new('Or', leaves(2), 'admin_assignee:name') + tree = branch('Or', leaf('open', operators::EQUAL, true), + branch('And', leaf('read', operators::EQUAL, true), relation_group)) + + expect { translate(tree) } + .to raise_error(UnsupportedOperatorError, + /"admin_assignee:name" expanded into, one equality per record it matched/) end # A branch carrying a single condition is unwrapped, so it spends no level: diff --git a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb index 537cdf97c..916d9884c 100644 --- a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb +++ b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb @@ -56,10 +56,15 @@ def self.other_inverse?(field, relation_field) field.origin_key == relation_field.foreign_key end + # ValidationError rather than a bare ForestException, for the same reason as + # below: every one of these three names a field the request asked for -- a + # filter, a sort, a scope written against another schema, a condition + # reaching through a relation that is not one this can traverse. That is a + # 400 the caller can read and fix, not a 500 saying the agent broke. def self.get_field_schema(collection, field_name) fields = collection.schema[:fields] unless field_name.include?(':') - raise ForestException, "Column not found #{collection.name}.#{field_name}" unless fields.key?(field_name) + raise ValidationError, "Column not found #{collection.name}.#{field_name}" unless fields.key?(field_name) return fields[field_name] end @@ -67,10 +72,11 @@ def self.get_field_schema(collection, field_name) association_name = field_name.split(':')[0] relation_schema = fields[association_name] - raise ForestException, "Relation not found #{collection.name}.#{association_name}" unless relation_schema + raise ValidationError, "Relation not found #{collection.name}.#{association_name}" unless relation_schema if relation_schema.type != 'ManyToOne' && relation_schema.type != 'OneToOne' - raise ForestException, "Unexpected field type #{relation_schema.type}: #{collection.name}.#{association_name}" + raise ValidationError, + "Unexpected field type #{relation_schema.type}: #{collection.name}.#{association_name}" end get_field_schema( @@ -161,7 +167,14 @@ def self.list_relation(collection, primary_key_values, relation_name, caller, fo projection.nest(prefix: foreign_relation) ) - return records.map { |r| r[foreign_relation] } + # Compacted: a through row whose target the foreign collection no + # longer answers -- a record deleted, outside the caller's reach, or + # dropped by a datasource that reads its targets in bounded pages -- + # carries a nil where a record was expected, and every consumer of a + # related list down to the JSON:API serializer reads a row by key. + # A join whose other side is gone yields no row, it does not yield an + # empty one. + return records.filter_map { |r| r[foreign_relation] } end end diff --git a/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/collection_spec.rb b/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/collection_spec.rb index fb7d88535..ce0036c25 100644 --- a/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/collection_spec.rb +++ b/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/collection_spec.rb @@ -221,7 +221,7 @@ module Utils expect do described_class.get_field_schema(collection_person, 'foo') - end.to raise_error(ForestException, 'Column not found Person.foo') + end.to raise_error(ValidationError, 'Column not found Person.foo') end it 'get_field_schema should work with simple column' do @@ -233,14 +233,14 @@ module Utils expect do described_class.get_field_schema(collection_person, 'unknown:foo') - end.to raise_error(ForestException, 'Relation not found Person.unknown') + end.to raise_error(ValidationError, 'Relation not found Person.unknown') end it 'get_field_schema should throw with invalid relation type' do expect do described_class.get_field_schema(collection_book, 'myBookPersons:bookId') - end.to raise_error(ForestException, 'Unexpected field type OneToMany: Book.myBookPersons') + end.to raise_error(ValidationError, 'Unexpected field type OneToMany: Book.myBookPersons') end it 'get_field_schema should work with relation column' do @@ -311,6 +311,25 @@ module Utils ForestAdminDatasourceToolkit::Components::Query::Projection.new)).to eq([1]) end + # A through row whose target the foreign collection no longer answers -- + # deleted, outside the caller's reach, or dropped by a datasource that + # reads its targets in bounded pages -- carries a nil where a record was + # expected, and every consumer down to the JSON:API serializer reads a + # row by key. A join whose other side is gone yields no row. + it 'list_relation should drop a through row whose target resolved to nothing' do + book_person_class = Struct.new(:bookId, :personId, :myPerson, :myBook) + stub_const('BookPerson', book_person_class) + allow(collection_book_person).to receive(:list).and_return( + [ + BookPerson.new(1, 1, 1, 1), + BookPerson.new(1, 2, nil, 1) + ] + ) + + expect(described_class.list_relation(collection_book, [1], 'myPersons', caller, ForestAdminDatasourceToolkit::Components::Query::Filter.new, + ForestAdminDatasourceToolkit::Components::Query::Projection.new)).to eq([1]) + end + it 'aggregate_relation should work with one to many relation' do allow(collection_book_person).to receive(:aggregate).and_return(1)