diff --git a/docs/changelog/7.x.x.txt b/docs/changelog/7.x.x.txt index c96740755..039766b9b 100644 --- a/docs/changelog/7.x.x.txt +++ b/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. diff --git a/lib/WebGUI/Asset/Wobject/Survey/ExpressionEngine.pm b/lib/WebGUI/Asset/Wobject/Survey/ExpressionEngine.pm index e8cc2dcbc..859670a0b 100644 --- a/lib/WebGUI/Asset/Wobject/Survey/ExpressionEngine.pm +++ b/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}) { - my $values = $other_instance->{values}; + if (my $otherInstance = $otherInstances->{$asset_spec}) { + 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 - if ( @_ == 2 ) { - 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 $key = shift; + _validateVariable($key, 'score'); 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}) { - my $scores = $other_instance->{scores}; + if (my $otherInstance = $otherInstances->{$asset_spec}) { + 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}) { - my $values = $other_instance->{values}; + if (my $otherInstance = $otherInstances->{$asset_spec}) { + 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}) { - my $tags = $other_instance->{tags}; + if (my $otherInstance = $otherInstances->{$asset_spec}) { + 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} ) { - $session->log->debug("Invalid target [$target]"); - if ($validate) { - die("Invalid jump target \"$target\""); # bail and report error - } - else { - return; # skip jump but continue with expression - } + if (!_validateVariable($target, 'jump')) { + 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} ) { - my $asset_spec = $@->{other_instance}; + if ( ref $@ && ref $@ eq 'HASH' && $@->{otherInstance} ) { + my $asset_spec = $@->{otherInstance}; $session->log->debug("Resolving external reference: $asset_spec"); my $asset; diff --git a/lib/WebGUI/Asset/Wobject/Survey/SurveyJSON.pm b/lib/WebGUI/Asset/Wobject/Survey/SurveyJSON.pm index a5d446763..fc1740ec0 100644 --- a/lib/WebGUI/Asset/Wobject/Survey/SurveyJSON.pm +++ b/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. diff --git a/www/extras/wobject/Survey/editsurvey/object.js b/www/extras/wobject/Survey/editsurvey/object.js index 7c963ecc9..de9a1ba2c 100644 --- a/www/extras/wobject/Survey/editsurvey/object.js +++ b/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",xy); - + YAHOO.util.Dom.setXY("editor_container",YAHOO.util.Dom.getXY(YAHOO.util.Dom.get("texteditortarget").id)); }, 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 diff --git a/www/extras/wobject/Survey/surveyedit.css b/www/extras/wobject/Survey/surveyedit.css index b6f4c84e2..6ba112d0d 100644 --- a/www/extras/wobject/Survey/surveyedit.css +++ b/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;