From dfc47588ad5dce59ae8aacd1804503226326da80 Mon Sep 17 00:00:00 2001 From: Corinna Hillebrand Date: Wed, 27 Apr 2022 17:22:56 +0200 Subject: [PATCH] Add accessor field for payment status - it was hard to look up the payment status through the payment specific fields - enable easier lookup This commit can be reverted when doing https://github.com/squizlabs/PHP_CodeSniffer/issues/3479 --- src/Domain/Model/BankTransferPayment.php | 9 +++ src/Domain/Model/CreditCardPayment.php | 5 +- src/Domain/Model/DirectDebitPayment.php | 9 +++ src/Domain/Model/LegacyBookingStatusTrait.php | 17 ++++ src/Domain/Model/LegacyPaymentData.php | 5 +- src/Domain/Model/LegacyPaymentStatus.php | 28 +++++++ src/Domain/Model/PayPalPayment.php | 2 + src/Domain/Model/Payment.php | 11 ++- src/Domain/Model/SofortPayment.php | 6 ++ src/Services/LegacyDataProvider.php | 54 ------------- src/UseCases/GetPayment/GetPaymentUseCase.php | 3 +- .../Domain/Model/BankTransferPaymentTest.php | 23 +++++- .../Domain/Model/CreditCardPaymentTest.php | 10 +++ tests/Unit/Domain/Model/PayPalPaymentTest.php | 6 +- tests/Unit/Domain/Model/PaymentTest.php | 7 +- tests/Unit/Domain/Model/SofortPaymentTest.php | 10 +++ .../Unit/Services/LegacyDataProviderTest.php | 79 ------------------- .../GetPayment/GetPaymentUseCaseTest.php | 17 +++- 18 files changed, 158 insertions(+), 143 deletions(-) create mode 100644 src/Domain/Model/LegacyBookingStatusTrait.php create mode 100644 src/Domain/Model/LegacyPaymentStatus.php delete mode 100644 src/Services/LegacyDataProvider.php delete mode 100644 tests/Unit/Services/LegacyDataProviderTest.php diff --git a/src/Domain/Model/BankTransferPayment.php b/src/Domain/Model/BankTransferPayment.php index a02c0b43..2580a126 100644 --- a/src/Domain/Model/BankTransferPayment.php +++ b/src/Domain/Model/BankTransferPayment.php @@ -71,4 +71,13 @@ public function getDisplayValues(): array { $subtypeValues ); } + + /** + * @return string + */ + protected function getLegacyPaymentStatus(): string { + if ( $this->isCancelled() ) { return LegacyPaymentStatus::CANCELLED->value; + } + return LegacyPaymentStatus::BANK_TRANSFER->value; + } } diff --git a/src/Domain/Model/CreditCardPayment.php b/src/Domain/Model/CreditCardPayment.php index 89a559d1..9d4858c5 100644 --- a/src/Domain/Model/CreditCardPayment.php +++ b/src/Domain/Model/CreditCardPayment.php @@ -16,6 +16,8 @@ */ class CreditCardPayment extends Payment implements BookablePayment { + use LegacyBookingStatusTrait; + private const PAYMENT_METHOD = 'MCP'; /** @@ -34,7 +36,7 @@ public function getValuationDate(): ?DateTimeImmutable { return $this->valuationDate; } - private function isBooked(): bool { + public function isBooked(): bool { return $this->valuationDate !== null && !empty( $this->bookingData ); } @@ -71,5 +73,4 @@ public function getDisplayValues(): array { $subtypeValues ); } - } diff --git a/src/Domain/Model/DirectDebitPayment.php b/src/Domain/Model/DirectDebitPayment.php index 8ffb9a7f..f0e00f1a 100644 --- a/src/Domain/Model/DirectDebitPayment.php +++ b/src/Domain/Model/DirectDebitPayment.php @@ -78,4 +78,13 @@ public function getDisplayValues(): array { $subtypeValues ); } + + /** + * @return string (N is the NEW payment status (legacy) for directdebit payments) + */ + protected function getLegacyPaymentStatus(): string { + if ( $this->isCancelled() ) { return LegacyPaymentStatus::CANCELLED->value; + } + return LegacyPaymentStatus::DIRECT_DEBIT->value; + } } diff --git a/src/Domain/Model/LegacyBookingStatusTrait.php b/src/Domain/Model/LegacyBookingStatusTrait.php new file mode 100644 index 00000000..a0bacb2a --- /dev/null +++ b/src/Domain/Model/LegacyBookingStatusTrait.php @@ -0,0 +1,17 @@ +isBooked() ) { return LegacyPaymentStatus::EXTERNAL_BOOKED->value; + } + return LegacyPaymentStatus::EXTERNAL_INCOMPLETE->value; + } + +} diff --git a/src/Domain/Model/LegacyPaymentData.php b/src/Domain/Model/LegacyPaymentData.php index df5735c0..1dacf903 100644 --- a/src/Domain/Model/LegacyPaymentData.php +++ b/src/Domain/Model/LegacyPaymentData.php @@ -7,17 +7,20 @@ * This is the value object for {@see LegacyDataTransformer}. */ class LegacyPaymentData { + /** * @param int $amountInEuroCents * @param int $intervalInMonths * @param string $paymentName 3-letter payment name * @param array $paymentSpecificValues + * @param string $paymentStatus Deprecated status, should be removed when https://phabricator.wikimedia.org/T281853 is done */ public function __construct( public readonly int $amountInEuroCents, public readonly int $intervalInMonths, public readonly string $paymentName, - public readonly array $paymentSpecificValues + public readonly array $paymentSpecificValues, + public readonly string $paymentStatus ) { } } diff --git a/src/Domain/Model/LegacyPaymentStatus.php b/src/Domain/Model/LegacyPaymentStatus.php new file mode 100644 index 00000000..cfea0e96 --- /dev/null +++ b/src/Domain/Model/LegacyPaymentStatus.php @@ -0,0 +1,28 @@ +amount->getEuroCents(), $this->interval->value, $this->getPaymentName(), - $this->getPaymentSpecificLegacyData() + $this->getPaymentSpecificLegacyData(), + $this->getLegacyPaymentStatus(), ); } @@ -67,4 +68,12 @@ public function getDisplayValues(): array { "paymentType" => $this->getPaymentName() ]; } + + /** + * Get the legacy status {@see LegacyPaymentStatus} for possible values + * + * @deprecated See https://phabricator.wikimedia.org/T281853 + * @return string + */ + abstract protected function getLegacyPaymentStatus(): string; } diff --git a/src/Domain/Model/SofortPayment.php b/src/Domain/Model/SofortPayment.php index 700e9d46..d78903c1 100644 --- a/src/Domain/Model/SofortPayment.php +++ b/src/Domain/Model/SofortPayment.php @@ -12,6 +12,8 @@ class SofortPayment extends Payment implements BookablePayment { + use LegacyBookingStatusTrait; + private const PAYMENT_METHOD = 'SUB'; /** @@ -48,6 +50,10 @@ public function getValuationDate(): ?DateTimeImmutable { return $this->valuationDate; } + public function isBooked(): bool { + return $this->valuationDate !== null && $this->transactionId !== null; + } + public function canBeBooked( array $transactionData ): bool { return $this->transactionId === null; } diff --git a/src/Services/LegacyDataProvider.php b/src/Services/LegacyDataProvider.php deleted file mode 100644 index c3d11aab..00000000 --- a/src/Services/LegacyDataProvider.php +++ /dev/null @@ -1,54 +0,0 @@ -repository->getPaymentById( $paymentId ); - } catch ( PaymentNotFoundException ) { - throw new \DomainException( sprintf( - 'Payment was not found. This is a domain error, where did you get the payment ID "%d" from?', - $paymentId - ) ); - } - $legacyData = $payment->getLegacyData(); - - if ( $payment instanceof DirectDebitPayment && $payment->getIban() !== null ) { - $legacyData = $this->createExtendedLegacyData( $legacyData, $payment->getIban() ); - } - - return $legacyData; - } - - private function createExtendedLegacyData( LegacyPaymentData $legacyData, Iban $iban ): LegacyPaymentData { - $extendedBankData = $this->bankDataGenerator->getBankDataFromIban( $iban ); - return new LegacyPaymentData( - $legacyData->amountInEuroCents, - $legacyData->intervalInMonths, - $legacyData->paymentName, - [ - 'iban' => $extendedBankData->iban->toString(), - 'bic' => $extendedBankData->bic, - // "Legacy" also means German field names in this case - 'konto' => $extendedBankData->account, - 'blz' => $extendedBankData->bankCode, - 'bankname' => $extendedBankData->bankName - ] - ); - } -} diff --git a/src/UseCases/GetPayment/GetPaymentUseCase.php b/src/UseCases/GetPayment/GetPaymentUseCase.php index 358cb255..2bf8b885 100644 --- a/src/UseCases/GetPayment/GetPaymentUseCase.php +++ b/src/UseCases/GetPayment/GetPaymentUseCase.php @@ -73,7 +73,8 @@ private function createExtendedLegacyData( LegacyPaymentData $legacyData, Iban $ $legacyData->amountInEuroCents, $legacyData->intervalInMonths, $legacyData->paymentName, - $this->getLegacyBankdataFieldsArray( $extendedBankData ) + $this->getLegacyBankdataFieldsArray( $extendedBankData ), + $legacyData->paymentStatus ); } diff --git a/tests/Unit/Domain/Model/BankTransferPaymentTest.php b/tests/Unit/Domain/Model/BankTransferPaymentTest.php index 0ddacfbd..bb8510da 100644 --- a/tests/Unit/Domain/Model/BankTransferPaymentTest.php +++ b/tests/Unit/Domain/Model/BankTransferPaymentTest.php @@ -7,6 +7,7 @@ use WMDE\Euro\Euro; use WMDE\Fundraising\PaymentContext\Domain\Model\BankTransferPayment; use WMDE\Fundraising\PaymentContext\Domain\Model\LegacyPaymentData; +use WMDE\Fundraising\PaymentContext\Domain\Model\LegacyPaymentStatus; use WMDE\Fundraising\PaymentContext\Domain\Model\PaymentInterval; use WMDE\Fundraising\PaymentContext\Domain\Model\PaymentReferenceCode; @@ -38,7 +39,27 @@ public function testGetLegacyData(): void { 7821, 1, 'UEB', - [ 'ueb_code' => 'XW-TAR-ARA-X' ] + [ 'ueb_code' => 'XW-TAR-ARA-X' ], + LegacyPaymentStatus::BANK_TRANSFER->value + ); + + $this->assertEquals( $expectedLegacyData, $payment->getLegacyData() ); + } + + public function testGetLegacyDataForCancelledPayment(): void { + $payment = BankTransferPayment::create( + 1, + Euro::newFromCents( 7821 ), + PaymentInterval::Monthly, + new PaymentReferenceCode( 'XW', 'TARARA', 'X' ) + ); + $payment->cancel(); + $expectedLegacyData = new LegacyPaymentData( + 7821, + 1, + 'UEB', + [ 'ueb_code' => 'XW-TAR-ARA-X' ], + LegacyPaymentStatus::CANCELLED->value ); $this->assertEquals( $expectedLegacyData, $payment->getLegacyData() ); diff --git a/tests/Unit/Domain/Model/CreditCardPaymentTest.php b/tests/Unit/Domain/Model/CreditCardPaymentTest.php index bd04b078..22df239e 100644 --- a/tests/Unit/Domain/Model/CreditCardPaymentTest.php +++ b/tests/Unit/Domain/Model/CreditCardPaymentTest.php @@ -7,6 +7,7 @@ use PHPUnit\Framework\TestCase; use WMDE\Euro\Euro; use WMDE\Fundraising\PaymentContext\Domain\Model\CreditCardPayment; +use WMDE\Fundraising\PaymentContext\Domain\Model\LegacyPaymentStatus; use WMDE\Fundraising\PaymentContext\Domain\Model\PaymentInterval; use WMDE\Fundraising\PaymentContext\Tests\Data\CreditCardPaymentBookingData; use WMDE\Fundraising\PaymentContext\Tests\Fixtures\DummyPaymentIdRepository; @@ -84,6 +85,15 @@ public function testGivenNewPayment_paymentLegacyDataIsEmptyArray(): void { $this->assertSame( [], $creditCardPayment->getLegacyData()->paymentSpecificValues ); } + public function testStatusInLegacyDataChangesWithBookedStatus(): void { + $creditCardPayment = new CreditCardPayment( 1, Euro::newFromInt( 1000 ), PaymentInterval::Monthly ); + $bookedCreditCardPayment = new CreditCardPayment( 1, Euro::newFromInt( 1000 ), PaymentInterval::Monthly ); + $bookedCreditCardPayment->bookPayment( CreditCardPaymentBookingData::newValidBookingData(), new DummyPaymentIdRepository() ); + + $this->assertSame( LegacyPaymentStatus::EXTERNAL_INCOMPLETE->value, $creditCardPayment->getLegacyData()->paymentStatus ); + $this->assertSame( LegacyPaymentStatus::EXTERNAL_BOOKED->value, $bookedCreditCardPayment->getLegacyData()->paymentStatus ); + } + public function testGetDisplayDataReturnsAllFieldsToDisplayForBookedPayment(): void { $creditCardPayment = new CreditCardPayment( 1, Euro::newFromInt( 1000 ), PaymentInterval::Monthly ); $creditCardPayment->bookPayment( CreditCardPaymentBookingData::newValidBookingData(), new DummyPaymentIdRepository() ); diff --git a/tests/Unit/Domain/Model/PayPalPaymentTest.php b/tests/Unit/Domain/Model/PayPalPaymentTest.php index 985dfc0f..7d528a10 100644 --- a/tests/Unit/Domain/Model/PayPalPaymentTest.php +++ b/tests/Unit/Domain/Model/PayPalPaymentTest.php @@ -5,6 +5,7 @@ use PHPUnit\Framework\TestCase; use WMDE\Euro\Euro; +use WMDE\Fundraising\PaymentContext\Domain\Model\LegacyPaymentStatus; use WMDE\Fundraising\PaymentContext\Domain\Model\PaymentInterval; use WMDE\Fundraising\PaymentContext\Domain\Model\PayPalPayment; use WMDE\Fundraising\PaymentContext\Domain\Repositories\PaymentIDRepository; @@ -120,13 +121,14 @@ public function testCreateFollowupDisallowsFollowUpsFromNonRecurringPayments(): ); } - public function testGetLegacyDataIsEmptyForUnbookedPayments(): void { + public function testGetLegacyDataForUnbookedPayments(): void { $payment = new PayPalPayment( 1, Euro::newFromCents( 1000 ), PaymentInterval::OneTime ); $legacyData = $payment->getLegacyData(); $this->assertSame( [], $legacyData->paymentSpecificValues ); $this->assertSame( 'PPL', $legacyData->paymentName ); + $this->assertSame( LegacyPaymentStatus::EXTERNAL_INCOMPLETE->value, $legacyData->paymentStatus ); } public function testGetLegacyDataHasDataForBookedPayments(): void { @@ -140,6 +142,8 @@ public function testGetLegacyDataHasDataForBookedPayments(): void { $this->assertSame( '42', $legacyData->paymentSpecificValues['paypal_payer_id'] ); $this->assertSame( '8RHHUM3W3PRH7QY6B59', $legacyData->paymentSpecificValues['ext_subscr_id'] ); $this->assertSame( '2022-01-01 01:01:01', $legacyData->paymentSpecificValues['ext_payment_timestamp'] ); + // Check booked status + $this->assertSame( LegacyPaymentStatus::EXTERNAL_BOOKED->value, $legacyData->paymentStatus ); } public function testGetDisplayDataReturnsAllFieldsToDisplayForBookedPayment(): void { diff --git a/tests/Unit/Domain/Model/PaymentTest.php b/tests/Unit/Domain/Model/PaymentTest.php index 5c0e5ec3..9a2852c7 100644 --- a/tests/Unit/Domain/Model/PaymentTest.php +++ b/tests/Unit/Domain/Model/PaymentTest.php @@ -28,7 +28,8 @@ public function testGetLegacyDataCollectsPaymentInformation(): void { 1199, 0, 'TST', - [ 'value' => 'infinite' ] + [ 'value' => 'infinite' ], + 'Y' ); $this->assertEquals( $expectedLegacyData, $payment->getLegacyData() ); @@ -76,6 +77,10 @@ protected function getPaymentName(): string { protected function getPaymentSpecificLegacyData(): array { return [ 'value' => 'infinite' ]; } + + protected function getLegacyPaymentStatus(): string { + return 'Y'; + } }; } } diff --git a/tests/Unit/Domain/Model/SofortPaymentTest.php b/tests/Unit/Domain/Model/SofortPaymentTest.php index 9095ba96..0072829b 100644 --- a/tests/Unit/Domain/Model/SofortPaymentTest.php +++ b/tests/Unit/Domain/Model/SofortPaymentTest.php @@ -6,6 +6,7 @@ use PHPUnit\Framework\TestCase; use WMDE\Euro\Euro; +use WMDE\Fundraising\PaymentContext\Domain\Model\LegacyPaymentStatus; use WMDE\Fundraising\PaymentContext\Domain\Model\PaymentInterval; use WMDE\Fundraising\PaymentContext\Domain\Model\PaymentReferenceCode; use WMDE\Fundraising\PaymentContext\Domain\Model\SofortPayment; @@ -126,6 +127,15 @@ public function testBookedPaymentHasTransactionDataInLegacyData(): void { $this->assertEquals( $expectedLegacyData, $legacyData->paymentSpecificValues ); } + public function testStatusInLegacyDataChangesWithBookedStatus(): void { + $sofortPayment = $this->makeSofortPayment(); + $bookedSofortPayment = $this->makeSofortPayment(); + $bookedSofortPayment->bookPayment( $this->makeValidTransactionData(), new DummyPaymentIdRepository() ); + + $this->assertSame( LegacyPaymentStatus::EXTERNAL_INCOMPLETE->value, $sofortPayment->getLegacyData()->paymentStatus ); + $this->assertSame( LegacyPaymentStatus::EXTERNAL_BOOKED->value, $bookedSofortPayment->getLegacyData()->paymentStatus ); + } + public function testGetDisplayDataReturnsAllFieldsToDisplay(): void { $payment = $this->makeSofortPayment(); $payment->bookPayment( $this->makeValidTransactionData(), new DummyPaymentIdRepository() ); diff --git a/tests/Unit/Services/LegacyDataProviderTest.php b/tests/Unit/Services/LegacyDataProviderTest.php deleted file mode 100644 index 68917d59..00000000 --- a/tests/Unit/Services/LegacyDataProviderTest.php +++ /dev/null @@ -1,79 +0,0 @@ -createStub( CreditCardPayment::class ); - $payment->method( 'getLegacyData' )->willReturn( $legacyPaymentData ); - $provider = new LegacyDataProvider( new PaymentRepositorySpy( [ 7 => $payment ] ), $this->makeBankDataGeneratorDummy() ); - - $legacyData = $provider->getDataForPayment( 7 ); - - $this->assertSame( $legacyPaymentData, $legacyData ); - } - - public function testGivenPaymentNotFound_willThrowException(): void { - $repo = $this->createStub( PaymentRepository::class ); - $repo->method( 'getPaymentById' )->willThrowException( new PaymentNotFoundException() ); - $provider = new LegacyDataProvider( $repo, $this->makeBankDataGeneratorDummy() ); - - $this->expectException( \DomainException::class ); - - $provider->getDataForPayment( 7 ); - } - - public function testGivenADirectDebitPayment_itLooksUpAdditionalBankData(): void { - $legacyPaymentData = new LegacyPaymentData( 1299, 12, 'BEZ', [ 'iban' => 'DE02100500000054540402' ] ); - $payment = $this->createStub( DirectDebitPayment::class ); - $payment->method( 'getLegacyData' )->willReturn( $legacyPaymentData ); - $payment->method( 'getIban' )->willReturn( new Iban( 'DE02100500000054540402' ) ); - $provider = new LegacyDataProvider( new PaymentRepositorySpy( [ 7 => $payment ] ), $this->makeBankDataGeneratorStub() ); - - $legacyData = $provider->getDataForPayment( 7 ); - - $this->assertNotSame( $legacyPaymentData, $legacyData ); - $this->assertSame( 'DE02100500000054540402', $legacyData->paymentSpecificValues['iban'] ); - $this->assertSame( '10050000', $legacyData->paymentSpecificValues['blz'] ); - $this->assertSame( '0054540402', $legacyData->paymentSpecificValues['konto'] ); - $this->assertEquals( 'Landesbank Berlin', $legacyData->paymentSpecificValues['bankname'] ); - $this->assertEquals( 'BELADEBE', $legacyData->paymentSpecificValues['bic'] ); - } - - private function makeBankDataGeneratorStub(): BankDataGenerator { - $generator = $this->createStub( BankDataGenerator::class ); - $generator->method( 'getBankDataFromIban' ) - ->willReturn( new ExtendedBankData( - new Iban( 'DE02100500000054540402' ), - 'BELADEBE', - '0054540402', - '10050000', - 'Landesbank Berlin' - ) ); - return $generator; - } - - private function makeBankDataGeneratorDummy(): BankDataGenerator { - $generator = $this->createStub( BankDataGenerator::class ); - $generator->method( 'getBankDataFromIban' ) - ->willThrowException( new \LogicException( 'Code should not call this' ) ); - return $generator; - } -} diff --git a/tests/Unit/UseCases/GetPayment/GetPaymentUseCaseTest.php b/tests/Unit/UseCases/GetPayment/GetPaymentUseCaseTest.php index f7d7659f..7d18378c 100644 --- a/tests/Unit/UseCases/GetPayment/GetPaymentUseCaseTest.php +++ b/tests/Unit/UseCases/GetPayment/GetPaymentUseCaseTest.php @@ -13,6 +13,7 @@ use WMDE\Fundraising\PaymentContext\Domain\Model\ExtendedBankData; use WMDE\Fundraising\PaymentContext\Domain\Model\Iban; use WMDE\Fundraising\PaymentContext\Domain\Model\LegacyPaymentData; +use WMDE\Fundraising\PaymentContext\Domain\Model\LegacyPaymentStatus; use WMDE\Fundraising\PaymentContext\Domain\Model\PaymentInterval; use WMDE\Fundraising\PaymentContext\Domain\PaymentRepository; use WMDE\Fundraising\PaymentContext\Tests\Fixtures\PaymentRepositorySpy; @@ -63,7 +64,13 @@ public function testGivenPaymentNotFound_ArrayMethodWillThrowException(): void { } public function testGivenAPaymentId_itReturnsLegacyDataForPayment(): void { - $legacyPaymentData = new LegacyPaymentData( 1299, 12, 'MCP', [] ); + $legacyPaymentData = new LegacyPaymentData( + 1299, + 12, + 'MCP', + [], + LegacyPaymentStatus::EXTERNAL_INCOMPLETE->value + ); $payment = $this->createStub( CreditCardPayment::class ); $payment->method( 'getLegacyData' )->willReturn( $legacyPaymentData ); $useCase = new GetPaymentUseCase( new PaymentRepositorySpy( [ 7 => $payment ] ), $this->makeBankDataGeneratorDummy() ); @@ -84,7 +91,13 @@ public function testGivenPaymentNotFound_LegacyObjectMethodWillThrowException(): } public function testGivenADirectDebitPayment_itLooksUpAdditionalBankData(): void { - $legacyPaymentData = new LegacyPaymentData( 1299, 12, 'BEZ', [ 'iban' => 'DE02100500000054540402' ] ); + $legacyPaymentData = new LegacyPaymentData( + 1299, + 12, + 'BEZ', + [ 'iban' => 'DE02100500000054540402' ], + LegacyPaymentStatus::DIRECT_DEBIT->value + ); $payment = $this->createStub( DirectDebitPayment::class ); $payment->method( 'getLegacyData' )->willReturn( $legacyPaymentData ); $payment->method( 'getIban' )->willReturn( new Iban( 'DE02100500000054540402' ) );