Open
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
spec/operations/fdsh/ridp/rba/transform_family_to_rba_request_spec.rb (1)
8-8: Consider renaminglet(:subject)to avoid shadowing RSpec's built-in method.Using
let(:subject)shadows RSpec's built-insubjecthelper, which can cause confusion. While functional, a more explicit name improves clarity.♻️ Suggested rename
- let(:subject) { described_class.new } + let(:operation) { described_class.new }Then update usages like
subject.call(payload)tooperation.call(payload), or alternatively use RSpec's implicitsubject { described_class.new }syntax.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/operations/fdsh/ridp/rba/transform_family_to_rba_request_spec.rb` at line 8, Rename the test helper that currently uses let(:subject) to avoid shadowing RSpec's built-in subject; change the declaration let(:subject) { described_class.new } to a clearer name such as let(:operation) { described_class.new } and update all usages (e.g., subject.call(payload)) to operation.call(payload) or switch to RSpec's implicit subject { described_class.new } and adjust calls accordingly so references use the new identifier (operation) instead of subject.app/operations/fdsh/ridp/rba/transform_family_to_rba_request.rb (1)
53-56: Consider preferring 'home' address similar to phone selection logic.The code takes the first address regardless of
kind, whilefetch_telephone(lines 143-146) explicitly prefers 'mobile', then 'home'. For consistency and accuracy, consider preferring the 'home' address for residential verification.♻️ Suggested approach
def build_request_hash(person, family_hash) - address = person.addresses&.first + address = person.addresses&.detect { |a| a.kind == 'home' } || + person.addresses&.first ridp_rba = family_hash.dig(:external_payloads, :ridp_rba)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/operations/fdsh/ridp/rba/transform_family_to_rba_request.rb` around lines 53 - 56, build_request_hash currently grabs person.addresses&.first which may pick a non-residential address; update it to prefer an address with kind == 'home' (falling back to first or another appropriate kind) similar to the fetch_telephone selection logic. Locate build_request_hash and change the address selection to search person.addresses for a 'home' entry first, then fall back to the first address if none found, ensuring downstream usage of address uses this chosen address.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/operations/fdsh/ridp/rba/transform_family_to_rba_request.rb`:
- Around line 103-105: The ZIP parsing assumes a continuous-digit string and
fails on hyphenated inputs; in the transform (transform_family_to_rba_request /
where zipCode and zipCodeExtension are built) normalize address.zip first by
stripping non-digit chars (e.g., gsub(/\D/, '')) into a local variable, then
derive zipCode from digits[0..4] and zipCodeExtension from digits[5..8] and only
set the extension when present (keep the .presence guard). This ensures inputs
like "04101-1234" yield "04101" and "1234" and prevents leading hyphens from
being returned.
In `@lib/aca_entities/fdsh/ridp/rba/schemas/RBA-RIDP-Response-schema.json`:
- Around line 28-37: The description for the JSON schema property
finalDecisionCode is inconsistent with its enum: it mentions "NO DECISION" but
the enum value is "NODECISION"; update the description string in the
finalDecisionCode block to use "NODECISION" (or otherwise match the exact enum
token used) so the human-readable docs align with the enum values defined for
finalDecisionCode.
---
Nitpick comments:
In `@app/operations/fdsh/ridp/rba/transform_family_to_rba_request.rb`:
- Around line 53-56: build_request_hash currently grabs person.addresses&.first
which may pick a non-residential address; update it to prefer an address with
kind == 'home' (falling back to first or another appropriate kind) similar to
the fetch_telephone selection logic. Locate build_request_hash and change the
address selection to search person.addresses for a 'home' entry first, then fall
back to the first address if none found, ensuring downstream usage of address
uses this chosen address.
In `@spec/operations/fdsh/ridp/rba/transform_family_to_rba_request_spec.rb`:
- Line 8: Rename the test helper that currently uses let(:subject) to avoid
shadowing RSpec's built-in subject; change the declaration let(:subject) {
described_class.new } to a clearer name such as let(:operation) {
described_class.new } and update all usages (e.g., subject.call(payload)) to
operation.call(payload) or switch to RSpec's implicit subject {
described_class.new } and adjust calls accordingly so references use the new
identifier (operation) instead of subject.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f20f6be-91cb-4510-96df-278ca9a09dc9
📒 Files selected for processing (5)
app/operations/fdsh/ridp/rba/transform_family_to_rba_request.rblib/aca_entities/fdsh/ridp/rba/schemas/RBA-RIDP-Request-schema.jsonlib/aca_entities/fdsh/ridp/rba/schemas/RBA-RIDP-Response-schema.jsonlib/aca_entities/fdsh/ridp/rba/schemas/USStateCode-schema.jsonspec/operations/fdsh/ridp/rba/transform_family_to_rba_request_spec.rb
app/operations/fdsh/ridp/rba/transform_family_to_rba_request.rb
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/operations/fdsh/ridp/rba/transform_family_to_rba_request.rb (1)
83-94: Consider defensive nil-check onperson_demographics.If
person_demographicsis unexpectedly nil (e.g., malformed data bypassing contract),demographics.dobwould raiseNoMethodErrorcaught by the rescue block, but with a less informative message. The FamilyContract likely guarantees this field's presence, so this is optional.🛡️ Optional defensive check
def build_person_fields(person) demographics = person.person_demographics + return Failure('Missing person demographics') unless demographics {Note: This returns a
Failurewhich would require changing the method signature to return aResultand yielding it inbuild_request_hash. Alternatively, rely on the existing contract validation to guarantee presence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/operations/fdsh/ridp/rba/transform_family_to_rba_request.rb` around lines 83 - 94, build_person_fields currently assumes person.person_demographics is present; add a defensive check at the top of build_person_fields to handle nil demographics (e.g., if demographics = person.person_demographics; raise ArgumentError, "missing person_demographics for person #{person.person_id || person.person_name&.last_name}" if demographics.nil?) so the error is explicit and informative instead of a generic NoMethodError; if you prefer returning a Failure Result instead, change build_person_fields to return a Result and propagate that from build_request_hash (update build_request_hash to handle the returned Failure), but at minimum add the explicit nil guard and a clear error message referencing build_person_fields and person.person_demographics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/operations/fdsh/ridp/rba/transform_family_to_rba_request.rb`:
- Around line 83-94: build_person_fields currently assumes
person.person_demographics is present; add a defensive check at the top of
build_person_fields to handle nil demographics (e.g., if demographics =
person.person_demographics; raise ArgumentError, "missing person_demographics
for person #{person.person_id || person.person_name&.last_name}" if
demographics.nil?) so the error is explicit and informative instead of a generic
NoMethodError; if you prefer returning a Failure Result instead, change
build_person_fields to return a Result and propagate that from
build_request_hash (update build_request_hash to handle the returned Failure),
but at minimum add the explicit nil guard and a clear error message referencing
build_person_fields and person.person_demographics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e1bdd73-6556-4b14-90bf-09bab101c379
📒 Files selected for processing (1)
app/operations/fdsh/ridp/rba/transform_family_to_rba_request.rb
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/operations/fdsh/ridp/rba/transform_family_to_rba_request.rb (2)
54-55: Prefer deterministic home-address selection before fallback.Using the first address can be order-dependent and may send a mailing/old address to RIDP. Prefer selecting the home/residential address first, then fallback to first.
♻️ Proposed refinement
- address = person.addresses&.first + address = person.addresses&.detect { |a| a.kind == 'home' } || person.addresses&.first🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/operations/fdsh/ridp/rba/transform_family_to_rba_request.rb` around lines 54 - 55, The code currently picks an address with person.addresses&.first which is order-dependent; change the selection in transform_family_to_rba_request (or the surrounding method in transform_family_to_rba_request.rb) to first attempt a deterministic home/residential lookup (e.g., find an address where the type/kind equals "home" or "residential" or a residence flag is true) and only if none found fall back to addresses.first; update the local variable address (and any downstream usage such as ridp_rba handling) to use this deterministic selection so RIDP receives the residential/home address when available.
69-70: Avoid broadStandardErrorrescue in request assembly.Catching everything here can mask real coding bugs and make debugging harder. Consider rescuing only expected payload/shape exceptions and letting unexpected errors bubble.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/operations/fdsh/ridp/rba/transform_family_to_rba_request.rb` around lines 69 - 70, The rescue in TransformFamilyToRBARequest (in method call/build) currently swallows all StandardError; change it to only rescue expected payload/shape errors (for example KeyError, TypeError, and JSON::ParserError if parsing is involved) and return Failure with the specific error message for those exceptions, but re-raise any other unexpected exceptions so real bugs bubble up; update the rescue clause to list these specific exception classes and ensure other errors are not rescued.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/operations/fdsh/ridp/rba/transform_family_to_rba_request.rb`:
- Around line 54-55: The code currently picks an address with
person.addresses&.first which is order-dependent; change the selection in
transform_family_to_rba_request (or the surrounding method in
transform_family_to_rba_request.rb) to first attempt a deterministic
home/residential lookup (e.g., find an address where the type/kind equals "home"
or "residential" or a residence flag is true) and only if none found fall back
to addresses.first; update the local variable address (and any downstream usage
such as ridp_rba handling) to use this deterministic selection so RIDP receives
the residential/home address when available.
- Around line 69-70: The rescue in TransformFamilyToRBARequest (in method
call/build) currently swallows all StandardError; change it to only rescue
expected payload/shape errors (for example KeyError, TypeError, and
JSON::ParserError if parsing is involved) and return Failure with the specific
error message for those exceptions, but re-raise any other unexpected exceptions
so real bugs bubble up; update the rescue clause to list these specific
exception classes and ensure other errors are not rescued.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 38b6ebd1-2d21-40a8-b132-f95cd8aa15f8
📒 Files selected for processing (1)
app/operations/fdsh/ridp/rba/transform_family_to_rba_request.rb
ymhari
reviewed
Mar 23, 2026
| Success(primary.person) | ||
| end | ||
|
|
||
| def build_request_hash(person, family_hash) |
Contributor
There was a problem hiding this comment.
Can we move this into its own operation/class.
ymhari
requested changes
Mar 23, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spec/operations/fdsh/ridp/rba/build_rba_request_spec.rb (1)
63-68: Consider usingletblocks instead of instance variables.Using
let(:result) { subject.call(person, family_hash) }instead of@resultin abeforeblock would be more idiomatic RSpec and provide lazy evaluation.♻️ Suggested refactor
context 'with valid inputs' do - before { `@result` = subject.call(person, family_hash) } + let(:result) { subject.call(person, family_hash) } it 'returns success' do - expect(`@result.success`?).to be_truthy + expect(result.success?).to be_truthy endApply similar changes throughout the file for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/operations/fdsh/ridp/rba/build_rba_request_spec.rb` around lines 63 - 68, Replace the instance variable usage for the call result with an RSpec let: remove the before hook that assigns `@result` = subject.call(person, family_hash) and add let(:result) { subject.call(person, family_hash) }, then update assertions that reference `@result` (e.g., expect(`@result.success`?).to be_truthy) to use result (e.g., expect(result.success?).to be_truthy); apply the same change consistently anywhere else in this spec file where `@result` is set or referenced (keeping subject.call(person, family_hash), person and family_hash as the call inputs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@spec/operations/fdsh/ridp/rba/build_rba_request_spec.rb`:
- Around line 63-68: Replace the instance variable usage for the call result
with an RSpec let: remove the before hook that assigns `@result` =
subject.call(person, family_hash) and add let(:result) { subject.call(person,
family_hash) }, then update assertions that reference `@result` (e.g.,
expect(`@result.success`?).to be_truthy) to use result (e.g.,
expect(result.success?).to be_truthy); apply the same change consistently
anywhere else in this spec file where `@result` is set or referenced (keeping
subject.call(person, family_hash), person and family_hash as the call inputs).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 024bdf26-8c38-4eba-8507-661dec49dc7c
📒 Files selected for processing (4)
app/operations/fdsh/ridp/rba/build_rba_request.rbapp/operations/fdsh/ridp/rba/transform_family_to_rba_request.rbspec/operations/fdsh/ridp/rba/build_rba_request_spec.rbspec/operations/fdsh/ridp/rba/transform_family_to_rba_request_spec.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/operations/fdsh/ridp/rba/transform_family_to_rba_request_spec.rb
ymhari
requested changes
Mar 24, 2026
Contributor
ymhari
left a comment
ymhari
left a comment
There was a problem hiding this comment.
Make the changes as per our discussion in the call.
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spec/operations/fdsh/ridp/rba/build_rba_request_spec.rb (1)
191-229: Consolidate duplicatedaddress priorityspecs.
context 'address priority'is defined twice with the same behavior checks. Keep one block (preferably theinstance_doubleversion) to reduce duplication and spec maintenance overhead.♻️ Suggested cleanup
- context 'address priority' do - it 'prefers home address' do - home = double(kind: 'home', address_1: 'Home St', city: 'Augusta', state: 'ME', zip: '04210') - work = double(kind: 'work', address_1: 'Work St', city: 'Augusta', state: 'ME', zip: '04211') - allow(person).to receive(:addresses).and_return([work, home]) - result = subject.call(valid_params) - expect(result.value![:rbaRIDPRequest][:streetName]).to eq('Home St') - end - - it 'falls back to first address when no home address' do - work = double(kind: 'work', address_1: 'Work St', city: 'Augusta', state: 'ME', zip: '04211') - allow(person).to receive(:addresses).and_return([work]) - result = subject.call(valid_params) - expect(result.value![:rbaRIDPRequest][:streetName]).to eq('Work St') - end - end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/operations/fdsh/ridp/rba/build_rba_request_spec.rb` around lines 191 - 229, Remove the duplicated "context 'address priority'" block and keep only the version using instance_double(AcaEntities::Locations::Address); specifically, delete the earlier block that uses plain double(...) and retain the later block that sets up home/work as instance_double, leaves the allow(person).to receive(:addresses) stubs, and the expectations against subject.call(valid_params)[:rbaRIDPRequest][:streetName]; ensure there is a single context 'address priority' with both the "prefers home address" and "falls back to first address when no home address" examples.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@spec/operations/fdsh/ridp/rba/build_rba_request_spec.rb`:
- Around line 191-229: Remove the duplicated "context 'address priority'" block
and keep only the version using
instance_double(AcaEntities::Locations::Address); specifically, delete the
earlier block that uses plain double(...) and retain the later block that sets
up home/work as instance_double, leaves the allow(person).to receive(:addresses)
stubs, and the expectations against
subject.call(valid_params)[:rbaRIDPRequest][:streetName]; ensure there is a
single context 'address priority' with both the "prefers home address" and
"falls back to first address when no home address" examples.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 517c5370-acb6-4ecb-9c26-00acc5564e73
📒 Files selected for processing (3)
app/operations/fdsh/ridp/rba/build_rba_request.rbapp/operations/fdsh/ridp/rba/transform_family_to_rba_request.rbspec/operations/fdsh/ridp/rba/build_rba_request_spec.rb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://app.clickup.com/t/868hwptd9
Summary by CodeRabbit
New Features
Tests