test: PeachPaymentsGateway unit & integration tests (#50)#55
Merged
Conversation
- Add PeachPayments = 15 to PaymentGatewayType enum - Add PeachPaymentsConfig (EntityId, AccessToken, IsSandbox) - Implement PeachPaymentsGateway: checkout initiation, payment verification, refund - Register gateway in IServiceCollectionExtensions and PaymentGatewayFactory - Add ZAR/KES/NGN/BWP routing to PeachPayments in PaymentService - Add PEACH_ prefix detection for gateway resolution
… helpers (#51) - Add MockHttpMessageHandler: queue-based fake HTTP handler with assertion helpers (AssertRequestCount, AssertLastRequestPath, AssertLastMethod). Supports multi-step gateways via sequential queuing. - Add PaymentRequestFactory: builds valid PaymentRequest and RefundRequest with sensible defaults; supports configure lambda. - Add GatewayConfigFactory: one builder per gateway, sets only the minimum keys required to pass constructor validation. - Add IntegrationTestBase: abstract base that checks env vars and exposes SkipIfMissingEnvVars() for clean CI output. - Add paybridge.runsettings: supports dotnet test --filter Category=Unit and --filter Category=Integration with code coverage config. - Add TestInfrastructureTests: 20 unit tests covering all helpers. Closes #51
- ci-tests.yml: runs on every push/PR to master and feature/* branches - Filters to Category=Unit only (no network, no secrets needed) - Publishes TRX results via dorny/test-reporter as PR comment - Uploads coverage.cobertura.xml artifact - integration-tests.yml: manual trigger (workflow_dispatch) + nightly cron - Runs Category=Integration tests with all 15 gateway env vars injected - Uses a protected 'integration' GitHub environment for secret scoping - fail-on-error=false so missing sandbox keys skip rather than fail
- Remove double-dash from XML comments in paybridge.runsettings (invalid XML: 'An XML comment cannot contain --') - Remove --settings flag from both workflows (runsettings only needed for local coverage; was causing the runner to reject it) - Rename workflow title to 'CI - Unit Tests' (dash safe in YAML) - Add fail-on-empty: false to dorny/test-reporter so it does not error when the TRX file is absent due to an upstream failure
GitHub denies test-reporter from creating check runs on PRs unless the workflow explicitly grants 'checks: write' and 'pull-requests: write'. Added permissions block to both ci-tests.yml and integration-tests.yml.
GatewayExtractorTests (15 tests): - DetectGatewayFromWebhook: Paystack (event key), Flutterwave (flw_ref), Stripe (ExpandoObject with stripe. prefix), Checkout (_links), Korapay (KR_ prefix), unknown payload, non-dictionary input, null input, unrecognised reference prefix, priority ordering - ExtractReferenceFromWebhook: Korapay (ExpandoObject dynamic), null for unknown payload, null for bad input type, fallback common property names (4 cases via Theory) PaymentGatewayFactoryTests (14 tests): - Constructor: null serviceProvider, null config, null logger - CreateGateways (empty EnabledGateways): single gateway returned, skips unregistered DI gateway, empty dict when nothing registered, multiple gateways when multiple registered - CreateGateways (explicit EnabledGateways): only enabled gateways returned, Automatic enum value ignored, no throw when constructor throws - GatewayType correctness: correct enum value, implements IPaymentGateway Note: Stripe and Korapay tests use ExpandoObject (not Dictionary) because the source implementation uses dynamic property access (data.type) which only works when the object satisfies both IDictionary<string,object> cast and dynamic member resolution — ExpandoObject satisfies both. Closes #52
Unit tests (27 tests) — PeachPaymentsGatewayTests.cs: - Constructor: null config, null logger, valid config, GatewayType - CreatePaymentAsync: success path, posts to /checkouts endpoint, sets Bearer auth header, sends entityId in form body, returns failure on non-2xx, returns failure on non-000 result code, currency-to-country mapping (ZAR/KES/NGN/BWP/USD via Theory), amount formatted to 2dp, sandbox URL used, live URL used - VerifyPaymentAsync: success on 000.0xx code, success on 000.100.xxx, failure on non-matching code, failure on non-2xx API response, GET request contains entityId and merchantTransactionId params - RefundPaymentAsync: success path with refund reference, failure on non-000 code, POST to correct endpoint, amount formatted to 2dp Helper added: - MockHttpClientFactory.cs — IHttpClientFactory stub backed by MockHttpMessageHandler for gateway constructor injection Integration tests (2 tests) — PeachPaymentsIntegrationTests.cs: - CreatePaymentAsync returns sandbox checkout URL (PEACH_ prefix) - VerifyPaymentAsync returns a response for a freshly created ref - Both auto-skip when PEACH_ENTITY_ID / PEACH_ACCESS_TOKEN env vars absent Closes #50
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces Peach Payments support into the SDK (gateway implementation + DI/selection wiring) and adds a comprehensive .NET 8 test/CI setup to validate gateway behavior via unit tests (mocked HTTP) and optional integration tests (real sandbox).
Changes:
- Added
PeachPaymentsGatewayplus config + enum updates, and wired it into DI, factory creation, and reference-based gateway detection. - Expanded currency routing in
PaymentServiceand introduced BWP routing. - Added test infrastructure (mock HTTP, request/config factories), Peach Payments unit/integration tests, and GitHub Actions workflows for unit/integration test execution.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| PayBridge.SDK/Services/PaymentService.cs | Updates currency routing and reference-prefix gateway detection to include Peach Payments. |
| PayBridge.SDK/Gateways/PeachPaymentsGateway.cs | Adds Peach Payments gateway implementation (create/verify/refund). |
| PayBridge.SDK/Factories/PaymentGatewayFactory.cs | Adds Peach Payments to factory initialization and DI resolution. |
| PayBridge.SDK/Externsions/IServiceCollectionExtensions.cs | Registers PeachPaymentsGateway in DI. |
| PayBridge.SDK/Enums/PaymentGatewayType.cs | Adds PeachPayments = 15 enum member. |
| PayBridge.SDK/Dtos/PaymentGatewayConfig.cs | Adds PeachPaymentsConfig to configuration object model. |
| PayBridge.SDK.Test/Unit/TestInfrastructureTests.cs | Validates shared test helpers (mock handler, factories, integration base). |
| PayBridge.SDK.Test/Unit/PeachPaymentsGatewayTests.cs | Unit tests for PeachPaymentsGateway using mocked HTTP. |
| PayBridge.SDK.Test/Unit/PaymentGatewayFactoryTests.cs | Unit tests for DI wiring/factory gateway creation paths. |
| PayBridge.SDK.Test/Unit/GatewayExtractorTests.cs | Unit tests for webhook gateway detection/reference extraction helper. |
| PayBridge.SDK.Test/PayBridge.SDK.Test.csproj | Adds test dependencies and references SDK project. |
| PayBridge.SDK.Test/paybridge.runsettings | Adds runsettings for filtering unit vs integration and coverage scoping. |
| PayBridge.SDK.Test/Integration/PeachPaymentsIntegrationTests.cs | Integration tests against Peach sandbox with env-var-based auto-skip. |
| PayBridge.SDK.Test/Helpers/PaymentRequestFactory.cs | Adds request/config factories to reduce duplication across tests. |
| PayBridge.SDK.Test/Helpers/MockHttpMessageHandler.cs | Adds queued-response HttpMessageHandler for deterministic HTTP mocking. |
| PayBridge.SDK.Test/Helpers/MockHttpClientFactory.cs | Adds IHttpClientFactory stub for gateways in unit tests. |
| PayBridge.SDK.Test/Helpers/IntegrationTestBase.cs | Adds shared integration-test skip/env-var utilities. |
| .github/workflows/integration-tests.yml | Adds scheduled/manual integration test workflow with secrets-based env. |
| .github/workflows/ci-tests.yml | Adds CI workflow to run unit tests + collect coverage on PRs/pushes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
261
to
+265
| case "CDF": | ||
| case "XOF": | ||
| case "XAF": | ||
| case "MWK": | ||
| return ChooseAvailableGateway(PaymentGatewayType.PawaPay, PaymentGatewayType.DpoGroup, PaymentGatewayType.Flutterwave, PaymentGatewayType.Paystack); | ||
| return ChooseAvailableGateway(PaymentGatewayType.PeachPayments, PaymentGatewayType.PawaPay, PaymentGatewayType.DpoGroup, PaymentGatewayType.Flutterwave, PaymentGatewayType.Paystack); |
Comment on lines
+265
to
+268
| return ChooseAvailableGateway(PaymentGatewayType.PeachPayments, PaymentGatewayType.PawaPay, PaymentGatewayType.DpoGroup, PaymentGatewayType.Flutterwave, PaymentGatewayType.Paystack); | ||
|
|
||
| case "BWP": | ||
| return ChooseAvailableGateway(PaymentGatewayType.PeachPayments, PaymentGatewayType.DpoGroup); |
| [Trait("Category", "Integration")] | ||
| public class PeachPaymentsIntegrationTests : IntegrationTestBase | ||
| { | ||
| private readonly PeachPaymentsGateway _gateway; |
Comment on lines
+143
to
+145
| var json = JsonDocument.Parse(body).RootElement; | ||
| var payment = json.ValueKind == JsonValueKind.Array ? json[0] : json; | ||
|
|
| PawaPay = 14, | ||
|
|
||
| /// <summary> | ||
| /// Peach Payments gateway (South Africa, Kenya, Nigeria) |
Comment on lines
+1
to
+9
| using FluentAssertions; | ||
| using Microsoft.Extensions.DependencyInjection; | ||
| using Microsoft.Extensions.Logging; | ||
| using Moq; | ||
| using PayBridge.SDK.Application.Dtos; | ||
| using PayBridge.SDK.Enums; | ||
| using PayBridge.SDK.Interfaces; | ||
| using PayBridge.SDK.Test.Helpers; | ||
| using Xunit; |
Comment on lines
+1
to
+8
| using System.Net; | ||
| using FluentAssertions; | ||
| using Microsoft.Extensions.Logging.Abstractions; | ||
| using Moq; | ||
| using PayBridge.SDK.Application.Dtos.Request; | ||
| using PayBridge.SDK.Enums; | ||
| using PayBridge.SDK.Test.Helpers; | ||
| using Xunit; |
Comment on lines
+1
to
+7
| using FluentAssertions; | ||
| using Microsoft.Extensions.Logging.Abstractions; | ||
| using PayBridge.SDK.Application.Dtos; | ||
| using PayBridge.SDK.Enums; | ||
| using PayBridge.SDK.Test.Helpers; | ||
| using Xunit; | ||
|
|
📝 WalkthroughWalkthroughAdds PeachPayments gateway implementation and config, updates DI/factory/service routing to include PeachPayments, introduces test helpers and factories, extensive unit tests and env-gated integration tests for Peach sandbox, adjusts DTO/entity string initializers, and adds GitHub Actions workflows for unit and integration test runs. ChangesPeach Payments Gateway Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 7
🧹 Nitpick comments (5)
PayBridge.SDK.Test/Unit/PaymentGatewayFactoryTests.cs (1)
116-266: ⚡ Quick winAdd explicit PeachPayments coverage for the newly added factory branch.
The suite doesn’t currently lock the
PaymentGatewayType.PeachPaymentspath inCreateGateways()(both enable-all and enabled-list scenarios). A small targeted test here would prevent regressions on the exact branch added in this PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PayBridge.SDK.Test/Unit/PaymentGatewayFactoryTests.cs` around lines 116 - 266, Add tests that exercise the PaymentGatewayType.PeachPayments branch in PaymentGatewayFactory.CreateGateways: create one test that registers PeachPayments in the service provider (use BuildServiceProviderWith with PaymentGatewayType.PeachPayments and provide a valid PeachPayments config on PaymentGatewayConfig/GatewayConfigFactory) and assert the returned dictionary contains PaymentGatewayType.PeachPayments and the gateway implements IPaymentGateway; add another test for the EnabledGateways scenario (set EnabledGateways to include PaymentGatewayType.PeachPayments while also registering it) and assert it is present, and optionally a test ensuring PeachPayments is skipped when not registered in DI; place them alongside the existing CreateGateways_* tests so they cover the new branch.PayBridge.SDK.Test/Integration/PeachPaymentsIntegrationTests.cs (1)
85-86: ⚡ Quick winTighten verification status assertion to match gateway contract.
Line 85 currently accepts
Pending, butVerifyPaymentAsyncmaps toSuccessful/Failedonly. KeepingPendinghere weakens regression detection.Suggested test tightening
- result.Status.Should().BeOneOf( - PaymentStatus.Pending, PaymentStatus.Successful, PaymentStatus.Failed); + result.Status.Should().BeOneOf( + PaymentStatus.Successful, PaymentStatus.Failed); + result.Success.Should().Be(result.Status == PaymentStatus.Successful);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PayBridge.SDK.Test/Integration/PeachPaymentsIntegrationTests.cs` around lines 85 - 86, The test assertion currently allows PaymentStatus.Pending but VerifyPaymentAsync only returns PaymentStatus.Successful or PaymentStatus.Failed; tighten the assertion in PeachPaymentsIntegrationTests (the check on result.Status) to only accept Successful or Failed so the test reflects the gateway contract enforced by VerifyPaymentAsync and will fail if a Pending status is returned unexpectedly..github/workflows/integration-tests.yml (1)
6-10: ⚡ Quick winRemove or implement the
gatewaydispatch input.Lines 7-10 define
gateway, but it is never used to filter/select tests. This is misleading for manual runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/integration-tests.yml around lines 6 - 10, The workflow exposes an unused dispatch input named "gateway"; either remove this input or wire it into the job that runs tests so manual runs actually filter by gateway. If you choose to implement it, propagate github.event.inputs.gateway into the job (e.g., set an env GATEWAY or use it to build the strategy.matrix) and update the test step (the script or command that runs integration tests) to read that env/variable and filter tests accordingly; otherwise delete the "gateway" input block to avoid misleading callers.PayBridge.SDK.Test/Unit/PeachPaymentsGatewayTests.cs (1)
246-287: ⚡ Quick winAdd regression tests for malformed verify payloads and non-2xx refund responses.
Please add unit cases for: (1)
VerifyPaymentAsyncreceiving[], and (2)RefundPaymentAsyncreceiving non-success HTTP status. These paths currently have no direct test lock.Also applies to: 330-342
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PayBridge.SDK.Test/Unit/PeachPaymentsGatewayTests.cs` around lines 246 - 287, Add two regression unit tests: one that uses MockHttpMessageHandler to respond with HttpStatusCode.OK and an empty JSON array body "[]" when calling VerifyPaymentAsync (via BuildGateway) and assert the returned result indicates failure (result.Success false and result.Status PaymentStatus.Failed) and that GatewayResponse is handled safely; and another that sets MockHttpMessageHandler to return a non-2xx status (e.g., HttpStatusCode.InternalServerError) for RefundPaymentAsync and assert the refund call returns a failed result (Success false, Status PaymentStatus.Failed) and does not throw. Use the existing test patterns (BuildGateway, MockHttpMessageHandler) and mirror assertions from the other VerifyPaymentAsync/Refund tests.PayBridge.SDK.Test/Helpers/IntegrationTestBase.cs (1)
21-21: 💤 Low valueConsider removing the unused
_requiredVarsfield.The
_requiredVarsfield is assigned in the constructor but never read. The constructor uses therequiredEnvVarsparameter directly on lines 40-42, making the field storage unnecessary.♻️ Proposed fix to remove the unused field
public abstract class IntegrationTestBase { - private readonly string[] _requiredVars; - /// <summary> /// The reason string reported by xUnit when a test is skipped. /// Set in the constructor if any env var is missing. /// </summary> protected string? SkipReason { get; } /// <summary> /// Whether all required env vars are present. /// Use this in test bodies to conditionally Skip: /// <code>Skip.If(ShouldSkip, SkipReason);</code> /// </summary> protected bool ShouldSkip => SkipReason != null; protected IntegrationTestBase(params string[] requiredEnvVars) { - _requiredVars = requiredEnvVars; - var missing = requiredEnvVarsAlso applies to: 38-38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PayBridge.SDK.Test/Helpers/IntegrationTestBase.cs` at line 21, Remove the unused private field _requiredVars from the IntegrationTestBase class and any assignments to it in the constructor; instead continue using the constructor parameter requiredEnvVars directly (or store it in a property if persistence is required). Update the constructor in IntegrationTestBase to delete the line that assigns to _requiredVars and remove the field declaration to eliminate the dead storage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci-tests.yml:
- Line 25: Pin all GitHub Action `uses:` refs to immutable commit SHAs instead
of tags for the listed workflows and add the checkout credential hardening:
replace tag references like "actions/checkout@v4", "actions/setup-dotnet@v4",
"actions/upload-artifact@v4", and "dorny/test-reporter@v1" with their
corresponding commit SHA pins, and update the actions/checkout steps (the
checkout invocation identified by "actions/checkout@v4") to include a `with:
persist-credentials: false` entry to disable checkout credentials unless
explicitly required; ensure both ci-tests and integration-tests workflows are
updated similarly.
- Around line 24-26: Add explicit disabling of persisted checkout credentials to
each actions/checkout@v4 step: update the checkout step(s) that use "uses:
actions/checkout@v4" to include a "with:" block containing "persist-credentials:
false" (properly indented under the checkout step) in both CI workflow
definitions so the job does not persist the auth token to git config.
In `@PayBridge.SDK/Externsions/IServiceCollectionExtensions.cs`:
- Around line 142-143: The concrete PeachPaymentsGateway is only registered as
its class, so PaymentService (which resolves IEnumerable<IPaymentGateway>) won’t
see it; update the registrations in IServiceCollectionExtensions to register the
implementation against the interface (register IPaymentGateway ->
PeachPaymentsGateway) instead of only the concrete type for both occurrences
where services.AddScoped<PeachPaymentsGateway>() is used so PeachPaymentsGateway
is discoverable by PaymentService and other consumers expecting IPaymentGateway.
In `@PayBridge.SDK/Gateways/PeachPaymentsGateway.cs`:
- Around line 30-38: The constructor for PeachPaymentsGateway must validate its
dependencies and Peach credentials up-front: ensure httpClientFactory and logger
are not null (throw ArgumentNullException for httpClientFactory and logger),
ensure _config and _config.PeachPayments are not null (throw
ArgumentNullException for PeachPayments), and validate PeachPayments.EntityId
and PeachPayments.AccessToken are non-empty (throw ArgumentException if
null/empty); only after these checks create _httpClient via
httpClientFactory.CreateClient(nameof(PeachPaymentsGateway)) and assign _logger
and _config — update the PeachPaymentsGateway constructor to perform these
deterministic startup validations so null/misconfiguration fail fast.
- Around line 202-214: The refund handling in PeachPaymentsGateway currently
parses the response body even for non-success HTTP responses; update the method
(the refund logic around the call to _httpClient.PostAsync and subsequent
JsonDocument.Parse) to first check response.IsSuccessStatusCode and handle
non-2xx responses before parsing JSON: read the body, log an error with status
code and body, and return a failed RefundResponse (or populate failure fields)
instead of attempting JsonDocument.Parse on error payloads; only proceed to
parse JSON and determine resultCode/refundId when response.IsSuccessStatusCode
is true.
- Around line 91-106: The code currently sets success based only on resultCode,
which can mark Success=true and Status=Pending even when checkoutId is null;
update the logic so Success (and Pending status) requires a non-null checkoutId
as well: compute checkoutId first (from json.TryGetProperty("id", out var
idEl)), then set the success boolean to require resultCode.StartsWith("000") AND
checkoutId != null, and ensure PaymentResponse.Success, Status and CheckoutUrl
reflect that (i.e., if checkoutId is missing, set Success=false and
Status=Failed and leave CheckoutUrl empty). Make these changes around the
variables checkoutId, success, paymentUrl and the returned PaymentResponse.
- Around line 143-145: The current parsing sets payment = json[0] when
json.ValueKind == JsonValueKind.Array which throws on an empty array; update the
block around JsonDocument.Parse(body).RootElement and the payment variable so
that if json.ValueKind == JsonValueKind.Array and json.GetArrayLength() == 0 you
short-circuit to a failed verification response (do not throw), otherwise index
the first element as before; ensure downstream logic that uses payment handles
the failed-path consistently (return the verification failure result rather than
letting an exception propagate).
---
Nitpick comments:
In @.github/workflows/integration-tests.yml:
- Around line 6-10: The workflow exposes an unused dispatch input named
"gateway"; either remove this input or wire it into the job that runs tests so
manual runs actually filter by gateway. If you choose to implement it, propagate
github.event.inputs.gateway into the job (e.g., set an env GATEWAY or use it to
build the strategy.matrix) and update the test step (the script or command that
runs integration tests) to read that env/variable and filter tests accordingly;
otherwise delete the "gateway" input block to avoid misleading callers.
In `@PayBridge.SDK.Test/Helpers/IntegrationTestBase.cs`:
- Line 21: Remove the unused private field _requiredVars from the
IntegrationTestBase class and any assignments to it in the constructor; instead
continue using the constructor parameter requiredEnvVars directly (or store it
in a property if persistence is required). Update the constructor in
IntegrationTestBase to delete the line that assigns to _requiredVars and remove
the field declaration to eliminate the dead storage.
In `@PayBridge.SDK.Test/Integration/PeachPaymentsIntegrationTests.cs`:
- Around line 85-86: The test assertion currently allows PaymentStatus.Pending
but VerifyPaymentAsync only returns PaymentStatus.Successful or
PaymentStatus.Failed; tighten the assertion in PeachPaymentsIntegrationTests
(the check on result.Status) to only accept Successful or Failed so the test
reflects the gateway contract enforced by VerifyPaymentAsync and will fail if a
Pending status is returned unexpectedly.
In `@PayBridge.SDK.Test/Unit/PaymentGatewayFactoryTests.cs`:
- Around line 116-266: Add tests that exercise the
PaymentGatewayType.PeachPayments branch in PaymentGatewayFactory.CreateGateways:
create one test that registers PeachPayments in the service provider (use
BuildServiceProviderWith with PaymentGatewayType.PeachPayments and provide a
valid PeachPayments config on PaymentGatewayConfig/GatewayConfigFactory) and
assert the returned dictionary contains PaymentGatewayType.PeachPayments and the
gateway implements IPaymentGateway; add another test for the EnabledGateways
scenario (set EnabledGateways to include PaymentGatewayType.PeachPayments while
also registering it) and assert it is present, and optionally a test ensuring
PeachPayments is skipped when not registered in DI; place them alongside the
existing CreateGateways_* tests so they cover the new branch.
In `@PayBridge.SDK.Test/Unit/PeachPaymentsGatewayTests.cs`:
- Around line 246-287: Add two regression unit tests: one that uses
MockHttpMessageHandler to respond with HttpStatusCode.OK and an empty JSON array
body "[]" when calling VerifyPaymentAsync (via BuildGateway) and assert the
returned result indicates failure (result.Success false and result.Status
PaymentStatus.Failed) and that GatewayResponse is handled safely; and another
that sets MockHttpMessageHandler to return a non-2xx status (e.g.,
HttpStatusCode.InternalServerError) for RefundPaymentAsync and assert the refund
call returns a failed result (Success false, Status PaymentStatus.Failed) and
does not throw. Use the existing test patterns (BuildGateway,
MockHttpMessageHandler) and mirror assertions from the other
VerifyPaymentAsync/Refund tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f3af945-8fb8-48e3-9c6c-d349fc3655c7
📒 Files selected for processing (19)
.github/workflows/ci-tests.yml.github/workflows/integration-tests.ymlPayBridge.SDK.Test/Helpers/IntegrationTestBase.csPayBridge.SDK.Test/Helpers/MockHttpClientFactory.csPayBridge.SDK.Test/Helpers/MockHttpMessageHandler.csPayBridge.SDK.Test/Helpers/PaymentRequestFactory.csPayBridge.SDK.Test/Integration/PeachPaymentsIntegrationTests.csPayBridge.SDK.Test/PayBridge.SDK.Test.csprojPayBridge.SDK.Test/Unit/GatewayExtractorTests.csPayBridge.SDK.Test/Unit/PaymentGatewayFactoryTests.csPayBridge.SDK.Test/Unit/PeachPaymentsGatewayTests.csPayBridge.SDK.Test/Unit/TestInfrastructureTests.csPayBridge.SDK.Test/paybridge.runsettingsPayBridge.SDK/Dtos/PaymentGatewayConfig.csPayBridge.SDK/Enums/PaymentGatewayType.csPayBridge.SDK/Externsions/IServiceCollectionExtensions.csPayBridge.SDK/Factories/PaymentGatewayFactory.csPayBridge.SDK/Gateways/PeachPaymentsGateway.csPayBridge.SDK/Services/PaymentService.cs
Comment on lines
+24
to
+26
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
|
|
There was a problem hiding this comment.
Secure token handling in CI: disable persisted checkout credentials
Both .github/workflows/ci-tests.yml and .github/workflows/integration-tests.yml use actions/checkout@v4 without with: persist-credentials: false (default persists the auth token in git config during the job). Add with: persist-credentials: false to each actions/checkout@v4 step.
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 24-25: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 25-25: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci-tests.yml around lines 24 - 26, Add explicit disabling
of persisted checkout credentials to each actions/checkout@v4 step: update the
checkout step(s) that use "uses: actions/checkout@v4" to include a "with:" block
containing "persist-credentials: false" (properly indented under the checkout
step) in both CI workflow definitions so the job does not persist the auth token
to git config.
|
|
||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the two workflow files mentioned
ls -la .github/workflows || true
for f in .github/workflows/ci-tests.yml .github/workflows/integration-tests.yml; do
echo "---- $f ----"
if [ -f "$f" ]; then
sed -n '1,200p' "$f" | nl -ba | sed -n '1,200p'
else
echo "MISSING: $f"
fi
done
# Extract all `uses:` entries from both files (with surrounding context)
for f in .github/workflows/ci-tests.yml .github/workflows/integration-tests.yml; do
if [ -f "$f" ]; then
echo "==== uses entries in $f ===="
rg -n --context 2 '^\s*uses:\s*' "$f" || true
echo "==== checkout step in $f (if any) ===="
rg -n --context 6 'actions/checkout@' "$f" || true
fi
doneRepository: teesofttech/PayBridge
Length of output: 479
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in .github/workflows/ci-tests.yml .github/workflows/integration-tests.yml; do
echo "---- $f (first 220 lines with line numbers) ----"
if [ -f "$f" ]; then
awk '{printf "%d\t%s\n", NR, $0}' "$f" | sed -n '1,220p'
else
echo "MISSING: $f"
fi
done
for f in .github/workflows/ci-tests.yml .github/workflows/integration-tests.yml; do
if [ -f "$f" ]; then
echo "==== uses entries in $f (context) ===="
rg -n --context 2 '^\s*uses:\s*' "$f" || true
echo "==== checkout references in $f (context) ===="
rg -n --context 6 'actions/checkout@' "$f" || true
fi
doneRepository: teesofttech/PayBridge
Length of output: 8753
Major security hardening: pin GitHub Actions uses: refs (and disable checkout credentials) in CI workflows.
.github/workflows/ci-tests.ymland.github/workflows/integration-tests.ymluse tag-baseduses:refs (actions/checkout@v4,actions/setup-dotnet@v4,actions/upload-artifact@v4,dorny/test-reporter@v1); pin each to immutable commit SHAs.- Both
actions/checkoutsteps (ci-tests: line 25; integration-tests: line 29) lackwith: persist-credentials: false; add it unless explicitly required.
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 24-25: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 25-25: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci-tests.yml at line 25, Pin all GitHub Action `uses:`
refs to immutable commit SHAs instead of tags for the listed workflows and add
the checkout credential hardening: replace tag references like
"actions/checkout@v4", "actions/setup-dotnet@v4", "actions/upload-artifact@v4",
and "dorny/test-reporter@v1" with their corresponding commit SHA pins, and
update the actions/checkout steps (the checkout invocation identified by
"actions/checkout@v4") to include a `with: persist-credentials: false` entry to
disable checkout credentials unless explicitly required; ensure both ci-tests
and integration-tests workflows are updated similarly.
Source: Linters/SAST tools
Comment on lines
+142
to
143
| services.AddScoped<PeachPaymentsGateway>(); | ||
| return; |
There was a problem hiding this comment.
Register PeachPayments as IPaymentGateway, not only as concrete type.
Line 142 and Line 195 register only PeachPaymentsGateway. PaymentService builds its runtime map from IEnumerable<IPaymentGateway> (PayBridge.SDK/Services/PaymentService.cs, Line 24 and Line 32), so Peach won’t be discoverable there and will fail as “not configured”.
💡 Suggested fix
- services.AddScoped<PeachPaymentsGateway>();
+ AddGatewayRegistration<PeachPaymentsGateway>(services);
return;
...
case PaymentGatewayType.PeachPayments:
- services.AddScoped<PeachPaymentsGateway>();
+ AddGatewayRegistration<PeachPaymentsGateway>(services);
break;Also applies to: 194-196
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PayBridge.SDK/Externsions/IServiceCollectionExtensions.cs` around lines 142 -
143, The concrete PeachPaymentsGateway is only registered as its class, so
PaymentService (which resolves IEnumerable<IPaymentGateway>) won’t see it;
update the registrations in IServiceCollectionExtensions to register the
implementation against the interface (register IPaymentGateway ->
PeachPaymentsGateway) instead of only the concrete type for both occurrences
where services.AddScoped<PeachPaymentsGateway>() is used so PeachPaymentsGateway
is discoverable by PaymentService and other consumers expecting IPaymentGateway.
Comment on lines
+30
to
+38
| public PeachPaymentsGateway( | ||
| PaymentGatewayConfig config, | ||
| IHttpClientFactory httpClientFactory, | ||
| ILogger<PeachPaymentsGateway> logger) | ||
| { | ||
| _config = config ?? throw new ArgumentNullException(nameof(config)); | ||
| _httpClient = httpClientFactory.CreateClient(nameof(PeachPaymentsGateway)); | ||
| _logger = logger ?? throw new ArgumentNullException(nameof(logger)); | ||
| } |
There was a problem hiding this comment.
Validate constructor dependencies and Peach credentials upfront.
httpClientFactory, _config.PeachPayments, EntityId, and AccessToken are used unguarded. Misconfiguration currently fails later with null-driven runtime errors instead of deterministic startup-time validation.
Suggested fix
public PeachPaymentsGateway(
PaymentGatewayConfig config,
IHttpClientFactory httpClientFactory,
ILogger<PeachPaymentsGateway> logger)
{
- _config = config ?? throw new ArgumentNullException(nameof(config));
- _httpClient = httpClientFactory.CreateClient(nameof(PeachPaymentsGateway));
+ _config = config ?? throw new ArgumentNullException(nameof(config));
+ if (httpClientFactory is null) throw new ArgumentNullException(nameof(httpClientFactory));
_logger = logger ?? throw new ArgumentNullException(nameof(logger));
+ if (_config.PeachPayments is null)
+ throw new ArgumentException("PeachPayments config is required.", nameof(config));
+ if (string.IsNullOrWhiteSpace(_config.PeachPayments.EntityId))
+ throw new ArgumentException("PeachPayments EntityId is required.", nameof(config));
+ if (string.IsNullOrWhiteSpace(_config.PeachPayments.AccessToken))
+ throw new ArgumentException("PeachPayments AccessToken is required.", nameof(config));
+
+ _httpClient = httpClientFactory.CreateClient(nameof(PeachPaymentsGateway));
}Also applies to: 42-48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PayBridge.SDK/Gateways/PeachPaymentsGateway.cs` around lines 30 - 38, The
constructor for PeachPaymentsGateway must validate its dependencies and Peach
credentials up-front: ensure httpClientFactory and logger are not null (throw
ArgumentNullException for httpClientFactory and logger), ensure _config and
_config.PeachPayments are not null (throw ArgumentNullException for
PeachPayments), and validate PeachPayments.EntityId and
PeachPayments.AccessToken are non-empty (throw ArgumentException if null/empty);
only after these checks create _httpClient via
httpClientFactory.CreateClient(nameof(PeachPaymentsGateway)) and assign _logger
and _config — update the PeachPaymentsGateway constructor to perform these
deterministic startup validations so null/misconfiguration fail fast.
Comment on lines
+91
to
+106
| var checkoutId = json.TryGetProperty("id", out var idEl) ? idEl.GetString() : null; | ||
| bool success = resultCode != null && resultCode.StartsWith("000"); | ||
|
|
||
| var paymentUrl = checkoutId != null | ||
| ? $"{BaseUrl}/paymentWidgets.js?checkoutId={checkoutId}" | ||
| : null; | ||
|
|
||
| return new PaymentResponse | ||
| { | ||
| Success = success, | ||
| TransactionReference = txRef, | ||
| CheckoutUrl = paymentUrl ?? string.Empty, | ||
| Message = resultEl.TryGetProperty("description", out var descEl) | ||
| ? descEl.GetString() ?? "Unknown" : "Unknown", | ||
| Status = success ? PaymentStatus.Pending : PaymentStatus.Failed, | ||
| GatewayResponse = new Dictionary<string, string> |
There was a problem hiding this comment.
Do not return Success=true when checkoutId is missing.
A successful result code without id still returns Success = true and Pending, but checkout cannot proceed without a checkout id.
Suggested fix
- var checkoutId = json.TryGetProperty("id", out var idEl) ? idEl.GetString() : null;
- bool success = resultCode != null && resultCode.StartsWith("000");
+ var checkoutId = json.TryGetProperty("id", out var idEl) ? idEl.GetString() : null;
+ bool success = resultCode != null &&
+ resultCode.StartsWith("000") &&
+ !string.IsNullOrWhiteSpace(checkoutId);📝 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.
Suggested change
| var checkoutId = json.TryGetProperty("id", out var idEl) ? idEl.GetString() : null; | |
| bool success = resultCode != null && resultCode.StartsWith("000"); | |
| var paymentUrl = checkoutId != null | |
| ? $"{BaseUrl}/paymentWidgets.js?checkoutId={checkoutId}" | |
| : null; | |
| return new PaymentResponse | |
| { | |
| Success = success, | |
| TransactionReference = txRef, | |
| CheckoutUrl = paymentUrl ?? string.Empty, | |
| Message = resultEl.TryGetProperty("description", out var descEl) | |
| ? descEl.GetString() ?? "Unknown" : "Unknown", | |
| Status = success ? PaymentStatus.Pending : PaymentStatus.Failed, | |
| GatewayResponse = new Dictionary<string, string> | |
| var checkoutId = json.TryGetProperty("id", out var idEl) ? idEl.GetString() : null; | |
| bool success = resultCode != null && | |
| resultCode.StartsWith("000") && | |
| !string.IsNullOrWhiteSpace(checkoutId); | |
| var paymentUrl = checkoutId != null | |
| ? $"{BaseUrl}/paymentWidgets.js?checkoutId={checkoutId}" | |
| : null; | |
| return new PaymentResponse | |
| { | |
| Success = success, | |
| TransactionReference = txRef, | |
| CheckoutUrl = paymentUrl ?? string.Empty, | |
| Message = resultEl.TryGetProperty("description", out var descEl) | |
| ? descEl.GetString() ?? "Unknown" : "Unknown", | |
| Status = success ? PaymentStatus.Pending : PaymentStatus.Failed, | |
| GatewayResponse = new Dictionary<string, string> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PayBridge.SDK/Gateways/PeachPaymentsGateway.cs` around lines 91 - 106, The
code currently sets success based only on resultCode, which can mark
Success=true and Status=Pending even when checkoutId is null; update the logic
so Success (and Pending status) requires a non-null checkoutId as well: compute
checkoutId first (from json.TryGetProperty("id", out var idEl)), then set the
success boolean to require resultCode.StartsWith("000") AND checkoutId != null,
and ensure PaymentResponse.Success, Status and CheckoutUrl reflect that (i.e.,
if checkoutId is missing, set Success=false and Status=Failed and leave
CheckoutUrl empty). Make these changes around the variables checkoutId, success,
paymentUrl and the returned PaymentResponse.
Comment on lines
+143
to
+145
| var json = JsonDocument.Parse(body).RootElement; | ||
| var payment = json.ValueKind == JsonValueKind.Array ? json[0] : json; | ||
|
|
There was a problem hiding this comment.
Guard against empty-array verification responses.
When the API returns [], indexing json[0] throws. This should degrade to a failed verification response, not exception flow.
Suggested fix
var json = JsonDocument.Parse(body).RootElement;
- var payment = json.ValueKind == JsonValueKind.Array ? json[0] : json;
+ if (json.ValueKind == JsonValueKind.Array && json.GetArrayLength() == 0)
+ {
+ return new VerificationResponse
+ {
+ Success = false,
+ TransactionReference = transactionReference,
+ Status = PaymentStatus.Failed,
+ Message = "PeachPayments verification returned no payment records"
+ };
+ }
+ var payment = json.ValueKind == JsonValueKind.Array ? json[0] : json;📝 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.
Suggested change
| var json = JsonDocument.Parse(body).RootElement; | |
| var payment = json.ValueKind == JsonValueKind.Array ? json[0] : json; | |
| var json = JsonDocument.Parse(body).RootElement; | |
| if (json.ValueKind == JsonValueKind.Array && json.GetArrayLength() == 0) | |
| { | |
| return new VerificationResponse | |
| { | |
| Success = false, | |
| TransactionReference = transactionReference, | |
| Status = PaymentStatus.Failed, | |
| Message = "PeachPayments verification returned no payment records" | |
| }; | |
| } | |
| var payment = json.ValueKind == JsonValueKind.Array ? json[0] : json; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PayBridge.SDK/Gateways/PeachPaymentsGateway.cs` around lines 143 - 145, The
current parsing sets payment = json[0] when json.ValueKind ==
JsonValueKind.Array which throws on an empty array; update the block around
JsonDocument.Parse(body).RootElement and the payment variable so that if
json.ValueKind == JsonValueKind.Array and json.GetArrayLength() == 0 you
short-circuit to a failed verification response (do not throw), otherwise index
the first element as before; ensure downstream logic that uses payment handles
the failed-path consistently (return the verification failure result rather than
letting an exception propagate).
Comment on lines
+202
to
+214
| var response = await _httpClient.PostAsync($"{BaseUrl}/payments/{request.TransactionReference}", content); | ||
| var body = await response.Content.ReadAsStringAsync(); | ||
| _logger.LogDebug("PeachPayments refund response: {Body}", body); | ||
|
|
||
| var json = JsonDocument.Parse(body).RootElement; | ||
| var resultCode = json.TryGetProperty("result", out var resultEl) && | ||
| resultEl.TryGetProperty("code", out var codeEl) | ||
| ? codeEl.GetString() : null; | ||
|
|
||
| bool success = resultCode != null && resultCode.StartsWith("000"); | ||
| var refundId = json.TryGetProperty("id", out var idEl) ? idEl.GetString() ?? string.Empty : string.Empty; | ||
|
|
||
| return new RefundResponse |
There was a problem hiding this comment.
Handle non-success refund HTTP statuses before JSON parsing.
Refund currently parses and evaluates body even on non-2xx responses. This can misclassify failures or throw on non-JSON error payloads.
Suggested fix
var response = await _httpClient.PostAsync($"{BaseUrl}/payments/{request.TransactionReference}", content);
var body = await response.Content.ReadAsStringAsync();
_logger.LogDebug("PeachPayments refund response: {Body}", body);
+if (!response.IsSuccessStatusCode)
+{
+ return new RefundResponse
+ {
+ Success = false,
+ TransactionReference = request.TransactionReference,
+ Amount = request.Amount,
+ Status = PaymentStatus.Failed,
+ Message = $"PeachPayments refund failed: {response.StatusCode}"
+ };
+}
+
var json = JsonDocument.Parse(body).RootElement;📝 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.
Suggested change
| var response = await _httpClient.PostAsync($"{BaseUrl}/payments/{request.TransactionReference}", content); | |
| var body = await response.Content.ReadAsStringAsync(); | |
| _logger.LogDebug("PeachPayments refund response: {Body}", body); | |
| var json = JsonDocument.Parse(body).RootElement; | |
| var resultCode = json.TryGetProperty("result", out var resultEl) && | |
| resultEl.TryGetProperty("code", out var codeEl) | |
| ? codeEl.GetString() : null; | |
| bool success = resultCode != null && resultCode.StartsWith("000"); | |
| var refundId = json.TryGetProperty("id", out var idEl) ? idEl.GetString() ?? string.Empty : string.Empty; | |
| return new RefundResponse | |
| var response = await _httpClient.PostAsync($"{BaseUrl}/payments/{request.TransactionReference}", content); | |
| var body = await response.Content.ReadAsStringAsync(); | |
| _logger.LogDebug("PeachPayments refund response: {Body}", body); | |
| if (!response.IsSuccessStatusCode) | |
| { | |
| return new RefundResponse | |
| { | |
| Success = false, | |
| TransactionReference = request.TransactionReference, | |
| Amount = request.Amount, | |
| Status = PaymentStatus.Failed, | |
| Message = $"PeachPayments refund failed: {response.StatusCode}" | |
| }; | |
| } | |
| var json = JsonDocument.Parse(body).RootElement; | |
| var resultCode = json.TryGetProperty("result", out var resultEl) && | |
| resultEl.TryGetProperty("code", out var codeEl) | |
| ? codeEl.GetString() : null; | |
| bool success = resultCode != null && resultCode.StartsWith("000"); | |
| var refundId = json.TryGetProperty("id", out var idEl) ? idEl.GetString() ?? string.Empty : string.Empty; | |
| return new RefundResponse |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PayBridge.SDK/Gateways/PeachPaymentsGateway.cs` around lines 202 - 214, The
refund handling in PeachPaymentsGateway currently parses the response body even
for non-success HTTP responses; update the method (the refund logic around the
call to _httpClient.PostAsync and subsequent JsonDocument.Parse) to first check
response.IsSuccessStatusCode and handle non-2xx responses before parsing JSON:
read the body, log an error with status code and body, and return a failed
RefundResponse (or populate failure fields) instead of attempting
JsonDocument.Parse on error payloads; only proceed to parse JSON and determine
resultCode/refundId when response.IsSuccessStatusCode is true.
…CS8600/CS8601/CS8603/CS8604) - DTOs/Request: initialize all string properties to string.Empty (PaymentRequest, RefundRequest, PaymentMethodRequest, VerificationRequest) - DTOs/Response: initialize all string properties to string.Empty (PaymentResponse, VerificationResponse, PaymentStatusResponse, PaymentMethodResponse) - Entities: initialize all string properties to string.Empty (PaymentTransaction, RefundTransaction, PaymentMethod) - Exceptions: make contextual properties nullable where only set in some ctors (TransactionNotFoundException.TransactionReference, PaymentValidationException.ValidationErrors, PaymentGatewayException.Gateway, PaymentMethodNotFoundException.PaymentMethodId) - Presentation/Models: initialize all string properties to string.Empty (CreatePaymentRequest, ErrorResponse, SavePaymentMethodRequest) - GatewayExtractor: change ExtractReferenceFromWebhook return type to string? - PaystackGateway: add ?? string.Empty / ?? fallback messages / GetString()! null-forgiving operators for all JsonElement.GetString() calls; fix error ternary null propagation Build: 0 warnings, 0 errors across all 3 projects Tests: 76/76 unit tests passing
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@PayBridge.SDK/Gateways/PaystackGateway.cs`:
- Around line 160-161: The Paystack parsing is brittle: update the parsing logic
in PaystackGateway for RefundReference (data.id) and PaymentDate/RefundDate
(paid_at/created_at) to avoid exceptions by safely reading JSON types and
validating date formats; specifically, change the code that reads
data.GetProperty("id").GetString() to detect numeric vs string (use
JsonElement.ValueKind and TryGetInt64/TryGetInt32 or GetRawText and normalize to
a string) and assign RefundReference from the normalized value, and replace
direct DateTime.Parse(...) calls used when constructing PaymentDate and
RefundDate with guarded parsing (check for property existence/null/empty, use
DateTime.TryParse or TryParseExact with appropriate styles/culture and fallback
to DateTime.UtcNow or null) so malformed or missing timestamps don’t throw —
update the parsing blocks constructing PaymentDate/RefundDate and the
RefundReference assignment accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f470582-4aa2-4da0-be6a-d31ce51fb137
📒 Files selected for processing (22)
PayBridge.SDK.Presentation/Models/CreatePaymentRequest.csPayBridge.SDK.Presentation/Models/ErrorResponse.csPayBridge.SDK.Presentation/Models/SavePaymentMethodRequest.csPayBridge.SDK.Test/Integration/PeachPaymentsIntegrationTests.csPayBridge.SDK.Test/PayBridgeTests.csPayBridge.SDK/Dtos/Request/PaymentMethodRequest.csPayBridge.SDK/Dtos/Request/PaymentRequest.csPayBridge.SDK/Dtos/Request/RefundRequest.csPayBridge.SDK/Dtos/Request/VerificationRequest.csPayBridge.SDK/Dtos/Response/PaymentMethodResponse.csPayBridge.SDK/Dtos/Response/PaymentResponse.csPayBridge.SDK/Dtos/Response/PaymentStatusResponse.csPayBridge.SDK/Dtos/Response/VerificationResponse.csPayBridge.SDK/Entities/PaymentMethod.csPayBridge.SDK/Entities/PaymentTransaction.csPayBridge.SDK/Entities/RefundTransaction.csPayBridge.SDK/Exceptions/PaymentGatewayException.csPayBridge.SDK/Exceptions/PaymentMethodNotFoundException.csPayBridge.SDK/Exceptions/PaymentValidationException.csPayBridge.SDK/Exceptions/TransactionNotFoundException.csPayBridge.SDK/Gateways/PaystackGateway.csPayBridge.SDK/Helper/GatewayExtractor.cs
✅ Files skipped from review due to trivial changes (7)
- PayBridge.SDK/Dtos/Response/PaymentStatusResponse.cs
- PayBridge.SDK/Exceptions/PaymentGatewayException.cs
- PayBridge.SDK/Exceptions/TransactionNotFoundException.cs
- PayBridge.SDK/Dtos/Request/PaymentMethodRequest.cs
- PayBridge.SDK.Presentation/Models/ErrorResponse.cs
- PayBridge.SDK/Exceptions/PaymentMethodNotFoundException.cs
- PayBridge.SDK/Dtos/Response/PaymentMethodResponse.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- PayBridge.SDK.Test/Integration/PeachPaymentsIntegrationTests.cs
Comment on lines
+160
to
161
| ? DateTime.Parse(paidAt.GetString()!) | ||
| : DateTime.UtcNow, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Paystack API docs: In refund/transaction verify sample responses, what are the documented types for data.id, paid_at, and created_at fields?
💡 Result:
In Paystack API sample responses for transaction verification and refunds, the fields are documented with the following types: data.id: Integer (int) [1][2][3][4] paid_at: String (string) [3][4][5] created_at: String (string) [3][4][5] These fields are returned as part of JSON response objects [6][7][8]. Specifically, the identification field (data.id) is represented as an integer [1][3][4], while the timestamp fields (paid_at and created_at) are represented as ISO 8601 formatted strings [6][9][7][10]. Note that in some SDK implementations or edge cases, these timestamp fields may be nullable (represented as string | null) [5].
Citations:
- 1: https://github.com/alexasomba/paystack-php/blob/HEAD/docs/Model/VerifyResponseData.md
- 2: https://github.com/alexasomba/paystack-php/blob/HEAD/docs/Model/RefundCreateResponseDataTransaction.md
- 3: https://github.com/alexasomba/paystack-php/blob/main/docs/Model/TransactionFetchResponseData.md
- 4: https://github.com/alexasomba/paystack-php/blob/main/docs/Model/TransactionListResponseArray.md
- 5: https://jsr.io/@irabeny/paystack-sdk/doc/~/TransactionResponseDataT
- 6: https://paystack.com/docs/payments/verify-payments/
- 7: https://docs-v2.paystack.com/payments/refunds/
- 8: https://docs-v2.paystack.com/api/
- 9: https://paystack.com/docs/api/transaction/
- 10: https://paystack.com/docs/api/refund/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the file and print the relevant regions with line numbers
FILE="PayBridge.SDK/Gateways/PaystackGateway.cs"
if [ ! -f "$FILE" ]; then
echo "File not found: $FILE" >&2
exit 1
fi
echo "== snippet around line 150-180 =="
nl -ba "$FILE" | sed -n '140,190p'
echo "== snippet around line 220-260 =="
nl -ba "$FILE" | sed -n '210,270p'
echo "== find all DateTime.Parse / TryParse usage in this file =="
rg -n "DateTime\.Parse\(|DateTime\.TryParse\(" "$FILE" || true
echo "== find RefundReference mapping in this file =="
rg -n "RefundReference|GetProperty\\(\"id\"\\)|TryGetProperty\\(\"id\"\\)" "$FILE" || trueRepository: teesofttech/PayBridge
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="PayBridge.SDK/Gateways/PaystackGateway.cs"
if [ ! -f "$FILE" ]; then
echo "File not found: $FILE" >&2
exit 1
fi
echo "== snippet around line 150-180 =="
awk -v start=140 -v end=200 'NR>=start && NR<=end {printf "%d\t%s\n", NR, $0}' "$FILE"
echo "== snippet around line 220-260 =="
awk -v start=200 -v end=280 'NR>=start && NR<=end {printf "%d\t%s\n", NR, $0}' "$FILE"
echo "== find all DateTime.Parse / TryParse usage in this file =="
rg -n "DateTime\.(Parse|TryParse)\(" "$FILE" || true
echo "== find RefundReference mapping in this file =="
rg -n "RefundReference|TryGetProperty\\(\"id\"\\)|GetProperty\\(\"id\"\\)" "$FILE" || true
echo "== show any refund/verification mapping blocks (near 'paid_at' / 'created_at') =="
rg -n "paid_at|created_at" "$FILE" || trueRepository: teesofttech/PayBridge
Length of output: 6801
Harden Paystack refund/verification field parsing to prevent runtime exceptions.
- RefundReference:
data.idis documented as an integer, but code usesdata.GetProperty("id").GetString()(lines ~233), which can throw when the JSON value is numeric. - PaymentDate/RefundDate:
paid_atandcreated_atare parsed withDateTime.Parse(...)(lines ~159-161 and ~237-239);created_athas no null/empty/format guard, and both can throw on unexpected timestamp formats.
Proposed fix
+using System.Globalization;
...
- PaymentDate = data.TryGetProperty("paid_at", out var paidAt) && !string.IsNullOrEmpty(paidAt.GetString())
- ? DateTime.Parse(paidAt.GetString()!)
- : DateTime.UtcNow,
+ PaymentDate = data.TryGetProperty("paid_at", out var paidAt)
+ && DateTime.TryParse(
+ paidAt.GetString(),
+ CultureInfo.InvariantCulture,
+ DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal,
+ out var parsedPaidAt)
+ ? parsedPaidAt
+ : DateTime.UtcNow,
...
- RefundReference = data.GetProperty("id").GetString() ?? string.Empty,
+ RefundReference = data.TryGetProperty("id", out var refundId)
+ ? refundId.ValueKind switch
+ {
+ JsonValueKind.String => refundId.GetString() ?? string.Empty,
+ JsonValueKind.Number => refundId.GetInt64().ToString(CultureInfo.InvariantCulture),
+ _ => string.Empty
+ }
+ : string.Empty,
...
- RefundDate = data.TryGetProperty("created_at", out var createdAt)
- ? DateTime.Parse(createdAt.GetString()!)
- : DateTime.UtcNow
+ RefundDate = data.TryGetProperty("created_at", out var createdAt)
+ && DateTime.TryParse(
+ createdAt.GetString(),
+ CultureInfo.InvariantCulture,
+ DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal,
+ out var parsedCreatedAt)
+ ? parsedCreatedAt
+ : DateTime.UtcNow🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PayBridge.SDK/Gateways/PaystackGateway.cs` around lines 160 - 161, The
Paystack parsing is brittle: update the parsing logic in PaystackGateway for
RefundReference (data.id) and PaymentDate/RefundDate (paid_at/created_at) to
avoid exceptions by safely reading JSON types and validating date formats;
specifically, change the code that reads data.GetProperty("id").GetString() to
detect numeric vs string (use JsonElement.ValueKind and TryGetInt64/TryGetInt32
or GetRawText and normalize to a string) and assign RefundReference from the
normalized value, and replace direct DateTime.Parse(...) calls used when
constructing PaymentDate and RefundDate with guarded parsing (check for property
existence/null/empty, use DateTime.TryParse or TryParseExact with appropriate
styles/culture and fallback to DateTime.UtcNow or null) so malformed or missing
timestamps don’t throw — update the parsing blocks constructing
PaymentDate/RefundDate and the RefundReference assignment accordingly.
Source: MCP tools
This was referenced Jul 18, 2026
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
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.
Summary
Closes #50
Files Added
PayBridge.SDK.Test/Unit/PeachPaymentsGatewayTests.cs— 27 unit testsPayBridge.SDK.Test/Integration/PeachPaymentsIntegrationTests.cs— 2 integration testsPayBridge.SDK.Test/Helpers/MockHttpClientFactory.cs— reusable IHttpClientFactory stubUnit Tests (27 tests, zero live HTTP)
Integration Tests (2 tests, auto-skip when env vars absent)
Requires:
PEACH_ENTITY_ID+PEACH_ACCESS_TOKENTest Results
Release note
This PR completes the test coverage for PeachPayments alongside PR #35 (gateway implementation). Together they are ready for the v1.2.0 release.
Summary by CodeRabbit
New Features
Tests
Improvements