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.

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

CRDCDH-1995 Validation Results Aggregated/Expanded Table Views #557

Open
wants to merge 19 commits into
base: 3.2.0
Choose a base branch
from

Conversation

Alejandro-Vega
Copy link
Collaborator

@Alejandro-Vega Alejandro-Vega commented Dec 9, 2024

Overview

Added Aggregated/Expanded view to the table in Validation Results tab in a Data Submission.

Note

The BE currently does not populate issue codes. (They all return as null) which breaks some functionality such as the issue type filter, "Expand" and button. There is also a BE bug when passing -1 for the "first" param of the aggregated submission QC results API.

Change Details (Specifics)

  • Added switch to toggle between Aggregated and Expanded view
  • Added additional filter "Issue Type" to filter by a specific issue code
  • Defined table for Aggregated view. The Expanded view is the existing table
  • Set default pagination to 20
  • When user clicks on "Expand" button it should select the filter in the Issue Type dropdown and switch to Expanded view

Related Ticket(s)

CRDCDH-2037 (Aggregated View Task)
CRDCDH-1995 (Expanded View Task)
CRDCDH-1889 (US)

@Alejandro-Vega Alejandro-Vega added the 🚧 Do Not Merge This PR is not ready for merging label Dec 9, 2024
@Alejandro-Vega Alejandro-Vega added this to the 3.2.0 (PMVP-M3) milestone Dec 9, 2024
@coveralls
Copy link
Collaborator

coveralls commented Dec 10, 2024

Pull Request Test Coverage Report for Build 12589515921

Details

  • 113 of 129 (87.6%) changed or added relevant lines in 5 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 56.841%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/content/dataSubmissions/QualityControl.tsx 50 66 75.76%
Files with Coverage Reduction New Missed Lines %
src/content/dataSubmissions/QualityControl.tsx 1 73.04%
Totals Coverage Status
Change from base Build 12505620523: 0.2%
Covered Lines: 3887
Relevant Lines: 6382

💛 - Coveralls

@Alejandro-Vega Alejandro-Vega marked this pull request as ready for review December 31, 2024 22:49
Copy link
Member

@amattu2 amattu2 left a comment

Choose a reason for hiding this comment

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

I tested it locally as best as I could and annotated a few comments. Feel free to dismiss any.

// "All" is the default selection for all filters
expect(mockMatcher).not.toHaveBeenCalledWith(
expect.objectContaining({ batchIDs: expect.anything(), nodeTypes: expect.anything() })
);
expect(mockMatcher).toHaveBeenCalledWith(expect.objectContaining({ severities: "All" }));
// TODO: FIX TEST
// expect(mockMatcher).toHaveBeenCalledWith(expect.objectContaining({ severities: "All" }));
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 a TODO for when the API works, or is this broken due to ACT/etc?

Just pointing this out in case it was forgotten. If it is an ACT issue, feel free to leave it commented out. Tracing the root cause for act is a nightmare 😃

src/content/dataSubmissions/QualityControl.tsx Outdated Show resolved Hide resolved
const handleFilterChange = (field: keyof FilterForm) => {
setTouchedFilters((prev) => ({ ...prev, [field]: true }));
};

const Actions = useMemo<React.ReactNode>(
Copy link
Member

@amattu2 amattu2 Jan 2, 2025

Choose a reason for hiding this comment

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

Currently, the export button is only exporting the QC Results. I think the expectation would be that it exports the Aggregated Results when that's selected? I could be wrong.

src/types/Submissions.d.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚧 Do Not Merge This PR is not ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants