View Issue Details

Related ChangesetsJump to NotesJump to History
This bug affects 1 person(s).
 6
IDProjectCategoryView StatusDate SubmittedLast Update
06079Bug reportsExpression Managerpublic2012-05-10 11:072012-06-03 07:51
Reporteruser19811Assigned ToTMSWhite  
PrioritynormalSeveritypartial_block 
Status closedResolutionfixed 
Product Version1.92+ 
Summary06079: sub-question filtering broken if "other" or "no answer" option is given
Description

You have two questions:

  1. A "Multiple Choice" with "other" and mandatory
  2. An "Array" mandatory

Each line of the array corresponds with one answer from the first question. If check one line of the first question, the corresponding line of the second question shoul be shown.

For the answer options "no answer" and "other", there is no line in the second question. So we only have conditions for the other answer options of the first question.

If you show the question, that the browser will not do anything, as there is an error in the JavaScript code produced by the LimeExpressionManager.php class. In line 5510, the if part is build with this line:

$relParts[] = " if ( " . $sq['relevancejs'] . " ) {\n";

But the index 'relevancejs' is empty and so the output is:

if ( ) {

which is not valid JavaScript. A quick fix, I could find, was replacing the line with the following:

$relParts[] = " if ( " . ( empty( $sq['relevancejs'] )? 'false' : $sq['relevancejs'] ) . " ) {\n";

which will than produce the following for such a case:

if ( false ) {

This is valid JavaScript code and the else part will be executed, which is the desired behaviour in that case.

I don't know if it's a conceptional bug or something else, but I just wanted to share my bugfix with you.

TagsNo tags attached.
Attached Files
limesurvey_survey_98464.lss (26,264 bytes)
diff6079.diff (3,000 bytes)    
diff --git a/classes/expressions/LimeExpressionManager.php b/classes/expressions/LimeExpressionManager.php
index 5a3f2db..8f7ee35 100644
--- a/classes/expressions/LimeExpressionManager.php
+++ b/classes/expressions/LimeExpressionManager.php
@@ -3260,26 +3260,31 @@
 
             if (!is_null($questionNum)) {
                 $jsVars = $this->em->GetJSVarsUsed();
-                $relevanceVars = implode('|',$this->em->GetJSVarsUsed());
-                $relevanceJS = $this->em->GetJavaScriptEquivalentOfExpression();
-
-                if (!isset($this->subQrelInfo[$questionNum])) {
-                    $this->subQrelInfo[$questionNum] = array();
+                if(count ($jsVars)){
+                    $relevanceVars = implode('|',$this->em->GetJSVarsUsed());
+                    $relevanceJS = $this->em->GetJavaScriptEquivalentOfExpression();
+                    if (!isset($this->subQrelInfo[$questionNum])) {
+                        $this->subQrelInfo[$questionNum] = array();
+                    }
+                    $this->subQrelInfo[$questionNum][$rowdivid] = array(
+                    'qid' => $questionNum,
+                    'eqn' => $eqn,
+                    'prettyPrintEqn' => $prettyPrint,
+                    'result' => $result,
+                    'numJsVars' => count($jsVars),
+                    'relevancejs' => $relevanceJS,
+                    'relevanceVars' => $relevanceVars,
+                    'rowdivid' => $rowdivid,
+                    'type'=>$type,
+                    'qtype'=>$qtype,
+                    'sgqa'=>$sgqa,
+                    'hasErrors'=>$hasErrors,
+                    );
+                }
+                else
+                {
+                    $this->subQrelInfo[$questionNum][$rowdivid] = false;
                 }
-                $this->subQrelInfo[$questionNum][$rowdivid] = array(
-                'qid' => $questionNum,
-                'eqn' => $eqn,
-                'prettyPrintEqn' => $prettyPrint,
-                'result' => $result,
-                'numJsVars' => count($jsVars),
-                'relevancejs' => $relevanceJS,
-                'relevanceVars' => $relevanceVars,
-                'rowdivid' => $rowdivid,
-                'type'=>$type,
-                'qtype'=>$qtype,
-                'sgqa'=>$sgqa,
-                'hasErrors'=>$hasErrors,
-                );
             }
             return $result;
         }
@@ -5494,10 +5499,11 @@
                     {
                         foreach ($LEM->subQrelInfo[$arg['qid']] as $subq)
                         {
-                            $subqParts[$subq['rowdivid']] = $subq;
+                            if($subq){
+                                $subqParts[$subq['rowdivid']] = $subq;
+                            }
                         }
                     }
-
                     $qidList[$arg['qid']] = $arg['qid'];
                     if (!isset($gseq_qidList[$arg['gseq']]))
                     {
diff6079.diff (3,000 bytes)   
ProcessSubQRelevance.php (2,434 bytes)
Bug heat6
Complete LimeSurvey version number (& build)120501
I will donate to the project if issue is resolvedNo
Browser
Database type & version5.1.61-0+squeeze1-log - (Debian)
Server OS (if known)Debian 2.6.32-41
Webserver software & version (if known)Apache/2.2.16 (Debian)
PHP VersionPHP Version 5.3.3-7+squeeze8

Relationships

Relationship GraphDependency Graph
related to 05977 closedDenisChenu Unable to use array_filter : javascript error 
related to 06165 closedTMSWhite unable to implement affirmative exclusive non-answer in array_filter_exclude 

Users monitoring this issue

There are no users monitoring this issue.

Activities

DenisChenu

DenisChenu

2012-05-10 12:32

developer   ~18654

Thank you kauboy.
And think it make a lot of helping for "bad expression manager manipualtion" ;)

Carsten:
"which is the desired behaviour in that case." ?

I don't know the exact desired behaviour
DenisChenu

DenisChenu

2012-05-10 14:30

developer   ~18663

kauboy : i can't reproduce on 120501 ?

I upload a survey sample, maybe i don't understand.

Can you upload a survey sample too ?
DenisChenu

DenisChenu

2012-05-11 11:49

developer   ~18695

kauboy : need an example survey, i can't reproduce with my test survey.
Thank you.

user19811

2012-05-11 14:16

  ~18700

The newly attached survey has the two questions with the conditions set, that will cause the error.
DenisChenu

DenisChenu

2012-05-11 15:02

developer   ~18701

OK,

Don't put the "no answer" at the good question.

It's not really a bug, more a new feature.

I see with "Expression Manager Great Guru"
DenisChenu

DenisChenu

2012-05-11 15:15

developer   ~18704

Hello Tom !

I knew you have some time for us and LimeSurvey now ;) .

I put a patch diff6079.diff for the "new feature".
But this patch change function _ProcessSubQRelevance , i put too the new function.

This patch work for kauboy and for other problem like 5977 .

It don't change the "showlogicfile", i think it can be a good idea to have permit logical error but don't throw error to user.

If OK, return the bug to me after while.

Thank's a lot.
TMSWhite

TMSWhite

2012-05-11 15:26

reporter   ~18706

kauboy-

That is an error in the survey. According to the documentation, if you want to use array_filter, then the sub-questions in each must match. Your question 2 has SQ003, which is not present in question 1.

Can you provide a real example of why you think extra sub-questions in array_filter-ed questions should be allowed?
DenisChenu

DenisChenu

2012-05-11 15:41

developer   ~18709

Hello Tom,

It's why i say it's a "new feature" :).

Maybe we can put :
// Now check whether there is sub-question relevance to perform for this question
$subqParts = array();
if (isset($LEM->subQrelInfo[$arg['qid']]))
{
foreach ($LEM->subQrelInfo[$arg['qid']] as $subq)
{
if(isset($subq['relevancejs']){
$subqParts[$subq['rowdivid']] = $subq;
}
}
}

Less modification, but still work if SubQ aren't present or if user made an error ;).

Denis
TMSWhite

TMSWhite

2012-05-11 15:44

reporter   ~18711

Denis-

Glad to see you're starting to master EM :-)

Yes, that is an option. Another is to show the error on the logic file, but surround the generated javascript in try { } catch { } so that it doesn't block the survey. Personally, I'd vote for the 2nd option, since in a long survey, users might not notice that they didn't keep their set of answer options consistent.

user19811

2012-05-11 16:12

  ~18712

That's absolutely correct! I just asked my colleague, why she wanted to have it the way. Her answer was: If you don't make the question mandatory, than the user can not choose "no answer". So she added "no answer" but it would have make any sense to add something to the array.

But I explained to her, that yuo don't need a "no answer" if a question is not mandatory. So that's OK.

You can close the ticket!
DenisChenu

DenisChenu

2012-05-11 16:14

developer   ~18713

<quote>but surround the generated javascript in try { } catch { }</quote>

Great !

Give it to me and go to work ! ;)
TMSWhite

TMSWhite

2012-05-11 16:17

reporter   ~18714

Per reporter, no change necessary, so closing ticket
TMSWhite

TMSWhite

2012-06-03 07:50

reporter   ~19042

Fix committed to master branch: http://bugs.limesurvey.org/plugin.php?page=Source/view&id=8633
TMSWhite

TMSWhite

2012-06-03 07:51

reporter   ~19044

Fix committed to Yii branch: http://bugs.limesurvey.org/plugin.php?page=Source/view&id=8635

Related Changesets

LimeSurvey: master 8b549dca

2012-06-03 00:48

TMSWhite


Details Diff		 Fixed issue 06165: unable to implement affirmative exclusive non-answer in array_filter_exclude
Fixed issue 06079: sub-question filtering broken if "other" or "no answer" option is given
Dev Array_filter and array_filter_exclude now handle mismatched lists of sub-questions gracefully. Values in list B that are not in list A will not be filtered, and will not throw errors.
Dev This lets authors support exclusive options like "none of the above" in array filtered questions		 Affected Issues
06079, 06165
mod - classes/expressions/LimeExpressionManager.php Diff File

LimeSurvey: Yii a2b96ed8

2012-06-03 00:51

TMSWhite


Details Diff		 Fixed issue 06165: unable to implement affirmative exclusive non-answer in array_filter_exclude
Fixed issue 06079: sub-question filtering broken if "other" or "no answer" option is given
Dev Array_filter and array_filter_exclude now handle mismatched lists of sub-questions gracefully. Values in list B that are not in list A will not be filtered, and will not throw errors.
Dev This lets authors support exclusive options like "none of the above" in array filtered questions		 Affected Issues
06079, 06165
mod - application/helpers/expressions/em_manager_helper.php Diff File

Issue History

Date Modified Username Field Change
2012-05-10 11:07 user19811 New Issue
2012-05-10 12:24 c_schmitz Assigned To => DenisChenu
2012-05-10 12:24 c_schmitz Status new => assigned
2012-05-10 12:32 DenisChenu Note Added: 18654
2012-05-10 14:30 DenisChenu Note Added: 18663
2012-05-10 14:31 DenisChenu File Added: limesurvey_survey_27314.lss
2012-05-11 11:49 DenisChenu Note Added: 18695
2012-05-11 11:49 DenisChenu Status assigned => feedback
2012-05-11 14:15 user19811 File Added: limesurvey_survey_98464.lss
2012-05-11 14:16 user19811 Note Added: 18700
2012-05-11 14:16 user19811 Status feedback => assigned
2012-05-11 15:02 DenisChenu Note Added: 18701
2012-05-11 15:08 DenisChenu Relationship added related to 05977
2012-05-11 15:10 DenisChenu File Added: diff6079.diff
2012-05-11 15:10 DenisChenu File Added: ProcessSubQRelevance.php
2012-05-11 15:10 DenisChenu File Deleted: limesurvey_survey_27314.lss
2012-05-11 15:11 DenisChenu Assigned To DenisChenu => TMSWhite
2012-05-11 15:15 DenisChenu Note Added: 18704
2012-05-11 15:26 TMSWhite Note Added: 18706
2012-05-11 15:26 TMSWhite Status assigned => feedback
2012-05-11 15:41 DenisChenu Note Added: 18709
2012-05-11 15:44 TMSWhite Note Added: 18711
2012-05-11 16:12 user19811 Note Added: 18712
2012-05-11 16:12 user19811 Status feedback => assigned
2012-05-11 16:14 DenisChenu Note Added: 18713
2012-05-11 16:17 TMSWhite Note Added: 18714
2012-05-11 16:17 TMSWhite Status assigned => resolved
2012-05-11 16:17 TMSWhite Resolution open => no change required
2012-05-17 10:28 c_schmitz Status resolved => closed
2012-06-03 07:50 TMSWhite Changeset attached => LimeSurvey master 8b549dca
2012-06-03 07:50 TMSWhite Note Added: 19042
2012-06-03 07:50 TMSWhite Resolution no change required => fixed
2012-06-03 07:51 TMSWhite Changeset attached => LimeSurvey Yii a2b96ed8
2012-06-03 07:51 TMSWhite Note Added: 19044
2012-06-03 07:51 TMSWhite Relationship added related to 06165