Python Medium

Missing Test Coverage for Payment Edge Case

New payment processor logic has zero test coverage for refund reversal scenarios.

Repository billing-service

PR Title Implement refund reversal for Stripe refunds

Author @priya_s

Files changed 3

Diff size +142 / -28 lines

Changed file

billing/refund_handler.py

class RefundHandler:
    def __init__(self, stripe_client):
        self.stripe_client = stripe_client
    def reverse_refund(self, charge_id: str, amount: Optional[int] = None) -> RefundResult:
        charge = self.stripe_client.charges.retrieve(charge_id)
        if charge.refunded:
            raise ValueError("Charge already refunded")
        return self.stripe_client.refunds.create(charge=charge_id, amount=amount)
    def reverse_refund(self, charge_id: str, amount: Optional[int] = None) -> RefundResult:
        if not charge_id or not isinstance(charge_id, str):
            raise ValueError("charge_id must be a non-empty string")
        charge = self.stripe_client.charges.retrieve(charge_id)
        if charge.refunded:
            raise ValueError("Charge already refunded")
        refund = self.stripe_client.refunds.create(charge=charge_id, amount=amount)
        logger.info(f"Refund reversed for charge {charge_id}", extra={"amount": amount})
        return refund
    def _get_pending_reversal(self, charge_id: str) -> Optional[Dict]:
        return next(
            (r for r in self.stripe_client.charge_refunds.list(charge_id).data
             if r.status == "failed"),
            None
        )

Codio Review

Summary

This PR adds refund reversal logic to the billing service — a new, non-trivial code path that touches Stripe's external API and involves state transitions (initiated → pending → completed or failed). The implementation looks solid, but the PR ships zero test coverage for this new behavior. Any regression in the refund reversal logic will go undetected until it hits production. With payment logic, that's a business-critical blind spot.

Findings

Finding #1 — Zero Test Coverage for New Logic (Critical Gap) Medium

The diff touches three files (billing/refund_handler.py, billing/models.py, api/routes.py) but no test files are included in the PR. The coverage report (run: pytest --cov=billing --cov-report=term-missing) shows billing/refund_handler.py at 0% coverage.

Refund reversal is a stateful, side-effect-heavy operation that calls an external API with real money at stake. The scenarios that need coverage include:

Already refunded: the existing guard check — verify this path is tested and that it raises ValueError as expected.
Partial reversal: amount is passed through to Stripe — verify behavior when amount < charge.amount vs. amount == charge.amount.
Invalid charge ID: Stripe raises InvalidRequestError when the charge doesn't exist — verify this surfaces correctly to API callers.
Reversal already pending: the _get_pending_reversal helper exists but is never called — is this dead code or is it part of the reversal flow that was forgotten?

Finding #2 — Dead Code: _get_pending_reversal Never Called Medium

The method _get_pending_reversal is defined on line 18 of the new file but has no call site in the diff or in the surrounding codebase. It looks like it was either:

Part of an earlier draft of this feature that was removed but the helper was left behind, or
Intended to be integrated but the integration was forgotten.

If it's intended, it needs a test and a call site. If it's not, it should be removed — dead code creates confusion and maintenance debt. Leaving it in with a comment explaining its purpose (or an explicit TODO linking to a Linear issue) is better than silently leaving it unused.

Finding #3 — Missing Logging Context for Error Traceability Low

The new logging call at line 23 captures the charge_id and amount, which is good. However, if the Stripe API call raises an exception, there's no structured error log that captures the full request context. When a customer support ticket comes in about a failed reversal, having charge_id, amount, and the Stripe error message in one structured log entry will cut triage time significantly.

Recommendation

This PR should not be merged without tests. The logic is sound, but payment reversal without test coverage is a business risk. Before merging:

Add at minimum the three happy-path test cases and two error-path cases described in Finding #1. Use unittest.mock to patch stripe_client — do not hit the Stripe test API in unit tests.
Decide on _get_pending_reversal: either integrate it into the reversal flow and test it, or remove it. Document the decision with a Linear issue if integration is deferred.
Add a structured error log in the exception handler for the Stripe call — at minimum capture charge_id, error type, and error message.

The good news: the code quality is high, the validation is correct, and the logging call is well-placed. This is close to ready — it just needs the tests to land alongside it.

Suggested Fix

tests/billing/test_refund_handler.py

import pytest
from unittest.mock import MagicMock
from billing.refund_handler import RefundHandler

class TestReverseRefund:
    def setup_method(self):
        self.mock_stripe = MagicMock()
        self.handler = RefundHandler(self.mock_stripe)

    def test_raises_when_charge_already_refunded(self):
        charge = MagicMock()
        charge.refunded = True
        self.mock_stripe.charges.retrieve.return_value = charge

        with pytest.raises(ValueError, match="already refunded"):
            self.handler.reverse_refund("ch_test123")

    def test_partial_reversal_calls_stripe_with_correct_amount(self):
        charge = MagicMock()
        charge.refunded = False
        charge.amount = 5000
        self.mock_stripe.charges.retrieve.return_value = charge
        self.mock_stripe.refunds.create.return_value = "re_test456"

        self.handler.reverse_refund("ch_test123", amount=2500)

        self.mock_stripe.refunds.create.assert_called_once_with(
            charge="ch_test123", amount=2500
        )

    def test_invalid_charge_id_propagates_stripe_error(self):
        from stripe.error import InvalidRequestError
        self.mock_stripe.charges.retrieve.side_effect = InvalidRequestError(
            "No such charge", param="id"
        )
        with pytest.raises(InvalidRequestError):
            self.handler.reverse_refund("ch_invalid")

Why Codio flagged this

Codio runs a coverage diff on every PR that touches Python or TypeScript files. When new logic lands without a corresponding test file, it flags the gap and explains exactly what cases are missing. For payment logic — anything that moves money — zero coverage is a high-severity concern regardless of how clean the code looks. Codio also cross-references the diff to find dead code helpers that are defined but never called, so unused code doesn't silently accumulate.

Get Codio reviewing your PRs

Ship reviews like this on every pull request. No reminders. No missed PRs.

← Previous SQL Injection in User Input Handler Next → Race Condition in Async Cache Refresh