Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BI-1647 (View Experiments: Observation Dataset Table) #322

Merged
merged 18 commits into from
Aug 22, 2023
Merged

BI-1647 (View Experiments: Observation Dataset Table) #322

merged 18 commits into from
Aug 22, 2023

Conversation

davedrp
Copy link
Contributor

@davedrp davedrp commented Aug 2, 2023 •
edited
Loading

Description

BI-1647 (View Experiments: Observation Dataset Table)

Dependencies

bi-api: branch feature/BI-1647

Testing

  1. Import an Experiment with multiple Observation Units and multiple observations per Observation Unit.
  2. From the Experiments & Observations page, navigate to the imported experiment (from step 1).
  3. Verify that the data displayed in the table is correct.
  4. Test that the sort and filter feature works on each column of the table.

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 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: link to TAF run

@davedrp davedrp requested review from dmeidlin, a team and nickpalladino and removed request for a team August 2, 2023 19:17
@davedrp davedrp requested review from a team and mlm483 and removed request for dmeidlin and a team August 7, 2023 13:44
@davedrp davedrp mentioned this pull request Aug 7, 2023
Merged
6 tasks
@davedrp davedrp marked this pull request as ready for review August 7, 2023 16:38
mlm483
mlm483 requested changes Aug 7, 2023
View reviewed changes
Copy link
Contributor

@mlm483 mlm483 left a comment

Choose a reason for hiding this comment

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

I left a bunch of comments, it looks good overall.

src/breeding-insight/utils/BrAPIUtils.ts Outdated Show resolved Hide resolved
src/breeding-insight/service/ObservationService.ts Outdated Show resolved Hide resolved
src/breeding-insight/model/DatasetTableRow.ts Outdated Show resolved Hide resolved
src/breeding-insight/model/Experiment.ts Outdated Show resolved Hide resolved
src/breeding-insight/model/Observation.ts Outdated Show resolved Hide resolved
src/breeding-insight/model/ObservationVariable.ts Outdated Show resolved Hide resolved
src/views/import/Dataset.vue Outdated Show resolved Hide resolved
src/views/import/Dataset.vue Show resolved Hide resolved
src/views/import/Dataset.vue Outdated Show resolved Hide resolved
src/components/observations/ObservationsListTable.vue Show resolved Hide resolved
davedrp added 3 commits August 8, 2023 13:56
@davedrp
[BI-1647] Clean up code. delete model/Experiment.ts
db95978
@davedrp
[BI-1647]
89ac073 
Set minimum withd for columns in Dataset table
Made Dataset table scrollable
Observation model element names conform to BrAPI
Fixed bug in observatin column names
Improve colore of the no-data elements in the Dataset table
Commented Code
Misc bug fixes
@davedrp
[BI-1647] sort GID numerically
ad54553
@davedrp davedrp requested a review from mlm483 August 11, 2023 16:40
mlm483
mlm483 approved these changes Aug 14, 2023
View reviewed changes
Copy link
Contributor

@mlm483 mlm483 left a comment

Choose a reason for hiding this comment

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

One suggested change, looks good.

src/breeding-insight/model/Trial.ts Outdated
Comment on lines 44 to 45
console.log('xxxxxxxxxxxxxxxxxxxxxxxxxxxxx');
console.log(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
console.log('xxxxxxxxxxxxxxxxxxxxxxxxxxxxx');
console.log(this);

Copy link
Member

Choose a reason for hiding this comment

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

+1

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!

nickpalladino
nickpalladino reviewed Aug 15, 2023
View reviewed changes
Copy link
Member

@nickpalladino nickpalladino left a comment

Choose a reason for hiding this comment

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

Had an issue with a test experiment loading but wasn't able to reproduce with Dave so thinking it's a local issue. Included it here for reference but ok moving forward.

davetest.xls

src/breeding-insight/model/Trial.ts Outdated
Comment on lines 44 to 45
console.log('xxxxxxxxxxxxxxxxxxxxxxxxxxxxx');
console.log(this);
Copy link
Member

Choose a reason for hiding this comment

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

+1

src/components/observations/ObservationsListTable.vue Outdated
this.newObservationActive = true;
}

// updateObservation(updatedObservation: Observation) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this commented out stuff being saved for a future card?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. I removed the commented code and unused methods.

@davedrp davedrp requested a review from nickpalladino August 21, 2023 16:22
@davedrp davedrp merged commit 8c6cc09 into develop Aug 22, 2023
@davedrp davedrp deleted the feature/BI-1647 branch August 22, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mlm483 mlm483 mlm483 approved these changes

@nickpalladino nickpalladino nickpalladino approved these changes

Assignees

@nickpalladino nickpalladino

Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@davedrp @nickpalladino @mlm483