Adds more testing/error verbosity, fixes nested previous records bug (#…

…25)

* Test count_exact none case

* Make compatibility error message more verbose

* Fix case for nested previous record comparisons

CHANGELOG.md

@@ -7,13 +7,15 @@ Documentation of release versions of `nacc-form-validator`
 * Updates `_validate_compute_gds` to remove partial GDS score calculation
 * Updates `_check_subschema_valid` to accept an optional record parameter, defaults to the document - used for rules that require the previous record
 * Updates `compare_with` to support the `abs` operator
+* Updates `compatibility` error message to be more verbose
 * Refactors the tests to be more modularized so that they're more manageable
 * Refactors logic for `compare_values` by moving it to its own utility method
 * Adds `get_previous_record` method to grab previous record from Datastore, which can grab the previous record or the previous record where a specific field is non-empty
 	* Updates `_validate_temporalrules` to also support the `ignore_empty` parameter
 * Adds support for comparing against the previous record in `compare_with`
 * Adds new rule `compare_age` to handle rules that need to compare ages relative to a date
 * Adds custom operator `count_exact` to `json_logic.py`
+* Fixes issue where `datastore` was not being set for the temp validator in `_check_subschema_valid`, causing nested conditions with previous records to not evaluate correctly
 
 ## 0.3.0
 

nacc_form_validator/errors.py

@@ -26,8 +26,8 @@ class ErrorDefs:
     INVALID_DATE_MIN = ErrorDefinition(0x1005, "min")
     FILLED_TRUE = ErrorDefinition(0x1006, "filled")
     FILLED_FALSE = ErrorDefinition(0x1007, "filled")
-    COMPATIBILITY_TRUE = ErrorDefinition(0x1008, "compatibility")
-    COMPATIBILITY_FALSE = ErrorDefinition(0x1009, "compatibility")
+    COMPATIBILITY = ErrorDefinition(0x1008, "compatibility")
+    COMPATIBILITY_ELSE = ErrorDefinition(0x1009, "compatibility")
     TEMPORAL = ErrorDefinition(0x2000, "temporalrules")
     NO_PRIMARY_KEY = ErrorDefinition(0x2001, "temporalrules")
     NO_PREV_VISIT = ErrorDefinition(0x2002, "temporalrules")
@@ -84,9 +84,9 @@ def __set_custom_error_codes(self):
             0x1007:
             "must be empty",
             0x1008:
-            "{1} for {2} - compatibility rule no: {0}",
+            "{1} for if {2} then {3} - compatibility rule no: {0}",
             0x1009:
-            "{1} for {2} - compatibility rule no: {0}",
+            "{1} for if {2} else {3} - compatibility rule no: {0}",
             0x2000:
             "{1} in current visit for {2} in previous visit - temporal rule no: {0}",
             0x2001:

nacc_form_validator/nacc_validator.py

@@ -501,6 +501,12 @@ def _check_subschema_valid(
                 allow_unknown=True,
                 error_handler=CustomErrorHandler(subschema),
             )
+
+            # pass the same datastore
+            if self.primary_key and self.datastore:
+                temp_validator.primary_key = self.primary_key
+                temp_validator.datastore = self.datastore
+
             if operator == "OR":
                 valid = valid or temp_validator.validate(record,
                                                          normalize=False)
@@ -600,29 +606,32 @@ def _validate_compatibility(self, constraints: List[Mapping], field: str,
             else_conds = constraint.get(SchemaDefs.ELSE, None)
 
             # Check if dependencies satisfied the If clause
-            error_def = ErrorDefs.COMPATIBILITY_FALSE
+            error_def = ErrorDefs.COMPATIBILITY
             errors = None
             valid, _ = self._check_subschema_valid(if_conds, if_operator)
 
             # If the If clause valid, validate the Then clause
             if valid:
                 valid, errors = self._check_subschema_valid(
                     then_conds, then_operator)
-                error_def = ErrorDefs.COMPATIBILITY_TRUE
 
             # Otherwise validate the else clause, if they exist
             elif else_conds:
                 valid, errors = self._check_subschema_valid(
                     else_conds, else_operator)
-                error_def = ErrorDefs.COMPATIBILITY_FALSE
+                error_def = ErrorDefs.COMPATIBILITY_ELSE
             else:  # if the If condition is not satisfied, do nothing
                 pass
 
             # Something in the then/else clause failed - report errors
             if errors:
                 for error in errors.items():
-                    self._error(field, error_def, rule_no, str(error),
-                                if_conds)
+                    if error_def == ErrorDefs.COMPATIBILITY:
+                        self._error(field, error_def, rule_no, str(error),
+                                    if_conds, then_conds)
+                    else:
+                        self._error(field, error_def, rule_no, str(error),
+                                    if_conds, else_conds)
 
     # pylint: disable=(too-many-locals)
     def _validate_temporalrules(self, temporalrules: List[Mapping], field: str,

tests/test_nacc_validator.py

@@ -131,9 +131,9 @@ def test_lots_of_rules(create_nacc_validator):
 
     # invalid cases - compatibility
     assert not nv.validate({'adcid': 0, 'prevenrl': 1, 'oldadcid': None})
-    assert nv.errors == {'oldadcid': ["('oldadcid', ['null value not allowed']) for {'prevenrl': {'allowed': [1]}} - compatibility rule no: 0"]}
+    assert nv.errors == {'oldadcid': ["('oldadcid', ['null value not allowed']) for if {'prevenrl': {'allowed': [1]}} then {'oldadcid': {'nullable': False}} - compatibility rule no: 0"]}
     assert not nv.validate({'adcid': 0, 'prevenrl': 0, 'oldadcid': 1})
-    assert nv.errors == {'oldadcid': ["('oldadcid', ['must be empty']) for {'prevenrl': {'allowed': [0, 9]}} - compatibility rule no: 1"]}
+    assert nv.errors == {'oldadcid': ["('oldadcid', ['must be empty']) for if {'prevenrl': {'allowed': [0, 9]}} then {'oldadcid': {'nullable': True, 'filled': False}} - compatibility rule no: 1"]}
 
     # invalid cases, logic (adcid != oldadcid)
     assert not nv.validate({'adcid': 0, 'prevenrl': 1, 'oldadcid': 0})

tests/test_nacc_validator_datastore.py

@@ -264,6 +264,36 @@ def test_compare_with_previous_different_variable():
     nv.reset_record_cache()
     assert nv.validate({'patient_id': 'PatientID1', 'visit_num': 2, 'birthyear': 1950})
 
+def test_temporal_check_with_nested_compare_with_previous_record():
+    """ Test when compare_with previous_record nested inside a temporalrules """
+    schema = {
+        "patient_id": {"type": "string"},
+        "visit_num": {"type": "integer"},
+        "birthyr": {
+            "type": "integer",
+            "temporalrules": [
+                {
+                    "index": 0,
+                    "previous": {"birthyr": {"forbidden": [-1]}},
+                    "current": {
+                        "birthyr": {
+                            "compare_with": {
+                                "comparator": "==",
+                                "base": "birthyr",
+                                "previous_record": True
+                            }
+                        }
+                    }
+                }
+            ]
+        }
+    }
+    nv = create_nacc_validator_with_ds(schema, 'patient_id', 'visit_num')
+    assert nv.validate({'patient_id': 'PatientID1', 'visit_num': 4, 'birthyr': 1950})
+
+    assert not nv.validate({'patient_id': 'PatientID1', 'visit_num': 4, 'birthyr': 1951})
+    assert nv.errors == {'birthyr': ['(\'birthyr\', ["input value doesn\'t satisfy the condition birthyr == birthyr (previous record)"]) in current visit for {\'birthyr\': {\'forbidden\': [-1]}} in previous visit - temporal rule no: 0']}
+
 def test_check_with_rxnorm():
     """ Test checking drugID is a valid RXCUI """
     schema = {