Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for ImprovMX rules API endpoints to the gem, enabling advanced email routing capabilities based on patterns, regular expressions, or conditions.
- Implements all rules-related API endpoints (create, list, get, update, delete, bulk operations)
- Updates minimum Ruby version from 2.4 to 3.0 and upgrades development dependencies
- Adds comprehensive test coverage for the new rules functionality
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/improvmx/rules.rb | Adds new Rules module with methods for managing email routing rules (alias, regex, and CEL types) |
| lib/improvmx/client.rb | Integrates the Rules module into the Client class |
| spec/improvmx/rules_spec.rb | Adds comprehensive test suite for all rules-related methods |
| improvmx.gemspec | Updates minimum Ruby version requirement to 3.0 and rest-client dependency to ~> 2.1 |
| README.md | Documents the new rules functionality with examples for all rule types |
| Gemfile | Updates development dependencies to newer versions compatible with Ruby 3.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def create_alias_rule(domain, alias_name, forward_to, rank: nil, active: true, id: nil) | ||
| data = { | ||
| type: 'alias', | ||
| config: { | ||
| alias: alias_name, | ||
| forward: forward(forward_to) | ||
| }, | ||
| active: active | ||
| } | ||
| data[:rank] = rank if rank | ||
| data[:id] = id if id | ||
|
|
||
| response = post("/domains/#{domain}/rules/", data) | ||
| response.to_h['rule'] | ||
| end |
There was a problem hiding this comment.
The parameter order is inconsistent with the existing codebase patterns. Looking at the aliases module, the pattern used is create_alias(alias_name, forward_to, domain) where the resource identifier comes first, then the data, then the domain. However, this method has create_alias_rule(domain, alias_name, forward_to, ...) with domain first. For consistency, consider reordering parameters to match the existing pattern: create_alias_rule(alias_name, forward_to, domain, rank: nil, active: true, id: nil).
| def create_regex_rule(domain, regex, scopes, forward_to, rank: nil, active: true, id: nil) | ||
| data = { | ||
| type: 'regex', | ||
| config: { | ||
| regex: regex, | ||
| scopes: scopes, | ||
| forward: forward(forward_to) | ||
| }, | ||
| active: active | ||
| } | ||
| data[:rank] = rank if rank | ||
| data[:id] = id if id | ||
|
|
||
| response = post("/domains/#{domain}/rules/", data) | ||
| response.to_h['rule'] | ||
| end |
There was a problem hiding this comment.
The parameter order is inconsistent with the existing codebase patterns. Looking at the aliases module, the pattern used is create_alias(alias_name, forward_to, domain) where the resource identifier comes first, then the data, then the domain. However, this method has create_regex_rule(domain, regex, scopes, forward_to, ...) with domain first. For consistency, consider reordering parameters to match the existing pattern: create_regex_rule(regex, scopes, forward_to, domain, rank: nil, active: true, id: nil).
| def create_cel_rule(domain, expression, forward_to, rank: nil, active: true, id: nil) | ||
| data = { | ||
| type: 'cel', | ||
| config: { | ||
| expression: expression, | ||
| forward: forward(forward_to) | ||
| }, | ||
| active: active | ||
| } | ||
| data[:rank] = rank if rank | ||
| data[:id] = id if id | ||
|
|
||
| response = post("/domains/#{domain}/rules/", data) | ||
| response.to_h['rule'] | ||
| end |
There was a problem hiding this comment.
The parameter order is inconsistent with the existing codebase patterns. Looking at the aliases module, the pattern used is create_alias(alias_name, forward_to, domain) where the resource identifier comes first, then the data, then the domain. However, this method has create_cel_rule(domain, expression, forward_to, ...) with domain first. For consistency, consider reordering parameters to match the existing pattern: create_cel_rule(expression, forward_to, domain, rank: nil, active: true, id: nil).
| def update_rule(rule_id, domain, config: nil, rank: nil, active: nil) | ||
| data = {} | ||
| data[:config] = config if config | ||
| data[:rank] = rank if rank | ||
| data[:active] = active unless active.nil? | ||
|
|
||
| response = put("/domains/#{domain}/rules/#{rule_id}", data) | ||
| response.to_h['rule'] | ||
| rescue NotFoundError | ||
| false | ||
| end |
There was a problem hiding this comment.
The update_rule method has an inconsistent parameter order compared to update_alias in the aliases module. The update_alias method uses update_alias(alias_name, forward_to, domain), but this uses update_rule(rule_id, domain, config: nil, ...). For consistency, consider using update_rule(rule_id, config, domain, rank: nil, active: nil) to match the pattern where the identifier comes first, then the update data, then the domain.
| def bulk_modify_rules(domain, rules, behavior: 'add') | ||
| data = { | ||
| rules: rules, | ||
| behavior: behavior | ||
| } | ||
|
|
||
| response = post("/domains/#{domain}/rules/bulk", data) | ||
| response.to_h | ||
| end |
There was a problem hiding this comment.
The bulk_modify_rules method has an inconsistent parameter order. Looking at existing methods in the codebase, the pattern is to have the resource identifier or primary data first, then the domain. Consider reordering to bulk_modify_rules(rules, domain, behavior: 'add') for consistency.
| def bulk_modify_rules(domain, rules, behavior: 'add') | ||
| data = { | ||
| rules: rules, | ||
| behavior: behavior | ||
| } |
There was a problem hiding this comment.
Missing input validation for the behavior parameter. The documentation indicates valid values are 'add', 'update', or 'delete', but the method accepts any string value without validation. Consider adding validation to ensure only valid behavior values are accepted.
1 participant
add improvMX rules to this gem https://improvmx.com/api#rules
This would open up the option of using rule based routing and make spam filters