Skip to content

Commit

Permalink
Address review feedback and some code clean up.
Browse files Browse the repository at this point in the history
  • Loading branch information
tdilauro committed Aug 23, 2024
1 parent 47d8419 commit fca9653
Show file tree
Hide file tree
Showing 8 changed files with 50 additions and 69 deletions.
13 changes: 9 additions & 4 deletions src/components/ContextProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,20 @@ import * as React from "react";
import { Store } from "@reduxjs/toolkit";
import * as PropTypes from "prop-types";
import buildStore, { RootState } from "../store";
import { FeatureFlags, PathFor, TestingFlags } from "../interfaces";
import {
AdminRoleData,
DashboardCollectionsBarChart,
FeatureFlags,
PathFor,
} from "../interfaces";
import Admin from "../models/Admin";
import PathForProvider from "@thepalaceproject/web-opds-client/lib/components/context/PathForContext";
import ActionCreator from "../actions";
import AppContextProvider, { AppContextType } from "../context/appContext";

// Note: Not all elements of these props make it into the `ContextProvider`.
// Some are exposed only through the `AppContextProvider` component (which
// this component wraps.
// this component wraps).
// TODO: We should get this interface to the point where we can just extend
// the `ConfigurationSettings` interface.
export interface ContextProviderProps extends React.Props<ContextProvider> {
Expand All @@ -25,7 +30,7 @@ export interface ContextProviderProps extends React.Props<ContextProvider> {
}[];
featureFlags: FeatureFlags;
quicksightPagePath?: string;
testingFlags?: TestingFlags;
dashboardCollectionsBarChart?: DashboardCollectionsBarChart;
}

/** Provides a redux store, configuration options, and a function to create URLs
Expand Down Expand Up @@ -110,7 +115,7 @@ export default class ContextProvider extends React.Component<
admin: this.admin,
featureFlags: this.props.featureFlags,
quicksightPagePath: this.props.quicksightPagePath,
testingFlags: this.props.testingFlags,
dashboardCollectionsBarChart: this.props.dashboardCollectionsBarChart,
};
return (
<PathForProvider pathFor={this.pathFor}>
Expand Down
2 changes: 1 addition & 1 deletion src/components/LibraryStats.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export const inventoryKeyToLabelMap = {
export const ALL_LIBRARIES_HEADING = "Dashboard for All Authorized Libraries";

/** Displays statistics about patrons, licenses, and collections from the server,
for a single library or all libraries the admin has access to. */
for a single library or all libraries to which the admin has access. */
const LibraryStats = ({ stats, library }: LibraryStatsProps) => {
const {
name: libraryName,
Expand Down
46 changes: 21 additions & 25 deletions src/components/StatsCollectionsBarChart.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import * as React from "react";
import { useTestingFlagEnabled } from "../context/appContext";
import { useDashboardCollectionsBarChartSettings } from "../context/appContext";
import { CollectionInventory } from "../interfaces";
import { ValueType } from "recharts/types/component/DefaultTooltipContent";
import {
Bar,
BarChart,
ResponsiveContainer as RechartsResponsiveContainer,
ResponsiveContainer,
Tooltip,
TooltipProps,
XAxis,
Expand All @@ -14,14 +14,11 @@ import {
import { inventoryKeyToLabelMap } from "./LibraryStats";
import { formatNumber } from "../utils/sharedFunctions";

// These values are needed for testing, in some cases.
export const COLLECTION_BAR_CHART_TEST_FLAG_KEY =
"COLLECTION_BAR_CHART_TEST_FLAG_KEY";
const TestResponsiveContainer = ({ children }) => (
<RechartsResponsiveContainer width={800} height={800}>
{children}
</RechartsResponsiveContainer>
);
const stackId = "collections";
const barSize = 50;
const meteredColor = "#606060";
const unlimitedColor = "#404040";
const openAccessColor = "#202020";

type Props = {
collections: CollectionInventory[];
Expand All @@ -36,14 +33,13 @@ const StatsCollectionsBarChart = ({ collections }: Props) => {
_by_medium: inventoryByMedium || {},
}))
.sort((a, b) => (a.name.toLowerCase() > b.name.toLowerCase() ? 1 : -1));
const chartWidth = useDashboardCollectionsBarChartSettings()?.width || "100%";

const ResponsiveContainer = useTestingFlagEnabled(
COLLECTION_BAR_CHART_TEST_FLAG_KEY
)
? TestResponsiveContainer
: RechartsResponsiveContainer;
return (
<ResponsiveContainer height={chartItems.length * 100 + 75} width="100%">
<ResponsiveContainer
height={chartItems.length * 100 + 75}
width={chartWidth}
>
<BarChart
data={chartItems}
layout="vertical"
Expand All @@ -61,25 +57,25 @@ const StatsCollectionsBarChart = ({ collections }: Props) => {
<XAxis type="number" />
<Tooltip content={<CustomTooltip />} />
<Bar
stackId="collections"
stackId={stackId}
name={inventoryKeyToLabelMap.meteredLicenseTitles}
dataKey={"meteredLicenseTitles"}
barSize={50}
fill="#606060"
barSize={barSize}
fill={meteredColor}
/>
<Bar
stackId="collections"
stackId={stackId}
name={inventoryKeyToLabelMap.unlimitedLicenseTitles}
dataKey={"unlimitedLicenseTitles"}
barSize={50}
fill="#404040"
barSize={barSize}
fill={unlimitedColor}
/>
<Bar
stackId="collections"
stackId={stackId}
name={inventoryKeyToLabelMap.openAccessTitles}
dataKey={"openAccessTitles"}
barSize={50}
fill="#202020"
barSize={barSize}
fill={openAccessColor}
/>
</BarChart>
</ResponsiveContainer>
Expand Down
2 changes: 0 additions & 2 deletions src/components/StatsUsageReportsGroup.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
import React = require("react");
import { Button } from "library-simplified-reusable-components";
import StatsGroup from "./StatsGroup";
import { InventoryStatistics } from "../interfaces";
import InventoryReportRequestModal from "./InventoryReportRequestModal";
import { useState } from "react";
import { useAppContext } from "../context/appContext";

type Props = {
heading?: string;
Expand Down
20 changes: 7 additions & 13 deletions src/context/appContext.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { createContext, useContext } from "react";
import { FeatureFlags, TestingFlags } from "../interfaces";
import { DashboardCollectionsBarChart, FeatureFlags } from "../interfaces";
import Admin from "../models/Admin";

export type AppContextType = {
Expand All @@ -8,8 +8,7 @@ export type AppContextType = {
admin: Admin;
featureFlags: FeatureFlags;
quicksightPagePath: string;
// These flags are used only during testing and are discouraged wherever possible.
testingFlags?: { [key: string]: boolean };
dashboardCollectionsBarChart?: DashboardCollectionsBarChart;
};

// Don't export this, since we always want the error handling behavior of our hook.
Expand All @@ -18,23 +17,18 @@ const AppContext = createContext<AppContextType | undefined>(undefined);
export const useAppContext = (): AppContextType => {
const context = useContext(AppContext);
if (context === undefined) {
throw new Error("useAppContext must be used within an AppContext povider.");
throw new Error(
"useAppContext must be used within an AppContext provider."
);
}
return context;
};

const flagEnabled = (flags: TestingFlags, flagName: string) => {
// A flag must be affirmatively set and `true` to be considered "enabled".
return !!flags?.[flagName];
};
export const useTestingFlagEnabled = (flagName: string) => {
const testingFlags = useAppContext().testingFlags;
return flagEnabled(testingFlags, flagName);
};

export const useCsrfToken = () => useAppContext().csrfToken;
export const useAppAdmin = () => useAppContext().admin;
export const useAppEmail = () => useAppAdmin().email;
export const useAppFeatureFlags = () => useAppContext().featureFlags;
export const useDashboardCollectionsBarChartSettings = () =>
useAppContext().dashboardCollectionsBarChart;

export default AppContext.Provider;
9 changes: 6 additions & 3 deletions src/interfaces.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,12 @@ export interface ConfigurationSettings {
Currently, this value does not change, so we can share it via fixed config. */
quicksightPagePath: string;

/** These flags are used only during testing and are discouraged wherever
possible. */
testingFlags?: TestingFlags;
/** Configuration for dashboard collections barchart. */
dashboardCollectionsBarChart?: DashboardCollectionsBarChart;
}

export interface DashboardCollectionsBarChart {
width?: number;
}

export interface TestingFlags {
Expand Down
23 changes: 4 additions & 19 deletions tests/jest/components/Stats.test.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
import * as React from "react";
import { prettyDOM, render, within } from "@testing-library/react";
import LibraryStats, {
ALL_LIBRARIES_HEADING,
} from "../../../src/components/LibraryStats";
import StatsCollectionsBarChart, {
COLLECTION_BAR_CHART_TEST_FLAG_KEY,
CustomTooltip,
} from "../../../src/components/StatsCollectionsBarChart";
import { render } from "@testing-library/react";
import { ALL_LIBRARIES_HEADING } from "../../../src/components/LibraryStats";
import { CustomTooltip } from "../../../src/components/StatsCollectionsBarChart";
import {
componentWithProviders,
renderWithProviders,
Expand All @@ -15,9 +10,7 @@ import { ContextProviderProps } from "../../../src/components/ContextProvider";

import {
statisticsApiResponseData,
testLibraryKey,
testLibraryKey as sampleLibraryKey,
testLibraryName,
testLibraryName as sampleLibraryName,
} from "../../__data__/statisticsApiResponseData";

Expand All @@ -32,12 +25,6 @@ import { store } from "../../../src/store";
import { api } from "../../../src/features/api/apiSlice";

const normalizedData = normalizeStatistics(statisticsApiResponseData);
const librariesStatsTestDataByKey = normalizedData.libraries.reduce(
(map, library) => ({ ...map, [library.key]: library }),
{}
);
const sampleLibraryCollections =
librariesStatsTestDataByKey[sampleLibraryKey].collections;

global.ResizeObserver = require("resize-observer-polyfill");

Expand Down Expand Up @@ -320,9 +307,7 @@ describe("Dashboard Statistics", () => {
it("tests BarChart component", () => {
const contextProviderProps: Partial<ContextProviderProps> = {
roles: systemAdmin,
// This flag is needed to force Recharts Responsive container to
// render its children in a manner that allows these tests.
testingFlags: { [COLLECTION_BAR_CHART_TEST_FLAG_KEY]: true },
dashboardCollectionsBarChart: { width: 800 },
};
const { container, getByRole } = renderWithProviders(
<Stats library={sampleLibraryKey} />,
Expand Down
4 changes: 2 additions & 2 deletions tests/jest/testUtils/withProviders.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ const defaultReduxStore = store;

// The `csrfToken` context provider prop is required, so we provide
// a default value here, so it can be easily merged with other props.
const defaultContextProviderProps: ContextProviderProps = {
const requiredContextProviderProps: ContextProviderProps = {
csrfToken: "",
featureFlags: defaultFeatureFlags,
};
Expand All @@ -49,7 +49,7 @@ export const componentWithProviders = ({
queryClient = new QueryClient(),
}: TestProviderWrapperOptions = {}): React.FunctionComponent => {
const effectiveContextProviderProps = {
...defaultContextProviderProps,
...requiredContextProviderProps,
...contextProviderProps,
...reduxProviderProps.store, // Context and Redux Provider stores must match.
};
Expand Down

0 comments on commit fca9653

Please sign in to comment.