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

Certain HTML tags break courses when used as condition for access

    Details

    • Testing Instructions:
      Hide

      0. Create a new course with completion tracking enabled.
      1. In first week, add a new Label with one paragraph 'Simple text'.
      2. In second week, add a new Assignment with arbitrary name/description. Under 'Restrict access', choose that 'Simple text' must be marked 'Complete'.
      3. In third week, add a new Label with two paragraphs 'Multiple' and then 'Paragraphs'.
      4. In fourth week, add a new Assignment with arbitrary name/description. Under 'Restrict access', choose that 'Multiple paragraphs' must be marked 'Complete'.
      5. As admin, look at the module page with editing on.

      EXPECTED: The restriction display for both Assignments are the same. The following weeks (below all these activities) display correctly.

      6. Still as admin, turn editing off and look at module page.

      EXPECTED: As above.

      7. Enrol a test student account in the course.
      8. Log in as the test student and look at the course.

      EXPECTED: As above.

      Show
      0. Create a new course with completion tracking enabled. 1. In first week, add a new Label with one paragraph 'Simple text'. 2. In second week, add a new Assignment with arbitrary name/description. Under 'Restrict access', choose that 'Simple text' must be marked 'Complete'. 3. In third week, add a new Label with two paragraphs 'Multiple' and then 'Paragraphs'. 4. In fourth week, add a new Assignment with arbitrary name/description. Under 'Restrict access', choose that 'Multiple paragraphs' must be marked 'Complete'. 5. As admin, look at the module page with editing on. EXPECTED: The restriction display for both Assignments are the same. The following weeks (below all these activities) display correctly. 6. Still as admin, turn editing off and look at module page. EXPECTED: As above. 7. Enrol a test student account in the course. 8. Log in as the test student and look at the course. EXPECTED: As above.
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull Master Branch:
      MDL-42582-master

      Description

      When enableavailability is enabled, certain html tags in labels can break a course when being used as a dependency.

      An example would be:

      Create a label with the following html and allow students to manually mark it complete:

      <hr />
      <p><strong>Topic 0 completion</strong></p>
      <p>When you have completed the activities above and are ready to continue, tick the checkbox on the right to open the next topic.</p>

      Then in the next topic create an assignment and use that label to restrict access to it. (use it as activity completion condition). It will break the course layout.

      However, if you change the html in the label to:

      <p><strong>Topic 1 completion<br /><br /></strong>Once you have completed updating your profile and uploading a profile photo, tick the checkbox to the right to open the next topic.</p>

      The course will no longer be broken.

      I suspect it has something to do with the <hr /> and multiple <p></p> tags.

      If you need some screen shots let me know.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            quen Sam Marshall added a comment -

            I can reproduce the problem in current master (2.6beta). It seems like the 'Restricted:' message is displayed totally incorrectly. Strangely, I don't think it's accidentally using the hr tag or anything.

            Show
            quen Sam Marshall added a comment - I can reproduce the problem in current master (2.6beta). It seems like the 'Restricted:' message is displayed totally incorrectly. Strangely, I don't think it's accidentally using the hr tag or anything.
            Hide
            quen Sam Marshall added a comment -

            I've written a more specific test script. It seems that any label with multiple paragraphs causes this problem. This applies to Moodle 2.5+ (not 2.4).

            Show
            quen Sam Marshall added a comment - I've written a more specific test script. It seems that any label with multiple paragraphs causes this problem. This applies to Moodle 2.5+ (not 2.4).
            Hide
            quen Sam Marshall added a comment -

            This is a minimal fix to the bug by correcting both instances of this regular expression. (Arguably, there should be a shared function that handles this, but since this can be fixed with 2 lines changed...)

            Show
            quen Sam Marshall added a comment - This is a minimal fix to the bug by correcting both instances of this regular expression. (Arguably, there should be a shared function that handles this, but since this can be fixed with 2 lines changed...)
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Sam,

            Patch looks good, one question is there a need to check for empty spaces, before and after <li> ? I am not sure if that is needed. Setting PCRE_DOTALL should be sufficient.

            Also, on other note, it breaks theme and blocks appear below ... should this be a "MUST FIX" ? Added integrators here as watcher for there thoughts.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Sam, Patch looks good, one question is there a need to check for empty spaces, before and after <li> ? I am not sure if that is needed. Setting PCRE_DOTALL should be sufficient. Also, on other note, it breaks theme and blocks appear below ... should this be a "MUST FIX" ? Added integrators here as watcher for there thoughts.
            Hide
            quen Sam Marshall added a comment -

            This is a while back so I don't remember, but I think you may be right that the \s* is not strictly needed and it is only the added 's' that is strictly required to fix the problem. I don't think the \s* does any harm either, though! Makes it a bit safer in case for some reason it returns an <li>...</li> followed by an LF. (I don't think it really does that at present.)

            Would you like me to revise the patch to remove the probably-unnecessary whitespace matches?

            Show
            quen Sam Marshall added a comment - This is a while back so I don't remember, but I think you may be right that the \s* is not strictly needed and it is only the added 's' that is strictly required to fix the problem. I don't think the \s* does any harm either, though! Makes it a bit safer in case for some reason it returns an <li>...</li> followed by an LF. (I don't think it really does that at present.) Would you like me to revise the patch to remove the probably-unnecessary whitespace matches?
            Hide
            quen Sam Marshall added a comment -

            ugh, by 'a while back' I appear to mean 2 weeks ago. I think my memory is getting shorter as I get older...

            Show
            quen Sam Marshall added a comment - ugh, by 'a while back' I appear to mean 2 weeks ago. I think my memory is getting shorter as I get older...
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Sam,

            I agree there is no harm in keeping \s*, so not sure if patch needs to be revised. Pushing it for integration, in case integrators feels strongly about it then you might have to revise it.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Sam, I agree there is no harm in keeping \s*, so not sure if patch needs to be revised. Pushing it for integration, in case integrators feels strongly about it then you might have to revise it.
            Hide
            stronk7 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
            stronk7 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
            quen Sam Marshall added a comment -

            Rebased.

            Show
            quen Sam Marshall added a comment - Rebased.
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Sam,

            Integrated to 25, 26 and master.

            I would have preferred it without the unnecessary whitespace checking - but it wasn't a big enough deal to hold anything up.

            Show
            damyon Damyon Wiese added a comment - Thanks Sam, Integrated to 25, 26 and master. I would have preferred it without the unnecessary whitespace checking - but it wasn't a big enough deal to hold anything up.
            Hide
            dmonllao David Monllaó added a comment -

            It passes. Tested in 25 and 26

            Show
            dmonllao David Monllaó added a comment - It passes. Tested in 25 and 26
            Hide
            dmonllao David Monllaó added a comment -

            Ups, I forgot to click passed...

            Show
            dmonllao David Monllaó added a comment - Ups, I forgot to click passed...
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            ...
            But still, I thank you, for you made me stronger…

            Stronger as the beast that roar in the wild
            Stronger as the storm across the ocean
            Stronger as the diamond that won’t break
            Stronger enough to take all heart aches….

            I thank you my friend, for you made me stronger…

            ---- Juneah Landicho

            Closing as fixed. Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - ... But still, I thank you, for you made me stronger… Stronger as the beast that roar in the wild Stronger as the storm across the ocean Stronger as the diamond that won’t break Stronger enough to take all heart aches…. I thank you my friend, for you made me stronger… ---- Juneah Landicho Closing as fixed. Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  13/Jan/14