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

Extending navigation from report progress is inefficient

    Details

    • Testing Instructions:
      Hide
      • Run the unit tests lib/tests/completion_advanced_test.php
      1. Enable enablecompletion
      2. Enable the completion tracking on a course, and visit the course
      3. Make sure you can see 'Activity completion' under 'Reports' in 'Course administration' if you have activities with completion tracking enabled
      4. Make sure you CANNOT see 'Activity completion' under 'Reports' in 'Course administration' if you DO NOT have activities with completion tracking enabled
      Show
      Run the unit tests lib/tests/completion_advanced_test.php Enable enablecompletion Enable the completion tracking on a course, and visit the course Make sure you can see 'Activity completion' under 'Reports' in 'Course administration' if you have activities with completion tracking enabled Make sure you CANNOT see 'Activity completion' under 'Reports' in 'Course administration' if you DO NOT have activities with completion tracking enabled
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-39536-master

      Description

      While debugging MDL-37773, I came across report_progress_extend_navigation_course() which is highly inefficient when it comes to adding nodes to the navigation.

      If activity completion is enabled on the course, then it gets all the activities just to ensure that at least one has completion enabled. This is a massive processing where a simple DB->record_exists would be just enough.

      $showonnavigation = ($showonnavigation && $completion->is_enabled() && ($completion->get_activities())>0);
      

      Will call:

          public function get_activities($modinfo=null) {
              global $DB;
       
              // Obtain those activities which have completion turned on
              $withcompletion = $DB->get_records_select('course_modules', 'course='.$this->course->id.
                ' AND completion<>'.COMPLETION_TRACKING_NONE);
              if (!$withcompletion) {
                  return array();
              }
       
              // Use modinfo to get section order and also add in names
              if (empty($modinfo)) {
                  $modinfo = get_fast_modinfo($this->course);
              }
              $result = array();
              foreach ($modinfo->sections as $sectioncms) {
                  foreach ($sectioncms as $cmid) {
                      if (array_key_exists($cmid, $withcompletion)) {
                          $result[$cmid] = $withcompletion[$cmid];
                          $result[$cmid]->modname = $modinfo->cms[$cmid]->modname;
                          $result[$cmid]->name    = $modinfo->cms[$cmid]->name;
                      }
                  }
              }
       
              return $result;
          }
      

      loading modinfo for each module.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            fred Frédéric Massart added a comment -

            I've added a patch for master, but before pushing it forward I would like to create unit tests for it. Though it's a bit mysterious and so I'll delay that to later. Meantime I'll leave the default assignee as it had been set.

            Show
            fred Frédéric Massart added a comment - I've added a patch for master, but before pushing it forward I would like to create unit tests for it. Though it's a bit mysterious and so I'll delay that to later. Meantime I'll leave the default assignee as it had been set.
            Hide
            quen Sam Marshall added a comment -

            Hi, I agree this is really bad - don't understand why it was done that way. I don't think the suggested fix is the best one though.

            What I don't understand is - that function already uses get_fast_modinfo - modinfo includes the completion information that it's checking, so why isn't it using that? That would be much faster than the record_exists, never mind the get_records_sql, as it doesn't need to make any database queries because get_fast_modinfo will already be cached.

            I think the best fix would probably be:

            1. Change get_activities so that it uses modinfo to do that first check instead of the db query.
            2. Create has_activities function as per your patch, but make this just call get_activities now that it probably isn't a performance problem. (Then it can easily be optimised later if profiling identifies a problem).

            Agree that a unit test would be a good idea.

            Do you want me to code this or are you happy to keep working on it?

            Show
            quen Sam Marshall added a comment - Hi, I agree this is really bad - don't understand why it was done that way. I don't think the suggested fix is the best one though. What I don't understand is - that function already uses get_fast_modinfo - modinfo includes the completion information that it's checking, so why isn't it using that? That would be much faster than the record_exists, never mind the get_records_sql, as it doesn't need to make any database queries because get_fast_modinfo will already be cached. I think the best fix would probably be: 1. Change get_activities so that it uses modinfo to do that first check instead of the db query. 2. Create has_activities function as per your patch, but make this just call get_activities now that it probably isn't a performance problem. (Then it can easily be optimised later if profiling identifies a problem). Agree that a unit test would be a good idea. Do you want me to code this or are you happy to keep working on it?
            Hide
            fred Frédéric Massart added a comment -

            I don't mind fixing this, but you seem to know much more about the area and modinfo than I do. If you don't have much time, would you mind guiding me a bit into fixing that? I understand that modinfo is the way to go. Would you just foreach over the modules of the course and check if there is any condition set on a (visible?) module? Should this logic be part of modinfo, or get_activities() is the right place?

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - I don't mind fixing this, but you seem to know much more about the area and modinfo than I do. If you don't have much time, would you mind guiding me a bit into fixing that? I understand that modinfo is the way to go. Would you just foreach over the modules of the course and check if there is any condition set on a (visible?) module? Should this logic be part of modinfo, or get_activities() is the right place? Cheers, Fred
            Hide
            quen Sam Marshall added a comment -

            I wrote the current version of modinfo (obviously based on what was there before, so it was more restructuring than writing from scratch), so yes I do know a fair bit about it, although my memory isn't great!

            I think get_activities is the right place for it as the change is basically information that's needed as part of the completion system.

            What I was thinking of was just to call $modinfo->get_cms(), loop through the result, and see which have ->completion set to anything other than zero (COMPLETION_TRACKING_NONE); put those in the array it was making. Even if there are on average 100 activities per course, this loop should probably still be faster than a database query, and it's better to keep the number of database queries down because in large performance-sensitive Moodle deployments, it's easy to use multiple web servers but more difficult to scale the database server.

            Does that make sense?

            Show
            quen Sam Marshall added a comment - I wrote the current version of modinfo (obviously based on what was there before, so it was more restructuring than writing from scratch), so yes I do know a fair bit about it, although my memory isn't great! I think get_activities is the right place for it as the change is basically information that's needed as part of the completion system. What I was thinking of was just to call $modinfo->get_cms(), loop through the result, and see which have ->completion set to anything other than zero (COMPLETION_TRACKING_NONE); put those in the array it was making. Even if there are on average 100 activities per course, this loop should probably still be faster than a database query, and it's better to keep the number of database queries down because in large performance-sensitive Moodle deployments, it's easy to use multiple web servers but more difficult to scale the database server. Does that make sense?
            Hide
            fred Frédéric Massart added a comment - - edited

            Thanks Sam, it seems to make sense. I assigned this issue to me but I don't think I'll have time to work on it during the following days. Feel free to grab it if you need/want to. Cheers!

            Show
            fred Frédéric Massart added a comment - - edited Thanks Sam, it seems to make sense. I assigned this issue to me but I don't think I'll have time to work on it during the following days. Feel free to grab it if you need/want to. Cheers!
            Hide
            fred Frédéric Massart added a comment -

            Requesting peer review.

            So here are 4 branches with unit tests attached. I have created a new test file because I could not make use of the existing one, unless I rewrote the whole class not to use DB mock stuff. I have also added/improved has_activities() so that it does its own loop, and return true as soon as it can. And the whole thing makes use of the recommended method of modinfo.

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Requesting peer review. So here are 4 branches with unit tests attached. I have created a new test file because I could not make use of the existing one, unless I rewrote the whole class not to use DB mock stuff. I have also added/improved has_activities() so that it does its own loop, and return true as soon as it can. And the whole thing makes use of the recommended method of modinfo. Cheers, Fred
            Hide
            quen Sam Marshall added a comment -

            Thanks, I have looked through the code and I didn't spot anything wrong at all!

            Just one comment about the manual testing instructions - aren't these incorrect i.e. the report should only appear if there is actually at least one activity that actually uses completion, so adding an activity or changing activity settings should be part of the test?

            The unit tests pass (I ran both the old and new tests), so please just fix the manual instructions in this issue, then go ahead and submit for integration.

            [Y] Syntax
            [-] Output
            [Y] Whitespace
            [-] Language
            [-] Databases
            [N] Testing - Y once manual instructions corrected
            [-] Security
            [-] Documentation
            [Y] Git
            [Y] Sanity check

            Please go ahead and submit for integration.

            Show
            quen Sam Marshall added a comment - Thanks, I have looked through the code and I didn't spot anything wrong at all! Just one comment about the manual testing instructions - aren't these incorrect i.e. the report should only appear if there is actually at least one activity that actually uses completion, so adding an activity or changing activity settings should be part of the test? The unit tests pass (I ran both the old and new tests), so please just fix the manual instructions in this issue, then go ahead and submit for integration. [Y] Syntax [-] Output [Y] Whitespace [-] Language [-] Databases [N] Testing - Y once manual instructions corrected [-] Security [-] Documentation [Y] Git [Y] Sanity check Please go ahead and submit for integration.
            Hide
            poltawski Dan Poltawski added a comment -

            (I see Fred updated the testing instructions but didn't comment )

            Show
            poltawski Dan Poltawski added a comment - (I see Fred updated the testing instructions but didn't comment )
            Hide
            poltawski Dan Poltawski added a comment - - edited

            Hi Fred,

            global $DB in has_activities()?

            Show
            poltawski Dan Poltawski added a comment - - edited Hi Fred, global $DB in has_activities()?
            Hide
            fred Frédéric Massart added a comment -

            Thanks Dan, I've amended the patches.

            Show
            fred Frédéric Massart added a comment - Thanks Dan, I've amended the patches.
            Hide
            poltawski Dan Poltawski added a comment -

            Integrated to master, 25, 24 and 23 - thanks Fred

            Show
            poltawski Dan Poltawski added a comment - Integrated to master, 25, 24 and 23 - thanks Fred
            Hide
            andyjdavis Andrew Davis added a comment -

            Works as described. Passing.

            Show
            andyjdavis Andrew Davis added a comment - Works as described. Passing.
            Hide
            poltawski Dan Poltawski added a comment -

            Feature: Thanks to our superb contributors
              In order to make Moodle better
              As an integrator
              I need to thank all our contributors
             
              Scenario: Dan thanks you all
                Given I log in as "dan"
                And I see "lots of fixed issues"
                When I follow "Close integrated issues"
                Then I should see "Lots of thanks to all our contributors"
            

            Your changes are upstream

            Show
            poltawski Dan Poltawski added a comment - Feature: Thanks to our superb contributors In order to make Moodle better As an integrator I need to thank all our contributors   Scenario: Dan thanks you all Given I log in as "dan" And I see "lots of fixed issues" When I follow "Close integrated issues" Then I should see "Lots of thanks to all our contributors" Your changes are upstream

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  8/Jul/13