Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-47201

Course outline report always using one reader when legacy and standard logs are enabled

    XMLWordPrintable

    Details

    • Database:
      MySQL
    • Testing Instructions:
      Hide
      1. Install 2.6
      2. Create a course, with a few modules
      3. Go to each of them (generating activity)
      4. Upgrade to the version you are testing (2.7 or master, you'll need to do both)
      5. Go to Site administration ► Plugins ► Logging ► Manage log stores
      6. Re-enable legacy log, making sure standard log and legacy are both enabled (don't go to any of the modules at this point)
      7. Go to Courses ► Miscellaneous ► Course 1 ► Reports ► Activity report
      8. Ensure that the activity report shows the activity for those modules that you did before upgrading
      9. Visit some of the activities again
      10. Ensure that the new activity is also shown
      Show
      Install 2.6 Create a course, with a few modules Go to each of them (generating activity) Upgrade to the version you are testing (2.7 or master, you'll need to do both) Go to Site administration ► Plugins ► Logging ► Manage log stores Re-enable legacy log, making sure standard log and legacy are both enabled (don't go to any of the modules at this point) Go to Courses ► Miscellaneous ► Course 1 ► Reports ► Activity report Ensure that the activity report shows the activity for those modules that you did before upgrading Visit some of the activities again Ensure that the new activity is also shown
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-47201-master

      Description

      When using course outline report there are several issues when both legacy log and standard log are enabled. Basically Outline report uses only the first available reader when both are enabled while it should use both readers and merge data. The issue is particularly evident when working with sites upgraded to 2.7 where a legacy log contains several data.

      Digging into the code we found the following (lines of code taken from Moodle 2.7.2 Build: 20140908)

      1) The outline report has the logic to manage the use of both log readers, however in localib.php, the function report_outline_get_common_log_variable returns only the first available log reader (FILE: locallib.php ROWs: 81 - 99):

          // Get preferred reader.
          if (!empty($readers)) {
              foreach ($readers as $readerpluginname => $reader) {
       
                  // If legacy reader is preferred reader.
                  if ($readerpluginname == 'logstore_legacy') {
                      $uselegacyreader = true;
                      break;
                  }
       
                  // If sql_internal_reader is preferred reader.
                  if ($reader instanceof \core\log\sql_internal_reader) {
                      $useinternalreader = true;
                      $logtable = $reader->get_internal_log_table_name();
                      $minloginternalreader = $DB->get_field_sql('SELECT min(timecreated) FROM {' . $logtable . '}');
                      break;
                  }
              }
          }
      

      Possible solution: deleting the "break" statements from the two "if" readers are returned correctly

      2) After solving the readers issue, the query to build the outline report receives only 3 parameters instead of 4. FILE: index.php, ROWs: 106 - 134

      // If using legacy log then get users from old table.
      if ($uselegacyreader) {
          // If we are going to use the internal (not legacy) log table, we should only get records
          // from the legacy table that exist before we started adding logs to the new table.
          $limittime = '';
          if (!empty($minloginternalreader)) {
              $limittime = ' AND time < :timeto ';
              $params['timeto'] = $minloginternalreader;        
          }
          // Check if we need to show the last access.
          $sqllasttime = '';
          if ($showlastaccess) {
              $sqllasttime = ", MAX(time) AS lasttime";
          }
          $logactionlike = $DB->sql_like('l.action', ':action');
          $sql = "SELECT cm.id, COUNT('x') AS numviews $sqllasttime
                    FROM {course_modules} cm
                    JOIN {modules} m
                      ON m.id = cm.module
                    JOIN {log} l
                      ON l.cmid = cm.id
                   WHERE cm.course = :courseid
                     AND $logactionlike
                     AND m.visible = :visible $limittime
                GROUP BY cm.id";
          $params = array('courseid' => $course->id, 'action' => 'view%', 'visible' => 1);
       
          $views = $DB->get_records_sql($sql, $params);
      }
      

      Possible solution: move down the "timeto" management:

      $params = array('courseid' => $course->id, 'action' => 'view%', 'visible' => 1);
      if (!empty($minloginternalreader)) {
          $params['timeto'] = $minloginternalreader;        
      }
      

      3) when legacy and standard log are both active the dates shown in "last access" report coloumn are always taken from legacy log. Looks like there is no check against dates from both logs FILE: index.php ROWs: 156 - 167:

          if (empty($views)) {
              $views = $v;
          } else {
              // Merge two view arrays.
              foreach ($v as $key => $value) {
                  if (isset($views[$key]) && !empty($views[$key]->numviews)) {
                      $views[$key]->numviews += $value->numviews;
                  } else {
                      $views[$key] = $value;
                  }
              }
          }
      

      Possible solution: add date check and use the most recent

         if (empty($views)) {
              $views = $v;
          } else {
              // Merge two view arrays.
              foreach ($v as $key => $value) {
                  if (isset($views[$key]) && !empty($views[$key]->numviews)) {
                      $views[$key]->numviews += $value->numviews;
                      if ($value->lasttime > $views[$key]->lasttime) {
                          $views[$key]->lasttime = $value->lasttime;
                      }
                  } else {
                      $views[$key] = $value;
                  }
              }
          } 
      

        Attachments

          Activity

            People

            Assignee:
            Unassigned
            Reporter:
            andreabix Andrea Bicciolo
            Peer reviewer:
            John Okely
            Integrator:
            Damyon Wiese
            Tester:
            Mark Nelson
            Participants:
            Component watchers:
            Adrian Greeve, Jake Dallimore, Mathew May, Mihail Geshoski, Peter Dias
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:
              Fix Release Date:
              10/Nov/14