From 3315702b5f2feacbc703f5dcee5944f89cf2d407 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 7 Apr 2014 19:13:32 +0200 Subject: [PATCH 1/3] [ticket/12370] Do not delete topic notifications when the topic is visible We should only delete the topic notifications, when the topic is not approved anymore. This happens, when the post was the last approved but is now unapproved because the user is on queue, or when it has been softdeleted while editing. PHPBB3-12370 --- phpBB/includes/functions_posting.php | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/phpBB/includes/functions_posting.php b/phpBB/includes/functions_posting.php index 6a0aedf8c6..79fcb892ef 100644 --- a/phpBB/includes/functions_posting.php +++ b/phpBB/includes/functions_posting.php @@ -2272,8 +2272,10 @@ function submit_post($mode, $subject, $username, $topic_type, &$poll, &$data, $u case 'edit_first_post': case 'edit': case 'edit_last_post': - // @todo: Check whether these notification deletions are correct - $phpbb_notifications->delete_notifications('topic', $data['topic_id']); + if ($data['topic_visibility'] != ITEM_APPROVED) + { + $phpbb_notifications->delete_notifications('topic', $data['topic_id']); + } $phpbb_notifications->delete_notifications(array( 'quote', @@ -2297,8 +2299,10 @@ function submit_post($mode, $subject, $username, $topic_type, &$poll, &$data, $u case 'edit_first_post': case 'edit': case 'edit_last_post': - // @todo: Check whether these notification deletions are correct - $phpbb_notifications->delete_notifications('topic', $data['topic_id']); + if ($data['topic_visibility'] != ITEM_APPROVED) + { + $phpbb_notifications->delete_notifications('topic', $data['topic_id']); + } $phpbb_notifications->delete_notifications(array( 'quote', From 708db0f05c3d2277df005f295a229d3b2785d95f Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 7 Apr 2014 19:42:18 +0200 Subject: [PATCH 2/3] [ticket/12370] Fix functional notification test and remove unneeded requests PHPBB3-12370 --- tests/functional/notification_test.php | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/tests/functional/notification_test.php b/tests/functional/notification_test.php index 9ae37842fe..18e8a4ecc0 100644 --- a/tests/functional/notification_test.php +++ b/tests/functional/notification_test.php @@ -59,20 +59,17 @@ class phpbb_functional_notification_test extends phpbb_functional_test_case $this->create_user('notificationtestuser'); $this->add_user_group('NEWLY_REGISTERED', array('notificationtestuser')); $this->login('notificationtestuser'); - $crawler = self::request('GET', 'index.php'); - $this->assertContains('notificationtestuser', $crawler->filter('#username_logged_in')->text()); // Post a new post that needs approval $this->create_post(2, 1, 'Re: Welcome to phpBB3', 'This is a test [b]post[/b] posted by notificationtestuser.', array(), 'POST_STORED_MOD'); $crawler = self::request('GET', "viewtopic.php?t=1&sid={$this->sid}"); $this->assertNotContains('This is a test post posted by notificationtestuser.', $crawler->filter('html')->text()); - // logout - $crawler = self::request('GET', 'ucp.php?sid=' . $this->sid . '&mode=logout'); - - // admin login + // Login as admin + $this->logout(); $this->login(); $this->add_lang('ucp'); + $crawler = self::request('GET', 'ucp.php?i=ucp_notifications'); // At least one notification should exist From c99584ec7b6b32439b4af9486cddd38bf30b64d4 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 7 Apr 2014 21:29:03 +0200 Subject: [PATCH 3/3] [ticket/12370] Add unit tests for topic notifications PHPBB3-12370 --- .../fixtures/submit_post_topic.xml | 134 ++++++++++++++++ tests/notification/submit_post_base.php | 2 +- .../submit_post_type_topic_test.php | 149 ++++++++++++++++++ 3 files changed, 284 insertions(+), 1 deletion(-) create mode 100644 tests/notification/fixtures/submit_post_topic.xml create mode 100644 tests/notification/submit_post_type_topic_test.php diff --git a/tests/notification/fixtures/submit_post_topic.xml b/tests/notification/fixtures/submit_post_topic.xml new file mode 100644 index 0000000000..5e179d9b99 --- /dev/null +++ b/tests/notification/fixtures/submit_post_topic.xml @@ -0,0 +1,134 @@ + + + + forum_id + user_id + notify_status + + 1 + 6 + 0 + + + 1 + 7 + 0 + + + 1 + 8 + 0 + +
+ + notification_type_id + user_id + item_id + item_parent_id + notification_read + notification_data + + 1 + 8 + 1 + 1 + 0 + + +
+ + notification_type_id + notification_type_name + notification_type_enabled + + 1 + topic + 1 + +
+ + post_id + topic_id + forum_id + post_text + + 1 + 1 + 1 + + +
+ + topic_id + forum_id + + 1 + 1 + +
+ + user_id + username_clean + user_permissions + user_sig + + 2 + poster + + + + + 6 + noauth + + + + + 7 + default + + + + + 8 + notified + + + +
+ + item_type + item_id + user_id + method + notify + + topic + 0 + 2 + + 1 + + + topic + 0 + 6 + + 1 + + + topic + 0 + 7 + + 1 + + + topic + 0 + 8 + + 1 + +
+ diff --git a/tests/notification/submit_post_base.php b/tests/notification/submit_post_base.php index 10d676c687..fbfd034381 100644 --- a/tests/notification/submit_post_base.php +++ b/tests/notification/submit_post_base.php @@ -101,7 +101,7 @@ abstract class phpbb_notification_submit_post_base extends phpbb_database_test_c $user_loader = new \phpbb\user_loader($db, $phpbb_root_path, $phpEx, USERS_TABLE); // Notification Types - $notification_types = array('quote', 'bookmark', 'post', 'post_in_queue', 'topic', 'approve_topic', 'approve_post'); + $notification_types = array('quote', 'bookmark', 'post', 'post_in_queue', 'topic', 'topic_in_queue', 'approve_topic', 'approve_post'); $notification_types_array = array(); foreach ($notification_types as $type) { diff --git a/tests/notification/submit_post_type_topic_test.php b/tests/notification/submit_post_type_topic_test.php new file mode 100644 index 0000000000..29f67c9c78 --- /dev/null +++ b/tests/notification/submit_post_type_topic_test.php @@ -0,0 +1,149 @@ +expects($this->any()) + ->method('acl_get_list') + ->with($this->anything(), + $this->stringContains('_'), + $this->greaterThan(0)) + ->will($this->returnValueMap(array( + array( + array(6, 7, 8), + 'f_read', + 1, + array( + 1 => array( + 'f_read' => array(7, 8), + ), + ), + ), + ))); + + $phpbb_log = $this->getMock('\phpbb\log\null'); + } + + /** + * submit_post() Notifications test + * + * submit_post() $mode = 'post' + * Notification item_type = 'topic' + */ + public function submit_post_data() + { + return array( + /** + * Normal post + * + * User => State description + * 2 => Poster, should NOT receive a notification + * 6 => Forum subscribed, but no-auth reading, should NOT receive a notification + * 7 => Forum subscribed, should receive a notification + * 8 => Forum subscribed, but already notified, should NOT receive a new notification + */ + array( + array(), + array( + array('user_id' => 8, 'item_id' => 1, 'item_parent_id' => 1), + ), + array( + array('user_id' => 7, 'item_id' => 2, 'item_parent_id' => 1), + array('user_id' => 8, 'item_id' => 1, 'item_parent_id' => 1), + array('user_id' => 8, 'item_id' => 2, 'item_parent_id' => 1), + ), + ), + + /** + * Unapproved post + * + * No new notifications + */ + array( + array('force_approved_state' => false), + array( + array('user_id' => 8, 'item_id' => 1, 'item_parent_id' => 1), + ), + array( + array('user_id' => 8, 'item_id' => 1, 'item_parent_id' => 1), + ), + ), + ); + } + + /** + * @dataProvider submit_post_data + */ + public function test_submit_post($additional_post_data, $expected_before, $expected_after) + { + $sql = 'SELECT user_id, item_id, item_parent_id + FROM ' . NOTIFICATIONS_TABLE . ' n, ' . NOTIFICATION_TYPES_TABLE . " nt + WHERE nt.notification_type_name = '" . $this->item_type . "' + AND n.notification_type_id = nt.notification_type_id + ORDER BY user_id ASC, item_id ASC"; + $result = $this->db->sql_query($sql); + $this->assertEquals($expected_before, $this->db->sql_fetchrowset($result)); + $this->db->sql_freeresult($result); + + $poll_data = array(); + $post_data = array_merge($this->post_data, $additional_post_data); + submit_post('post', '', 'poster-name', POST_NORMAL, $poll_data, $post_data, false, false); + + // Check whether the notifications got added successfully + $result = $this->db->sql_query($sql); + $this->assertEquals($expected_after, $this->db->sql_fetchrowset($result), + 'Check whether the notifications got added successfully'); + $this->db->sql_freeresult($result); + + if (isset($additional_post_data['force_approved_state']) && $additional_post_data['force_approved_state'] === false) + { + return; + } + + $reply_data = array_merge($this->post_data, array( + 'topic_id' => 2, + )); + $url = submit_post('reply', '', 'poster-name', POST_NORMAL, $poll_data, $reply_data, false, false); + $reply_id = 3; + $this->assertStringEndsWith('p' . $reply_id, $url, 'Post ID of reply is not ' . $reply_id); + + // Check whether the notifications are still correct after a reply has been added + $result = $this->db->sql_query($sql); + $this->assertEquals($expected_after, $this->db->sql_fetchrowset($result), + 'Check whether the notifications are still correct after a reply has been added'); + $this->db->sql_freeresult($result); + + $result = $this->db->sql_query( + 'SELECT * + FROM ' . POSTS_TABLE . ' + WHERE post_id = ' . $reply_id); + $reply_edit_data = array_merge($this->post_data, $this->db->sql_fetchrow($result), array( + 'force_approved_state' => false, + 'post_edit_reason' => 'PHPBB3-12370', + )); + submit_post('edit', '', 'poster-name', POST_NORMAL, $poll_data, $reply_edit_data, false, false); + + // Check whether the notifications are still correct after the reply has been edit + $result = $this->db->sql_query($sql); + $this->assertEquals($expected_after, $this->db->sql_fetchrowset($result), + 'Check whether the notifications are still correct after the reply has been edit'); + $this->db->sql_freeresult($result); + } +}