Merge pull request #228 from wmde/moderate-emails

Implement proper email block list

src/Domain/Model/Donation.php

@@ -192,6 +192,18 @@ public function getOptsIntoDonationReceipt(): bool {
 		return $this->donor->wantsReceipt();
 	}
 
+	public function shouldSendConfirmationMail(): bool {
+		if ( !$this->getDonor()->hasEmailAddress() ) {
+			return false;
+		}
+		foreach ( $this->moderationReasons as $moderationReason ) {
+			if ( $moderationReason->getModerationIdentifier() === ModerationIdentifier::EMAIL_BLOCKED ) {
+				return false;
+			}
+		}
+		return true;
+	}
+
 	public function donorIsAnonymous(): bool {
 		return $this->donor instanceof AnonymousDonor;
 	}

src/Domain/Model/ModerationIdentifier.php

@@ -8,4 +8,5 @@ enum ModerationIdentifier {
 	case ADDRESS_CONTENT_VIOLATION;
 	case COMMENT_CONTENT_VIOLATION;
 	case MANUALLY_FLAGGED_BY_ADMIN;
+	case EMAIL_BLOCKED;
 }

src/UseCases/AddDonation/AddDonationUseCase.php

@@ -71,10 +71,6 @@ public function addDonation( AddDonationRequest $donationRequest ): AddDonationR
 			$donation->markForModeration( ...$moderationResult->getViolations() );
 		}
 
-		if ( $this->policyValidator->isAutoDeleted( $donationRequest ) ) {
-			$donation->cancelWithoutChecks();
-		}
-
 		$this->donationRepository->storeDonation( $donation );
 
 		$this->eventEmitter->emit( new DonationCreatedEvent( $donation->getId(), $donation->getDonor() ) );
@@ -159,7 +155,7 @@ private function newTrackingInfoFromRequest( AddDonationRequest $request ): Dona
 	}
 
 	private function sendDonationConfirmationEmail( Donation $donation, bool $paymentIsComplete ): void {
-		if ( $donation->getDonor()->hasEmailAddress() && $paymentIsComplete ) {
+		if ( $donation->shouldSendConfirmationMail() && $paymentIsComplete ) {
 			$this->notifier->sendConfirmationFor( $donation );
 		}
 	}

src/UseCases/AddDonation/Moderation/ModerationService.php

@@ -41,6 +41,7 @@ class ModerationService {
 
 	private AmountPolicyValidator $amountPolicyValidator;
 	private TextPolicyValidator $textPolicyValidator;
+
 	/**
 	 * @var string[]
 	 */
@@ -51,7 +52,7 @@ class ModerationService {
 	/**
 	 * @param AmountPolicyValidator $amountPolicyValidator
 	 * @param TextPolicyValidator $textPolicyValidator
-	 * @param string[] $forbiddenEmailAddresses
+	 * @param string[] $forbiddenEmailAddresses A list of email addresses
 	 */
 	public function __construct( AmountPolicyValidator $amountPolicyValidator, TextPolicyValidator $textPolicyValidator,
 		array $forbiddenEmailAddresses = [] ) {
@@ -73,8 +74,9 @@ public function moderateDonationRequest( AddDonationRequest $request ): Moderati
 			return $this->result;
 		}
 
-		$this->getAmountViolations( $paymentParameters );
-		$this->getBadWordViolations( $request );
+		$this->moderateAmountViolations( $paymentParameters );
+		$this->moderateBadWordViolations( $request );
+		$this->moderateForbiddenEmailViolations( $request );
 
 		return $this->result;
 	}
@@ -83,30 +85,6 @@ public function needsModeration( AddDonationRequest $request ): bool {
 		return $this->moderateDonationRequest( $request )->needsModeration();
 	}
 
-	/**
-	 * Indicate go-ahead for deleting donations where the email is on the forbidden list.
-	 *
-	 * When the request indicates that the donor is anonymous, don't check the list.
-	 * This behavior ensures that even when the frontend sends form data,
-	 * it will not lead to validation for anonymous users.
-	 *
-	 * @param AddDonationRequest $request
-	 * @return bool
-	 * @deprecated This has not been used after 2016 and might be removed. See https://phabricator.wikimedia.org/T280391
-	 */
-	public function isAutoDeleted( AddDonationRequest $request ): bool {
-		if ( $request->donorIsAnonymous() ) {
-			return false;
-		}
-		foreach ( $this->forbiddenEmailAddresses as $blocklistEntry ) {
-			if ( preg_match( $blocklistEntry, $request->getDonorEmailAddress() ) ) {
-				return true;
-			}
-		}
-
-		return false;
-	}
-
 	/**
 	 * Validate address fields with text policy (allow- and deny lists).
 	 *
@@ -116,22 +94,22 @@ public function isAutoDeleted( AddDonationRequest $request ): bool {
 	 *
 	 * @param AddDonationRequest $request
 	 */
-	private function getBadWordViolations( AddDonationRequest $request ): void {
+	private function moderateBadWordViolations( AddDonationRequest $request ): void {
 		if ( $request->donorIsAnonymous() ) {
 			return;
 		}
 
-		$this->getPolicyViolationsForField( $request->getDonorFirstName(), Result::SOURCE_DONOR_FIRST_NAME );
-		$this->getPolicyViolationsForField( $request->getDonorLastName(), Result::SOURCE_DONOR_LAST_NAME );
-		$this->getPolicyViolationsForField( $request->getDonorCompany(), Result::SOURCE_DONOR_COMPANY );
-		$this->getPolicyViolationsForField(
+		$this->addAddressPolicyViolationsForField( $request->getDonorFirstName(), Result::SOURCE_DONOR_FIRST_NAME );
+		$this->addAddressPolicyViolationsForField( $request->getDonorLastName(), Result::SOURCE_DONOR_LAST_NAME );
+		$this->addAddressPolicyViolationsForField( $request->getDonorCompany(), Result::SOURCE_DONOR_COMPANY );
+		$this->addAddressPolicyViolationsForField(
 			$request->getDonorStreetAddress(),
 			Result::SOURCE_DONOR_STREET_ADDRESS
 		);
-		$this->getPolicyViolationsForField( $request->getDonorCity(), Result::SOURCE_DONOR_CITY );
+		$this->addAddressPolicyViolationsForField( $request->getDonorCity(), Result::SOURCE_DONOR_CITY );
 	}
 
-	private function getPolicyViolationsForField( string $fieldContent, string $fieldName ): void {
+	private function addAddressPolicyViolationsForField( string $fieldContent, string $fieldName ): void {
 		if ( $fieldContent === '' ) {
 			return;
 		}
@@ -141,7 +119,7 @@ private function getPolicyViolationsForField( string $fieldContent, string $fiel
 		$this->result->addModerationReason( new ModerationReason( ModerationIdentifier::ADDRESS_CONTENT_VIOLATION, $fieldName ) );
 	}
 
-	private function getAmountViolations( PaymentParameters $request ): void {
+	private function moderateAmountViolations( PaymentParameters $request ): void {
 		$amountViolations = $this->amountPolicyValidator->validate(
 			Euro::newFromCents( $request->amountInEuroCents )->getEuroFloat(),
 			$request->interval
@@ -156,4 +134,12 @@ private function getAmountViolations( PaymentParameters $request ): void {
 	private function paymentTypeBypassesModeration( string $paymentType ): bool {
 		return in_array( $paymentType, self::PAYMENT_TYPES_THAT_SKIP_MODERATION );
 	}
+
+	private function moderateForbiddenEmailViolations( AddDonationRequest $request ): void {
+		if ( !$request->donorIsAnonymous() && in_array( $request->getDonorEmailAddress(), $this->forbiddenEmailAddresses ) ) {
+			$this->result->addModerationReason(
+				new ModerationReason( ModerationIdentifier::EMAIL_BLOCKED, Result::SOURCE_DONOR_EMAIL )
+			);
+		}
+	}
 }

tests/Integration/UseCases/AddDonation/AddDonationUseCaseTest.php

@@ -175,6 +175,19 @@ public function testGivenValidRequest_withIncompletePayment_confirmationEmailIsN
 		$useCase->addDonation( $request );
 	}
 
+	public function testGivenValidRequest_withBlockedEmail_confirmationEmailIsNotSent(): void {
+		$mockNotifier = $this->createMock( DonationNotifier::class );
+		$mockNotifier->expects( $this->never() )->method( 'sendConfirmationFor' );
+
+		$useCase = $this->makeUseCase(
+			policyValidator: $this->makeEmailBlockedModerationService(),
+			notifier: $mockNotifier
+		);
+
+		$request = $this->newValidAddDonationRequestWithEmail( '[email protected]' );
+		$useCase->addDonation( $request );
+	}
+
 	public function testGivenValidRequestWithPolicyViolation_donationIsModerated(): void {
 		$useCase = $this->makeUseCase( policyValidator: $this->makeFakeFailingModerationService() );
 
@@ -291,21 +304,6 @@ public function testEventIsEmittedAfterDonationWasStored(): void {
 		$this->assertInstanceOf( CompanyContactName::class, $events[0]->getDonor()->getName() );
 	}
 
-	public function testWhenEmailAddressIsBlacklisted_donationIsMarkedAsCancelled(): void {
-		$repository = $this->makeDonationRepositoryStub();
-		$useCase = $this->makeUseCase(
-			idGenerator: new StaticDonationIdRepository(),
-			repository: $repository,
-			policyValidator: $this->makeFakeAutodeletingPolicyValidator()
-		);
-
-		$useCase->addDonation( $this->newValidAddDonationRequestWithEmail( '[email protected]' ) );
-		$donation = $repository->getDonationById( StaticDonationIdRepository::DONATION_ID );
-
-		$this->assertNotNull( $donation );
-		$this->assertTrue( $donation->isCancelled() );
-	}
-
 	public function testOptingIntoDonationReceipt_persistedInDonor(): void {
 		$repository = $this->makeDonationRepositoryStub();
 		$useCase = $this->makeUseCase(
@@ -426,10 +424,11 @@ private function makeFakeFailingModerationService(): ModerationService {
 		return $validator;
 	}
 
-	private function makeFakeAutodeletingPolicyValidator(): ModerationService {
+	private function makeEmailBlockedModerationService(): ModerationService {
+		$result = new ModerationResult();
+		$result->addModerationReason( new ModerationReason( ModerationIdentifier::EMAIL_BLOCKED ) );
 		$validator = $this->createStub( ModerationService::class );
-		$validator->method( 'moderateDonationRequest' )->willReturn( new ModerationResult() );
-		$validator->method( 'isAutoDeleted' )->willReturn( true );
+		$validator->method( 'moderateDonationRequest' )->willReturn( $result );
 		return $validator;
 	}
 

tests/Integration/UseCases/AddDonation/ModerationServiceTest.php

@@ -6,7 +6,9 @@
 
 use PHPUnit\Framework\TestCase;
 use WMDE\Fundraising\DonationContext\Domain\Model\DonorType;
+use WMDE\Fundraising\DonationContext\Domain\Model\ModerationIdentifier;
 use WMDE\Fundraising\DonationContext\Tests\Data\ValidAddDonationRequest;
+use WMDE\Fundraising\DonationContext\UseCases\AddDonation\AddDonationValidationResult;
 use WMDE\Fundraising\DonationContext\UseCases\AddDonation\Moderation\ModerationService;
 use WMDE\Fundraising\PaymentContext\UseCases\CreatePayment\PaymentParameters;
 use WMDE\FunValidators\ConstraintViolation;
@@ -103,12 +105,12 @@ private function newFailingTextPolicyValidator(): TextPolicyValidator {
 	}
 
 	/** @dataProvider allowedEmailAddressProvider */
-	public function testWhenEmailAddressIsNotForbidden_isAutoDeletedReturnsFalse( string $emailAddress ): void {
+	public function testWhenEmailAddressIsNotOnBlockList_needsModerationReturnsFalse( string $emailAddress ): void {
 		$policyValidator = $this->newPolicyValidatorWithForbiddenEmails();
 		$request = ValidAddDonationRequest::getRequest();
 		$request->setDonorEmailAddress( $emailAddress );
 
-		$this->assertFalse( $policyValidator->isAutoDeleted( ValidAddDonationRequest::getRequest() ) );
+		$this->assertFalse( $policyValidator->needsModeration( $request ) );
 	}
 
 	/**
@@ -123,12 +125,17 @@ public static function allowedEmailAddressProvider(): array {
 	}
 
 	/** @dataProvider forbiddenEmailsProvider */
-	public function testWhenEmailAddressIsForbidden_isAutoDeletedReturnsTrue( string $emailAddress ): void {
+	public function testWhenEmailAddressIsOnBlockList_needsModerationReturnsTrue( string $emailAddress ): void {
 		$policyValidator = $this->newPolicyValidatorWithForbiddenEmails();
 		$request = ValidAddDonationRequest::getRequest();
 		$request->setDonorEmailAddress( $emailAddress );
 
-		$this->assertTrue( $policyValidator->isAutoDeleted( $request ) );
+		$result = $policyValidator->moderateDonationRequest( $request );
+
+		$this->assertTrue( $result->needsModeration() );
+		$this->assertCount( 1, $result->getViolations() );
+		$this->assertSame( ModerationIdentifier::EMAIL_BLOCKED, $result->getViolations()[0]->getModerationIdentifier() );
+		$this->assertSame( AddDonationValidationResult::SOURCE_DONOR_EMAIL, $result->getViolations()[0]->getSource() );
 	}
 
 	/**
@@ -137,25 +144,24 @@ public function testWhenEmailAddressIsForbidden_isAutoDeletedReturnsTrue( string
 	public static function forbiddenEmailsProvider(): array {
 		return [
 			[ '[email protected]' ],
-			[ '[email protected]' ],
-			[ '[email protected]' ]
+			[ '[email protected]' ]
 		];
 	}
 
 	private function newPolicyValidatorWithForbiddenEmails(): ModerationService {
 		return new ModerationService(
 			$this->newSucceedingAmountValidator(),
 			$this->newSucceedingTextPolicyValidator(),
-			[ '/^blocked.person@bar\.baz$/', '/@example.com$/i' ]
+			[ '[email protected]', 'foo@example.com' ]
 		);
 	}
 
-	public function testGivenAnonymousDonorWithEmailData_itIgnoresForbiddenEmails(): void {
+	public function testGivenAnonymousDonorWithEmailData_itDoesNotModerateEmail(): void {
 		$policyValidator = $this->newPolicyValidatorWithForbiddenEmails();
 		$request = ValidAddDonationRequest::getRequest();
 		$request->setDonorType( DonorType::ANONYMOUS() );
 		$request->setDonorEmailAddress( '[email protected]' );
 
-		$this->assertFalse( $policyValidator->isAutoDeleted( $request ) );
+		$this->assertFalse( $policyValidator->needsModeration( $request ) );
 	}
 }

tests/Unit/Domain/Model/DonationTest.php

@@ -140,4 +140,21 @@ public function testMarkForModerationNeedsAtLeastOneModerationReason(): void {
 		$donation->markForModeration();
 	}
 
+	public function testAnonymousDonorsShouldNotReceiveConfirmationEmail(): void {
+		$donation = ValidDonation::newIncompleteAnonymousPayPalDonation();
+		$this->assertFalse( $donation->shouldSendConfirmationMail() );
+	}
+
+	public function testDonationWithEmailModerationShouldNotReceiveConfirmationEmail(): void {
+		$donation = ValidDonation::newDirectDebitDonation();
+		$donation->markForModeration( new ModerationReason( ModerationIdentifier::EMAIL_BLOCKED ) );
+		$this->assertFalse( $donation->shouldSendConfirmationMail() );
+	}
+
+	public function testDonationWithEmailAndOtherModerationReasonsShouldReceiveConfirmationEmail(): void {
+		$donation = ValidDonation::newDirectDebitDonation();
+		$donation->markForModeration( new ModerationReason( ModerationIdentifier::MANUALLY_FLAGGED_BY_ADMIN ) );
+		$this->assertTrue( $donation->shouldSendConfirmationMail() );
+	}
+
 }