Merge pull request #1157 from thebiggive/DON-998-allow-donations-with…

…out-transaction-id DON-998: Stop assuming donations will always have transaction IDs
thebiggive · Jan 8, 2025 · d2e300f · d2e300f
2 parents 064f96d + 1ba369e
commit d2e300f
Show file tree

Hide file tree

Showing 11 changed files with 70 additions and 75 deletions.
diff --git a/integrationTests/DonationRepositoryTest.php b/integrationTests/DonationRepositoryTest.php
@@ -11,10 +11,8 @@
 use MatchBot\Domain\CampaignFunding;
 use MatchBot\Domain\Charity;
 use MatchBot\Domain\DoctrineDonationRepository;
-use MatchBot\Domain\DomainException\MissingTransactionId;
 use MatchBot\Domain\Donation;
 use MatchBot\Domain\DonationRepository;
-use MatchBot\Domain\DonationService;
 use MatchBot\Domain\DonationStatus;
 use MatchBot\Domain\EmailAddress;
 use MatchBot\Domain\FundingWithdrawal;
@@ -23,7 +21,6 @@
 use MatchBot\Domain\Salesforce18Id;
 use MatchBot\Tests\TestCase;
 use Prophecy\Argument;
-use Ramsey\Uuid\Uuid;
 use Symfony\Component\Messenger\Envelope;
 use Symfony\Component\Messenger\RoutableMessageBus;
 

diff --git a/src/Application/Actions/Donations/Confirm.php b/src/Application/Actions/Donations/Confirm.php
@@ -90,6 +90,9 @@ protected function action(Request $request, Response $response, array $args): Re
             throw new HttpBadRequestException($request, $message);
         }
 
+        $paymentIntentId = $donation->getTransactionId();
+        Assertion::notNull($paymentIntentId);
+
         try {
             $updatedIntent = $this->donationService->confirmOnSessionDonation(
                 $donation,
@@ -102,7 +105,7 @@ protected function action(Request $request, Response $response, array $args): Re
                 context: 'confirmPaymentIntent',
                 exception: $exception,
                 donation: $donation,
-                paymentIntentId: $donation->getTransactionId(),
+                paymentIntentId: $paymentIntentId,
             );
         } catch (InvalidRequestException $exception) {
             if (!DonationService::errorMessageFromStripeIsExpected($exception)) {
@@ -114,7 +117,7 @@ protected function action(Request $request, Response $response, array $args): Re
                 'Stripe %s on Confirm for donation %s (%s): %s',
                 $exceptionClass,
                 $donation->getUuid(),
-                $donation->getTransactionId(),
+                $paymentIntentId,
                 $exception->getMessage(),
             ));
 
@@ -131,7 +134,7 @@ protected function action(Request $request, Response $response, array $args): Re
                 'Stripe %s on Confirm for donation %s (%s): %s',
                 get_class($exception),
                 $donation->getUuid(),
-                $donation->getTransactionId(),
+                $paymentIntentId,
                 $exception->getMessage(),
             ));
 

diff --git a/src/Application/Actions/Donations/Update.php b/src/Application/Actions/Donations/Update.php
@@ -11,6 +11,7 @@
 use MatchBot\Application\Actions\Action;
 use MatchBot\Application\Actions\ActionError;
 use MatchBot\Application\Actions\ActionPayload;
+use MatchBot\Application\Assertion;
 use MatchBot\Application\AssertionFailedException;
 use MatchBot\Application\HttpModels;
 use MatchBot\Client\Stripe;
@@ -387,8 +388,10 @@ private function addData(
             }
 
             if ($donationData->autoConfirmFromCashBalance) {
+                $transactionId = $donation->getTransactionId();
+                Assertion::notNull($transactionId);
                 try {
-                    $confirmedIntent = $this->stripe->confirmPaymentIntent($donation->getTransactionId());
+                    $confirmedIntent = $this->stripe->confirmPaymentIntent($transactionId);
 
                     /* @var string|null $nextActionType */
                     $nextActionType = null;
@@ -450,7 +453,7 @@ private function addData(
                             'Stripe %s on Update for donation %s (%s): %s',
                             $exceptionClass,
                             $donation->getUuid(),
-                            $donation->getTransactionId(),
+                            $transactionId,
                             $exception->getMessage(),
                         ));
 
@@ -483,7 +486,12 @@ private function addData(
      */
     private function updatePaymentIntent(Donation $donation): void
     {
-        $this->stripe->updatePaymentIntent($donation->getTransactionId(), [
+        $paymentIntentId = $donation->getTransactionId();
+        if ($paymentIntentId === null) {
+            return;
+        }
+
+        $this->stripe->updatePaymentIntent($paymentIntentId, [
             'amount' => $donation->getAmountFractionalIncTip(),
             'currency' => strtolower($donation->getCurrencyCode()),
             'metadata' => [
@@ -524,7 +532,10 @@ private function handleGeneralStripeError(
             $exception instanceof InvalidRequestException &&
             str_starts_with($exception->getMessage(), $alreadyCapturedMsg)
         ) {
-            $latestPI = $this->stripe->retrievePaymentIntent($donation->getTransactionId());
+            $paymentIntentId = $donation->getTransactionId();
+            Assertion::notNull($paymentIntentId);
+
+            $latestPI = $this->stripe->retrievePaymentIntent($paymentIntentId);
             if ($latestPI->application_fee_amount === $donation->getAmountToDeductFractional()) {
                 $noFeeChangeMessage = 'Stripe Payment Intent update ignored after capture; no fee ' .
                     'change on %s, %s [%s]: %s';

diff --git a/src/Application/Actions/Hooks/StripePaymentsUpdate.php b/src/Application/Actions/Hooks/StripePaymentsUpdate.php
@@ -13,7 +13,6 @@
 use MatchBot\Domain\CardBrand;
 use MatchBot\Domain\Country;
 use MatchBot\Domain\Currency;
-use MatchBot\Domain\DomainException\MissingTransactionId;
 use MatchBot\Domain\Donation;
 use MatchBot\Domain\DonationFundsNotifier;
 use MatchBot\Domain\DonationRepository;

diff --git a/src/Application/Messenger/DonationUpserted.php b/src/Application/Messenger/DonationUpserted.php
@@ -2,7 +2,6 @@
 
 namespace MatchBot\Application\Messenger;
 
-use MatchBot\Domain\DomainException\MissingTransactionId;
 use MatchBot\Domain\Donation;
 use Symfony\Component\Messenger\Bridge\AmazonSqs\MessageGroupAwareInterface;
 
@@ -16,9 +15,6 @@ private function __construct(public string $uuid, public array $jsonSnapshot)
         parent::__construct($uuid, $jsonSnapshot);
     }
 
-    /**
-     * @throws MissingTransactionId
-     */
     public static function fromDonation(Donation $donation): self
     {
         return new self(

diff --git a/src/Domain/DoctrineDonationRepository.php b/src/Domain/DoctrineDonationRepository.php
@@ -17,7 +17,6 @@
 use MatchBot\Application\Messenger\DonationUpserted;
 use MatchBot\Client\BadRequestException;
 use MatchBot\Client\NotFoundException;
-use MatchBot\Domain\DomainException\MissingTransactionId;
 use Ramsey\Uuid\UuidInterface;
 use Symfony\Component\Messenger\Envelope;
 use Symfony\Component\Messenger\MessageBusInterface;
@@ -604,13 +603,7 @@ public function pushSalesforcePending(\DateTimeImmutable $now, MessageBusInterfa
                 continue;
             }
 
-            try {
-                $newDonation = DonationUpserted::fromDonation($proxy);
-            } catch (MissingTransactionId) {
-                $this->logger->warning("Missing transaction id for donation {$proxy->getId()}, cannot push to SF");
-                continue;
-            }
-            $bus->dispatch(new Envelope($newDonation));
+            $bus->dispatch(new Envelope(DonationUpserted::fromDonation($proxy)));
         }
 
         $proxiesToUpdate = $this->findBy(

diff --git a/src/Domain/DomainException/MissingTransactionId.php b/src/Domain/DomainException/MissingTransactionId.php
diff --git a/src/Domain/Donation.php b/src/Domain/Donation.php
@@ -18,7 +18,6 @@
 use MatchBot\Application\Fees\Calculator;
 use MatchBot\Application\HttpModels\DonationCreate;
 use MatchBot\Application\LazyAssertionException;
-use MatchBot\Domain\DomainException\MissingTransactionId;
 use Messages;
 use Ramsey\Uuid\Uuid;
 use Ramsey\Uuid\UuidInterface;
@@ -510,9 +509,6 @@ public function preUpdate(PreUpdateEventArgs $args): void
         }
     }
 
-    /**
-     * @throws MissingTransactionId
-     */
     public function toSFApiModel(): array
     {
         $data = [...$this->toFrontEndApiModel(), 'originalPspFee' => (float) $this->getOriginalPspFee()];
@@ -530,9 +526,6 @@ public function toSFApiModel(): array
         return $data;
     }
 
-    /**
-     * @throws MissingTransactionId
-     */
     public function toFrontEndApiModel(): array
     {
         $totalPaidByDonor = $this->getTotalPaidByDonor();
@@ -831,21 +824,17 @@ private function getConfirmedPledgeWithdrawalTotal(): string
     }
 
     /**
-     * We may call this safely *only* after a donation has a PSP's transaction ID.
-     * Stripe assigns the ID before we return a usable donation object to the Donate client,
-     * so this should be true in most of the app for usable donations.
-     *
-     * @throws MissingTransactionId if the transaction ID is not set
+     * @return string|null For a stripe based donation this is the payment intent ID. Usually set immediately for
+     * each new donation, but for delayed regular giving donations will not be set until we're ready to collect
+     * the payment.
      */
-    public function getTransactionId(): string
+    public function getTransactionId(): ?string
     {
-        if ($this->transactionId === null) {
-            throw new MissingTransactionId('Transaction ID not set for donation ' . ($this->id ?? 'null'));
-        }
-
         return $this->transactionId;
     }
 
+
+
     public function getChargeId(): ?string
     {
         return $this->chargeId;
@@ -1514,20 +1503,8 @@ public function update(
      */
     public function assertIsReadyToConfirm(): true
     {
-        Assert::lazy()
-            ->that($this->donorFirstName, 'donorFirstName')->notNull('Missing Donor First Name')
-            ->that($this->donorLastName, 'donorLastName')->notNull('Missing Donor Last Name')
-            ->that($this->donorEmailAddress)->notNull('Missing Donor Email Address')
-            ->that($this->donorCountryCode)->notNull('Missing Billing Country')
-            ->that($this->donorBillingPostcode)->notNull('Missing Billing Postcode')
-            ->that($this->tbgComms)->notNull('Missing tbgComms preference')
-            ->that($this->charityComms)->notNull('Missing charityComms preference')
-            ->that($this->donationStatus, 'donationStatus')
-            ->that($this->donationStatus)->inArray(
-                [DonationStatus::Pending, DonationStatus::PreAuthorized],
-                "Donation status is '{$this->donationStatus->value}', must be " .
-                "'Pending' or 'PreAuthorized' to confirm payment"
-            )
+        $this->assertionsForConfirmOrPreAuth()
+            ->that($this->transactionId)->notNull('Missing Transaction ID')
             ->verifyNow();
 
         return true;
@@ -1538,7 +1515,7 @@ public function assertIsReadyToConfirm(): true
      */
     public function preAuthorize(DateTimeImmutable $paymentDate): void
     {
-        $this->assertIsReadyToConfirm();
+        $this->assertionsForConfirmOrPreAuth()->verifyNow();
         $this->preAuthorizationDate = $paymentDate;
         $this->donationStatus = DonationStatus::PreAuthorized;
     }
@@ -1688,4 +1665,27 @@ public function totalCharityValueAmount(): string
             ])
         )->toNumericString();
     }
+
+    /**
+     * These assertions are required both for a donation to be confirmed and for it to be pre-authorized. They do
+     * NOT include an assertion that the donation has a transaction ID, as that is only required for confirmation, not
+     * for pre-auth.
+     */
+    private function assertionsForConfirmOrPreAuth(): \Assert\LazyAssertion
+    {
+        return Assert::lazy()
+            ->that($this->donorFirstName, 'donorFirstName')->notNull('Missing Donor First Name')
+            ->that($this->donorLastName, 'donorLastName')->notNull('Missing Donor Last Name')
+            ->that($this->donorEmailAddress)->notNull('Missing Donor Email Address')
+            ->that($this->donorCountryCode)->notNull('Missing Billing Country')
+            ->that($this->donorBillingPostcode)->notNull('Missing Billing Postcode')
+            ->that($this->tbgComms)->notNull('Missing tbgComms preference')
+            ->that($this->charityComms)->notNull('Missing charityComms preference')
+            ->that($this->donationStatus, 'donationStatus')
+            ->that($this->donationStatus)->inArray(
+                [DonationStatus::Pending, DonationStatus::PreAuthorized],
+                "Donation status is '{$this->donationStatus->value}', must be " .
+                "'Pending' or 'PreAuthorized' to confirm payment"
+            );
+    }
 }
diff --git a/src/Domain/DonationService.php b/src/Domain/DonationService.php
@@ -20,7 +20,6 @@
 use MatchBot\Domain\DomainException\DonationAlreadyFinalised;
 use MatchBot\Domain\DomainException\DonationCreateModelLoadFailure;
 use MatchBot\Domain\DomainException\MandateNotActive;
-use MatchBot\Domain\DomainException\MissingTransactionId;
 use MatchBot\Domain\DomainException\NoDonorAccountException;
 use MatchBot\Domain\DomainException\RegularGivingCollectionEndPassed;
 use MatchBot\Domain\DomainException\StripeAccountIdNotSetForAccount;
@@ -31,7 +30,6 @@
 use Random\Randomizer;
 use Stripe\Exception\ApiErrorException;
 use Stripe\Exception\InvalidRequestException;
-use Stripe\PaymentIntent;
 use Stripe\StripeObject;
 use Symfony\Component\Clock\ClockInterface;
 use Symfony\Component\Messenger\Envelope;
@@ -206,8 +204,11 @@ public function confirmOnSessionDonation(
         // or what we're charging.
         $this->entityManager->flush();
 
+        $paymentIntentId = $donation->getTransactionId();
+        Assertion::notNull($paymentIntentId);
+
         return $this->stripe->confirmPaymentIntent(
-            $donation->getTransactionId(),
+            $paymentIntentId,
             ['confirmation_token' => $tokenId->stripeConfirmationTokenId]
         );
     }
@@ -258,8 +259,10 @@ public function confirmPreAuthorized(Donation $donation): void
             );
         }
 
+        $paymentIntentId = $donation->getTransactionId();
+        Assertion::notNull($paymentIntentId);
         $paymentIntent = $this->stripe->confirmPaymentIntent(
-            $donation->getTransactionId(),
+            $paymentIntentId,
             ['payment_method' => $paymentMethod->stripePaymentMethodId]
         );
         if ($paymentIntent->status !== 'succeeded') {
@@ -393,7 +396,10 @@ private function doUpdateDonationFees(
             // Note that `on_behalf_of` is set up on create and is *not allowed* on update.
         ];
 
-        $this->stripe->updatePaymentIntent($donation->getTransactionId(), $updatedIntentData);
+        $paymentIntentId = $donation->getTransactionId();
+        if ($paymentIntentId !== null) {
+            $this->stripe->updatePaymentIntent($paymentIntentId, $updatedIntentData);
+        }
     }
 
     private function updateDonationFeesFromConfirmationToken(
@@ -528,9 +534,10 @@ public function cancel(Donation $donation): void
             $this->donationRepository->releaseMatchFunds($donation);
         }
 
-        if ($donation->getPsp() === 'stripe') {
+        $transactionId = $donation->getTransactionId();
+        if ($donation->getPsp() === 'stripe' && $transactionId !== null) {
             try {
-                $this->stripe->cancelPaymentIntent($donation->getTransactionId());
+                $this->stripe->cancelPaymentIntent($transactionId);
             } catch (ApiErrorException $exception) {
                 /**
                  * As per the notes in {@see DonationRepository::releaseMatchFunds()}, we

diff --git a/tests/Application/Actions/Hooks/StripePaymentsUpdateTest.php b/tests/Application/Actions/Hooks/StripePaymentsUpdateTest.php
@@ -11,7 +11,6 @@
 use MatchBot\Application\Notifier\StripeChatterInterface;
 use MatchBot\Application\Persistence\RetrySafeEntityManager;
 use MatchBot\Domain\CampaignRepository;
-use MatchBot\Domain\DomainException\MissingTransactionId;
 use MatchBot\Domain\Donation;
 use MatchBot\Domain\DonationRepository;
 use MatchBot\Domain\DonationService;

diff --git a/tests/Domain/DonationTest.php b/tests/Domain/DonationTest.php
@@ -452,6 +452,8 @@ public function testIsReadyToConfirmWithRequiredFieldsSet(): void
             tbgComms: false,
         );
 
+        $donation->setTransactionId('any-string');
+
         $this->assertTrue($donation->assertIsReadyToConfirm());
     }
-Original file line number
+Diff line change
@@ Expand Up @@
                 tbgComms: false,
             );
+            $donation->setTransactionId('any-string');
             $this->assertTrue($donation->assertIsReadyToConfirm());
         }
@@ Expand Down @@