g4.85 Update Validators: EmptyCell, ValueInList and DuplicateTraits

[carolyncaron]

Issue #85

Dependencies

#105 - complete!

Motivation

The 3 validators I have been primarily working on - EmptyCell, ValueInList, and Duplicate Traits - needed to be updated to accept configuration using setters and to return the new return values. This involved updating the tests and configuring the validators in the importer formValidate().

What does this PR do?

1. Updates each validator to use their respective traits to set and get configuration values

   EmptyCell:

   • Use getIndices() in validateRow()
   • Use setIndices() in configureValidators()

   ValueInList:

   • Use getIndices() in validateRow()
   • Use setIndices() in configureValidators()
   • Use getValidValues() in validateRow()
   • Use setValidValues() in configureValidators()

   DuplicateTraits:

   • Use getConfiguredGenus() in validateRow()
   • Use setConfiguredGenus() in configureValidators()
   • Use getIndices() in validateRow()
   • Use setIndices() in configureValidators()

2. Updates the validators to indicate their inputType and removes their scope in the annotation block.

   • EmptyCell
   • ValueInList
   • DuplicateTraits
   • Update ValidatorFileRowScopeTest and ValidatorTraitImporterTest to remove mention of scope

3. Updates the validators to return the new style of return information:

   'case': a developer focused string describing the case checked
   'valid': either TRUE or FALSE depending on if the data is valid or not
   'failedItems': an array of "items" to be used in the message to the user. This is empty if the data was valid

   • EmptyCell
   • ValueInList
   • DuplicateTraits

4. Updates were made to the formValidate() method in the Traits Importer class:

   • Removed all reference to the old return style specifically for data-row validators
   • Removed the declaration of the $validation array and its use- validation results are no longer stored as they happen
   • Instead, validation results are now being stored in the $failures array only if validation fails. The $failures[$validation_name] array only gets declared if validation occurs at all for that validator instance, which will be used later to determine which validation statuses need to be 'pass' vs 'todo'.
   • Added a new method processValidationMessages() to handle the $failures array and to define the default messages which will later be passed to the user through the UI

Testing

Automated Testing

Some tests were re-organized as follows:

• ValidatorFileRowScopeTest.php was removed
  • ValidatorFileRowScopeTest::testValidatorValueInList() => ValidatorValueInListTest::testValidatorValueInList()
  • ValidatorFileRowScopeTest::testValidatorEmptyCell() => ValidatorEmptyCellTest::testValidatorEmptyCell()
• ValidatorTraitImporterTest was renamed and all of its methods moved to ValidatorDuplicateTraitsTest

NEW Test added

• testIndexKeyExceptions() was added to ValidatorDuplicateTraitsTest
  • checks for each of the index keys that were configured with setIndices() using data provider provideWrongIndexKeys()
    • Trait Name
    • Method Short Name
    • Unit

The following tests were updated by adding additional asserts to check for all of the expected values in the return array

• ValidatorValueInListTest::testValidatorValueInList()
• ValidatorEmptyCellTest::testValidatorEmptyCell()
• ValidatorDuplicateTraitsTest::testValidatorDuplicateTraitsFile()
• ValidatorDuplicateTraitsTest::testValidatorDuplicateTraitsDatabase()

Miscellaneous updates

• TraitImporterFormValidateTest::testTraitFormValidation() has a new assert for ensure the 'details' value in the data provider is not an empty string
  • also added documentation to both the data provider and test method
• ValidatorBaseTest was modified to use the base fake validator instead of being dependent on an instance of the ValueInList validator

[laceysanderson]

Overall this looks great! Just a few more concrete comments:

• Do we need to set the genus in the validator or is it done by the trait?
• ensure raw_results at the same level as the title/details
• make clear in comments that the key from processValidationMessages() is the unique identifier of the validator UI line that just happens to currently be the validator instance key but may not continue to be so.

[laceysanderson]

All the changes look great.

Only thing I noticed was that there is a gap in the test coverage that must have snuck in the previous PR 🙈

[codeclimate]

Code Climate has analyzed commit e688f14 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 94.5% (50% is the threshold).

This pull request will bring the total coverage in the repository to 70.8% (0.0% change).

View more on Code Climate.

[carolyncaron]

Thanks @laceysanderson! I created a new test testIndexKeyExceptions and data provider provideWrongIndexKeys to cover all 3 of those exception messages in commit b65a321

[laceysanderson]

Changes look good! Only remaining gaps in test coverage are related to new vs. old return style which will soon be resolved!

Ready to merge! 🎉