BEN-390 Author error and testing suite#6
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
📝 WalkthroughWalkthroughAdds a manual GitHub release workflow, IDE ignores and configs, RuboCop changes, extensive DeliveryMethod error handling and a richer DeliveryError API, new type signatures, many new tests and fixtures, and test helper utilities. Changes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 3
🧹 Nitpick comments (17)
.github/workflows/manual-release.yml (2)
28-29: Remove redundantgit fetch --tagsstep.Line 26 already enables
fetch-tags: truein the checkout action, making this explicit fetch redundant.- - name: Fetch tags - run: git fetch --tags -
53-54: Use--localscope for git config in automation.Scoping git config to the local repository prevents polluting global git configuration when the workflow runs locally for testing or manual debugging.
- git config user.name "github-actions" - git config user.email "github-actions@users.noreply.github.com" + git config --local user.name "github-actions" + git config --local user.email "github-actions@users.noreply.github.com"test/test_helper.rb (2)
3-11: Avoid global Warning.warn monkey‑patch; prefer Warning.ignore (fallback only if missing).Current override is global and may swallow unrelated warnings. Also future Ruby may pass extra args. Gate on Warning.ignore, else fallback with a safer signature.
-module Warning - class << self - alias_method :original_warn, :warn - def warn(msg) - return if msg =~ /assigned but unused variable - testEof/ - original_warn(msg) - end - end -end +if Warning.respond_to?(:ignore) + Warning.ignore(/assigned but unused variable - testEof/) +else + module Warning + class << self + alias_method :original_warn, :warn + def warn(*args, **kwargs) + msg = args.first.to_s + return if msg =~ /assigned but unused variable - testEof/ + original_warn(*args, **kwargs) + end + end + end +end
19-21: Redundant double suppression.You already filter this warning above. Keep one path (ignore vs override), not both.
test/fixtures/api_responses.json (1)
1-23: Fixture looks good.Covers happy path, 4xx/5xx, and malformed payloads. Consider adding 429 to client_errors for rate‑limit handling tests.
test/bento/test_actionmailer.rb (3)
6-7: Drop local ResponseMock; use shared helper.test_helper_extensions already provides build_response. Remove the struct to avoid drift.
-class BentoActionMailerTest < Minitest::Test - ResponseMock = Struct.new(:code, :body, :message) +class BentoActionMailerTest < Minitest::Test
8-14: Use build_delivery_method helper for consistency.Replace manual constructor with build_delivery_method to reuse defaults.
- def setup - @delivery_method = BentoActionMailer::DeliveryMethod.new( - site_uuid: "test-uuid", - publishable_key: "test-publishable-key", - secret_key: "test-secret-key" - ) - end + def setup + @delivery_method = build_delivery_method( + site_uuid: "test-uuid", + publishable_key: "test-publishable-key", + secret_key: "test-secret-key" + ) + end
161-165: Remove duplicate build_response; rely on test helper.Avoid redefining the same utility here.
- private - - def build_response(status, body:, message:) - ResponseMock.new(status.to_s, body, message) - end + # use build_response from Bento::TestHelperExtensions.rubocop.yml (2)
3-4: Consider enabling new cops.NewCops: disable can hide useful checks. Suggest enabling and waiving selectively.
- NewCops: disable + NewCops: enable
36-55: Add BlockLength exemption for tests.Prevents noise on long test contexts.
Metrics/BlockLength: Exclude: - 'test/**/*'lib/bento_actionmailer.rb (2)
80-85: Return trimmed subject.You validate stripped value, but return the untrimmed one. Return the sanitized value.
- value = subject.to_s.strip - raise DeliveryError, "Mail subject is required" if value.empty? - subject.to_s + value = subject.to_s.strip + raise DeliveryError, "Mail subject is required" if value.empty? + value
60-65: Minor: ensure_mail! could validate parts presence sooner.Optional: also check mail responds to :parts to fail fast for non‑Mail objects.
- raise DeliveryError, "Mail message is required" unless mail&.respond_to?(:body) + raise DeliveryError, "Mail message is required" unless mail&.respond_to?(:body) + # Optional hardening: + # raise DeliveryError, "Mail message must respond to parts/body" unless mail.respond_to?(:parts) || mail.body.respond_to?(:parts)test/bento/test_railtie.rb (1)
49-101: Guard constant stubbing against parallel test runs.These global constant swaps aren’t thread-safe. If the suite ever enables parallelization, run this test serially or wrap setup/teardown in a shared Mutex to avoid cross-test flakiness.
sig/bento/actionmailer.rbs (1)
10-26: Optional: capture response shape in RBS for better tooling.Typing the HTTP-ish response improves editor checks around error handling. Consider:
module BentoActionMailer class DeliveryMethod + type ResponseLike = { code: String, body: String, message: String? } + attr_accessor settings: Hash[Symbol, untyped] @@ def initialize: (?Hash[Symbol, untyped]) -> void def deliver!: (untyped mail) -> void + private def handle_response: (ResponseLike) -> voidtest/bento/test_helper_extensions.rb (3)
3-7: Ensure Mail gem is available in test env.These helpers depend on
75-78: Specify UTF-8 when reading fixtures.Windows/locale variance can bite JSON parsing. Read with explicit encoding.
- JSON.parse(File.read(fixture_path)) + JSON.parse(File.read(fixture_path, mode: "r:UTF-8"))
65-73: Avoid mutable default arg (minor).Not mutated here, but safer pattern:
-def build_mail_with_custom_parts(to:, from:, subject:, parts: []) +def build_mail_with_custom_parts(to:, from:, subject:, parts: nil) mail = Mail.new @@ - parts.each { |part| mail.add_part(part) } + Array(parts).each { |part| mail.add_part(part) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (18)
.github/workflows/manual-release.yml(1 hunks).gitignore(1 hunks).idea/.gitignore(1 hunks).idea/bento-actionmailer.iml(1 hunks).idea/modules.xml(1 hunks).idea/php.xml(1 hunks).idea/vcs.xml(1 hunks).rubocop.yml(2 hunks)AGENTS.md(1 hunks)lib/bento_actionmailer.rb(2 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_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 (3)
test/bento/test_helper_extensions.rb (1)
test/bento/test_actionmailer.rb (1)
new(5-166)
test/bento/test_delivery_method.rb (2)
test/bento/test_helper_extensions.rb (7)
build_delivery_method(19-21)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)build_response(23-25)lib/bento_actionmailer.rb (1)
deliver!(38-49)
lib/bento_actionmailer.rb (1)
test/bento/test_actionmailer.rb (1)
new(5-166)
🔇 Additional comments (9)
test/test_helper.rb (1)
23-25: LGTM: test helpers mixed into Minitest.Keeps tests DRY.
test/bento/test_actionmailer.rb (1)
20-159: Broad coverage is solid.Success path, 401/403 specialization, 4xx/5xx, malformed JSON, and unexpected statuses are all exercised.
test/bento/test_delivery_method.rb (3)
64-83: LGTM: multipart extraction and param mapping verified.Good capture of send_mail args and address/subject/body plumbing.
221-345: HTTP stubbing is clean and asserts the right things.Verifies path, headers, payload, host/port/SSL, and error wrapping.
347-412: Utility methods well‑covered.Success range, error parsing (nil/empty/whitespace/malformed), and DeliveryError shaping all tested.
.rubocop.yml (1)
6-8: Line length bump is fine.140 suits request specs and JSON bodies.
lib/bento_actionmailer.rb (1)
121-124: Good: network errors mapped to DeliveryError with details.Covers Timeout, ECONNREFUSED, SocketError, SSL.
test/bento/test_railtie.rb (1)
20-46: Nice coverage of initializer behavior and idempotence.Assertions exercise inheritance, registration timing, and callback effects cleanly. LGTM.
sig/bento/actionmailer.rbs (1)
1-5: Confirm legacy namespace exposure.You still declare Bento::Actionmailer::VERSION here. If the runtime no longer defines it, either keep intentionally for compat (document as deprecated) or remove to avoid drift. Please confirm intent.
| def extract_html_body(mail) | ||
| html_part = Array(mail.body&.parts).find { |part| part.content_type =~ %r{text/html} } | ||
| raise DeliveryError, "No HTML body given. Bento requires an html email body." unless html_part | ||
|
|
||
| html_part.decoded | ||
| end |
There was a problem hiding this comment.
Bug: HTML part detection is case‑sensitive; fails for 'TEXT/HTML'.
This breaks test_deliver_handles_uppercase_html_content_type. Make the match case‑insensitive (or downcase).
- html_part = Array(mail.body&.parts).find { |part| part.content_type =~ %r{text/html} }
+ parts = mail.respond_to?(:parts) ? mail.parts : mail.body&.parts
+ html_part = Array(parts).find { |part| part.content_type.to_s =~ %r{text/html}i }📝 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_html_body(mail) | |
| html_part = Array(mail.body&.parts).find { |part| part.content_type =~ %r{text/html} } | |
| raise DeliveryError, "No HTML body given. Bento requires an html email body." unless html_part | |
| html_part.decoded | |
| end | |
| def extract_html_body(mail) | |
| parts = mail.respond_to?(:parts) ? mail.parts : mail.body&.parts | |
| html_part = Array(parts).find { |part| part.content_type.to_s =~ %r{text/html}i } | |
| raise DeliveryError, "No HTML body given. Bento requires an html email body." unless html_part | |
| html_part.decoded | |
| end |
🤖 Prompt for AI Agents
In lib/bento_actionmailer.rb around lines 66 to 71, the content_type match is
case-sensitive and will miss values like "TEXT/HTML"; make the detection
case-insensitive by either using a case-insensitive regex (e.g. %r{text/html}i)
or comparing part.content_type&.downcase =~ /text\/html/ so uppercase variants
are matched; also guard against nil content_type by using safe navigation
(part.content_type&.) to avoid exceptions.
| def test_deliver_handles_uppercase_html_content_type | ||
| html_part = Mail::Part.new do | ||
| content_type "TEXT/HTML; charset=UTF-8" | ||
| body "<p>Hi</p>" | ||
| end | ||
|
|
||
| mail = build_mail_with_custom_parts( | ||
| to: "user@example.com", | ||
| from: "sender@example.com", | ||
| subject: "Caps", | ||
| parts: [html_part] | ||
| ) | ||
|
|
||
| @delivery_method.stub(:send_mail, ->(**params) { params[:html_body] }) do | ||
| assert_equal "<p>Hi</p>", @delivery_method.deliver!(mail) | ||
| end | ||
| end |
There was a problem hiding this comment.
Nice edge case: uppercase content type.
This exposes a bug in extract_html_body (case sensitivity). See fix in lib.
🤖 Prompt for AI Agents
In test/bento/test_delivery_method.rb around lines 154 to 170, the test shows
extract_html_body is case-sensitive for content types; update the
extract_html_body implementation in the library to normalize the Mail::Part
content_type before checking for "text/html" (for example call to_s and
downcase, or use Mail::Utilities to parse the media type), handle nil
content_type safely, and use a starts_with / media-type compare rather than
exact case-sensitive equality so parts like "TEXT/HTML; charset=UTF-8" are
recognized as HTML.
| 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.
Use a valid From header with display name.
"✨ sender@example.com" isn’t a valid address; prefer display name + address.
- def special_character_mail
- multipart_html_mail(
- to: "büyer+test@example.com",
- from: "✨ sender@example.com",
- subject: "Unicode ✓"
- )
+ def special_character_mail
+ multipart_html_mail(
+ to: "büyer+test@example.com",
+ from: "✨ Sender <sender@example.com>",
+ subject: "Unicode ✓"
+ )
end🤖 Prompt for AI Agents
In test/fixtures/sample_mail.rb around lines 44 to 50, the From header uses an
invalid raw email string ("✨ sender@example.com"); change it to a valid
RFC‑style address with a display name and a separate email address (e.g. a
Unicode display name followed by the actual mailbox in angle brackets), ensuring
the mailbox portion is a valid ASCII email and the display name contains the
emoji if desired; update the test fixture to use that display-name <address>
format so the From header validates correctly.
Ignore benchmark-related files and directories to prevent unintended tracking of performance testing artifacts in version control
feat: Add bench directory to gitignore
1 participant
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