Add Mobile Money channel support via Charges resource#1
Add Mobile Money channel support via Charges resource#1nanafox merged 3 commits intonanafox:mainfrom
Conversation
Add PaystackSdk::Resources::Charges with:
POST /charge for Mobile Money initiation (mobile_money.phone, mobile_money.provider)
POST /charge/submit_otp for OTP/voucher flows (e.g., Vodafone)
Validate payloads (email, amount, currency, reference) and provider codes
(mtn, atl, vod, mpesa, orange, wave)
Expose client.charges
Add specs for Charges resource and client accessor
No breaking changes. Mobile Money status verification continues via
/transaction/verify/{reference} and webhook handling remains app-level.
Also, Refactor code to standardize string delimiters and improve readability
- No functional changes were made; this is purely a formatting improvement.
nanafox
left a comment
nanafox
left a comment
There was a problem hiding this comment.
Thank you for your submission, @smorttey. Your contribution will help improve the project. To ensure consistency, I use Standard Ruby for linting and formatting. Since the current submission uses Rubocop, it introduces formatting differences with the established codebase. For this project, let's stick to Standard Ruby and avoid Rubocop to maintain a uniform style.
If you'd prefer to use Rubocop, I suggest updating your .rubocop.yml file to include and use StandardRB's rules.
…m with standard rb instead of rubocop
WalkthroughAdds a new Charges resource (mobile money and OTP flows), wires it into the client, adds related specs, refactors response iteration and several validators for early returns, applies minor stylistic/test changes, and adjusts development dependencies in the gemspec. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Client as PaystackSdk::Client
participant Charges as Resources::Charges
participant API as Paystack API
App->>Client: client.charges
Client->>Charges: instantiate with @connection
App->>Charges: mobile_money(payload)
Note right of Charges: validate payload (email, amount, currency)\nvalidate mobile_money (phone, provider)
Charges->>API: POST /charge (payload)
API-->>Charges: HTTP response
Charges-->>App: PaystackSdk::Response
opt OTP required
App->>Charges: submit_otp({reference, otp})
Charges->>API: POST /charge/submit_otp
API-->>Charges: HTTP response
Charges-->>App: PaystackSdk::Response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Comment |
|
Bro, missed your comment before reverting to standard rb. Need to change my github to receive emails. |
![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 (9)
paystack_sdk.gemspec (1)
42-46: Dev deps: confirm intent to keepdebugand finalize versions.AI summary says
debugwas removed, but gemspec still includes it. If unused, remove to slim dev surface.Apply if you want to drop
debug:spec.add_dependency "faraday", "~> 2.13.1" -spec.add_development_dependency "debug", "~> 1.9.0" spec.add_development_dependency "irb", "~> 1.15.1" spec.add_development_dependency "rake", "~> 13.2.1" spec.add_development_dependency "rspec", "~> 3.13" spec.add_development_dependency "standard", "~> 1.49.0"lib/paystack_sdk/response.rb (2)
212-221: Enumerator-friendlyeachis correct.
enum_for(:each)+ wrapped yields are idiomatic.Optional: include Enumerable for free methods:
class Response + include Enumerable
244-249: Wrappedfirst/lastare correct.Consider aligning
#[]with string/symbol dual‑key access (doc claims both). Proposed outside this hunk:- if @raw_data.is_a?(Hash) - value = @raw_data[key.is_a?(String) ? key.to_sym : key] + if @raw_data.is_a?(Hash) + k = key + value = + if k.is_a?(String) + @raw_data[k] || @raw_data[k.to_sym] + else + @raw_data[k] || @raw_data[k.to_s] + end wrap_value(value)lib/paystack_sdk/validations.rb (1)
56-60: Missing params: consider reporting all, not just first.Improves DX in tests and clients.
Example:
- param = missing_params.first - raise PaystackSdk::MissingParamError.new(param) + raise PaystackSdk::MissingParamError.new(missing_params.join(", "))If the error class expects a single field, consider a new error type for multiple fields instead.
bin/console (1)
10-11: Optional: load env vars in console for easier local testing.
If you rely on PAYSTACK_SECRET_KEY locally, consider auto-loading dotenv.require 'bundler/setup' +require 'dotenv/load' # optional: auto‑load .env for console sessions require 'paystack_sdk'spec/client_spec.rb (1)
45-52: Also assert memoization of client.charges.
Ensures we only instantiate Charges once.- describe "#charges" do + describe "#charges" do it "returns an instance of Charges resource" do - charges_double = instance_double(PaystackSdk::Resources::Charges) - allow(PaystackSdk::Resources::Charges).to receive(:new).and_return(charges_double) + charges_double = instance_double(PaystackSdk::Resources::Charges) + expect(PaystackSdk::Resources::Charges).to receive(:new).once.and_return(charges_double) - expect(client.charges).to eq(charges_double) + expect(client.charges).to eq(charges_double) + # Memoized on subsequent calls + expect(client.charges).to equal(charges_double) end endspec/resources/charges_spec.rb (1)
7-59: Add tests for missing provider and missing reference.
These round out edge cases aligned with validations.# In describe "#mobile_money" it "raises an error when mobile_money provider is missing" do payload[:mobile_money].delete(:provider) expect { charges.mobile_money(payload) } .to raise_error(PaystackSdk::MissingParamError, /mobile_money provider/i) end # In describe "#submit_otp" it "raises an error when reference is missing" do invalid = otp_payload.except(:reference) expect { charges.submit_otp(invalid) } .to raise_error(PaystackSdk::MissingParamError, /reference/i) endAlso applies to: 61-83
lib/paystack_sdk/resources/charges.rb (2)
14-14: Provider list OK; consider future-proofing with alias handling.
If Paystack adds aliases (e.g., vodafone/vod), consider accepting known aliases via a mapping table.
61-83: Optional: default currency per provider group.
If currency is omitted, consider defaulting GHS for mtn/vod/atl, KES for mpesa, etc., or at least warn/log when omitted to reduce integration errors.Would you like me to add a small mapping with defaults and update docs?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
bin/console(1 hunks)lib/paystack_sdk/client.rb(2 hunks)lib/paystack_sdk/resources/charges.rb(1 hunks)lib/paystack_sdk/resources/customers.rb(1 hunks)lib/paystack_sdk/response.rb(6 hunks)lib/paystack_sdk/validations.rb(8 hunks)paystack_sdk.gemspec(1 hunks)spec/client_spec.rb(1 hunks)spec/resources/banks_spec.rb(1 hunks)spec/resources/charges_spec.rb(1 hunks)spec/resources/transactions_spec.rb(4 hunks)spec/resources/transfer_recipients_spec.rb(1 hunks)spec/resources/transfers_spec.rb(2 hunks)spec/resources/verification_spec.rb(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
spec/resources/verification_spec.rb (2)
lib/paystack_sdk/client.rb (1)
verification(122-124)lib/paystack_sdk/resources/verification.rb (4)
resolve_account(6-34)resolve_account(9-13)resolve_card_bin(17-20)validate_account(26-33)
spec/resources/charges_spec.rb (3)
lib/paystack_sdk/client.rb (1)
charges(136-138)lib/paystack_sdk/resources/charges.rb (2)
mobile_money(31-36)submit_otp(46-57)lib/paystack_sdk/response.rb (1)
success?(122-124)
spec/resources/transfer_recipients_spec.rb (1)
lib/paystack_sdk/resources/transfer_recipients.rb (2)
create(5-45)create(8-16)
spec/resources/transfers_spec.rb (1)
lib/paystack_sdk/resources/transfers.rb (2)
create(5-45)create(8-16)
lib/paystack_sdk/resources/customers.rb (1)
lib/paystack_sdk/validations.rb (1)
validate_date_format!(106-118)
spec/resources/banks_spec.rb (2)
lib/paystack_sdk/resources/transactions.rb (1)
list(105-128)lib/paystack_sdk/resources/banks.rb (2)
list(5-19)list(8-18)
spec/client_spec.rb (1)
lib/paystack_sdk/client.rb (1)
charges(136-138)
lib/paystack_sdk/resources/charges.rb (2)
lib/paystack_sdk/resources/base.rb (1)
handle_response(43-45)lib/paystack_sdk/validations.rb (4)
validate_fields!(204-240)validate_hash!(39-43)validate_presence!(67-71)validate_allowed_values!(137-148)
🔇 Additional comments (28)
spec/resources/banks_spec.rb (1)
27-30: Style switch toexpect do … endlooks good.Functionally identical, aligns suite style.
spec/resources/transfers_spec.rb (2)
9-9: Numeric literal with underscore is fine.Improves readability without changing value.
26-29: Style switch toexpect do … endlooks good.Keeps behavior; consistent with other specs.
spec/resources/verification_spec.rb (4)
18-21: LGTM on style change.
24-27: LGTM on style change.
41-44: LGTM on style change.
72-75: LGTM on style change.spec/resources/transfer_recipients_spec.rb (1)
25-28: LGTM on style change.No behavioral impact.
lib/paystack_sdk/response.rb (3)
232-236: Delegation to size/length/count/empty? looks good.
263-263: Good switch to Regexp.last_match.Avoids globals, thread‑safer.
89-89: Raising AuthenticationError for 401 is intended — callers must handle AuthenticationError.Search of the repo shows resources call handle_response(...) and README/tests expect or rescue AuthenticationError (and tests mock successful Faraday::Response); no callers were found that rely on receiving an unsuccessful Response object for 401.
lib/paystack_sdk/validations.rb (7)
40-43: Early return invalidate_hash!is fine.
68-71: Presence check reads well.
94-97: Reference format guard is good.
110-110: Nil date early return keeps flow clear.
156-166: Email regex baseline is acceptable.
174-184: Currency ISO-3 validator is fine.
144-148: Incorrect — case-insensitive handling already implementedvalidate_mobile_money_provider! downcases the provider before calling validate_allowed_values! (lib/paystack_sdk/resources/charges.rb:85–90) and specs cover mixed-case "MTN", so adding a case_insensitive option is unnecessary.
Likely an incorrect or invalid review comment.
lib/paystack_sdk/resources/customers.rb (1)
71-74: Inline date validations are concise and clear.bin/console (1)
4-5: LGTM — console requires look good.
Single quotes and require order are consistent; no functional concerns.spec/resources/transactions_spec.rb (1)
11-11: LGTM — numeric literal separators only.
Underscore formatting improves readability without changing values.Also applies to: 136-136, 144-144, 157-157, 196-196
lib/paystack_sdk/client.rb (2)
9-9: LGTM — charges resource required.
Import placement matches existing pattern.
126-138: LGTM — public charges accessor mirrors other resources.
Docs and memoization are consistent.spec/resources/charges_spec.rb (5)
20-32: LGTM — happy-path mobile money initiation covered.
Good verification of POST target and response wrapping.
46-51: LGTM — invalid provider path exercised.
Regex expectation is clear.
53-58: LGTM — missing phone validation covered.
Good negative coverage.
61-74: LGTM — OTP submission happy-path covered.
Covers endpoint and response wrapping.
76-81: LGTM — missing OTP validation covered.
Consistent with validations API.
| it "accepts provider codes in different cases" do | ||
| payload[:mobile_money][:provider] = "MTN" | ||
| response_double = double("Response", success?: true) | ||
| expect(connection).to receive(:post) | ||
| .with("/charge", payload) | ||
| .and_return(response_double) | ||
| expect(PaystackSdk::Response).to receive(:new).with(response_double) | ||
| .and_return(response_double) | ||
|
|
||
| expect { charges.mobile_money(payload) }.not_to raise_error | ||
| end |
There was a problem hiding this comment.
Normalize provider before POST; adjust expectation accordingly.
Validation accepts case-insensitive providers, but the outgoing payload currently preserves caller case. Paystack typically expects lowercase codes; normalize to lowercase before posting and update the spec to assert the normalized payload.
- it "accepts provider codes in different cases" do
- payload[:mobile_money][:provider] = "MTN"
- response_double = double("Response", success?: true)
- expect(connection).to receive(:post)
- .with("/charge", payload)
- .and_return(response_double)
- expect(PaystackSdk::Response).to receive(:new).with(response_double)
- .and_return(response_double)
-
- expect { charges.mobile_money(payload) }.not_to raise_error
- end
+ it "accepts provider codes in different cases and normalizes before POST" do
+ payload[:mobile_money][:provider] = "MTN"
+ normalized_payload = payload.merge(
+ mobile_money: payload[:mobile_money].merge(provider: "mtn")
+ )
+ response_double = double("Response", success?: true)
+ expect(connection).to receive(:post)
+ .with("/charge", normalized_payload)
+ .and_return(response_double)
+ expect(PaystackSdk::Response).to receive(:new).with(response_double)
+ .and_return(response_double)
+
+ expect { charges.mobile_money(payload) }.not_to raise_error
+ end📝 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.
| it "accepts provider codes in different cases" do | |
| payload[:mobile_money][:provider] = "MTN" | |
| response_double = double("Response", success?: true) | |
| expect(connection).to receive(:post) | |
| .with("/charge", payload) | |
| .and_return(response_double) | |
| expect(PaystackSdk::Response).to receive(:new).with(response_double) | |
| .and_return(response_double) | |
| expect { charges.mobile_money(payload) }.not_to raise_error | |
| end | |
| it "accepts provider codes in different cases and normalizes before POST" do | |
| payload[:mobile_money][:provider] = "MTN" | |
| normalized_payload = payload.merge( | |
| mobile_money: payload[:mobile_money].merge(provider: "mtn") | |
| ) | |
| response_double = double("Response", success?: true) | |
| expect(connection).to receive(:post) | |
| .with("/charge", normalized_payload) | |
| .and_return(response_double) | |
| expect(PaystackSdk::Response).to receive(:new).with(response_double) | |
| .and_return(response_double) | |
| expect { charges.mobile_money(payload) }.not_to raise_error | |
| end |
🤖 Prompt for AI Agents
In spec/resources/charges_spec.rb around lines 34–44, the test expects the
outgoing payload to preserve caller case but the code should normalize
mobile_money.provider to lowercase before the POST; update the implementation to
downcase the provider (e.g., use a safe non-destructive copy or set
payload_for_post = payload.deep_dup and
payload_for_post[:mobile_money][:provider] =
payload_for_post[:mobile_money][:provider].to_s.downcase) and change the test
expectation to expect the POST with the payload whose mobile_money.provider is
downcased, keeping the rest of the assertions the same.
2 participants
Summary
/charge) and OTP submission (/charge/submit_otp) for provider voucher flows.Details
New Resource
lib/paystack_sdk/resources/charges.rbmobile_money(payload): posts to/chargewithemail,amount,currency, andmobile_money(phone,provider).submit_otp(payload): posts to/charge/submit_otpwithreferenceandotp.Validations
email,amount,mobile_money.currency,reference.mobile_moneyhash requiresphoneand a supportedprovider.mtn,atl,vod,mpesa,orange,wave(case-insensitive).Client Wiring
client.chargesaccessor inlib/paystack_sdk/client.rb.Specs
spec/resources/charges_spec.rb→ initiation, provider validation, OTP.spec/client_spec.rb→ updated to coverclient.charges.Usage
Initiate
OTP (e.g., Vodafone voucher)
Verify Final Status
Tests
bundle exec rspecbundle exec rspec spec/resources/charges_spec.rbNon-goals
submit_pin,submit_phone, etc.) not included here; can be added if needed.Notes
Refactor
Summary by CodeRabbit
New Features
Improvements
Tests