Moodle
  1. Moodle
  2. MDL-30592

Quiz statistics report - images in the question text do not show up

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.3, 2.2, 2.3
    • Fix Version/s: 2.1.4, 2.2.1
    • Component/s: Quiz
    • Labels:
    • Testing Instructions:
      Hide

      1. Create a question with an image in the question text.
      2. Go to the question bank, and turn on "Show question text in the question list". Verify that the image appears in the question text.
      3. Add the question (and any others you like) to a new quiz, and attempt it as a student.
      4. As teacher, go to the statistics report for the quiz.
      5. Download the full report as XHTML. Verify that the images in the question texts show up.
      6. Click on the question name in the Quiz structure analysis. On the page you get to, verify that the image in the question text shows up.

      Note that the cherry-pick to 21 was not clean. In particular changes had to be merged in plugin_callback. Please also test the rating and comments systems, to verify that there are no regressions.

      Show
      1. Create a question with an image in the question text. 2. Go to the question bank, and turn on "Show question text in the question list". Verify that the image appears in the question text. 3. Add the question (and any others you like) to a new quiz, and attempt it as a student. 4. As teacher, go to the statistics report for the quiz. 5. Download the full report as XHTML. Verify that the images in the question texts show up. 6. Click on the question name in the Quiz structure analysis. On the page you get to, verify that the image in the question text shows up. Note that the cherry-pick to 21 was not clean. In particular changes had to be merged in plugin_callback. Please also test the rating and comments systems, to verify that there are no regressions.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      33376

      Description

      Create a question with an image in the question text. Attempt the quiz a few times as different students, then go to the statistics report, and drill down into the detailed analysis of one question.

      The images does not appear (there is just a PLUGINFILE URL output).

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          This also affects 'display question text' in the question bank.

          Show
          Tim Hunt added a comment - This also affects 'display question text' in the question bank.
          Hide
          Tim Hunt added a comment -

          Petr, I would be most grateful if you could take a quick look at this and tell me whether you think this is sensible. Thanks.

          Show
          Tim Hunt added a comment - Petr, I would be most grateful if you could take a quick look at this and tell me whether you think this is sensible. Thanks.
          Hide
          Petr Škoda added a comment -

          I like the component related changes - component means any system subsystem or plugin type+name, I think it is better than separate $plugintype + $pluginname parameters. The rest makes sense too.

          Show
          Petr Škoda added a comment - I like the component related changes - component means any system subsystem or plugin type+name, I think it is better than separate $plugintype + $pluginname parameters. The rest makes sense too.
          Hide
          Tim Hunt added a comment -

          Thanks Petr. I plan to test some more tomorrow morning, then submit for integration.

          Show
          Tim Hunt added a comment - Thanks Petr. I plan to test some more tomorrow morning, then submit for integration.
          Hide
          Tim Hunt added a comment -

          I am now that this is ready to be integrated, but I want to wait until MDL-29579 is integrated, then rebase, before submitting this one for integration.

          Show
          Tim Hunt added a comment - I am now that this is ready to be integrated, but I want to wait until MDL-29579 is integrated, then rebase, before submitting this one for integration.
          Hide
          Tim Hunt added a comment -

          Submitting for integration now.

          Show
          Tim Hunt added a comment - Submitting for integration now.
          Hide
          Sam Hemelryk added a comment -

          Hi Tim,

          Things look nearly 100% spot on thanks and great to have the callback thing finally sorted.
          Sending this back at the moment so a couple of minor things that can be cleaned up.

          1. component_callback exceptions still refer to plugin_callback.
          2. upgrade.txt should be updated to include move from plugin_callback to component_callback
          3. Personally I think that existing plugin_callback should be upgraded as part of this change. I'm open to discussion on this, however there are only 10 so it's not a big task.

          As I imagine that this will be back up promptly feel free to ask Eloy to integrate it (I am 100% happy for this to be taken).

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Tim, Things look nearly 100% spot on thanks and great to have the callback thing finally sorted. Sending this back at the moment so a couple of minor things that can be cleaned up. component_callback exceptions still refer to plugin_callback. upgrade.txt should be updated to include move from plugin_callback to component_callback Personally I think that existing plugin_callback should be upgraded as part of this change. I'm open to discussion on this, however there are only 10 so it's not a big task. As I imagine that this will be back up promptly feel free to ask Eloy to integrate it (I am 100% happy for this to be taken). Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Used the wrong button sorry for the spam

          Show
          Sam Hemelryk added a comment - Used the wrong button sorry for the spam
          Hide
          Tim Hunt added a comment -

          Hmm. I was not planning to deprecate the old plugin_callback API, just to introduce a new one (and then refactor to eliminate duplicated code). Is the old plugin_callback so terrible that it needs to be killed? If not, can't we just have a new MDL for that in future (assigned to moodle.com).

          Show
          Tim Hunt added a comment - Hmm. I was not planning to deprecate the old plugin_callback API, just to introduce a new one (and then refactor to eliminate duplicated code). Is the old plugin_callback so terrible that it needs to be killed? If not, can't we just have a new MDL for that in future (assigned to moodle.com).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I've created MDL-30743... about to decide to deprecate & drop plugin_callback()

          Show
          Eloy Lafuente (stronk7) added a comment - I've created MDL-30743 ... about to decide to deprecate & drop plugin_callback()
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Getting this for integration...

          Show
          Eloy Lafuente (stronk7) added a comment - Getting this for integration...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been integrated (21, 22 and master), thanks!

          One small commit was added on top tidying phpdocs and exception message.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This has been integrated (21, 22 and master), thanks! One small commit was added on top tidying phpdocs and exception message. Ciao
          Hide
          Rossiani Wijaya added a comment -

          Hi Tim,

          While testing the statistics' report, the following error occurs (debug: developer mode).

          Notice: Undefined variable: context in /home/moodle/mod/quiz/report/statistics/report.php on line 88

          Looking at the patch it just needs a minor tweak to $this->context.

          Test failed.

          Show
          Rossiani Wijaya added a comment - Hi Tim, While testing the statistics' report, the following error occurs (debug: developer mode). Notice: Undefined variable: context in /home/moodle/mod/quiz/report/statistics/report.php on line 88 Looking at the patch it just needs a minor tweak to $this->context. Test failed.
          Hide
          Tim Hunt added a comment -

          Ah, the problem was that MDL-27183 added a new reference to $context, so when that change was merged with this, we got the error you report. I will do one more small commit to fix this in a couple of hours when I get into work.

          Show
          Tim Hunt added a comment - Ah, the problem was that MDL-27183 added a new reference to $context, so when that change was merged with this, we got the error you report. I will do one more small commit to fix this in a couple of hours when I get into work.
          Show
          Tim Hunt added a comment - Fixup commit ready for integration: https://github.com/timhunt/moodle/compare/master...MDL-30592_fixup https://github.com/timhunt/moodle/compare/MOODLE_22_STABLE...MDL-30592_fixup_22 https://github.com/timhunt/moodle/compare/MOODLE_21_STABLE...MDL-30592_fixup_21
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Re-integrated with the $this->context fixup.

          Show
          Eloy Lafuente (stronk7) added a comment - Re-integrated with the $this->context fixup.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Uhm, SamH just noticed that this in 21_STABLE is leading to tons of debug warnings... looking back... it seems that surely we'll need to backport the changes in comments stuff done by MDL-29504 (commit 60c452b4) to avoid them asap.

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm, SamH just noticed that this in 21_STABLE is leading to tons of debug warnings... looking back... it seems that surely we'll need to backport the changes in comments stuff done by MDL-29504 (commit 60c452b4) to avoid them asap.
          Hide
          Sam Hemelryk added a comment -

          Created MDL-30791 to fix those notices. Has been peer-reviewed by Tim and is just waiting integration now.

          Show
          Sam Hemelryk added a comment - Created MDL-30791 to fix those notices. Has been peer-reviewed by Tim and is just waiting integration now.
          Hide
          Rossiani Wijaya added a comment -

          This is working great.

          Thanks for fixing this Tim.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working great. Thanks for fixing this Tim. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks!

          Now... disconnect, relax and enjoy the next days, yay!

          Closing...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks! Now... disconnect, relax and enjoy the next days, yay! Closing...ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: