Moodle
  1. Moodle
  2. MDL-27955

Error on navigating back through a lesson activity (reviewing lesson)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0.3, 2.1
    • Fix Version/s: 2.0.4
    • Component/s: Lesson
    • Labels:
    • Testing Instructions:
      Hide

      1. Login as a teacher, update a lesson activity and set 'Allow student review' to Yes and 'Maximum number of attempts' to a value greater than 1.
      2. Login as a student and attempt the activity.
      3. Click the 'Review lesson' button at the end of the activity.

      Able to review the questions.

      Show
      1. Login as a teacher, update a lesson activity and set 'Allow student review' to Yes and 'Maximum number of attempts' to a value greater than 1. 2. Login as a student and attempt the activity. 3. Click the 'Review lesson' button at the end of the activity. Able to review the questions.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-27955_HEAD
    • Rank:
      17596

      Description

      upon reviewing, error message displayed (with PHP errors debug mode ON):
      Notice: Trying to get property of non-object in \moodle\mod\lesson\pagetypes\multichoice.php on line 508

      Notice: Undefined offset: 0 in \moodle\mod\lesson\pagetypes\matching.php on line 507
      Notice: Undefined index: in \moodle\mod\lesson\pagetypes\matching.php on line 507
      Notice: Trying to get property of non-object in \moodle\mod\lesson\pagetypes\matching.php on line 507

        Issue Links

          Activity

          Hide
          Rossiani Wijaya added a comment -

          Add Helen to watcher list.

          Hi Helen,
          I would like to notify you for functional changes in lesson activity.

          I talked with Martin and he suggested to make some adjustment to it.

          This is the list of changes:
          1. Multiple attempts could only be done upon attempting the lesson. If answer's jump setting is set to 'this page', selecting this answer will allow user to re-submit the answer until the max attempt is reach or the correct answer is selected.
          2. During review process, student could not re-submit their answer.
          3. If 'display ongoing score' is enable, it will only display upon attempting the lesson.

          With the changes above, the 4th and 5th steps test instruction for MDLQA-1021 need to be modified.

          Please let me know if there's anything else need to be change.

          Rosie

          Show
          Rossiani Wijaya added a comment - Add Helen to watcher list. Hi Helen, I would like to notify you for functional changes in lesson activity. I talked with Martin and he suggested to make some adjustment to it. This is the list of changes: 1. Multiple attempts could only be done upon attempting the lesson. If answer's jump setting is set to 'this page', selecting this answer will allow user to re-submit the answer until the max attempt is reach or the correct answer is selected. 2. During review process, student could not re-submit their answer. 3. If 'display ongoing score' is enable, it will only display upon attempting the lesson. With the changes above, the 4th and 5th steps test instruction for MDLQA-1021 need to be modified. Please let me know if there's anything else need to be change. Rosie
          Hide
          Andrew Davis added a comment -
          if (!$nretakes && !$this->lesson->retake) {
                              $DB->insert_record("lesson_attempts", $attempt);
                          }
          

          Probably make the $nretakes condition something like $nretakes == 0 rather than !$nretakes. It isnt really different but it is an integer and treating it like an integer in the if is probably better.

          in /lesson/pagetypes/matching.php this comment
          //reassing array keys for answers
          I'm not sure what that means.

          Otherwise fine

          Show
          Andrew Davis added a comment - if (!$nretakes && !$ this ->lesson->retake) { $DB->insert_record( "lesson_attempts" , $attempt); } Probably make the $nretakes condition something like $nretakes == 0 rather than !$nretakes. It isnt really different but it is an integer and treating it like an integer in the if is probably better. in /lesson/pagetypes/matching.php this comment //reassing array keys for answers I'm not sure what that means. Otherwise fine
          Hide
          Helen Foster added a comment -

          -1 to changing lesson functionality just before the 2.1 release. However if others agree that it is necessary I will change the QA test accordingly.

          Chat with Rosie about this issue:

          Helen: what do you mean by "Multiple attempts could only be done upon attempting the lesson."?
          Rosie: on lesson settings, there is a setting for maximum number of attempts. if it sets to greater than 1, student will be able to re-try and submit different answer
          Rosie: if they reach the end of lesson, they are not allow to re-try/re-submit the answer (for example during review lesson)
          Helen: so your functional change is to make it so when students review a lesson (i.e. click the 'Review lesson' button at the end of the activity then navigate back through it) they are not allowed to re-answer any questions, right?
          Rosie: yes
          Helen: why is it necessary to make this change in functionality?
          Rosie: because it creates confusion that student could change their answer during review. according to martin, review should only able to see student selected answer and no changes should allow.
          Helen: there are several confusing lesson settings e.g. provide option to try a question again, maximum number of attempts, ...
          Helen: lesson really needs a complete rewrite and for the settings to be simplified
          Helen: I don't understand why martin is suggesting a change in functionality 2 days before release
          Helen: it is likely to cause complaints from lesson users who are used to the current functionality
          Rosie: yes, lesson definitely needs to be rewrite
          Helen: can't you just fix the error message?
          Rosie: I had discussion with Martin regarding the issue on friday.
          Rosie: I could. I fixed the error message last week and while testing it, i found the confusion on the lesson setting and have discussion with Martin.

          Show
          Helen Foster added a comment - -1 to changing lesson functionality just before the 2.1 release. However if others agree that it is necessary I will change the QA test accordingly. Chat with Rosie about this issue: Helen: what do you mean by "Multiple attempts could only be done upon attempting the lesson."? Rosie: on lesson settings, there is a setting for maximum number of attempts. if it sets to greater than 1, student will be able to re-try and submit different answer Rosie: if they reach the end of lesson, they are not allow to re-try/re-submit the answer (for example during review lesson) Helen: so your functional change is to make it so when students review a lesson (i.e. click the 'Review lesson' button at the end of the activity then navigate back through it) they are not allowed to re-answer any questions, right? Rosie: yes Helen: why is it necessary to make this change in functionality? Rosie: because it creates confusion that student could change their answer during review. according to martin, review should only able to see student selected answer and no changes should allow. Helen: there are several confusing lesson settings e.g. provide option to try a question again, maximum number of attempts, ... Helen: lesson really needs a complete rewrite and for the settings to be simplified Helen: I don't understand why martin is suggesting a change in functionality 2 days before release Helen: it is likely to cause complaints from lesson users who are used to the current functionality Rosie: yes, lesson definitely needs to be rewrite Helen: can't you just fix the error message? Rosie: I had discussion with Martin regarding the issue on friday. Rosie: I could. I fixed the error message last week and while testing it, i found the confusion on the lesson setting and have discussion with Martin.
          Hide
          Michael de Raadt added a comment -

          This change has been in the works for a while now, but has just taken a bit of time to come to fruition.

          The change prevents students from being able to update their answers when they shouldn't be able to, so it is reflecting the expected behaviour. In that light, I just had a look at the QA test and I don't think we need to add or change the instructions as a result of this fix.

          Lesson is on our list for redevelopment for 2.2, and climbing in priority each time I hear about it.

          Show
          Michael de Raadt added a comment - This change has been in the works for a while now, but has just taken a bit of time to come to fruition. The change prevents students from being able to update their answers when they shouldn't be able to, so it is reflecting the expected behaviour. In that light, I just had a look at the QA test and I don't think we need to add or change the instructions as a result of this fix. Lesson is on our list for redevelopment for 2.2, and climbing in priority each time I hear about it.
          Hide
          Rossiani Wijaya added a comment -

          Thanks Andrew for reviewing.

          Update the patch and submitting for integration.

          Show
          Rossiani Wijaya added a comment - Thanks Andrew for reviewing. Update the patch and submitting for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been integrated, thanks. I briefly tested it (with a simple 4-page lesson) to look that nothing is broken when attempting the lesson.

          Some notes:

          • final grade does not observe the max grade / scale defined in lesson settings at all. I've commented about that at post-release issue MDL-28060.
          • Rossie, it always surprise me you use the "_HEAD" suffix for master branches, lol. Only a silly detail but I thing (no suffix) or "_master" sounds better.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This has been integrated, thanks. I briefly tested it (with a simple 4-page lesson) to look that nothing is broken when attempting the lesson. Some notes: final grade does not observe the max grade / scale defined in lesson settings at all. I've commented about that at post-release issue MDL-28060 . Rossie, it always surprise me you use the "_HEAD" suffix for master branches, lol. Only a silly detail but I thing (no suffix) or "_master" sounds better. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Passing test without further action. Will be tested by MDLQA-1021 once this meets upstream.

          Show
          Eloy Lafuente (stronk7) added a comment - Passing test without further action. Will be tested by MDLQA-1021 once this meets upstream.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Upstream-ized! Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Upstream-ized! Thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: