DRUPALMM-207: Add ContributionPayability API for generic payability checks #9
Conversation
…hecks Add a generic payability API that works across multiple payment processors using a registry/provider pattern similar to the existing WebhookHandlerRegistry. New components: - PayabilityProviderInterface: Contract for processor-specific providers - PayabilityResult: DTO for payability check results - PayabilityProviderAdapter: Adapter for duck-typed providers (autoload safety) - PayabilityProviderRegistry: Service to register and lookup providers - ContributionPayability.getStatus: API4 action to check payability The API allows payment processor extensions to register their payability providers, which determine whether contributions can be paid now or are managed by the processor (subscriptions, payment plans, etc.). Example usage: ContributionPayability.getStatus(contactId=123, contributionStatus=['Pending'])
Summary of ChangesHello @erawat, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes a robust and extensible framework for determining the payability of contributions within the system. By introducing a generic API4 action and a provider-based architecture, it allows different payment processors to integrate their specific rules for whether a contribution can be paid immediately or is managed by a recurring system. This standardization simplifies future development and ensures consistent handling of payment options across various integrations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new ContributionPayability API, including a GetStatus action, to determine if contributions can be paid immediately or are managed by a payment processor. The changes involve creating a PayabilityProviderRegistry service to manage processor-specific payability providers, a PayabilityProviderInterface for consistent contracts, a PayabilityProviderAdapter to handle duck-typed providers, and a PayabilityResult Data Transfer Object (DTO) to encapsulate payability status. Review comments highlight three main issues: first, the GetStatus action incorrectly uses array_merge instead of array_replace when combining payability results, leading to potential data corruption due to re-indexing; second, the PayabilityProviderAdapter needs to be enhanced to convert duck-typed provider array results into PayabilityResult objects for consistent output; and third, the PayabilityResult DTO's canPayNow and reason properties should be made nullable to align with the API's contract and the GetStatus action's logic, which allows for NULL values when no provider is registered.
| if ($registry->hasProvider($processorType)) { | ||
| $provider = $registry->getProvider($processorType); | ||
| $providerResults = $provider->getPayabilityForContributions($contributionIds); | ||
| $payabilityResults = array_merge($payabilityResults, $providerResults); |
There was a problem hiding this comment.
Using array_merge here is incorrect and will lead to data corruption. When merging arrays with integer keys, array_merge re-indexes the keys, which means the contribution IDs will be lost and replaced with a 0-based index. This will break the mapping between contributions and their payability results.
To merge the arrays while preserving the integer keys, you should use array_replace() or the + operator. array_replace() is generally safer and more explicit about its behavior.
$payabilityResults = array_replace($payabilityResults, $providerResults);| public function getPayabilityForContributions(array $contributionIds): array { | ||
| /** @var callable $callback */ | ||
| $callback = [$this->provider, 'getPayabilityForContributions']; | ||
| return $callback($contributionIds); | ||
| } |
There was a problem hiding this comment.
The adapter currently acts as a simple wrapper, delegating the call but not adapting the result. This forces consumers like the GetStatus action to handle multiple return formats (arrays and PayabilityResult objects), which undermines the purpose of the Adapter pattern.
To improve the design, the adapter should be responsible for converting the array returned by a duck-typed provider into a PayabilityResult object. This ensures that getProvider() always returns an object that adheres to a consistent contract, simplifying consumer logic. This change depends on making the PayabilityResult properties nullable, as suggested in another comment.
public function getPayabilityForContributions(array $contributionIds): array {
$results = $this->provider->getPayabilityForContributions($contributionIds);
$payabilityResults = [];
foreach ($results as $id => $result) {
if ($result instanceof PayabilityResult) {
$payabilityResults[$id] = $result;
}
elseif (is_array($result)) {
$payabilityResults[$id] = new PayabilityResult(
$result['can_pay_now'] ?? NULL,
$result['payability_reason'] ?? NULL,
$result['payment_type'] ?? NULL,
$result['payability_metadata'] ?? []
);
}
}
return $payabilityResults;
}| public bool $canPayNow; | ||
|
|
||
| /** | ||
| * Human-readable explanation of the payability status. | ||
| * | ||
| * Examples: | ||
| * - "User can initiate payment via checkout" | ||
| * - "Managed by GoCardless subscription" | ||
| * - "Managed by Direct Debit payment plan" | ||
| * | ||
| * @var string | ||
| */ | ||
| public string $reason; | ||
|
|
||
| /** | ||
| * Type of payment this contribution belongs to. | ||
| * | ||
| * Possible values: | ||
| * - 'one_off': Single payment, not linked to recurring | ||
| * - 'subscription': Part of a recurring subscription (auto-managed) | ||
| * - 'payment_plan': Part of a payment plan with membership (auto-managed) | ||
| * | ||
| * @var string|null | ||
| */ | ||
| public ?string $paymentType; | ||
|
|
||
| /** | ||
| * Processor-specific metadata. | ||
| * | ||
| * Contains additional information relevant to the payment processor, | ||
| * such as mandate status, subscription ID, customer ID, etc. | ||
| * | ||
| * @var array<string, mixed> | ||
| */ | ||
| public array $metadata; | ||
|
|
||
| /** | ||
| * Construct a PayabilityResult. | ||
| * | ||
| * @param bool $canPayNow | ||
| * Whether the contribution can be paid now. | ||
| * @param string $reason | ||
| * Human-readable explanation of the status. | ||
| * @param string|null $paymentType | ||
| * Type of payment (one_off, subscription, payment_plan). | ||
| * @param array<string, mixed> $metadata | ||
| * Processor-specific metadata. | ||
| */ | ||
| public function __construct( | ||
| bool $canPayNow, | ||
| string $reason, | ||
| ?string $paymentType = NULL, | ||
| array $metadata = [] | ||
| ) { | ||
| $this->canPayNow = $canPayNow; | ||
| $this->reason = $reason; | ||
| $this->paymentType = $paymentType; | ||
| $this->metadata = $metadata; | ||
| } |
There was a problem hiding this comment.
There's an inconsistency between this DTO and the API's contract. The properties $canPayNow and $reason are defined as non-nullable (bool and string), but the API specification allows can_pay_now to be NULL, and the logic for duck-typed providers in GetStatus.php also allows NULL values.
To create a consistent and robust contract, it's best to align the DTO with the API. Making these properties and their corresponding constructor arguments nullable will resolve this inconsistency and make the system more flexible.
public ?bool $canPayNow;
/**
* Human-readable explanation of the payability status.
*
* Examples:
* - "User can initiate payment via checkout"
* - "Managed by GoCardless subscription"
* - "Managed by Direct Debit payment plan"
*
* @var string|null
*/
public ?string $reason;
/**
* Type of payment this contribution belongs to.
*
* Possible values:
* - 'one_off': Single payment, not linked to recurring
* - 'subscription': Part of a recurring subscription (auto-managed)
* - 'payment_plan': Part of a payment plan with membership (auto-managed)
*
* @var string|null
*/
public ?string $paymentType;
/**
* Processor-specific metadata.
*
* Contains additional information relevant to the payment processor,
* such as mandate status, subscription ID, customer ID, etc.
*
* @var array<string, mixed>
*/
public array $metadata;
/**
* Construct a PayabilityResult.
*
* @param bool|null $canPayNow
* Whether the contribution can be paid now.
* @param string|null $reason
* Human-readable explanation of the status.
* @param string|null $paymentType
* Type of payment (one_off, subscription, payment_plan).
* @param array<string, mixed> $metadata
* Processor-specific metadata.
*/
public function __construct(
?bool $canPayNow,
?string $reason,
?string $paymentType = NULL,
array $metadata = []
) {
$this->canPayNow = $canPayNow;
$this->reason = $reason;
$this->paymentType = $paymentType;
$this->metadata = $metadata;
}
Summary
Add a generic payability API that works across multiple payment processors using a registry/provider pattern similar to the existing
WebhookHandlerRegistry.New Components
canPay(),cannotPay())Architecture
API Parameters
contactId(required): Contact ID to check contributions forcontributionStatus(optional): Filter by status names (e.g.,['Pending'])startDate/endDate(optional): Date range filterResponse Fields
can_pay_nowtrueif user can pay,falseif managed by processor,nullif no providerpayability_reasonpayment_typeone_off,subscription, orpayment_planpayability_metadataExample Usage
Test plan
phpunit9 --filter PayabilityProviderRegistryTestphpunit9 --filter PayabilityResultTestRelated PRs