Issue 61 : Frontend - Add details in the datasource card #130

yaelleC · 2022-03-07T19:06:02Z

Fixes #61

Description

When listing datasources, a card is composed of a Heading ('Amazon Redshift'), a Figure (the logo) and some meta info. Meta include the datasource typeName, url and isDefault.

This PR uses the endpoint created by #129 to use a dropdown instead of an input field and populate the url field.

It also upgrades aws-sdk to use allowCustomValue in case the user does not have permission to fetch the clusters

On the managed Secret tab, the dropdown is hidden and a plain text field is shown. The reason for that is that since the dropdown gets its options populated dynamically, when selecting a secret it would not show the clusterID associated with it (as it's a disabled dropdown with no valid options)

Screenshots

No change to the managed secret UI
Dropdown instead of text input for Temp credential UI
Details added to card in datasources list view

Before	After

If the user does not have permission to fetch the clusters, she can create one	And the `AWS Secret Manager` view works as expected

Test coverage

Running: yarn test:coverage:change

-------------------|---------|----------|---------|---------|-------------------
File               | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
-------------------|---------|----------|---------|---------|-------------------
  ConfigEditor.tsx |   91.23 |    79.31 |   94.74 |      90 | 39-44,95-96 
-------------------|---------|----------|---------|---------|-------------------

src/ConfigEditor/ConfigEditor.test.tsx

yaelleC · 2022-03-07T19:07:52Z

src/ConfigEditor/ConfigEditor.tsx

-import React, { useState, useEffect, FormEvent } from 'react';
+import { ConfigSelect, ConnectionConfig, InlineInput } from '@grafana/aws-sdk';
 import { DataSourcePluginOptionsEditorProps, SelectableValue } from '@grafana/data';
+import { getBackendSrv } from '@grafana/runtime';
+import React, { FormEvent, useEffect, useState } from 'react';
+import { selectors } from 'selectors';
+
 import {
  RedshiftDataSourceOptions,
  RedshiftDataSourceSecureJsonData,
  RedshiftDataSourceSettings,
  RedshiftManagedSecret,
 } from '../types';
-import { InlineInput, ConfigSelect, ConnectionConfig } from '@grafana/aws-sdk';
-import { getBackendSrv } from '@grafana/runtime';
 import { AuthTypeSwitch } from './AuthTypeSwitch';
-import { selectors } from 'selectors';


auto re-org

yaelleC · 2022-03-07T19:13:26Z

src/ConfigEditor/ConfigEditor.tsx

+    return res.map((c) => ({
+      label: c.clusterIdentifier,
+      value: c.clusterIdentifier,
+      description: `${c.endpoint.address}:${c.endpoint.port}/${c.database}`,


this returns a database, should we make the input disable and fill automatically based on clusterId selected?

I don't think we should, that's the default database, it's possible to create a different one in the same cluster and use that one instead. I would also not display this URL in this selector as the description for the same reason.

Thanks!
So I should use the database entered in the db field to create the url in the datasource card instead of this default one?

yes, that would make sense

andresmgot

Thanks! Just a couple of minor comments here.

Also, would you able to create a PR for the SDK allowing to set other properties? (context) We can open a different issue for that if you don't have time now to do it atm.

andresmgot · 2022-03-08T08:19:05Z

src/ConfigEditor/ConfigEditor.tsx

+    return res.map((c) => ({
+      label: c.clusterIdentifier,
+      value: c.clusterIdentifier,
+      description: `${c.endpoint.address}:${c.endpoint.port}/${c.database}`,


I don't think we should, that's the default database, it's possible to create a different one in the same cluster and use that one instead. I would also not display this URL in this selector as the description for the same reason.

andresmgot · 2022-03-08T08:24:32Z

src/ConfigEditor/ConfigEditor.tsx

        data-testid={selectors.components.ConfigEditor.ClusterID.testID}
-        disabled={useManagedSecret}
+        saveOptions={saveOptions}
+        hidden={useManagedSecret}


we should still show this field as disabled when useManagedSecret

weird, when I have disabled={useManagedSecret} it does not disable the select.

When I inspect, I see that although ConfigSelect has disabled=true it's child ResourceSelector has disabled=false.

It seems to be because we override disable there: https://github.com/grafana/grafana-aws-sdk-react/blob/6a051089daf5c577297f4bf695909b4da0194f9f/src/sql/ConfigEditor/ConfigSelect.tsx#L47

I will fix that as part of the SDK PR 🙂

andresmgot · 2022-03-08T08:32:54Z

BTW, you need to adapt the e2e test (smoke.test.ts) to this change. See how it's done for e2eSelectors.ConfigEditor.ManagedSecret as an example. If you need any help running the e2e tests locally (or you have any question) let me know!

andresmgot

LGTM! but it would be better if we fix the bugs in the SDK before merging this PR (so main is still releaseable, just in case)

andresmgot · 2022-03-08T15:52:50Z

cypress/integration/smoke.spec.ts

              .click({ force: true })
              .type(datasource.jsonData.defaultRegion)
              .type('{enter}');
            e2eSelectors.ConfigEditor.ClusterID.testID()


you need to click first, wait for the clusters to load and then type here (see lines 113:115 for the example with secrets)

yaelleC · 2022-03-08T19:45:00Z

LGTM! but it would be better if we fix the bugs in the SDK before merging this PR (so main is still releaseable, just in case)

Agreed!
PR: grafana/grafana-aws-sdk-react#13

yaelleC · 2022-03-17T12:05:15Z

src/ConfigEditor/ConfigEditor.test.tsx

+    expect(screen.getByText(selectors.components.ConfigEditor.ManagedSecret.input)).not.toBeVisible();
+    expect(screen.getByTestId(selectors.components.ConfigEditor.ClusterIDText.testID)).not.toBeVisible();
+    expect(screen.getByTestId(selectors.components.ConfigEditor.ClusterID.testID)).toBeVisible();
+    expect(screen.getByTestId(selectors.components.ConfigEditor.ClusterID.testID)).not.toBeDisabled();
+    expect(screen.getByText(selectors.components.ConfigEditor.DatabaseUser.input)).not.toBeDisabled();
+    expect(screen.getByText(selectors.components.ConfigEditor.Database.input)).not.toBeDisabled();
  });

  it('should switch to use the Secret Manager', () => {
    render(<ConfigEditor {...props} />);
    screen.getByText('AWS Secrets Manager').click();
-    expect(screen.getByText(selectors.components.ConfigEditor.ManagedSecret.input)).toBeInTheDocument();
-    expect(screen.getByText(selectors.components.ConfigEditor.ClusterID.input)).toBeInTheDocument();
-    expect(screen.getByText(selectors.components.ConfigEditor.Database.input)).toBeInTheDocument();
+    expect(screen.getByText(selectors.components.ConfigEditor.ManagedSecret.input)).toBeVisible();
+    expect(screen.getByTestId(selectors.components.ConfigEditor.ClusterIDText.testID)).toBeVisible();
+    expect(screen.getByTestId(selectors.components.ConfigEditor.ClusterIDText.testID)).toBeDisabled();
+    expect(screen.getByTestId(selectors.components.ConfigEditor.ClusterID.testID)).not.toBeVisible();
+    expect(screen.getByTestId(selectors.components.ConfigEditor.DatabaseUser.testID)).toBeDisabled();
+    expect(screen.getByText(selectors.components.ConfigEditor.Database.input)).not.toBeDisabled();


These tests would always pass as even hidden or disabled components will be in the document 😄

I've made them stricter to check which inputs are disabled and which are visible

yaelleC · 2022-03-17T12:06:38Z

src/ConfigEditor/ConfigEditor.tsx

+  const getClusterUrl = async (clusterID: string) => {
+    const { jsonData } = props.options;
+    if (clusterID != jsonData.clusterIdentifier) {
+      const clusters = await fetchClusters();
+      return `${clusters.find((c) => c.value === clusterID)?.description || clusterID}/${jsonData.database || ''}`;
+    }
+    return props.options.url;
+  };


I tried to use memoize here to avoid fetching unnecessary clusters (if clusterID is still the same) but I could not get it to work so I added the memo check myself 😉

yaelleC · 2022-03-17T12:07:09Z

src/ConfigEditor/ConfigEditor.tsx

        hidden={!useManagedSecret}
      />
-      <InlineInput
+      <ConfigSelect


A dropdown for better use experience in temporary Creds

yaelleC · 2022-03-17T12:07:29Z

src/ConfigEditor/ConfigEditor.tsx

+        saveOptions={saveOptions}
+        hidden={useManagedSecret}
+      />
+      <InlineInput


A text input to display clusterID for managed Secret

yaelleC · 2022-03-17T12:26:59Z

cypress/integration/smoke.spec.ts

            e2eSelectors.ConfigEditor.ManagedSecret.input().click({ force: true });
            // wait for it to load
-            e2eSelectors.ConfigEditor.ManagedSecret.testID().contains(datasource.jsonData.managedSecret.name);
+            // e2eSelectors.ConfigEditor.ManagedSecret.testID().contains(datasource.jsonData.managedSecret.name);


any idea why this line fails? 🤔
I figured we don't need it as the next few ones types in the expected secret and selects it but leaving as commented out for visibility

mmh, it should work, if not we are risking that the options loads after start typing the name so it messes it up. Are you able to reproduce the error locally? If not it could be the case that the CI account doesn't have enough permissions after the last changes(?).

It's managedSecret, not clusters so permissions should not be an issue 🤷‍♀️
I can replicate locally, it is set up exactly the same way as the clusterID dropdown (which passes) - some puzzling behaviour! 🤔

mmh, weird, anyway it seems stable now

andresmgot

Some comments but mostly minor. Good job!

andresmgot · 2022-03-17T12:33:53Z

src/ConfigEditor/ConfigEditor.tsx

+      <InlineInput
+        {...props}
+        value={props.options.jsonData.clusterIdentifier ?? ''}
+        onChange={() => {}}


hehe we should make this property optional (not needed now)

Added issue: grafana/grafana-aws-sdk-react#15

andresmgot · 2022-03-17T12:34:19Z

src/ConfigEditor/ConfigEditor.tsx

+        onChange={() => {}}
+        label={selectors.components.ConfigEditor.ClusterIDText.input}
+        data-testid={selectors.components.ConfigEditor.ClusterIDText.testID}
        disabled={useManagedSecret}


nit: disabled={true} since it will always be disabled

Well spotted 😄

andresmgot · 2022-03-17T12:49:26Z

src/ConfigEditor/ConfigEditor.test.tsx

+    expect(onChange).toHaveBeenCalledTimes(1);
+    expect(onChange).toHaveBeenCalledWith({
+      ...props.options,
+      jsonData: { ...props.options.jsonData, database: 'abcd' },


can you also cover the case in which the url is set here? I believe it's not modifying it in this test because the url was empty before

andresmgot · 2022-03-17T13:04:48Z

src/ConfigEditor/ConfigEditor.tsx

-            clusterIdentifier: s.dbClusterIdentifier,
-            dbUser: s.username,
-          },
+        getClusterUrl(s.dbClusterIdentifier).then((url) => {


We can add a ".catch" for the getClusterUrl so even if the user doesn't have permissions to get clusters, we set the identifier and the db user as we did before.

Added inside fetchClusters to return an empty array in catch

andresmgot · 2022-03-17T13:06:00Z

src/ConfigEditor/ConfigEditor.tsx

+  };
  const onChange = (resource: InputResourceType) => (e: FormEvent<HTMLInputElement>) => {
    const value = e.currentTarget.value;
+    const url = resource === 'database' ? props.options.url.replace(/\/.*$/, `/${value}`) : props.options.url;


this regex is a bit risky because it assumes that the URL will contain one and only one /. One option would be to store the current cluster URL (without the database) in the state, so this URL can be re-calculated on every change but up to you since it's not that critical.

yes, I agree, I could not find a workaround but using state seems like a good idea, I'll try that

andresmgot · 2022-03-17T13:09:22Z

cypress/integration/smoke.spec.ts

            e2eSelectors.ConfigEditor.ManagedSecret.input().click({ force: true });
            // wait for it to load
-            e2eSelectors.ConfigEditor.ManagedSecret.testID().contains(datasource.jsonData.managedSecret.name);
+            // e2eSelectors.ConfigEditor.ManagedSecret.testID().contains(datasource.jsonData.managedSecret.name);


mmh, it should work, if not we are risking that the options loads after start typing the name so it messes it up. Are you able to reproduce the error locally? If not it could be the case that the CI account doesn't have enough permissions after the last changes(?).

…er ¯\_(ツ)_/¯

andresmgot

👍 👍

yaelleC added 2 commits March 7, 2022 18:28

Using a dropdown instead of input for clusterID

b902347

Added tests

3b00e14

yaelleC requested a review from a team as a code owner March 7, 2022 19:06

yaelleC requested review from andresmgot and fridgepoet and removed request for a team March 7, 2022 19:06

yaelleC commented Mar 7, 2022

View reviewed changes

src/ConfigEditor/ConfigEditor.test.tsx Outdated Show resolved Hide resolved

yaelleC commented Mar 7, 2022

View reviewed changes

andresmgot reviewed Mar 8, 2022

View reviewed changes

yaelleC added 2 commits March 8, 2022 14:47

use db from text input instead of default cluster db

72124fb

update e2e test

d204eda

andresmgot approved these changes Mar 8, 2022

View reviewed changes

yaelleC added 2 commits March 8, 2022 20:41

wait for loading of clusterIDs

9ddfde6

uncomment

c86a2bd

yaelleC mentioned this pull request Mar 8, 2022

ConfigSelect : Fixed disabled prop overriden and added prop to allow custom input grafana/grafana-aws-sdk-react#13

Merged

yaelleC added 6 commits March 11, 2022 09:39

bump aws-sky version and use new prop

80c5b4b

Updated yarn lock

d278234

Updated db in url onChange

06e46e9

Added fetching cluster info on managed secret

7ce3fe8

Removed 'text' from cluster ID label in managed secrets

d58ab15

Fixed unit test for temp credential and secret manager

48430f9

yaelleC commented Mar 17, 2022

View reviewed changes

yaelleC added 2 commits March 17, 2022 13:10

fix linting errors

815a10d

fix linting errors

b93b4a0

yaelleC commented Mar 17, 2022

View reviewed changes

andresmgot approved these changes Mar 17, 2022

View reviewed changes

yaelleC added 2 commits March 17, 2022 22:12

Addressed comments

4f0a6e3

Work around failing e2e test: start typing wait for text then hit ent…

0169767

…er ¯\_(ツ)_/¯

andresmgot approved these changes Mar 21, 2022

View reviewed changes

yaelleC merged commit 01ca096 into main Mar 21, 2022

yaelleC deleted the yaelle_61_clusterID-in-dropdown branch March 21, 2022 09:04

fridgepoet mentioned this pull request Mar 24, 2022

Release 1.0.4 #134

Merged

Issue 61 : Frontend - Add details in the datasource card #130

Issue 61 : Frontend - Add details in the datasource card #130

Uh oh!

Conversation

yaelleC commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Screenshots

Test coverage

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andresmgot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andresmgot commented Mar 8, 2022

Uh oh!

andresmgot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaelleC commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaelleC Mar 17, 2022 • edited by andresmgot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andresmgot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andresmgot Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

yaelleC commented Mar 7, 2022 •

edited

Loading

yaelleC commented Mar 8, 2022 •

edited

Loading

yaelleC Mar 17, 2022 •

edited by andresmgot

Loading

andresmgot Mar 17, 2022 •

edited

Loading