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

yaelleC · 2022-02-25T16:28:05Z

Frontend work for #61
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 #121 to populate the url field

Screenshots

Before	After

Test coverage

Running: yarn test:coverage:change

-------------------|---------|----------|---------|---------|-------------------
File               | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
-------------------|---------|----------|---------|---------|-------------------
All files          |   88.73 |    84.85 |   87.50 |   88.19 |                   
  ConfigEditor.tsx |   91.23 |    84.09 |   94.74 |    90.2 | 38-43,103-104     
-------------------|---------|----------|---------|---------|-------------------

yaelleC · 2022-03-01T08:25:46Z

src/ConfigEditor/ConfigEditor.test.tsx

+import { fireEvent, render, screen, waitFor } from '@testing-library/react';
 import React from 'react';
-import { render, screen, waitFor } from '@testing-library/react';
-import { ConfigEditor } from './ConfigEditor';
-import { selectors } from '../selectors';
-import { mockDatasourceOptions } from '../__mocks__/datasource';
 import { select } from 'react-select-event';

+import { mockDatasourceOptions } from '../__mocks__/datasource';
+import { selectors } from '../selectors';
+import { ConfigEditor } from './ConfigEditor';
+


Auto re-organised ;)

yaelleC · 2022-03-01T10:40:16Z

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';


yaelleC · 2022-03-01T10:41:01Z

src/ConfigEditor/ConfigEditor.test.tsx

    });
  });

+  it('should allow user to enter a database', async () => {


Not technically used in this PR but improves coverage :)

yaelleC · 2022-03-01T13:45:19Z

src/ConfigEditor/ConfigEditor.tsx


+  // Description to show in datasource card in list
+  const { onOptionsChange: propsOnOptionsChange } = props;
+  useEffect(() => {


This should not be done here: it's triggered for every key stroke on clusterIdentifier Input 😞

option 1: do this on save
option 2: use onBlur on input rather than onChange

I agree, there is no exposed hook for "on save" but we can make it on blur. We need to modify the component in the SDK but it should be simple: https://github.com/grafana/grafana-aws-sdk-react/blob/main/src/sql/ConfigEditor/InlineInput.tsx#L28

In fact, if we can expose all the Input properties there we will avoid this kind of issues in the future.

If you create a PR against that repo, you can just bump the version there and it will be automatically released so you can use it here.

I was wondering if we could instead make clusterId a dropdown by exposing another endpoint (same call without filtering on cluster ID)?
It seems more user friendly to select a cluster rather than typing it?

that's a good point, it's actually the experience that you get in the AWS console. The only downside of this is that the API permission that is optional now would become mandatory. We could workaround that allowing people to type the the clusterId anyway in the selector but that also requires changes in the SDK 😅 (since the wrapper around the Select doesn't allow to set the allowCustomValue at the moment).

In any case, since it would be a UX improvement and it's trivial to implement now that we have already the endpoint to get a Cluster I would go for it. I would change the current Cluster endpoint for a its plural and return the list of all the clusters (so no additional API requests are needed). WDYT?

yes, that sounds good, only having the one API request and get the url from the cluster selected in the dropdown 👍

Re-permissions, are you saying we should also make changes to the SDK to allow custom values for clusterID? Or make the API permission compulsory?

since we can avoid the "breaking" change yes, we should modify the SDK to allow that parameter. Basically we would need to modify this Select and modify the props of the wrappers ResourceSelector and ConfigSelect

andresmgot · 2022-03-02T09:17:30Z

src/ConfigEditor/ConfigEditor.tsx


+  // Description to show in datasource card in list
+  const { onOptionsChange: propsOnOptionsChange } = props;
+  useEffect(() => {


I agree, there is no exposed hook for "on save" but we can make it on blur. We need to modify the component in the SDK but it should be simple: https://github.com/grafana/grafana-aws-sdk-react/blob/main/src/sql/ConfigEditor/InlineInput.tsx#L28

In fact, if we can expose all the Input properties there we will avoid this kind of issues in the future.

If you create a PR against that repo, you can just bump the version there and it will be automatically released so you can use it here.

andresmgot · 2022-03-02T09:20:10Z

src/ConfigEditor/ConfigEditor.tsx

    return res;
  };
+  const fetchCluster = async (clusterId: string) => {
+    const res: Cluster = await getBackendSrv().post(resourcesURL + '/cluster', { clusterIdentifier: clusterId });


I tried this and it's curious that if you get a 400 error (e.g. the clusterId does not exists), the error does not appear in the UI but if it's a 404 or a 500 you get it. Since this is optional, I will wrap this in a try {} catch so we don't show the error in any case.

yaelleC mentioned this pull request Feb 25, 2022

Issue 61 : Backend Add details in the datasource card #121

Merged

yaelleC commented Mar 1, 2022

View reviewed changes

yaelleC linked an issue Mar 1, 2022 that may be closed by this pull request

Add datasource details in the list #61

Closed

yaelleC commented Mar 1, 2022

View reviewed changes

yaelleC marked this pull request as ready for review March 1, 2022 14:47

yaelleC requested a review from a team as a code owner March 1, 2022 14:47

yaelleC requested review from andresmgot and sunker and removed request for a team March 1, 2022 14:47

andresmgot reviewed Mar 2, 2022

View reviewed changes

yaelleC mentioned this pull request Mar 4, 2022

Issue 61: Updated endpoint to return list of all clusters #129

Merged

yaelleC closed this Mar 7, 2022

yaelleC force-pushed the yaelle_61_fe branch from b2718db to 239649c Compare March 7, 2022 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

yaelleC commented Feb 25, 2022 •

edited

Loading

Uh oh!

yaelleC Mar 1, 2022

Uh oh!

yaelleC Mar 1, 2022

Uh oh!

yaelleC Mar 1, 2022

Uh oh!

yaelleC Mar 1, 2022

Uh oh!

andresmgot Mar 2, 2022

Uh oh!

yaelleC Mar 3, 2022

Uh oh!

andresmgot Mar 3, 2022

Uh oh!

yaelleC Mar 3, 2022

Uh oh!

andresmgot Mar 3, 2022

Uh oh!

andresmgot Mar 2, 2022

Uh oh!

andresmgot Mar 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

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

Uh oh!

Conversation

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

Uh oh!

Description

Screenshots

Test coverage

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yaelleC commented Feb 25, 2022 •

edited

Loading