Feat: improve delete private probe flow #951

w1kman · 2024-10-02T09:57:57Z

What this PR does / why we need it:
This PR aims to improve the management of probes, including better error handling (specifically when deleting probes). Probe listing now uses more of the components shipped by @grafana/ui, which (hopefully) will allow for the view to be more in line with Grafana.

UI has been updated in collaboration with the design dep. It's intended to be good enough, until a potential UI overhaul.

Probe Listing

Updates:

Removed badges
Added state (online/offline) indicator with tooltip
Labels now have their actual name listed
Improves key/value UI for labels
Added unsupported check types in Probe listing (browser/scripted)
Possibility to mark, copy & paste values from the probe card
Moved the onClick (edit) to only be attached to the probe name and the Edit/View button (no longer hidden when not hovering)
Remove "public/private" from probe listing as probes are group by "public/private" already
Region is moved into the card title
Added usage link for when a probe is used in one or several checks (link filters checks by probe)
Added icons to View/Edit buttons

Probe delete button

Updates:

Disabled if a probe is in use
Tooltip when disabled (with link to checks that are using the probe)

Confirm modal (async)

Updates:

Added new component ConfirmModal
When prop async is not present or false, the ConfirmModal from @grafana/ui is returned
With async
- An enhanced version of @grafana/ui is returned instead.
- onConfirm callback is .catched by the modal
- If onError is passed, the error message can be handled (and passed as prob) from the outside
- If onError is not passed, the component tries to use .name and .message from the caught error (with generic fallbacks).

Additional fixes and refactoring

DataSource.fetchAPI was added to keep the code bit more DRY
Removed some redundant variables

Which issue(s) this PR fixes:

Fixes https://github.com/grafana/synthetic-monitoring/issues/97

Special notes for your reviewer:
To trigger error handling for the ConfirmDialog, change line 48 in DeleteProbeButton from
if (!canDelete) { to if (!!canDelete) { (which will allow you to press the delete button for probes that are in use). You can also use tweak if you want to go all-in on testing different types of errors/responses

- remove redundant variables - clean-up unneeded if-statements

- add `ConfirmModal` that supports async confirm (including errors/catch). - add business logic to avoid error when trying to delete probe that is in use

CLAassistant · 2024-10-02T09:58:04Z

All committers have signed the CLA.

…s-handling # Conflicts: # src/components/ProbeEditor/ProbeEditor.tsx # src/page/EditProbe/EditProbe.tsx # src/test/fixtures/probes.ts

w1kman

self-review 👍🏻

VikaCep · 2024-10-04T20:09:41Z

Maybe we could rewrite this error on the frontend to make it more user friendly (capitalize the title and change the id to to the probe name or even add the link to the checks that use it)

VikaCep

Really nice improvement! Love how it looks now 💯 💯 💯
I left a few comments/questions.

VikaCep · 2024-10-04T19:24:04Z

src/datasource/DataSource.ts

@@ -42,6 +37,20 @@ export class SMDataSource extends DataSourceApi<SMQuery, SMOptions> {
    super(instanceSettings);
  }

+  async fetchAPI<T>(url: BackendSrvRequest['url'], options?: Omit<BackendSrvRequest, 'url'>) {


Nice addition!

VikaCep · 2024-10-04T19:26:10Z

src/datasource/DataSource.ts

-      })
-    ).then((res) => {
-      return res.data;
+    return this.fetchAPI(`${this.instanceSettings.url}/sm/probe/add`, {


Can we type it as we do above? Like this.fetchAPI<AddProbeResult>(`${this.instanceSettings.url}/sm/probe/add`, {

Same question for all other calls where the type is not set when calling fetchAPI

If the type is missing, it's because it was also missing previously.
I will take a look at how the usage trickles down, and update with types.

src/datasource/DataSource.ts

VikaCep · 2024-10-04T19:31:49Z

src/datasource/DataSource.ts

@@ -50,6 +50,8 @@ export class SMDataSource extends DataSourceApi<SMQuery, SMOptions> {
        ...options,
      })
    ).catch((error: unknown) => {
+      // We could log the error here


Yeah, I think this is a good idea

src/components/ConfirmModal/ConfirmModal.tsx

w1kman · 2024-10-09T06:13:21Z

Maybe we could rewrite this error on the frontend to make it more user friendly (capitalize the title and change the id to to the probe name or even add the link to the checks that use it)

While I agree that the error message could look better, I wouldn't focus on this particular error message, as it results from modifying code. Ideally, this error shouldn't be shown to a user of the app (unless someone starts using a probe while another one tries to delete it).

We could definitely upper case first letter of the error.name and we could match and replace numeric portions of the error.message. 👍🏻

w1kman · 2024-10-09T11:30:25Z

@VikaCep how about this?

The ConfirmModal will uppercase first char
The error message (the id) is handled by the DeleteProbeButton error handler.

- Add missing type in `DataSource.fetchAPI` usage - Fragment code to make it more readable - Upper case first letter in `ConfirmModal` - Refactor `DeleteProbeButton`

VikaCep

👏👏👏

ckbedwell

Mostly nitty nits. A few other comments but on a whole this is ace 💪

ckbedwell · 2024-10-10T08:54:30Z

src/components/ConfirmModal/ConfirmModal.tsx

+        <Alert className={styles.alert} title={error?.name ?? `${title} error`} severity="error">
+          <div>{error?.message ?? GENERIC_ERROR_MESSAGE}</div>


Nittiest of nits: Don't need the optional chaining for error?.name or error?.message here or you need to update the ConfirmError type to allow them to be optional.

src/components/DeleteProbeButton/DeleteProbeButton.tsx

ckbedwell · 2024-10-10T09:11:49Z

src/components/DeleteProbeButton/DeleteProbeButton.tsx

+    );
+
+    return (
+      <Tooltip content={tooltipContent} interactive={canEdit && !canDelete}>


You can add the tooltip prop directly to button now and it works better for accessibility as it becomes keyboard accessible and reads out the tooltip despite the button being 'disabled'.

With that said, I don't love the link in the tooltip as Grafana doesn't support accessing interactive content via the keyboard in tooltips but as there is an alternative presentation of the link in the sidebar this is okay 😄

I had to use both the Tooltip component and the tooltip prop 😓
With only the prop, the tooltip isn't interactable
With disabled on the Button there is no focus (unless aria-disabled is used, which is only possible when tooltip is truthy).

ckbedwell · 2024-10-10T09:15:09Z

src/components/ProbeCard/ProbeCard.test.tsx

  const { history, user } = render(<ProbeCard probe={probe} />);
  await screen.findByText(probe.name);
  await user.click(screen.getByText(probe.name));
  expect(history.location.pathname).toBe(`${getRoute(ROUTES.EditProbe)}/${probe.id}`);
 });
+
+it.each<[ExtendedProbe, string]>([


TIL. I always manually looped over when I wanted to do something like this 😅

ckbedwell · 2024-10-10T09:23:56Z

src/components/ProbeCard/ProbeCard.tsx

-    <Card className={styles.card} href={href}>
-      <div className={styles.cardContent}>
+    <Card>
+      <Card.Heading>


I am torn on moving back to using @grafana/ui's Card component. IMO they have painted themselves into a corner with how prescriptive it is and it is doing way more work than what I would expect a Design System's card to provide.

I think you've done a good job with what they provide here but I do hate that the heading is a h2 regardless as I'd flag this as an accessibility issue on this page when doing an audit. We could probably add props to the Card.Heading upstream in grafana/grafana to fix this at a later date so can leave it but just want to flag why I made a local Card component in the first place.

src/components/ProbeCard/ProbeCard.tsx

src/components/ProbeUsageLink.tsx

src/components/DeleteProbeButton/DeleteProbeButton.tsx

ckbedwell · 2024-10-10T10:01:46Z

Last comment but I don't think this is for this PR but future enhancements for the probes UI.

I was experimenting with what happens when I:

go to edit a probe that no longer exists: I get redirected -- I think it'd be better to explicitly say the probe doesn't exist as it is less confusing that way.
and what happens if the probe list request fails -- it doesn't mention it errored but just says there are no private or public probes available.

Some things for us to do in the future 😃

- Remove optional chaining

- Remove optional chaining - Remove commented code - Navigate to probe list on delete success - Make tooltip accessible

- Remove unused styles - Add test id to `dataTestIds.ts`

…s-handling

ckbedwell

LGTM 🔥🔥🔥

grafanaman and others added 2 commits August 13, 2024 08:31

refactor(checks-filters): simplify code

6727c84

- remove redundant variables - clean-up unneeded if-statements

refactor(probes): improve delete probe flow

db35d37

- add `ConfirmModal` that supports async confirm (including errors/catch). - add business logic to avoid error when trying to delete probe that is in use

w1kman added 6 commits October 2, 2024 12:29

refactor(datasource): use .fetchAPI

4a62c5b

refactor: remove redundant code

8f7a088

Merge branch 'refs/heads/main' into feat/private-probes-in-use-proble…

aced6c6

…s-handling # Conflicts: # src/components/ProbeEditor/ProbeEditor.tsx # src/page/EditProbe/EditProbe.tsx # src/test/fixtures/probes.ts

refactor: update tests

8c76f7d

chore: fix linting issues

f440804

chore: fix ts errors after refactor

c95f938

w1kman marked this pull request as ready for review October 3, 2024 07:38

w1kman requested a review from a team as a code owner October 3, 2024 07:38

w1kman commented Oct 3, 2024

View reviewed changes

fix(ConfirmModal): wire up button text props

fa6fad6

w1kman requested review from VikaCep and ckbedwell and removed request for a team October 3, 2024 08:11

VikaCep reviewed Oct 4, 2024

View reviewed changes

fix: change requests

c15932c

- Add missing type in `DataSource.fetchAPI` usage - Fragment code to make it more readable - Upper case first letter in `ConfirmModal` - Refactor `DeleteProbeButton`

w1kman requested a review from VikaCep October 9, 2024 11:33

VikaCep approved these changes Oct 9, 2024

View reviewed changes

ckbedwell requested changes Oct 10, 2024

View reviewed changes

w1kman added 4 commits October 15, 2024 12:51

fix: change requests #2

96cbe92

- Remove optional chaining

fix: change requests #2

74d8020

- Remove optional chaining - Remove commented code - Navigate to probe list on delete success - Make tooltip accessible

fix: change requests #3

1f5c22c

- Remove unused styles - Add test id to `dataTestIds.ts`

fix: linting error

d584621

w1kman requested a review from ckbedwell October 15, 2024 11:50

Merge branch 'refs/heads/main' into feat/private-probes-in-use-proble…

2fe04b2

…s-handling

ckbedwell approved these changes Oct 15, 2024

View reviewed changes

w1kman merged commit 8be4a9e into main Oct 15, 2024
5 checks passed

w1kman deleted the feat/private-probes-in-use-probles-handling branch October 15, 2024 14:24

sm-release-app bot mentioned this pull request Oct 15, 2024

chore(main): release 1.16.5 #969

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: improve delete private probe flow #951

Feat: improve delete private probe flow #951

w1kman commented Oct 2, 2024 •

edited

Loading

CLAassistant commented Oct 2, 2024 •

edited

Loading

w1kman left a comment

VikaCep commented Oct 4, 2024

VikaCep left a comment

VikaCep Oct 4, 2024

VikaCep Oct 4, 2024

w1kman Oct 9, 2024

VikaCep Oct 4, 2024

w1kman commented Oct 9, 2024

w1kman commented Oct 9, 2024

VikaCep left a comment

ckbedwell left a comment

ckbedwell Oct 10, 2024

ckbedwell Oct 10, 2024

w1kman Oct 15, 2024

ckbedwell Oct 10, 2024

ckbedwell Oct 10, 2024

ckbedwell commented Oct 10, 2024

ckbedwell left a comment

		<Alert className={styles.alert} title={error?.name ?? `${title} error`} severity="error">
		<div>{error?.message ?? GENERIC_ERROR_MESSAGE}</div>

Feat: improve delete private probe flow #951

Feat: improve delete private probe flow #951

Conversation

w1kman commented Oct 2, 2024 • edited Loading

CLAassistant commented Oct 2, 2024 • edited Loading

w1kman left a comment

Choose a reason for hiding this comment

VikaCep commented Oct 4, 2024

VikaCep 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

w1kman commented Oct 9, 2024

w1kman commented Oct 9, 2024

VikaCep left a comment

Choose a reason for hiding this comment

ckbedwell 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

ckbedwell commented Oct 10, 2024

ckbedwell left a comment

Choose a reason for hiding this comment

w1kman commented Oct 2, 2024 •

edited

Loading

CLAassistant commented Oct 2, 2024 •

edited

Loading