From 718d7a43b949cd04490396f60f7cc700686c6eff Mon Sep 17 00:00:00 2001 From: Simey Lameze Date: Wed, 25 Sep 2019 12:25:55 +0800 Subject: [PATCH 1/4] MDL-66631 mod_forum: add date filters to export --- mod/forum/classes/form/export_form.php | 6 +++ mod/forum/classes/local/vaults/post.php | 61 +++++++++++++++++++------ mod/forum/export.php | 19 ++++---- mod/forum/lang/en/forum.php | 2 + 4 files changed, 65 insertions(+), 23 deletions(-) diff --git a/mod/forum/classes/form/export_form.php b/mod/forum/classes/form/export_form.php index 905e5e63e39..ecff1facb2e 100644 --- a/mod/forum/classes/form/export_form.php +++ b/mod/forum/classes/form/export_form.php @@ -69,6 +69,12 @@ class export_form extends \moodleform { ]; $mform->addElement('autocomplete', 'discussionids', get_string('discussions', 'mod_forum'), $discussions, $options); + // Date fields. + $mform->addElement('date_time_selector', 'from', get_string('postsfrom', 'mod_forum'), + ['optional' => true]); + $mform->addElement('date_time_selector', 'to', get_string('poststo', 'mod_forum'), + ['optional' => true]); + // Export formats. $formats = \core_plugin_manager::instance()->get_plugins_of_type('dataformat'); $options = []; diff --git a/mod/forum/classes/local/vaults/post.php b/mod/forum/classes/local/vaults/post.php index daf5e7fb3d4..88d3663245d 100644 --- a/mod/forum/classes/local/vaults/post.php +++ b/mod/forum/classes/local/vaults/post.php @@ -157,39 +157,70 @@ class post extends db_table_vault { } /** - * The method returns posts made by the supplied users in the supplied discussions. + * The method returns posts based on a set of filters. * * @param stdClass $user Only used when restricting private replies - * @param int[] $discussionids The list of discussion ids to load posts for - * @param int[] $userids Only return posts made by these users + * @param array $filters Export filters, valid filters are: + * + * 'discussionids' => array of discussion ids eg [1,2,3] + * 'userids' => array of user ids eg [1,2,3] + * 'from' => timestamp to filter posts from this date. + * 'to' => timestamp to filter posts till this date. + * * @param bool $canseeprivatereplies Whether this user can see all private replies or not * @param string $orderby Order the results * @return post_entity[] */ - public function get_from_discussion_ids_and_user_ids( + public function get_from_filters( stdClass $user, - array $discussionids, - array $userids, + array $filters, bool $canseeprivatereplies, string $orderby = '' ): array { - if (empty($discussionids) || empty($userids)) { + if (count($filters) == 0) { return []; } - + $wheresql = []; + $params = []; $alias = $this->get_table_alias(); - list($indiscussionssql, $indiscussionsparams) = $this->get_db()->get_in_or_equal($discussionids, SQL_PARAMS_NAMED); - list($inuserssql, $inusersparams) = $this->get_db()->get_in_or_equal($userids, SQL_PARAMS_NAMED); + // Filter by discussion ids. + if (!empty($filters['discussionids'])) { + list($indiscussionssql, $indiscussionsparams) = $this->get_db()->get_in_or_equal($filters['discussionids'], + SQL_PARAMS_NAMED); + $wheresql[] = "{$alias}.discussion {$indiscussionssql}"; + $params += $indiscussionsparams; + } + // Filter by user ids. + if (!empty($filters['userids'])) { + list($inuserssql, $inusersparams) = $this->get_db()->get_in_or_equal($filters['userids'], + SQL_PARAMS_NAMED); + $wheresql[] = "{$alias}.userid {$inuserssql}"; + $params += $inusersparams; + } + + // Filter posts by from and to dates. + if (isset($filters['from'])) { + $wheresql[] = "{$alias}.created >= :from"; + $params['from'] = $filters['from']; + } + + if (isset($filters['to'])) { + $wheresql[] = "{$alias}.created < :to"; + $params['to'] = $filters['to']; + } + + // We need to build the WHERE here, because get_private_reply_sql returns the query with the AND clause. + $wheresql = implode(' AND ', $wheresql); + + // Build private replies sql. [ 'where' => $privatewhere, 'params' => $privateparams, ] = $this->get_private_reply_sql($user, $canseeprivatereplies); - - $wheresql = "{$alias}.discussion {$indiscussionssql} - AND {$alias}.userid {$inuserssql} - {$privatewhere}"; + $wheresql .= "{$privatewhere}"; + $params += $privateparams; if ($orderby) { $orderbysql = $alias . '.' . $orderby; @@ -198,7 +229,7 @@ class post extends db_table_vault { } $sql = $this->generate_get_records_sql($wheresql, $orderbysql); - $records = $this->get_db()->get_records_sql($sql, array_merge($indiscussionsparams, $inusersparams, $privateparams)); + $records = $this->get_db()->get_records_sql($sql, $params); return $this->transform_db_records_to_entities($records); } diff --git a/mod/forum/export.php b/mod/forum/export.php index 419997754a6..e268e586f18 100644 --- a/mod/forum/export.php +++ b/mod/forum/export.php @@ -78,16 +78,19 @@ if ($form->is_cancelled()) { }, $discussions); } + $filters = ['discussionids' => $discussionids]; if ($data->userids) { - $posts = $postvault->get_from_discussion_ids_and_user_ids($USER, - $discussionids, - $data->userids, - $capabilitymanager->can_view_any_private_reply($USER)); - } else { - $posts = $postvault->get_from_discussion_ids($USER, - $discussionids, - $capabilitymanager->can_view_any_private_reply($USER)); + $filters['userids'] = $data->userids; } + if ($data->from) { + $filters['from'] = $data->from; + } + if ($data->to) { + $filters['to'] = $data->to; + } + + // Retrieve posts based on the selected filters. + $posts = $postvault->get_from_filters($USER, $filters, $capabilitymanager->can_view_any_private_reply($USER)); $fields = ['id', 'discussion', 'parent', 'userid', 'created', 'modified', 'mailed', 'subject', 'message', 'messageformat', 'messagetrust', 'attachment', 'totalscore', 'mailnow', 'deleted', 'privatereplyto']; diff --git a/mod/forum/lang/en/forum.php b/mod/forum/lang/en/forum.php index da3424c2451..8fa58b0eefc 100644 --- a/mod/forum/lang/en/forum.php +++ b/mod/forum/lang/en/forum.php @@ -495,8 +495,10 @@ $string['postrating1'] = 'Mostly separate knowing'; $string['postrating2'] = 'Separate and connected'; $string['postrating3'] = 'Mostly connected knowing'; $string['posts'] = 'Posts'; +$string['postsfrom'] = 'Posts from'; $string['postsmadebyuser'] = 'Posts made by {$a}'; $string['postsmadebyuserincourse'] = 'Posts made by {$a->fullname} in {$a->coursename}'; +$string['poststo'] = 'Posts to'; $string['posttoforum'] = 'Post to forum'; $string['postupdated'] = 'Your post was updated'; $string['potentialsubscribers'] = 'Potential subscribers'; From 6e23f294b8f9d3f4c7a56d669683a28ba5a57cc3 Mon Sep 17 00:00:00 2001 From: Simey Lameze Date: Wed, 2 Oct 2019 13:51:22 +0800 Subject: [PATCH 2/4] MDL-66631 mod_forum: make get_from_discussion_ids use new fn --- mod/forum/classes/local/vaults/post.php | 21 +-------------------- 1 file changed, 1 insertion(+), 20 deletions(-) diff --git a/mod/forum/classes/local/vaults/post.php b/mod/forum/classes/local/vaults/post.php index 88d3663245d..fec45d2fabb 100644 --- a/mod/forum/classes/local/vaults/post.php +++ b/mod/forum/classes/local/vaults/post.php @@ -134,26 +134,7 @@ class post extends db_table_vault { return []; } - $alias = $this->get_table_alias(); - - list($insql, $params) = $this->get_db()->get_in_or_equal($discussionids, SQL_PARAMS_NAMED); - [ - 'where' => $privatewhere, - 'params' => $privateparams, - ] = $this->get_private_reply_sql($user, $canseeprivatereplies); - - $wheresql = "{$alias}.discussion {$insql} {$privatewhere}"; - - if ($orderby) { - $orderbysql = $alias . '.' . $orderby; - } else { - $orderbysql = ''; - } - - $sql = $this->generate_get_records_sql($wheresql, $orderbysql); - $records = $this->get_db()->get_records_sql($sql, array_merge($params, $privateparams)); - - return $this->transform_db_records_to_entities($records); + return $this->get_from_filters($user, ['discussionids' => $discussionids], $canseeprivatereplies, $orderby); } /** From 77c01b42c69dab0f697f0a29e95fa166bd4a57eb Mon Sep 17 00:00:00 2001 From: Simey Lameze Date: Wed, 2 Oct 2019 14:00:22 +0800 Subject: [PATCH 3/4] MDL-66631 mod_forum: unit tests refactoring --- mod/forum/tests/vaults_post_test.php | 64 +++++++++++++++------------- 1 file changed, 35 insertions(+), 29 deletions(-) diff --git a/mod/forum/tests/vaults_post_test.php b/mod/forum/tests/vaults_post_test.php index 6e607b9cddf..80fbfe77792 100644 --- a/mod/forum/tests/vaults_post_test.php +++ b/mod/forum/tests/vaults_post_test.php @@ -925,11 +925,11 @@ class mod_forum_vaults_post_testcase extends advanced_testcase { } /** - * Test get_from_discussion_ids_and_user_ids. + * Test get_from_filters. * - * @covers ::get_from_discussion_ids_and_user_ids + * @covers ::get_from_filters */ - public function test_get_from_discussion_ids_and_user_ids() { + public function test_get_from_filters() { $this->resetAfterTest(); $datagenerator = $this->getDataGenerator(); @@ -945,9 +945,8 @@ class mod_forum_vaults_post_testcase extends advanced_testcase { $discussionids = [$discussion1->id, $discussion2->id]; $userids = [$user->id]; - $entities = array_values($this->vault->get_from_discussion_ids_and_user_ids($user, - $discussionids, - $userids, + $entities = array_values($this->vault->get_from_filters($user, + ['discussionids' => $discussionids, 'userids' => $userids], true, 'id ASC')); @@ -957,14 +956,17 @@ class mod_forum_vaults_post_testcase extends advanced_testcase { $this->assertEquals($post3->id, $entities[2]->get_id()); $this->assertEquals($post4->id, $entities[3]->get_id()); - $entities = $this->vault->get_from_discussion_ids_and_user_ids($user, [$discussion1->id], $userids, false); + $entities = $this->vault->get_from_filters($user, ['discussionids' => $discussion1->id, 'userids' => $userids], + false); $this->assertCount(3, $entities); $this->assertArrayHasKey($post1->id, $entities); $this->assertArrayHasKey($post2->id, $entities); $this->assertArrayHasKey($post3->id, $entities); - $entities = $this->vault->get_from_discussion_ids_and_user_ids($user, [$discussion1->id, $discussion2->id], - [$user->id, $user2->id], false); + $discussionids = [$discussion1->id, $discussion2->id]; + $userids = [$user->id, $user2->id]; + $entities = $this->vault->get_from_filters($user, ['discussionids' => $discussionids, 'userids' => $userids], + false); $this->assertCount(4, $entities); $this->assertArrayHasKey($post1->id, $entities); $this->assertArrayHasKey($post2->id, $entities); @@ -972,16 +974,16 @@ class mod_forum_vaults_post_testcase extends advanced_testcase { $this->assertArrayHasKey($post4->id, $entities); // Test ordering by id descending. - $entities = $this->vault->get_from_discussion_ids_and_user_ids($user, [$discussion1->id, $discussion2->id], - [$user->id], false, 'id DESC'); + $entities = $this->vault->get_from_filters($user, ['discussionids' => $discussionids, 'userids' => $user->id], + false, 'id DESC'); $this->assertEquals($post4->id, array_values($entities)[0]->get_id()); $this->assertEquals($post3->id, array_values($entities)[1]->get_id()); $this->assertEquals($post2->id, array_values($entities)[2]->get_id()); $this->assertEquals($post1->id, array_values($entities)[3]->get_id()); // Test ordering by id ascending. - $entities = $this->vault->get_from_discussion_ids_and_user_ids($user, [$discussion1->id, $discussion2->id], - [$user->id], false, 'id ASC'); + $entities = $this->vault->get_from_filters($user, ['discussionids' => $discussionids, 'userids' => $user->id], + false, 'id ASC'); $this->assertEquals($post1->id, array_values($entities)[0]->get_id()); $this->assertEquals($post2->id, array_values($entities)[1]->get_id()); $this->assertEquals($post3->id, array_values($entities)[2]->get_id()); @@ -989,30 +991,28 @@ class mod_forum_vaults_post_testcase extends advanced_testcase { } /** - * Test get_from_discussion_ids_and_user_ids when no discussion ids were provided. + * Test get_from_filters when no discussion ids were provided. * - * @covers ::get_from_discussion_ids_and_user_ids + * @covers ::get_from_filters */ - public function test_get_from_discussion_ids_and_user_ids_empty() { + public function test_get_from_filters_empty() { $this->resetAfterTest(); $datagenerator = $this->getDataGenerator(); $course = $datagenerator->create_course(); - [$student1, $student2] = $this->helper_create_users($course, 2, 'student'); + [$student1] = $this->helper_create_users($course, 1, 'student'); $forum = $datagenerator->create_module('forum', ['course' => $course->id]); - [$discussion, $post] = $this->helper_post_to_forum($forum, $student1); - $this->assertEquals([], $this->vault->get_from_discussion_ids_and_user_ids($student1, [], [], false)); - $this->assertEquals([], $this->vault->get_from_discussion_ids_and_user_ids($student1, [$discussion->id], [], false)); - $this->assertEquals([], $this->vault->get_from_discussion_ids_and_user_ids($student1, [], [$student2->id], false)); + $this->helper_post_to_forum($forum, $student1); + $this->assertEquals([], $this->vault->get_from_filters($student1, [], false)); } /** * Ensure that selecting posts in a discussion only returns posts that the user can see, when considering private * replies. * - * @covers ::get_from_discussion_ids_and_user_ids + * @covers ::get_from_filters */ - public function test_get_from_discussion_ids_and_user_ids_private_replies() { + public function test_get_from_filters_private_replies() { $this->resetAfterTest(); $course = $this->getDataGenerator()->create_course(); @@ -1048,7 +1048,8 @@ class mod_forum_vaults_post_testcase extends advanced_testcase { $discussionids = [$discussion->id, $otherdiscussion->id]; // Teacher 1. Request all posts from the vault, telling the vault that the teacher CAN see private replies made by anyone. - $entities = $this->vault->get_from_discussion_ids_and_user_ids($teacher, $discussionids, $userids, true); + $entities = $this->vault->get_from_filters($teacher, ['discussionids' => $discussionids, 'userids' => $userids], + true); $this->assertCount(4, $entities); $this->assertArrayHasKey($postprivatereply->id, $entities); $this->assertArrayHasKey($otherpost->id, $entities); @@ -1057,7 +1058,8 @@ class mod_forum_vaults_post_testcase extends advanced_testcase { // Student 1. Request all posts from the vault, telling the vault that the student CAN'T see private replies made by anyone. // Teacher2's private reply to otherpost is omitted. - $entities = $this->vault->get_from_discussion_ids_and_user_ids($student, $discussionids, $userids, false); + $entities = $this->vault->get_from_filters($student, ['discussionids' => $discussionids, 'userids' => $userids], + false); $this->assertCount(3, $entities); $this->assertArrayHasKey($postprivatereply->id, $entities); $this->assertArrayHasKey($otherpost->id, $entities); @@ -1065,7 +1067,8 @@ class mod_forum_vaults_post_testcase extends advanced_testcase { // Student 1. Request all posts from the vault, telling the vault that student CAN see all private replies made. // The private reply made by teacher 2 to otherpost is now included. - $entities = $this->vault->get_from_discussion_ids_and_user_ids($student, $discussionids, $userids, true); + $entities = $this->vault->get_from_filters($student, ['discussionids' => $discussionids, 'userids' => $userids], + true); $this->assertCount(4, $entities); $this->assertArrayHasKey($postprivatereply->id, $entities); $this->assertArrayHasKey($otherpost->id, $entities); @@ -1073,7 +1076,8 @@ class mod_forum_vaults_post_testcase extends advanced_testcase { $this->assertArrayHasKey($otherpostreplyprivatereply->id, $entities); // Teacher 2. Request all posts from the vault, telling the vault that teacher2 CAN see all private replies made. - $entities = $this->vault->get_from_discussion_ids_and_user_ids($otherteacher, $discussionids, $userids, true); + $entities = $this->vault->get_from_filters($otherteacher, + ['discussionids' => $discussionids, 'userids' => $userids], true); $this->assertCount(4, $entities); $this->assertArrayHasKey($otherpost->id, $entities); $this->assertArrayHasKey($otherpostprivatereply->id, $entities); @@ -1081,14 +1085,16 @@ class mod_forum_vaults_post_testcase extends advanced_testcase { // Teacher 2. Request all posts from the vault, telling the vault that teacher2 CANNOT see all private replies made. // The private replies not relating to teacher 2 directly are omitted. - $entities = $this->vault->get_from_discussion_ids_and_user_ids($otherteacher, $discussionids, $userids, false); + $entities = $this->vault->get_from_filters($otherteacher, + ['discussionids' => $discussionids, 'userids' => $userids], false); $this->assertCount(2, $entities); $this->assertArrayHasKey($otherpost->id, $entities); $this->assertArrayHasKey($otherpostprivatereply->id, $entities); // Student 2. Request all posts from the vault, telling the vault that student2 CAN'T see all private replies made. // All private replies are omitted, as none relate to student2. - $entities = $this->vault->get_from_discussion_ids_and_user_ids($otherstudent, $discussionids, $userids, false); + $entities = $this->vault->get_from_filters($otherstudent, + ['discussionids' => $discussionids, 'userids' => $userids], false); $this->assertCount(1, $entities); $this->assertArrayHasKey($otherpost->id, $entities); } From 8074b23f5646398868f273cadca52935308dedbf Mon Sep 17 00:00:00 2001 From: Simey Lameze Date: Thu, 3 Oct 2019 10:15:44 +0800 Subject: [PATCH 4/4] MDL-66631 mod_forum: unit tests for date filters --- mod/forum/tests/vaults_post_test.php | 30 ++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/mod/forum/tests/vaults_post_test.php b/mod/forum/tests/vaults_post_test.php index 80fbfe77792..e9b8a95d7db 100644 --- a/mod/forum/tests/vaults_post_test.php +++ b/mod/forum/tests/vaults_post_test.php @@ -990,6 +990,36 @@ class mod_forum_vaults_post_testcase extends advanced_testcase { $this->assertEquals($post4->id, array_values($entities)[3]->get_id()); } + public function test_get_from_filters_from_to_dates() { + $this->resetAfterTest(); + + $datagenerator = $this->getDataGenerator(); + $course = $datagenerator->create_course(); + [$user, $user2] = $this->helper_create_users($course, 2, 'student'); + $forum = $datagenerator->create_module('forum', ['course' => $course->id]); + + [$discussion1, $post1] = $this->helper_post_to_forum($forum, $user); + + $date = new DateTime('2019-07-05'); + $post2 = $this->helper_reply_to_post($post1, $user, ['created' => $date->getTimestamp()]); + $post3 = $this->helper_reply_to_post($post1, $user, ['created' => $date->getTimestamp()]); + $date->modify('+1 month'); + $post4 = $this->helper_reply_to_post($post1, $user, ['created' => $date->getTimestamp()]); + $post5 = $this->helper_reply_to_post($post1, $user, ['created' => $date->getTimestamp()]); + $post6 = $this->helper_reply_to_post($post1, $user, ['created' => $date->getTimestamp()]); + + [$discussion2, $post4] = $this->helper_post_to_forum($forum, $user); + + $datefilter = new DateTime('2019-07-01'); + $filters = ['from' => $datefilter->getTimestamp()]; + $entities = $this->vault->get_from_filters($user, $filters, false); + $this->assertCount(7, $entities); + + $filters['to'] = $datefilter->modify('+1 month')->getTimestamp(); + $entities = $this->vault->get_from_filters($user, $filters, false); + $this->assertCount(2, $entities); + } + /** * Test get_from_filters when no discussion ids were provided. *