added: validation for appearances that depend on other appearances,

fixed: false error for repeats without ref attribute kobotoolbox/enketo-express#1050
enketo · Jun 13, 2018 · a31c34d · a31c34d
1 parent 4e41d6c
commit a31c34d
Show file tree

Hide file tree

Showing 8 changed files with 41 additions and 24 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,17 +2,19 @@
 All notable changes to this project will be documented in this file.
 This project adheres to [Semantic Versioning](http://semver.org/).
 
-Unreleased
----------------------
+[1.4.0] - 2018-06-13
+--------------------
 ##### Added
-- Version property when used as CommonJS module
+- Version property when used as CommonJS module.
+- Validation for appearances that depend on other appearances.
 
 ##### Changed
 - Ignore deprecated appearance usage errors in --oc mode.
 - No longer providing final 'Valid' or 'Invalid' line in output in --oc mode.
 
 ##### Fixed
 - Analog-scale appearance outputs warning.
+- False error for repeat without ref attribute.
 
 [1.3.0] - 2018-06-07
 ---------------------

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -1,8 +1,9 @@
 {
     "name": "enketo-validate",
-    "version": "1.3.0",
+    "version": "1.4.0",
     "description": "An XForm validator around Enketo's form engine",
     "main": "src/validator.js",
+    "bin": "./validate",
     "scripts": {
         "test": "mocha test/spec/*.spec.js",
         "install": "browserify src/FormModel.js > build/FormModel-bundle.js"
@@ -37,4 +38,4 @@
         "mocha": "^5.0.1",
         "pkg": "^4.3.0"
     }
-}
+}
diff --git a/src/appearances.json b/src/appearances.json
@@ -121,5 +121,11 @@
         "controls": [ "input" ],
         "types": [ "decimal", "xsd:decimal", "int", "xsd:int" ]
     },
-    "no-ticks": "analog-scale"
+    "no-ticks": {
+        "appearances": [ "analog-scale" ]
+    },
+    "comment": {
+        "types": [ "string", "xsd:string" ]
+    },
+    "dn": "comment"
 }
diff --git a/src/xform.js b/src/xform.js
@@ -237,38 +237,41 @@ class XForm {
                     if ( typeof rules === 'string' ) {
                         rules = appearanceRules[ rules ];
                     }
-                    const ref = control.getAttribute( 'ref' );
+                    const controlNsPrefix = this.nsPrefixResolver( control.namespaceURI );
+                    const controlName = controlNsPrefix && /:/.test( control.nodeName ) ? controlNsPrefix + ':' + control.nodeName.split( ':' )[ 1 ] : control.nodeName;
+                    const pathAttr = controlName === 'repeat' ? 'nodeset' : 'ref';
+                    const ref = control.getAttribute( pathAttr );
                     if ( !ref ) {
-                        errors.push( 'Question found in body that has no ref attribute' );
+                        errors.push( `Question found in body that has no ${pathAttr} attribute (${control.nodeName}).` );
                         return;
                     }
                     const nodeName = ref.substring( ref.lastIndexOf( '/' ) + 1 ); // in model!
-                    const controlNsPrefix = this.nsPrefixResolver( control.namespaceURI );
                     const bindEl = this.bind( ref );
-                    const controlName = controlNsPrefix && /:/.test( control.nodeName ) ? controlNsPrefix + ':' + control.nodeName.split( ':' )[ 1 ] : control.nodeName;
                     let dataType = bindEl ? bindEl.getAttribute( 'type' ) : 'string';
                     // Convert ns prefix to properly evaluate XML Schema datatypes regardless of namespace prefix used in XForm.
                     const typeValNs = /:/.test( dataType ) ? bindEl.lookupNamespaceURI( dataType.split( ':' )[ 0 ] ) : null;
                     dataType = typeValNs ? `${this.nsPrefixResolver(typeValNs)}:${dataType.split(':')[1]}` : dataType;
                     if ( !rules ) {
-                        warnings.push( `Appearance "${appearance}" for question "${nodeName}" is not supported` );
+                        warnings.push( `Appearance "${appearance}" for question "${nodeName}" is not supported.` );
                         return;
                     }
                     if ( rules.controls && !rules.controls.includes( controlName ) ) {
-                        warnings.push( `Appearance "${appearance}" for question "${nodeName}" is not valid for this question type (${control.nodeName})` );
+                        warnings.push( `Appearance "${appearance}" for question "${nodeName}" is not valid for this question type (${control.nodeName}).` );
                         return;
                     }
                     if ( rules.types && !rules.types.includes( dataType ) ) {
                         // Only check types if controls check passed.
                         // TODO check namespaced types when it becomes applicable (for XML Schema types).
-                        warnings.push( `Appearance "${appearance}" for question "${nodeName}" is not valid for this data type (${dataType})` );
+                        warnings.push( `Appearance "${appearance}" for question "${nodeName}" is not valid for this data type (${dataType}).` );
+                        return;
+                    }
+                    if ( rules.appearances && !rules.appearances.some( appearanceMatch => appearances.includes( appearanceMatch ) ) ) {
+                        warnings.push( `Appearance "${appearance}" for question "${nodeName}" requires any of these appearances: ${rules.appearances}.` );
                         return;
                     }
-                    // TODO: if an appearance is only valid when another appearance is used (e.g. no-ticks)
-
                     // switched off when warnings are output as errors (for OC) - may need different approach
                     if ( rules.preferred && warnings !== errors ) {
-                        warnings.push( `Appearance "${appearance}" for question "${nodeName}" is deprecated, use "${rules.preferred}" instead` );
+                        warnings.push( `Appearance "${appearance}" for question "${nodeName}" is deprecated, use "${rules.preferred}" instead.` );
                     }
                     // Possibilities for future additions:
                     // - check accept/mediaType

diff --git a/test/spec/xform.spec.js b/test/spec/xform.spec.js
@@ -108,7 +108,7 @@ describe( 'XForm', () => {
         const xf = loadXForm( 'appearances.xml' );
         const result = validator.validate( xf );
         const resultOc = validator.validate( xf, { openclinica: true } );
-        const ISSUES = 13;
+        const ISSUES = 14;
 
         it( 'outputs warnings', () => {
             expect( result.warnings.length ).to.equal( ISSUES );
@@ -122,6 +122,7 @@ describe( 'XForm', () => {
             expect( arrContains( result.warnings, /"numbers" for question "g"/i ) ).to.equal( true );
             expect( arrContains( result.warnings, /"horizontal-compact" for question "k" .+ deprecated.+"compact"/i ) ).to.equal( true );
             expect( arrContains( result.warnings, /"field-list" for question "two"/i ) ).to.equal( true );
+            expect( arrContains( result.warnings, /"no-ticks" for question "g"/i ) ).to.equal( true );
         } );
 
         it( 'outputs no errors', () => {

diff --git a/test/xform/appearances.xml b/test/xform/appearances.xml
@@ -74,7 +74,7 @@
                 </item>
             </select1>
 
-            <input ref="/data/one/g" appearance="numbers">
+            <input ref="/data/one/g" appearance="numbers no-ticks">
                 <label>label</label>
             </input>
             <input ref="/data/one/h" appearance="numbers maps signature">                <!-- numbers is correct -->
@@ -99,6 +99,6 @@
                 </item>
             </select1>
         </group>
-        <repeat ref="/data/two" appearance="field-list"/>
+        <repeat nodeset="/data/two" appearance="field-list"/>
     </h:body>
 </h:html>
diff --git a/validate b/validate
@@ -22,7 +22,11 @@ const _getFileContents = filePath => {
     } );
 };
 
-const _output = ( issues = [], error = false ) => console[ error ? 'error' : 'log' ]( `${issues.join( '\n\n' )}` );
+const _output = ( issues = [], error = false ) => {
+    if ( issues.length ) {
+        console[ error ? 'error' : 'log' ]( `\n\n${issues.join( '\n\n' )}` );
+    }
+};
 
 program
     .usage( '[options] <file>' )
@@ -53,10 +57,10 @@ if ( program.me ) {
             _output( result.errors, true );
 
             if ( result.errors.length ) {
-                _output( [ options.openclinica ? '' : '\n\nResult: Invalid\n\n' ], true );
+                _output( [ options.openclinica ? '' : 'Result: Invalid\n\n' ], true );
                 process.exit( 1 );
             } else {
-                _output( [ options.openclinica ? '' : '\n\n>> XForm is valid! See above for any warnings.\n\n' ] );
+                _output( [ options.openclinica ? '' : '>> XForm is valid! See above for any warnings.\n\n' ] );
                 process.exit( 0 );
             }
         } );