From 768a2ceafd77094496a3e3c732423e695c7c828c Mon Sep 17 00:00:00 2001 From: Adrian Greeve Date: Tue, 2 Oct 2012 15:52:00 +0800 Subject: [PATCH] MDL-34448 - mod/data - Fixing separate groups viewing all entries. --- mod/data/lib.php | 32 +++-- mod/data/tests/fixtures/test_data_records.csv | 132 +++++++++--------- mod/data/tests/search_test.php | 28 ++++ mod/data/view.php | 28 +++- mod/upgrade.txt | 11 ++ 5 files changed, 149 insertions(+), 82 deletions(-) diff --git a/mod/data/lib.php b/mod/data/lib.php index fc26f78beaa..583ea73bb5e 100644 --- a/mod/data/lib.php +++ b/mod/data/lib.php @@ -3408,21 +3408,28 @@ function data_page_type_list($pagetype, $parentcontext, $currentcontext) { /** * Get all of the record ids from a database activity. * - * @param int $dataid The dataid of the database module. - * @return array $idarray An array of record ids + * @param int $dataid The dataid of the database module. + * @param object $selectdata Contains an additional sql statement for the + * where clause for group and approval fields. + * @param array $params Parameters that coincide with the sql statement. + * @return array $idarray An array of record ids */ -function data_get_all_recordids($dataid) { +function data_get_all_recordids($dataid, $selectdata = '', $params = null) { global $DB; - $initsql = 'SELECT c.recordid - FROM {data_fields} f, - {data_content} c - WHERE f.dataid = :dataid - AND f.id = c.fieldid - GROUP BY c.recordid'; - $initrecord = $DB->get_recordset_sql($initsql, array('dataid' => $dataid)); + $initsql = 'SELECT r.id + FROM {data_records} r + WHERE r.dataid = :dataid'; + if ($selectdata != '') { + $initsql .= $selectdata; + $params = array_merge(array('dataid' => $dataid), $params); + } else { + $params = array('dataid' => $dataid); + } + $initsql .= ' GROUP BY r.id'; + $initrecord = $DB->get_recordset_sql($initsql, $params); $idarray = array(); foreach ($initrecord as $data) { - $idarray[] = $data->recordid; + $idarray[] = $data->id; } // Close the record set and free up resources. $initrecord->close(); @@ -3567,8 +3574,7 @@ function data_get_advanced_search_sql($sort, $data, $recordids, $selectdata, $so } else { list($insql, $inparam) = $DB->get_in_or_equal(array('-1'), SQL_PARAMS_NAMED); } - $nestfromsql .= ' AND c.recordid ' . $insql . $groupsql; - $nestfromsql = "$nestfromsql $selectdata"; + $nestfromsql .= ' AND c.recordid ' . $insql . $selectdata . $groupsql; $sqlselect['sql'] = "$nestselectsql $nestfromsql $sortorder"; $sqlselect['params'] = $inparam; return $sqlselect; diff --git a/mod/data/tests/fixtures/test_data_records.csv b/mod/data/tests/fixtures/test_data_records.csv index 5ffa5bf1d3d..f740429b2b1 100644 --- a/mod/data/tests/fixtures/test_data_records.csv +++ b/mod/data/tests/fixtures/test_data_records.csv @@ -1,101 +1,101 @@ id,userid,groupid,dataid,timecreated,timemodified,approved 1,1,0,1,1234567891,1234567892,1 -2,2,0,1,1234567891,1234567892,1 -3,3,0,1,1234567891,1234567892,1 -4,4,0,1,1234567891,1234567892,1 -5,5,0,1,1234567891,1234567892,1 -6,6,0,1,1234567891,1234567892,1 +2,2,1,1,1234567891,1234567892,1 +3,3,2,1,1234567891,1234567892,1 +4,4,1,1,1234567891,1234567892,1 +5,5,1,1,1234567891,1234567892,1 +6,6,2,1,1234567891,1234567892,1 7,7,0,1,1234567891,1234567892,1 -8,8,0,1,1234567891,1234567892,1 -9,9,0,1,1234567891,1234567892,1 -10,10,0,1,1234567891,1234567892,1 -11,11,0,1,1234567891,1234567892,1 -12,12,0,1,1234567891,1234567892,1 -13,13,0,1,1234567891,1234567892,1 +8,8,2,1,1234567891,1234567892,1 +9,9,1,1,1234567891,1234567892,1 +10,10,1,1,1234567891,1234567892,1 +11,11,1,1,1234567891,1234567892,1 +12,12,2,1,1234567891,1234567892,1 +13,13,2,1,1234567891,1234567892,1 14,14,0,1,1234567891,1234567892,1 -15,15,0,1,1234567891,1234567892,1 +15,15,2,1,1234567891,1234567892,1 16,16,0,1,1234567891,1234567892,1 -17,17,0,1,1234567891,1234567892,1 -18,18,0,1,1234567891,1234567892,1 +17,17,1,1,1234567891,1234567892,1 +18,18,2,1,1234567891,1234567892,1 19,19,0,1,1234567891,1234567892,1 -20,20,0,1,1234567891,1234567892,1 +20,20,1,1,1234567891,1234567892,1 21,21,0,1,1234567891,1234567892,1 -22,22,0,1,1234567891,1234567892,1 +22,22,2,1,1234567891,1234567892,1 23,23,0,1,1234567891,1234567892,1 -24,24,0,1,1234567891,1234567892,1 -25,25,0,1,1234567891,1234567892,1 +24,24,2,1,1234567891,1234567892,1 +25,25,2,1,1234567891,1234567892,1 26,26,0,1,1234567891,1234567892,1 -27,27,0,1,1234567891,1234567892,1 -28,28,0,1,1234567891,1234567892,1 +27,27,1,1,1234567891,1234567892,1 +28,28,2,1,1234567891,1234567892,1 29,29,0,1,1234567891,1234567892,1 -30,30,0,1,1234567891,1234567892,1 +30,30,2,1,1234567891,1234567892,1 31,31,0,1,1234567891,1234567892,1 -32,32,0,1,1234567891,1234567892,1 -33,33,0,1,1234567891,1234567892,1 -34,34,0,1,1234567891,1234567892,1 -35,35,0,1,1234567891,1234567892,1 +32,32,1,1,1234567891,1234567892,1 +33,33,1,1,1234567891,1234567892,1 +34,34,2,1,1234567891,1234567892,1 +35,35,1,1,1234567891,1234567892,1 36,36,0,1,1234567891,1234567892,1 37,37,0,1,1234567891,1234567892,1 -38,38,0,1,1234567891,1234567892,1 -39,39,0,1,1234567891,1234567892,1 -40,40,0,1,1234567891,1234567892,1 +38,38,2,1,1234567891,1234567892,1 +39,39,1,1,1234567891,1234567892,1 +40,40,1,1,1234567891,1234567892,1 41,41,0,1,1234567891,1234567892,1 -42,42,0,1,1234567891,1234567892,1 +42,42,1,1,1234567891,1234567892,1 43,43,0,1,1234567891,1234567892,1 -44,44,0,1,1234567891,1234567892,1 +44,44,1,1,1234567891,1234567892,1 45,45,0,1,1234567891,1234567892,1 46,46,0,1,1234567891,1234567892,1 -47,47,0,1,1234567891,1234567892,1 +47,47,1,1,1234567891,1234567892,1 48,48,0,1,1234567891,1234567892,1 49,49,0,1,1234567891,1234567892,1 -50,50,0,1,1234567891,1234567892,1 +50,50,1,1,1234567891,1234567892,1 51,51,0,1,1234567891,1234567892,1 52,52,0,1,1234567891,1234567892,1 -53,53,0,1,1234567891,1234567892,1 -54,54,0,1,1234567891,1234567892,1 +53,53,1,1,1234567891,1234567892,1 +54,54,1,1,1234567891,1234567892,1 55,55,0,1,1234567891,1234567892,1 -56,56,0,1,1234567891,1234567892,1 -57,57,0,1,1234567891,1234567892,1 -58,58,0,1,1234567891,1234567892,1 -59,59,0,1,1234567891,1234567892,1 -60,60,0,1,1234567891,1234567892,1 +56,56,2,1,1234567891,1234567892,1 +57,57,2,1,1234567891,1234567892,1 +58,58,2,1,1234567891,1234567892,1 +59,59,1,1,1234567891,1234567892,1 +60,60,1,1,1234567891,1234567892,1 61,61,0,1,1234567891,1234567892,1 -62,62,0,1,1234567891,1234567892,1 +62,62,2,1,1234567891,1234567892,1 63,63,0,1,1234567891,1234567892,1 64,64,0,1,1234567891,1234567892,1 -65,65,0,1,1234567891,1234567892,1 -66,66,0,1,1234567891,1234567892,1 +65,65,1,1,1234567891,1234567892,1 +66,66,1,1,1234567891,1234567892,1 67,67,0,1,1234567891,1234567892,1 68,68,0,1,1234567891,1234567892,1 -69,69,0,1,1234567891,1234567892,1 -70,70,0,1,1234567891,1234567892,1 +69,69,2,1,1234567891,1234567892,1 +70,70,2,1,1234567891,1234567892,1 71,71,0,1,1234567891,1234567892,1 -72,72,0,1,1234567891,1234567892,1 -73,73,0,1,1234567891,1234567892,1 +72,72,1,1,1234567891,1234567892,1 +73,73,1,1,1234567891,1234567892,1 74,74,0,1,1234567891,1234567892,1 75,75,0,1,1234567891,1234567892,1 -76,76,0,1,1234567891,1234567892,1 -77,77,0,1,1234567891,1234567892,1 +76,76,2,1,1234567891,1234567892,1 +77,77,2,1,1234567891,1234567892,1 78,78,0,1,1234567891,1234567892,1 -79,79,0,1,1234567891,1234567892,1 -80,80,0,1,1234567891,1234567892,1 +79,79,1,1,1234567891,1234567892,1 +80,80,1,1,1234567891,1234567892,1 81,81,0,1,1234567891,1234567892,1 -82,82,0,1,1234567891,1234567892,1 -83,83,0,1,1234567891,1234567892,1 -84,84,0,1,1234567891,1234567892,1 -85,85,0,1,1234567891,1234567892,1 +82,82,1,1,1234567891,1234567892,1 +83,83,1,1,1234567891,1234567892,1 +84,84,1,1,1234567891,1234567892,1 +85,85,1,1,1234567891,1234567892,1 86,86,0,1,1234567891,1234567892,1 87,87,0,1,1234567891,1234567892,1 88,88,0,1,1234567891,1234567892,1 -89,89,0,1,1234567891,1234567892,1 -90,90,0,1,1234567891,1234567892,1 -91,91,0,1,1234567891,1234567892,1 -92,92,0,1,1234567891,1234567892,1 -93,93,0,1,1234567891,1234567892,1 -94,94,0,1,1234567891,1234567892,1 -95,95,0,1,1234567891,1234567892,1 -96,96,0,1,1234567891,1234567892,1 -97,97,0,1,1234567891,1234567892,1 -98,98,0,1,1234567891,1234567892,1 -99,99,0,1,1234567891,1234567892,1 -100,100,0,1,1234567891,1234567892,1 +89,89,1,1,1234567891,1234567892,1 +90,90,1,1,1234567891,1234567892,0 +91,91,2,1,1234567891,1234567892,0 +92,92,0,1,1234567891,1234567892,0 +93,93,2,1,1234567891,1234567892,0 +94,94,1,1,1234567891,1234567892,0 +95,95,1,1,1234567891,1234567892,0 +96,96,1,1,1234567891,1234567892,0 +97,97,0,1,1234567891,1234567892,0 +98,98,1,1,1234567891,1234567892,0 +99,99,2,1,1234567891,1234567892,0 +100,100,0,1,1234567891,1234567892,0 diff --git a/mod/data/tests/search_test.php b/mod/data/tests/search_test.php index 150bc5d3298..29806438853 100644 --- a/mod/data/tests/search_test.php +++ b/mod/data/tests/search_test.php @@ -70,6 +70,11 @@ class data_advanced_search_sql_test extends advanced_testcase { */ public $datarecordcount = 100; + /** + * @var int $groupdatarecordcount The number of records in the database in groups 0 and 1. + */ + public $groupdatarecordcount = 75; + /** * @var array $datarecordset Expected record IDs. */ @@ -80,6 +85,11 @@ class data_advanced_search_sql_test extends advanced_testcase { */ public $finalrecord = array(); + /** + * @var int $approvedatarecordcount The number of approved records in the database. + */ + public $approvedatarecordcount = 89; + /** * Set up function. In this instance we are setting up database * records to be used in the unit tests. @@ -169,6 +179,13 @@ class data_advanced_search_sql_test extends advanced_testcase { * Test 4: data_get_advanced_search_sql provides an array which contains an sql string to be used for displaying records * to the user when they use the advanced search criteria and the parameters that go with the sql statement. This test * takes that information and does a search on the database, returning a record. + * + * Test 5: Returning to data_get_all_recordids(). Here we are ensuring that the total amount of record ids is reduced to + * match the group conditions that are provided. There are 25 entries which relate to group 2. They are removed + * from the total so we should only have 75 records total. + * + * Test 6: data_get_all_recordids() again. This time we are testing approved database records. We only want to + * display the records that have been approved. In this record set we have 89 approved records. */ function test_advanced_search_sql_section() { global $DB; @@ -193,5 +210,16 @@ class data_advanced_search_sql_test extends advanced_testcase { $allparams = array_merge($html['params'], array('dataid' => $this->recorddata->id)); $records = $DB->get_records_sql($html['sql'], $allparams); $this->assertEquals($records, $this->finalrecord); + + // Test 5 + $groupsql = " AND (r.groupid = :currentgroup OR r.groupid = 0)"; + $params = array('currentgroup' => 1); + $recordids = data_get_all_recordids($this->recorddata->id, $groupsql, $params); + $this->assertEquals($this->groupdatarecordcount, count($recordids)); + + // Test 6 + $approvesql = ' AND r.approved=1 '; + $recordids = data_get_all_recordids($this->recorddata->id, $approvesql, $params); + $this->assertEquals($this->approvedatarecordcount, count($recordids)); } } diff --git a/mod/data/view.php b/mod/data/view.php index 3e9146e35e1..1b2d85b73ef 100644 --- a/mod/data/view.php +++ b/mod/data/view.php @@ -321,6 +321,13 @@ groups_print_activity_menu($cm, $returnurl); $currentgroup = groups_get_activity_group($cm); $groupmode = groups_get_activity_groupmode($cm); + // If a student is not part of a group and seperate groups is enabled, we don't + // want them seeing all records. + if ($currentgroup == 0 && $groupmode == 1 && !has_capability('mod/data:manageentries', $context)) { + $canviewallrecords = false; + } else { + $canviewallrecords = true; + } // detect entries not approved yet and show hint instead of not found error if ($record and $data->approval and !$record->approved and $record->userid != $USER->id and !has_capability('mod/data:manageentries', $context)) { @@ -465,7 +472,13 @@ if ($showactivity) { $groupselect = " AND (r.groupid = :currentgroup OR r.groupid = 0)"; $params['currentgroup'] = $currentgroup; } else { - $groupselect = ' '; + if ($canviewallrecords) { + $groupselect = ' '; + } else { + // If separate groups are enabled and the user isn't in a group or + // a teacher, manager, admin etc, then just show them entries for 'All participants'. + $groupselect = " AND r.groupid = 0"; + } } // Init some variables to be used by advanced search @@ -590,10 +603,19 @@ if ($showactivity) { $sqlmax = "SELECT $count FROM $tables $where $groupselect $approveselect"; // number of all recoirds user may see $allparams = array_merge($params, $advparams); - $recordids = data_get_all_recordids($data->id); + // Provide initial sql statements and parameters to reduce the number of total records. + $selectdata = $groupselect . $approveselect; + $initialparams = array(); + if ($currentgroup) { + $initialparams['currentgroup'] = $params['currentgroup']; + } + if (!$approvecap && $data->approval && isloggedin()) { + $initialparams['myid1'] = $params['myid1']; + } + + $recordids = data_get_all_recordids($data->id, $selectdata, $initialparams); $newrecordids = data_get_advance_search_ids($recordids, $search_array, $data->id); $totalcount = count($newrecordids); - $selectdata = $groupselect . $approveselect; if (!empty($advanced)) { $advancedsearchsql = data_get_advanced_search_sql($sort, $data, $newrecordids, $selectdata, $sortorder); diff --git a/mod/upgrade.txt b/mod/upgrade.txt index 72411c8168d..6eac247ae9a 100644 --- a/mod/upgrade.txt +++ b/mod/upgrade.txt @@ -21,6 +21,9 @@ optional - no changes needed: xxx_dndupload_register() and xxx_dndupload_handle($uploadinfo) see: http://docs.moodle.org/dev/Implementing_Course_drag_and_drop_upload_support_in_a_module +* mod/data/lib.php data_get_all_recordids() now has two new optional variables: $selectdata and $params. + + === 2.2 === required changes in code: @@ -29,12 +32,20 @@ required changes in code: * textlib->asort() replaced by specialized collatorlib::asort() * use new make_temp_directory() and make_cache_directory() +optional - no changes needed: + +* mod/data/lib.php data_get_all_recordids() now has two new optional variables: $selectdata and $params. + === 2.1 === required changes in code: * add new support for basic restore from 1.9 +optional - no changes needed: + +* mod/data/lib.php data_get_all_recordids() now has two new optional variables: $selectdata and $params. + === 2.0 ===