From 6598012c26256a182338da29c75651b5e9470afb Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Wed, 19 Aug 2026 19:10:13 +0200 Subject: [PATCH 1/3] feat(pylon): close and create-with-notification action plugins Two plugins at parity with the Ruby Zendesk ones, built on the write primitives of the previous commit. CloseIssue moves the selected issues to a state, `closed` unless told otherwise -- a custom status slug is taken as readily, Pylon accepting one wherever it accepts a standard state. One variant per scope rather than the four Zendesk builds, there being a single terminal state here. Each id is written under its own rescue and named in the message, so a batch that only half applied never reads as a plain success. CreateIssueWithNotification opens an issue and delivers its first message. Pylon says the delivery outright where Zendesk infers it from a public comment: `destination_metadata.destination` names the channel, and no metadata at all is what leaves the issue internal, which is what the "send as internal note" checkbox writes. The form carries no Type field, `POST /issues` taking none, and the priority it does carry is never read back -- no Pylon read returns one. Both find the issues to act on through one option: `issue_id_field` names a column of the host collection, and its absence falls back to the primary keys, which is what an action registered on PylonIssue acts on. Snooze is left out until its endpoint is confirmed against a live organization; IssueTargets and the messages module are already shared. Co-Authored-By: Claude Opus 5 (1M context) --- .../issue_enums.rb | 21 ++ .../plugins/close_issue.rb | 101 +++++ .../plugins/close_issue/messages.rb | 52 +++ .../plugins/create_issue_with_notification.rb | 109 ++++++ .../form_builder.rb | 153 ++++++++ .../create_issue_with_notification/payload.rb | 70 ++++ .../plugins/issue_targets.rb | 39 ++ .../plugins/close_issue_spec.rb | 214 +++++++++++ .../create_issue_with_notification_spec.rb | 351 ++++++++++++++++++ 9 files changed, 1110 insertions(+) create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/issue_enums.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/close_issue.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/close_issue/messages.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification/form_builder.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification/payload.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/issue_targets.rb create mode 100644 packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/plugins/close_issue_spec.rb create mode 100644 packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/plugins/create_issue_with_notification_spec.rb diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/issue_enums.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/issue_enums.rb new file mode 100644 index 000000000..220fb6866 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/issue_enums.rb @@ -0,0 +1,21 @@ +module ForestAdminDatasourcePylon + # The closed sets `POST /issues` and `PATCH /issues/{id}` document, shared by + # the plugins building forms over them. + module IssueEnums + # Accepted on a create, and absent from every read: Pylon never returns the + # priority of an issue, which is why no column carries it. + PRIORITY = %w[urgent high medium low].freeze + + # Where the first message of a created issue is delivered. `internal` is the + # absence of a delivery, and travels as no `destination_metadata` at all. + DESTINATION = %w[email slack in_app_chat customer_portal sms whatsapp internal].freeze + + INTERNAL_DESTINATION = 'internal'.freeze + + # The states Pylon ships. An organization defines its own on top of them, so + # this list is what a form offers, never what a write is checked against. + STANDARD_STATES = %w[new waiting_on_you waiting_on_customer on_hold closed].freeze + + CLOSED_STATE = 'closed'.freeze + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/close_issue.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/close_issue.rb new file mode 100644 index 000000000..602fcc67b --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/close_issue.rb @@ -0,0 +1,101 @@ +module ForestAdminDatasourcePylon + module Plugins + # Moves the selected issues to a state, `closed` unless told otherwise. + # + # One variant per scope, where the Zendesk plugin builds four: Zendesk has + # two terminal statuses to tell apart, Pylon has `closed` and, past it, the + # custom status slugs an organization defines — which the `state` option + # takes, rather than a second dimension of action names nobody would read. + # + # The state is written through the client rather than through the + # collection: the action is registered on the host collection, which is + # rarely PylonIssue, and going through a collection would mean resolving it + # from a datasource the plugin was not given. + class CloseIssue < ForestAdminDatasourceCustomizer::Plugins::Plugin + BaseAction = ForestAdminDatasourceCustomizer::Decorators::Action::BaseAction + ActionScope = ForestAdminDatasourceCustomizer::Decorators::Action::Types::ActionScope + ForestException = ForestAdminDatasourceToolkit::Exceptions::ForestException + + SCOPE_KEYS = %i[single bulk].freeze + SCOPES = { single: ActionScope::SINGLE, bulk: ActionScope::BULK }.freeze + NAMES = { single: 'Close Pylon issue', bulk: 'Close selected Pylon issues' }.freeze + NAME_OPTIONS = { single: :action_name, bulk: :bulk_action_name }.freeze + + def run(_datasource_customizer, collection_customizer = nil, options = {}) + opts = options.is_a?(Hash) ? options : {} + datasource = opts[:datasource] + raise ForestException, 'CloseIssue plugin requires :datasource' unless datasource + raise ForestException, 'CloseIssue plugin requires a collection' unless collection_customizer + + state = normalize_state(opts[:state]) + + normalize_scopes(opts[:scopes]).each do |scope_key| + collection_customizer.add_action(name_for(scope_key, opts), + build_action(datasource, SCOPES[scope_key], state, opts[:issue_id_field])) + end + end + + private + + # Left unchecked against `STANDARD_STATES`: Pylon takes the slug of a + # custom status just as well, and refusing one would refuse the very + # workflow an organization built. + def normalize_state(value) + state = value.nil? ? IssueEnums::CLOSED_STATE : value.to_s + return state unless state.strip.empty? + + raise ForestException, 'CloseIssue :state cannot be empty.' + end + + def normalize_scopes(value) + scopes = Array(value).map(&:to_sym).uniq + scopes = SCOPE_KEYS if scopes.empty? + unknown = scopes - SCOPE_KEYS + return scopes if unknown.empty? + + raise ForestException, + "Unknown CloseIssue scopes: #{unknown.join(", ")}. Allowed: #{SCOPE_KEYS.join(", ")}." + end + + def name_for(scope_key, opts) + opts[NAME_OPTIONS[scope_key]] || NAMES[scope_key] + end + + def build_action(datasource, scope, state, issue_id_field) + BaseAction.new(scope: scope, &executor(datasource, state, issue_id_field)) + end + + def executor(datasource, state, issue_id_field) + lambda do |context, result_builder| + ids = IssueTargets.resolve_issue_ids(context, issue_id_field) + next result_builder.error(message: Messages.no_target(issue_id_field)) if ids.empty? + + succeeded, failed = apply_state(datasource, ids, state) + next result_builder.error(message: Messages.error(failed, state)) if succeeded.empty? + + result_builder.success(message: Messages.success(succeeded, failed, state)) + end + end + + # One rescue per id: a single issue Pylon refuses — deleted, or outside + # the token's scope — must not cost the operator the rest of a selection, + # and what failed is named in the message rather than left to a log. + def apply_state(datasource, ids, state) + succeeded = [] + failed = [] + + ids.each do |id| + datasource.client.update_issue(id, 'state' => state) + succeeded << id + rescue StandardError => e + ForestAdminDatasourcePylon.logger.warn( + "[forest_admin_datasource_pylon] failed to move issue #{id} to '#{state}': #{e.class}: #{e.message}" + ) + failed << [id, "#{e.class}: #{e.message}"] + end + + [succeeded, failed] + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/close_issue/messages.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/close_issue/messages.rb new file mode 100644 index 000000000..eb0e07178 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/close_issue/messages.rb @@ -0,0 +1,52 @@ +module ForestAdminDatasourcePylon + module Plugins + class CloseIssue + # What the operator reads once the batch ran. Every id that failed is + # named: an action reporting a plain success over a batch it only half + # applied is the one thing the panel cannot recover from. + module Messages + module_function + + def success(succeeded, failed, state) + [succeeded_phrase(succeeded, state), failed_phrase(failed)].compact.join(' ') + end + + def error(failed, state) + return "Failed to #{verb(state)} issue #{failed.first.first}: #{failed.first.last}" if failed.size == 1 + + "Failed to #{verb(state)} all #{failed.size} issues. First error: #{failed.first.last}" + end + + def no_target(field) + return 'No Pylon issue selected.' if field.nil? + + "No Pylon issue id found in '#{field}'." + end + + def succeeded_phrase(succeeded, state) + return nil if succeeded.empty? + + return "Issue #{succeeded.first} #{past_verb(state)}." if succeeded.size == 1 + + "#{succeeded.size} issues #{past_verb(state)}." + end + + def failed_phrase(failed) + return nil if failed.empty? + + "#{failed.size} failed: #{failed.map(&:first).join(", ")}." + end + + # A custom status is named as it is, where the state every organization + # has reads as the verb an operator used to fire the action. + def verb(state) + state == IssueEnums::CLOSED_STATE ? 'close' : "move to #{state}" + end + + def past_verb(state) + state == IssueEnums::CLOSED_STATE ? 'closed' : "moved to #{state}" + end + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification.rb new file mode 100644 index 000000000..3f8467e5a --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification.rb @@ -0,0 +1,109 @@ +module ForestAdminDatasourcePylon + module Plugins + # Opens a Pylon issue and delivers its first message to the requester. + # + # Pylon creates the contact on the fly from the form's email, so the action + # can be registered on any host collection — no relation to Pylon needed. + # + # Where Zendesk notifies as a side effect of a public comment, Pylon says it + # outright: `destination_metadata.destination` names the channel the + # issue's `body_html` is delivered through, and no `destination_metadata` at + # all is what leaves the issue internal. The "Send as internal note" + # checkbox is that choice, worded the way the Zendesk plugin words it. + # + # The form is FormBuilder's, the wire payload is Payload's; what is left + # here is the registration, its options, and what the operator reads back. + class CreateIssueWithNotification < ForestAdminDatasourceCustomizer::Plugins::Plugin + BaseAction = ForestAdminDatasourceCustomizer::Decorators::Action::BaseAction + ActionScope = ForestAdminDatasourceCustomizer::Decorators::Action::Types::ActionScope + ForestException = ForestAdminDatasourceToolkit::Exceptions::ForestException + + NAME = 'Create Pylon issue and notify'.freeze + + def run(_datasource_customizer, collection_customizer = nil, options = {}) + options = {} unless options.is_a?(Hash) + datasource = options[:datasource] + raise ForestException, 'CreateIssueWithNotification plugin requires :datasource' unless datasource + raise ForestException, 'CreateIssueWithNotification plugin requires a collection' unless collection_customizer + + opts = options.except(:datasource) + opts[:email_templates] = Array(opts[:email_templates]).compact + opts[:destination] = normalize_destination(opts[:destination]) + opts[:priority_override] = normalize_priority(opts[:priority_override]) + + collection_customizer.add_action(opts[:action_name] || NAME, build_action(datasource, opts)) + end + + private + + def normalize_destination(value) + return Payload::EMAIL_DESTINATION if value.nil? + + normalize(value, IssueEnums::DESTINATION, 'destination') + end + + def normalize_priority(value) + return nil unless Payload.present?(value) + + normalize(value, IssueEnums::PRIORITY, 'priority') + end + + def normalize(value, allowed, label) + normalized = value.to_s + return normalized if allowed.include?(normalized) + + raise ForestException, + "Unknown CreateIssueWithNotification #{label}: #{normalized}. Allowed: #{allowed.join(", ")}." + end + + def build_action(datasource, opts) + BaseAction.new(scope: ActionScope::SINGLE, form: FormBuilder.build(opts), &executor(datasource, opts)) + end + + def executor(datasource, opts) + lambda do |context, result_builder| + values = context.form_values + email = values['Requester email'] + next result_builder.error(message: 'Requester email is required.') unless Payload.present?(email) + + issue = datasource.client.create_issue(Payload.build(values, email, opts)) + writeback = write_back_issue_id(context, opts[:issue_id_field], issue['id']) + result_builder.success(message: success_message(issue, values, opts, writeback)) + end + end + + # Best-effort: Pylon has no transaction to roll back, and the issue exists + # whether or not the host record could be stamped with its id. + def write_back_issue_id(context, field, issue_id) + return :skipped if field.nil? + + context.collection.update(context.filter, { field => issue_id }) + :ok + rescue StandardError => e + ForestAdminDatasourcePylon.logger.warn( + "[forest_admin_datasource_pylon] failed to store the issue id in '#{field}': #{e.class}: #{e.message}" + ) + [:failed, "#{e.class}: #{e.message}"] + end + + def success_message(issue, values, opts, writeback) + base = base_success_message(issue, values, opts) + return base unless writeback.is_a?(Array) && writeback.first == :failed + + "#{base} (warning: could not store the issue id on the record: #{writeback.last})" + end + + # The number is what an operator recognises an issue by; the id stands in + # when Pylon answered without one. + def base_success_message(issue, values, opts) + reference = issue['number'] || issue['id'] + destination = Payload.destination_for(values, opts) + if Payload.internal?(destination) + return "Issue ##{reference} created (internal, the requester was not contacted)." + end + + "Issue ##{reference} created and the requester notified by #{destination.tr("_", " ")}." + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification/form_builder.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification/form_builder.rb new file mode 100644 index 000000000..ba0e7280d --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification/form_builder.rb @@ -0,0 +1,153 @@ +require 'cgi' + +module ForestAdminDatasourcePylon + module Plugins + class CreateIssueWithNotification + module FormBuilder + FieldType = ForestAdminDatasourceCustomizer::Decorators::Action::Types::FieldType + + NO_TEMPLATE = 'No template'.freeze + TOKEN_RE = /\{\{\s*record\.([a-zA-Z_][a-zA-Z0-9_]*)\s*\}\}/ + + module_function + + # ActionCollectionDecorator rejects forms that mix Page elements with + # non-Page elements, so each mode (flat / wizard) stays homogeneous. + def build(opts) + body = body_fields(opts) + return body if opts[:email_templates].empty? + + [ + { type: 'Layout', component: 'Page', next_button_label: 'Continue', + elements: [template_field(opts[:email_templates])] }, + { type: 'Layout', component: 'Page', previous_button_label: 'Back', + elements: body } + ] + end + + # No Type field, unlike the Zendesk form: `POST /issues` does not take + # one -- Pylon accepts `type` on an update only, which is what + # `Issue::UPDATE_ONLY` already says. + def body_fields(opts) + fields = [requester_field(opts[:requester_email_default]), + subject_field(opts[:default_subject]), + message_field(opts[:default_message], opts[:email_templates])] + fields << priority_field unless present?(opts[:priority_override]) + fields << internal_note_field if opts[:show_internal_note] + fields + end + + def requester_field(default) + { type: FieldType::STRING, label: 'Requester email', is_required: true, + description: 'Email of the Pylon requester; the contact is created on the fly when it is unknown. ' \ + 'Pre-filled from the selected record when available.', + default_value: requester_default(default) } + end + + def template_field(templates) + { type: FieldType::ENUM, label: 'Template', is_required: true, + enum_values: [NO_TEMPLATE] + templates.map { |t| t[:title] }, + default_value: NO_TEMPLATE, + description: 'Pick a template to pre-fill the Message on the next page.' } + end + + def subject_field(default_subject) + { type: FieldType::STRING, label: 'Subject', is_required: true, + default_value: template_default(default_subject, escape_html: false) } + end + + def message_field(default_message, templates) + field = { type: FieldType::STRING, label: 'Message', widget: 'RichText', is_required: true, + description: 'The body of the issue (HTML). Unless it is sent as an internal note, this is ' \ + 'the message Pylon delivers to the requester.' } + return field.merge(default_value: template_default(default_message, escape_html: true)) if templates.empty? + + # `value:` (not `default_value:`) — drop_default runs once (data + # key sticks after the first render); drop_deferred re-evaluates + # on every fetch, so Template changes re-fire the message proc. + field.merge(value: message_value(templates)) + end + + # No default: Pylon applies its own when the key is absent, and no + # priority is ever read back — the issue payload does not carry one, so + # nothing in Forest will show the operator what they picked. + def priority_field + { type: FieldType::ENUM, label: 'Priority', enum_values: IssueEnums::PRIORITY, + description: 'Set on creation only; Pylon does not return the priority of an issue, so it is not ' \ + 'shown anywhere in Forest afterwards.' } + end + + def internal_note_field + { type: FieldType::BOOLEAN, label: 'Send as internal note', + description: 'When checked, the issue is created without contacting the requester.', + default_value: false } + end + + def requester_default(value) + return nil if value.nil? + return template_default(value, escape_html: false) if value.is_a?(String) + + lambda do |context| + record = fetch_record(context) + record.empty? ? nil : value.call(record) + rescue StandardError => e + ForestAdminDatasourcePylon.logger.warn( + "[forest_admin_datasource_pylon] requester_email_default resolver raised: #{e.class}: #{e.message}" + ) + nil + end + end + + def template_default(template, escape_html:) + return nil unless present?(template) + return template unless template.match?(TOKEN_RE) + + ->(context) { interpolate(template, fetch_record(context), escape_html: escape_html) } + end + + # Returns nil unless Template was just changed, so set_watch_changes + # carries over the user's current Message edits between renders. + def message_value(templates) + by_title = templates.to_h { |t| [t[:title], t[:content].to_s] } + lambda do |context| + return nil unless context.field_changed?('Template') + + title = context.get_form_value('Template') + return '' if title == NO_TEMPLATE + + content = by_title[title].to_s + return content unless content.match?(TOKEN_RE) + + interpolate(content, fetch_record(context), escape_html: true) + end + end + + def fetch_record(context) + context.get_record([]) || {} + rescue StandardError => e + ForestAdminDatasourcePylon.logger.warn( + "[forest_admin_datasource_pylon] failed to fetch record for token interpolation: #{e.class}: #{e.message}" + ) + {} + end + + # The message ships as `body_html` and is delivered as such — an + # unescaped `<` or `&` coming from a record value would break the + # outbound message or smuggle markup into it. + def interpolate(template, record, escape_html:) + template.gsub(TOKEN_RE) do + key = ::Regexp.last_match(1) + value = record[key] + next '' if value.nil? + + escape_html ? CGI.escapeHTML(value.to_s) : value.to_s + end + end + + def present?(value) + !value.nil? && value.to_s != '' + end + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification/payload.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification/payload.rb new file mode 100644 index 000000000..e85cfcb3a --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification/payload.rb @@ -0,0 +1,70 @@ +module ForestAdminDatasourcePylon + module Plugins + class CreateIssueWithNotification + # What the filled form becomes on the wire, where FormBuilder owns what + # the operator fills. + module Payload + # Only meaningful on an email delivery: Pylon reads the sending address + # and the copies off the email app they belong to. + EMAIL_DESTINATION = 'email'.freeze + + module_function + + def build(values, email, opts) + payload = { + 'title' => values['Subject'], + 'body_html' => values['Message'], + # Pylon wants a name alongside the address when it creates the + # contact; derive it from the local part. It is ignored when the + # contact already exists. + 'requester_email' => email, + 'requester_name' => derive_requester_name(email) + } + priority = opts[:priority_override] || values['Priority'] + payload['priority'] = priority if present?(priority) + + destination = destination_for(values, opts) + payload['destination_metadata'] = metadata(destination, opts) unless internal?(destination) + payload + end + + # The checkbox wins over the configured destination: it is the + # operator's call, made on the record they are looking at. + def destination_for(values, opts) + truthy?(values['Send as internal note']) ? IssueEnums::INTERNAL_DESTINATION : opts[:destination] + end + + # An internal issue travels as no metadata at all rather than as + # `{destination: 'internal'}`: that is the form the API reference names + # for "do not contact the requester", and the one that stays right if + # Pylon ever adds a required companion field to a real destination. + def internal?(destination) + destination == IssueEnums::INTERNAL_DESTINATION + end + + def metadata(destination, opts) + metadata = { 'destination' => destination } + return metadata unless destination == EMAIL_DESTINATION + + metadata['email'] = opts[:sender_email] if present?(opts[:sender_email]) + metadata['email_ccs'] = Array(opts[:email_ccs]) if Array(opts[:email_ccs]).any? + metadata['email_bccs'] = Array(opts[:email_bccs]) if Array(opts[:email_bccs]).any? + metadata + end + + def derive_requester_name(email) + local = email.to_s.split('@').first.to_s + local.empty? ? email.to_s : local + end + + def truthy?(value) + value == true || value.to_s.casecmp('true').zero? + end + + def present?(value) + !value.nil? && value.to_s != '' + end + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/issue_targets.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/issue_targets.rb new file mode 100644 index 000000000..599b7656c --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/issue_targets.rb @@ -0,0 +1,39 @@ +module ForestAdminDatasourcePylon + module Plugins + # Which Pylon issues an action was fired on. + # + # Two shapes, one option: `issue_id_field` names a column of the host + # collection holding a Pylon issue id — a business collection keeping the + # issue it opened — and, left out, the ids are the primary keys of the + # selected records, which is what an action registered on PylonIssue itself + # acts on. + module IssueTargets + module_function + + # Never raises: a collection whose column was renamed, or a record the + # scope hides, answers "no issue selected" through the action's own + # message rather than through a stack trace in the panel. + def resolve_issue_ids(context, field = nil) + ids = field.nil? ? primary_key_ids(context) : column_ids(context, field) + ids.filter_map do |id| + id.to_s unless id.nil? || id.to_s.empty? + end + rescue StandardError => e + source = field ? "from '#{field}'" : 'from the selected records' + ForestAdminDatasourcePylon.logger.warn( + "[forest_admin_datasource_pylon] failed to resolve the issues to act on #{source}: " \ + "#{e.class}: #{e.message}" + ) + [] + end + + def primary_key_ids(context) + Array(context.get_record_ids) + end + + def column_ids(context, field) + context.get_records([field.to_s]).map { |record| record[field.to_s] } + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/plugins/close_issue_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/plugins/close_issue_spec.rb new file mode 100644 index 000000000..7d628ec64 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/plugins/close_issue_spec.rb @@ -0,0 +1,214 @@ +module ForestAdminDatasourcePylon + # Stand-in for an action context. The executor only asks it which records were + # selected -- through a column of the host collection, or through their + # primary keys -- so the full ActionContext is not needed here. + class FakeCloseContext + def initialize(records: [], record_ids: []) + @records = records + @record_ids = record_ids + end + + def get_records(_fields = []) = @records + + # Named after the ActionContext method it stands in for. + def get_record_ids = @record_ids # rubocop:disable Naming/AccessorMethodName + end + + RSpec.describe Plugins::CloseIssue 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(:issue_id_field) { 'pylon_issue_id' } + 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 + end + + describe '#run' do + it 'registers one variant per scope' do + register + + expect(collection_customizer.registered.keys) + .to contain_exactly('Close Pylon issue', 'Close selected Pylon issues') + end + + it 'binds the matching ActionScope to each variant' do + registered = register + + expect(registered['Close Pylon issue'].scope).to eq(action_scope::SINGLE) + expect(registered['Close selected Pylon issues'].scope).to eq(action_scope::BULK) + end + + it 'honors :scopes to keep only the requested variant' do + register(scopes: %i[single]) + + expect(collection_customizer.registered.keys).to contain_exactly('Close Pylon issue') + end + + it 'accepts string scopes as well as symbols' do + register(scopes: %w[bulk]) + + expect(collection_customizer.registered.keys).to contain_exactly('Close selected Pylon issues') + end + + it 'honors the custom names of both variants' do + register(action_name: 'Resolve', bulk_action_name: 'Resolve all') + + expect(collection_customizer.registered.keys).to contain_exactly('Resolve', 'Resolve all') + end + + it 'raises a ForestException on an unknown scope' do + expect { register(scopes: %i[single weird]) }.to raise_error(forest_exception, /Unknown.*weird/) + end + + it 'raises a ForestException on an empty state' do + expect { register(state: ' ') }.to raise_error(forest_exception, /state cannot be empty/) + end + + it 'raises a ForestException without :datasource' do + expect { described_class.new.run(nil, collection_customizer, {}) } + .to raise_error(forest_exception, /datasource/) + end + + it 'raises a ForestException without a collection' do + expect { described_class.new.run(nil, nil, datasource: datasource) } + .to raise_error(forest_exception, /collection/) + end + end + + describe 'the issues an execution acts on' do + let(:single) { register(scopes: %i[single], issue_id_field: issue_id_field)['Close Pylon issue'] } + let(:on_primary_key) { register(scopes: %i[single])['Close Pylon issue'] } + + it 'reads the id from the configured column of the host record' do + allow(client).to receive(:update_issue) + + result = single.execute.call(FakeCloseContext.new(records: [{ issue_id_field => 'i1' }]), result_builder) + + expect(client).to have_received(:update_issue).with('i1', 'state' => 'closed') + expect(result[:type]).to eq('Success') + expect(result[:message]).to include('Issue i1 closed.') + end + + # What an action registered on PylonIssue itself acts on: no column to + # name, the record's own primary key is the issue. + it 'reads the primary keys when no column is configured' do + allow(client).to receive(:update_issue) + + result = on_primary_key.execute.call(FakeCloseContext.new(record_ids: %w[i7]), result_builder) + + expect(client).to have_received(:update_issue).with('i7', 'state' => 'closed') + expect(result[:message]).to include('Issue i7 closed.') + end + + it 'returns an error naming the column when no host record carries an id' do + allow(client).to receive(:update_issue) + + result = single.execute.call(FakeCloseContext.new(records: [{ issue_id_field => nil }]), result_builder) + + expect(client).not_to have_received(:update_issue) + expect(result[:type]).to eq('Error') + expect(result[:message]).to include(issue_id_field) + end + + it 'returns an error when nothing was selected' do + allow(client).to receive(:update_issue) + + result = on_primary_key.execute.call(FakeCloseContext.new(record_ids: []), result_builder) + + expect(client).not_to have_received(:update_issue) + expect(result[:message]).to eq('No Pylon issue selected.') + end + + it 'logs and answers an error when the records cannot be read at all' do + context = instance_double(ForestAdminDatasourceCustomizer::Decorators::Action::Context::ActionContextSingle) + allow(context).to receive(:get_records).and_raise(StandardError, 'boom') + allow(ForestAdminDatasourcePylon.logger).to receive(:warn) + allow(client).to receive(:update_issue) + + result = single.execute.call(context, result_builder) + + expect(client).not_to have_received(:update_issue) + expect(result[:type]).to eq('Error') + expect(ForestAdminDatasourcePylon.logger).to have_received(:warn) + .with(a_string_including(issue_id_field, 'boom')) + end + end + + describe 'the state an execution writes' do + let(:bulk) { register(scopes: %i[bulk])['Close selected Pylon issues'] } + + it 'patches every selected issue' do + allow(client).to receive(:update_issue) + + result = bulk.execute.call(FakeCloseContext.new(record_ids: %w[i1 i2 i3]), result_builder) + + %w[i1 i2 i3].each { |id| expect(client).to have_received(:update_issue).with(id, 'state' => 'closed') } + expect(result[:message]).to include('3 issues closed.') + end + + # An organization defining its own statuses names the slug it wants; Pylon + # takes it exactly like a standard one. + it 'writes the configured state, custom slug included, and says so' do + allow(client).to receive(:update_issue) + action = register(scopes: %i[single], state: 'on_hold')['Close Pylon issue'] + + result = action.execute.call(FakeCloseContext.new(record_ids: %w[i1]), result_builder) + + expect(client).to have_received(:update_issue).with('i1', 'state' => 'on_hold') + expect(result[:message]).to include('Issue i1 moved to on_hold.') + end + end + + describe 'an execution Pylon refuses' do + let(:bulk) { register(scopes: %i[bulk])['Close selected Pylon issues'] } + let(:context) { FakeCloseContext.new(record_ids: %w[i1 i2 i3]) } + + # The point of the per-id rescue: one refusal costs one issue, and the + # operator is told which, rather than reading a success over a batch that + # was only half applied. + it 'keeps going and names what failed' do + allow(client).to receive(:update_issue) + allow(client).to receive(:update_issue).with('i2', anything).and_raise(APIError, 'HTTP 404 not found') + allow(ForestAdminDatasourcePylon.logger).to receive(:warn) + + result = bulk.execute.call(context, result_builder) + + expect(result[:type]).to eq('Success') + expect(result[:message]).to include('2 issues closed.', '1 failed: i2.') + expect(ForestAdminDatasourcePylon.logger).to have_received(:warn).with(a_string_including('i2', 'not found')) + end + + it 'answers an error when every issue failed' do + allow(client).to receive(:update_issue).and_raise(APIError, 'HTTP 403 forbidden') + allow(ForestAdminDatasourcePylon.logger).to receive(:warn).exactly(3).times + + result = bulk.execute.call(context, result_builder) + + expect(result[:type]).to eq('Error') + expect(result[:message]).to include('Failed to close all 3 issues', 'forbidden') + end + + it 'names the single issue that failed' do + allow(client).to receive(:update_issue).and_raise(APIError, 'HTTP 403 forbidden') + allow(ForestAdminDatasourcePylon.logger).to receive(:warn) + action = register(scopes: %i[single])['Close Pylon issue'] + + result = action.execute.call(FakeCloseContext.new(record_ids: %w[i1]), result_builder) + + expect(result[:message]).to include('Failed to close issue i1', 'forbidden') + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/plugins/create_issue_with_notification_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/plugins/create_issue_with_notification_spec.rb new file mode 100644 index 000000000..aea9a0287 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/plugins/create_issue_with_notification_spec.rb @@ -0,0 +1,351 @@ +module ForestAdminDatasourcePylon + # Stand-in for an ActionContextSingle. Not a Struct: `Struct` mixes in + # Enumerable, which already defines `#filter`, so a `:filter` member is both + # a cop violation and genuinely ambiguous. + class FakeCreateContext + attr_reader :form_values, :collection, :filter + + def initialize(form_values: {}, collection: nil, filter: nil, record: {}) + @form_values = form_values + @collection = collection + @filter = filter + @record = record + end + + def get_record(_fields = []) = @record + end + + RSpec.describe Plugins::CreateIssueWithNotification 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(: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 form_values(overrides = {}) + { 'Requester email' => 'ada@acme.test', 'Subject' => 'Boom', + 'Message' => '

it broke

' }.merge(overrides) + end + + def run_action(action, values, context_options = {}) + action.execute.call(FakeCreateContext.new(form_values: values, **context_options), result_builder) + end + + describe '#run' do + it 'registers a SINGLE-scoped action under the default name' do + action = register + + expect(collection_customizer.registered.keys).to contain_exactly(described_class::NAME) + expect(action.scope).to eq(action_scope::SINGLE) + end + + # No Type field, unlike the Zendesk form: POST /issues does not take one. + it 'builds the form Pylon accepts on a create' do + expect(register.form.map { |field| field[:label] }) + .to eq(['Requester email', 'Subject', 'Message', 'Priority']) + end + + it 'drops the Priority field when the priority is imposed' do + expect(register(priority_override: 'urgent').form.map { |field| field[:label] }) + .to eq(['Requester email', 'Subject', 'Message']) + end + + it 'adds the internal-note checkbox when asked for' do + expect(register(show_internal_note: true).form.map { |field| field[:label] }) + .to eq(['Requester email', 'Subject', 'Message', 'Priority', 'Send as internal note']) + end + + it 'splits the form in two pages when templates are configured' do + form = register(email_templates: [{ title: 'Outage', content: 'Sorry' }]).form + + expect(form.map { |element| element[:component] }).to eq(%w[Page Page]) + expect(form.first[:elements].map { |field| field[:label] }).to eq(['Template']) + end + + it 'sends the message as RichText and requires the requester' do + form = register.form + + expect(form.find { |field| field[:label] == 'Message' }[:widget]).to eq('RichText') + expect(form.first[:is_required]).to be(true) + end + + it 'offers the priorities Pylon documents, with no default' do + priority = register.form.find { |field| field[:label] == 'Priority' } + + expect(priority[:enum_values]).to eq(IssueEnums::PRIORITY) + expect(priority[:default_value]).to be_nil + end + + # The plain hashes above only become form elements once the agent builds + # them, and a form the factory refuses -- Page elements mixed with plain + # ones, two fields sharing a label -- fails in the panel, not here. + it 'builds through the real form factory, flat and paged alike' do + flat = register(show_internal_note: true) + paged = register(action_name: 'paged', email_templates: [{ title: 'Outage', content: 'Sorry' }]) + + expect { flat.build_elements.validate_fields_ids }.not_to raise_error + expect { paged.build_elements.validate_fields_ids }.not_to raise_error + expect(flat.static_form).to be(true) + expect(paged.static_form).to be(false) + end + + it 'honors :action_name' do + register + register(action_name: 'Open a ticket') + + expect(collection_customizer.registered.keys).to contain_exactly(described_class::NAME, 'Open a ticket') + end + + it 'raises a ForestException on an unknown destination' do + expect { register(destination: 'pigeon') }.to raise_error(forest_exception, /Unknown.*pigeon/) + end + + it 'raises a ForestException on an unknown priority' do + expect { register(priority_override: 'critical') }.to raise_error(forest_exception, /Unknown.*critical/) + end + + it 'raises a ForestException without :datasource' do + expect { described_class.new.run(nil, collection_customizer, {}) } + .to raise_error(forest_exception, /datasource/) + end + + it 'raises a ForestException without a collection' do + expect { described_class.new.run(nil, nil, datasource: datasource) } + .to raise_error(forest_exception, /collection/) + end + end + + describe 'the issue an execution creates' do + it 'posts the form as an issue delivered to the requester by email' do + allow(client).to receive(:create_issue).and_return({ 'id' => 'i1', 'number' => 12 }) + + result = run_action(register, form_values('Priority' => 'high')) + + expect(client).to have_received(:create_issue).with( + 'title' => 'Boom', 'body_html' => '

it broke

', + 'requester_email' => 'ada@acme.test', 'requester_name' => 'ada', + 'priority' => 'high', 'destination_metadata' => { 'destination' => 'email' } + ) + expect(result[:type]).to eq('Success') + expect(result[:message]).to include('Issue #12 created and the requester notified by email.') + end + + it 'carries the sending address and the copies of an email delivery' do + allow(client).to receive(:create_issue).and_return({ 'id' => 'i1' }) + + run_action(register(sender_email: 'support@acme.test', email_ccs: ['lead@acme.test'], + email_bccs: ['audit@acme.test']), form_values) + + expect(client).to have_received(:create_issue).with( + hash_including('destination_metadata' => { 'destination' => 'email', 'email' => 'support@acme.test', + 'email_ccs' => ['lead@acme.test'], + 'email_bccs' => ['audit@acme.test'] }) + ) + end + + # Those three belong to the email app they are configured on; another + # channel would carry them for nothing. + it 'leaves the email settings out of another channel' do + allow(client).to receive(:create_issue).and_return({ 'id' => 'i1' }) + + run_action(register(destination: 'slack', sender_email: 'support@acme.test'), form_values) + + expect(client).to have_received(:create_issue) + .with(hash_including('destination_metadata' => { 'destination' => 'slack' })) + end + + # "Do not contact the requester" is the absence of the key, which is the + # form the API reference names for it. + it 'sends no destination at all when the operator asks for an internal issue' do + allow(client).to receive(:create_issue).and_return({ 'id' => 'i1', 'number' => 12 }) + + result = run_action(register(show_internal_note: true), + form_values('Send as internal note' => true)) + + expect(client).to have_received(:create_issue).with(hash_excluding('destination_metadata')) + expect(result[:message]).to include('Issue #12 created (internal, the requester was not contacted).') + end + + it 'sends no destination when the plugin itself is configured as internal' do + allow(client).to receive(:create_issue).and_return({ 'id' => 'i1' }) + + run_action(register(destination: 'internal'), form_values) + + expect(client).to have_received(:create_issue).with(hash_excluding('destination_metadata')) + end + + it 'imposes the configured priority over the form' do + allow(client).to receive(:create_issue).and_return({ 'id' => 'i1' }) + + run_action(register(priority_override: 'urgent'), form_values('Priority' => 'low')) + + expect(client).to have_received(:create_issue).with(hash_including('priority' => 'urgent')) + end + + it 'omits the priority when none was picked' do + allow(client).to receive(:create_issue).and_return({ 'id' => 'i1' }) + + run_action(register, form_values) + + expect(client).to have_received(:create_issue).with(hash_excluding('priority')) + end + + it 'falls back on the id when Pylon answers without a number' do + allow(client).to receive(:create_issue).and_return({ 'id' => 'i1' }) + + expect(run_action(register, form_values)[:message]).to include('Issue #i1 created') + end + + it 'refuses to post without a requester' do + allow(client).to receive(:create_issue) + + result = run_action(register, form_values('Requester email' => '')) + + expect(client).not_to have_received(:create_issue) + expect(result[:type]).to eq('Error') + expect(result[:message]).to include('Requester email is required.') + end + end + + describe 'writing the issue id back on the host record' do + let(:collection) { instance_double(ForestAdminDatasourceCustomizer::Context::RelaxedWrappers::RelaxedCollection) } + let(:context_options) { { collection: collection, filter: :a_filter } } + + it 'updates the configured column' do + allow(client).to receive(:create_issue).and_return({ 'id' => 'i1', 'number' => 12 }) + allow(collection).to receive(:update) + + result = run_action(register(issue_id_field: 'pylon_issue_id'), form_values, context_options) + + expect(collection).to have_received(:update).with(:a_filter, { 'pylon_issue_id' => 'i1' }) + expect(result[:type]).to eq('Success') + end + + it 'writes nothing when no column is configured' do + allow(client).to receive(:create_issue).and_return({ 'id' => 'i1' }) + allow(collection).to receive(:update) + + run_action(register, form_values, context_options) + + expect(collection).not_to have_received(:update) + end + + # Pylon has no transaction: the issue exists, so the action succeeds and + # the failed writeback is a warning inside the success message. + it 'degrades the success message when the column cannot be written' do + allow(client).to receive(:create_issue).and_return({ 'id' => 'i1', 'number' => 12 }) + allow(collection).to receive(:update).and_raise(StandardError, 'column is read-only') + allow(ForestAdminDatasourcePylon.logger).to receive(:warn) + + result = run_action(register(issue_id_field: 'pylon_issue_id'), form_values, context_options) + + expect(result[:type]).to eq('Success') + expect(result[:message]).to include('Issue #12 created', 'could not store the issue id', 'read-only') + expect(ForestAdminDatasourcePylon.logger).to have_received(:warn) + .with(a_string_including('pylon_issue_id', 'read-only')) + end + end + + describe 'the record tokens a default is written with' do + let(:context) { FakeCreateContext.new(record: { 'email' => 'ada@acme.test', 'name' => 'Ada & Co ' }) } + + it 'keeps a token-free default as the literal it is' do + expect(register(default_subject: 'Outage').form[1][:default_value]).to eq('Outage') + end + + it 'interpolates the subject without escaping it' do + subject_field = register(default_subject: 'Outage for {{ record.name }}').form[1] + + expect(subject_field[:default_value].call(context)).to eq('Outage for Ada & Co ') + end + + # The message ships as body_html and is delivered as such: a record value + # carrying markup must not become markup. + it 'escapes the html of a value interpolated into the message' do + message_field = register(default_message: '

Hi {{ record.name }}

').form[2] + + expect(message_field[:default_value].call(context)).to eq('

Hi Ada & Co <boss>

') + end + + # A default that cannot be resolved must not take the form down with it: + # the operator gets the template with its tokens emptied, and types. + it 'logs and interpolates nothing when the record cannot be read' do + allow(ForestAdminDatasourcePylon.logger).to receive(:warn) + unreadable = instance_double( + ForestAdminDatasourceCustomizer::Decorators::Action::Context::ActionContextSingle + ) + allow(unreadable).to receive(:get_record).and_raise(StandardError, 'boom') + subject_field = register(default_subject: 'Outage for {{ record.name }}').form[1] + + expect(subject_field[:default_value].call(unreadable)).to eq('Outage for ') + expect(ForestAdminDatasourcePylon.logger).to have_received(:warn) + .with(a_string_including('token interpolation', 'boom')) + end + + it 'reads an empty string for a token the record has no value for' do + subject_field = register(default_subject: 'Hi {{ record.unknown }}!').form[1] + + expect(subject_field[:default_value].call(context)).to eq('Hi !') + end + + it 'resolves a lambda requester default against the record' do + field = register(requester_email_default: ->(record) { record['email'] }).form.first + + expect(field[:default_value].call(context)).to eq('ada@acme.test') + end + + it 'answers nothing when the requester resolver raises' do + allow(ForestAdminDatasourcePylon.logger).to receive(:warn) + field = register(requester_email_default: ->(_record) { raise 'boom' }).form.first + + expect(field[:default_value].call(context)).to be_nil + expect(ForestAdminDatasourcePylon.logger).to have_received(:warn).with(a_string_including('boom')) + end + end + + describe 'picking a template' do + let(:templates) { [{ title: 'Outage', content: '

Sorry {{ record.name }}

' }] } + let(:message_field) do + register(email_templates: templates).form.last[:elements].find { |field| field[:label] == 'Message' } + end + + def template_context(changed:, chosen:, record: {}) + context = instance_double(ForestAdminDatasourceCustomizer::Decorators::Action::Context::ActionContextSingle) + allow(context).to receive(:field_changed?).with('Template').and_return(changed) + allow(context).to receive(:get_form_value).with('Template').and_return(chosen) + allow(context).to receive(:get_record).and_return(record) + context + end + + it 'fills the message with the chosen template, tokens escaped' do + value = message_field[:value].call(template_context(changed: true, chosen: 'Outage', + record: { 'name' => 'Ada & Co' })) + + expect(value).to eq('

Sorry Ada & Co

') + end + + it 'clears the message when the template is taken back' do + expect(message_field[:value].call(template_context(changed: true, chosen: 'No template'))).to eq('') + end + + # Nil means "leave what the operator typed", which is what keeps their + # edits across the re-renders of the other fields. + it 'leaves the message alone while the template does not change' do + expect(message_field[:value].call(template_context(changed: false, chosen: 'Outage'))).to be_nil + end + end + end +end From 389318624997f96abf641e4f794335230ddd9f79 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Mon, 24 Aug 2026 16:45:44 +0200 Subject: [PATCH 2/3] fix(pylon): surface Pylon refusals and require the sending address A 4xx on POST /issues escaped the create action as a raw APIError, which the agent answers with a generic message and does not log below 500: the operator read "Bad request" where Pylon had named the reason. It now reaches them as the action's own error, message intact, and is logged. The reason it named turned out to be structural: Pylon refuses an email delivery that does not carry the sending address, so :sender_email is now mandatory when the destination is email -- refused at registration, where it names the option, rather than at the first execution. Co-Authored-By: Claude Opus 5 (1M context) --- .../plugins/create_issue_with_notification.rb | 36 +++++++++++++++- .../create_issue_with_notification/payload.rb | 6 ++- .../create_issue_with_notification_spec.rb | 43 ++++++++++++++++--- 3 files changed, 77 insertions(+), 8 deletions(-) diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification.rb index 3f8467e5a..ab6ac61ab 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification.rb @@ -30,6 +30,7 @@ def run(_datasource_customizer, collection_customizer = nil, options = {}) opts[:email_templates] = Array(opts[:email_templates]).compact opts[:destination] = normalize_destination(opts[:destination]) opts[:priority_override] = normalize_priority(opts[:priority_override]) + require_sender_email!(opts) collection_customizer.add_action(opts[:action_name] || NAME, build_action(datasource, opts)) end @@ -42,6 +43,20 @@ def normalize_destination(value) normalize(value, IssueEnums::DESTINATION, 'destination') end + # `POST /issues` refuses an email delivery that does not name the address it + # is sent from, so the option is mandatory there rather than optional. The + # refusal belongs at registration, where it names the option and the agent + # will not boot without it, rather than at the first execution, where it + # reaches the operator as a Pylon 400 on a form they filled correctly. + def require_sender_email!(opts) + return unless opts[:destination] == Payload::EMAIL_DESTINATION + return if Payload.present?(opts[:sender_email]) + + raise ForestException, + 'CreateIssueWithNotification requires :sender_email when the destination is email. ' \ + 'It must be one of the addresses configured in the Pylon email app.' + end + def normalize_priority(value) return nil unless Payload.present?(value) @@ -66,12 +81,31 @@ def executor(datasource, opts) email = values['Requester email'] next result_builder.error(message: 'Requester email is required.') unless Payload.present?(email) - issue = datasource.client.create_issue(Payload.build(values, email, opts)) + issue = create_issue(datasource, Payload.build(values, email, opts)) + next result_builder.error(message: issue.last) if issue.is_a?(Array) + writeback = write_back_issue_id(context, opts[:issue_id_field], issue['id']) result_builder.success(message: success_message(issue, values, opts, writeback)) end end + # A 4xx is Pylon naming what the operator filled in, and reaches them as + # the action's own error, message intact: raised, it would leave the agent + # to answer 'Unexpected error' — APIError is none of the classes whose + # message the translator passes through — and to log nothing either, its + # status being under 500. Anything else is Pylon or the network failing, + # which no edit of the form would change, and stays the 500 it is. + def create_issue(datasource, payload) + datasource.client.create_issue(payload) + rescue APIError => e + raise unless (400..499).cover?(e.status.to_i) + + ForestAdminDatasourcePylon.logger.warn( + "[forest_admin_datasource_pylon] Pylon refused the issue creation: #{e.message}" + ) + [:rejected, e.message] + end + # Best-effort: Pylon has no transaction to roll back, and the issue exists # whether or not the host record could be stamped with its id. def write_back_issue_id(context, field, issue_id) diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification/payload.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification/payload.rb index e85cfcb3a..54ea3f9cb 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification/payload.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification/payload.rb @@ -5,7 +5,9 @@ class CreateIssueWithNotification # the operator fills. module Payload # Only meaningful on an email delivery: Pylon reads the sending address - # and the copies off the email app they belong to. + # and the copies off the email app they belong to. The address is + # mandatory there — `CreateIssueWithNotification` refuses to register an + # email delivery without one. EMAIL_DESTINATION = 'email'.freeze module_function @@ -46,7 +48,7 @@ def metadata(destination, opts) metadata = { 'destination' => destination } return metadata unless destination == EMAIL_DESTINATION - metadata['email'] = opts[:sender_email] if present?(opts[:sender_email]) + metadata['email'] = opts[:sender_email] metadata['email_ccs'] = Array(opts[:email_ccs]) if Array(opts[:email_ccs]).any? metadata['email_bccs'] = Array(opts[:email_bccs]) if Array(opts[:email_bccs]).any? metadata diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/plugins/create_issue_with_notification_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/plugins/create_issue_with_notification_spec.rb index aea9a0287..eb3772635 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/plugins/create_issue_with_notification_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/plugins/create_issue_with_notification_spec.rb @@ -30,8 +30,12 @@ def add_action(name, action) = @registered[name] = action end.new end + # `sender_email` rides along by default: Pylon refuses an email delivery + # that does not name the sending address, so the plugin refuses to register + # one, and an email destination is what the options default to. def register(opts = {}) - described_class.new.run(nil, collection_customizer, { datasource: datasource }.merge(opts)) + options = { datasource: datasource, sender_email: 'support@acme.test' }.merge(opts) + described_class.new.run(nil, collection_customizer, options) collection_customizer.registered[opts[:action_name] || described_class::NAME] end @@ -113,6 +117,17 @@ def run_action(action, values, context_options = {}) expect { register(destination: 'pigeon') }.to raise_error(forest_exception, /Unknown.*pigeon/) end + it 'refuses an email delivery that names no sending address' do + expect { described_class.new.run(nil, collection_customizer, datasource: datasource) } + .to raise_error(forest_exception, /:sender_email when the destination is email/) + end + + # The address belongs to the email app; the other channels never carry it. + it 'asks for no sending address on another channel' do + expect { described_class.new.run(nil, collection_customizer, datasource: datasource, destination: 'slack') } + .not_to raise_error + end + it 'raises a ForestException on an unknown priority' do expect { register(priority_override: 'critical') }.to raise_error(forest_exception, /Unknown.*critical/) end @@ -137,7 +152,8 @@ def run_action(action, values, context_options = {}) expect(client).to have_received(:create_issue).with( 'title' => 'Boom', 'body_html' => '

it broke

', 'requester_email' => 'ada@acme.test', 'requester_name' => 'ada', - 'priority' => 'high', 'destination_metadata' => { 'destination' => 'email' } + 'priority' => 'high', + 'destination_metadata' => { 'destination' => 'email', 'email' => 'support@acme.test' } ) expect(result[:type]).to eq('Success') expect(result[:message]).to include('Issue #12 created and the requester notified by email.') @@ -146,8 +162,7 @@ def run_action(action, values, context_options = {}) it 'carries the sending address and the copies of an email delivery' do allow(client).to receive(:create_issue).and_return({ 'id' => 'i1' }) - run_action(register(sender_email: 'support@acme.test', email_ccs: ['lead@acme.test'], - email_bccs: ['audit@acme.test']), form_values) + run_action(register(email_ccs: ['lead@acme.test'], email_bccs: ['audit@acme.test']), form_values) expect(client).to have_received(:create_issue).with( hash_including('destination_metadata' => { 'destination' => 'email', 'email' => 'support@acme.test', @@ -161,7 +176,7 @@ def run_action(action, values, context_options = {}) it 'leaves the email settings out of another channel' do allow(client).to receive(:create_issue).and_return({ 'id' => 'i1' }) - run_action(register(destination: 'slack', sender_email: 'support@acme.test'), form_values) + run_action(register(destination: 'slack'), form_values) expect(client).to have_received(:create_issue) .with(hash_including('destination_metadata' => { 'destination' => 'slack' })) @@ -218,6 +233,24 @@ def run_action(action, values, context_options = {}) expect(result[:type]).to eq('Error') expect(result[:message]).to include('Requester email is required.') end + + it 'hands the operator the reason Pylon refused the creation' do + allow(client).to receive(:create_issue) + .and_raise(APIError.new('Pylon API call failed: create(issues): HTTP 400 account_id is required', + status: 400)) + + result = run_action(register, form_values) + + expect(result[:type]).to eq('Error') + expect(result[:message]).to include('account_id is required') + end + + it 'lets a Pylon failure that is not the operator\'s to fix stay an error' do + allow(client).to receive(:create_issue) + .and_raise(APIError.new('Pylon API call failed: create(issues): HTTP 503', status: 503)) + + expect { run_action(register, form_values) }.to raise_error(APIError, /503/) + end end describe 'writing the issue id back on the host record' do From 6f5a00b62173380136799d2f78c37fde14b49cf4 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Mon, 24 Aug 2026 17:05:20 +0200 Subject: [PATCH 3/3] fix(pylon): keep the configured default message in the template wizard Configuring both :default_message and :email_templates silently dropped the default: the Message field only carried `value:`, so the wizard's second page opened on an empty required field, and taking a template back wrote '' rather than the default the operator started from. The field now carries both keys. `default_value:` fills the first render, drop_default running once, and `value:` stays what a Template change re-fires; NO_TEMPLATE restores the default, interpolated when it carries tokens, and still empties the field when none is configured. Also drops STANDARD_STATES, which no caller ever read, and strips the :state option it documented: ' closed ' passed the emptiness check and was then written to Pylon with its spaces, as a status it does not know. Co-Authored-By: Claude Opus 5 (1M context) --- .../issue_enums.rb | 7 ++-- .../plugins/close_issue.rb | 6 +-- .../form_builder.rb | 39 +++++++++++------ .../create_issue_with_notification_spec.rb | 42 ++++++++++++++++++- 4 files changed, 74 insertions(+), 20 deletions(-) diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/issue_enums.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/issue_enums.rb index 220fb6866..b463878ad 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/issue_enums.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/issue_enums.rb @@ -12,10 +12,9 @@ module IssueEnums INTERNAL_DESTINATION = 'internal'.freeze - # The states Pylon ships. An organization defines its own on top of them, so - # this list is what a form offers, never what a write is checked against. - STANDARD_STATES = %w[new waiting_on_you waiting_on_customer on_hold closed].freeze - + # The one state every organization has: the others Pylon ships, and the + # custom ones defined on top of them, are named by the option that writes + # them rather than listed here. CLOSED_STATE = 'closed'.freeze end end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/close_issue.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/close_issue.rb index 602fcc67b..ee4fd9d8c 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/close_issue.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/close_issue.rb @@ -37,12 +37,12 @@ def run(_datasource_customizer, collection_customizer = nil, options = {}) private - # Left unchecked against `STANDARD_STATES`: Pylon takes the slug of a + # Left unchecked against the states Pylon ships: it takes the slug of a # custom status just as well, and refusing one would refuse the very # workflow an organization built. def normalize_state(value) - state = value.nil? ? IssueEnums::CLOSED_STATE : value.to_s - return state unless state.strip.empty? + state = value.nil? ? IssueEnums::CLOSED_STATE : value.to_s.strip + return state unless state.empty? raise ForestException, 'CloseIssue :state cannot be empty.' end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification/form_builder.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification/form_builder.rb index ba0e7280d..8f2ca0c96 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification/form_builder.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/plugins/create_issue_with_notification/form_builder.rb @@ -60,12 +60,14 @@ def message_field(default_message, templates) field = { type: FieldType::STRING, label: 'Message', widget: 'RichText', is_required: true, description: 'The body of the issue (HTML). Unless it is sent as an internal note, this is ' \ 'the message Pylon delivers to the requester.' } - return field.merge(default_value: template_default(default_message, escape_html: true)) if templates.empty? - - # `value:` (not `default_value:`) — drop_default runs once (data - # key sticks after the first render); drop_deferred re-evaluates - # on every fetch, so Template changes re-fire the message proc. - field.merge(value: message_value(templates)) + default = template_default(default_message, escape_html: true) + return field.merge(default_value: default) if templates.empty? + + # Both keys: `default_value:` fills the first render — drop_default + # runs once, the data key sticking after it — where `value:` is + # re-evaluated by drop_deferred on every fetch, which is what a + # Template change re-fires the message proc through. + field.merge(default_value: default, value: message_value(templates, default)) end # No default: Pylon applies its own when the key is absent, and no @@ -107,21 +109,34 @@ def template_default(template, escape_html:) # Returns nil unless Template was just changed, so set_watch_changes # carries over the user's current Message edits between renders. - def message_value(templates) + # + # Taking a template back restores the configured default rather than + # emptying a required field: it is what the operator was handed before + # they picked one. + def message_value(templates, default) by_title = templates.to_h { |t| [t[:title], t[:content].to_s] } lambda do |context| return nil unless context.field_changed?('Template') title = context.get_form_value('Template') - return '' if title == NO_TEMPLATE - - content = by_title[title].to_s - return content unless content.match?(TOKEN_RE) + return evaluate_default(default, context) if title == NO_TEMPLATE - interpolate(content, fetch_record(context), escape_html: true) + interpolated(by_title[title].to_s, context) end end + # A default carrying tokens is a proc, and one without is the string + # itself; no default at all empties the field, as it always did. + def evaluate_default(default, context) + default.is_a?(Proc) ? default.call(context) : default.to_s + end + + def interpolated(content, context) + return content unless content.match?(TOKEN_RE) + + interpolate(content, fetch_record(context), escape_html: true) + end + def fetch_record(context) context.get_record([]) || {} rescue StandardError => e diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/plugins/create_issue_with_notification_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/plugins/create_issue_with_notification_spec.rb index eb3772635..3bd6e15fa 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/plugins/create_issue_with_notification_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/plugins/create_issue_with_notification_spec.rb @@ -355,6 +355,16 @@ def run_action(action, values, context_options = {}) register(email_templates: templates).form.last[:elements].find { |field| field[:label] == 'Message' } end + def with_default(default_message) + register(action_name: "with default #{default_message}", email_templates: templates, + default_message: default_message) + .form.last[:elements].find { |field| field[:label] == 'Message' } + end + + def context_with_record + FakeCreateContext.new(record: { 'name' => 'Ada & Co' }) + end + def template_context(changed:, chosen:, record: {}) context = instance_double(ForestAdminDatasourceCustomizer::Decorators::Action::Context::ActionContextSingle) allow(context).to receive(:field_changed?).with('Template').and_return(changed) @@ -370,10 +380,40 @@ def template_context(changed:, chosen:, record: {}) expect(value).to eq('

Sorry Ada & Co

') end - it 'clears the message when the template is taken back' do + it 'clears the message when the template is taken back and no default was configured' do expect(message_field[:value].call(template_context(changed: true, chosen: 'No template'))).to eq('') end + # The wizard carries the configured default like the flat form does: the + # first page decides whether a template replaces it, not whether the + # operator is handed an empty required field. + it 'still fills the first render from the configured default' do + field = with_default('

Hi

') + + expect(field[:default_value]).to eq('

Hi

') + expect(field[:value]).to be_a(Proc) + end + + it 'interpolates the default of the first render, tokens escaped' do + field = with_default('

Hi {{ record.name }}

') + + expect(field[:default_value].call(context_with_record)).to eq('

Hi Ada & Co

') + end + + it 'restores the configured default when the template is taken back' do + value = with_default('

Hi

')[:value] + .call(template_context(changed: true, chosen: 'No template')) + + expect(value).to eq('

Hi

') + end + + it 'restores a default carrying tokens, interpolated' do + value = with_default('

Hi {{ record.name }}

')[:value] + .call(template_context(changed: true, chosen: 'No template', record: { 'name' => 'Ada & Co' })) + + expect(value).to eq('

Hi Ada & Co

') + end + # Nil means "leave what the operator typed", which is what keeps their # edits across the re-renders of the other fields. it 'leaves the message alone while the template does not change' do