Survey Expression Engine validation++

Survey Expression Engine now detects invalid variable names passed to value(), score(), etc..
Also now detects situations where you have jump targets/expressions defined at multiple levels - thus causing precedence rules to kick in (normally this indicates a mistake)
Fixed Survey edit page bug where TextEditor would not move after gotoExpression textarea resize caused items to move

docs/changelog/7.x.x.txt

@@ -19,6 +19,7 @@
  - The transaction items now store the tax that is applied to them, as well as
    tax plugin specific data that needs to be stored together with
    transactions. (Martin Kamerbeek / Oqapi )
+ - Added better Survey Expression Engine validation warnings
 
 7.7.5
  - Adding StoryManager.

lib/WebGUI/Asset/Wobject/Survey/ExpressionEngine.pm

@@ -44,6 +44,7 @@ Returns the recorded response value for the answer to question_variable
 
 sub value {
     my $key   = shift;
+    _validateVariable($key, 'value');
     my $value = $tags->{$key} || $values->{$key};
     if (ref $value eq 'ARRAY') {
         my $joined = join ', ', @$value;
@@ -72,8 +73,8 @@ sub valueX {
     my ( $asset_spec, $key ) = @_;
     
     # See if $otherInstances already contains the external survey
-    if (my $other_instance = $otherInstances->{$asset_spec}) {
+    if (my $otherInstance = $otherInstances->{$asset_spec}) {
-        my $values = $other_instance->{values};
+        my $values = $otherInstance->{values};
         my $value  = $values->{$key};
         if (ref $value eq 'ARRAY') {
             my $joined = join ', ', @$value;
@@ -90,7 +91,7 @@ sub valueX {
         }
     } else {
         # Throw an exception, triggering run() to resolve the external reference and re-run
-        die( { other_instance => $asset_spec } );
+        die( { otherInstance => $asset_spec } );
     }
 }
 
@@ -108,25 +109,11 @@ case the sub is applied to the most recent completed response for the user on th
 =cut
 
 sub score {
-    # Two arguments implies the first arg is an asset_spec
+    my $key = shift;
-    if ( @_ == 2 ) {
+    _validateVariable($key, 'score');
-        my ( $asset_spec, $key ) = @_;
-        
-        # See if $otherInstances already contains the external survey
-        if (my $other_instance = $otherInstances->{$asset_spec}) {
-            my $scores = $other_instance->{scores};
-            my $score  = $scores->{$key};
-            $session->log->debug("score($asset_spec, $key) resolves to [$score]");
-            return $score;
-        } else {
-            # Throw an exception, triggering run() to resolve the external reference and re-run
-            die( { other_instance => $asset_spec } );
-        }
-    }
-    my $key   = shift;
     my $score = $scores->{$key};
     $session->log->debug("score($key) resolves to [$score]");
-    return $score;    # scalar variable, so no need to clone
+    return $score;
 }
 
 =head2 scoreX
@@ -141,17 +128,43 @@ sub scoreX {
     my ( $asset_spec, $key ) = @_;
     
     # See if $otherInstances already contains the external survey
-    if (my $other_instance = $otherInstances->{$asset_spec}) {
+    if (my $otherInstance = $otherInstances->{$asset_spec}) {
-        my $scores = $other_instance->{scores};
+        my $scores = $otherInstance->{scores};
         my $score  = $scores->{$key};
         $session->log->debug("scoreX($asset_spec, $key) resolves to [$score]");
         return $score;
     } else {
         # Throw an exception, triggering run() to resolve the external reference and re-run
-        die( { other_instance => $asset_spec } );
+        die( { otherInstance => $asset_spec } );
     }
 }
 
+=head2 _validateVariable ($key, $fn)
+
+Convenience sub to do optional validation of variable names
+
+=head3 key
+
+Variable name to validate
+
+=head3 fn
+
+Function name of caller (for diagnostic output)
+
+=cut
+
+sub _validateVariable {
+    my $key = shift;
+    my $fn = shift || 'unspecified function';
+    if ( $validTargets && !exists $validTargets->{$key} ) {
+        my $error = "Param [$key] to $fn is not a valid variable name";
+        $session->log->debug($error);
+        die($error) if $validate;
+        return;
+    }
+    return 1;
+}
+
 =head2 answered
 
 Returns true/false depending on whether use has actually reached and responded to the given question
@@ -163,6 +176,8 @@ case the sub is applied to the most recent completed response for the user on th
 
 sub answered {
     my $key   = shift;
+    
+    _validateVariable($key, 'answered');
     my $answered = exists $values->{$key};
     $session->log->debug("answered($key) returns [$answered]");
     return $answered;
@@ -180,14 +195,14 @@ sub answeredX {
     my ( $asset_spec, $key ) = @_;
     
     # See if $otherInstances already contains the external survey
-    if (my $other_instance = $otherInstances->{$asset_spec}) {
+    if (my $otherInstance = $otherInstances->{$asset_spec}) {
-        my $values = $other_instance->{values};
+        my $values = $otherInstance->{values};
         my $answered  = exists $values->{$key};
         $session->log->debug("answeredX($asset_spec, $key) returns [$answered]");
         return $answered;
     } else {
         # Throw an exception, triggering run() to resolve the external reference and re-run
-        die( { other_instance => $asset_spec } );
+        die( { otherInstance => $asset_spec } );
     }
 }
 
@@ -249,15 +264,15 @@ sub taggedX {
         $session->log->warn("Three arguments passed to taggedX($args). Did you mean tag($args)?");
     }
     # See if $otherInstances already contains the external survey
-    if (my $other_instance = $otherInstances->{$asset_spec}) {
+    if (my $otherInstance = $otherInstances->{$asset_spec}) {
-        my $tags = $other_instance->{tags};
+        my $tags = $otherInstance->{tags};
         
         my $value = $tags->{$name};
         $session->log->debug("taggedX($asset_spec, $name) returns [$value]");
         return $value;
     } else {
         # Throw an exception, triggering run() to resolve the external reference and re-run
-        die( { other_instance => $asset_spec } );
+        die( { otherInstance => $asset_spec } );
     }
 }
 
@@ -275,14 +290,8 @@ sub jump(&$) {
     $jumpCount++;
 
     # If $validTargets known, make sure target is valid
-    if ( $validTargets && !exists $validTargets->{$target} ) {
+    if (!_validateVariable($target, 'jump')) {
-        $session->log->debug("Invalid target [$target]");
+        return; # skip jump but continue with expression
-        if ($validate) {
-            die("Invalid jump target \"$target\"");    # bail and report error
-        }
-        else {
-            return;                                    # skip jump but continue with expression
-        }
     }
 
     if ( $sub->() ) {
@@ -432,8 +441,8 @@ sub run {
         }
 
         # See if an unresolved external reference was encountered
-        if ( ref $@ && ref $@ eq 'HASH' && $@->{other_instance} ) {
+        if ( ref $@ && ref $@ eq 'HASH' && $@->{otherInstance} ) {
-            my $asset_spec = $@->{other_instance};
+            my $asset_spec = $@->{otherInstance};
             $session->log->debug("Resolving external reference: $asset_spec");
             my $asset;
 

lib/WebGUI/Asset/Wobject/Survey/SurveyJSON.pm

@@ -1215,6 +1215,9 @@ sub validateSurvey{
         if(my $error = $self->validateGotoExpression($section,$goodTargets)){
             push @messages,"Section $sNum has invalid Jump Expression: \"$section->{gotoExpression}\". Error: $error";
         }
+        if(my @errors = $self->validateGotoPrecedenceRules($section, $section->{variable} || $sNum)){
+            push @messages,@errors;
+        }
         if (my $var = $section->{variable}) {
             if (my $count = $duplicateTargets->{$var}) {
                 push @messages, "Section $sNum variable name $var is re-used in $count other place(s).";
@@ -1301,6 +1304,47 @@ sub validateGotoExpression{
     return $engine->run($self->session, $object->{gotoExpression}, { validate => 1, validTargets => $goodTargets } );
 }
 
+sub validateGotoPrecedenceRules {
+    my $self = shift;
+    my $s = shift;
+    my $sLabel = shift;
+    my @errors;
+    my $endMsg = 'Precedence rules will apply.';
+    
+    my $hasSection
+        = $s->{goto}           =~ /\w/ ? 'Jump Target'
+        : $s->{gotoExpression} =~ /\w/ ? 'Jump Expression'
+        :                                '';
+    my $qNum = 0;
+    for my $q (@{$s->{questions}}) {
+        $qNum++;
+        my $qLabel = $q->{variable} || "Question $qNum";
+        my $hasQuestion
+            = $q->{goto}           =~ /\w/ ? 'Jump Target'
+            : $q->{gotoExpression} =~ /\w/ ? 'jump Expression'
+            :                                '';
+        if ( $hasSection && $hasQuestion) {
+            push @errors, "You have a $hasSection at $sLabel and a $hasQuestion at $qLabel. $endMsg";
+        }
+        my $aNum = 0;
+        for my $a (@{$q->{answers}}) {
+            $aNum++;
+            my $aLabel = "Answer $aNum";
+            my $hasAnswer
+                = $a->{goto}           =~ /\w/ ? 'Jump Target'
+                : $a->{gotoExpression} =~ /\w/ ? 'Jump Expression'
+                :                                '';
+            if ( $hasSection && $hasAnswer) {
+                push @errors, "You have a $hasSection at $sLabel and a $hasAnswer at $aLabel. $endMsg";
+            }
+            if ( $hasQuestion && $hasAnswer) {
+                push @errors, "You have a $hasQuestion at $qLabel and a $hasAnswer at $aLabel. $endMsg";
+            }
+        }
+    }
+    return @errors;
+}
+    
 =head2 section ($address)
 
 Returns a reference to one section.

www/extras/wobject/Survey/editsurvey/object.js

@@ -18,9 +18,7 @@ Survey.ObjectTemplate = (function(){
             editor.get('element').value = YAHOO.util.Dom.get('texteditortarget').value;
             editor.setEditorHTML(YAHOO.util.Dom.get('texteditortarget').value);
             YAHOO.util.Dom.setStyle("editor_container","visibility","visible");
-            var xy = YAHOO.util.Dom.getXY(YAHOO.util.Dom.get("texteditortarget").id);
+            YAHOO.util.Dom.setXY("editor_container",YAHOO.util.Dom.getXY(YAHOO.util.Dom.get("texteditortarget").id));
-            YAHOO.util.Dom.setXY("editor_container",xy);
-
         },
 
         initObjectEditor: function() {
@@ -158,6 +156,9 @@ Survey.ObjectTemplate = (function(){
             resizeGotoExpression.on('resize', function(ev) {
                 YAHOO.util.Dom.setStyle('gotoExpression_formId', 'width', (ev.width - 6) + "px");
                 YAHOO.util.Dom.setStyle('gotoExpression_formId', 'height', (ev.height - 6) + "px");
+                
+                // Resizing the gotoExpression box can cause the texteditor to move, so update its position
+                YAHOO.util.Dom.setXY("editor_container",YAHOO.util.Dom.getXY(YAHOO.util.Dom.get("texteditortarget").id));
             });
             
             // build the goto auto-complete widget

www/extras/wobject/Survey/surveyedit.css

@@ -134,6 +134,9 @@ li.answer {
     margin:5px;
     padding:10px;
 }
+.warning {
+    padding: 5px;
+}
 #sections-panel .bd {
     overflow: auto;
     background-color:#fff;