Skip to content

BI-1612 - Experiment Import Phenotype Data Persistence#220

Merged
nickpalladino merged 13 commits intodevelopfrom
feature/BI-1612
Nov 7, 2022
Merged

BI-1612 - Experiment Import Phenotype Data Persistence#220
nickpalladino merged 13 commits intodevelopfrom
feature/BI-1612

Conversation

@nickpalladino
Copy link
Member

@nickpalladino nickpalladino commented Oct 25, 2022

Description

Story: BI-1612

  • Added minimal BrAPIObservation construction for import which is persisted to the database and returned to the front end
  • Added observations to import statistics

Dependencies

  • bi-web develop

Testing

  • Evaluate how blank observation values are handled and how we want that to work. Right now it just ignores them and will not include in the preview data or posted data.
  • Change to PendingImport may affect prototype importer and should probably be regression tested at some point

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>

@nickpalladino nickpalladino requested review from a team, dmeidlin and timparsons and removed request for a team October 25, 2022 21:24
@timparsons timparsons self-assigned this Nov 1, 2022
Copy link
Member

@timparsons timparsons left a comment

Choose a reason for hiding this comment

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

I need to test the code still, but had two minor questions

}

private String createObservationUnitKey(ExperimentObservation importRow) {
private static String createObservationUnitKey(ExperimentObservation importRow) {
Copy link
Member

Choose a reason for hiding this comment

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

Why was this changed to be static?

Copy link
Member Author

Choose a reason for hiding this comment

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

Think I just changed it when I was in there in line with the rationale in the following comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed changes.

Comment on lines +333 to +345
private static String getImportObservationHash(ExperimentObservation importRow, String variableName) {
// TODO: handle timestamps once we support them
return getObservationHash(createObservationUnitKey(importRow), variableName, importRow.getEnv());
}

//TODO: Add timestamp parameter once we support them
private static String getObservationHash(String observationUnitName, String variableName, String studyName) {
String concat = DigestUtils.sha256Hex(observationUnitName) +
DigestUtils.sha256Hex(variableName) +
DigestUtils.sha256Hex(studyName);
return DigestUtils.sha256Hex(concat);
}

Copy link
Member

Choose a reason for hiding this comment

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

Why are these methods static?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had them as static in the old ObservationProcessor and just brought that over because there is no dependence on the class instance.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. I'd recommend removing the static modifier to avoid any potential weirdness going between an instance of a class and a static method outside of the object

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed changes.

Copy link
Member

@timparsons timparsons left a comment

Choose a reason for hiding this comment

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

Testing passed.

@nickpalladino nickpalladino merged commit 35ca1b0 into develop Nov 7, 2022
@nickpalladino nickpalladino deleted the feature/BI-1612 branch November 7, 2022 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants