Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1, 2.2, 2.3.7, 2.4.4, 2.5, 2.6.3, 2.7
    • Fix Version/s: 2.6.4, 2.7.1
    • Component/s: Feedback
    • Labels:
    • Testing Instructions:
      Hide
      1. Activate the feedback module.
      2. Add a Multiple choice question (name=q1, label=q1) with answers "Yes" and "No".
      3. Mark this question as required.
      4. Add a page break.
      5. Add a Short text answer question (name=q2, label=q2) with Dependence item q1 and Dependence value "Yes".
      6. Mark this question as required.
      7. Add a Short text answer question (name=q3, label=q3) with Dependence item q1 and Dependence value "No".
      8. Mark this question as required.
      9. Go to the "Overview" page and answer this feedback with different choices in q1 and ensure on the last page you can successfully submit your answers.
      Show
      Activate the feedback module. Add a Multiple choice question (name=q1, label=q1) with answers "Yes" and "No". Mark this question as required. Add a page break. Add a Short text answer question (name=q2, label=q2) with Dependence item q1 and Dependence value "Yes". Mark this question as required. Add a Short text answer question (name=q3, label=q3) with Dependence item q1 and Dependence value "No". Mark this question as required. Go to the "Overview" page and answer this feedback with different choices in q1 and ensure on the last page you can successfully submit your answers.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE, MOODLE_27_STABLE
    • Pull from Repository:
    • Pull 2.7 Branch:
    • Pull Master Branch:
      MDL-31998_master

      Description

      Tested with Moodle 2.1.4 and Moodle Moodle 2.2.1+ (Build: 20120301)

      Feedback with depend item does not work as expected.

      Q1 (owncar) do you own a car?*
      yes
      no
      Page break----------------------------
      Q2 (carcolor) what color is your car?* (owncar->yes)
      blue
      black
      red
      Page break----------------------------
      Q3 (label) Survey is finished, you can now submit it.

      scenario #1
      Q1: yes
      Q2: blue (or any other color)
      Q3: Survey is finished, you can now submit it.
      Submit: OK

      scenario #2
      Q2: no
      next screen: no question text is shown, only the Previous page and Next page buttons and a Cancel button.
      Press Next page OR Previous button: error message "Saving failed because missing or false values"

        Gliffy Diagrams

          Activity

          Hide
          rezeau Joseph Rézeau added a comment -

          I think I've found the bug. The problem happens because Q2 is marked as "required" AND it's also marked as "depend" (on Q1 owncar->yes).
          It seems that these 2 options are incompatible and lead to the error I have found.

          Moreover, once a question marked as "depending" on another question has been marked as "required", it's impossible when you edit it to remove the "required" parameter.

          I think the bug is somewhere in feedback/lib.php in function feedback_check_values($firstitem, $lastitem).

          Joseph

          Show
          rezeau Joseph Rézeau added a comment - I think I've found the bug. The problem happens because Q2 is marked as "required" AND it's also marked as "depend" (on Q1 owncar->yes). It seems that these 2 options are incompatible and lead to the error I have found. Moreover, once a question marked as "depending" on another question has been marked as "required", it's impossible when you edit it to remove the "required" parameter. I think the bug is somewhere in feedback/lib.php in function feedback_check_values($firstitem, $lastitem). Joseph
          Hide
          salvetore Michael de Raadt added a comment -

          Thanks for reporting this, Joseph.

          Andreas, perhaps you could tell us if this is working as expected or if it is a bug.

          Show
          salvetore Michael de Raadt added a comment - Thanks for reporting this, Joseph. Andreas, perhaps you could tell us if this is working as expected or if it is a bug.
          Hide
          grabs Andreas Grabs added a comment -

          Hi Michael,

          I am not sure what is the definition of a bug.
          It is so as Joseph said.
          This behavior is unexpected.
          But there are some unmerged fixes for the feedback-module so I will wait until these fixes are integrated before I do a fix here. It is no a security issue.
          Sorry for my bad english

          Best regards
          Andreas

          Show
          grabs Andreas Grabs added a comment - Hi Michael, I am not sure what is the definition of a bug. It is so as Joseph said. This behavior is unexpected. But there are some unmerged fixes for the feedback-module so I will wait until these fixes are integrated before I do a fix here. It is no a security issue. Sorry for my bad english Best regards Andreas
          Hide
          pferre22 Pau Ferrer Ocaña (crazyserver) added a comment -

          Hi, I made a patch for this situation:

          On mod/feedback/lib.php Function feedback_check_values Arround line 2220

                           [...]
                             $value = optional_param($formvalname, null, PARAM_RAW);
                           }
                           $value = $itemobj->clean_input_value($value);
           
                          //ADD THIS: Check if depending is correct and discard required if isn't
          		if($item->dependitem > 0 AND $item->required == 1){
          			if (!isset($feedbackcompletedtmp->id)){
          				$feedbackcompletedtmp = feedback_get_current_completed($item->feedback, true);
          			}
          			$fb_compare_value = true;
          			if (isset($feedbackcompletedtmp->id)){
          				$fb_compare_value = feedback_compare_item_value($feedbackcompletedtmp->id,
                                                                              $item->dependitem,
                                                                              $item->dependvalue,
                                                                              true);
          			}
          			
          			if (!isset($feedbackcompletedtmp->id) OR !$fb_compare_value) {
          				$item->required	= 0;
          			}
          		}
                          //END
                         //check if the value is set
                         if (is_null($value) AND $item->required == 1) {
                            return false;
                          }
                          [...]
          		

          It ensures that if the question is required but it does not needed to be shown, the required value falls to 0 and the show can go on!

          Pau

          Show
          pferre22 Pau Ferrer Ocaña (crazyserver) added a comment - Hi, I made a patch for this situation: On mod/feedback/lib.php Function feedback_check_values Arround line 2220 [...] $value = optional_param($formvalname, null, PARAM_RAW); } $value = $itemobj->clean_input_value($value);   //ADD THIS: Check if depending is correct and discard required if isn't if($item->dependitem > 0 AND $item->required == 1){ if (!isset($feedbackcompletedtmp->id)){ $feedbackcompletedtmp = feedback_get_current_completed($item->feedback, true); } $fb_compare_value = true; if (isset($feedbackcompletedtmp->id)){ $fb_compare_value = feedback_compare_item_value($feedbackcompletedtmp->id, $item->dependitem, $item->dependvalue, true); } if (!isset($feedbackcompletedtmp->id) OR !$fb_compare_value) { $item->required = 0; } } //END //check if the value is set if (is_null($value) AND $item->required == 1) { return false; } [...] It ensures that if the question is required but it does not needed to be shown, the required value falls to 0 and the show can go on! Pau
          Hide
          grabs Andreas Grabs added a comment -

          Hi Pau,
          thank you for this fix. I'll try this soon.
          Best regards
          Andreas

          Show
          grabs Andreas Grabs added a comment - Hi Pau, thank you for this fix. I'll try this soon. Best regards Andreas
          Hide
          quar Christian Rojas added a comment -

          Hi,

          I just added the code of Pau in version 2.6.1 and it works perfectly. Each branch is completely isolated and does not give error submitting the form.

          It would be great to be integrated this solution into the production version.

          Sorry for my english.

          Best regards

          Christian Rojas

          Show
          quar Christian Rojas added a comment - Hi, I just added the code of Pau in version 2.6.1 and it works perfectly. Each branch is completely isolated and does not give error submitting the form. It would be great to be integrated this solution into the production version. Sorry for my english. Best regards Christian Rojas
          Hide
          jamatrucola Joe Amatrucola added a comment -

          I ran into this same problem today, and the patch above resolved the issue for us. I, too, request that this code be integrated into the production version.

          Joe

          Show
          jamatrucola Joe Amatrucola added a comment - I ran into this same problem today, and the patch above resolved the issue for us. I, too, request that this code be integrated into the production version. Joe
          Hide
          vaughany Paul Vaughan added a comment -

          Can anyone confirm use of and success with the above patch on 2.6?

          Show
          vaughany Paul Vaughan added a comment - Can anyone confirm use of and success with the above patch on 2.6?
          Hide
          jamatrucola Joe Amatrucola added a comment -

          I am running Moodle 2.6.1+ (Build: 20140131)
          I should follow-up from my previous post by saying that after implementing the fix above, it seemed to solve the problem involving dependent and required questions. However, within 2 days, folks were still reporting difficulty, which I then verified. Not having much more time to troubleshoot, I just changed the dependent question to not be required.

          Show
          jamatrucola Joe Amatrucola added a comment - I am running Moodle 2.6.1+ (Build: 20140131) I should follow-up from my previous post by saying that after implementing the fix above, it seemed to solve the problem involving dependent and required questions. However, within 2 days, folks were still reporting difficulty, which I then verified. Not having much more time to troubleshoot, I just changed the dependent question to not be required.
          Hide
          grabs Andreas Grabs added a comment -

          Many thanks to Pau Ferrer Ocaña!
          Best regards
          Andreas

          Show
          grabs Andreas Grabs added a comment - Many thanks to Pau Ferrer Ocaña! Best regards Andreas
          Hide
          cibot CiBoT added a comment -
          Show
          cibot CiBoT added a comment - Results for MDL-31998 Remote repository: git://github.com/grabs/moodle.git Remote branch MDL-31998 _25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3650 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3650/artifact/work/smurf.html Remote branch MDL-31998 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3651 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3651/artifact/work/smurf.html Remote branch MDL-31998 _27 to be integrated into upstream MOODLE_27_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3652 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3652/artifact/work/smurf.html Remote branch MDL-31998 _master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3653 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3653/artifact/work/smurf.html
          Hide
          markn Mark Nelson added a comment -

          Thanks Andreas and Pau,

          1. Why is "if (!isset($feedbackcompletedtmp->id)) {" needed in this function? The variable $feedbackcompletedtmp is not declared in this function, so this if statement will always be true.
          2. Can you please squash your last commit? The coding style you are fixing is a part of the code you are introducing.

          Thanks!

          Show
          markn Mark Nelson added a comment - Thanks Andreas and Pau, Why is "if (!isset($feedbackcompletedtmp->id)) {" needed in this function? The variable $feedbackcompletedtmp is not declared in this function, so this if statement will always be true. Can you please squash your last commit? The coding style you are fixing is a part of the code you are introducing. Thanks!
          Hide
          grabs Andreas Grabs added a comment -

          Hi Mark,
          thank you for your hints. I optimized the code and squashed it into one single commit. Now I know how squashing works .
          Best regards
          Andreas

          Show
          grabs Andreas Grabs added a comment - Hi Mark, thank you for your hints. I optimized the code and squashed it into one single commit. Now I know how squashing works . Best regards Andreas
          Hide
          cibot CiBoT added a comment -
          Show
          cibot CiBoT added a comment - Results for MDL-31998 Remote repository: git://github.com/grabs/moodle.git Remote branch MDL-31998 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3930 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3930/artifact/work/smurf.html Remote branch MDL-31998 _27 to be integrated into upstream MOODLE_27_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3931 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3931/artifact/work/smurf.html Remote branch MDL-31998 _master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3932 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3932/artifact/work/smurf.html
          Hide
          markn Mark Nelson added a comment -

          Thanks for the update Andreas.

          Couple more points.

          1. Is there a reason you call the variable $fbcomparevalue and not simply $comparevalue? The 'fb' part was confusing to begin with until I realised it stood for feedback - which isn't really necessary
          2. You immediately overwrite the $fbcomparevalue after declaring it, so the "$fbcomparevalue = true;" is useless. What you could do is simply set $fbcomparevalue to false, instead of true, at the beginning and remove the else statement.

          Cheers!

          Show
          markn Mark Nelson added a comment - Thanks for the update Andreas. Couple more points. Is there a reason you call the variable $fbcomparevalue and not simply $comparevalue? The 'fb' part was confusing to begin with until I realised it stood for feedback - which isn't really necessary You immediately overwrite the $fbcomparevalue after declaring it, so the "$fbcomparevalue = true;" is useless. What you could do is simply set $fbcomparevalue to false, instead of true, at the beginning and remove the else statement. Cheers!
          Hide
          markn Mark Nelson added a comment -

          I just read Joe Amatrucola's comment, where he said students were still experiencing issues. I am wondering if there an edge case where this solution does not work? Can you think of any Andreas?

          Show
          markn Mark Nelson added a comment - I just read Joe Amatrucola's comment, where he said students were still experiencing issues. I am wondering if there an edge case where this solution does not work? Can you think of any Andreas?
          Hide
          grabs Andreas Grabs added a comment -

          Hi Mark,
          here is the next try. Thank you again!
          Best regards
          Andreas

          Show
          grabs Andreas Grabs added a comment - Hi Mark, here is the next try. Thank you again! Best regards Andreas
          Hide
          cibot CiBoT added a comment -
          Show
          cibot CiBoT added a comment - Results for MDL-31998 Remote repository: git://github.com/grabs/moodle.git Remote branch MDL-31998 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3939 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3939/artifact/work/smurf.html Remote branch MDL-31998 _27 to be integrated into upstream MOODLE_27_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3940 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3940/artifact/work/smurf.html Remote branch MDL-31998 _master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3941 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3941/artifact/work/smurf.html
          Hide
          markn Mark Nelson added a comment -

          Thanks Andreas. I personally tested this out and it works as described, however Joe Amatrucola stated that he still had issues after applying the patch provided in the earlier comments. Do you have any idea why this may be the case? I am going to submit this for integration as it appears from two above comments that this fix worked as expected, and all is well. If there is still a bug a new tracker issue can be created.

          Show
          markn Mark Nelson added a comment - Thanks Andreas. I personally tested this out and it works as described, however Joe Amatrucola stated that he still had issues after applying the patch provided in the earlier comments. Do you have any idea why this may be the case? I am going to submit this for integration as it appears from two above comments that this fix worked as expected, and all is well. If there is still a bug a new tracker issue can be created.
          Hide
          jamatrucola Joe Amatrucola added a comment -

          Guys, Thanks for looking into this further, and working to implement the fix into Moodle. Note that I have implemented the patch referenced in this bug report I went back to the feedback activity in question again just now, tested pretty thoroughly, and was not able to replicate the bug in question. Unfortunately, I did not document much in my notes because I was pressed for time, so I don't know what wasn't working in my second go-around. I want to say it had something to do with answering the first (independent) question with a response that didn't cause the second (dependent) question to appear, still being able to proceed through the feedback activity [a good thing] but then encountering difficulty if you try to use the Previous and Next Page buttons to navigate through the feedback activity. Sorry I can't be more specific. I agree that if it is testing properly now, if there is still an error, it will surface and can get logged as a separate issue.

          Show
          jamatrucola Joe Amatrucola added a comment - Guys, Thanks for looking into this further, and working to implement the fix into Moodle. Note that I have implemented the patch referenced in this bug report I went back to the feedback activity in question again just now, tested pretty thoroughly, and was not able to replicate the bug in question. Unfortunately, I did not document much in my notes because I was pressed for time, so I don't know what wasn't working in my second go-around. I want to say it had something to do with answering the first (independent) question with a response that didn't cause the second (dependent) question to appear, still being able to proceed through the feedback activity [a good thing] but then encountering difficulty if you try to use the Previous and Next Page buttons to navigate through the feedback activity. Sorry I can't be more specific. I agree that if it is testing properly now, if there is still an error, it will surface and can get logged as a separate issue.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          cibot CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          damyon Damyon Wiese added a comment -

          Thanks Everyone,

          This also seems to fix the bug for me. Integrated to 26, 27 and master.

          Show
          damyon Damyon Wiese added a comment - Thanks Everyone, This also seems to fix the bug for me. Integrated to 26, 27 and master.
          Hide
          abgreeve Adrian Greeve added a comment -

          Tested on the 2.6, 2.7 and master integration branches.
          Everything worked as described in the testing instructions.
          Test passed.

          Show
          abgreeve Adrian Greeve added a comment - Tested on the 2.6, 2.7 and master integration branches. Everything worked as described in the testing instructions. Test passed.
          Hide
          rezeau Joseph Rézeau added a comment -

          I've just tested the fix on my local install of Moodle 2.7 and confirm that all works as expected.

          However, may I point out that there is one more point of improvement that should be considered in the flow of a "depend questions" feature, as described in my comment dated 25/Jul/13 3:28 PM in MDL-40689.

          Joseph

          Show
          rezeau Joseph Rézeau added a comment - I've just tested the fix on my local install of Moodle 2.7 and confirm that all works as expected. However, may I point out that there is one more point of improvement that should be considered in the flow of a "depend questions" feature, as described in my comment dated 25/Jul/13 3:28 PM in MDL-40689 . Joseph
          Hide
          grabs Andreas Grabs added a comment -

          Hi Joseph,
          do you mean the skipping of empty pages? That indeed should be improved.
          I will look at it.
          Best regards
          Andreas

          Show
          grabs Andreas Grabs added a comment - Hi Joseph, do you mean the skipping of empty pages? That indeed should be improved. I will look at it. Best regards Andreas
          Hide
          jamatrucola Joe Amatrucola added a comment -

          Hi Andreas,

          On a related, if not identical note to what Joseph is pointing out is what I perceived to be the non-resolution of this bug back in March. Though I no longer believe it to be related to this bug, it might be worth considering: Consider a Feedback activity where all the questions are required, spread across 3 pages, As the activity currently works, if the user answers the questions on page 1, progresses to page 2 but answers no questions and clicks the back button to return to page 1 to fix or change something, the user receives the error "Saving failed because missing or false values." I presume this results from all the blank entries on page 2. However, one could argue that the user shouldn't be harassed to complete the required questions on page 2 until he/she attempts to go BEYOND page 2 without completing them, not when he/she tries to go BACK to a previous page.

          My apologies for posting this note here, since I fully agree it's not directly related to this bug - but this is where the conversation currently is.

          Thanks
          Joe

          Show
          jamatrucola Joe Amatrucola added a comment - Hi Andreas, On a related, if not identical note to what Joseph is pointing out is what I perceived to be the non-resolution of this bug back in March. Though I no longer believe it to be related to this bug, it might be worth considering: Consider a Feedback activity where all the questions are required, spread across 3 pages, As the activity currently works, if the user answers the questions on page 1, progresses to page 2 but answers no questions and clicks the back button to return to page 1 to fix or change something, the user receives the error "Saving failed because missing or false values." I presume this results from all the blank entries on page 2. However, one could argue that the user shouldn't be harassed to complete the required questions on page 2 until he/she attempts to go BEYOND page 2 without completing them, not when he/she tries to go BACK to a previous page. My apologies for posting this note here, since I fully agree it's not directly related to this bug - but this is where the conversation currently is. Thanks Joe
          Hide
          damyon Damyon Wiese added a comment -

          Look at bug
          CTRL-C
          www.stackoverflow.com
          CTRL-V
          <enter>
          CTRL-C
          CTRL-V
          Bug fixed

          (it's never that easy really)

          Thanks for working on this issue, it has been released as part of Moodle.

          Show
          damyon Damyon Wiese added a comment - Look at bug CTRL-C www.stackoverflow.com CTRL-V <enter> CTRL-C CTRL-V Bug fixed (it's never that easy really) Thanks for working on this issue, it has been released as part of Moodle.

            People

            • Votes:
              12 Vote for this issue
              Watchers:
              15 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                14/Jul/14