From b856786efcda2fba614af3b6be34e8045f823ce8 Mon Sep 17 00:00:00 2001 From: "alex.hill@gmail.com" Date: Tue, 17 Sep 2024 11:50:05 +0100 Subject: [PATCH 1/4] display warnings and errors on plots --- src/components/CovariateOptions.tsx | 2 +- src/components/LinePlot.tsx | 62 +++++++++++++++++++---------- src/components/PlotError.tsx | 17 ++++++++ src/components/PlotWarnings.tsx | 24 +++++++++++ src/components/SplineOptions.tsx | 2 +- src/generated.d.ts | 14 +++---- src/services/apiService.ts | 11 +++++ src/services/dataService.ts | 2 +- 8 files changed, 102 insertions(+), 32 deletions(-) create mode 100644 src/components/PlotError.tsx create mode 100644 src/components/PlotWarnings.tsx diff --git a/src/components/CovariateOptions.tsx b/src/components/CovariateOptions.tsx index 67e3990..ec0c2ef 100644 --- a/src/components/CovariateOptions.tsx +++ b/src/components/CovariateOptions.tsx @@ -31,7 +31,7 @@ export default function CovariateOptions({covariates}: Props) { type: ActionType.SELECT_COVARIATE, payload: { name: selectedVariable.name, - levels: selectedVariable.levels, + levels: selectedVariable.levels.filter(l => l === 0 || l), display: selectedDisplayOption } }) diff --git a/src/components/LinePlot.tsx b/src/components/LinePlot.tsx index 2179cb9..bf03358 100644 --- a/src/components/LinePlot.tsx +++ b/src/components/LinePlot.tsx @@ -1,10 +1,13 @@ import React, {useContext, useState} from 'react'; import Plot from 'react-plotly.js'; import {RootContext, RootDispatchContext} from "../RootContext"; -import {DataSeries} from "../generated"; +import {DataSeries, ErrorDetail} from "../generated"; import {dataService} from "../services/dataService"; import {toFilename} from "../services/utils"; import {useDebouncedEffect} from "../hooks/useDebouncedEffect"; +import PlotError from "./PlotError"; +import PlotWarnings from "./PlotWarnings"; +import {Dict} from "../types"; interface Props { biomarker: string @@ -35,6 +38,7 @@ export default function LinePlot({ const dispatch = useContext(RootDispatchContext); const [seriesData, setSeries] = useState(null); + const [plotError, setPlotError] = useState(null); const len = facetVariables.length const facetDefinitions: string[] = []; @@ -48,6 +52,7 @@ export default function LinePlot({ const scale = state.datasetSettings[state.selectedDataset].scale; const splineSettings = state.datasetSettings[state.selectedDataset].splineSettings; useDebouncedEffect(() => { + setPlotError(null); const fetchData = async () => { const result = await dataService(state.language, dispatch) .getDataSeries(state.selectedDataset, @@ -58,34 +63,44 @@ export default function LinePlot({ } else { setSeries(null) } + if (result && result.errors?.length) { + setPlotError(result.errors[0]) + } } fetchData(); }, [state.language, dispatch, state.selectedDataset, biomarker, facetDefinition, covariateSettings, scale, splineSettings], 100); let series: any[] = []; + const warnings: Dict = {}; if (seriesData) { - series = seriesData.flatMap((series, index) => ([{ - x: series.model.x, - y: series.model.y, - name: series.name, - legendgroup: series.name, - type: "scatter", - mode: "line", - line: {shape: 'spline', width: 2}, - showlegend: seriesData.length > 1, - marker: {color: colors[index]} - }, - { - x: series.raw.x, - y: series.raw.y, - name: series.name, - legendgroup: series.name, + series = seriesData.flatMap((series, index) => { + const name = series.name || "unknown"; + if (series.warnings) { + warnings[name] = series.warnings; + } + return [{ + x: series.model?.x || [], + y: series.model?.y || [], + name: name, + legendgroup: name, type: "scatter", - mode: "markers", + mode: "line", + line: {shape: 'spline', width: 2}, showlegend: false, - marker: {color: colors[index], opacity: 0.5} - }])) + marker: {color: colors[index]} + }, + { + x: series.raw.x, + y: series.raw.y, + name: name, + legendgroup: name, + type: "scatter", + mode: "markers", + showlegend: seriesData.length > 1, + marker: {color: colors[index], opacity: 0.5} + }] + }) } else { series = [] } @@ -95,7 +110,7 @@ export default function LinePlot({ title += " " + facetDefinition } - return {series.length > 0 && + />}{plotError && + + } + } diff --git a/src/components/PlotError.tsx b/src/components/PlotError.tsx new file mode 100644 index 0000000..2b923fb --- /dev/null +++ b/src/components/PlotError.tsx @@ -0,0 +1,17 @@ +import {Alert} from "react-bootstrap"; +import React from "react"; +import {ErrorDetail} from "../generated"; + +interface Props { + error: ErrorDetail + title: string +} + +export default function PlotError({error, title}: Props) { + const message = error.detail ?? `API returned an error: ${error.error}`; + return +

Facet for {title} could not be generated due to the following error:

+ {message} {error.error === "SESSION_EXPIRED" && + Re-upload your data to continue.} + +} diff --git a/src/components/PlotWarnings.tsx b/src/components/PlotWarnings.tsx new file mode 100644 index 0000000..ca7584b --- /dev/null +++ b/src/components/PlotWarnings.tsx @@ -0,0 +1,24 @@ +import {Alert} from "react-bootstrap"; +import React from "react"; +import {Dict} from "../types"; + +interface Props { + warnings: Dict +} + +export default function PlotWarnings({warnings}: Props) { + const keys = Object.keys(warnings); + if (keys.length == 0) { + return null; + } + return +

Some traces generated warnings

+ {keys.map((k, i) =>
{k}: +
    { + warnings[k].map(w => +
  • {w}
    • )} +
+
)} + +} diff --git a/src/components/SplineOptions.tsx b/src/components/SplineOptions.tsx index 595fd61..d12450b 100644 --- a/src/components/SplineOptions.tsx +++ b/src/components/SplineOptions.tsx @@ -32,7 +32,7 @@ export default function SplineOptions() { const settings = state.datasetSettings[state.selectedDataset].splineSettings; - return + return Method: diff --git a/src/generated.d.ts b/src/generated.d.ts index 2cd3e90..f6a2733 100644 --- a/src/generated.d.ts +++ b/src/generated.d.ts @@ -5,16 +5,16 @@ * and run ./generate_types.sh to regenerate this file. */ export type DataSeries = { - name?: string; + name: string; model: { - x: number[]; + x: (number | string)[]; y: (number | null)[]; - }; + } | null; raw: { - x: number[]; + x: (number | string)[]; y: (number | null)[]; }; - [k: string]: unknown; + warnings: string[] | null; }[]; export interface DatasetMetadata { variables: VariableSchema[]; @@ -23,7 +23,7 @@ export interface DatasetMetadata { } export interface VariableSchema { name: string; - levels: string[]; + levels: (string | number | null)[]; } export type DatasetNames = string[]; export interface ErrorDetail { @@ -50,6 +50,6 @@ export interface ResponseSuccess { export type UploadResult = string; export interface Variable { name: string; - levels: string[]; + levels: (string | number | null)[]; } export type Version = string; diff --git a/src/services/apiService.ts b/src/services/apiService.ts index 592c314..6d4f67e 100644 --- a/src/services/apiService.ts +++ b/src/services/apiService.ts @@ -113,6 +113,17 @@ export class APIService implements API { if (e.response && e.response.status === 404) { this._dispatchError(APIService.createError("Your session may have expired.", "SESSION_EXPIRED")); } + + const error = (e.response && e.response.data); + + if (isPorcelainResponse(error)) { + return error + } else { + return { + status: "failure", + errors: [APIService.createError("Could not parse API response. If error persists, please contact support.")] + } + } }; private _dispatchError = (error: ErrorDetail) => { diff --git a/src/services/dataService.ts b/src/services/dataService.ts index 5b97b14..83f45a4 100644 --- a/src/services/dataService.ts +++ b/src/services/dataService.ts @@ -72,7 +72,7 @@ export class DataService { return await this._api .ignoreSuccess() - .withError(ActionType.ERROR_ADDED) + .ignoreErrors() .get("/dataset/" + selectedDataset + "/trace/" + biomarker + "/" + queryString) } } From 2ebdb017d9bcdc568303ce6796b4fceedbcabd49 Mon Sep 17 00:00:00 2001 From: "alex.hill@gmail.com" Date: Tue, 17 Sep 2024 12:13:21 +0100 Subject: [PATCH 2/4] tests --- src/components/LinePlot.tsx | 2 +- src/components/PlotWarnings.tsx | 2 +- test/components/ExploreDataset.test.tsx | 15 +++-- test/components/LinePlot.test.tsx | 89 ++++++++++++++++++------- test/mocks.ts | 11 +-- test/services/utils.test.ts | 2 +- 6 files changed, 83 insertions(+), 38 deletions(-) diff --git a/src/components/LinePlot.tsx b/src/components/LinePlot.tsx index bf03358..7c4d97d 100644 --- a/src/components/LinePlot.tsx +++ b/src/components/LinePlot.tsx @@ -76,7 +76,7 @@ export default function LinePlot({ if (seriesData) { series = seriesData.flatMap((series, index) => { const name = series.name || "unknown"; - if (series.warnings) { + if (series.warnings && series.warnings.length > 0) { warnings[name] = series.warnings; } return [{ diff --git a/src/components/PlotWarnings.tsx b/src/components/PlotWarnings.tsx index ca7584b..9f44da6 100644 --- a/src/components/PlotWarnings.tsx +++ b/src/components/PlotWarnings.tsx @@ -8,7 +8,7 @@ interface Props { export default function PlotWarnings({warnings}: Props) { const keys = Object.keys(warnings); - if (keys.length == 0) { + if (keys.length === 0) { return null; } return ({ @@ -37,10 +36,11 @@ describe("", () => { biomarkers: ["ab", "ba"] }) }); - await act(() => render( + render( - )); + ); + await waitFor(() => expect(screen.getAllByText("PLOT").length).toBe(2)); expect(screen.getAllByTestId("sidebar").length).toBe(1); expect(screen.getAllByText("PLOT").length).toBe(2); }); @@ -74,12 +74,13 @@ describe("", () => { }) } }); - await act(() => render( + render( - )); + ); + await waitFor(() => expect(screen.getAllByText("PLOT").length).toBe(4)); expect(screen.getAllByTestId("sidebar").length).toBe(1); - expect(screen.getAllByText("PLOTPLOT").length).toBe(2); + expect(screen.getAllByText("PLOT").length).toBe(4); }); }); }); \ No newline at end of file diff --git a/test/components/LinePlot.test.tsx b/test/components/LinePlot.test.tsx index 6449b89..20d07e1 100644 --- a/test/components/LinePlot.test.tsx +++ b/test/components/LinePlot.test.tsx @@ -4,7 +4,7 @@ import { mockDatasetSettings, mockFailure, mockSuccess } from "../mocks"; -import {render, waitFor} from "@testing-library/react"; +import {render, screen, waitFor} from "@testing-library/react"; import { RootContext, RootDispatchContext @@ -15,7 +15,11 @@ import {DataSeries} from "../../src/generated"; import Plot from "react-plotly.js"; import Mock = jest.Mock; -jest.mock("react-plotly.js", () => jest.fn()); +// mock the react-plotly.js library +jest.mock("react-plotly.js", () => ({ + __esModule: true, + default: jest.fn(() => "PLOT"), +})); describe("", () => { @@ -34,7 +38,8 @@ describe("", () => { raw: { x: [1, 2], y: [3, 4] - } + }, + warnings: null }])); const dispatch = jest.fn(); @@ -58,10 +63,10 @@ describe("", () => { .toBe(1)); await waitFor(() => expect((Plot as Mock)) - .toBeCalledTimes(2)); + .toBeCalledTimes(1)); const plot = Plot as Mock - expect(plot.mock.calls[1][0].data).toEqual([ + expect(plot.mock.calls[0][0].data).toEqual([ { legendgroup: "all", line: { @@ -93,6 +98,49 @@ describe("", () => { }]) }); + test("displays warnings", async () => { + mockAxios.onGet(`/dataset/d1/trace/ab/?scale=natural&method=auto&span=0.75&k=10`) + .reply(200, mockSuccess([{ + name: "all", + model: { + x: [1.1, 2.2], + y: [3.3, 4.4] + }, + raw: { + x: [1, 2], + y: [3, 4] + }, + warnings: ["test warning"] + }])); + + const dispatch = jest.fn(); + const state = mockAppState({ + selectedDataset: "d1", + datasetMetadata: mockDatasetMetadata(), + datasetSettings: { + "d1": mockDatasetSettings() + } + }); + render( + + + + + ); + + await waitFor(() => expect(mockAxios.history.get.length) + .toBe(1)); + + await waitFor(() => expect((Plot as Mock)) + .toBeCalledTimes(1)); + + expect(screen.getByRole("alert")).toHaveClass("alert-warning"); + expect(screen.getByRole("alert").textContent) + .toBe("Some traces generated warningsall:test warning") + }); + test("requests data for given facet variables", async () => { mockAxios.onGet(`/dataset/d1/trace/ab/?filter=age%3A0%2Bsex%3AF&scale=natural&method=auto&span=0.75&k=10`) .reply(200, mockSuccess([{ @@ -104,7 +152,8 @@ describe("", () => { raw: { x: [1, 2], y: [3, 4] - } + }, + warnings: null }])); const dispatch = jest.fn(); @@ -128,10 +177,10 @@ describe("", () => { .toBe(1)); await waitFor(() => expect((Plot as Mock)) - .toBeCalledTimes(2)); + .toBeCalledTimes(1)); const plot = Plot as Mock - expect(plot.mock.calls[1][0].data).toEqual([ + expect(plot.mock.calls[0][0].data).toEqual([ { legendgroup: "all", line: { @@ -163,7 +212,7 @@ describe("", () => { }]) }); - test("clears plot data if request to API fails", async () => { + test("clears plot and renders error if request to API fails", async () => { mockAxios.onGet("/dataset/d1/trace/ab/?scale=natural&method=auto&span=0.75&k=10") .reply(200, mockSuccess([{ name: "all", @@ -174,12 +223,10 @@ describe("", () => { raw: { x: [1, 2], y: [3, 4] - } + }, + warnings: null }])); - mockAxios.onGet("/dataset/d1/trace/ab/?filter=sex%3AF&scale=natural&method=auto&span=0.75&k=10") - .reply(404, mockFailure("bad")); - const dispatch = jest.fn(); const state = mockAppState({ selectedDataset: "d1", @@ -199,11 +246,10 @@ describe("", () => { await waitFor(() => expect(mockAxios.history.get.length) .toBe(1)); - await waitFor(() => expect((Plot as Mock)) - .toBeCalledTimes(2)); + expect(screen.getAllByText("PLOT").length).toBe(1); - let plot = Plot as Mock - expect(plot.mock.calls[1][0].data.length).toBeGreaterThan(0); + mockAxios.onGet("/dataset/d1/trace/ab/?filter=sex%3AF&scale=natural&method=auto&span=0.75&k=10") + .reply(404, mockFailure("bad")); rerender( @@ -216,12 +262,9 @@ describe("", () => { await waitFor(() => expect(mockAxios.history.get.length) .toBe(2)); - await waitFor(() => expect((Plot as Mock)) - .toBeCalledTimes(4)); - - plot = Plot as Mock - expect(plot.mock.calls[2][0].data.length).toBe(2); - expect(plot.mock.calls[3][0].data.length).toBe(0); + expect(screen.queryAllByText("PLOT").length).toBe(0); + expect(screen.getByRole("alert").textContent) + .toBe("Facet for sex:F could not be generated due to the following error:bad "); }); }); diff --git a/test/mocks.ts b/test/mocks.ts index 8cf9379..5bc0f25 100644 --- a/test/mocks.ts +++ b/test/mocks.ts @@ -87,13 +87,14 @@ export function mockSeriesData(): DataSeries { return [{ name: "d1", model: { - x: [], - y: [] + x: [1, 2], + y: [1, 2] }, raw: { - x: [], - y: [] - } + x: [1, 2], + y: [1, 2] + }, + warnings: [] }] } diff --git a/test/services/utils.test.ts b/test/services/utils.test.ts index 388397e..05c35fd 100644 --- a/test/services/utils.test.ts +++ b/test/services/utils.test.ts @@ -16,7 +16,7 @@ describe("plotUtils", () => { } ] - const result = calculateFacets(facetVariables[0].levels, facetVariables[1].levels, facetVariables[2].levels); + const result = calculateFacets(facetVariables[0].levels as any, facetVariables[1].levels as any, facetVariables[2].levels as any); expect(result.length).toBe(8); expect(result[0]).toEqual(["F", "1", "ab"]) expect(result[1]).toEqual(["F", "1", "ba"]) From 68264980a888109c55bad415fb968165369be105 Mon Sep 17 00:00:00 2001 From: "alex.hill@gmail.com" Date: Tue, 17 Sep 2024 12:18:32 +0100 Subject: [PATCH 3/4] tests for plot error --- test/components/PlotError.test.tsx | 43 ++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 test/components/PlotError.test.tsx diff --git a/test/components/PlotError.test.tsx b/test/components/PlotError.test.tsx new file mode 100644 index 0000000..72421e2 --- /dev/null +++ b/test/components/PlotError.test.tsx @@ -0,0 +1,43 @@ +import React from "react"; +import {render, screen} from "@testing-library/react"; +import {mockError} from "../mocks"; +import {RootDispatchContext} from "../../src/RootContext"; +import PlotError from "../../src/components/PlotError"; + +describe("", () => { + + test("should display error detail if present", () => { + const error = mockError("custom message"); + const dispatch = jest.fn(); + render( + + ); + const alert = screen.getByRole("alert"); + expect(alert.textContent) + .toBe("Facet for nice-plot could not be generated due to the following error:custom message "); + }); + + test("should display error type if no detail present", () => { + const error = mockError(null as any); + const dispatch = jest.fn(); + render( + + ); + const alert = screen.getByRole("alert"); + expect(alert.textContent) + .toBe("Facet for nice-plot could not be generated due to the following error:API returned an error: OTHER_ERROR "); + }); + + test("should display home link if type is SESSION_EXPIRED", () => { + const error = mockError("Session expired.", "SESSION_EXPIRED"); + const dispatch = jest.fn(); + render( + + ); + const alert = screen.getByRole("alert"); + expect(alert.textContent) + .toBe("Facet for nice-plot could not be generated due to the following error:Session expired. Re-upload your data to continue."); + const link = screen.getByRole("link") as HTMLAnchorElement; + expect(link.href).toBe("http://localhost/"); + }); +}); From b5ed7230b6106a014c2d9d0c5e63de28ac5050b9 Mon Sep 17 00:00:00 2001 From: "alex.hill@gmail.com" Date: Tue, 17 Sep 2024 12:25:11 +0100 Subject: [PATCH 4/4] test warnings --- test/components/PlotWarnings.test.tsx | 34 +++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 test/components/PlotWarnings.test.tsx diff --git a/test/components/PlotWarnings.test.tsx b/test/components/PlotWarnings.test.tsx new file mode 100644 index 0000000..1ca6d94 --- /dev/null +++ b/test/components/PlotWarnings.test.tsx @@ -0,0 +1,34 @@ +import React from "react"; +import {render, screen} from "@testing-library/react"; +import {RootDispatchContext} from "../../src/RootContext"; +import PlotWarnings from "../../src/components/PlotWarnings"; + +describe("", () => { + + test("should display warnings for each trace", () => { + const warnings = { + "trace1": ["warning", "another warning"], + "trace2": ["something bad"] + } + const dispatch = jest.fn(); + render( + + ); + const alert = screen.getByRole("alert"); + expect(alert).toHaveClass("alert-warning"); + const lists = screen.getAllByRole("list"); + const firstTrace = lists[0] as HTMLUListElement; + expect(firstTrace.textContent).toBe("warninganother warning"); + + const secondTrace = lists[1] as HTMLUListElement; + expect(secondTrace.textContent).toBe("something bad"); + }); + + test("should return null if no warnings", () => { + const dispatch = jest.fn(); + const {container} = render( + + ); + expect(container).toBeEmptyDOMElement(); + }); +});