[ticket/11103] Working on the add/update notifications functions

Some cleanup and additional commenting as well

PHPBB3-11103

phpBB/includes/notifications/method/base.php

@@ -7,6 +7,8 @@
 *
 */
 
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+
 /**
 * @ignore
 */
@@ -25,21 +27,7 @@ abstract class phpbb_notifications_method_base implements phpbb_notifications_me
 	protected $db;
 	protected $user;
 
-	/**
+	public function __construct(ContainerBuilder $phpbb_container, $data = array())
-	* notification_id
-	* item_type
-	* item_id
-	*
-	* by_user_id (one who caused the notification)
-	* user_id
-	* time
-	* unread
-	*
-	* data (special serialized field that each notification type can use to store stuff)
-	*/
-	protected $data = array();
-
-	public function __construct(Symfony\Component\DependencyInjection\ContainerBuilder $phpbb_container, $data = array())
 	{
 		// phpBB Container
 		$this->phpbb_container = $phpbb_container;

phpBB/includes/notifications/service.php

@@ -7,6 +7,8 @@
 *
 */
 
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+
 /**
 * @ignore
 */
@@ -50,7 +52,7 @@ class phpbb_notifications_service
 	*	sms?
 	*/
 
-	public function __construct(Symfony\Component\DependencyInjection\ContainerBuilder $phpbb_container)
+	public function __construct(ContainerBuilder $phpbb_container)
 	{
 		$this->phpbb_container = $phpbb_container;
 
@@ -58,23 +60,13 @@ class phpbb_notifications_service
 		$this->db = $phpbb_container->get('dbal.conn');
 	}
 
-	private function get_type_class_name(&$type, $safe = false)
-	{
-		if (!$safe)
-		{
-			$type = preg_replace('#[^a-z]#', '', $type);
-		}
-
-		return 'phpbb_notifications_type_' . $type;
-	}
-
 	/**
 	* Load the user's notifications
 	*
 	* @param array $options Optional options to control what notifications are loaded
-	*					user_id		User id to load notifications for (Default: $user->data['user_id'])
+	*				user_id		User id to load notifications for (Default: $user->data['user_id'])
-	* 					limit		Number of notifications to load (Default: 5)
+	* 				limit		Number of notifications to load (Default: 5)
-	* 					start		Notifications offset (Default: 0)
+	* 				start		Notifications offset (Default: 0)
 	*/
 	public function load_notifications($options = array())
 	{
@@ -128,38 +120,58 @@ class phpbb_notifications_service
 		return $notifications;
 	}
 
+	/**
+	* Add a notification
+	*
+	* @param string $type Type identifier
+	* @param int $type_id Identifier within the type
+	* @param array $data Data specific for this type that will be inserted
+	*/
 	public function add_notifications($type, $data)
 	{
 		$type_class_name = $this->get_type_class_name($type);
 
-		$notification_objects = array(); // 'user_id'	=> object
+		$notify_users = array();
-		$methods = $new_rows = array();
+		$notification_objects = $notification_methods = array();
+		$new_rows = array();
 
 		// find out which users want to receive this type of notification
 		$sql = 'SELECT user_id FROM ' . USERS_TABLE . '
 			WHERE ' . $this->db->sql_in_set('user_id', array(2));
 		$result = $this->db->sql_query($sql);
-
 		while ($row = $this->db->sql_fetchrow($result))
 		{
-			$row['method'] = '';
+			if (!isset($notify_users[$row['user_id']]))
+			{
+				$notify_users[$row['user_id']] = array();
+			}
 
+			$notify_users[$row['user_id']][] = '';
+		}
+		$this->db->sql_freeresult($result);
+
+		// Go through each user so we can insert a row in the DB and then notify them by their desired means
+		foreach ($notify_users as $user => $methods)
+		{
 			$notification = new $type_class_name($this->phpbb_container);
 
-			$notification->user_id = $row['user_id'];
+			$notification->user_id = (int) $user;
 
 			$new_rows[] = $notification->create_insert_array($data);
 
-			// setup the notification methods and add the notification to the queue
+			foreach ($methods as $method)
-			if ($row['method'])
 			{
-				if (!isset($methods[$row['method']]))
+				// setup the notification methods and add the notification to the queue
+				if ($row['method'])
 				{
-					$method_class_name = 'phpbb_notifications_method_' . $row['method'];
+					if (!isset($notification_methods[$row['method']]))
-					$methods[$row['method']] = new $$method_class_name();
+					{
-				}
+						$method_class_name = 'phpbb_notifications_method_' . $row['method'];
+						$notification_methods[$row['method']] = new $method_class_name();
+					}
 
-				$methods[$row['method']]->add_to_queue($notification);
+					$notification_methods[$row['method']]->add_to_queue($notification);
+				}
 			}
 		}
 
@@ -167,31 +179,43 @@ class phpbb_notifications_service
 		$this->db->sql_multi_insert(NOTIFICATIONS_TABLE, $new_rows);
 
 		// run the queue for each method to send notifications
-		foreach ($methods as $method)
+		foreach ($notification_methods as $method)
 		{
 			$method->run_queue();
 		}
 	}
 
+	/**
+	* Update a notification
+	*
+	* @param string $type Type identifier
+	* @param int $type_id Identifier within the type
+	* @param array $data Data specific for this type that will be updated
+	*/
 	public function update_notifications($type, $type_id, $data)
 	{
 		$type_class_name = $this->get_type_class_name($type);
 
-		$object = new $$type_class($this->phpbb_container);
+		$notification = new $type_class_name($this->phpbb_container);
-		$update = $object->update($data);
+		$update_array = $notification->create_update_array($data);
 
 		$sql = 'UPDATE ' . NOTIFICATIONS_TABLE . '
-			SET ' . $this->db->sql_build_array('UPDATE', $update) . "
+			SET ' . $this->db->sql_build_array('UPDATE', $update_array) . "
-			WHERE type = '" . $this->db->sql_escape($type) . "'
+			WHERE item_type = '" . $this->db->sql_escape($type) . "'
-				AND type_id = " . (int) $type_id;
+				AND item_id = " . (int) $type_id;
-		$result = $this->db->sql_query($sql);
+		$this->db->sql_query($sql);
+	}
 
-		while ($row = $this->db->sql_fetchrow($result))
+	/**
+	* Helper to get the notifications type class name and clean it if unsafe
+	*/
+	private function get_type_class_name(&$type, $safe = false)
+	{
+		if (!$safe)
 		{
-			$object = new $type_class_name($this->phpbb_container, $row);
+			$type = preg_replace('#[^a-z]#', '', $type);
-			$object->update($data);
-
-			$update_rows[] = $object->getForUpdate();
 		}
+
+		return 'phpbb_notifications_type_' . $type;
 	}
 }

phpBB/includes/notifications/type/base.php

@@ -7,6 +7,8 @@
 *
 */
 
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+
 /**
 * @ignore
 */
@@ -26,7 +28,12 @@ abstract class phpbb_notifications_type_base implements phpbb_notifications_type
 	protected $phpbb_root_path;
 	protected $php_ext;
 
-	protected $users;
+	/**
+	* Array of user data containing information needed to output the notifications to the template
+	*
+	* @var array
+	*/
+	protected $users = array();
 
 	/**
 	* Indentification data
@@ -40,11 +47,11 @@ abstract class phpbb_notifications_type_base implements phpbb_notifications_type
 	* data (special serialized field that each notification type can use to store stuff)
 	*
 	* @var array $data Notification row from the database
-	* 		This must be private, all interaction should use __get(), __set()
+	* 		This must be private, all interaction should use __get(), __set(), get_data(), set_data()
 	*/
 	private $data = array();
 
-	public function __construct(Symfony\Component\DependencyInjection\ContainerBuilder $phpbb_container, $data = array())
+	public function __construct(ContainerBuilder $phpbb_container, $data = array())
 	{
 		// phpBB Container
 		$this->phpbb_container = $phpbb_container;
@@ -69,16 +76,33 @@ abstract class phpbb_notifications_type_base implements phpbb_notifications_type
 		$this->data[$name] = $value;
 	}
 
-	public function get_data($name)
+	/**
+	* Get special data (only important for the classes that extend this)
+	*
+	* @param string $name Name of the variable to get
+	*
+	* @return mixed
+	*/
+	protected function get_data($name)
 	{
 		return $this->data['data'][$name];
 	}
 
-	public function set_data($name, $value)
+	/**
+	* Set special data (only important for the classes that extend this)
+	*
+	* @param string $name Name of the variable to set
+	* @param mixed $value Value to set to the variable
+	*/
+	protected function set_data($name, $value)
 	{
 		$this->data['data'][$name] = $value;
 	}
 
+	/**
+	* Function to store the users loaded from the database (for output to the template)
+	* (The service handles this)
+	*/
 	public function users(&$users)
 	{
 		$this->users = $users;
@@ -110,7 +134,15 @@ abstract class phpbb_notifications_type_base implements phpbb_notifications_type
 		));
 	}
 
-	public function create_insert_array($data)
+	/**
+	* Function for preparing the data for insertion in an SQL query
+	* (The service handles insertion)
+	*
+	* @param array $special_data Data unique to this notification type
+	*
+	* @return array Array of data ready to be inserted into the database
+	*/
+	public function create_insert_array($special_data)
 	{
 		// Defaults
 		$data = array_merge(array(
@@ -125,4 +157,26 @@ abstract class phpbb_notifications_type_base implements phpbb_notifications_type
 
 		return $data;
 	}
+
+	/**
+	* Function for preparing the data for update in an SQL query
+	* (The service handles insertion)
+	*
+	* @param array $special_data Data unique to this notification type
+	*
+	* @return array Array of data ready to be updated in the database
+	*/
+	public function create_update_array($special_data)
+	{
+		$data = $this->create_insert_array($special_data);
+
+		// Unset data unique to each row
+		unset(
+			$data['notification_id'],
+			$data['unread'],
+			$data['user_id']
+		);
+
+		return $data;
+	}
 }

phpBB/includes/notifications/type/interface.php

@@ -27,5 +27,5 @@ interface phpbb_notifications_type_interface
 
 	public function get_url();
 
-	public function create_insert_array($data);
+	public function create_insert_array($special_data);
 }

phpBB/includes/notifications/type/post.php

@@ -30,11 +30,21 @@ class phpbb_notifications_type_post extends phpbb_notifications_type_base
 		return 'post';
 	}
 
+	/**
+	* Get the title of this notification
+	*
+	* @return string
+	*/
 	public function get_title()
 	{
 		return $this->data['post_username'] . ' posted in the topic ' . censor_text($this->data['topic_title']);
 	}
 
+	/**
+	* Get the url to this item
+	*
+	* @return string URL
+	*/
 	public function get_url()
 	{
 		return append_sid($this->phpbb_root_path . 'viewtopic.' . $this->php_ext, "p={$this->item_id}#p{$this->item_id}");
@@ -50,6 +60,14 @@ class phpbb_notifications_type_post extends phpbb_notifications_type_base
 		return array($this->data['poster_id']);
 	}
 
+	/**
+	* Function for preparing the data for insertion in an SQL query
+	* (The service handles insertion)
+	*
+	* @param array $post Data from submit_post
+	*
+	* @return array Array of data ready to be inserted into the database
+	*/
 	public function create_insert_array($post)
 	{
 		$this->item_id = $post['post_id'];
@@ -60,6 +78,8 @@ class phpbb_notifications_type_post extends phpbb_notifications_type_base
 
 		$this->set_data('post_username', $post['post_username']);
 
+		$this->time = $post['post_time'];
+
 		return parent::create_insert_array($post);
 	}
 }