Skip to content

CIVIMM-462: Copy payment_instrument_id to instalments #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
erawat wants to merge 1 commit into
base: master
Choose a base branch
from
+29 −3
Open

CIVIMM-462: Copy payment_instrument_id to instalments #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions Civi/Paymentprocessingcore/DTO/RecurringContributionDTO.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@ class RecurringContributionDTO {
*/
private int $frequencyInterval;

/**
* @var int|null
*/
private ?int $paymentInstrumentId;

/**
* Private constructor — use fromApiResult() factory method.
*
Expand All @@ -76,6 +81,8 @@ class RecurringContributionDTO {
* Frequency unit (day, week, month, year).
* @param int $frequencyInterval
* Frequency interval.
* @param int|null $paymentInstrumentId
* Payment instrument ID or NULL.
*/
private function __construct(
int $id,
Expand All @@ -86,7 +93,8 @@ private function __construct(
?int $campaignId,
string $nextSchedContributionDate,
string $frequencyUnit,
int $frequencyInterval
int $frequencyInterval,
?int $paymentInstrumentId
) {
$this->id = $id;
$this->contactId = $contactId;
Expand All @@ -97,6 +105,7 @@ private function __construct(
$this->nextSchedContributionDate = $nextSchedContributionDate;
$this->frequencyUnit = $frequencyUnit;
$this->frequencyInterval = $frequencyInterval;
$this->paymentInstrumentId = $paymentInstrumentId;
}

/**
Expand Down Expand Up @@ -148,6 +157,10 @@ public static function fromApiResult(array $record): self {
? (int) $record['campaign_id']
: NULL;

$paymentInstrumentId = isset($record['payment_instrument_id']) && is_numeric($record['payment_instrument_id'])
? (int) $record['payment_instrument_id']
: NULL;
Comment on lines +160 to +162
Copy link

@gemini-code-assist gemini-code-assist bot Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The use of isset() here could allow a payment_instrument_id of 0 to be considered valid. In CiviCRM, entity IDs are typically positive integers, so 0 is not a valid ID. This could lead to incorrect data being propagated.

To ensure only valid, positive IDs are processed, it would be safer to use !empty() which correctly handles 0, null, and empty strings as invalid values. This aligns with the principle of failing fast if critical data is missing, rather than masking potential issues with invalid default values.

    $paymentInstrumentId = !empty($record['payment_instrument_id']) && is_numeric($record['payment_instrument_id'])
      ? (int) $record['payment_instrument_id']
      : NULL;
References
  1. Avoid overly defensive variable extraction that can mask underlying data issues. It's safer to fail fast if critical data is missing, especially when default values can lead to unexpected behavior (e.g., strtotime('') evaluating to 1970-01-01).


return new self(
(int) $id,
(int) $contactId,
Expand All @@ -157,7 +170,8 @@ public static function fromApiResult(array $record): self {
$campaignId,
(string) $nextSchedDate,
(string) $frequencyUnit,
(int) $frequencyInterval
(int) $frequencyInterval,
$paymentInstrumentId
);
}

Expand Down Expand Up @@ -224,4 +238,11 @@ public function getFrequencyInterval(): int {
return $this->frequencyInterval;
}

/**
* Get the payment instrument ID, or NULL if not set.
*/
public function getPaymentInstrumentId(): ?int {
return $this->paymentInstrumentId;
}

}
7 changes: 6 additions & 1 deletion Civi/Paymentprocessingcore/Service/InstalmentGenerationService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,8 @@ public function getDueRecurringContributions(string $processorType, int $batchSi
'amount',
'currency',
'financial_type_id',
'campaign_id'
'campaign_id',
'payment_instrument_id'
Copy link

@gemini-code-assist gemini-code-assist bot Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

While adding payment_instrument_id to the query is correct, this new functionality is not covered by unit tests. The repository style guide requires new features to have corresponding tests.

Please add tests to InstalmentGenerationServiceTest.php to verify that when a recurring contribution has a payment_instrument_id, the generated instalment also has it. Additionally, tests in RecurringContributionDTOTest.php should be added to ensure payment_instrument_id is correctly handled by the DTO.

References
  1. New features and bug fixes include unit tests. (link)

)
->addJoin(
'PaymentProcessor AS pp',
Expand Down Expand Up @@ -258,6 +259,10 @@ public function createInstalment(RecurringContributionDTO $recur, string $receiv
$createAction->addValue('campaign_id', $recur->getCampaignId());
}

if ($recur->getPaymentInstrumentId() !== NULL) {
$createAction->addValue('payment_instrument_id', $recur->getPaymentInstrumentId());
}

$result = $createAction->execute()->first();

if (!is_array($result)) {
Expand Down