Moodle
  1. Moodle
  2. MDL-31631

Navigation block: Unnecessary cache can cause deleted activities to remain listed

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.1.5, 2.2.2, 2.3
    • Fix Version/s: 2.1.6, 2.2.3
    • Component/s: Navigation
    • Labels:
    • Testing Instructions:
      Hide

      You will need a course that has one section containing multiple activities. I'm referring to these as A and B. For example, on qa.moodle.net you could use the 'Upload a single file' and 'Offline activity' activities.

      1. In one browser, log in as student and go to Activity A.
      + Note that Activity B is shown in the navigation menu.
      2. In another browser, log in as manager and delete Activity B.
      3. Back in the student's browser, hit Reload.

      Expected result:

      • Activity B is no longer shown in the navigation.

      Actual result:

      • Activity B remains in the navigation. Clicking it gives an 'Invalid course module ID' error.
      Show
      You will need a course that has one section containing multiple activities. I'm referring to these as A and B. For example, on qa.moodle.net you could use the 'Upload a single file' and 'Offline activity' activities. 1. In one browser, log in as student and go to Activity A. + Note that Activity B is shown in the navigation menu. 2. In another browser, log in as manager and delete Activity B. 3. Back in the student's browser, hit Reload. Expected result: Activity B is no longer shown in the navigation. Actual result: Activity B remains in the navigation. Clicking it gives an 'Invalid course module ID' error.
    • Workaround:
      Hide

      Wait ten minutes or so, then it sorts itself out.

      Show
      Wait ten minutes or so, then it sorts itself out.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      wip-MDL-31631-m23
    • Rank (Obsolete):
      38243

      Description

      The navigation block caches data about course activities in the user session. This is bad for two reasons:

      1. It causes unexpected results if a teacher creates or deletes an activity while students are using the site (for some minutes, the deleted activity link will still be available but give an error).

      2. This information is already cached in modinfo.

      As modinfo can be obtained free-of-charge in most requests, this means that this part of the cache is unnecessary and is wasting user session space. If the course has hundreds of activities, it might be a moderate proportion of session space.

      There are probably other aspects of the navigation block that are not cached in modinfo and therefore it is convenient to cache the whole thing even the bit that is not necessary, but I think somebody should at least consider this; it might be possible to avoid storing the part that comes from modinfo in cache.

      Note: I'm not certain all the necessary data about sessions is in modinfo. But if it isn't, it totally ought to be.

      The consequences of this bug are not very serious so unfortunately I don't have time to code it at the moment, I guess maybe other people will also not have time, but, thought it was worth filing it after testing staff here reported the problem.

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          Hi Sam,

          I absolutely agree, I think after all of the changes that both the navigation and mod info have gone through those caches are either no longer needed, or require serious consideration and reworking to ensure that we only store things we won't already have.
          I've marked this triaged now and added it to the stable backlog.
          It is something that I've thought about in the past while while working on other navigation issues so I may try to get to this one sooner rather than later. I've given it a higher priority both to get it up my list and to reflect the performance improvements that this perhaps can lead to.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Sam, I absolutely agree, I think after all of the changes that both the navigation and mod info have gone through those caches are either no longer needed, or require serious consideration and reworking to ensure that we only store things we won't already have. I've marked this triaged now and added it to the stable backlog. It is something that I've thought about in the past while while working on other navigation issues so I may try to get to this one sooner rather than later. I've given it a higher priority both to get it up my list and to reflect the performance improvements that this perhaps can lead to. Cheers Sam
          Hide
          Sam Marshall added a comment -

          Great - thanks. By the way - if you find that in order to implement it you would need some extra data in modinfo, I'm happy to code that part as a subtask if you would prefer. (Since I did a bunch of stuff in modinfo some while back, I'm fairly familiar with it.) Of course also happy for you to do it all.

          Show
          Sam Marshall added a comment - Great - thanks. By the way - if you find that in order to implement it you would need some extra data in modinfo, I'm happy to code that part as a subtask if you would prefer. (Since I did a bunch of stuff in modinfo some while back, I'm fairly familiar with it.) Of course also happy for you to do it all.
          Hide
          Sam Hemelryk added a comment -

          Ready for peer-review, changes are very simple although I need to continue investigating the performance implications.

          Show
          Sam Hemelryk added a comment - Ready for peer-review, changes are very simple although I need to continue investigating the performance implications.
          Hide
          Sam Marshall added a comment -

          If I am allowed to review it, well, I just did

          This change looks correct to me.

          Regarding performance, at present, this will potentially increase DB queries by 1 because of the get_all_sections call (when looking at a page that does not usually show sections, such as a an activity page).

          However, the other changes I'm working on in MDL-24419 will switch that get_all_sections call to use the section cache which will then be included in get_fast_modinfo so will not require any DB queries. (get_fast_modinfo is already called on essentially all pages, so calling it is always free in terms of DB requests.)

          Show
          Sam Marshall added a comment - If I am allowed to review it, well, I just did This change looks correct to me. Regarding performance, at present, this will potentially increase DB queries by 1 because of the get_all_sections call (when looking at a page that does not usually show sections, such as a an activity page). However, the other changes I'm working on in MDL-24419 will switch that get_all_sections call to use the section cache which will then be included in get_fast_modinfo so will not require any DB queries. (get_fast_modinfo is already called on essentially all pages, so calling it is always free in terms of DB requests.)
          Hide
          Sam Hemelryk added a comment -

          Cool thanks for looking at that Sam. Putting it up for integration now.

          Thanks for reporting on the performance btw, I had done some preliminary tests myself and whilst definitely seeing a drop in session size I wasn't sure about the database side of things so good to know!

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Cool thanks for looking at that Sam. Putting it up for integration now. Thanks for reporting on the performance btw, I had done some preliminary tests myself and whilst definitely seeing a drop in session size I wasn't sure about the database side of things so good to know! Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Aparup Banerjee added a comment -

          Hi Sam (and Sam),

          this all looks good until i saw the changes in 2.1 branch undoing commit 9b0e51000ed4b75861f82dee3471f316f02c597f (MDL-29224)

          i'm not sure i understand what this has to do with caching issue here.

          Show
          Aparup Banerjee added a comment - Hi Sam (and Sam), this all looks good until i saw the changes in 2.1 branch undoing commit 9b0e51000ed4b75861f82dee3471f316f02c597f ( MDL-29224 ) i'm not sure i understand what this has to do with caching issue here.
          Hide
          Sam Hemelryk added a comment -

          Good spotting Apu, I've fixed that up now and its all ready to go again

          Show
          Sam Hemelryk added a comment - Good spotting Apu, I've fixed that up now and its all ready to go again
          Hide
          Aparup Banerjee added a comment -

          Thank you Sam.
          Thank you Sam.

          This has been integrated now into 21, 22 and master.

          should be a quick test.

          Show
          Aparup Banerjee added a comment - Thank you Sam. Thank you Sam. This has been integrated now into 21, 22 and master. should be a quick test.
          Hide
          Aparup Banerjee added a comment -

          yup this works for me (twice now)

          navigation menu in course page has no cached menu artifacts after activity deletion.

          Show
          Aparup Banerjee added a comment - yup this works for me (twice now) navigation menu in course page has no cached menu artifacts after activity deletion.
          Hide
          Aparup Banerjee added a comment -

          The code here has been spread to upstream moodle repositories and mirrors for anyone to use .

          Closing, have a good weekend!

          Show
          Aparup Banerjee added a comment - The code here has been spread to upstream moodle repositories and mirrors for anyone to use . Closing, have a good weekend!

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: