Add quicksight dashboard #97

dbernstein · 2023-12-02T07:16:10Z

Description

nvm install 18
nvm use 18
npm run dev-server -- --env=backend=https://minotaur.dev.palaceproject.io

Once you've logged in, to see the dashboard you need to go to:

http://localhost:8080/admin/web/quicksight

The URL is not exposed in the App (since I don't think it is quite ready for prime time).

Motivation and Context

https://ebce-lyrasis.atlassian.net/browse/PP-517

How Has This Been Tested?

Added unit test.
Tested it manually.

Checklist:

I have updated the documentation accordingly.
All new and existing tests passed.

tdilauro

I wasn't entirely sure if this PR was ready for review yet, but I have a few comments/suggestions that I hope can be helpful in any case.

src/components/QuicksightDashboardPage.tsx

tdilauro · 2024-01-08T03:31:29Z

src/index.tsx

+              <Route
+                  path="/admin/web/dashboard/quicksight(/:library)"
+                  component={QuicksightDashboardPage}
+              />


I believe this dashboard is meant to be displayed as an alternative to the current "snapshot" dashboard via a toggle on the DashboardPage, so we shouldn't need a QuicksightDashboardPage or its associated route.

Okay - I'll fix that.

tdilauro · 2024-01-08T03:47:54Z

tests/jest/components/QuicksightDashboard.test.tsx

+  const server = setupServer(
+      rest.get("*/admin/libraries", (req, res, ctx) => res(ctx.json(libraries))),
+
+      rest.get("*/admin/quicksight_embed/" + dashboardId + "?libraryUuids=" + libraries["libraries"][0]["uuid"], (req, res, ctx) => res(ctx.json(dashboardUrlData)))


For msw `setupServer, we should setup rest endpoint routes. rather than full URLs, so that we're matching the path part, but not the query params. We can get at the query params with something like...

const libraryUuids = req.url.searchParams.get("libraryUuids");

... and use them to adjust the response as needed (e.g., to distinguish "good" or "bad" requests.

tdilauro · 2024-01-08T03:50:02Z

src/actions.ts

@@ -1095,4 +1097,12 @@ export default class ActionCreator extends BaseActionCreator {
      value,
    };
  }
+
+  fetchDashboardUri(dashboardId: string) {
+    const url = "/admin/quicksight_embed/" + dashboardId;


We can use string interpolation here:

const url = `/admin/quicksight_embed/${dashboardId}`;

Of course, you would need to append the libraryUuids query param, as well.

tdilauro · 2024-01-08T03:52:11Z

src/actions.ts

@@ -1095,4 +1097,12 @@ export default class ActionCreator extends BaseActionCreator {
      value,
    };
  }
+
+  fetchDashboardUri(dashboardId: string) {


A few things here:

We need a reducer for this action (note also that other server fetch-related actions have additional state constants (for loading, success, etc.) above for use with the associated reducer).

Need tests for this action in ./src/__tests__/actions-test.ts.

Since we already have a Dashboard in the app, we should probably call this something different, like fetchQuicksiteUri, to avoid confusion. What is currently the ActionCreator.DASHBOARD_URI constant above (and associated state constants) should match this.

We will probably need to pass in the library UUIDs here, as well, so that you can use them to form the complete URL.

If we plan to fetch the QS embed names, we'll probably want an additional action.

tdilauro · 2024-01-08T03:55:35Z

src/components/QuicksightDashboard.tsx

+        fetch(
+          "/admin/quicksight_embed/" +
+            this.props.dashboardId +
+            "?libraryUuids=" +
+            library_uuids
+        )
+          .then((response) => response.json())
+          .then((data) => this.setState({ embedUrl: data["embedUrl"] }))
+          .catch((error) => {
+            console.error(error);
+          });


Usually with Redux we would dispatch a Redux action to perform this fetch elsewhere (and it looks like we have added an action for this). Any reason why we need to code the fetch here?

I changed this to dispatch to an action, but I don't think it is exactly right yet. I would like to get this out since this is really just a rough pass.

tdilauro · 2024-01-08T03:58:53Z

src/components/QuicksightDashboard.tsx

+  embedUrl: string;
+}
+
+class QuicksightDashboard extends React.Component<


Can we implement this as a functional component with hooks, rather than as a higher order connected class component? The former is the current recommendation when developing new components. The latter requires a lot more boilerplate (and obfuscation), especially with Redux connected HoCs.

Yes - will rework this once we get some feedback.

This is not critical for now. We already have Redux connected HoCs, so continuing to use them is fine.

tests/jest/components/QuicksightDashboard.test.tsx

tdilauro · 2024-01-08T13:59:48Z

tests/jest/components/QuicksightDashboard.test.tsx

We should test some non-happy paths here, as well:

What happens when there is an error in one or more of the requested UUIDs?

What happens when a requested UUID doesn't exist (same as above or does QS ignore)?

... maybe some others -- I don't understand the QS dashboard flow as well as I'd like.

tdilauro · 2024-01-08T14:36:08Z

src/components/QuicksightDashboard.tsx

+          "/admin/quicksight_embed/" +
+            this.props.dashboardId +
+            "?libraryUuids=" +
+            library_uuids
+        )


We can use string interpolation here, if we don't remove this fetch in favor of a dispatched Redux action.

…tats.

…to fit the container.

tdilauro · 2024-01-23T17:34:07Z

src/components/QuicksightDashboard.tsx

+  embedUrl: string;
+}
+
+class QuicksightDashboard extends React.Component<


This is not critical for now. We already have Redux connected HoCs, so continuing to use them is fine.

tdilauro · 2024-01-23T18:01:28Z

src/components/QuicksightDashboard.tsx

+    // TODO:  For whatever reason, the "this.props.libraries" variable was not being
+    // set when I tried to put this logic in the render method (nor did it work trying to access it in this method).


It doesn't look like libraries is being passed as a prop from QuicksightDashboardPage l.46, so that's probably why we don't see it here. We are passing dashboardId there, but not library.

Right - "libraries" is the list of libraries that is being pulled from the store - at least I'd like to be pulling it from the store. But it doesn't get loaded. I removed the reference to "library" for now, because of the way the QS filtering is working: it's always returning the list of libraries the user has access to. So in the library QS view, users can look across all libraries, any group of libraries or a single library.

tdilauro · 2024-01-23T18:15:52Z

src/components/QuicksightDashboardPage.tsx

+}
+
+/** Page holds quicksight dashboards. */
+export default class DashboardPage extends React.Component<QuicksightDashboardPageProps> {


We should use QuicksightDashboardPage -- rather than DashboardPage -- here, since it could be confusing. DashboardPage is used in the DashboardPage component, so using it here could be confusing.

Good point I'll fix that.

tdilauro · 2024-01-23T18:29:08Z

src/index.tsx

@@ -87,6 +88,10 @@ class CirculationAdmin {
                path="/admin/web/dashboard(/:library)"
                component={DashboardPage}
              />
+              <Route
+                path="/admin/web/quicksight(/:library)"


Not sure why we're passing the library here, since it is not currently used to select which libraryUuids are selected for inclusion.

Right - I'll remove that for now. Oversight.

src/components/QuicksightDashboardPage.tsx

tdilauro · 2024-01-23T18:33:28Z

src/components/QuicksightDashboardPage.tsx

+      <div className="quicksight-dashboard">
+        <Header />
+        <main className="body">
+          <QuicksightDashboard dashboardId="library"/>


This is where we can pass the library prop into the QuicksightDashboard component.

For the demo, we'll leave the library prop off so the user can select the libraries they have access to when viewing the report.

dbernstein · 2024-01-23T18:51:06Z

@tdilauro : updates are in.

tdilauro

I added a few follow-on comments related to the most recent changes.

Also a reminder about the hardcoded dashboardId and follow-on changes that would be needed if it were removed.

tdilauro · 2024-01-23T19:21:08Z

src/components/QuicksightDashboardPage.tsx

+export interface QuicksightDashboardPageProps
+  extends React.Props<QuicksightDashboardPageProps> {
+  params: {
+    library?: string;
+  };
+}


Since we're no longer passing a library param here, we no longer need the QuicksightDashboardPageProps interface, so we should get rid of this.

We'll also need to remove it from the class definition below.

We're not passing it now, but we may want to pass it later. Ie if we want to pre-select the library. To make this work, we'll need to make some changes on the backend. But I'm thinking we may want to leave this param in here for the time being.

tdilauro · 2024-01-23T19:23:00Z

src/components/QuicksightDashboardPage.tsx

+export default class QuicksightDashboardPage extends React.Component<
+  QuicksightDashboardPageProps
+> {


Since we no longer need QuicksightDashboardPageProps, we can change the props type here to none, like this:

export default class QuicksightDashboardPage extends React.Component<never> { ...

tdilauro · 2024-01-23T19:24:07Z

src/components/QuicksightDashboardPage.tsx

+      <div className="quicksight-dashboard">
+        <Header />
+        <main className="body">
+          <QuicksightDashboard dashboardId="library" />


Again, we don't need to pass this hardcoded dashboarId prop here. We can just use the hardcoded value directly in the child component.

I can remove this - but it was my assumption that the admin interface may provide more than one dashboard depending on the access level of the user.

tdilauro · 2024-01-23T19:27:31Z

src/components/QuicksightDashboard.tsx

+
+export interface QuicksightDashboardOwnProps {
+  store?: Store<RootState>;
+  dashboardId?: string;


We won't need dashboardId as a prop here, assuming that we stop passing the hardcoded value from the parent page.

tdilauro · 2024-01-23T19:30:06Z

src/components/QuicksightDashboard.tsx

+    this.props.fetchLibraries().then((libs) => {
+      try {
+        this.props
+          .fetchQuicksightEmbedUrl(this.props.dashboardId, libs)


Assuming that we stop passing the hardcoded value from the parent page, we should replace this.props.dashboardId with a new constant in this module.

We may offer other dashboards down the road depending on the user's permissions. However for now, I suppose I can make this a constant.

tdilauro · 2024-01-23T19:42:16Z

src/actions.ts

@@ -190,6 +190,7 @@ export default class ActionCreator extends BaseActionCreator {
  static readonly RESET_ADOBE_ID = "RESET_ADOBE_ID";

  static readonly DIAGNOSTICS = "DIAGNOSTICS";
+  static readonly QUICKSIGHT_EMBEDDED_URL: "QUICKSIGHT_EMBEDDED_URL";


I think I missed this before, but the value of this constant needs to assigned with and equal sign.

static readonly QUICKSIGHT_EMBEDDED_URL = "QUICKSIGHT_EMBEDDED_URL";

This might be why we were running into trouble using Redux actions dispatch. I also note that the various state constants for the QUICKSIGHT_EMBEDDED_URL action are not defined, which would also prevent actions dispatch from working properly. There are assumptions/naming conventions for these on which fetchJSON depends. A good template for that would be the STATS action:

... static readonly STATS = "STATS"; ... static readonly STATS_REQUEST = "STATS_REQUEST"; static readonly STATS_SUCCESS = "STATS_SUCCESS"; static readonly STATS_FAILURE = "STATS_FAILURE"; static readonly STATS_LOAD = "STATS_LOAD"; ...

Good catch.

I'm understanding what you did with the Stats piece now a little better. I'd like to get this out there for front-end feedback, but I will plan to rework the fetching using RTK before we goes live.

tdilauro

This looks good for now and doesn't appear to break functionality elsewhere in the app.

dbernstein force-pushed the add-quicksight-dashboard branch 2 times, most recently from ca15ea1 to c042fc3 Compare December 11, 2023 22:23

dbernstein force-pushed the add-quicksight-dashboard branch from c042fc3 to 06f4d42 Compare December 12, 2023 01:09

dbernstein marked this pull request as ready for review December 22, 2023 20:53

tdilauro reviewed Jan 8, 2024

View reviewed changes

dbernstein added 8 commits January 19, 2024 17:01

Towards quicksight dashboard.

accbfcf

Adds quicksight library dashboard to the Dashboard screen under the s…

f857639

…tats.

Add /admin/dashboard/quicksight endpoint. Ensure iframe is expanding …

3c7f306

…to fit the container.

Add jest test

80af5f6

Use string interpolation.

3116880

Fix linting issues.

c5dba40

Add new line at end of file.

9589a48

Add TODO and other comments.

ac37ef2

dbernstein force-pushed the add-quicksight-dashboard branch from 7de047b to ac37ef2 Compare January 20, 2024 01:02

dbernstein added 4 commits January 19, 2024 17:05

Fix format.

929afc2

Rename components and clean up unused imports.

1fa3428

Change quicksight dashboard path to /admin/quicksight

bd998fc

Fix test.

46862ed

tdilauro reviewed Jan 23, 2024

View reviewed changes

dbernstein added 2 commits January 23, 2024 10:44

Remove unused library param from quicksight route.

88a2722

Rename DashboardPage to QuicksightDashboardPage

b77601a

tdilauro reviewed Jan 23, 2024

View reviewed changes

Use = for assignment.

bcfaaea

tdilauro approved these changes Jan 24, 2024

View reviewed changes

dbernstein merged commit 2f3f3fa into main Jan 24, 2024
1 check passed

dbernstein deleted the add-quicksight-dashboard branch January 24, 2024 18:55

		// TODO: For whatever reason, the "this.props.libraries" variable was not being
		// set when I tried to put this logic in the render method (nor did it work trying to access it in this method).

Add quicksight dashboard #97

Add quicksight dashboard #97

Conversation

dbernstein commented Dec 2, 2023 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Checklist:

tdilauro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbernstein commented Jan 23, 2024

tdilauro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdilauro left a comment

Choose a reason for hiding this comment

dbernstein commented Dec 2, 2023 •

edited

Loading