Moodle
  1. Moodle
  2. MDL-29236

Filter settings for Label do not work

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.1
    • Fix Version/s: 2.2
    • Component/s: Course, Filters
    • Labels:
    • Testing Instructions:
      Hide

      Easy:
      1. Ensure that the smiley filter is enabled for your site and course.
      2. Add a label with the following text:
      Happy label
      3. Save the label.

      • A smiley appears in place of the colon and right bracket.
        4. Edit the label again. In the Settings block, click Filters.
        5. The smiley filter is shown as on (default). Select Off and save changes.
        6. Go back to the course.
      • The smiley should not appear, instead showing the colon and right bracket characters.

      (Additional test now that MDL-27001 has landed)
      7. Add a forum and type an intro with in. Turn on the 'Display description on course page' checkbox. Save and return to course.

      • Intro, including smiley (as graphic) should display below the forum name.
        8. Go back to the forum and click Filters in the setting block. Turn off the smiley filter and save changes. Go back to the course.
      • In the intro below the forum name, the smiley should now display as colon and right bracket characters.

      9. Run lib/simpletest/testfilterconfig.php unittests. All them should pass.

      Show
      Easy: 1. Ensure that the smiley filter is enabled for your site and course. 2. Add a label with the following text: Happy label 3. Save the label. A smiley appears in place of the colon and right bracket. 4. Edit the label again. In the Settings block, click Filters. 5. The smiley filter is shown as on (default). Select Off and save changes. 6. Go back to the course. The smiley should not appear, instead showing the colon and right bracket characters. (Additional test now that MDL-27001 has landed) 7. Add a forum and type an intro with in. Turn on the 'Display description on course page' checkbox. Save and return to course. Intro, including smiley (as graphic) should display below the forum name. 8. Go back to the forum and click Filters in the setting block. Turn off the smiley filter and save changes. Go back to the course. In the intro below the forum name, the smiley should now display as colon and right bracket characters. 9. Run lib/simpletest/testfilterconfig.php unittests. All them should pass.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-29236-master
    • Rank:
      18778

      Description

      If you change the filter settings for a Label (for example, to disable the smiley filter) this does not have any effect. Labels always use the course filter settings.

      NOTE: I discovered this as a result of work for MDL-27001 and it will also affect the intro display from that feature. However it is an independent problem. I will be looking at it.

        Issue Links

          Activity

          Hide
          Sam Marshall added a comment -

          By the way, a trivial fix would be to change line 1380 of course/lib.php from:

          $coursecontext = get_context_instance(CONTEXT_COURSE, $course->id);

          to

          $coursecontext = get_context_instance(CONTEXT_MODULE, $cm->id);

          However this causes a dramatic performance problem (not getting the context, that should be fine - but using it to call format_text later on.)

          I'm investigating the least inefficient way I can think of to get this data.

          Show
          Sam Marshall added a comment - By the way, a trivial fix would be to change line 1380 of course/lib.php from: $coursecontext = get_context_instance(CONTEXT_COURSE, $course->id); to $coursecontext = get_context_instance(CONTEXT_MODULE, $cm->id); However this causes a dramatic performance problem (not getting the context, that should be fine - but using it to call format_text later on.) I'm investigating the least inefficient way I can think of to get this data.
          Hide
          Michael de Raadt added a comment -

          Thanks for working on this, Sam.

          Show
          Michael de Raadt added a comment - Thanks for working on this, Sam.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Sam, surely it's an idiot question but... if cm_info objects already contain the complete context objects... couldn't we be using them instead of $coursecontext?

          Edited: Ah, the problem is in the format_text() calls, got it!

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Sam, surely it's an idiot question but... if cm_info objects already contain the complete context objects... couldn't we be using them instead of $coursecontext? Edited: Ah, the problem is in the format_text() calls, got it!
          Hide
          Sam Marshall added a comment -

          Submitting this for integration, I am fairly convinced it works and efficiently fixes the bug, but not 100% certain it's desired, may need somebody to approve it (or not)..

          It works as follows:

          • A new filter_preload_activities function can be called with a course modinfo object. This loads all the filter settings for all activity contexts (CONTEXT_MODULE) on the course, using 2 database queries. Settings are stored in an in-memory cache which is only for the single request (no long-term caching).
          • The filter_get_active_in_context function, which is the one normally used to obtain this data one context at a time, was changed so that it uses this new cache if available.
          • I then changed the code that displays module content (inc. label content) on course page so that it uses the context from the course-module instead of from the course, and so that it calls the preload function.

          With these changes, the filter options now work correctly for labels.

          There is one catch: according to my testing, this adds one database query to the course view page, which is one of the most important pages in Moodle.

          I am a bit surprised it wasn't two queries (the new 'preload' function I added makes 2 queries) but this probably is because we are saved from looking up filters for course context. So, it's only one extra query - not bad - and it corrects a definite bug. Probably I think this change should go ahead but other people may disagree.

          Also note: I wrote a fairly extensive unit test for this feature (which is good because before I did so, it was broken in all sorts of ways). The filter settings stuff is a bit complicated so this was necessary. Anyway the unit test passes (eh for me).

          Show
          Sam Marshall added a comment - Submitting this for integration, I am fairly convinced it works and efficiently fixes the bug, but not 100% certain it's desired, may need somebody to approve it (or not).. It works as follows: A new filter_preload_activities function can be called with a course modinfo object. This loads all the filter settings for all activity contexts (CONTEXT_MODULE) on the course, using 2 database queries. Settings are stored in an in-memory cache which is only for the single request (no long-term caching). The filter_get_active_in_context function, which is the one normally used to obtain this data one context at a time, was changed so that it uses this new cache if available. I then changed the code that displays module content (inc. label content) on course page so that it uses the context from the course-module instead of from the course, and so that it calls the preload function. With these changes, the filter options now work correctly for labels. There is one catch: according to my testing, this adds one database query to the course view page, which is one of the most important pages in Moodle. I am a bit surprised it wasn't two queries (the new 'preload' function I added makes 2 queries) but this probably is because we are saved from looking up filters for course context. So, it's only one extra query - not bad - and it corrects a definite bug. Probably I think this change should go ahead but other people may disagree. Also note: I wrote a fairly extensive unit test for this feature (which is good because before I did so, it was broken in all sorts of ways). The filter settings stuff is a bit complicated so this was necessary. Anyway the unit test passes (eh for me).
          Hide
          Sam Marshall added a comment -

          note: I am away again next week and the week after - have added Tim as watcher, if he has time (and since he is a filter settings expert) he might be able to respond to questions and problems etc, otherwise might not get a response for a bit, so apologies in advance.

          Show
          Sam Marshall added a comment - note: I am away again next week and the week after - have added Tim as watcher, if he has time (and since he is a filter settings expert) he might be able to respond to questions and problems etc, otherwise might not get a response for a bit, so apologies in advance.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Side note: Some day we should start moving all those current globals (ACCESSLIB_PRIVATE, FILTERLIB_PRIVATE...) to class properties or singleton-like objects, easy to access/handle and, why not, easy to apply advanced caching solutions to them.

          Show
          Eloy Lafuente (stronk7) added a comment - Side note: Some day we should start moving all those current globals (ACCESSLIB_PRIVATE, FILTERLIB_PRIVATE...) to class properties or singleton-like objects, easy to access/handle and, why not, easy to apply advanced caching solutions to them.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And, if I'm not wrong, this fixes both the labels and the "description below link" ones, correct? Perhaps we should re-title it a bit and add some extra testing steps for, say, forum and assignment modules or so?

          Show
          Eloy Lafuente (stronk7) added a comment - And, if I'm not wrong, this fixes both the labels and the "description below link" ones, correct? Perhaps we should re-title it a bit and add some extra testing steps for, say, forum and assignment modules or so?
          Hide
          Sam Marshall added a comment -

          Yes you're right this should also fix cases when the intro is displayed below the module. There are other cases in third party modules that will also be fixed. (E.g. we have an RSS feed module that puts loads of content on the course page; I can imagine we might need to tweak filters for that sometimes.)

          Updated test to add a check using forum intro. It's the same code, so probably that's enough.

          Show
          Sam Marshall added a comment - Yes you're right this should also fix cases when the intro is displayed below the module. There are other cases in third party modules that will also be fixed. (E.g. we have an RSS feed module that puts loads of content on the course page; I can imagine we might need to tweak filters for that sometimes.) Updated test to add a check using forum intro. It's the same code, so probably that's enough.
          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
          Eloy Lafuente (stronk7) added a comment -

          Just guessing if now that we have implemented that $FILTERLIB_PRIVATE caching, we would get any benefit (in other places) by also adding to it the information calculated in filter_get_active_in_context() (1 individual context active & configs) before returning it.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Just guessing if now that we have implemented that $FILTERLIB_PRIVATE caching, we would get any benefit (in other places) by also adding to it the information calculated in filter_get_active_in_context() (1 individual context active & configs) before returning it. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I became a bit crazy trying to understand the login within filter_preload_activities() but I guess (and hope) it's ok.

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - I became a bit crazy trying to understand the login within filter_preload_activities() but I guess (and hope) it's ok. Integrated, thanks!
          Hide
          Sam Hemelryk added a comment -

          Tested... passed... thanks

          Show
          Sam Hemelryk added a comment - Tested... passed... thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          YTC !

          (aka, yay, thanks and ciao ) Closing.

          Show
          Eloy Lafuente (stronk7) added a comment - YTC ! (aka, yay, thanks and ciao ) Closing.

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: