Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new custom_webhook integration (v3) that allows users to define custom YAML-based webhook transformation rules. The integration sends webhook data to an external service that processes it according to user-defined rules and returns structured alert data. This enables flexible webhook handling for third-party services not directly supported by PagerTree.
Key Changes
- Added
CustomWebhook::V3integration model with YAML rule configuration support - Implemented HTTP-based communication with external webhook processing service
- Added comprehensive test suite with VCR cassettes for various action types (create, acknowledge, resolve, ignore)
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
app/models/pager_tree/integrations/custom_webhook/v3.rb |
Core integration model with HTTParty-based external service communication, YAML configuration handling, and alert processing logic |
app/views/pager_tree/integrations/custom_webhook/v3/_form_options.html.erb |
Form view with YAML code editor for custom webhook rule configuration |
app/views/pager_tree/integrations/custom_webhook/v3/_show_options.html.erb |
Empty show view (no display options needed) |
test/models/pager_tree/integrations/custom_webhook/v3_test.rb |
Test suite covering sanity checks, action routing, thirdparty_id extraction, and alert creation |
test/fixtures/pager_tree/integrations/integrations.yml |
Test fixture for custom_webhook_v3 integration |
test/vcr_cassettes/custom_webhook_v3_adapter_*.yml |
VCR cassettes for HTTP interactions with webhook processing service |
test/support/vcr.rb |
VCR configuration to filter sensitive service URL |
lib/pager_tree/integrations.rb |
Global configuration for custom webhook service URL |
config/locales/en.yml |
Localization strings for form labels and hints |
.github/workflows/ci.yml |
CI environment variable for webhook service URL |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
config/locales/en.yml
Outdated
| option_thread_same_alert: "Thread Same Alert" | ||
| "pager_tree/integrations/channel/microsoft_teams/v3": | ||
| option_time_zone: "Display Time Zone" | ||
| "pager_tree/integration/custom_webhook/v3": |
There was a problem hiding this comment.
The path should be "pager_tree/integrations/custom_webhook/v3" (with 's' in "integrations") to match the module namespace and be consistent with all other integration paths in this file (e.g., lines 193-198, 201-203).
| "pager_tree/integration/custom_webhook/v3": | |
| "pager_tree/integrations/custom_webhook/v3": |
|
|
||
| custom_webhook_v3: | ||
| type: "PagerTree::Integrations::CustomWebhook::V3" | ||
| # optionss: no_options |
There was a problem hiding this comment.
There's a typo: optionss: should be options: (single 's'). This should follow the same pattern as other fixtures in this file.
| # optionss: no_options | |
| # options: no_options |
| end | ||
|
|
||
| def _additional_datums | ||
| custom_response_result.dig("additional_data") || [] |
There was a problem hiding this comment.
The _additional_datums method should map the response data to AdditionalDatum objects instead of returning raw hashes. Following the pattern used in other integrations (e.g., Cloudflare::V3), it should be:
def _additional_datums
data = custom_response_result.dig("additional_data") || []
data.map { |datum| AdditionalDatum.new(datum) }
endThis ensures the data is properly validated and conforms to the expected AdditionalDatum interface.
| custom_response_result.dig("additional_data") || [] | |
| data = custom_response_result.dig("additional_data") || [] | |
| data.map { |datum| AdditionalDatum.new(datum) } |
| @integration.adapter_incoming_deferred_request.body = @acknowledge_request | ||
| assert_equal :acknowledge, @integration.adapter_action | ||
| end | ||
| end |
There was a problem hiding this comment.
[nitpick] Missing blank line between test methods. For consistency with the rest of the file (see lines 97-99, 105-107, 127-129), there should be a blank line before the test "adapter_action_resolve" method definition.
| end | |
| end |
| @integration.adapter_incoming_deferred_request.body = @resolve_request | ||
| assert_equal :resolve, @integration.adapter_action | ||
| end | ||
| end |
There was a problem hiding this comment.
[nitpick] Missing blank line between test methods. For consistency with the rest of the file (see lines 97-99, 105-107, 127-129), there should be a blank line before the test "adapter_action_other" method definition.
| end | |
| end |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 30 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Rails.logger.error("Custom Webhook Service error: #{e.message}") | ||
| adapter_source_log&.sublog("Custom Webhook Service error: #{e.message}") | ||
| adapter_source_log&.save | ||
| raise "Custom Webhook Service error #{e.message}" |
There was a problem hiding this comment.
Missing colon after "error" in the error message string. Should be "Custom Webhook Service error: #{e.message}" for consistency with other error messages in this file.
| raise "Custom Webhook Service error #{e.message}" | |
| raise "Custom Webhook Service error: #{e.message}" |
|
|
||
| body_hash = { | ||
| log: log_hash, | ||
| config: JSON.parse(PagerTree::Integrations::FormatConverters::YamlJsonConverter.convert_to_json(option_custom_definition)) |
There was a problem hiding this comment.
Inefficient double-conversion. The code converts YAML to JSON string using convert_to_json, then immediately parses it back with JSON.parse. This creates unnecessary overhead. Consider having YamlJsonConverter provide a method that returns the parsed data structure directly, or use convert_to_json(option_custom_definition, pretty: false) and skip the JSON.parse to directly use the JSON string if that's the desired format.
| # lib/pager_tree/integrations/format_converters/yaml_json_converter.rb | ||
| require "yaml" | ||
| require "json" | ||
|
|
||
| module PagerTree | ||
| module Integrations | ||
| module FormatConverters | ||
| class YamlJsonConverter | ||
| class InvalidFormatError < StandardError; end | ||
|
|
||
| class << self | ||
| def convert_to_json(str, pretty: true) | ||
| format = detect_format(str) | ||
|
|
||
| case format | ||
| when :json | ||
| data = JSON.parse(str) | ||
| pretty ? JSON.pretty_generate(data) : data.to_json | ||
| when :yaml | ||
| data = YAML.safe_load(str) | ||
| pretty ? JSON.pretty_generate(data) : data.to_json | ||
| else | ||
| raise InvalidFormatError, "Input is neither valid JSON nor YAML" | ||
| end | ||
| end | ||
|
|
||
| def convert_to_yaml(str) | ||
| format = detect_format(str) | ||
|
|
||
| case format | ||
| when :json | ||
| data = JSON.parse(str) | ||
| data.to_yaml | ||
| when :yaml | ||
| data = YAML.safe_load(str) | ||
| data.to_yaml | ||
| else | ||
| raise InvalidFormatError, "Input is neither valid JSON nor YAML" | ||
| end | ||
| end | ||
|
|
||
| def detect_format(str) | ||
| return :json if json?(str) | ||
| return :yaml if yaml?(str) | ||
| :unknown | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def json?(str) | ||
| JSON.parse(str) | ||
| true | ||
| rescue JSON::ParserError | ||
| false | ||
| end | ||
|
|
||
| def yaml?(str) | ||
| YAML.safe_load(str) | ||
| true | ||
| rescue Psych::SyntaxError | ||
| false | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
The YamlJsonConverter utility class lacks test coverage. This new utility should have comprehensive tests covering format detection, conversion between YAML and JSON, error handling for invalid inputs, and edge cases. Consider adding a test file at test/lib/pager_tree/integrations/format_converters/yaml_json_converter_test.rb.
|
|
||
| custom_webhook_v3: | ||
| type: "PagerTree::Integrations::CustomWebhook::V3" | ||
| # optionss: no_options |
There was a problem hiding this comment.
Typo in YAML comment - "optionss" should be "options" (extra 's').
| # optionss: no_options | |
| # options: no_options |
| data = YAML.safe_load(str) | ||
| pretty ? JSON.pretty_generate(data) : data.to_json | ||
| else | ||
| raise InvalidFormatError, "Input is neither valid JSON nor YAML" | ||
| end | ||
| end | ||
|
|
||
| def convert_to_yaml(str) | ||
| format = detect_format(str) | ||
|
|
||
| case format | ||
| when :json | ||
| data = JSON.parse(str) | ||
| data.to_yaml | ||
| when :yaml | ||
| data = YAML.safe_load(str) | ||
| data.to_yaml | ||
| else | ||
| raise InvalidFormatError, "Input is neither valid JSON nor YAML" | ||
| end | ||
| end | ||
|
|
||
| def detect_format(str) | ||
| return :json if json?(str) | ||
| return :yaml if yaml?(str) | ||
| :unknown | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def json?(str) | ||
| JSON.parse(str) | ||
| true | ||
| rescue JSON::ParserError | ||
| false | ||
| end | ||
|
|
||
| def yaml?(str) | ||
| YAML.safe_load(str) |
There was a problem hiding this comment.
Using YAML.safe_load without specifying permitted classes could be a security risk if the YAML contains complex objects. Consider using YAML.safe_load(str, permitted_classes: [], permitted_symbols: [], aliases: false) to be more explicit about what's allowed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Rails.logger.error("Custom Webhook Service error: #{e.message}") | ||
| adapter_source_log&.sublog("Custom Webhook Service error: #{e.message}") | ||
| adapter_source_log&.save | ||
| raise "Custom Webhook Service error #{e.message}" |
There was a problem hiding this comment.
Inconsistent error message format. The error message at line 124 is missing a colon after "error" (should be "Custom Webhook Service error: #{e.message}"). For consistency with the error message at line 105 and standard error message formatting, add a colon between "error" and the interpolated message.
| raise "Custom Webhook Service error #{e.message}" | |
| raise "Custom Webhook Service error: #{e.message}" |
| # lib/pager_tree/integrations/format_converters/yaml_json_converter.rb | ||
| require "yaml" | ||
| require "json" | ||
|
|
||
| module PagerTree | ||
| module Integrations | ||
| module FormatConverters | ||
| class YamlJsonConverter | ||
| class InvalidFormatError < StandardError; end | ||
|
|
||
| class << self | ||
| def convert_to_json(str, pretty: true) | ||
| format = detect_format(str) | ||
|
|
||
| case format | ||
| when :json | ||
| data = JSON.parse(str) | ||
| pretty ? JSON.pretty_generate(data) : data.to_json | ||
| when :yaml | ||
| data = YAML.safe_load(str) | ||
| pretty ? JSON.pretty_generate(data) : data.to_json | ||
| else | ||
| raise InvalidFormatError, "Input is neither valid JSON nor YAML" | ||
| end | ||
| end | ||
|
|
||
| def convert_to_yaml(str) | ||
| format = detect_format(str) | ||
|
|
||
| case format | ||
| when :json | ||
| data = JSON.parse(str) | ||
| data.to_yaml | ||
| when :yaml | ||
| data = YAML.safe_load(str) | ||
| data.to_yaml | ||
| else | ||
| raise InvalidFormatError, "Input is neither valid JSON nor YAML" | ||
| end | ||
| end | ||
|
|
||
| def detect_format(str) | ||
| return :json if json?(str) | ||
| return :yaml if yaml?(str) | ||
| :unknown | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def json?(str) | ||
| JSON.parse(str) | ||
| true | ||
| rescue JSON::ParserError | ||
| false | ||
| end | ||
|
|
||
| def yaml?(str) | ||
| YAML.safe_load(str) | ||
| true | ||
| rescue Psych::SyntaxError | ||
| false | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
The new YamlJsonConverter utility class lacks test coverage. This class handles conversion of YAML/JSON input that could come from users, and includes error handling for invalid formats. Add unit tests to verify:
- Successful conversion from YAML to JSON (pretty and compact)
- Successful conversion from JSON to YAML
- Error handling for invalid YAML input
- Error handling for invalid JSON input
- Format detection logic
- Edge cases (empty strings, malformed input, etc.)
This is especially important since the converter handles user-provided configuration data.
No description provided.