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

Activity modules creating calendar events apply filters on the event description

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 3.7.5, 3.8.2, 3.9
    • Fix Version/s: 3.9
    • Component/s: Unit tests
    • Labels:
    • Testing Instructions:
      Hide

      Fix tests

      • Covered by unit tests that must pass even after MDL-68563 (which must be integrated before this)

      Regression tests (all branches!)

      1. As an admin, enable the multi-language content filter on the site and install one additional language pack.
      2. Create a course and enrol a teacher and a student into it.
      3. Log in as the teacher and go to the course.
      4. Create one instance of every activity module that creates a calendar event for something happening in the module (e.g. submissions deadline etc.). This should include Assignment, Chat, Choise, Database, Feedback, Forum, Lesson, Quiz, SCORM and Workshop.
      5. When adding these activities, embed an image into their Description and use the multi-language syntax in the description, e.g.

        <span lang="en" class="multilang">ENGLISH TEXT HERE</span>
        <span lang="cs" class="multilang">ČESKÝ TEXT ZDE</span>
        

      6. Log out and log in as the student. Go to the calendar.
      7. Check that the calendar contains the events for the activity modules. Make sure that multi-language content works there (changing the language must change the shown text) and that the event description displays the images embedded to the descriptions.
      Show
      Fix tests Covered by unit tests that must pass even after MDL-68563 (which must be integrated before this) Regression tests (all branches!) As an admin, enable the multi-language content filter on the site and install one additional language pack. Create a course and enrol a teacher and a student into it. Log in as the teacher and go to the course. Create one instance of every activity module that creates a calendar event for something happening in the module (e.g. submissions deadline etc.). This should include Assignment, Chat, Choise, Database, Feedback, Forum, Lesson, Quiz, SCORM and Workshop. When adding these activities, embed an image into their Description and use the multi-language syntax in the description, e.g. <span lang="en" class="multilang">ENGLISH TEXT HERE</span> <span lang="cs" class="multilang">ČESKÝ TEXT ZDE</span> Log out and log in as the student. Go to the calendar. Check that the calendar contains the events for the activity modules. Make sure that multi-language content works there (changing the language must change the shown text) and that the event description displays the images embedded to the descriptions.
    • Affected Branches:
      MOODLE_37_STABLE, MOODLE_38_STABLE, MOODLE_39_STABLE
    • Fixed Branches:
      MOODLE_39_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-68645-master-earlyoutputinit

      Description

      Notes on the origin of the issue

      This issue was discovered during test of MDL-68563 where the filter_emoticon plugin was changed to be enabled by default.

      It was originally reported as "Data generators use incorrect COURSE, PAGE and OUTPUT vars" and it was to implement the the third option in the below choices. The tl;dr of which is:

      Update data generators to create new page globals ($PAGE, $OUTPUT, and $COURSE) during data generation to better simulate the user actions when we create content.

      However, it turned out that there is nothing wrong with tests setup. As was discovered in comments below, the primary cause of the problems is that some activity modules call format_module_intro() with enabled filtering when they create calendar events. That call triggers the output initialisation because filters may need a theme which in turn need the page to be set up.

      The details below are copied/pasted from a comment in MDL-68563 and they may help to understand how we got here.

      Explanation

      The issue is two-fold...

      The filter_emoticon needs the current theme in order to get the set of emoticons that it should use.
      When we fetch the theme we initialise the theme and output.
      That behaviour is 100% correct. By the time that the theme is set up things cannot change and we can't change the theme once it is set in the $PAGE.
      The things which require that the theme has not yet been set include:

      1. $PAGE->set_course();
      2. $PAGE->set_category_by_id();
      3. $PAGE->force_theme();
      4. $PAGE->set_cm();
        In all of these cases it's because setting a specific context can change the theme - i.e. because there is a course, or course category theme (or because a page has explicitly set the theme for that page).

      With the change in MDL-68563, filter_emoticon is now enabled by default. It therefore gets called any time that format text is called.

      It just so happens that the process of creating a new activity/resource includes a call to format_text() which, of course, now includes filtering through filter_emoticon.

      It also just so happens that many of our unit tests include data generation such as the creation of modules.

      In most of our unit tests that isn't a problem because the unit tests do not do anything that would cause any of the above methods to be called, and they often do not do anything to touch anything else relating to $OUTPUT or $PAGE. In the cases where we do so deliberately we usually already know about this limitation (because we're doing things like testing moodle_page and we are therefore already mocking or re-creating it.

      So with the change in MDL-68563, we have inadvertently moved the init of theme to a much earlier point in most unit tests - but as I say, no real problem there.

      However... this does become a problem when we have a unit test which calls a function like require_login, or require_course_login, because these are both responsible for calling some of these functions on $PAGE (mostly set_course and set_cm. In these cases we are typically creating test data (like activities) as part of a test set up, then doing some more preparation, then calling the function under test, which calls require_login(), which tries to set the current course/cm, which ensures that the theme is not set, and then throws an Exception.

      It's worth pointing out that the filter_emoticon, the moodle_page functions, the require_login calls, and the data generators are all behaving correctly, but we hit this when they are used together because of the unusual order when they are used together.

      Possible solutions

      There are a few things that we can do to work around this:

      Disable the filter

      We could disable the filter for unit tests.

      Note: see MDL-68563 for more info.

      Modify the filter

      We can modify the filter to take the current page state into account and not initialise the global $PAGE if it has not been started.
      We probably only want to do this for unit tests, because in normal circumstances this could give us incorrect behaviour. A patch like this would probably be adequate:

      Note: see MDL-68563 for more info.

      Update data generators to create a new global $PAGE during generation

      Essentially this would mean that every time we create things like activities (and possibly blocks) as part of the test data setup, we push the current $OUTPUT and $PAGE variables onto a stack, and generate new ones, and then after the test data is setup, we restore them.
      Ideally we would also then set the current course on the $PAGE as part of that setup.

      Strictly speaking this is a much more correct approach anyway. There's a chance that this issue has the ability to cause other weird bugs in unit tests, though probably only for those specifically unit testing a theme, or those unit testing features such as course and category theming. Since both of these are atypical we probably aren't currently affected, though those in the community may be.

      Pros

      1. Unit tests should pass
      2. Is a more 'correct' approach as we will always be using the correct $PAGE variable with the correct course and therefore the correct course theme

      Cons

      1. More complex
      2. May cause slowdown on unit tests (hopefully minor).

        Attachments

          Issue Links

            Activity

              People

              Assignee:
              mudrd8mz David Mudrák (@mudrd8mz)
              Reporter:
              dobedobedoh Andrew Nicols
              Peer reviewer:
              Tim Hunt
              Integrator:
              Eloy Lafuente (stronk7)
              Tester:
              Anna Carissa Sadia
              Participants:
              Component watchers:
              Andrew Nicols, Jun Pataleta, Michael Hawkins, Shamim Rezaie, Simey Lameze
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Fix Release Date:
                15/Jun/20

                  Time Tracking

                  Estimated:
                  Original Estimate - 0 minutes
                  0m
                  Remaining:
                  Remaining Estimate - 0 minutes
                  0m
                  Logged:
                  Time Spent - 1 day, 2 hours, 30 minutes
                  1d 2h 30m