Moodle
  1. Moodle
  2. MDL-29411

questiontext images not displayed in randomsamatch generated questions

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.5
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      1. In a question category, create a number of shortanswer questions, all including an image in their question text.
      2. In that question category, create a Random Short-Answer Matching question using those questions.
      3. View the Random Short-Answer Matching question. The images which should be displayed in the left column (sub-questions) are absent.

      Show
      1. In a question category, create a number of shortanswer questions, all including an image in their question text. 2. In that question category, create a Random Short-Answer Matching question using those questions. 3. View the Random Short-Answer Matching question. The images which should be displayed in the left column (sub-questions) are absent.
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      19382

      Description

      randomsamatch question type does not display images which are contained in the text of the short-answer questions it uses

      diagnostic
      ransomsamatch question type "Extends 'match' as there are quite a few simularities...", but there is a problem
      in question/type/match/questiontype.php function print_question_formulation_and_controls(&$question, &$state, $cmoptions, $options)
      line 287
      $text = quiz_rewrite_question_urls($subquestion->questiontext, 'pluginfile.php', $context->id, 'qtype_match', 'subquestion', array($state->attempt, $state->question), $subquestion->id);
      should be re-written as:
      $text = quiz_rewrite_question_urls($subquestion->questiontext, 'pluginfile.php', $context->id, 'question', 'questiontext', array($state->attempt, $state->question), $subquestion->id);

      the only solution I see at this stage is to NOT rely on the match function print_question_formulation_and_controls() but to re-write that function inside of question/type/randomsamatch/questiontype.php, with the correction mentioned above: 'question', 'questiontext'...

      Tested successfully on Moodle 2.0.4+ (Build: 20110916).

      NOTE: this does not apply to moodle 1.9 since - unfortunately - it is not possible to use images in a match question in that moodle version (except with a workaround)

        Activity

        Hide
        Tim Hunt added a comment -

        This patch is not a sufficiently good solution for the problem.

        It is not acceptable to copy-and-paste 89 lines of code just because you need to change one line. You need to refactor the code to eliminate the duplication.

        Show
        Tim Hunt added a comment - This patch is not a sufficiently good solution for the problem. It is not acceptable to copy-and-paste 89 lines of code just because you need to change one line. You need to refactor the code to eliminate the duplication.
        Hide
        Joseph Rézeau added a comment -

        Of course, but which code should be "refactored" : the code in randomsamatch/questiontype.php or in match/questiontype.php ? The latter, I suppose?
        Thanks for advice and I will try to do it.

        Show
        Joseph Rézeau added a comment - Of course, but which code should be "refactored" : the code in randomsamatch/questiontype.php or in match/questiontype.php ? The latter, I suppose? Thanks for advice and I will try to do it.
        Hide
        Joseph Rézeau added a comment -

        Hi Tim, the following proposed changes work as expected, please test.
        https://github.com/rezeau/moodle/compare/MOODLE_20_STABLE...MDL-29411-20

        Joseph
        PS.- You will notice that I am at long last using my github to propose bug fixes!

        Show
        Joseph Rézeau added a comment - Hi Tim, the following proposed changes work as expected, please test. https://github.com/rezeau/moodle/compare/MOODLE_20_STABLE...MDL-29411-20 Joseph PS.- You will notice that I am at long last using my github to propose bug fixes!
        Hide
        Tim Hunt added a comment -

        Thanks for using git. Makes it much easier for me.

        That is not the right approach, however. You can't change the API of a method like print_question_formulation_and_controls that is defined in the base class. It is asking for trouble in future.

        The right sort of approach looks like https://github.com/timhunt/moodle/compare/MOODLE_20_STABLE...MDL-29411_20. Note that I have not tested this at all. I just did the code to show you the general idea.

        Show
        Tim Hunt added a comment - Thanks for using git. Makes it much easier for me. That is not the right approach, however. You can't change the API of a method like print_question_formulation_and_controls that is defined in the base class. It is asking for trouble in future. The right sort of approach looks like https://github.com/timhunt/moodle/compare/MOODLE_20_STABLE...MDL-29411_20 . Note that I have not tested this at all. I just did the code to show you the general idea.
        Hide
        Joseph Rézeau added a comment -

        Hi Tim,
        Have just tested your fix at https://github.com/timhunt/moodle/commits/MDL-29411_20.
        It is working just as expected. Good job! Hope you will integrate this in next week's 2.0 release.
        Joseph

        PS Now for the trickier job of making randomsamatch work in 2.1+ ...

        Show
        Joseph Rézeau added a comment - Hi Tim, Have just tested your fix at https://github.com/timhunt/moodle/commits/MDL-29411_20 . It is working just as expected. Good job! Hope you will integrate this in next week's 2.0 release. Joseph PS Now for the trickier job of making randomsamatch work in 2.1+ ...
        Hide
        Tim Hunt added a comment -

        Thanks Joseph. Submitting for integration.

        Note to integrators. This fix is not relevant to any other branch.

        Show
        Tim Hunt added a comment - Thanks Joseph. Submitting for integration. Note to integrators. This fix is not relevant to any other branch.
        Hide
        Sam Hemelryk added a comment -

        Hi Tim,

        The solution looks fine however I'd like to check with you the new function name format_sumquestion_text shouldn't that be format_subquestion_text?

        Once sorted I'll integrate.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Tim, The solution looks fine however I'd like to check with you the new function name format_sumquestion_text shouldn't that be format_subquestion_text ? Once sorted I'll integrate. Cheers Sam
        Hide
        Tim Hunt added a comment -

        Drat! yes. If you've got the code open in your editor, can you just fix it? If not, I will do it later. Thanks.

        Show
        Tim Hunt added a comment - Drat! yes. If you've got the code open in your editor, can you just fix it? If not, I will do it later. Thanks.
        Hide
        Tim Hunt added a comment -

        Actually, I just amended the commit myself. Same branch as above, if you want to pull it.

        Show
        Tim Hunt added a comment - Actually, I just amended the commit myself. Same branch as above, if you want to pull it.
        Hide
        Sam Hemelryk added a comment -

        Thanks Tim - this has been integrated now

        Show
        Sam Hemelryk added a comment - Thanks Tim - this has been integrated now
        Hide
        Rossiani Wijaya added a comment -

        The patch works great on 2.0.

        However on 2.1 and master, there's missing string for randomsamatchnumber and randomsamatch.

        I will create new issue for it.

        Test passed.

        Show
        Rossiani Wijaya added a comment - The patch works great on 2.0. However on 2.1 and master, there's missing string for randomsamatchnumber and randomsamatch. I will create new issue for it. Test passed.
        Hide
        Aparup Banerjee added a comment -

        fixes have been rolled merrily up the stream! Thanks everybody!

        Show
        Aparup Banerjee added a comment - fixes have been rolled merrily up the stream! Thanks everybody!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: