From 97ccb4d2c45ff705d6f776cc9a841f0680d8238f Mon Sep 17 00:00:00 2001
From: Hypolite Petovan <hypolite@mrpetovan.com>
Date: Thu, 28 Jul 2022 05:25:41 -0400
Subject: [PATCH] Make server domain pattern block reason mandatory

---
 src/Console/ServerBlock.php                  |  6 ++--
 src/Moderation/DomainPatternBlocklist.php    | 27 +++++---------
 tests/src/Console/ServerBlockConsoleTest.php | 37 +++++++-------------
 3 files changed, 25 insertions(+), 45 deletions(-)

diff --git a/src/Console/ServerBlock.php b/src/Console/ServerBlock.php
index 9ef6777db..65b806fc7 100644
--- a/src/Console/ServerBlock.php
+++ b/src/Console/ServerBlock.php
@@ -155,12 +155,12 @@ HELP;
 	 */
 	private function addBlockedServer(): int
 	{
-		if (count($this->args) < 2 || count($this->args) > 3) {
-			throw new CommandArgsException('Add needs a domain pattern and optionally a reason.');
+		if (count($this->args) != 3) {
+			throw new CommandArgsException('Add needs a domain pattern and a reason.');
 		}
 
 		$pattern = $this->getArgument(1);
-		$reason  = (count($this->args) === 3) ? $this->getArgument(2) : DomainPatternBlocklist::DEFAULT_REASON;
+		$reason  = $this->getArgument(2);
 
 		$result = $this->blocklist->addPattern($pattern, $reason);
 		if ($result) {
diff --git a/src/Moderation/DomainPatternBlocklist.php b/src/Moderation/DomainPatternBlocklist.php
index c60634687..a57b5f04c 100644
--- a/src/Moderation/DomainPatternBlocklist.php
+++ b/src/Moderation/DomainPatternBlocklist.php
@@ -31,8 +31,6 @@ use Friendica\Util\Emailer;
 
 class DomainPatternBlocklist
 {
-	const DEFAULT_REASON = 'blocked';
-
 	/** @var IManageConfigValues */
 	private $config;
 
@@ -73,11 +71,11 @@ class DomainPatternBlocklist
 	}
 
 	/**
-	 * @param string      $pattern
-	 * @param string|null $reason
+	 * @param string $pattern
+	 * @param string $reason
 	 * @return int 0 if the block list couldn't be saved, 1 if the pattern was added, 2 if it was updated in place
 	 */
-	public function addPattern(string $pattern, string $reason = null): int
+	public function addPattern(string $pattern, string $reason): int
 	{
 		$update = false;
 
@@ -86,7 +84,7 @@ class DomainPatternBlocklist
 			if ($blocked['domain'] === $pattern) {
 				$blocklist[] = [
 					'domain' => $pattern,
-					'reason' => $reason ?? self::DEFAULT_REASON,
+					'reason' => $reason,
 				];
 
 				$update = true;
@@ -98,7 +96,7 @@ class DomainPatternBlocklist
 		if (!$update) {
 			$blocklist[] = [
 				'domain' => $pattern,
-				'reason' => $reason ?? self::DEFAULT_REASON,
+				'reason' => $reason,
 			];
 		}
 
@@ -179,18 +177,11 @@ class DomainPatternBlocklist
 
 		$blocklist = [];
 		while (($data = fgetcsv($fp, 1000)) !== false) {
-			$domain = $data[0];
-			if (count($data) == 0) {
-				$reason = self::DEFAULT_REASON;
-			} else {
-				$reason = $data[1];
-			}
-
-			$data = [
-				'domain' => $domain,
-				'reason' => $reason
+			$item = [
+				'domain' => $data[0],
+				'reason' => $data[1] ?? '',
 			];
-			if (!in_array($data, $blocklist)) {
+			if (!in_array($item, $blocklist)) {
 				$blocklist[] = $data;
 			}
 		}
diff --git a/tests/src/Console/ServerBlockConsoleTest.php b/tests/src/Console/ServerBlockConsoleTest.php
index cdd7efef0..9837fbb8e 100644
--- a/tests/src/Console/ServerBlockConsoleTest.php
+++ b/tests/src/Console/ServerBlockConsoleTest.php
@@ -93,25 +93,6 @@ CONS;
 		self::assertEquals('The domain pattern \'testme.now\' is now blocked. (Reason: \'I like it!\')' . "\n", $txt);
 	}
 
-	/**
-	 * Test blockedservers add command with the default reason
-	 */
-	public function testAddBlockedServerWithDefaultReason()
-	{
-		$this->blocklistMock
-			->shouldReceive('addPattern')
-			->with('testme.now', DomainPatternBlocklist::DEFAULT_REASON)
-			->andReturn(1)
-			->once();
-
-		$console = new ServerBlock($this->blocklistMock, $this->consoleArgv);
-		$console->setArgument(0, 'add');
-		$console->setArgument(1, 'testme.now');
-		$txt = $this->dumpExecute($console);
-
-		self::assertEquals('The domain pattern \'testme.now\' is now blocked. (Reason: \'' . DomainPatternBlocklist::DEFAULT_REASON . '\')' . "\n", $txt);
-	}
-
 	/**
 	 * Test blockedservers add command on existed domain
 	 */
@@ -170,16 +151,16 @@ CONS;
 	{
 		$this->blocklistMock
 			->shouldReceive('removePattern')
-			->with('not.exiting')
+			->with('not.existing')
 			->andReturn(1)
 			->once();
 
 		$console = new ServerBlock($this->blocklistMock, $this->consoleArgv);
 		$console->setArgument(0, 'remove');
-		$console->setArgument(1, 'not.exiting');
+		$console->setArgument(1, 'not.existing');
 		$txt = $this->dumpExecute($console);
 
-		self::assertEquals('The domain pattern \'not.exiting\' wasn\'t blocked.' . "\n", $txt);
+		self::assertEquals('The domain pattern \'not.existing\' wasn\'t blocked.' . "\n", $txt);
 	}
 
 	/**
@@ -191,7 +172,14 @@ CONS;
 		$console->setArgument(0, 'add');
 		$txt = $this->dumpExecute($console);
 
-		self::assertStringStartsWith('[Warning] Add needs a domain pattern and optionally a reason.', $txt);
+		self::assertStringStartsWith('[Warning] Add needs a domain pattern and a reason.', $txt);
+
+		$console = new ServerBlock($this->blocklistMock, $this->consoleArgv);
+		$console->setArgument(0, 'add');
+		$console->setArgument(1, 'testme.now');
+		$txt = $this->dumpExecute($console);
+
+		self::assertStringStartsWith('[Warning] Add needs a domain pattern and a reason.', $txt);
 	}
 
 	/**
@@ -201,13 +189,14 @@ CONS;
 	{
 		$this->blocklistMock
 			->shouldReceive('addPattern')
-			->with('testme.now', DomainPatternBlocklist::DEFAULT_REASON)
+			->with('testme.now', 'I like it!')
 			->andReturn(0)
 			->once();
 
 		$console = new ServerBlock($this->blocklistMock, $this->consoleArgv);
 		$console->setArgument(0, 'add');
 		$console->setArgument(1, 'testme.now');
+		$console->setArgument(2, 'I like it!');
 		$txt = $this->dumpExecute($console);
 
 		self::assertEquals('Couldn\'t save \'testme.now\' as blocked domain pattern' . "\n", $txt);