From da54565223b845faf363de0d898b4d42a55d9b80 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Fri, 30 Oct 2026 15:00:19 +0100 Subject: [PATCH 1/9] feat(datasource): add Pylon reply and note client methods [PRD-1873] Client::Writes gains reply_to_issue and create_issue_note, posting to /issues/{id}/reply and /issues/{id}/note with the id escaped. Both get their own 30/min window in RateLimits instead of the shared fallback. fetch_complete_issue_messages reads the thread for a caller acting on it: a failed or truncated read raises instead of degrading to nil, so an unreadable thread is never taken for one with no customer message. Co-Authored-By: Claude Opus 5.5 --- .../forest_admin_datasource_pylon/client.rb | 14 ++++-- .../client/writes.rb | 6 +++ .../rate_limits.rb | 2 + .../client/writes_spec.rb | 36 ++++++++++++++ .../client_spec.rb | 47 +++++++++++++++++++ .../rate_limits_spec.rb | 9 ++++ 6 files changed, 110 insertions(+), 4 deletions(-) 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..15c185d5e 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 @@ -58,6 +58,14 @@ def fetch_issue_messages(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 = "issues/#{Faraday::Utils.escape(issue_id)}/messages" + + 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 @@ -241,10 +249,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..af1439274 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,14 @@ 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_resource(issue_path(id, 'reply'), attributes) + def create_issue_note(id, attributes) = post_resource(issue_path(id, 'note'), attributes) + private + def issue_path(id, action) = "issues/#{Faraday::Utils.escape(id)}/#{action}" + def post_resource(resource, attributes) operation = "create(#{resource})" 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..adc857d7d 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,24 @@ 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 + 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 +154,24 @@ 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 '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/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 From 8e06c7acba9937004fc4b70316a45399d6aa662a Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Fri, 30 Oct 2026 15:01:58 +0100 Subject: [PATCH 2/9] feat(datasource): expose email recipients in the Pylon message thread [PRD-1873] Each embedded message now carries email_from, email_to and email_cc, flattened from Pylon's email_info and null outside email. A reply to an email issue must name its recipients, which Pylon does not infer. Blind copies and the RFC 5322 message id header stay out. Also corrects the thread endpoint budget in the MessagesEmbedder comment (120/min). Co-Authored-By: Claude Opus 5.5 --- .../collections/issue.rb | 5 ++- .../collections/issue/messages_embedder.rb | 16 ++++++++-- .../issue/messages_embedder_spec.rb | 32 +++++++++++++++++++ 3 files changed, 50 insertions(+), 3 deletions(-) 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/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')) } From 143b144301e0f006e28ddce57d67ee6d1a9dcd57 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Fri, 30 Oct 2026 15:38:07 +0100 Subject: [PATCH 3/9] feat(datasource): add the Pylon ReplyToIssue action plugin [PRD-1873] Answers a Pylon issue from Forest on the channel the customer used, or posts an internal note. The channel is the source of the first customer-visible message; the reply targets the latest message on that channel. Email recipients are pre-filled from that message, support addresses left out, since Pylon does not infer them. The form reads the thread once per render. The executor reads it again, strictly: an unreadable thread is reported as such, never as an issue without customer message. Pylon 4xx reach the operator as the action error. Replies are signed by the API token account for now. Co-Authored-By: Claude Opus 5.5 --- .../plugins/reply_to_issue.rb | 118 ++++++ .../plugins/reply_to_issue/form_builder.rb | 75 ++++ .../plugins/reply_to_issue/reply_target.rb | 57 +++ .../plugins/reply_to_issue/thread_lookup.rb | 29 ++ .../plugins/reply_to_issue_spec.rb | 357 ++++++++++++++++++ 5 files changed, 636 insertions(+) create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue/form_builder.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue/reply_target.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue/thread_lookup.rb create mode 100644 packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/plugins/reply_to_issue_spec.rb 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..5e8e65b0a --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue.rb @@ -0,0 +1,118 @@ +module ForestAdminDatasourcePylon + module Plugins + # Answers a Pylon issue from Forest, on the channel the customer used, or + # posts an internal note on it. Pylon keeps the channel: a reply is + # delivered into the conversation of the message it names, which + # ReplyTarget picks. Email is the exception Pylon leaves to the caller, + # which must name the recipients. + # + # SINGLE scope only: the form is built from one issue's thread. + 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, + 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 + + # The thread is read again rather than trusted from the form: the + # conversation may have moved since it was rendered. + def executor(datasource, opts) + lambda do |context, result_builder| + state = ThreadLookup.call(datasource, context, opts) + next result_builder.error(message: state.error) if state.error + + body = context.form_values[FormBuilder::MESSAGE_LABEL].to_s + next result_builder.error(message: 'A message is required.') if body.strip.empty? + + status, message = if FormBuilder.public_reply?(context, opts) + reply(datasource, state, body, context.form_values, opts) + else + note(datasource, state, body) + end + result_builder.public_send(status, message: message) + end + end + + def reply(datasource, state, body, values, opts) + return [:error, no_target_message(state.issue_id, opts)] if state.target.nil? + + payload = { 'body_html' => body, 'message_id' => state.target.message_id } + if state.target.email? + to = addresses(values[FormBuilder::TO_LABEL]) + return [:error, 'At least one recipient (To) is required to reply by email.'] if to.empty? + + payload['email_info'] = { 'to_emails' => to, 'cc_emails' => addresses(values[FormBuilder::CC_LABEL]) } + end + + write(state.issue_id, reply_success(state, payload)) do + datasource.client.reply_to_issue(state.issue_id, payload) + end + end + + def note(datasource, state, body) + write(state.issue_id, "Internal note added to Pylon issue #{state.issue_id}.") do + datasource.client.create_issue_note(state.issue_id, 'body_html' => body) + end + end + + # A 4xx is Pylon naming what is wrong with this reply, handed to the + # operator as the action's error. Anything else stays the failure it is. + def write(issue_id, success) + yield + [:success, success] + rescue APIError => e + raise unless (400..499).cover?(e.status.to_i) + + ForestAdminDatasourcePylon.logger.warn( + "[forest_admin_datasource_pylon] Pylon refused to write on issue #{issue_id}: #{e.message}" + ) + [:error, e.message] + end + + def addresses(value) + Array(value).map { |address| address.to_s.strip }.reject(&:empty?).uniq(&:downcase) + 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) + signed = 'signed by the account of the Pylon API token' + 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..b83fa37c1 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue/form_builder.rb @@ -0,0 +1,75 @@ +module ForestAdminDatasourcePylon + module Plugins + class ReplyToIssue + # Every dynamic property of the form reads the same ThreadLookup state, + # memoized on the render's context: one thread read per render, however + # many fields depend on it. + 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) + states = ObjectSpace::WeakMap.new + state = ->(context) { states[context] ||= ThreadLookup.call(datasource, context, opts) } + + 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: ->(context) { state.call(context).target ? [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 + + def channel_field(state, opts) + { type: FieldType::STRING, label: CHANNEL_LABEL, is_read_only: true, + if_condition: ->(context) { public_reply?(context, opts) }, + 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 "Reply sent 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/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..7e5c1f7f7 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue/reply_target.rb @@ -0,0 +1,57 @@ +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'] + new(visible.reverse.find { |message| message['source'] == source }, support_emails) + end + + def initialize(message, support_emails) + @message_id = message['id'] + @source = message['source'] + @email_info = message['email_info'].is_a?(Hash) ? message['email_info'] : {} + @to, @cc = recipients(support_emails.map(&:downcase)) + 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 + + # The sender joins To: answering a customer's email means writing back to + # them. The support addresses drop out, being the side that replies. + def recipients(support) + to = unique_addresses([@email_info['from_email'], *Array(@email_info['to_emails'])], support) + cc = unique_addresses(Array(@email_info['cc_emails']), support + to.map(&:downcase)) + [to, cc] + end + + def unique_addresses(addresses, excluded) + addresses.map { |address| address.to_s.strip } + .reject { |address| address.empty? || excluded.include?(address.downcase) } + .uniq(&:downcase) + 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..3f8fcfe48 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue/thread_lookup.rb @@ -0,0 +1,29 @@ +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 nothing else is. + module ThreadLookup + State = Struct.new(:issue_id, :target, :error, keyword_init: true) + + module_function + + def call(datasource, context, opts) + issue_id = IssueTargets.resolve_issue_ids(context, opts[:issue_id_field]).first + return State.new(error: no_issue(opts[:issue_id_field])) if issue_id.nil? + + 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/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..a7fa66679 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/plugins/reply_to_issue_spec.rb @@ -0,0 +1,357 @@ +module ForestAdminDatasourcePylon + class FakeReplyContext + attr_reader :form_values, :record_ids + + def initialize(form_values: {}, record_ids: ['i1'], records: []) + @form_values = form_values + @record_ids = record_ids + @records = records + 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 + + 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) + { 'id' => id, 'source' => source, 'is_private' => is_private, 'email_info' => email_info } + end + + def email_message(id, from:, to:, cc: []) + message(id, source: 'email', email_info: { 'from_email' => from, 'to_emails' => to, 'cc_emails' => cc }) + 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 'pre-fills an email reply from the latest email, its sender included' 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', 'bob@acme.com']) + expect(target.cc).to eq(['carol@acme.com']) + 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']), + support: ['support@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('Reply sent 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' do + 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 + + 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 '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 + + it 'lets a Pylon failure that is not the operator\'s to fix stay an error' do + thread(message('m1', source: 'slack')) + allow(client).to receive(:reply_to_issue) + .and_raise(ForestAdminDatasourcePylon::APIError.new('HTTP 502', status: 502)) + + expect { run_action(register, values) }.to raise_error(ForestAdminDatasourcePylon::APIError, /502/) + end + end + end +end From 1be7d81dfa62be43bc6ccbcc3470f4018f6cd74b Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Fri, 30 Oct 2026 15:54:24 +0100 Subject: [PATCH 4/9] feat(datasource): sign Pylon replies as the Forest operator [PRD-1873] With post_as_operator, a public reply passes the user_id of the active Pylon user whose email or emails match the Forest caller email, case insensitive. No match, several matches or a caller without email refuse the reply and send nothing: falling back to the token would sign some replies in the operator name and others not, out of their sight. Internal notes stay signed by the token account. Off by default. Co-Authored-By: Claude Opus 5.5 --- .../plugins/reply_to_issue.rb | 28 +++-- .../plugins/reply_to_issue/operator_lookup.rb | 44 ++++++++ .../plugins/reply_to_issue_spec.rb | 101 +++++++++++++++++- 3 files changed, 164 insertions(+), 9 deletions(-) create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue/operator_lookup.rb 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 index 5e8e65b0a..e6ac54316 100644 --- 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 @@ -7,6 +7,10 @@ module Plugins # which must name the recipients. # # SINGLE scope only: the form is built from one issue's thread. + # + # With `post_as_operator`, a public reply is signed by the Pylon user + # sharing the Forest operator's email, and refused when there is none. + # Internal notes are always signed by the token's account. class ReplyToIssue < ForestAdminDatasourceCustomizer::Plugins::Plugin BaseAction = ForestAdminDatasourceCustomizer::Decorators::Action::BaseAction ActionScope = ForestAdminDatasourceCustomizer::Decorators::Action::Types::ActionScope @@ -23,6 +27,7 @@ def run(_datasource_customizer, collection_customizer = nil, options = {}) 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]) } @@ -51,7 +56,7 @@ def executor(datasource, opts) next result_builder.error(message: 'A message is required.') if body.strip.empty? status, message = if FormBuilder.public_reply?(context, opts) - reply(datasource, state, body, context.form_values, opts) + reply(datasource, state, body, context, opts) else note(datasource, state, body) end @@ -59,18 +64,23 @@ def executor(datasource, opts) end end - def reply(datasource, state, body, values, opts) + def reply(datasource, state, body, context, opts) return [:error, no_target_message(state.issue_id, opts)] if state.target.nil? payload = { 'body_html' => body, 'message_id' => state.target.message_id } if state.target.email? - to = addresses(values[FormBuilder::TO_LABEL]) + to = addresses(context.form_values[FormBuilder::TO_LABEL]) return [:error, 'At least one recipient (To) is required to reply by email.'] if to.empty? - payload['email_info'] = { 'to_emails' => to, 'cc_emails' => addresses(values[FormBuilder::CC_LABEL]) } + payload['email_info'] = { 'to_emails' => to, + 'cc_emails' => addresses(context.form_values[FormBuilder::CC_LABEL]) } end - write(state.issue_id, reply_success(state, payload)) do + 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_success(state, payload, operator&.user)) do datasource.client.reply_to_issue(state.issue_id, payload) end end @@ -104,8 +114,12 @@ def no_target_message(issue_id, opts) opts[:allow_internal_note] ? "#{message} Post an internal note instead." : message end - def reply_success(state, payload) - signed = 'signed by the account of the Pylon API token' + def reply_success(state, payload, operator) + signed = if operator + "signed by #{operator["name"] || operator["email"]}" + 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}." 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..34422dece --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue/operator_lookup.rb @@ -0,0 +1,44 @@ +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? + + matches = datasource.client.fetch_users.select { |user| active?(user) && addresses(user).include?(email) } + return Result.new(user: matches.first) if matches.size == 1 + + Result.new(error: refusal(email, matches.size)) + 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/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 index a7fa66679..c77f31246 100644 --- 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 @@ -1,11 +1,12 @@ module ForestAdminDatasourcePylon class FakeReplyContext - attr_reader :form_values, :record_ids + attr_reader :form_values, :record_ids, :caller - def initialize(form_values: {}, record_ids: ['i1'], records: []) + 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] @@ -353,5 +354,101 @@ def values(overrides = {}) expect { run_action(register, values) }.to raise_error(ForestAdminDatasourcePylon::APIError, /502/) 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 '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 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 From 4359b4c8e6afdc7a5929ee3a1283c53a2bcc14dd Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Fri, 30 Oct 2026 15:57:02 +0100 Subject: [PATCH 5/9] docs(datasource): document the Pylon ReplyToIssue plugin [PRD-1873] Options, the channel rule, how email recipients are pre-filled, who signs a reply, and what to know before granting the action: the issue_id_field column is the authority, and post_as_operator trusts the emails shared between Forest and Pylon. Co-Authored-By: Claude Opus 5.5 --- .../forest_admin_datasource_pylon/README.md | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/packages/forest_admin_datasource_pylon/README.md b/packages/forest_admin_datasource_pylon/README.md index de565ab19..4d8761ef0 100644 --- a/packages/forest_admin_datasource_pylon/README.md +++ b/packages/forest_admin_datasource_pylon/README.md @@ -169,6 +169,51 @@ 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, its sender included, and leaves out the `support_emails`. The operator +can edit both; a reply with no To is refused. + +**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 and again on submit**, one `GET /issues/{id}/messages` + each (120 a minute). An unreadable thread is shown as an error, never as an issue without + customer message. + ## Development ```bash From 3617bcb7a71bc572a76413c7c4c13a60afab0de0 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Fri, 30 Oct 2026 16:11:57 +0100 Subject: [PATCH 6/9] fix(datasource): address review findings on Pylon ReplyToIssue [PRD-1873] - Recipients: the sender of the latest email joins To only when the customer wrote it, and the addresses the team wrote from in the thread are left out even when support_emails misses them. - An internal note no longer needs the thread, so an unreadable thread still lets the operator leave one. - A reply or note answered 2xx is taken as delivered whatever its body, and a non-4xx failure asks the operator to check Pylon before sending again instead of surfacing a bare 500. - The per-render thread cache holds its state strongly: WeakMap values could be collected mid-render. - An emptied rich-text editor (


) is refused as empty. - The operator lookup asks Pylon for active users only. Co-Authored-By: Claude Opus 5.5 --- .../forest_admin_datasource_pylon/README.md | 8 +- .../forest_admin_datasource_pylon/client.rb | 6 +- .../client/writes.rb | 13 ++- .../plugins/reply_to_issue.rb | 48 ++++++---- .../plugins/reply_to_issue/form_builder.rb | 16 ++-- .../plugins/reply_to_issue/operator_lookup.rb | 3 +- .../plugins/reply_to_issue/reply_target.rb | 42 ++++++--- .../plugins/reply_to_issue/thread_lookup.rb | 12 ++- .../client/writes_spec.rb | 8 ++ .../plugins/reply_to_issue_spec.rb | 90 ++++++++++++++++++- 10 files changed, 196 insertions(+), 50 deletions(-) diff --git a/packages/forest_admin_datasource_pylon/README.md b/packages/forest_admin_datasource_pylon/README.md index 4d8761ef0..17162ea28 100644 --- a/packages/forest_admin_datasource_pylon/README.md +++ b/packages/forest_admin_datasource_pylon/README.md @@ -194,9 +194,13 @@ read-only. An issue with no customer-visible message — one created as an inter 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, its sender included, and leaves out the `support_emails`. The operator +email of the conversation: its sender when the customer wrote it, its recipients when the team did. +The `support_emails` and the addresses the team wrote from in the thread are left out. The operator can edit both; a reply with no To is refused. +**When Pylon does not confirm.** A 4xx is shown as Pylon words it. Any other failure 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, @@ -212,7 +216,7 @@ Three things to know before granting it. consistent, and restrict the action to the roles that should answer customers. - **The thread is read on every form render and again on submit**, one `GET /issues/{id}/messages` each (120 a minute). An unreadable thread is shown as an error, never as an issue without - customer message. + customer message; an internal note needs no thread and stays available. ## Development 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 15c185d5e..9b386eea2 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,7 +53,7 @@ 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 @@ -61,11 +61,13 @@ def fetch_issue_messages(issue_id) # 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 = "issues/#{Faraday::Utils.escape(issue_id)}/messages" + path = issue_messages_path(issue_id) must_succeed(path) { collect_pages(path, refuse_past_cap: true) } end + def issue_messages_path(issue_id) = "issues/#{Faraday::Utils.escape(issue_id)}/messages" + 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 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 af1439274..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 @@ -30,13 +30,22 @@ 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_resource(issue_path(id, 'reply'), attributes) - def create_issue_note(id, attributes) = post_resource(issue_path(id, 'note'), attributes) + 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/plugins/reply_to_issue.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/reply_to_issue.rb index e6ac54316..7413633ac 100644 --- 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 @@ -45,26 +45,34 @@ def build_action(datasource, opts) &executor(datasource, opts)) end - # The thread is read again rather than trusted from the form: the - # conversation may have moved since it was rendered. + # 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| - state = ThreadLookup.call(datasource, context, opts) - next result_builder.error(message: state.error) if state.error + 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 body.strip.empty? + next result_builder.error(message: 'A message is required.') if blank_html?(body) status, message = if FormBuilder.public_reply?(context, opts) - reply(datasource, state, body, context, opts) + reply(datasource, ThreadLookup.read(datasource, issue_id, opts), body, context, opts) else - note(datasource, state, body) + 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, ' ').strip.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 = { 'body_html' => body, 'message_id' => state.target.message_id } @@ -80,33 +88,35 @@ def reply(datasource, state, body, context, opts) return [:error, operator.error] if operator&.error payload['user_id'] = operator.user['id'] if operator - write(state.issue_id, reply_success(state, payload, operator&.user)) do + write(state.issue_id, 'reply', reply_success(state, payload, operator&.user)) do datasource.client.reply_to_issue(state.issue_id, payload) end end - def note(datasource, state, body) - write(state.issue_id, "Internal note added to Pylon issue #{state.issue_id}.") do - datasource.client.create_issue_note(state.issue_id, 'body_html' => body) + 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 with this reply, handed to the - # operator as the action's error. Anything else stays the failure it is. - def write(issue_id, success) + # 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 - raise unless (400..499).cover?(e.status.to_i) - ForestAdminDatasourcePylon.logger.warn( - "[forest_admin_datasource_pylon] Pylon refused to write on issue #{issue_id}: #{e.message}" + "[forest_admin_datasource_pylon] #{what} on issue #{issue_id} failed: #{e.message}" ) - [:error, 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 addresses(value) - Array(value).map { |address| address.to_s.strip }.reject(&:empty?).uniq(&:downcase) + ReplyTarget.unique_addresses(Array(value)) end def no_target_message(issue_id, opts) 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 index b83fa37c1..5ac6104b5 100644 --- 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 @@ -2,8 +2,9 @@ module ForestAdminDatasourcePylon module Plugins class ReplyToIssue # Every dynamic property of the form reads the same ThreadLookup state, - # memoized on the render's context: one thread read per render, however - # many fields depend on it. + # kept for the render's context: one thread read per render, however many + # fields depend on it. One entry per thread, held strongly: a WeakMap would + # let the state be collected mid-render, and WeakKeyMap needs Ruby 3.3. module FormBuilder FieldType = ForestAdminDatasourceCustomizer::Decorators::Action::Types::FieldType @@ -19,8 +20,13 @@ module FormBuilder module_function def build(datasource, opts) - states = ObjectSpace::WeakMap.new - state = ->(context) { states[context] ||= ThreadLookup.call(datasource, context, opts) } + key = :"forest_admin_datasource_pylon_reply_state_#{opts.object_id}" + state = lambda do |context| + cached = Thread.current[key] + next cached.last if cached&.first.equal?(context) + + ThreadLookup.call(datasource, context, opts).tap { |fresh| Thread.current[key] = [context, fresh] } + end fields = [] fields << type_field(state) if opts[:allow_internal_note] @@ -43,7 +49,7 @@ def type_field(state) def channel_field(state, opts) { type: FieldType::STRING, label: CHANNEL_LABEL, is_read_only: true, - if_condition: ->(context) { public_reply?(context, opts) }, + 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 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 index 34422dece..a6d12df80 100644 --- 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 @@ -13,7 +13,8 @@ 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? - matches = datasource.client.fetch_users.select { |user| active?(user) && addresses(user).include?(email) } + users = datasource.client.fetch_users(include_deactivated: false) + matches = users.select { |user| active?(user) && addresses(user).include?(email) } return Result.new(user: matches.first) if matches.size == 1 Result.new(error: refusal(email, matches.size)) 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 index 7e5c1f7f7..71d1b3367 100644 --- 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 @@ -16,14 +16,33 @@ def self.resolve(messages, support_emails: []) return nil if visible.empty? source = visible.first['source'] - new(visible.reverse.find { |message| message['source'] == source }, support_emails) + excluded = support_emails.map(&:downcase) + team_addresses(visible) + new(visible.reverse.find { |message| message['source'] == source }, excluded) end - def initialize(message, support_emails) + # 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) } + .filter_map { |message| message.dig('email_info', 'from_email')&.to_s&.downcase } + 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 = message['email_info'].is_a?(Hash) ? message['email_info'] : {} - @to, @cc = recipients(support_emails.map(&:downcase)) + @to, @cc = recipients(message, excluded) end def email? @@ -38,19 +57,14 @@ def channel_label private - # The sender joins To: answering a customer's email means writing back to - # them. The support addresses drop out, being the side that replies. - def recipients(support) - to = unique_addresses([@email_info['from_email'], *Array(@email_info['to_emails'])], support) - cc = unique_addresses(Array(@email_info['cc_emails']), support + to.map(&:downcase)) + # Answering a customer's email writes back to its sender; answering the + # team's own last email writes again to whom it went. + def recipients(message, excluded) + sender = self.class.team_authored?(message) ? [] : [@email_info['from_email']] + to = self.class.unique_addresses(sender + Array(@email_info['to_emails']), excluded) + cc = self.class.unique_addresses(Array(@email_info['cc_emails']), excluded + to.map(&:downcase)) [to, cc] end - - def unique_addresses(addresses, excluded) - addresses.map { |address| address.to_s.strip } - .reject { |address| address.empty? || excluded.include?(address.downcase) } - .uniq(&:downcase) - 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 index 3f8fcfe48..c4d754f8a 100644 --- 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 @@ -2,16 +2,24 @@ 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 nothing else is. + # 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 = IssueTargets.resolve_issue_ids(context, opts[:issue_id_field]).first + 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])) 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 adc857d7d..c736fa78b 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 @@ -52,6 +52,14 @@ def json(payload, status = 200) 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| 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 index c77f31246..fe26c9b60 100644 --- 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 @@ -134,6 +134,26 @@ def resolve(*messages, support: []) expect(target.cc).to eq([]) end + it 'writes again to whom the team last wrote, not to the team itself' do + team = { 'user' => { 'id' => 'u1' } } + 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']) + .merge('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 + team = { 'user' => { 'id' => 'u1' } } + target = resolve(email_message('m1', from: 'Agent@us.io', to: ['ada@acme.com']).merge('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' })) @@ -199,12 +219,44 @@ def resolve(*messages, support: []) .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' do + 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 + 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 }) + action = register + + expect(field(action, form::TYPE_LABEL)[:enum_values].call(note)).to eq([form::INTERNAL_NOTE]) + expect(field(action, 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'])) @@ -346,12 +398,44 @@ def values(overrides = {}) expect(result[:message]).to eq('HTTP 400 message_id is not visible') end - it 'lets a Pylon failure that is not the operator\'s to fix stay an error' do + # 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)) - expect { run_action(register, values) }.to raise_error(ForestAdminDatasourcePylon::APIError, /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 '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')) + + ['


', '

 

', '
'].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 From c4da6bd3869f833a8beca50a3f562c51e4eb1523 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Fri, 30 Oct 2026 17:02:11 +0100 Subject: [PATCH 7/9] fix(datasource): address PR review findings on Pylon ReplyToIssue [PRD-1873] - Answering a customer email writes to its sender only: the customer's own To holds the support inbox, which nothing else names before the team's first email when support_emails is not set. - The support and team addresses are also removed from what the operator types in To and CC, and CC is deduplicated against To. - An email_info that is not an object no longer raises out of the thread lookup. - The per-render state is kept on the render context rather than in a thread-local slot that outlived the request. - A literal non-breaking space or zero-width character no longer passes as a message, and a blank Pylon user name falls back to the email. - issue_messages_path is private; the README states what a submit costs in thread reads, the agent rendering the form again before executing. Co-Authored-By: Claude Opus 5.5 --- .../forest_admin_datasource_pylon/README.md | 16 ++++--- .../forest_admin_datasource_pylon/client.rb | 4 +- .../plugins/reply_to_issue.rb | 32 +++++--------- .../plugins/reply_to_issue/form_builder.rb | 12 +++-- .../plugins/reply_to_issue/reply_target.rb | 20 ++++++--- .../plugins/reply_to_issue_spec.rb | 44 +++++++++++++++++-- 6 files changed, 82 insertions(+), 46 deletions(-) diff --git a/packages/forest_admin_datasource_pylon/README.md b/packages/forest_admin_datasource_pylon/README.md index 17162ea28..37374f541 100644 --- a/packages/forest_admin_datasource_pylon/README.md +++ b/packages/forest_admin_datasource_pylon/README.md @@ -194,9 +194,11 @@ read-only. An issue with no customer-visible message — one created as an inter 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: its sender when the customer wrote it, its recipients when the team did. -The `support_emails` and the addresses the team wrote from in the thread are left out. The operator -can edit both; a reply with no To is refused. +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. 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 as Pylon words it. Any other failure may come after Pylon delivered the message, so the action says so and asks to check the issue before sending again. @@ -214,9 +216,11 @@ Three things to know before granting it. - **`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 and again on submit**, one `GET /issues/{id}/messages` - each (120 a minute). An unreadable thread is shown as an error, never as an issue without - customer message; an internal note needs no thread and stays available. +- **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. An unreadable thread is + shown as an error, never as an issue without customer message; an internal note does not depend + on it and stays available. ## Development 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 9b386eea2..00c85a02f 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 @@ -66,8 +66,6 @@ def fetch_complete_issue_messages(issue_id) must_succeed(path) { collect_pages(path, refuse_past_cap: true) } end - def issue_messages_path(issue_id) = "issues/#{Faraday::Utils.escape(issue_id)}/messages" - 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 @@ -154,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) 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 index 7413633ac..dce2bf5ae 100644 --- 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 @@ -1,16 +1,9 @@ +require 'cgi' + module ForestAdminDatasourcePylon module Plugins - # Answers a Pylon issue from Forest, on the channel the customer used, or - # posts an internal note on it. Pylon keeps the channel: a reply is - # delivered into the conversation of the message it names, which - # ReplyTarget picks. Email is the exception Pylon leaves to the caller, - # which must name the recipients. - # - # SINGLE scope only: the form is built from one issue's thread. - # - # With `post_as_operator`, a public reply is signed by the Pylon user - # sharing the Forest operator's email, and refused when there is none. - # Internal notes are always signed by the token's account. + # 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 @@ -68,7 +61,8 @@ def executor(datasource, opts) def blank_html?(body) return false if body.match?(/]*>/, ' ').gsub(/ | /i, ' ').strip.empty? + text = CGI.unescapeHTML(body.gsub(/<[^>]*>/, ' ').gsub(/ /i, ' ')) + text.gsub(/[[:space:]\u200B-\u200D\u2060\uFEFF]/, '').empty? end def reply(datasource, state, body, context, opts) @@ -77,11 +71,13 @@ def reply(datasource, state, body, context, opts) payload = { 'body_html' => body, 'message_id' => state.target.message_id } if state.target.email? - to = addresses(context.form_values[FormBuilder::TO_LABEL]) + excluded = state.target.excluded + to = ReplyTarget.unique_addresses(Array(context.form_values[FormBuilder::TO_LABEL]), excluded) return [:error, 'At least one recipient (To) is required to reply by email.'] if to.empty? - payload['email_info'] = { 'to_emails' => to, - 'cc_emails' => addresses(context.form_values[FormBuilder::CC_LABEL]) } + cc = ReplyTarget.unique_addresses(Array(context.form_values[FormBuilder::CC_LABEL]), + excluded + to.map(&:downcase)) + payload['email_info'] = { 'to_emails' => to, 'cc_emails' => cc } end operator = OperatorLookup.call(datasource, context.caller) if opts[:post_as_operator] @@ -115,10 +111,6 @@ def write(issue_id, what, success) 'delivered: check the issue in Pylon before sending it again.'] end - def addresses(value) - ReplyTarget.unique_addresses(Array(value)) - 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 @@ -126,7 +118,7 @@ def no_target_message(issue_id, opts) def reply_success(state, payload, operator) signed = if operator - "signed by #{operator["name"] || operator["email"]}" + "signed by #{[operator["name"], operator["email"]].find { |v| !v.to_s.strip.empty? }}" else 'signed by the account of the Pylon API token' 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 index 5ac6104b5..ac9a2bbdc 100644 --- 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 @@ -2,9 +2,8 @@ module ForestAdminDatasourcePylon module Plugins class ReplyToIssue # Every dynamic property of the form reads the same ThreadLookup state, - # kept for the render's context: one thread read per render, however many - # fields depend on it. One entry per thread, held strongly: a WeakMap would - # let the state be collected mid-render, and WeakKeyMap needs Ruby 3.3. + # 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 @@ -20,12 +19,11 @@ module FormBuilder module_function def build(datasource, opts) - key = :"forest_admin_datasource_pylon_reply_state_#{opts.object_id}" + ivar = :"@forest_admin_datasource_pylon_reply_state_#{opts.object_id}" state = lambda do |context| - cached = Thread.current[key] - next cached.last if cached&.first.equal?(context) + next context.instance_variable_get(ivar) if context.instance_variable_defined?(ivar) - ThreadLookup.call(datasource, context, opts).tap { |fresh| Thread.current[key] = [context, fresh] } + context.instance_variable_set(ivar, ThreadLookup.call(datasource, context, opts)) end fields = [] 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 index 71d1b3367..3603923ac 100644 --- 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 @@ -9,7 +9,7 @@ class ReplyTarget EMAIL_SOURCE = 'email'.freeze CHANNEL_LABELS = { 'email' => 'email', 'slack' => 'Slack' }.freeze - attr_reader :message_id, :source, :to, :cc + attr_reader :message_id, :source, :to, :cc, :excluded def self.resolve(messages, support_emails: []) visible = Array(messages).select { |message| message.is_a?(Hash) && message['is_private'] != true } @@ -24,7 +24,11 @@ def self.resolve(messages, support_emails: []) # keeps a reply out of the support inbox when `support_emails` misses one. def self.team_addresses(messages) messages.select { |message| team_authored?(message) } - .filter_map { |message| message.dig('email_info', 'from_email')&.to_s&.downcase } + .filter_map { |message| email_info(message)['from_email']&.to_s&.downcase } + end + + def self.email_info(message) + message['email_info'].is_a?(Hash) ? message['email_info'] : {} end def self.team_authored?(message) @@ -41,7 +45,8 @@ def self.unique_addresses(addresses, excluded = []) def initialize(message, excluded) @message_id = message['id'] @source = message['source'] - @email_info = message['email_info'].is_a?(Hash) ? message['email_info'] : {} + @email_info = self.class.email_info(message) + @excluded = excluded @to, @cc = recipients(message, excluded) end @@ -57,11 +62,12 @@ def channel_label private - # Answering a customer's email writes back to its sender; answering the - # team's own last email writes again to whom it went. + # 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 recipients(message, excluded) - sender = self.class.team_authored?(message) ? [] : [@email_info['from_email']] - to = self.class.unique_addresses(sender + Array(@email_info['to_emails']), excluded) + to = self.class.team_authored?(message) ? Array(@email_info['to_emails']) : [@email_info['from_email']] + to = self.class.unique_addresses(to, excluded) cc = self.class.unique_addresses(Array(@email_info['cc_emails']), excluded + to.map(&:downcase)) [to, cc] end 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 index fe26c9b60..50a8355c9 100644 --- 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 @@ -115,19 +115,37 @@ def resolve(*messages, support: []) expect(target.message_id).to eq('m2') end - it 'pre-fills an email reply from the latest email, its sender included' do + 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', 'bob@acme.com']) + 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 + team = { 'user' => { 'id' => 'u1' } } + target = resolve(message('m1', source: 'slack', email_info: '').merge('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 + team = { 'user' => { 'id' => 'u1' } } target = resolve(email_message('m1', from: 'Support@Us.io', to: ['ada@acme.com', 'ADA@acme.com'], - cc: ['ada@acme.com', 'support@us.io']), + cc: ['ada@acme.com', 'support@us.io']).merge('author' => team), support: ['support@us.io']) expect(target.to).to eq(['ada@acme.com']) @@ -318,6 +336,18 @@ def values(overrides = {}) 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 'refuses an email reply with no recipient, and sends nothing' do thread(email_message('m1', from: 'ada@acme.com', to: [])) @@ -424,7 +454,7 @@ def values(overrides = {}) it 'refuses the markup an emptied rich-text editor submits' do thread(message('m1', source: 'slack')) - ['


', '

 

', '
'].each do |body| + ['


', '

 

', '
', "

\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) @@ -474,6 +504,12 @@ def run_as(caller, opts = {}) 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)]) From 50928c700cfad4bf6475f958616e7945a5523615 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Tue, 3 Nov 2026 11:07:08 +0100 Subject: [PATCH 8/9] fix(datasource): address second review on Pylon ReplyToIssue [PRD-1873] - give spec messages the real Pylon author shape, so a wrong team check fails - cover a reply that timed out with no status, in the plugin and the client - keep the public reply offered when the thread could not be read - name the typed addresses left out as the team's, via ReplyTarget#sanitize - refuse to sign as a matched Pylon user that has no id - word the channel field as what will happen, not what happened - refresh the stale fetch_all / collect_pages comments and the README Co-Authored-By: Claude Opus 5.5 --- .../forest_admin_datasource_pylon/README.md | 16 ++-- .../forest_admin_datasource_pylon/client.rb | 17 ++-- .../plugins/reply_to_issue.rb | 30 ++++--- .../plugins/reply_to_issue/form_builder.rb | 13 ++- .../plugins/reply_to_issue/operator_lookup.rb | 6 +- .../plugins/reply_to_issue/reply_target.rb | 25 ++++-- .../client/writes_spec.rb | 8 ++ .../plugins/reply_to_issue_spec.rb | 82 +++++++++++++++---- 8 files changed, 143 insertions(+), 54 deletions(-) diff --git a/packages/forest_admin_datasource_pylon/README.md b/packages/forest_admin_datasource_pylon/README.md index 37374f541..a216f8c0e 100644 --- a/packages/forest_admin_datasource_pylon/README.md +++ b/packages/forest_admin_datasource_pylon/README.md @@ -197,11 +197,13 @@ instance — cannot get a public reply; only the internal note is offered. 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. 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. +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 as Pylon words it. Any other failure may come after -Pylon delivered the message, so the action says so and asks to check the issue before sending again. +**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 @@ -218,9 +220,9 @@ Three things to know before granting it. 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. An unreadable thread is - shown as an error, never as an issue without customer message; an internal note does not depend - on it and stays available. + 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 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 00c85a02f..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 @@ -178,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 @@ -203,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. 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 index dce2bf5ae..f0505bac8 100644 --- 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 @@ -69,26 +69,34 @@ 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 = { 'body_html' => body, 'message_id' => state.target.message_id } - if state.target.email? - excluded = state.target.excluded - to = ReplyTarget.unique_addresses(Array(context.form_values[FormBuilder::TO_LABEL]), excluded) - return [:error, 'At least one recipient (To) is required to reply by email.'] if to.empty? - - cc = ReplyTarget.unique_addresses(Array(context.form_values[FormBuilder::CC_LABEL]), - excluded + to.map(&:downcase)) - payload['email_info'] = { 'to_emails' => to, 'cc_emails' => cc } - end + 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)) do + 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) 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 index ac9a2bbdc..90ada283a 100644 --- 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 @@ -40,11 +40,20 @@ def public_reply?(context, opts) def type_field(state) { type: FieldType::ENUM, label: TYPE_LABEL, is_required: true, - enum_values: ->(context) { state.call(context).target ? [PUBLIC_REPLY, INTERNAL_NOTE] : [INTERNAL_NOTE] }, + 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? }, @@ -54,7 +63,7 @@ def channel_field(state, opts) def channel_text(state, opts) return state.error if state.error - return "Reply sent via #{state.target.channel_label}" if state.target + 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 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 index a6d12df80..331bdfeba 100644 --- 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 @@ -15,9 +15,11 @@ def call(datasource, caller) users = datasource.client.fetch_users(include_deactivated: false) matches = users.select { |user| active?(user) && addresses(user).include?(email) } - return Result.new(user: matches.first) if matches.size == 1 + 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: refusal(email, matches.size)) + 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 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 index 3603923ac..89a104dbe 100644 --- 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 @@ -9,7 +9,7 @@ class ReplyTarget EMAIL_SOURCE = 'email'.freeze CHANNEL_LABELS = { 'email' => 'email', 'slack' => 'Slack' }.freeze - attr_reader :message_id, :source, :to, :cc, :excluded + 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 } @@ -24,7 +24,8 @@ def self.resolve(messages, support_emails: []) # keeps a reply out of the support inbox when `support_emails` misses one. def self.team_addresses(messages) messages.select { |message| team_authored?(message) } - .filter_map { |message| email_info(message)['from_email']&.to_s&.downcase } + .map { |message| email_info(message)['from_email'].to_s.strip.downcase } + .reject(&:empty?) end def self.email_info(message) @@ -47,7 +48,18 @@ def initialize(message, excluded) @source = message['source'] @email_info = self.class.email_info(message) @excluded = excluded - @to, @cc = recipients(message, 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? @@ -65,11 +77,8 @@ def channel_label # 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 recipients(message, excluded) - to = self.class.team_authored?(message) ? Array(@email_info['to_emails']) : [@email_info['from_email']] - to = self.class.unique_addresses(to, excluded) - cc = self.class.unique_addresses(Array(@email_info['cc_emails']), excluded + to.map(&:downcase)) - [to, cc] + def default_to(message) + self.class.team_authored?(message) ? @email_info['to_emails'] : [@email_info['from_email']] end end end 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 c736fa78b..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 @@ -171,6 +171,14 @@ def json(payload, status = 200) 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)) 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 index 50a8355c9..a2e7dad2b 100644 --- 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 @@ -30,17 +30,23 @@ 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) - { 'id' => id, 'source' => source, 'is_private' => is_private, 'email_info' => email_info } + 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: []) - message(id, source: 'email', email_info: { 'from_email' => from, 'to_emails' => to, 'cc_emails' => cc }) + 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') @@ -134,8 +140,7 @@ def resolve(*messages, support: []) end it 'reads a message whose email envelope is not an object as carrying none' do - team = { 'user' => { 'id' => 'u1' } } - target = resolve(message('m1', source: 'slack', email_info: '').merge('author' => team), + target = resolve(message('m1', source: 'slack', email_info: '', author: team), message('m2', source: 'slack', email_info: [])) expect(target.message_id).to eq('m2') @@ -143,9 +148,8 @@ def resolve(*messages, support: []) end it 'leaves out the support addresses whatever their case, and duplicates across To and CC' do - team = { 'user' => { 'id' => 'u1' } } target = resolve(email_message('m1', from: 'Support@Us.io', to: ['ada@acme.com', 'ADA@acme.com'], - cc: ['ada@acme.com', 'support@us.io']).merge('author' => team), + cc: ['ada@acme.com', 'support@us.io'], author: team), support: ['support@us.io']) expect(target.to).to eq(['ada@acme.com']) @@ -153,10 +157,9 @@ def resolve(*messages, support: []) end it 'writes again to whom the team last wrote, not to the team itself' do - team = { 'user' => { 'id' => 'u1' } } 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']) - .merge('author' => team)) + 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']) @@ -164,8 +167,7 @@ def resolve(*messages, support: []) end it 'leaves out the addresses the team wrote from, even when support_emails misses them' do - team = { 'user' => { 'id' => 'u1' } } - target = resolve(email_message('m1', from: 'Agent@us.io', to: ['ada@acme.com']).merge('author' => team), + 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']) @@ -219,7 +221,7 @@ def resolve(*messages, support: []) channel = field(register, form::CHANNEL_LABEL) expect(channel[:is_read_only]).to be(true) - expect(channel[:value].call(context)).to eq('Reply sent via Slack') + expect(channel[:value].call(context)).to eq('Replies via Slack') end it 'explains why there is no public reply' do @@ -244,14 +246,24 @@ def resolve(*messages, support: []) 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 }) - action = register - expect(field(action, form::TYPE_LABEL)[:enum_values].call(note)).to eq([form::INTERNAL_NOTE]) - expect(field(action, form::CHANNEL_LABEL)[:if_condition].call(note)).to be(true) + 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 @@ -348,6 +360,26 @@ def values(overrides = {}) ) 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: [])) @@ -442,6 +474,15 @@ def values(overrides = {}) '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) @@ -535,6 +576,13 @@ def run_as(caller, opts = {}) 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) From 20ff8e192be4502e5a41286962d4e8080074cd1c Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Wed, 4 Nov 2026 09:15:17 +0100 Subject: [PATCH 9/9] test(datasource): assert the reply payload in the Pylon ReplyToIssue specs [PRD-1873] Co-Authored-By: Claude Opus 5.5 --- .../plugins/reply_to_issue_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 index a2e7dad2b..b97a010f7 100644 --- 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 @@ -405,7 +405,7 @@ def values(overrides = {}) 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) + expect(client).to have_received(:reply_to_issue).with('i1', hash_including('body_html' => '

Hi

')) end it 'refuses a public reply on an issue with no customer message' do @@ -507,6 +507,7 @@ def values(overrides = {}) run_action(register, values(form::MESSAGE_LABEL => '

')) expect(client).to have_received(:reply_to_issue) + .with('i1', hash_including('body_html' => '

')) end end