Ben 405#7
Conversation
Enhance Bento ActionMailer delivery method with robust error handling, detailed error messages, and better response parsing. Add support for parsing JSON error responses, handling authorization errors, and providing more context for delivery failures.
…eryMethod Implement thorough test coverage for the BentoActionMailer Railtie and DeliveryMethod initialization. The changes include: - Create test suite for Railtie to validate initializer behavior - Add tests for delivery method registration - Implement tests for DeliveryMethod settings initialization - Ensure idempotent behavior of initializers - Provide comprehensive test scenarios for configuration options
Refactor test files to use consistent double quotes and improve code style. Remove unnecessary whitespace and ensure uniform string representation across test cases.
Modify RuboCop configuration to relax strict rules and increase development productivity. Disable several style checks and adjust metrics thresholds to better suit project requirements. Increase line length limit and exclude test files from certain metrics.
Remove GitHub Actions workflow for PR tests and add IDE-specific files to gitignore to prevent accidental tracking of local development environment files
BEN-390 Description Improved error handling in Bento ActionMailer delivery method Enhanced response processing with detailed error messages Added support for parsing JSON error responses Implemented comprehensive test coverage for BentoActionMailer Railtie and DeliveryMethod
Ignore benchmark-related files and directories to prevent unintended tracking of performance testing artifacts in version control
feat: Add bench directory to gitignore
Refactor delivery method to use a pipeline architecture for processing email messages. Introduce modular actions for extracting message details, improving separation of concerns and making the delivery process more flexible and extensible. Key changes: - Add Pipeline and DeliveryPipelineFactory classes - Create individual action classes for message processing - Improve message extraction with MessageExtractor - Support optional text body in email delivery - Simplify delivery method implementation
Enhance email delivery by automatically generating an HTML body when only a text body is present. This change: - Adds support for generating sanitized HTML from plain text - Preserves paragraph and line break formatting - Wraps generated HTML in a "bento-text-only" div - Maintains existing behavior for HTML-only emails
📝 WalkthroughWalkthroughRefactors email delivery using a pipeline architecture with discrete action steps. Adds Rails 7+ CSS inlining via premailer-rails, improves error handling with detailed response codes, updates configuration and documentation for Rails 4.2+ compatibility, and adds comprehensive test coverage. Changes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (10)
.github/workflows/manual-release.yml (2)
22-29: Remove redundantgit fetch --tagsstep.Line 26 already sets
fetch-tags: trueon the checkout action, which fetches all tags automatically. Line 29's explicitgit fetch --tagsis redundant.- - name: Fetch tags - run: git fetch --tags - - name: Prepare tag name
33-40: Consider adding version format validation.The workflow accepts any string for version and only checks for a 'v' prefix. Adding a regex check for semantic versioning format (e.g.,
\d+\.\d+\.\d+) would catch accidental invalid inputs early.- name: Prepare tag name id: prep run: | VERSION="${{ inputs.version }}" + if ! [[ "$VERSION" =~ ^v?[0-9]+\.[0-9]+\.[0-9]+(-[a-zA-Z0-9]+)?$ ]]; then + echo "Invalid version format: $VERSION" >&2 + exit 1 + fi if [[ "$VERSION" == v* ]]; then TAG="$VERSION" else TAG="v$VERSION" fi echo "tag=$TAG" >> "$GITHUB_OUTPUT"lib/bento_actionmailer/support/rails_version.rb (1)
8-30: Consider the module_function and private_class_method interaction.
module_function(line 8) makes methods callable as module methods, whileprivate_class_method :fetch_version(line 30) restricts it. This works but is unconventional—typically you'd either useprivatefor instance context or define singleton methods explicitly.Alternative approach:
- module_function - def current return unless defined?(::Rails) version = fetch_version Gem::Version.new(version) if version rescue ArgumentError nil end + module_function :current def rails_7_or_higher? version = current version && version >= MINIMUM_PREMAILER_VERSION end + module_function :rails_7_or_higher? def fetch_version return ::Rails.gem_version.to_s if ::Rails.respond_to?(:gem_version) return ::Rails.version.to_s if ::Rails.respond_to?(:version) nil end - private_class_method :fetch_versionREADME.md (1)
155-155: Optional: Consider hyphenating "dual-format" as an adjective.LanguageTool suggests "dual-format delivery" for consistency with compound modifiers. This is a style preference.
-2. **Dual format delivery** forwards both HTML and plain text bodies when your mailer renders them. +2. **Dual-format delivery** forwards both HTML and plain text bodies when your mailer renders them.test/bento/test_helper_extensions.rb (1)
88-108: Make Rails constant stubbing thread-safe.Directly removing/setting Rails is racy in parallel tests. Guard with a Mutex.
@@ -require "ostruct" +require "ostruct" +require "thread" @@ module TestHelperExtensions DEFAULT_SETTINGS = { @@ }.freeze + + RAILS_CONST_MUTEX = Mutex.new @@ - def with_stubbed_rails(version: nil) - original_defined = Object.const_defined?(:Rails) - original_rails = Object.const_get(:Rails) if original_defined - Object.send(:remove_const, :Rails) if original_defined + def with_stubbed_rails(version: nil) + original_defined = nil + original_rails = nil + RAILS_CONST_MUTEX.synchronize do + original_defined = Object.const_defined?(:Rails) + original_rails = Object.const_get(:Rails) if original_defined + Object.send(:remove_const, :Rails) if original_defined + end @@ - Object.const_set(:Rails, stubbed) - yield + RAILS_CONST_MUTEX.synchronize { Object.const_set(:Rails, stubbed) } + yield ensure - Object.send(:remove_const, :Rails) if Object.const_defined?(:Rails) - Object.const_set(:Rails, original_rails) if original_defined + RAILS_CONST_MUTEX.synchronize do + Object.send(:remove_const, :Rails) if Object.const_defined?(:Rails) + Object.const_set(:Rails, original_rails) if original_defined + end end @@ - def without_rails - original_defined = Object.const_defined?(:Rails) - original_rails = Object.const_get(:Rails) if original_defined - Object.send(:remove_const, :Rails) if original_defined + def without_rails + original_defined = nil + original_rails = nil + RAILS_CONST_MUTEX.synchronize do + original_defined = Object.const_defined?(:Rails) + original_rails = Object.const_get(:Rails) if original_defined + Object.send(:remove_const, :Rails) if original_defined + end @@ ensure - Object.const_set(:Rails, original_rails) if original_defined + RAILS_CONST_MUTEX.synchronize { Object.const_set(:Rails, original_rails) if original_defined } endAlso applies to: 110-118
test/fixtures/sample_mail.rb (1)
52-57: Large body fixture OK.Useful for stress tests; keep default size modest to avoid slow CI if needed.
lib/bento_actionmailer.rb (4)
72-85: Treat blank bodies as absent.Empty strings currently generate empty HTML, bypassing the “No HTML body” error. Check presence.
- html_body = extractor.html_body - return inline_html(html_body) if html_body + html_body = extractor.html_body + return inline_html(html_body) if html_body && !html_body.to_s.strip.empty? @@ - text_body = extractor.text_body - if text_body + text_body = extractor.text_body + if text_body && !text_body.to_s.strip.empty? generated_html = build_html_from_text(text_body) return inline_html(generated_html) end
151-153: Add timeouts and enforce TLS settings.Prevent hangs and ensure secure defaults.
- response = Net::HTTP.start(BENTO_ENDPOINT.hostname, BENTO_ENDPOINT.port, req_options) do |http| - http.request(request) - end + response = Net::HTTP.start(BENTO_ENDPOINT.hostname, BENTO_ENDPOINT.port, req_options) do |http| + http.open_timeout = 5 unless http.open_timeout && http.open_timeout > 0 + http.read_timeout = 10 unless http.read_timeout && http.read_timeout > 0 + http.write_timeout = 10 if http.respond_to?(:write_timeout) && (!http.write_timeout || http.write_timeout <= 0) + if http.use_ssl? + http.verify_mode = OpenSSL::SSL::VERIFY_PEER + http.min_version = OpenSSL::SSL::TLS1_2_VERSION if defined?(OpenSSL::SSL::TLS1_2_VERSION) + end + http.request(request) + end
112-120: Duplicate pipeline configuration.
delivery_actionsduplicatesDeliveryPipelineFactory::ACTION_CLASSES. Single source of truth to avoid drift.
47-55: Consider exposing personalization viadeliver!.Right now it’s always
{}; optional param keeps public API aligned with pipeline.- def deliver!(mail) + def deliver!(mail, personalization: {}) @@ - personalization: {} + personalization: personalization
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.github/workflows/manual-release.yml(1 hunks).gitignore(1 hunks).rubocop.yml(2 hunks)AGENTS.md(1 hunks)README.md(5 hunks)bento-actionmailer.gemspec(1 hunks)lib/bento_actionmailer.rb(1 hunks)lib/bento_actionmailer/actions.rb(1 hunks)lib/bento_actionmailer/actions/dispatch_email.rb(1 hunks)lib/bento_actionmailer/actions/ensure_mail.rb(1 hunks)lib/bento_actionmailer/actions/extract_addresses.rb(1 hunks)lib/bento_actionmailer/actions/extract_bodies.rb(1 hunks)lib/bento_actionmailer/actions/extract_subject.rb(1 hunks)lib/bento_actionmailer/delivery_pipeline_factory.rb(1 hunks)lib/bento_actionmailer/message_extractor.rb(1 hunks)lib/bento_actionmailer/pipeline.rb(1 hunks)lib/bento_actionmailer/premailer_inliner.rb(1 hunks)lib/bento_actionmailer/premailer_support.rb(1 hunks)lib/bento_actionmailer/support/rails_version.rb(1 hunks)sig/bento/actionmailer.rbs(1 hunks)test/bento/test_actionmailer.rb(1 hunks)test/bento/test_delivery_method.rb(1 hunks)test/bento/test_helper_extensions.rb(1 hunks)test/bento/test_rails_version_support.rb(1 hunks)test/bento/test_railtie.rb(1 hunks)test/fixtures/api_responses.json(1 hunks)test/fixtures/sample_mail.rb(1 hunks)test/test_helper.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (16)
lib/bento_actionmailer/actions/dispatch_email.rb (5)
lib/bento_actionmailer/actions/ensure_mail.rb (3)
initialize(5-18)initialize(6-8)call(10-13)lib/bento_actionmailer/actions/extract_addresses.rb (3)
initialize(5-20)initialize(6-8)call(10-15)lib/bento_actionmailer/actions/extract_bodies.rb (3)
initialize(5-20)initialize(6-8)call(10-15)lib/bento_actionmailer/actions/extract_subject.rb (3)
initialize(5-19)initialize(6-8)call(10-14)lib/bento_actionmailer/pipeline.rb (3)
initialize(4-20)initialize(5-7)call(9-15)
lib/bento_actionmailer/actions/extract_addresses.rb (5)
lib/bento_actionmailer/actions/dispatch_email.rb (3)
initialize(5-30)initialize(6-8)call(10-25)lib/bento_actionmailer/actions/ensure_mail.rb (3)
initialize(5-18)initialize(6-8)call(10-13)lib/bento_actionmailer/actions/extract_bodies.rb (3)
initialize(5-20)initialize(6-8)call(10-15)lib/bento_actionmailer/actions/extract_subject.rb (3)
initialize(5-19)initialize(6-8)call(10-14)lib/bento_actionmailer/pipeline.rb (3)
initialize(4-20)initialize(5-7)call(9-15)
lib/bento_actionmailer/message_extractor.rb (2)
lib/bento_actionmailer/actions/dispatch_email.rb (2)
initialize(5-30)initialize(6-8)lib/bento_actionmailer/actions/extract_bodies.rb (2)
initialize(5-20)initialize(6-8)
test/bento/test_rails_version_support.rb (2)
test/bento/test_helper_extensions.rb (2)
without_rails(110-118)with_stubbed_rails(88-108)lib/bento_actionmailer/support/rails_version.rb (2)
current(10-17)rails_7_or_higher?(19-22)
lib/bento_actionmailer/actions/extract_subject.rb (4)
lib/bento_actionmailer/actions/ensure_mail.rb (3)
initialize(5-18)initialize(6-8)call(10-13)lib/bento_actionmailer/actions/extract_addresses.rb (3)
initialize(5-20)initialize(6-8)call(10-15)lib/bento_actionmailer/actions/extract_bodies.rb (3)
initialize(5-20)initialize(6-8)call(10-15)lib/bento_actionmailer/pipeline.rb (3)
initialize(4-20)initialize(5-7)call(9-15)
lib/bento_actionmailer/delivery_pipeline_factory.rb (6)
lib/bento_actionmailer/actions/dispatch_email.rb (2)
initialize(5-30)initialize(6-8)lib/bento_actionmailer/actions/ensure_mail.rb (2)
initialize(5-18)initialize(6-8)lib/bento_actionmailer/actions/extract_addresses.rb (2)
initialize(5-20)initialize(6-8)lib/bento_actionmailer/actions/extract_bodies.rb (2)
initialize(5-20)initialize(6-8)lib/bento_actionmailer/actions/extract_subject.rb (2)
initialize(5-19)initialize(6-8)lib/bento_actionmailer/pipeline.rb (2)
initialize(4-20)initialize(5-7)
lib/bento_actionmailer/support/rails_version.rb (1)
lib/bento_actionmailer/premailer_support.rb (1)
rails_7_or_higher?(5-7)
lib/bento_actionmailer/premailer_inliner.rb (1)
test/test_helper.rb (1)
warn(6-17)
lib/bento_actionmailer/premailer_support.rb (2)
lib/bento_actionmailer/support/rails_version.rb (1)
rails_7_or_higher?(19-22)lib/bento_actionmailer.rb (1)
build_delivery_error(207-209)
lib/bento_actionmailer/pipeline.rb (6)
lib/bento_actionmailer/actions/dispatch_email.rb (3)
initialize(5-30)initialize(6-8)call(10-25)lib/bento_actionmailer/actions/ensure_mail.rb (3)
initialize(5-18)initialize(6-8)call(10-13)lib/bento_actionmailer/actions/extract_addresses.rb (3)
initialize(5-20)initialize(6-8)call(10-15)lib/bento_actionmailer/actions/extract_bodies.rb (3)
initialize(5-20)initialize(6-8)call(10-15)lib/bento_actionmailer/actions/extract_subject.rb (3)
initialize(5-19)initialize(6-8)call(10-14)lib/bento_actionmailer/delivery_pipeline_factory.rb (1)
initialize(13-15)
test/test_helper.rb (1)
lib/bento_actionmailer.rb (1)
include(20-226)
test/bento/test_helper_extensions.rb (2)
lib/bento_actionmailer/message_extractor.rb (3)
freeze(4-62)html_body(12-14)text_body(16-18)lib/bento_actionmailer/premailer_inliner.rb (1)
freeze(4-38)
lib/bento_actionmailer/actions/extract_bodies.rb (6)
lib/bento_actionmailer/actions/dispatch_email.rb (3)
initialize(5-30)initialize(6-8)call(10-25)lib/bento_actionmailer/actions/ensure_mail.rb (3)
initialize(5-18)initialize(6-8)call(10-13)lib/bento_actionmailer/actions/extract_addresses.rb (3)
initialize(5-20)initialize(6-8)call(10-15)lib/bento_actionmailer/actions/extract_subject.rb (3)
initialize(5-19)initialize(6-8)call(10-14)lib/bento_actionmailer/delivery_pipeline_factory.rb (1)
initialize(13-15)lib/bento_actionmailer/pipeline.rb (3)
initialize(4-20)initialize(5-7)call(9-15)
test/bento/test_delivery_method.rb (10)
test/bento/test_actionmailer.rb (1)
new(5-166)test/bento/test_railtie.rb (3)
setup(5-127)setup(8-11)teardown(13-18)test/bento/test_helper_extensions.rb (9)
build_delivery_method(19-21)reset_premailer_inliner(120-123)build_mail_message(32-59)build_text_only_mail(61-63)build_mail_stub(84-86)build_mail_with_custom_parts(65-73)fixture_mail(80-82)with_stubbed_rails(88-108)build_response(23-25)lib/bento_actionmailer.rb (1)
deliver!(47-55)lib/bento_actionmailer/actions/dispatch_email.rb (1)
call(10-25)lib/bento_actionmailer/actions/ensure_mail.rb (1)
call(10-13)lib/bento_actionmailer/actions/extract_addresses.rb (1)
call(10-15)lib/bento_actionmailer/actions/extract_bodies.rb (1)
call(10-15)lib/bento_actionmailer/actions/extract_subject.rb (1)
call(10-14)lib/bento_actionmailer/pipeline.rb (1)
call(9-15)
lib/bento_actionmailer.rb (12)
lib/bento_actionmailer/actions/dispatch_email.rb (3)
initialize(5-30)initialize(6-8)call(10-25)lib/bento_actionmailer/actions/ensure_mail.rb (3)
initialize(5-18)initialize(6-8)call(10-13)lib/bento_actionmailer/actions/extract_addresses.rb (3)
initialize(5-20)initialize(6-8)call(10-15)lib/bento_actionmailer/actions/extract_bodies.rb (3)
initialize(5-20)initialize(6-8)call(10-15)lib/bento_actionmailer/actions/extract_subject.rb (3)
initialize(5-19)initialize(6-8)call(10-14)lib/bento_actionmailer/delivery_pipeline_factory.rb (3)
initialize(13-15)freeze(4-24)build(17-19)lib/bento_actionmailer/message_extractor.rb (4)
initialize(8-10)freeze(4-62)html_body(12-14)text_body(16-18)lib/bento_actionmailer/pipeline.rb (3)
initialize(4-20)initialize(5-7)call(9-15)lib/bento_actionmailer/premailer_inliner.rb (3)
initialize(13-16)freeze(4-38)inline(18-24)test/bento/test_actionmailer.rb (1)
new(5-166)lib/bento_actionmailer/premailer_support.rb (2)
rails_7_or_higher?(5-7)premailer_inliner(23-28)lib/bento_actionmailer/support/rails_version.rb (1)
rails_7_or_higher?(19-22)
lib/bento_actionmailer/actions/ensure_mail.rb (5)
lib/bento_actionmailer/actions/dispatch_email.rb (3)
initialize(5-30)initialize(6-8)call(10-25)lib/bento_actionmailer/actions/extract_addresses.rb (3)
initialize(5-20)initialize(6-8)call(10-15)lib/bento_actionmailer/actions/extract_bodies.rb (3)
initialize(5-20)initialize(6-8)call(10-15)lib/bento_actionmailer/actions/extract_subject.rb (3)
initialize(5-19)initialize(6-8)call(10-14)lib/bento_actionmailer/pipeline.rb (3)
initialize(4-20)initialize(5-7)call(9-15)
🪛 LanguageTool
README.md
[grammar] ~155-~155: Use a hyphen to join words.
Context: ...l HTML is delivered unchanged. 2. Dual format delivery forwards both HTML and...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
README.md
25-25: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (40)
.github/workflows/manual-release.yml (1)
42-67: Workflow structure and error handling look solid.Tag existence validation, git user config, push logic, and release creation all follow best practices. Proper use of job outputs and environment variables.
.gitignore (1)
10-12: LGTM!Standard ignore patterns for JetBrains IDEs, macOS metadata, and benchmark artifacts.
sig/bento/actionmailer.rbs (1)
1-28: LGTM!Type signatures accurately reflect the delivery method API. The dual namespace (Bento::Actionmailer and BentoActionMailer) appears intentional for compatibility.
.rubocop.yml (2)
1-69: Configuration updates look reasonable.Increased line length to 140, disabled several style cops, and added metrics limits with test exclusions. This provides flexibility for the new pipeline architecture.
56-58: No issues found.The hyphenated file (
lib/bento-actionmailer.rb) is a require shim that loads the underscore-named main library. The Rubocop exclusion is correct and intentional.Likely an incorrect or invalid review comment.
test/test_helper.rb (3)
3-19: LGTM!Warning suppression prevents noise from Premailer's unused variables and character class warnings.
27-36: Dual suppression approach is intentional.Combines
Warning.warnoverride (all Rubies) withWarning.ignore(Ruby 2.7+) for maximum compatibility. The pattern duplication is expected.
38-40: LGTM!Extending Minitest::Test with shared helpers makes test utilities available across the suite.
lib/bento_actionmailer/message_extractor.rb (5)
8-18: LGTM!Clean public API with separate html_body and text_body methods delegating to a shared extraction strategy.
24-27: LGTM!Two-stage fallback (explicit parts, then content-type matching) handles multipart and simple messages.
29-35: LGTM!Pattern matching with respond_to? guards ensures compatibility across mail gem versions.
37-44: LGTM!Fallback strategy correctly checks message-level and body-level content types for single-part messages.
46-61: LGTM!Helper methods use duck-typing and multiple fallback strategies to handle varied message structures robustly.
bento-actionmailer.gemspec (2)
35-35: LGTM!OStruct as a development dependency is appropriate for test fixtures. No version constraint needed since it's stdlib.
33-33: Consider upgrading to~> 1.12; if intentionally locked to 1.11, document why.No vulnerabilities found. Version 1.12.0 exists (stable, Nov 2022) but has breaking changes: lazy ActionMailer loading and Accept headers on remote CSS requests. If 1.11 was chosen to avoid these, confirm compatibility testing is done. If 1.11 was a conservative default, test against 1.12 and upgrade if compatible.
lib/bento_actionmailer/actions.rb (1)
1-7: LGTM!Simple require aggregator for pipeline actions. Clean module organization.
AGENTS.md (1)
25-25: The test file already has behavior-driven assertions—no placeholders remain.The file contains 11 fully implemented test methods with proper assertions (assert_raises, assert_equal, assert_nil), covering success cases, auth errors, client errors, server errors, and edge cases. The suggested action has already been completed.
Likely an incorrect or invalid review comment.
test/fixtures/api_responses.json (1)
1-23: LGTM!Well-structured test fixture covering success, error, and malformed response scenarios.
lib/bento_actionmailer/actions/ensure_mail.rb (1)
5-18: LGTM!Action follows the pipeline pattern correctly. Private method invocation via
sendis consistent with other actions in the pipeline.lib/bento_actionmailer/delivery_pipeline_factory.rb (2)
5-11: LGTM!Action pipeline order is logical: ensure mail → extract addresses → extract subject → extract bodies → dispatch.
13-19: LGTM!Factory correctly instantiates each action with the delivery method and builds a cohesive pipeline.
lib/bento_actionmailer/actions/extract_subject.rb (1)
5-19: LGTM!Extraction logic is consistent with other pipeline actions.
test/bento/test_rails_version_support.rb (3)
6-11: LGTM!Test correctly validates behavior when Rails is undefined.
13-18: LGTM!Test correctly validates Rails 7+ version detection.
20-25: LGTM!Test correctly validates version below Rails 7 threshold.
lib/bento_actionmailer/actions/extract_addresses.rb (1)
5-20: LGTM!Address extraction follows the established pipeline pattern and handles both to/from addresses cleanly.
lib/bento_actionmailer/pipeline.rb (2)
5-7: LGTM!
Array()coercion ensures consistent handling of single or multiple actions.
9-15: LGTM!Elegant middleware chain implementation using functional composition.
lib/bento_actionmailer/actions/dispatch_email.rb (2)
10-17: LGTM!Payload correctly distinguishes required fields (fetch) from optional text_body.
19-24: LGTM!Email dispatch correctly stores result and continues pipeline execution for potential post-send actions.
lib/bento_actionmailer/actions/extract_bodies.rb (1)
1-22: LGTM!The action follows the established pipeline pattern consistently. Extraction delegates appropriately to the delivery method, and context propagation is correct.
test/bento/test_railtie.rb (1)
1-127: LGTM!Comprehensive test coverage for Railtie integration. Constant stubbing/restoration is properly handled, and the test scenarios validate initialization, registration, and idempotence effectively.
test/bento/test_actionmailer.rb (1)
1-166: LGTM!Thorough test coverage for error handling across HTTP status ranges. Response code, error messages, and error_details are properly validated for authorization, client, server, and unexpected errors.
test/bento/test_delivery_method.rb (1)
1-582: LGTM!Extensive test coverage across initialization, delivery flow, Premailer integration, network operations, and utility methods. Tests validate edge cases, error handling, and Rails version gating comprehensively.
lib/bento_actionmailer/premailer_inliner.rb (1)
1-39: LGTM!Solid implementation with defensive error handling. The loader callback pattern is clean, and fallback to original HTML on failure prevents delivery disruption.
README.md (1)
1-169: Documentation is clear and comprehensive.The updated narrative effectively explains Rails 7.0+ features and backward compatibility. Examples are helpful.
lib/bento_actionmailer/premailer_support.rb (1)
15-20: Original review comment is incorrect.PremailerSupport (lib/bento_actionmailer/premailer_support.rb:4) is included in DeliveryMethod (lib/bento_actionmailer.rb:21), making
build_delivery_error(defined at lib/bento_actionmailer.rb:207) accessible through standard Ruby mixin semantics. The code is valid.Likely an incorrect or invalid review comment.
test/bento/test_helper_extensions.rb (2)
19-21: Helper API looks good.Clean factory for DeliveryMethod. No issues.
32-59: Mail builder is solid.Parts set with proper content types/charset. Good coverage for text/html.
lib/bento_actionmailer.rb (1)
211-219: Text→HTML conversion looks good.Simple, safe, and preserves line breaks. LGTM.
| - `bin/console` loads the gem in IRB for manual delivery experiments against stubbed endpoints. | ||
|
|
||
| ## Coding Style & Naming Conventions | ||
| - Follow RuboCop: Ruby 2.6 target, double-quoted strings, and 120-character lines. |
There was a problem hiding this comment.
Line length inconsistency with .rubocop.yml.
This line states 120 characters, but .rubocop.yml Line 7 sets Layout/LineLength: Max: 140.
Apply this diff:
-- Follow RuboCop: Ruby 2.6 target, double-quoted strings, and 120-character lines.
+- Follow RuboCop: Ruby 2.6 target, double-quoted strings, and 140-character lines.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Follow RuboCop: Ruby 2.6 target, double-quoted strings, and 120-character lines. | |
| - Follow RuboCop: Ruby 2.6 target, double-quoted strings, and 140-character lines. |
🤖 Prompt for AI Agents
In AGENTS.md around line 17, the documented max line length says 120 characters
but .rubocop.yml sets Layout/LineLength: Max: 140; update this line in AGENTS.md
to state 140-character lines (or otherwise make the two files consistent) so the
documentation matches the .rubocop.yml setting.
| def extract_subject(subject) | ||
| value = subject.to_s.strip | ||
| raise DeliveryError, "Mail subject is required" if value.empty? | ||
|
|
||
| subject.to_s | ||
| end |
There was a problem hiding this comment.
Subject returned untrimmed.
You validate trimmed value but return the untrimmed string. Return the sanitized value.
- subject.to_s
+ value📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def extract_subject(subject) | |
| value = subject.to_s.strip | |
| raise DeliveryError, "Mail subject is required" if value.empty? | |
| subject.to_s | |
| end | |
| def extract_subject(subject) | |
| value = subject.to_s.strip | |
| raise DeliveryError, "Mail subject is required" if value.empty? | |
| value | |
| end |
🤖 Prompt for AI Agents
In lib/bento_actionmailer.rb around lines 97 to 102, the method validates a
trimmed subject but returns the original untrimmed string; change the return to
use the trimmed value variable (value) so the sanitized subject is returned
(keep the empty check and DeliveryError as-is).
| transactional: settings[:transactional], | ||
| personalizations: personalization |
There was a problem hiding this comment.
transactional may ignore string‑keyed config.
settings[:transactional] misses "transactional" keys (common when loaded from YAML). Use symbol or string fallback.
- transactional: settings[:transactional],
+ transactional: (settings[:transactional].nil? ? settings["transactional"] : settings[:transactional]),Alternatively, add a setting(key) helper and use it everywhere.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| transactional: settings[:transactional], | |
| personalizations: personalization | |
| transactional: (settings[:transactional].nil? ? settings["transactional"] : settings[:transactional]), | |
| personalizations: personalization |
🤖 Prompt for AI Agents
In lib/bento_actionmailer.rb around lines 140 to 141, the code uses
settings[:transactional] which will be nil when settings keys are strings (e.g.,
loaded from YAML); update the access to support both symbol and string keys (for
example, use settings[:transactional] || settings["transactional"] or
settings.fetch(:transactional) { settings["transactional"] }) or introduce a
small helper like setting(key) that returns settings[key.to_sym] ||
settings[key.to_s] and use that helper here and everywhere settings are read.
| error_data = parse_error_response(response) | ||
| error_message = error_data&.dig("error") || response.message || UNKNOWN_RESPONSE_MESSAGE | ||
|
|
There was a problem hiding this comment.
Non‑Hash JSON bodies can crash error handling.
If the API returns JSON that isn’t a Hash (e.g., "null", "[]"), error_data&.dig("error") can raise. Ensure parsed JSON is a Hash.
@@
- error_data = parse_error_response(response)
- error_message = error_data&.dig("error") || response.message || UNKNOWN_RESPONSE_MESSAGE
+ error_data = parse_error_response(response)
+ error_message = (error_data.is_a?(Hash) ? error_data["error"] : nil) || response.message || UNKNOWN_RESPONSE_MESSAGE
@@
- def parse_error_response(response)
+ def parse_error_response(response)
body = response.body
return nil if body.nil?
@@
- JSON.parse(body)
+ parsed = JSON.parse(body)
+ parsed.is_a?(Hash) ? parsed : nil
rescue JSON::ParserError
nil
endAlso applies to: 179-189
🤖 Prompt for AI Agents
In lib/bento_actionmailer.rb around lines 164-166 (and similarly 179-189),
parsed JSON from parse_error_response may be non-Hash (e.g., null or array) and
error_data&.dig("error") can raise; change the checks to guard the type before
digging — e.g., only call dig or [] when error_data.is_a?(Hash), otherwise treat
as nil/unknown; apply the same type-guarded access to the other block (179-189)
so non-Hash JSON won't crash the error handling.
| def special_character_mail | ||
| multipart_html_mail( | ||
| to: "büyer+test@example.com", | ||
| from: "✨ sender@example.com", | ||
| subject: "Unicode ✓" | ||
| ) | ||
| end |
There was a problem hiding this comment.
From address may be invalid.
"✨ sender@example.com" isn’t a valid RFC822 address. If you intend a display name, use: "✨ sender sender@example.com".
- from: "✨ sender@example.com",
+ from: "\"✨ sender\" <sender@example.com>",🤖 Prompt for AI Agents
In test/fixtures/sample_mail.rb around lines 44 to 50 the from address "✨
sender@example.com" is not a valid RFC822 address; change it to a valid format
by either removing the emoji from the local-part (e.g. "sender@example.com") or
use a display-name format with the emoji as the display name and a proper
mailbox, e.g. "✨ sender <sender@example.com>", and update the test fixture
accordingly.
1 participant
Also contains previous BEN-390
Description
Changes
Details