diff --git a/packages/forest_admin_datasource_pylon/README.md b/packages/forest_admin_datasource_pylon/README.md index de565ab19..a216f8c0e 100644 --- a/packages/forest_admin_datasource_pylon/README.md +++ b/packages/forest_admin_datasource_pylon/README.md @@ -169,6 +169,61 @@ The message body is HTML the operator writes and Pylon delivers to the requester requester address is a free-text field. Restrict both actions to the roles that should be able to send mail on your organization's behalf. +### `ReplyToIssue` + +Answers one issue on the channel the customer used — a Slack issue gets a Slack reply, an email +issue an email — or posts an internal note on it. Single scope only. + +```ruby +@agent.collection :Ticket do |collection| + collection.use(ForestAdminDatasourcePylon::Plugins::ReplyToIssue, { + datasource: pylon_datasource, # required + issue_id_field: 'pylon_issue_id', # omit when the action sits on PylonIssue itself + action_name: 'Reply to Pylon issue', # default + allow_internal_note: true, # default + support_emails: ['support@example.com'], + post_as_operator: false # default + }) +end +``` + +**Which channel.** Pylon delivers a reply into the conversation of the message it names, so the +action picks that message: the channel is the one of the first customer-visible message, and the +reply answers the latest customer-visible message on that channel. The form shows the channel +read-only. An issue with no customer-visible message — one created as an internal note, for +instance — cannot get a public reply; only the internal note is offered. + +**Email recipients.** Pylon does not infer them, so the form pre-fills To and CC from the latest +email of the conversation. When the customer wrote it, To is its sender only: the addresses the +customer wrote to include your support inbox. When the team wrote it, To is whom it went to. The +`support_emails` and the addresses the team wrote from in the thread are always left out, from what +the operator types as well, and the result message names any typed address left out. Set +`support_emails`: before the team's first email in a thread, it is the only way to know your own +addresses. A reply with no To is refused. + +**When Pylon does not confirm.** A 4xx is shown with Pylon's own reason. Any other failure, a +timeout included, may come after Pylon delivered the message, so the action says so and asks to +check the issue before sending again. + +**Who signs.** By default the account owning the API token signs every reply, and the success +message says so. With `post_as_operator: true`, a public reply is signed by the active Pylon user +whose email matches the Forest user's, primary or secondary address. With no such user, or several, +the reply is refused and nothing is sent: it never falls back to the token. Internal notes are +always signed by the token. + +Three things to know before granting it. + +- **The column is the authority.** With `issue_id_field`, as with `CloseIssue`, an operator who can + write that column can reply to any Pylon issue, on your organization's behalf. +- **`post_as_operator` trusts emails.** The token can sign as any agent of the workspace; the only + thing tying a Forest user to a Pylon user is that they share an email. Keep both user lists + consistent, and restrict the action to the roles that should answer customers. +- **The thread is read on every form render**, one `GET /issues/{id}/messages` (120 a minute). + A submit costs one or two more: the agent renders the form again before running the action, and + a public reply reads the thread once more to answer its latest state, plus one `GET /users` with + `post_as_operator`. An unreadable thread is shown as an error, never as an issue without customer + message: both reply types stay offered, and an internal note does not depend on the thread. + ## Development ```bash diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client.rb index 6a315cbf8..f6e4104ef 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client.rb @@ -53,11 +53,19 @@ def fetch_issue(id) # when asked to, so a future default page size stays handled rather than # silently truncating the thread. def fetch_issue_messages(issue_id) - path = "issues/#{Faraday::Utils.escape(issue_id)}/messages" + path = issue_messages_path(issue_id) best_effort("fetch_issue_messages(#{issue_id})", default: nil) { must_succeed(path) { collect_pages(path) } } end + # For a caller acting on the thread: a thread that could not be read, or + # only in part, raises instead of reading as a conversation without it. + def fetch_complete_issue_messages(issue_id) + path = issue_messages_path(issue_id) + + must_succeed(path) { collect_pages(path, refuse_past_cap: true) } + end + def search_accounts(limit:, cursor: nil, filter: nil, search_text: nil) search_resource('accounts/search', limit: limit, cursor: cursor, filter: filter, search_text: search_text) end @@ -144,6 +152,8 @@ def inspect private + def issue_messages_path(issue_id) = "issues/#{Faraday::Utils.escape(issue_id)}/messages" + def search_resource(path, limit:, cursor: nil, filter: nil, search_text: nil) body = { 'limit' => clamp_limit(limit) } body['cursor'] = cursor unless blank?(cursor) @@ -168,12 +178,14 @@ def list_resource(path, limit:, cursor: nil) # so the walk costs the single request it always did and only does something # the day that changes. # - # Past the cap the walk is refused rather than truncated, unlike the thread - # and the custom fields: `FetchAllCollection` filters, sorts, counts and - # groups in memory over what this returns, and calls the result exact - # BECAUSE it is every record Pylon holds. Truncated, that claim would stand - # over a fraction of the collection -- a count answered short and presented - # as exact, which is the one answer this datasource refuses everywhere else. + # Past the cap the walk is refused rather than truncated, like the thread a + # reply acts on (`fetch_complete_issue_messages`) and unlike the thread a + # page shows and the custom fields: `FetchAllCollection` filters, sorts, + # counts and groups in memory over what this returns, and calls the result + # exact BECAUSE it is every record Pylon holds. Truncated, that claim would + # stand over a fraction of the collection -- a count answered short and + # presented as exact, which is the one answer this datasource refuses + # everywhere else. def fetch_all(path, params = {}) must_succeed(path) { collect_pages(path, params, refuse_past_cap: true) } end @@ -193,7 +205,8 @@ def fetch_all(path, params = {}) # Only `next_cursor` being nil is Pylon saying there is nothing more. Every # other stop leaves records behind, and `refuse_past_cap` is which of the two # answers the caller wants to those: a warning and a short list, or a refusal. - # See `fetch_all`, whose answer is only correct whole. + # See `fetch_all`, whose answer is only correct whole, and + # `fetch_complete_issue_messages`, whose caller acts on the latest message. # # `conn` is what a caller reading on the boot path hands its own connection # through: every page of the walk is then bounded like the first. @@ -241,10 +254,8 @@ def truncation_reason(page, pages, seen) def refuse_truncated_walk(path, reason, pages, collected) raise APIError, "Pylon left the walk of #{path} short after #{pages} page(s) / #{collected} record(s): #{reason}. " \ - 'That endpoint is documented as answering with the whole collection in one response, which is what ' \ - 'PylonUser and PylonTeam filter, sort and count in memory as an exact answer. Refusing rather ' \ - 'than answering over a fraction of the collection: reading it needs cursor pagination, the way ' \ - 'PylonIssue reads its own.' + 'That endpoint is documented as answering with the whole collection in one response, and its ' \ + 'caller needs all of it. Refusing rather than answering over a fraction of it.' end def log_truncated_walk(path, reason, pages, collected) diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client/writes.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client/writes.rb index 9a50e9cb9..a4e2d8045 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client/writes.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client/writes.rb @@ -29,8 +29,23 @@ def update_team(id, attributes) = patch_resource('teams', id, attributes) def update_user(id, attributes) = patch_resource('users', id, attributes) + # `message_id` picks the customer conversation the reply is delivered to. + def reply_to_issue(id, attributes) = post_message(issue_path(id, 'reply'), attributes) + def create_issue_note(id, attributes) = post_message(issue_path(id, 'note'), attributes) + private + def issue_path(id, action) = "issues/#{Faraday::Utils.escape(id)}/#{action}" + + # A 2xx means Pylon delivered the message, whatever the body says: + # refusing an unexpected body would invite the operator to send it twice. + def post_message(path, attributes) + must_succeed("create(#{path})") do + body = connection.post(path, attributes).body + body['data'] if body.is_a?(Hash) && body['data'].is_a?(Hash) + end + end + def post_resource(resource, attributes) operation = "create(#{resource})" diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue.rb index 8367d47ce..ba15e905e 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue.rb @@ -40,7 +40,10 @@ class Issue < BaseCollection 'author_email' => 'String', 'author_avatar_url' => 'String', 'author_contact_id' => 'String', - 'author_user_id' => 'String' + 'author_user_id' => 'String', + 'email_from' => 'String', + 'email_to' => 'Json', + 'email_cc' => 'Json' }.freeze # One thread is one `GET /issues/{id}/messages`, and a thread is the whole diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/messages_embedder.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/messages_embedder.rb index 758e85f9a..1ab963cf7 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/messages_embedder.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/messages_embedder.rb @@ -5,7 +5,7 @@ class Issue < BaseCollection # way the Zendesk datasource embeds a ticket's comments. # # Pylon has no way to read the threads of several issues at once, so the - # thread costs one request per row — against an endpoint allowing 20 per + # thread costs one request per row — against an endpoint allowing 120 per # minute. The fan-out is therefore bounded like the primary-key lookups of # this collection: truncated with a warning rather than turned into a rate # limit error halfway through the page. A relation the projection does not @@ -49,7 +49,19 @@ def serialize_message(message) 'thread_id' => attrs['thread_id'], 'file_urls' => attrs['file_urls'], 'created_at' => attrs['timestamp'] - }.merge(flatten_author(attrs['author'])) + }.merge(flatten_author(attrs['author'])).merge(flatten_email(attrs['email_info'])) + end + + # Null outside email. `bcc_emails` and the RFC 5322 `message_id` header + # are left out: neither is something a reply should be built from. + def flatten_email(email_info) + attrs = email_info.is_a?(Hash) ? email_info : {} + + { + 'email_from' => attrs['from_email'], + 'email_to' => attrs['to_emails'], + 'email_cc' => attrs['cc_emails'] + } end # Pylon nests the author's contact and user sides side by side, both diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue.rb new file mode 100644 index 000000000..f0505bac8 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue.rb @@ -0,0 +1,142 @@ +require 'cgi' + +module ForestAdminDatasourcePylon + module Plugins + # Answers a Pylon issue on the channel the customer used, or posts an + # internal note on it. See the README for the channel rule and who signs. + class ReplyToIssue < ForestAdminDatasourceCustomizer::Plugins::Plugin + BaseAction = ForestAdminDatasourceCustomizer::Decorators::Action::BaseAction + ActionScope = ForestAdminDatasourceCustomizer::Decorators::Action::Types::ActionScope + ForestException = ForestAdminDatasourceToolkit::Exceptions::ForestException + + NAME = 'Reply to Pylon issue'.freeze + + def run(_datasource_customizer, collection_customizer = nil, options = {}) + options = {} unless options.is_a?(Hash) + datasource = options[:datasource] + raise ForestException, 'ReplyToIssue plugin requires :datasource' unless datasource + raise ForestException, 'ReplyToIssue plugin requires a collection' unless collection_customizer + + opts = { + issue_id_field: options[:issue_id_field], + allow_internal_note: options.fetch(:allow_internal_note, true) ? true : false, + post_as_operator: options[:post_as_operator] ? true : false, + support_emails: normalize_emails(options[:support_emails]) + } + + collection_customizer.add_action((options[:action_name] || NAME).to_s, build_action(datasource, opts)) + end + + private + + def normalize_emails(value) + Array(value).map { |email| email.to_s.strip.downcase }.reject(&:empty?).uniq + end + + def build_action(datasource, opts) + BaseAction.new(scope: ActionScope::SINGLE, form: FormBuilder.build(datasource, opts), + &executor(datasource, opts)) + end + + # A note needs the issue only. A reply reads the thread again rather than + # trusting the form: the conversation may have moved since it rendered. + def executor(datasource, opts) + lambda do |context, result_builder| + issue_id = ThreadLookup.issue_id(context, opts) + next result_builder.error(message: ThreadLookup.no_issue(opts[:issue_id_field])) if issue_id.nil? + + body = context.form_values[FormBuilder::MESSAGE_LABEL].to_s + next result_builder.error(message: 'A message is required.') if blank_html?(body) + + status, message = if FormBuilder.public_reply?(context, opts) + reply(datasource, ThreadLookup.read(datasource, issue_id, opts), body, context, opts) + else + note(datasource, issue_id, body) + end + result_builder.public_send(status, message: message) + end + end + + # An emptied rich-text editor still submits markup such as `


`. + def blank_html?(body) + return false if body.match?(/]*>/, ' ').gsub(/ /i, ' ')) + text.gsub(/[[:space:]\u200B-\u200D\u2060\uFEFF]/, '').empty? + end + + def reply(datasource, state, body, context, opts) + return [:error, state.error] if state.error + return [:error, no_target_message(state.issue_id, opts)] if state.target.nil? + + payload, dropped, refusal = reply_payload(state.target, body, context) + return [:error, refusal] if refusal + + operator = OperatorLookup.call(datasource, context.caller) if opts[:post_as_operator] + return [:error, operator.error] if operator&.error + + payload['user_id'] = operator.user['id'] if operator + write(state.issue_id, 'reply', reply_success(state, payload, operator&.user) + left_out(dropped)) do + datasource.client.reply_to_issue(state.issue_id, payload) + end + end + + def reply_payload(target, body, context) + payload = { 'body_html' => body, 'message_id' => target.message_id } + return [payload, []] unless target.email? + + to, cc, dropped = target.sanitize(to: context.form_values[FormBuilder::TO_LABEL], + cc: context.form_values[FormBuilder::CC_LABEL]) + refusal = "At least one recipient (To) is required to reply by email.#{left_out(dropped)}" if to.empty? + return [nil, dropped, refusal] if refusal + + [payload.merge('email_info' => { 'to_emails' => to, 'cc_emails' => cc }), dropped] + end + + def left_out(dropped) + dropped.empty? ? '' : " Left out as addresses of your team: #{dropped.join(", ")}." + end + + def note(datasource, issue_id, body) + write(issue_id, 'internal note', "Internal note added to Pylon issue #{issue_id}.") do + datasource.client.create_issue_note(issue_id, 'body_html' => body) + end + end + + # A 4xx is Pylon naming what is wrong, handed over as it is. Anything else + # may have failed after Pylon delivered the message, so the operator is + # told to check before sending it again rather than shown a bare failure. + def write(issue_id, what, success) + yield + [:success, success] + rescue APIError => e + ForestAdminDatasourcePylon.logger.warn( + "[forest_admin_datasource_pylon] #{what} on issue #{issue_id} failed: #{e.message}" + ) + return [:error, e.message] if (400..499).cover?(e.status.to_i) + + [:error, "Pylon did not confirm the #{what} on issue #{issue_id} (#{e.message}). It may have been " \ + 'delivered: check the issue in Pylon before sending it again.'] + end + + def no_target_message(issue_id, opts) + message = "Pylon issue #{issue_id} has no customer-visible message, so it cannot get a public reply." + opts[:allow_internal_note] ? "#{message} Post an internal note instead." : message + end + + def reply_success(state, payload, operator) + signed = if operator + "signed by #{[operator["name"], operator["email"]].find { |v| !v.to_s.strip.empty? }}" + else + 'signed by the account of the Pylon API token' + end + if payload['email_info'] + return "Reply emailed to #{payload["email_info"]["to_emails"].join(", ")} on Pylon issue " \ + "#{state.issue_id}, #{signed}." + end + + "Reply sent via #{state.target.channel_label} on Pylon issue #{state.issue_id}, #{signed}." + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue/form_builder.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue/form_builder.rb new file mode 100644 index 000000000..90ada283a --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue/form_builder.rb @@ -0,0 +1,88 @@ +module ForestAdminDatasourcePylon + module Plugins + class ReplyToIssue + # Every dynamic property of the form reads the same ThreadLookup state, + # kept on the render's context so it lives exactly as long as the render. + # A WeakMap would let the state be collected mid-render. + module FormBuilder + FieldType = ForestAdminDatasourceCustomizer::Decorators::Action::Types::FieldType + + TYPE_LABEL = 'Reply type'.freeze + CHANNEL_LABEL = 'Channel'.freeze + TO_LABEL = 'To'.freeze + CC_LABEL = 'CC'.freeze + MESSAGE_LABEL = 'Message'.freeze + + PUBLIC_REPLY = 'Public reply'.freeze + INTERNAL_NOTE = 'Internal note'.freeze + + module_function + + def build(datasource, opts) + ivar = :"@forest_admin_datasource_pylon_reply_state_#{opts.object_id}" + state = lambda do |context| + next context.instance_variable_get(ivar) if context.instance_variable_defined?(ivar) + + context.instance_variable_set(ivar, ThreadLookup.call(datasource, context, opts)) + end + + fields = [] + fields << type_field(state) if opts[:allow_internal_note] + fields << channel_field(state, opts) + fields << recipients_field(TO_LABEL, state, opts, :to) + fields << recipients_field(CC_LABEL, state, opts, :cc) + fields << message_field + end + + def public_reply?(context, opts) + !opts[:allow_internal_note] || context.get_form_value(TYPE_LABEL) == PUBLIC_REPLY + end + + def type_field(state) + { type: FieldType::ENUM, label: TYPE_LABEL, is_required: true, + enum_values: lambda { |context| + reply_possible?(state.call(context)) ? [PUBLIC_REPLY, INTERNAL_NOTE] : [INTERNAL_NOTE] + }, + default_value: ->(context) { state.call(context).target ? PUBLIC_REPLY : INTERNAL_NOTE }, + description: 'A public reply reaches the customer; an internal note is seen by your team only.' } + end + + # A thread that could not be read may well have a customer message: the + # public reply stays offered, not picked, and picking it renders the + # form again, which reads the thread again. + def reply_possible?(state) + !state.target.nil? || !state.error.nil? + end + + def channel_field(state, opts) + { type: FieldType::STRING, label: CHANNEL_LABEL, is_read_only: true, + if_condition: ->(context) { public_reply?(context, opts) || state.call(context).target.nil? }, + value: ->(context) { channel_text(state.call(context), opts) }, + description: 'Pylon delivers the reply on the channel the customer used. It cannot be changed here.' } + end + + def channel_text(state, opts) + return state.error if state.error + return "Replies via #{state.target.channel_label}" if state.target + + no_target = 'This issue has no customer-visible message, so it cannot get a public reply.' + opts[:allow_internal_note] ? "#{no_target} Post an internal note instead." : no_target + end + + def recipients_field(label, state, opts, side) + { type: FieldType::STRING_LIST, label: label, is_required: side == :to, + if_condition: lambda { |context| + target = state.call(context).target + public_reply?(context, opts) && !target.nil? && target.email? + }, + default_value: ->(context) { state.call(context).target&.public_send(side) || [] }, + description: 'Pre-filled from the latest email of the conversation, support addresses left out.' } + end + + def message_field + { type: FieldType::STRING, label: MESSAGE_LABEL, widget: 'RichText', is_required: true } + end + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue/operator_lookup.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue/operator_lookup.rb new file mode 100644 index 000000000..331bdfeba --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue/operator_lookup.rb @@ -0,0 +1,47 @@ +module ForestAdminDatasourcePylon + module Plugins + class ReplyToIssue + # The active Pylon user a reply is signed as, matched on the email of the + # Forest user running the action. Anything short of exactly one match + # refuses the reply rather than letting the token sign it instead. + module OperatorLookup + Result = Struct.new(:user, :error, keyword_init: true) + + module_function + + def call(datasource, caller) + email = caller.respond_to?(:email) ? caller.email.to_s.strip.downcase : '' + return Result.new(error: 'Your Forest user has no email to sign the reply with.') if email.empty? + + users = datasource.client.fetch_users(include_deactivated: false) + matches = users.select { |user| active?(user) && addresses(user).include?(email) } + return Result.new(error: refusal(email, matches.size)) unless matches.size == 1 + return Result.new(user: matches.first) unless matches.first['id'].to_s.strip.empty? + + Result.new(error: "The Pylon user with the email #{email} has no id, so the reply cannot be signed in " \ + 'your name and was not sent.') + rescue APIError => e + Result.new(error: "Could not read the Pylon users to sign the reply in your name: #{e.message}") + end + + def active?(user) + user.is_a?(Hash) && user['is_deactivated'] != true + end + + def addresses(user) + [user['email'], *Array(user['emails'])].map { |address| address.to_s.strip.downcase } + end + + def refusal(email, count) + if count.zero? + return "No active Pylon user has the email #{email}, so the reply cannot be signed in your name and " \ + 'was not sent. Use the same email in Forest and Pylon.' + end + + "#{count} active Pylon users have the email #{email}, so the reply cannot be signed in your name and " \ + 'was not sent.' + end + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue/reply_target.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue/reply_target.rb new file mode 100644 index 000000000..89a104dbe --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue/reply_target.rb @@ -0,0 +1,86 @@ +module ForestAdminDatasourcePylon + module Plugins + class ReplyToIssue + # The customer-visible message a public reply answers. The channel is the + # `source` of the first such message; the reply goes to the latest message + # on that channel, so an email answers the latest recipients rather than + # those of the opening message. + class ReplyTarget + EMAIL_SOURCE = 'email'.freeze + CHANNEL_LABELS = { 'email' => 'email', 'slack' => 'Slack' }.freeze + + attr_reader :message_id, :source, :to, :cc + + def self.resolve(messages, support_emails: []) + visible = Array(messages).select { |message| message.is_a?(Hash) && message['is_private'] != true } + return nil if visible.empty? + + source = visible.first['source'] + excluded = support_emails.map(&:downcase) + team_addresses(visible) + new(visible.reverse.find { |message| message['source'] == source }, excluded) + end + + # The addresses the team itself wrote from, read off the thread: what + # keeps a reply out of the support inbox when `support_emails` misses one. + def self.team_addresses(messages) + messages.select { |message| team_authored?(message) } + .map { |message| email_info(message)['from_email'].to_s.strip.downcase } + .reject(&:empty?) + end + + def self.email_info(message) + message['email_info'].is_a?(Hash) ? message['email_info'] : {} + end + + def self.team_authored?(message) + author = message['author'] + author.is_a?(Hash) && author['user'].is_a?(Hash) + end + + def self.unique_addresses(addresses, excluded = []) + addresses.map { |address| address.to_s.strip } + .reject { |address| address.empty? || excluded.include?(address.downcase) } + .uniq(&:downcase) + end + + def initialize(message, excluded) + @message_id = message['id'] + @source = message['source'] + @email_info = self.class.email_info(message) + @excluded = excluded + @to, @cc = sanitize(to: default_to(message), cc: @email_info['cc_emails']) + end + + # The excluded addresses come back apart, so an operator who typed one + # is told it was left out rather than finding it silently gone. + def sanitize(to:, cc:) + to_kept = self.class.unique_addresses(Array(to), @excluded) + cc_kept = self.class.unique_addresses(Array(cc), @excluded + to_kept.map(&:downcase)) + dropped = (Array(to) + Array(cc)).map { |address| address.to_s.strip } + .select { |address| @excluded.include?(address.downcase) } + .uniq(&:downcase) + [to_kept, cc_kept, dropped] + end + + def email? + @source == EMAIL_SOURCE || !@email_info['from_email'].to_s.empty? || Array(@email_info['to_emails']).any? + end + + def channel_label + return CHANNEL_LABELS[EMAIL_SOURCE] if email? + + CHANNEL_LABELS.fetch(@source.to_s) { @source.to_s.empty? ? 'an unnamed channel' : @source.to_s.tr('_', ' ') } + end + + private + + # Answering a customer's email writes back to its sender only: its own + # To is where the customer wrote, the support inbox among them. Answering + # the team's last email writes again to whom it went. + def default_to(message) + self.class.team_authored?(message) ? @email_info['to_emails'] : [@email_info['from_email']] + end + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue/thread_lookup.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue/thread_lookup.rb new file mode 100644 index 000000000..c4d754f8a --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue/thread_lookup.rb @@ -0,0 +1,37 @@ +module ForestAdminDatasourcePylon + module Plugins + class ReplyToIssue + # The issue an action targets and the reply target its thread resolves to, + # or why neither could be had. `error` set means `target` is not. + module ThreadLookup + State = Struct.new(:issue_id, :target, :error, keyword_init: true) + + module_function + + def call(datasource, context, opts) + issue_id = issue_id(context, opts) + return State.new(error: no_issue(opts[:issue_id_field])) if issue_id.nil? + + read(datasource, issue_id, opts) + end + + def issue_id(context, opts) + IssueTargets.resolve_issue_ids(context, opts[:issue_id_field]).first + end + + def read(datasource, issue_id, opts) + messages = datasource.client.fetch_complete_issue_messages(issue_id) + State.new(issue_id: issue_id, + target: ReplyTarget.resolve(messages, support_emails: opts[:support_emails])) + rescue APIError => e + State.new(issue_id: issue_id, error: "Could not read the conversation of Pylon issue #{issue_id}: " \ + "#{e.message}") + end + + def no_issue(field) + field.nil? ? 'No Pylon issue selected.' : "No Pylon issue id found in '#{field}'." + end + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/rate_limits.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/rate_limits.rb index 0dd780785..d687da1b3 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/rate_limits.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/rate_limits.rb @@ -40,6 +40,8 @@ class RateLimits ['get /issues/:id/messages', :get, %r{\A/issues/#{ID}/messages\z}, 120], ['get /issues', :get, %r{\A/issues\z}, 30], ['post /issues', :post, %r{\A/issues\z}, 30], + ['post /issues/:id/reply', :post, %r{\A/issues/#{ID}/reply\z}, 30], + ['post /issues/:id/note', :post, %r{\A/issues/#{ID}/note\z}, 30], ['get /issues/:id', :get, %r{\A/issues/#{ID}\z}, 300], ['patch /issues/:id', :patch, %r{\A/issues/#{ID}\z}, 120], ['get /accounts', :get, %r{\A/accounts\z}, 300], diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client/writes_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client/writes_spec.rb index 4b0e8fe72..a0b6a5f51 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client/writes_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client/writes_spec.rb @@ -36,6 +36,32 @@ def json(payload, status = 200) end end + { reply_to_issue: 'reply', create_issue_note: 'note' }.each do |method, action| + it "#{method} posts to /issues/{id}/#{action}" do + url = "#{base}/issues/i1/#{action}" + stub_request(:post, url).to_return(json('data' => { 'id' => 'm1', 'issue_id' => 'i1' })) + + expect(client.public_send(method, 'i1', 'body_html' => '

Hi

')).to eq('id' => 'm1', 'issue_id' => 'i1') + expect(WebMock).to have_requested(:post, url).with(body: { 'body_html' => '

Hi

' }) + end + + it "#{method} escapes the issue id" do + stub_request(:post, "#{base}/issues/a%2Fb/#{action}").to_return(json('data' => { 'id' => 'm1' })) + + client.public_send(method, 'a/b', 'body_html' => '

Hi

') + + expect(WebMock).to have_requested(:post, "#{base}/issues/a%2Fb/#{action}") + end + + # Pylon delivered the message once it answers 2xx: refusing the body + # would invite a second send. + it "#{method} accepts a 2xx whose body carries no record" do + stub_request(:post, "#{base}/issues/i1/#{action}").to_return(status: 204) + + expect(client.public_send(method, 'i1', 'body_html' => '

Hi

')).to be_nil + end + end + { delete_issue: 'issues', delete_account: 'accounts', delete_contact: 'contacts' }.each do |method, path| it "#{method} deletes /#{path}/{id}" do stub_request(:delete, "#{base}/#{path}/x1").to_return(status: 204) @@ -136,6 +162,32 @@ def json(payload, status = 200) expect(WebMock).to have_requested(:post, "#{base}/issues").once end + # A reply that reached the customer and is replayed is a message sent twice. + it 'does not retry a reply that failed on a gateway error' do + stub_request(:post, "#{base}/issues/i1/reply").to_return(json({ 'message' => 'bad gateway' }, 502)) + + expect { client.reply_to_issue('i1', 'body_html' => '

Hi

') } + .to raise_error(ForestAdminDatasourcePylon::APIError, %r{create\(issues/i1/reply\).*502}m) + expect(WebMock).to have_requested(:post, "#{base}/issues/i1/reply").once + end + + it 'does not retry a reply that timed out, and raises with no status' do + stub_request(:post, "#{base}/issues/i1/reply").to_timeout + + expect { client.reply_to_issue('i1', 'body_html' => '

Hi

') } + .to raise_error(ForestAdminDatasourcePylon::APIError) { |error| expect(error.status).to be_nil } + expect(WebMock).to have_requested(:post, "#{base}/issues/i1/reply").once + end + + it 'retries a rate-limited reply' do + stub_request(:post, "#{base}/issues/i1/reply") + .to_return(json({ 'message' => 'slow down' }, 429)) + .then.to_return(json('data' => { 'id' => 'm1' })) + + expect(client.reply_to_issue('i1', 'body_html' => '

Hi

')).to eq('id' => 'm1') + expect(WebMock).to have_requested(:post, "#{base}/issues/i1/reply").twice + end + it 'retries a rate-limited delete' do stub_request(:delete, "#{base}/issues/i1") .to_return(json({ 'message' => 'slow down' }, 429)) diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client_spec.rb index 0337f924d..9b67c31dc 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client_spec.rb @@ -352,6 +352,53 @@ def page(records, cursor: nil) end end + describe '#fetch_complete_issue_messages' do + def page(records, cursor: nil) + body = { 'data' => records } + body['pagination'] = { 'cursor' => cursor, 'has_next_page' => true } if cursor + json(body) + end + + it 'returns the whole thread of an issue' do + stub_request(:get, "#{base}/issues/i1/messages").to_return(page([{ 'id' => 'm1' }, { 'id' => 'm2' }])) + + expect(client.fetch_complete_issue_messages('i1')).to eq([{ 'id' => 'm1' }, { 'id' => 'm2' }]) + end + + it 'escapes the issue id' do + stub_request(:get, "#{base}/issues/..%2Fme/messages").to_return(page([])) + + client.fetch_complete_issue_messages('../me') + + expect(WebMock).to have_requested(:get, "#{base}/issues/..%2Fme/messages") + end + + it 'raises when the thread cannot be read' do + stub_request(:get, "#{base}/issues/i1/messages").to_return(json({ 'message' => 'boom' }, 500)) + + expect { client.fetch_complete_issue_messages('i1') } + .to raise_error(ForestAdminDatasourcePylon::APIError, /HTTP 500 boom/) + end + + it 'raises on a missing issue' do + stub_request(:get, "#{base}/issues/nope/messages").to_return(json({ 'message' => 'not found' }, 404)) + + expect { client.fetch_complete_issue_messages('nope') } + .to raise_error(ForestAdminDatasourcePylon::APIError) { |error| expect(error.status).to eq(404) } + end + + it 'raises rather than answering part of a thread Pylon never stops paginating' do + served = 0 + stub_request(:get, %r{/issues/i1/messages}).to_return do + served += 1 + page([{ 'id' => "m#{served}" }], cursor: "c#{served}") + end + + expect { client.fetch_complete_issue_messages('i1') } + .to raise_error(ForestAdminDatasourcePylon::APIError, %r{left the walk of issues/i1/messages short}) + end + end + describe '#search_accounts' do it 'posts the full search envelope and returns the records' do stub_request(:post, "#{base}/accounts/search").to_return(json('data' => [{ 'id' => 'a1' }])) diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/issue/messages_embedder_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/issue/messages_embedder_spec.rb index 46ad3dddd..93b0027bd 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/issue/messages_embedder_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/issue/messages_embedder_spec.rb @@ -129,6 +129,38 @@ def stub_messages(issue_id, *payloads) end end + describe 'the email envelope of a message' do + before { stub_issues(issue_payload('i1')) } + + def message_of(payload) + stub_messages('i1', payload) + issues.list(nil, filter, %w[id messages]).first['messages'].first + end + + it 'flattens the sender and recipients of an email' do + email_info = { 'from_email' => 'ada@acme.com', 'to_emails' => ['support@acme.io'], + 'cc_emails' => ['bob@acme.com'], 'bcc_emails' => ['audit@acme.io'], + 'message_id' => '' } + + expect(message_of(message_payload('msg-1', 'email_info' => email_info))).to include( + 'email_from' => 'ada@acme.com', 'email_to' => ['support@acme.io'], 'email_cc' => ['bob@acme.com'] + ) + end + + it 'leaves out the blind copies and the RFC 5322 message id' do + email_info = { 'bcc_emails' => ['audit@acme.io'], 'message_id' => '' } + message = message_of(message_payload('msg-1', 'email_info' => email_info)) + + expect(message.values.flatten).not_to include('audit@acme.io', '') + end + + it 'leaves every email field null on a message that is not an email' do + expect(message_of(message_payload('msg-1', 'source' => 'slack'))).to include( + 'email_from' => nil, 'email_to' => nil, 'email_cc' => nil + ) + end + end + describe 'the author of a message' do before { stub_issues(issue_payload('i1')) } diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/plugins/reply_to_issue_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/plugins/reply_to_issue_spec.rb new file mode 100644 index 000000000..a2e7dad2b --- /dev/null +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/plugins/reply_to_issue_spec.rb @@ -0,0 +1,622 @@ +module ForestAdminDatasourcePylon + class FakeReplyContext + attr_reader :form_values, :record_ids, :caller + + def initialize(form_values: {}, record_ids: ['i1'], records: [], caller: nil) + @form_values = form_values + @record_ids = record_ids + @records = records + @caller = caller + end + + def get_form_value(key) = @form_values[key] + alias get_record_ids record_ids + def get_records(_fields = []) = @records + end + + RSpec.describe Plugins::ReplyToIssue do + let(:client) { instance_double(ForestAdminDatasourcePylon::Client) } + let(:datasource) { instance_double(ForestAdminDatasourcePylon::Datasource, client: client) } + let(:result_builder) { ForestAdminDatasourceCustomizer::Decorators::Action::ResultBuilder.new } + let(:action_scope) { ForestAdminDatasourceCustomizer::Decorators::Action::Types::ActionScope } + let(:forest_exception) { ForestAdminDatasourceToolkit::Exceptions::ForestException } + let(:form) { described_class::FormBuilder } + let(:collection_customizer) do + Class.new do + attr_reader :registered + + def initialize = @registered = {} + def add_action(name, action) = @registered[name] = action + end.new + end + + # The author shapes Pylon sends: a customer message names its contact, a + # team message its user, the other side being nil. + let(:customer) { { 'contact' => { 'id' => 'c1' }, 'user' => nil } } + let(:team) { { 'contact' => nil, 'user' => { 'id' => 'u1' } } } + + def register(opts = {}) + described_class.new.run(nil, collection_customizer, { datasource: datasource }.merge(opts)) + collection_customizer.registered[opts[:action_name] || described_class::NAME] + end + + def message(id, source:, is_private: false, email_info: nil, author: customer) + { 'id' => id, 'source' => source, 'is_private' => is_private, 'email_info' => email_info, 'author' => author } + end + + def email_message(id, from:, to:, cc: [], author: customer) + message(id, source: 'email', email_info: { 'from_email' => from, 'to_emails' => to, 'cc_emails' => cc }, + author: author) + end + + def thread(*messages, issue_id: 'i1') + allow(client).to receive(:fetch_complete_issue_messages).with(issue_id).and_return(messages) + end + + def field(action, label) = action.form.find { |f| f[:label] == label } + + def run_action(action, values, **context_options) + action.execute.call(FakeReplyContext.new(form_values: values, **context_options), result_builder) + end + + describe '#run' do + it 'registers a SINGLE-scoped action under the default name' do + expect(register.scope).to eq(action_scope::SINGLE) + expect(collection_customizer.registered.keys).to contain_exactly(described_class::NAME) + end + + it 'honors :action_name' do + register(action_name: 'Answer the customer') + + expect(collection_customizer.registered.keys).to contain_exactly('Answer the customer') + end + + it 'offers the reply type when internal notes are allowed, which is the default' do + expect(register.form.map { |f| f[:label] }).to eq(['Reply type', 'Channel', 'To', 'CC', 'Message']) + end + + it 'drops the reply type when internal notes are not allowed' do + expect(register(allow_internal_note: false).form.map { |f| f[:label] }).to eq(%w[Channel To CC Message]) + end + + it 'builds through the real form factory' do + action = register + + expect { action.build_elements.validate_fields_ids }.not_to raise_error + expect(action.static_form).to be(false) + end + + it 'raises a ForestException without :datasource' do + expect { described_class.new.run(nil, collection_customizer, {}) } + .to raise_error(forest_exception, /requires :datasource/) + end + + it 'raises a ForestException without a collection' do + expect { described_class.new.run(nil, nil, datasource: datasource) } + .to raise_error(forest_exception, /requires a collection/) + end + end + + describe 'the message a public reply answers' do + def resolve(*messages, support: []) + described_class::ReplyTarget.resolve(messages, support_emails: support) + end + + it 'is nothing when no message is customer-visible' do + expect(resolve(message('n1', source: 'slack', is_private: true))).to be_nil + expect(resolve).to be_nil + end + + it 'keeps the channel of the first customer-visible message, skipping internal notes' do + target = resolve(message('n1', source: 'internal', is_private: true), message('m1', source: 'slack')) + + expect(target.source).to eq('slack') + expect(target.message_id).to eq('m1') + end + + it 'answers the latest message on that channel, not on another one' do + target = resolve(message('m1', source: 'slack'), message('m2', source: 'slack'), + message('m3', source: 'chat_widget')) + + expect(target.message_id).to eq('m2') + end + + it 'answers the sender of the latest customer email, keeping its copies' do + target = resolve(email_message('m1', from: 'old@acme.com', to: ['support@us.io']), + email_message('m2', from: 'ada@acme.com', to: ['support@us.io', 'bob@acme.com'], + cc: ['carol@acme.com']), + support: ['support@us.io']) + + expect(target.to).to eq(['ada@acme.com']) + expect(target.cc).to eq(['carol@acme.com']) + end + + # Before the team's first email, nothing in the thread names the support + # inbox the customer wrote to: To must not copy the customer's own To. + it 'keeps the inbox the customer wrote to out of To, with no support_emails configured' do + target = resolve(email_message('m1', from: 'ada@acme.com', to: ['help@us.io'])) + + expect(target.to).to eq(['ada@acme.com']) + end + + it 'reads a message whose email envelope is not an object as carrying none' do + target = resolve(message('m1', source: 'slack', email_info: '', author: team), + message('m2', source: 'slack', email_info: [])) + + expect(target.message_id).to eq('m2') + expect(target.email?).to be(false) + end + + it 'leaves out the support addresses whatever their case, and duplicates across To and CC' do + target = resolve(email_message('m1', from: 'Support@Us.io', to: ['ada@acme.com', 'ADA@acme.com'], + cc: ['ada@acme.com', 'support@us.io'], author: team), + support: ['support@us.io']) + + expect(target.to).to eq(['ada@acme.com']) + expect(target.cc).to eq([]) + end + + it 'writes again to whom the team last wrote, not to the team itself' do + target = resolve(email_message('m1', from: 'ada@acme.com', to: ['help@us.io']), + email_message('m2', from: 'agent@us.io', to: ['ada@acme.com'], cc: ['bob@acme.com'], + author: team)) + + expect(target.message_id).to eq('m2') + expect(target.to).to eq(['ada@acme.com']) + expect(target.cc).to eq(['bob@acme.com']) + end + + it 'leaves out the addresses the team wrote from, even when support_emails misses them' do + target = resolve(email_message('m1', from: ' Agent@us.io ', to: ['ada@acme.com'], author: team), + email_message('m2', from: 'ada@acme.com', to: ['agent@us.io'], cc: ['agent@us.io'])) + + expect(target.to).to eq(['ada@acme.com']) + expect(target.cc).to eq([]) + end + + it 'treats a message carrying an email envelope as email, whatever its source says' do + target = resolve(message('m1', source: 'gmail', email_info: { 'from_email' => 'ada@acme.com' })) + + expect(target.email?).to be(true) + expect(target.channel_label).to eq('email') + end + + it 'names the channel for the operator' do + expect(resolve(message('m1', source: 'slack')).channel_label).to eq('Slack') + expect(resolve(message('m1', source: 'chat_widget')).channel_label).to eq('chat widget') + expect(resolve(message('m1', source: nil)).channel_label).to eq('an unnamed channel') + end + end + + describe 'the form' do + let(:context) { FakeReplyContext.new(form_values: { form::TYPE_LABEL => form::PUBLIC_REPLY }) } + + it 'reads the thread once per render, however many fields depend on it' do + thread(message('m1', source: 'slack')) + dynamic = register.form.flat_map { |f| f.values_at(:enum_values, :default_value, :if_condition, :value) } + + dynamic.select { |value| value.respond_to?(:call) }.each { |value| value.call(context) } + + expect(client).to have_received(:fetch_complete_issue_messages).once + end + + it 'offers a public reply by default when the issue has a customer message' do + thread(message('m1', source: 'slack')) + type = field(register, form::TYPE_LABEL) + + expect(type[:enum_values].call(context)).to eq([form::PUBLIC_REPLY, form::INTERNAL_NOTE]) + expect(type[:default_value].call(context)).to eq(form::PUBLIC_REPLY) + end + + it 'offers only an internal note when the issue has no customer message' do + thread(message('n1', source: 'internal', is_private: true)) + type = field(register, form::TYPE_LABEL) + + expect(type[:enum_values].call(context)).to eq([form::INTERNAL_NOTE]) + expect(type[:default_value].call(context)).to eq(form::INTERNAL_NOTE) + end + + it 'shows the channel read-only' do + thread(message('m1', source: 'slack')) + channel = field(register, form::CHANNEL_LABEL) + + expect(channel[:is_read_only]).to be(true) + expect(channel[:value].call(context)).to eq('Replies via Slack') + end + + it 'explains why there is no public reply' do + thread + channel = field(register, form::CHANNEL_LABEL) + + expect(channel[:value].call(context)).to match(/no customer-visible message.*internal note instead/) + end + + it 'shows a failed thread read as the failure it is' do + allow(client).to receive(:fetch_complete_issue_messages) + .and_raise(ForestAdminDatasourcePylon::APIError.new('HTTP 500 boom', status: 500)) + + expect(field(register, form::CHANNEL_LABEL)[:value].call(context)) + .to eq('Could not read the conversation of Pylon issue i1: HTTP 500 boom') + end + + it 'hides the channel when an internal note is picked on an issue that could get a reply' do + thread(message('m1', source: 'slack')) + note = FakeReplyContext.new(form_values: { form::TYPE_LABEL => form::INTERNAL_NOTE }) + + expect(field(register, form::CHANNEL_LABEL)[:if_condition].call(note)).to be(false) + end + + # A transient failure is not an issue without customer message: picking + # the public reply renders the form again, and the thread is read again. + it 'keeps the public reply offered but not picked when the thread could not be read' do + allow(client).to receive(:fetch_complete_issue_messages) + .and_raise(ForestAdminDatasourcePylon::APIError.new('HTTP 429 slow down', status: 429)) + note = FakeReplyContext.new(form_values: { form::TYPE_LABEL => form::INTERNAL_NOTE }) + type = field(register, form::TYPE_LABEL) + + expect(type[:enum_values].call(note)).to eq([form::PUBLIC_REPLY, form::INTERNAL_NOTE]) + expect(type[:default_value].call(note)).to eq(form::INTERNAL_NOTE) + end + + it 'keeps explaining why a public reply is unavailable, even with an internal note picked' do + allow(client).to receive(:fetch_complete_issue_messages) + .and_raise(ForestAdminDatasourcePylon::APIError.new('HTTP 500 boom', status: 500)) + note = FakeReplyContext.new(form_values: { form::TYPE_LABEL => form::INTERNAL_NOTE }) + + expect(field(register, form::CHANNEL_LABEL)[:if_condition].call(note)).to be(true) + end + + it 'keeps the thread it read for the render across a garbage collection' do + thread(message('m1', source: 'slack')) + channel = field(register, form::CHANNEL_LABEL) + + channel[:value].call(context) + GC.start + channel[:value].call(context) + + expect(client).to have_received(:fetch_complete_issue_messages).once + end + + it 'reads the thread again for the next render' do + thread(message('m1', source: 'slack')) + channel = field(register, form::CHANNEL_LABEL) + + channel[:value].call(context) + channel[:value].call(FakeReplyContext.new(form_values: context.form_values)) + + expect(client).to have_received(:fetch_complete_issue_messages).twice + end + + describe 'the recipients' do + it 'are shown and pre-filled on an email issue' do + thread(email_message('m1', from: 'ada@acme.com', to: ['support@us.io'], cc: ['bob@acme.com'])) + action = register(support_emails: ['SUPPORT@us.io']) + + expect(field(action, form::TO_LABEL)[:if_condition].call(context)).to be(true) + expect(field(action, form::TO_LABEL)[:default_value].call(context)).to eq(['ada@acme.com']) + expect(field(action, form::CC_LABEL)[:default_value].call(context)).to eq(['bob@acme.com']) + expect(field(action, form::TO_LABEL)[:is_required]).to be(true) + end + + it 'are hidden on another channel' do + thread(message('m1', source: 'slack')) + + expect(field(register, form::TO_LABEL)[:if_condition].call(context)).to be(false) + expect(field(register, form::CC_LABEL)[:if_condition].call(context)).to be(false) + end + + it 'are hidden when the issue has no customer message' do + thread + + expect(field(register, form::TO_LABEL)[:if_condition].call(context)).to be(false) + end + end + end + + describe 'an execution' do + let(:posted) { { 'id' => 'r1', 'issue_id' => 'i1' } } + + def values(overrides = {}) + { form::TYPE_LABEL => form::PUBLIC_REPLY, form::MESSAGE_LABEL => '

Hi

' }.merge(overrides) + end + + before do + allow(client).to receive_messages(reply_to_issue: posted, create_issue_note: posted) + end + + it 'replies to the latest message of the customer channel' do + thread(message('m1', source: 'slack'), message('m2', source: 'slack')) + + result = run_action(register, values) + + expect(client).to have_received(:reply_to_issue).with('i1', 'body_html' => '

Hi

', 'message_id' => 'm2') + expect(result[:type]).to eq('Success') + expect(result[:message]).to eq('Reply sent via Slack on Pylon issue i1, signed by the account of the ' \ + 'Pylon API token.') + end + + it 'emails the recipients the operator kept' do + thread(email_message('m1', from: 'ada@acme.com', to: ['support@us.io'])) + + result = run_action(register, + values(form::TO_LABEL => ['ada@acme.com', ' '], form::CC_LABEL => ['bob@acme.com'])) + + expect(client).to have_received(:reply_to_issue).with( + 'i1', 'body_html' => '

Hi

', 'message_id' => 'm1', + 'email_info' => { 'to_emails' => ['ada@acme.com'], 'cc_emails' => ['bob@acme.com'] } + ) + expect(result[:message]).to start_with('Reply emailed to ada@acme.com on Pylon issue i1') + end + + it 'leaves the support and team addresses out of what the operator typed' do + thread(email_message('m1', from: 'ada@acme.com', to: ['support@us.io'])) + + run_action(register(support_emails: ['support@us.io']), + values(form::TO_LABEL => ['ada@acme.com', 'Support@us.io'], + form::CC_LABEL => ['ADA@acme.com', 'support@us.io', 'bob@acme.com'])) + + expect(client).to have_received(:reply_to_issue).with( + 'i1', hash_including('email_info' => { 'to_emails' => ['ada@acme.com'], 'cc_emails' => ['bob@acme.com'] }) + ) + end + + it 'names the typed addresses it left out' do + thread(email_message('m1', from: 'ada@acme.com', to: ['support@us.io'])) + + result = run_action(register(support_emails: ['support@us.io']), + values(form::TO_LABEL => ['ada@acme.com'], form::CC_LABEL => ['Support@us.io'])) + + expect(result[:type]).to eq('Success') + expect(result[:message]).to end_with('Left out as addresses of your team: Support@us.io.') + end + + it 'says why To is empty when it only held addresses of the team' do + thread(email_message('m1', from: 'ada@acme.com', to: ['support@us.io'])) + + result = run_action(register(support_emails: ['support@us.io']), values(form::TO_LABEL => ['support@us.io'])) + + expect(result[:message]).to eq('At least one recipient (To) is required to reply by email. Left out as ' \ + 'addresses of your team: support@us.io.') + expect(client).not_to have_received(:reply_to_issue) + end + + it 'refuses an email reply with no recipient, and sends nothing' do + thread(email_message('m1', from: 'ada@acme.com', to: [])) + + result = run_action(register, values(form::TO_LABEL => [])) + + expect(result[:type]).to eq('Error') + expect(result[:message]).to match(/At least one recipient/) + expect(client).not_to have_received(:reply_to_issue) + end + + it 'posts an internal note with no target' do + thread(message('m1', source: 'slack')) + + result = run_action(register, values(form::TYPE_LABEL => form::INTERNAL_NOTE)) + + expect(client).to have_received(:create_issue_note).with('i1', 'body_html' => '

Hi

') + expect(result[:message]).to eq('Internal note added to Pylon issue i1.') + end + + it 'never posts a note from an action that does not offer one' do + thread(message('m1', source: 'slack')) + + run_action(register(allow_internal_note: false), values(form::TYPE_LABEL => form::INTERNAL_NOTE)) + + expect(client).not_to have_received(:create_issue_note) + expect(client).to have_received(:reply_to_issue) + end + + it 'refuses a public reply on an issue with no customer message' do + thread(message('n1', source: 'internal', is_private: true)) + + result = run_action(register, values) + + expect(result[:type]).to eq('Error') + expect(result[:message]).to match(/i1 has no customer-visible message.*internal note instead/) + expect(client).not_to have_received(:reply_to_issue) + end + + it 'reports a failed thread read as such, never as an issue without customer message' do + allow(client).to receive(:fetch_complete_issue_messages) + .and_raise(ForestAdminDatasourcePylon::APIError.new('HTTP 503 down', status: 503)) + + result = run_action(register, values) + + expect(result[:message]).to eq('Could not read the conversation of Pylon issue i1: HTTP 503 down') + expect(client).not_to have_received(:reply_to_issue) + end + + it 'refuses an empty message' do + thread(message('m1', source: 'slack')) + + expect(run_action(register, values(form::MESSAGE_LABEL => ' '))[:message]).to eq('A message is required.') + expect(client).not_to have_received(:reply_to_issue) + end + + it 'names the missing issue' do + expect(run_action(register, values, record_ids: [])[:message]).to eq('No Pylon issue selected.') + expect(run_action(register(issue_id_field: 'pylon_id'), values, records: [{ 'pylon_id' => nil }])[:message]) + .to eq("No Pylon issue id found in 'pylon_id'.") + end + + it 'reads the issue id from the configured column' do + thread(message('m1', source: 'slack'), issue_id: 'i9') + + run_action(register(issue_id_field: 'pylon_id'), values, records: [{ 'pylon_id' => 'i9' }]) + + expect(client).to have_received(:reply_to_issue).with('i9', hash_including('message_id' => 'm1')) + end + + it 'hands the operator the reason Pylon refused the reply' do + thread(message('m1', source: 'slack')) + allow(client).to receive(:reply_to_issue) + .and_raise(ForestAdminDatasourcePylon::APIError.new('HTTP 400 message_id is not visible', status: 400)) + + result = run_action(register, values) + + expect(result[:type]).to eq('Error') + expect(result[:message]).to eq('HTTP 400 message_id is not visible') + end + + # Pylon may have delivered the reply before failing to answer: a bare + # failure would invite the operator to send it a second time. + it 'tells the operator to check Pylon when it did not confirm the reply' do + thread(message('m1', source: 'slack')) + allow(client).to receive(:reply_to_issue) + .and_raise(ForestAdminDatasourcePylon::APIError.new('HTTP 502', status: 502)) + + result = run_action(register, values) + + expect(result[:type]).to eq('Error') + expect(result[:message]).to eq('Pylon did not confirm the reply on issue i1 (HTTP 502). It may have been ' \ + 'delivered: check the issue in Pylon before sending it again.') + end + + it 'tells the operator to check Pylon when the reply timed out, with no status to read' do + thread(message('m1', source: 'slack')) + allow(client).to receive(:reply_to_issue) + .and_raise(ForestAdminDatasourcePylon::APIError.new('execution expired', status: nil)) + + expect(run_action(register, values)[:message]) + .to start_with('Pylon did not confirm the reply on issue i1 (execution expired). It may have been delivered') + end + + it 'posts an internal note without reading the thread, which may be unreadable' do + allow(client).to receive(:fetch_complete_issue_messages) + + run_action(register, values(form::TYPE_LABEL => form::INTERNAL_NOTE)) + + expect(client).to have_received(:create_issue_note).with('i1', 'body_html' => '

Hi

') + expect(client).not_to have_received(:fetch_complete_issue_messages) + end + + it 'refuses the markup an emptied rich-text editor submits' do + thread(message('m1', source: 'slack')) + + ['


', '

 

', '
', "

\u00A0

", '

 ​

'].each do |body| + expect(run_action(register, values(form::MESSAGE_LABEL => body))[:message]).to eq('A message is required.') + end + expect(client).not_to have_received(:reply_to_issue) + end + + it 'sends a message made of an image only' do + thread(message('m1', source: 'slack')) + + run_action(register, values(form::MESSAGE_LABEL => '

')) + + expect(client).to have_received(:reply_to_issue) + end + end + + describe 'signing a reply as the Forest operator' do + let(:operator) { Struct.new(:email).new('Ada@Acme.io') } + let(:values) { { form::TYPE_LABEL => form::PUBLIC_REPLY, form::MESSAGE_LABEL => '

Hi

' } } + + def user(id, email:, emails: [], is_deactivated: false, name: nil) + { 'id' => id, 'name' => name, 'email' => email, 'emails' => emails, 'is_deactivated' => is_deactivated } + end + + def run_as(caller, opts = {}) + run_action(register({ post_as_operator: true }.merge(opts)), values, caller: caller) + end + + before do + thread(message('m1', source: 'slack')) + allow(client).to receive_messages(reply_to_issue: { 'id' => 'r1' }, create_issue_note: { 'id' => 'n1' }) + end + + it 'passes the Pylon user sharing the operator email, whatever its case' do + allow(client).to receive(:fetch_users).and_return([user('u1', email: 'ada@acme.io', name: 'Ada')]) + + result = run_as(operator) + + expect(client).to have_received(:reply_to_issue).with('i1', hash_including('user_id' => 'u1')) + expect(result[:message]).to eq('Reply sent via Slack on Pylon issue i1, signed by Ada.') + end + + it 'matches one of the other addresses of a Pylon user' do + allow(client).to receive(:fetch_users) + .and_return([user('u2', email: 'ada@corp.io', emails: ['ada@acme.io'])]) + + run_as(operator) + + expect(client).to have_received(:reply_to_issue).with('i1', hash_including('user_id' => 'u2')) + end + + it 'names the signer by email when the Pylon user has a blank name' do + allow(client).to receive(:fetch_users).and_return([user('u1', email: 'ada@acme.io', name: ' ')]) + + expect(run_as(operator)[:message]).to end_with('signed by ada@acme.io.') + end + + it 'never signs as a deactivated Pylon user, and sends nothing' do + allow(client).to receive(:fetch_users).and_return([user('u1', email: 'ada@acme.io', is_deactivated: true)]) + + result = run_as(operator) + + expect(result[:type]).to eq('Error') + expect(result[:message]).to match(/No active Pylon user has the email ada@acme.io.*was not sent/) + expect(client).not_to have_received(:reply_to_issue) + end + + it 'refuses rather than letting the token sign when no Pylon user matches' do + allow(client).to receive(:fetch_users).and_return([user('u1', email: 'grace@acme.io')]) + + expect(run_as(operator)[:type]).to eq('Error') + expect(client).not_to have_received(:reply_to_issue) + end + + it 'refuses when several active Pylon users share the email' do + allow(client).to receive(:fetch_users) + .and_return([user('u1', email: 'ada@acme.io'), user('u2', email: 'x@acme.io', emails: ['ada@acme.io'])]) + + expect(run_as(operator)[:message]).to match(/2 active Pylon users have the email ada@acme.io/) + expect(client).not_to have_received(:reply_to_issue) + end + + it 'refuses rather than letting the token sign when the matched Pylon user has no id' do + allow(client).to receive(:fetch_users).and_return([user(nil, email: 'ada@acme.io')]) + + expect(run_as(operator)[:message]).to match(/Pylon user with the email ada@acme.io has no id.*was not sent/) + expect(client).not_to have_received(:reply_to_issue) + end + + it 'refuses when the Forest user has no email' do + allow(client).to receive(:fetch_users) + + expect(run_as(nil)[:message]).to eq('Your Forest user has no email to sign the reply with.') + expect(client).not_to have_received(:fetch_users) + end + + it 'reports a failed read of the Pylon users, and sends nothing' do + allow(client).to receive(:fetch_users) + .and_raise(ForestAdminDatasourcePylon::APIError.new('HTTP 500 boom', status: 500)) + + expect(run_as(operator)[:message]) + .to eq('Could not read the Pylon users to sign the reply in your name: HTTP 500 boom') + expect(client).not_to have_received(:reply_to_issue) + end + + it 'signs an internal note with the token, without looking the operator up' do + allow(client).to receive(:fetch_users) + + run_action(register(post_as_operator: true), values.merge(form::TYPE_LABEL => form::INTERNAL_NOTE), + caller: operator) + + expect(client).to have_received(:create_issue_note).with('i1', 'body_html' => '

Hi

') + expect(client).not_to have_received(:fetch_users) + end + + it 'leaves the token signing when the option is off' do + allow(client).to receive(:fetch_users) + + run_action(register, values, caller: operator) + + expect(client).to have_received(:reply_to_issue).with('i1', hash_excluding('user_id')) + expect(client).not_to have_received(:fetch_users) + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/rate_limits_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/rate_limits_spec.rb index c20e4b002..7a84d6988 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/rate_limits_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/rate_limits_spec.rb @@ -24,6 +24,8 @@ def rule_for(method, path) = described_class.for(method, path) it 'reads the documented budget of the writes' do expect(rule_for(:post, '/issues').limit).to eq(30) + expect(rule_for(:post, '/issues/abc/reply').limit).to eq(30) + expect(rule_for(:post, '/issues/abc/note').limit).to eq(30) expect(rule_for(:patch, '/issues/abc').limit).to eq(120) expect(rule_for(:delete, '/issues/abc').limit).to eq(120) expect(rule_for(:post, '/teams').limit).to eq(30) @@ -60,6 +62,7 @@ def rule_for(method, path) = described_class.for(method, path) called = [ [:get, '/me'], [:post, '/issues/search'], [:get, '/issues/abc'], [:get, '/issues/abc/messages'], [:post, '/issues'], [:patch, '/issues/abc'], + [:post, '/issues/abc/reply'], [:post, '/issues/abc/note'], [:delete, '/issues/abc'], [:post, '/accounts/search'], [:get, '/accounts'], [:get, '/accounts/acc_1'], [:post, '/accounts'], [:patch, '/accounts/acc_1'], [:delete, '/accounts/acc_1'], [:post, '/contacts/search'], [:get, '/contacts'], @@ -102,6 +105,12 @@ def rule_for(method, path) = described_class.for(method, path) expect(rule_for(:get, '/users').name).not_to eq(rule_for(:get, '/teams').name) end + it 'keeps a reply and a note out of the window of creating an issue' do + names = [rule_for(:post, '/issues'), rule_for(:post, '/issues/abc/reply'), rule_for(:post, '/issues/abc/note')] + + expect(names.map(&:name).uniq.size).to eq(3) + end + it 'is the same for two records of one endpoint' do expect(rule_for(:get, '/issues/one').name).to eq(rule_for(:get, '/issues/two').name) end