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

Incorrect navigation/jump behaviors in lesson module

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            stronk7 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
            stronk7 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
            samhemelryk 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
            samhemelryk 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
            andyjdavis Andrew Davis added a comment -

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

            Show
            andyjdavis Andrew Davis added a comment - Sam, the above diff link is busted as it still references cvshead
            Hide
            andyjdavis 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
            andyjdavis 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
            samhemelryk 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
            samhemelryk 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
            samhemelryk Sam Hemelryk added a comment -

            Pull request created: PULL-28

            Show
            samhemelryk Sam Hemelryk added a comment - Pull request created: PULL-28
            Hide
            skodak Petr Skoda added a comment -

            closing, thanks

            Show
            skodak Petr Skoda added a comment - closing, thanks

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  25/Dec/10