Moodle
  1. Moodle
  2. MDL-25632

Incorrect navigation/jump behaviors in lesson module

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.1
    • Component/s: Lesson
    • Labels:
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      15720

      Description

      I've created some lessons with complex logic in 19, and found problems with them in 2.0:

      Problem 1. If some page does not have questions, it should be displayed for reading. But now one blank page is displayed instead. (problem seems to be @ mod/lesson/view_form.php)

      Problem 2. In shortanswer questions i make answers representing typical mistakes, linking to pages in lesson, with description explaining why the answer is incorrect. I grade such answers with 0 points. But now, when answer is graded with 0, then lesson returns to same page, no matter of what is set in Jumps field.

      Problem 3. I've entered 2 answers for shortquestion, but it saves 4 answers, making last 2 of them blank. This does not affect logic, but makes it harder to read when all questions are on one page.

        Issue Links

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sending to STABLE backlog, with minor rewording. Thanks for the report!

          Note there is one linked issue, reported initially for 1.9.x that could be similar to Problem 2 here (MDL-22349).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Sending to STABLE backlog, with minor rewording. Thanks for the report! Note there is one linked issue, reported initially for 1.9.x that could be similar to Problem 2 here ( MDL-22349 ). Ciao
          Hide
          Sam Hemelryk added a comment -

          Hi guys, I've added a bunch of people as watchers here, could someone please peer review the patch I have for this bug:
          https://github.com/samhemelryk/moodle/compare/cvshead...wip-MDL-25632

          Once done if there are no changes I'll create a pull request.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've added a bunch of people as watchers here, could someone please peer review the patch I have for this bug: https://github.com/samhemelryk/moodle/compare/cvshead...wip-MDL-25632 Once done if there are no changes I'll create a pull request. Cheers Sam
          Hide
          Andrew Davis added a comment -

          Sam, the above diff link is busted as it still references cvshead

          Show
          Andrew Davis added a comment - Sam, the above diff link is busted as it still references cvshead
          Hide
          Andrew Davis added a comment -

          I was going to say to use new stdClass(); instead of new stdClass; but theres 1436 instances of my preferred way and 537 of yours so I suppose it doesn't matter. Not sure if we have an officially sanctioned way to declare instances of stdClass.

          That upgrade code is nasty but I dont know how to make it better.

          line 479 of /mod/lesson/report wasnt actually modified by you but its weird.
          $answerpages[] = $page->report_answers(clone($answerpage), clone($answerdata), $useranswer, $pagestats, $i, $n);

          Whats up with calling clone on the arguments? I get the point of clone() but what is going on that that is required? That makes me really uneasy.

          Anyhow, your changes look fine.

          Show
          Andrew Davis added a comment - I was going to say to use new stdClass(); instead of new stdClass; but theres 1436 instances of my preferred way and 537 of yours so I suppose it doesn't matter. Not sure if we have an officially sanctioned way to declare instances of stdClass. That upgrade code is nasty but I dont know how to make it better. line 479 of /mod/lesson/report wasnt actually modified by you but its weird. $answerpages[] = $page->report_answers(clone($answerpage), clone($answerdata), $useranswer, $pagestats, $i, $n); Whats up with calling clone on the arguments? I get the point of clone() but what is going on that that is required? That makes me really uneasy. Anyhow, your changes look fine.
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          Thanks for looking at that, hehe stdClass is a fun one, I remember when I first started here I asked Tim about that and he had a very good reason to use stdClass but I can't for the life of me remember what it was.
          Regarding the clone call it looks like it is being done because the original object is being modified in preparation for being passed of to a renderer, so perhaps is done to ensure further operations on the object aren't hindered by rendering information.... although I would've thought it were better to do that in the function rather than when the function is being called, anyway that'll hopefully all be tidied up with the lesson refactoring that will happen in the future.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, Thanks for looking at that, hehe stdClass is a fun one, I remember when I first started here I asked Tim about that and he had a very good reason to use stdClass but I can't for the life of me remember what it was. Regarding the clone call it looks like it is being done because the original object is being modified in preparation for being passed of to a renderer, so perhaps is done to ensure further operations on the object aren't hindered by rendering information.... although I would've thought it were better to do that in the function rather than when the function is being called, anyway that'll hopefully all be tidied up with the lesson refactoring that will happen in the future. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Pull request created: PULL-28

          Show
          Sam Hemelryk added a comment - Pull request created: PULL-28
          Hide
          Petr Škoda added a comment -

          closing, thanks

          Show
          Petr Škoda added a comment - closing, thanks

            People

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

              Dates

              • Created:
                Updated:
                Resolved: