Move idea logic out of the notification type and back to idea factory

Signed-off-by: Matt Friedman <maf675@gmail.com>

Author: iMattPro

config/services.yml

@@ -130,6 +130,6 @@ services:
         parent: notification.type.base
         shared: false
         calls:
-            - [set_additional_services, ['@config', '@controller.helper', '@phpbb.ideas.idea', '@user_loader']]
+            - [set_additional_services, ['@config', '@controller.helper', '@user_loader']]
         tags:
             - { name: notification.type }

factory/idea.php

@@ -72,13 +72,16 @@ public function set_status($idea_id, $status)
 		$this->update_idea_data($sql_ary, $idea_id, $this->table_ideas);
 
 		// Send a notification
+		$idea = $this->get_idea($idea_id);
 		$notifications = $this->notification_manager->get_notified_users(ext::NOTIFICATION_TYPE_STATUS, ['item_id' => (int) $idea_id]);
 		$this->notification_manager->{empty($notifications) ? 'add_notifications' : 'update_notifications'}(
 			ext::NOTIFICATION_TYPE_STATUS,
 			[
 				'idea_id' => (int) $idea_id,
 				'status'  => (int) $status,
 				'user_id' => (int) $this->user->data['user_id'],
+				'idea_author' => (int) $idea['idea_author'],
+				'idea_title' => $idea['idea_title'],
 			]
 		);
 	}

notification/type/status.php

@@ -13,7 +13,6 @@
 use phpbb\config\config;
 use phpbb\controller\helper;
 use phpbb\ideas\ext;
-use phpbb\ideas\factory\idea;
 use phpbb\user_loader;
 use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
 
@@ -28,31 +27,23 @@ class status extends \phpbb\notification\type\base
 	/** @var helper */
 	protected $helper;
 
-	/** @var idea */
-	protected $idea;
-
 	/** @var user_loader */
 	protected $user_loader;
 
 	/** @var int */
 	protected $ideas_forum_id;
 
-	/** @var array */
-	protected $idea_cache = [];
-
 	/**
 	 * Set additional services and properties
 	 *
 	 * @param config $config
 	 * @param helper $helper
-	 * @param idea $idea
 	 * @param user_loader $user_loader
 	 * @return void
 	 */
-	public function set_additional_services(config $config, helper $helper, idea $idea, user_loader $user_loader)
+	public function set_additional_services(config $config, helper $helper, user_loader $user_loader)
 	{
 		$this->helper = $helper;
-		$this->idea = $idea;
 		$this->user_loader = $user_loader;
 		$this->ideas_forum_id = (int) $config['ideas_forum_id'];
 	}
@@ -120,9 +111,7 @@ public function find_users_for_notification($type_data, $options = [])
 			'ignore_users'		=> [],
 		], $options);
 
-		$idea = $this->load_idea($type_data['idea_id']);
-
-		$users = $idea ? [$idea['idea_author']] : [];
+		$users = [$type_data['idea_author']];
 
 		return $this->get_authorised_recipients($users, $this->ideas_forum_id, $options);
 	}
@@ -206,31 +195,11 @@ public function get_email_template_variables()
 	 */
 	public function create_insert_array($type_data, $pre_create_data = [])
 	{
-		$idea = $this->load_idea($type_data['idea_id']);
-
 		$this->set_data('idea_id', (int) $type_data['idea_id']);
 		$this->set_data('status', (int) $type_data['status']);
 		$this->set_data('updater_id', (int) $type_data['user_id']);
-		$this->set_data('idea_title', $idea ? $idea['idea_title'] : '');
+		$this->set_data('idea_title', $type_data['idea_title']);
 
 		parent::create_insert_array($type_data, $pre_create_data);
 	}
-
-	/**
-	 * Load and cache an idea by id
-	 *
-	 * @param int $idea_id
-	 * @return array|false
-	 */
-	protected function load_idea($idea_id)
-	{
-		$idea_id = (int) $idea_id;
-
-		if (!array_key_exists($idea_id, $this->idea_cache))
-		{
-			$this->idea_cache[$idea_id] = $this->idea->get_idea($idea_id) ?: false;
-		}
-
-		return $this->idea_cache[$idea_id];
-	}
 }

tests/ideas/idea_attributes_test.php

@@ -72,7 +72,7 @@ public function test_set_status($idea_id, $status)
 		self::assertEquals($status, $idea['idea_status']);
 	}
 
-	public function test_set_status_notification_data()
+	public function set_status_notification_data()
 	{
 		return [
 			[1, 1, [], 'add_notifications'],
@@ -83,7 +83,7 @@ public function test_set_status_notification_data()
 	}
 
 	/**
-	 * @dataProvider test_set_status_notification_data
+	 * @dataProvider set_status_notification_data
 	 */
 	public function test_set_status_notification($idea_id, $status, $notified_users, $expected)
 	{

tests/notification/status_test.php

@@ -24,9 +24,6 @@ class status_test extends \phpbb_test_case
 	/** @var \phpbb\controller\helper|\PHPUnit\Framework\MockObject\MockObject */
 	protected $helper;
 
-	/** @var \phpbb\ideas\factory\idea|\PHPUnit\Framework\MockObject\MockObject */
-	protected $idea_factory;
-
 	/** @var \phpbb\user_loader|\PHPUnit\Framework\MockObject\MockObject */
 	protected $user_loader;
 
@@ -47,7 +44,6 @@ protected function setUp(): void
 
 		$this->config = $this->createMock(\phpbb\config\config::class);
 		$this->helper = $this->createMock(\phpbb\controller\helper::class);
-		$this->idea_factory = $this->createMock(\phpbb\ideas\factory\idea::class);
 		$this->user_loader = $this->createMock(\phpbb\user_loader::class);
 		$this->auth = $this->createMock(\phpbb\auth\auth::class);
 		$this->language = $this->createMock(\phpbb\language\language::class);
@@ -64,7 +60,7 @@ protected function setUp(): void
 			->willReturn($this->forum_id);
 
 		$this->notification_type = new status($db, $this->language, $user, $this->auth, $phpbb_root_path, $phpEx, 'phpbb_user_notifications');
-		$this->notification_type->set_additional_services($this->config, $this->helper, $this->idea_factory, $this->user_loader);
+		$this->notification_type->set_additional_services($this->config, $this->helper, $this->user_loader);
 
 		// Set protected properties using reflection
 		$reflection = new \ReflectionClass($this->notification_type);
@@ -130,20 +126,14 @@ public function test_find_users_for_notification()
 		$idea_id = 1;
 		$idea_author = 2;
 
-		$type_data = ['idea_id' => $idea_id];
-		$idea_data = ['idea_author' => $idea_author];
+		$type_data = ['idea_id' => $idea_id, 'idea_author' => $idea_author];
 		$default_methods = ['board', 'email'];
 
 		$this->auth->expects($this->once())
 			->method('acl_get_list')
 			->with([$idea_author], 'f_read', $this->forum_id)
 			->willReturn([$this->forum_id => ['f_read' => [$idea_author]]]);
 
-		$this->idea_factory->expects($this->once())
-			->method('get_idea')
-			->with($idea_id)
-			->willReturn($idea_data);
-
 		$this->notification_manager->expects($this->once())
 			->method('get_default_methods')
 			->willReturn($default_methods);
@@ -152,19 +142,6 @@ public function test_find_users_for_notification()
 		$this->assertEquals([$idea_author => $default_methods], $result);
 	}
 
-	public function test_find_users_for_notification_idea_not_found()
-	{
-		$type_data = ['idea_id' => 999];
-
-		$this->idea_factory->expects($this->once())
-			->method('get_idea')
-			->with(999)
-			->willReturn(false);
-
-		$result = $this->notification_type->find_users_for_notification($type_data);
-		$this->assertEquals([], $result);
-	}
-
 	public function test_get_avatar_with_author()
 	{
 		$this->setNotificationData(['updater_id' => 5]);
@@ -333,17 +310,10 @@ public function test_create_insert_array()
 		$type_data = [
 			'idea_id' => 7,
 			'status' => 4,
-			'user_id' => 3
-		];
-		$idea_data = [
+			'user_id' => 3,
 			'idea_title' => 'Sample Idea'
 		];
 
-		$this->idea_factory->expects($this->once())
-			->method('get_idea')
-			->with(7)
-			->willReturn($idea_data);
-
 		$this->notification_type->create_insert_array($type_data);
 
 		// Verify data was set by checking get_data