Skip to content

[BI-2328] Experimental observation file import removes periods from column headers#411

Merged
davedrp merged 4 commits intodevelopfrom
bug/BI-2328
Oct 24, 2024
Merged

[BI-2328] Experimental observation file import removes periods from column headers#411
davedrp merged 4 commits intodevelopfrom
bug/BI-2328

Conversation

@davedrp
Copy link
Contributor

@davedrp davedrp commented Oct 9, 2024

BI-2328 Experimental observation file import removes periods from column headers

Added a validation so that a user can no longer add an ontology term with a period (.) in the Name field.

Dependencies

bi-web: bug/BI-2328

Testing

Test 1 (Manually add Ontology)

  1. Given user is logged in the application as a Program Administrator
  2. Given user has selected an appropriate program (one in which they are a Program Administrator).
  3. WHEN user selects "Ontology" in navigation
  4. AND user selects 'New Term' button on ontology list page
  5. AND user enters a text value with a period ('.') into the 'Name' field
  6. AND user selects 'Save' button
  7. THEN an error banner will appear with the text "Period in name is invalid"
  8. AND the error message "Period in name is invalid" will appear below the 'Name' field

Test 2 (import Ontology)

  1. Given user is logged in the application as a Program Administrator
  2. Given user has selected an appropriate program (one in which they are a Program Administrator).
  3. WHEN user selects "Ontology" in navigation
  4. AND user selects "Manage Ontology" button
  5. AND user selects "Import file" link
  6. AND user uploads Ontology "bi_ontology_UAT-tyrwh-oat.xls" file
  7. AND user selects 'Import' button
  8. THEN an error banner will appear with the text, "Error(s) detected in file, bi_ontology_UAT-tyrwh-oat.xls . (See details below.) Import cannot proceed."
  9. AND the error table will have the following values:
  • Row = 7
  • Field = 'Name'
  • Error = 'Period is invalid'

Checklist:

  • I have performed a self-review of my own code
  • I have tested my code and ensured it meets the acceptance criteria of the story
  • I have tested that my code works with both the brapi-java-server and BreedBase
  • I have create/modified unit tests to cover this change
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to documentation
  • I have run TAF: <please include a link to TAF run>

@davedrp davedrp requested review from a team, dmeidlin and nickpalladino and removed request for a team October 9, 2024 19:21
@github-actions github-actions bot added the bug Something isn't working label Oct 9, 2024
@davedrp davedrp marked this pull request as ready for review October 9, 2024 20:02
.additionalInfo(toJsonObject(additionalInfoString))
.collector("intern")
.externalReferences(createExternalReferences())
.geoCoordinates(BrApiGeoJSON.builder().geometry(null).type("Feature").build())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the reason this was removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was a problem I had locally. It should not have been uploaded. I will revert it.
-Thanks

Comment on lines +70 to +71
return new ValidationError("observationVariableName", "Period in name is invalid", HttpStatus.BAD_REQUEST);
}
Copy link
Contributor

@dmeidlin dmeidlin Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to keep the error messages consistent between TraitValidatortError and TraitFileValidatorError, so for example, this message could be changed to "Period is invalid".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a banner error message. Table messages have a field the tells the user which field contains the error, banner messages do not. So, it was decided that this message should contain the name of the filed.

Comment on lines +220 to +229
if (name != null && name.contains(".")){
ValidationError error = traitValidatorErrors.getPeriodObsVarNameMsg();
errors.addError(traitValidatorErrors.getRowNumber(i), error);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is fine as is, but since this validation step is checking the entire trait format, it might be nice to use a regex here so that it can be easily updated in the future to handle additional characters or edge cases.

Suggested change
if (name != null && name.contains(".")){
ValidationError error = traitValidatorErrors.getPeriodObsVarNameMsg();
errors.addError(traitValidatorErrors.getRowNumber(i), error);
}
Pattern pattern = Pattern.compile("\\.");
Matcher matcher = pattern.matcher(name);
boolean containsInvalidCharacter = matcher.find();
if (name != null && containsInvalidCharacter){
ValidationError error = traitValidatorErrors.getPeriodObsVarNameMsg();
errors.addError(traitValidatorErrors.getRowNumber(i), error);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@dmeidlin
Copy link
Contributor

Looks good, and tests passed. Thanks for including the test import file!

@davedrp davedrp requested a review from dmeidlin October 23, 2024 15:58
@davedrp davedrp merged commit d6bcf37 into develop Oct 24, 2024
@davedrp davedrp deleted the bug/BI-2328 branch October 24, 2024 19:38
@davedrp davedrp mentioned this pull request Oct 28, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants