4763: #1
4763: #1
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds CI workflow, raises Ruby target to 3.0, updates dependencies, introduces top-level Members plus ProgramMember and MemberEnrollments resources, adds Client#members and ssl_verify init option, updates Programs#members signature, adds VCR fixtures/filters, and adjusts specs/cassettes and documentation to the new API surface. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Programs as ProgramsResource
participant ProgramMember as ProgramMemberResource
participant API as DataNexusAPI
Note over Client,API: Member-scoped consent creation flow
Client->>Programs: programs(program_id)
Programs->>ProgramMember: members(member_id)
Client->>ProgramMember: consents.create(consent_payload)
ProgramMember->>API: POST /api/members/:member_id/consents { consent: ... }
API-->>ProgramMember: 201 Created { data: ... }
ProgramMember-->>Client: created consent
sequenceDiagram
participant Client
participant Members as MembersResource
participant API as DataNexusAPI
Note over Client,API: Top-level members listing and enrollments CRUD
Client->>Members: members.list(born_on: ...)
Members->>API: GET /api/members?born_on=...
API-->>Members: 200 { data: [...], cursors }
Members-->>Client: Collection
Client->>Members: members.find(member_id)
Members->>API: GET /api/members/:member_id
API-->>Members: 200 { data: ... }
Members-->>Client: member data
Client->>Members: members(member_id).enrollments.create(enrollment: ...)
Members->>API: POST /api/members/:member_id/enrollments { enrollment: ... }
API-->>Members: 201 { data: ... }
Members-->>Client: enrollment data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (5)
✏️ Tip: You can disable this entire section by setting 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: 0
🧹 Nitpick comments (2)
spec/integration/member_consents_spec.rb (2)
41-41: Consider documenting why the consent ID is hardcoded.The
consent_idis hardcoded to2while other identifiers (program_id,member_id) are fetched from environment variables. This creates an inconsistent pattern. If this is intentional due to VCR cassette recordings, a brief comment explaining this would help future maintainers understand the reasoning.💡 Suggested comment
describe 'finding a consent' do + # Consent ID matches the recorded VCR cassette let(:consent_id) { 2 }
55-55: Same hardcoded ID pattern repeated.Consider extracting the shared
consent_idto the top-level describe block to reduce duplication and make it clear this ID is shared across find, update, and delete tests.♻️ Suggested refactor
RSpec.describe 'Member Consents', :vcr, order: :defined do let(:api_key) { ENV.fetch('DATANEXUS_API_KEY', 'test-api-key') } let(:base_url) { ENV.fetch('DATANEXUS_BASE_URL', 'http://localhost:4000') } let(:ssl_verify) { ENV.fetch('DATANEXUS_SSL_VERIFY', 'true') == 'true' } let(:program_id) { ENV.fetch('DATANEXUS_TEST_PROGRAM_ID', 'test-program-id') } let(:member_id) { ENV.fetch('DATANEXUS_TEST_MEMBER_ID', 'test-member-id') } + # Consent ID for find/update/delete tests - matches recorded VCR cassettes + let(:consent_id) { 2 } let(:client) { DataNexus::Client.new(api_key: api_key, base_url: base_url, ssl_verify: ssl_verify) }Then remove the individual
let(:consent_id)declarations from each nested describe block.Also applies to: 69-69
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.github/workflows/ci.yml.rubocop.ymlGemfiledata_nexus.gemspecspec/cassettes/member_consents/create.ymlspec/cassettes/member_consents/delete.ymlspec/cassettes/member_consents/find.ymlspec/cassettes/member_consents/update.ymlspec/integration/member_consents_spec.rbspec/support/vcr.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
🔇 Additional comments (10)
.rubocop.yml (1)
6-6: LGTM!The
TargetRubyVersionupdate to 3.0 is consistent with therequired_ruby_versionin the gemspec, ensuring RuboCop analyzes code with the correct Ruby syntax expectations.spec/support/vcr.rb (1)
15-16: LGTM!Good addition of the
BORN_ONfilter to protect date-of-birth PII in VCR cassettes. The pattern is consistent with existing sensitive data filters.data_nexus.gemspec (2)
15-15: LGTM!The Ruby version bump to
>= 3.0.0is consistent with the RuboCop target and Gemfile compatibility pins.
19-19: LGTM!Adding
changelog_uripointing to GitHub releases aligns with the PR objective to have RubyGems read from GitHub releases. This provides users with direct access to release notes from the gem's RubyGems page.spec/integration/member_consents_spec.rb (1)
3-3: LGTM!Adding
order: :definedensures the test execution order matches the CRUD lifecycle (create → find → update → delete), which is appropriate for integration tests that may have implicit dependencies.Gemfile (1)
19-24: Version constraints for Ruby 3.0/3.1 compatibility are correct.The pins for
public_suffix < 7.0anderb ~> 4.0are accurate—both 7.0+ versions require Ruby >= 3.2. These constraints properly support the Ruby 3.0 and 3.1 entries in the CI matrix. Consider adding a TODO to remove these constraints once Ruby 3.0/3.1 support is dropped.spec/cassettes/member_consents/update.yml (1)
3-40: LGTM!Cassette properly records the PATCH interaction with sensitive data correctly filtered using placeholders (
<API_KEY>,<MEMBER_ID>,<PROGRAM_ID>).spec/cassettes/member_consents/create.yml (1)
3-42: LGTM!Cassette properly records the POST interaction for consent creation. Sensitive data is correctly filtered, and the test phone number (
+15558675309) is an appropriate choice for test fixtures.spec/cassettes/member_consents/delete.yml (1)
3-36: LGTM!Cassette properly records the DELETE interaction with correct 204 No Content response. Sensitive data is appropriately filtered.
spec/cassettes/member_consents/find.yml (1)
3-40: LGTM!Cassette properly records the GET interaction for consent retrieval. Consent ID 2 is used consistently with other cassettes (update, delete), and sensitive data is correctly filtered.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/data_nexus/resources/program_members.rb (1)
57-61: Add pagination parameters toallowed_params.The
listmethod silently filters out pagination parameters (after,before,first,last) because they're not inallowed_params. However, the class documentation explicitly shows pagination usage in line 17:collection = client.programs("uuid").members.list(first: 50). This contradicts the implementation and breaks the documented example. Add these parameters toallowed_paramsto match theMembers#listpattern (seelib/data_nexus/resources/members.rblines 67-71).
🤖 Fix all issues with AI agents
In @lib/data_nexus/resources/program_member.rb:
- Around line 89-92: The household method currently constructs a Collection via
Collection.new(response, resource: self, params: {}), but ProgramMember lacks a
list method so Collection#next_page will raise NoMethodError; either implement a
ProgramMember#list signature that accepts params like (after: nil, **kwargs) and
delegates to the same endpoint (e.g., perform the GET to
"#{base_path}/#{member_id}/household" with pagination params and return a
Collection or items), or avoid returning a Collection by returning a
non-paginated object (e.g., a plain parsed response or a different wrapper) when
the API does not support pagination; update the household method accordingly and
ensure any code relying on Collection#next_page can call ProgramMember#list if
you implement it.
🧹 Nitpick comments (2)
spec/integration/members_spec.rb (1)
82-90: Consider verifying the updated field value in the response.The update test verifies the response structure but doesn't confirm that
phone_numberwas actually updated in the returned data. This could mask issues where the API accepts the request but doesn't apply the change.💡 Suggested enhancement
it 'returns the updated member', vcr: { cassette_name: 'members/update' } do response = client.members.update( member_id, member: { phone_number: '+15551234567' } ) expect(response).to be_a(Hash) expect(response).to have_key(:data) + expect(response[:data]).to have_key(:phone_number) endspec/integration/member_enrollments_spec.rb (1)
25-33: Consider using a separate VCR cassette for this test case.This test reuses the
member_enrollments/createcassette from the previous test but sends a different payload (onlyenrolled_atvs. both dates). If the cassette was recorded with the first test's payload, the HTTP interaction may not match, potentially causing test flakiness or false passes.Suggested fix
- it 'auto-injects the program_id', vcr: { cassette_name: 'member_enrollments/create' } do + it 'auto-injects the program_id', vcr: { cassette_name: 'member_enrollments/create_with_program_id' } do
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.mise.local.toml.examplelib/data_nexus/client.rblib/data_nexus/resources/member_consents.rblib/data_nexus/resources/member_enrollments.rblib/data_nexus/resources/members.rblib/data_nexus/resources/program_member.rblib/data_nexus/resources/program_members.rblib/data_nexus/resources/programs.rbspec/cassettes/member_enrollments/create.ymlspec/cassettes/member_enrollments/delete.ymlspec/cassettes/member_enrollments/find.ymlspec/cassettes/member_enrollments/update.ymlspec/cassettes/members/find.ymlspec/cassettes/members/list.ymlspec/cassettes/members/list_by_born_on.ymlspec/cassettes/members/list_by_name.ymlspec/cassettes/members/list_by_program.ymlspec/cassettes/members/list_by_updated_since.ymlspec/cassettes/members/list_paginated.ymlspec/cassettes/members/update.ymlspec/integration/member_consents_spec.rbspec/integration/member_enrollments_spec.rbspec/integration/members_spec.rbspec/integration/program_members_spec.rbspec/support/vcr.rb
✅ Files skipped from review due to trivial changes (6)
- spec/cassettes/members/list.yml
- spec/cassettes/member_enrollments/delete.yml
- spec/cassettes/members/list_by_name.yml
- spec/cassettes/members/list_paginated.yml
- spec/cassettes/members/find.yml
- spec/cassettes/member_enrollments/create.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- spec/support/vcr.rb
- spec/integration/member_consents_spec.rb
🧰 Additional context used
🧬 Code graph analysis (3)
spec/integration/members_spec.rb (2)
lib/data_nexus/client.rb (1)
members(59-61)lib/data_nexus/resources/members.rb (3)
list(66-76)find(86-89)update(102-105)
lib/data_nexus/resources/programs.rb (1)
lib/data_nexus/client.rb (1)
members(59-61)
lib/data_nexus/client.rb (1)
lib/data_nexus/resources/programs.rb (1)
members(61-67)
🪛 Brakeman (7.1.1)
lib/data_nexus/resources/member_enrollments.rb
[medium] 109-109: Possible SQL injection
Type: SQL Injection
Confidence: Medium
More info: https://brakemanscanner.org/docs/warning_types/sql_injection/
(SQL Injection)
🔇 Additional comments (25)
.mise.local.toml.example (1)
5-12: LGTM!The updated comment provides clearer context about the purpose of these environment variables, and the new
DATANEXUS_TEST_ENROLLMENT_IDvariable follows the established naming convention and placeholder pattern. This addition properly supports the new member enrollment test fixtures being introduced in this PR.lib/data_nexus/resources/member_consents.rb (2)
11-28: Documentation examples are consistent with the new fluent API.The updated examples correctly demonstrate the new nested resource pattern where
member_idis passed to.members()rather than to the terminal action. All examples are internally consistent across the class documentation.
61-111: LGTM!Method-level documentation examples are consistent with the class-level documentation and follow the same fluent API pattern.
spec/cassettes/members/update.yml (1)
1-44: VCR cassette properly configured with appropriate placeholders.Sensitive identifiers are correctly replaced with placeholders (
<MEMBER_ID>,<API_KEY>,<PROGRAM_ID>,<BORN_ON>,<EMPLOYEE_ID>). The cassette captures the expected PATCH request/response flow for member updates.spec/cassettes/members/list_by_born_on.yml (1)
1-56: LGTM!The cassette correctly captures the
born_onfilter scenario with proper placeholder usage in both the query parameter and response body. Pagination headers are preserved for testing cursor-based pagination.spec/cassettes/members/list_by_program.yml (1)
1-96: VCR cassette captures list-by-program scenario correctly.The cassette properly uses placeholders for sensitive identifiers. The selective placeholder usage (where some
born_on/employee_idvalues are placeholders while others are literal test data) is a common VCR pattern when filtering specific values that appear in multiple test fixtures.spec/cassettes/members/list_by_updated_since.yml (1)
1-96: LGTM!The cassette properly captures the
updated_sincefilter scenario. Using a fixed timestamp (2020-01-01T00:00:00Z) is appropriate for reproducible test playback, as this is a filter parameter rather than sensitive data.lib/data_nexus/resources/program_members.rb (1)
20-66: LGTM!The
ProgramMembersresource is well-structured with clear documentation, proper separation of concerns (listing-only, with member-specific operations delegated toProgramMember), and follows idiomatic Ruby patterns for parameter filtering.spec/cassettes/member_enrollments/find.yml (1)
1-41: LGTM!The VCR cassette is properly structured with sensitive data correctly filtered (
<MEMBER_ID>,<ENROLLMENT_ID>,<API_KEY>,<PROGRAM_ID>). The response structure aligns with the expected enrollment data format.spec/integration/members_spec.rb (1)
1-64: LGTM!The integration tests provide comprehensive coverage for the Members resource, including listing, filtering, pagination, and CRUD operations. Environment variable handling with sensible defaults is well-implemented, and the VCR cassette usage ensures reproducible tests.
spec/cassettes/member_enrollments/update.yml (1)
1-41: LGTM!The VCR cassette properly records the PATCH request for updating enrollment expiration. Sensitive data is correctly filtered, and the response confirms the
expires_atfield was updated as expected.lib/data_nexus/resources/programs.rb (2)
61-67: LGTM!The dual-mode
membersmethod is a clean implementation of the fluent API pattern. The optional parameter approach allows both listing (programs(id).members.list) and member-specific operations (programs(id).members(member_id).find) through a single intuitive interface. Documentation clearly explains both usage patterns.
3-4: LGTM!The require statements are properly ordered, with the new
program_memberdependency added alongside the existingprogram_membersrequire.spec/integration/member_enrollments_spec.rb (1)
1-72: Well-structured integration tests covering CRUD operations.The test suite covers the full lifecycle of member enrollments with proper VCR cassette usage, environment variable defaults for flexibility, and appropriate assertions on response structure.
lib/data_nexus/client.rb (2)
52-61: LGTM!The
membersaccessor follows the same pattern asprograms, providing a clean and consistent API surface.
84-91: Good handling of the ssl_verify parameter.The ternary on line 90 correctly distinguishes between an explicit
falsevalue and a missing/nil value, ensuringssl_verify: falseis respected.lib/data_nexus/resources/program_member.rb (1)
1-134: Well-structured resource with clear documentation.The
ProgramMemberclass provides a clean fluent API for accessing member-specific operations and nested resources (consents, enrollments). The YARD documentation is thorough with practical examples.lib/data_nexus/resources/member_enrollments.rb (3)
108-110: False positive: This is an HTTP DELETE, not SQL.The Brakeman SQL injection warning is a false positive. The
connection.deletemethod performs an HTTP DELETE request against the REST API, not a SQL query. Theenrollment_idis interpolated into a URL path, which is the expected behavior for RESTful resource deletion.
66-70: Good auto-injection of program_id with implicit override behavior.The
mergeensures the resource chain'sprogram_idis always used, which prevents accidental mismatches. Note that if a caller passesprogram_idin the enrollment hash, it will be silently overwritten. This is likely the intended behavior per the@notedocumentation.
1-122: Clean CRUD implementation for member enrollments.The resource provides a complete set of operations with consistent patterns and good documentation. The program_id injection is well-documented via the
@notetag.lib/data_nexus/resources/members.rb (2)
66-76: Clean parameter filtering implementation.Using
slicewith explicit allowed params followed bycompactis a good defensive pattern that prevents unexpected query parameters from reaching the API.
1-117: Well-implemented top-level Members resource.The resource provides listing with flexible filtering, pagination support via Collection, and standard find/update operations. Documentation is thorough with practical examples.
spec/integration/program_members_spec.rb (3)
48-53: LGTM!The fluent API pattern
members(member_id).findis cleaner and more readable than passing the ID as a method argument. The test appropriately validates the response structure.
59-66: LGTM!The fluent API pattern is consistently applied for the update operation. The test validates that the response contains the expected
:datawrapper.
72-77: LGTM!The fluent API pattern for
householdis consistent with other member operations. Correctly expects aDataNexus::Collectionfor the list of household members.
2 participants
Add github ci yml and ensure rubygems will read from github releases.
Add root level members routes. Add routes for enrollments.