feat: [UIE-8090] - DBaaS Settings and Landing Page #11152

smans-akamai · 2024-10-23T19:52:17Z

Description 📝

Adding Suspend and Resume functionality to Landing Page and database details Settings Views for GA

Changes 🔄

Suspend Cluster button can now appear in the DatabaseSettings that displays and Suspend Cluster dialog
Suspend and Resume actions added to Database Landing table
In the Database Landing table, clusters with suspending, suspended, and resuming status will not have detail links

Target release date 🗓️

11/12/2024

Preview 📷

Before	After	After 2

How to test 🧪

Prerequisites

Managed Databases account capability
LD flag: beta: false, enabled: true

Verification steps

Toggle LD will show Suspend and resume functionality
Check DatabaseSettings to see new Suspend Button and Dialog behavior
Check DatabaseLanding to see new suspend and resume actions
Check DatabaseLanding to confirm that Suspend can only be used on active databases
Check DatabaseLanding to confirm that the resume action can only be used on Suspending and Suspended database clusters
Check DatabaseLanding to confirm that clusters with suspending, suspended, and resuming status do not have detail links under Cluster Label column

As an Author I have considered 🤔

Check all that apply

Commit message and pull request title format standards

Note: Remove this section before opening the pull request
Make sure your PR title and commit message on squash and merge are as shown below

<commit type>: [JIRA-ticket-number] - <description>

Commit Types:

feat: New feature for the user (not a part of the code, or ci, ...).
fix: Bugfix for the user (not a fix to build something, ...).
change: Modifying an existing visual UI instance. Such as a component or a feature.
refactor: Restructuring existing code without changing its external behavior or visual UI. Typically to improve readability, maintainability, and performance.
test: New tests or changes to existing tests. Does not change the production code.
upcoming: A new feature that is in progress, not visible to users yet, and usually behind a feature flag.

Example: feat: [M3-1234] - Allow user to view their login history

github-actions · 2024-10-23T20:23:51Z

Coverage Report: ✅
Base Coverage: 87.33%
Current Coverage: 87.33%

bnussman-akamai · 2024-10-23T22:21:41Z

packages/manager/src/features/Databases/DatabaseLanding/DatabaseLanding.tsx

I'd recommend breaking up this PR up so that this banner into its own PR. The banner logic seems somewhat complex and the implementation may be flawed (see comment below)

@bnussman-akamai After discussion, we've removed the banner and associated logic from this pull request and will separate that into it's own ticket. The design and logic will be reworked as part of that new ticket.

bnussman-akamai · 2024-10-23T22:22:37Z

.../features/Databases/DatabaseDetail/DatabaseSettings/DatabaseSettingsSuspendClusterDialog.tsx

+  );
+};
+
+export default DatabaseSettingsSuspendClusterDialog;


Please avoid using default exports. You can just use the export const DatabaseSettingsSuspendClusterDialog and remove this export

bnussman-akamai · 2024-10-23T22:25:09Z

.../features/Databases/DatabaseDetail/DatabaseSettings/DatabaseSettingsSuspendClusterDialog.tsx

+  );
+
+  return (
+    <Dialog


You may want to consider using the Confirmation Dialog component rather than a pure Dialog. It may help you to fix the style/spacing issues

@bnussman-akamai Can the confirmation dialog support a checkbox that enables/disables the Suspend Cluster button?

@bnussman-akamai I've updated the suspend dialog to use the confirmation dialog.

bnussman-akamai · 2024-10-23T22:26:32Z

.../features/Databases/DatabaseDetail/DatabaseSettings/DatabaseSettingsSuspendClusterDialog.tsx

+  const [error, setError] = React.useState('');
+  const [isLoading, setIsLoading] = React.useState(false);


No need to define your own error and loading state. The useSuspendDatabaseMutation hook returns a isPending and error state you can use

I've updated this to leverage isPending and error from useSuspendDatabaseMutation

bnussman-akamai · 2024-10-23T22:28:02Z

.../features/Databases/DatabaseDetail/DatabaseSettings/DatabaseSettingsSuspendClusterDialog.tsx

+export const DatabaseSettingsSuspendClusterDialog: React.FC<SuspendDialogProps> = (
+  props
+) => {


We prefer to not use React.FC in our repo. You can type your props this way:

Suggested change

export const DatabaseSettingsSuspendClusterDialog: React.FC<SuspendDialogProps> = (

props

) => {

export const DatabaseSettingsSuspendClusterDialog = (

props: SuspendDialogProps

) => {

Thanks! I've made this update.

bnussman-akamai · 2024-10-23T22:41:26Z

packages/manager/src/features/Databases/DatabaseLanding/DatabaseLanding.tsx

+      return notifications;
+    }
+    // Loop through new databases to find suspended databases and check if a notification should be displayed.
+    newDatabases.data.forEach((database) => {


We may need to reconsider/rearchitect this banner feature. Our tables are API-paginated, meaning that newDatabases only contains the first page of databases. This logic may not produce the desired result a suspended database is not on the current API paginated page.

I do not reccomend fetching all databases to solve this because it kind of defeats the purpose of API based pagunation.

There may be a clever way we can use api-v4 x-filtering to query for only suspended databases, which might be a more scaleable solution.

I'm happy to do a proof of concept or talk through alternatives to this, just let me know!

bnussman-akamai · 2024-10-23T22:42:54Z

packages/manager/src/features/Databases/DatabaseLanding/DatabaseLanding.tsx

+  const isNewDatabase = isV2Enabled && !!newDatabases?.data.length;
+  const showSuspend = isDatabasesV2GA && !!newDatabases?.data.length;
+
+  const getSuspendNotification = (


In general, I recommend extracting stuff like this into utility functions outside of this component. It makes it easier to unit test and makes the code more readable.

@bnussman-akamai Thanks for the recommendation. We'll follow moving this into utilities once/if it's used in a second place.

bnussman-akamai · 2024-10-23T22:44:29Z

packages/manager/src/features/Databases/DatabaseLanding/DatabaseLanding.tsx

+      {showSuspend
+        ? suspendNotifications.map((notification) => (
+            <DismissibleBanner
+              preferenceKey={`${notification.id}-suspend-notice`}
+              variant="warning"
+              key={`${notification.id}-key`}
+            >
+              <Box>
+                <Typography>
+                  {`${notification.label} has been suspended for ${SUSPEND_WARNING_DAYS} days. If it is not resumed to be operational it will be automatically deleted ${notification.expirationCopy}.`}
+                </Typography>
+              </Box>
+            </DismissibleBanner>
+          ))
+        : null}


This is going to look very bad if a user has lots of suspended databases (especially on mobile). I think we should consider a more scaleable UI. Cloud Manager already has enough banners.

@bnussman-akamai Thanks, we'll note this concern.

@bnussman-akamai After discussion, we've removed the banner and associated logic from this pull request and will separate that into it's own ticket. The design and logic will be reworked as part of that new ticket.

bnussman-akamai · 2024-10-23T22:47:51Z

packages/manager/src/features/Databases/DatabaseLanding/DatabaseLandingTable.tsx

+    setIsSuspendClusterDialogOpen(true);
+  };
+
+  const handleResume = (database: DatabaseInstance) => {


Because this resume logic isn't really used in this component we might be able to move it down into the DatabaseActionMenu component

bnussman-akamai · 2024-10-23T22:51:17Z

packages/manager/src/queries/databases/databases.ts

+      queryClient.invalidateQueries({
+        queryKey: databaseQueries.databases.queryKey,
+      });
+      queryClient.removeQueries({


Is the removeQueries call here and in useResumeDatabaseMutation intentional? I believe that will evict that database from the cache.

invalidateQueries might be the desired function here. invalidateQueries will make Cloud Manager refetch the database as needed

After reviewing your comment, I believe that invalidateQueries makes more sense for this use case as the database hasn't been removed, only updated. So we'll want to trigger a refetch on stale data rather than removing the cache entirely.

I'll update this for both useResumeDatabaseMutation and useSuspendDatabaseMutation.

bnussman-akamai

Looking great. Thanks for responding to that initial feedback and breaking up this PR, we appreciate it!

Just a few more changes and I think this will be good to go

bnussman-akamai · 2024-10-25T18:21:02Z

.../features/Databases/DatabaseDetail/DatabaseSettings/DatabaseSettingsSuspendClusterDialog.tsx

+    <ConfirmationDialog
+      maxWidth="sm"
+      actions={actions}
+      onClose={onClose}
+      open={open}
+      title={`Suspend ${databaseLabel} cluster?`}
+    >
+      {isError && (
+        <Notice
+          text={getAPIErrorOrDefault(error, defaultError)[0].reason}
+          variant="error"
+        />
+      )}


Suggested change

<ConfirmationDialog

maxWidth="sm"

actions={actions}

onClose={onClose}

open={open}

title={`Suspend ${databaseLabel} cluster?`}

>

{isError && (

<Notice

text={getAPIErrorOrDefault(error, defaultError)[0].reason}

variant="error"

/>

)}

<ConfirmationDialog

error={

error ? getAPIErrorOrDefault(error, defaultError)[0].reason : undefined

}

actions={actions}

maxWidth="sm"

onClose={onClose}

open={open}

title={`Suspend ${databaseLabel} cluster?`}

>

Our ConfirmationDialog component has an error prop you can use to show the error state. It looks a bit different than the notice, but it allows us to maintain consistancy across Cloud Manager.

We also have an upcoming effort to improve this error state (Ticket M3-8792) so it will look very similar to what you implemented soon

bnussman-akamai · 2024-10-25T18:21:15Z

.../features/Databases/DatabaseDetail/DatabaseSettings/DatabaseSettingsSuspendClusterDialog.tsx

+          <b>{suspendClusterCopy}</b>
+        </Typography>
+      </Notice>
+      <div>


This <div> might be unnecessary

bnussman-akamai · 2024-10-25T18:22:27Z

...ures/Databases/DatabaseDetail/DatabaseSettings/DatabaseSettingsSuspendClusterDialog.test.tsx

+    const closeButton = getByTestId('CloseIcon');
+    expect(closeButton).toBeVisible();
+
+    fireEvent.click(closeButton);


We should use await userEvent rather than fireEvent whenever possible. It is recommended for testing interactions by React Testing library (docs)

bnussman-akamai · 2024-10-25T18:27:42Z

packages/manager/src/features/Databases/DatabaseLanding/DatabaseActionMenu.tsx

+          variant: 'success',
+        });
+      })
+      .catch((e: any) => {


Let's use APIError[] here

Suggested change

.catch((e: any) => {

.catch((e: APIError[]) => {

cpathipa

Functionality LGTM! left few comments to lift conditional rendering for modals and nit-picky things to improve readability.

packages/manager/src/features/Databases/DatabaseDetail/DatabaseSettings/DatabaseSettings.tsx

.../features/Databases/DatabaseDetail/DatabaseSettings/DatabaseSettingsSuspendClusterDialog.tsx

cpathipa · 2024-10-28T14:41:44Z

.../features/Databases/DatabaseDetail/DatabaseSettings/DatabaseSettingsSuspendClusterDialog.tsx

+    You can resume the clusters work within 180 days from its suspension.
+    After that time, the cluster will be deleted permanently.`;
+
+  const actions = (


Use capital naming conventions for JSX elements used within a component (Contributing Guidelines)

I get the point of this comment, but if you search the code for const actions = you'll see there are a dozen like this and none with capitalization. For now, it seems like consistency would be preferred, but I am new to this whole thing, so please correct me if I am wrong.

We are indeed transitioning to this new approach to maintain a cleaner and consistent codebase.

cpathipa · 2024-10-28T14:44:13Z

.../features/Databases/DatabaseDetail/DatabaseSettings/DatabaseSettingsSuspendClusterDialog.tsx

+    setHasConfirmed(false);
+  };
+
+  const suspendClusterCopy = `A suspended cluster stops working immediately and you won't be billed for it.


This can be moved to constants file or out side of the component.

It could be but there are two different "suspendClusterCopy" constants in the code and they are different. I'm just a visitor here while Sam is on vacation :) but I prefer the way Sam has done it, being close to the one place it's used.

cpathipa · 2024-10-28T14:44:25Z

.../features/Databases/DatabaseDetail/DatabaseSettings/DatabaseSettingsSuspendClusterDialog.tsx

+    reset,
+  } = useSuspendDatabaseMutation(databaseEngine, databaseId);
+
+  const defaultError = 'There was an error suspending this Database Cluster.';


This can be moved to constants file or out side of the component.

See comment above but in this case it's an even more generic name and is being used once in this area. I toyed with the idea of removing it altogether and just inlining the text as is done throughout this codebase, it seems, but in the end decided to leave it alone.

packages/manager/src/features/Databases/DatabaseLanding/DatabaseLandingTable.tsx

packages/manager/src/features/Databases/DatabaseLanding/DatabaseActionMenu.tsx

bnussman-akamai

Looks good to me!

There is a conflict that needs to be addressed

bnussman-akamai · 2024-11-01T15:44:29Z

.../features/Databases/DatabaseDetail/DatabaseSettings/DatabaseSettingsSuspendClusterDialog.tsx

+        <Typography style={{ fontSize: '0.875rem' }}>
+          <b>{suspendClusterCopy}</b>
+        </Typography>


We might just want to pass

Suggested change

<Typography style={{ fontSize: '0.875rem' }}>

<b>{suspendClusterCopy}</b>

</Typography>

{suspendClusterCopy}

and let the Notice handle the text size and bolding

bnussman-akamai · 2024-11-01T15:45:08Z

packages/manager/src/features/Databases/DatabaseLanding/DatabaseActionMenu.tsx

+import { useResumeDatabaseMutation } from 'src/queries/databases/databases';
+import { enqueueSnackbar } from 'notistack';
+import { getAPIErrorOrDefault } from 'src/utilities/errorUtils';
+import { APIError } from '@linode/api-v4/src/types';


Our import should not contain src. Might cause problems in the future

Suggested change

import { APIError } from '@linode/api-v4/src/types';

import { APIError } from '@linode/api-v4';

This was removed because the import was removed. (It was being used to because someone had added a type defn to the catch's error, but doing so was causing "TS1196: Catch clause variable type annotation must be 'any' or 'unknown' if specified." so I changed APIError to any and this import was no longer needed.) Good to know going forward though to look for this problem.

… view for GA

…nding UIE-8090: Add suspend and resume to Landing Page and details Settings…

cpathipa

Thank you for addressing the feedback, LGTM! Please update this branch with the latest from develop.

… view for GA

…ttings-landing

…nding UIE-8090 dbaas settings landing - updating from develop

… view for GA

cpathipa · 2024-11-05T16:21:31Z

@smans-akamai @mpolotsk-akamai @rodonnel-akamai Looks like there are some type errors in the DatabaseSettings file.

cpathipa · 2024-11-05T16:26:28Z

Also, there is a failed unit test in DatabaseSetting

…ttings-landing

…nding UIE 8090 dbaas settings landing - update to catch up with develop and fix merge problem

linode-gh-bot · 2024-11-05T20:00:36Z

Cloud Manager UI test results

🎉 445 passing tests on test run #13 ↗︎

❌ Failing	✅ Passing	↪️ Skipped	🕐 Duration
`0 Failing`	`445 Passing`	`2 Skipped`	`90m 8s`

cpathipa · 2024-11-05T20:08:41Z

Confirming failed tests passed and type errors are fixed.

smans-akamai requested a review from a team as a code owner October 23, 2024 19:52

smans-akamai requested review from carrillo-erik and cpathipa and removed request for a team October 23, 2024 19:52

bnussman-akamai assigned smans-akamai Oct 23, 2024

bnussman-akamai added the DBaaS Relates to Database as a Service label Oct 23, 2024

smans-akamai force-pushed the UIE-8090-dbaas-settings-landing branch 2 times, most recently from 2ccd150 to 5be73c9 Compare October 23, 2024 19:59

smans-akamai force-pushed the UIE-8090-dbaas-settings-landing branch from 5be73c9 to 4ac31c4 Compare October 23, 2024 22:15

bnussman-akamai reviewed Oct 23, 2024

View reviewed changes

cpathipa removed the request for review from carrillo-erik October 24, 2024 14:16

smans-akamai force-pushed the UIE-8090-dbaas-settings-landing branch 3 times, most recently from 9129ac6 to cd8bab9 Compare October 25, 2024 16:38

smans-akamai requested review from m-sulik and bnussman-akamai October 25, 2024 16:49

smans-akamai force-pushed the UIE-8090-dbaas-settings-landing branch from cd8bab9 to 7394385 Compare October 25, 2024 18:09

bnussman-akamai reviewed Oct 25, 2024

View reviewed changes

feat: [UIE-8090] - DBaaS Settings and Landing Page

05ea8b4

smans-akamai force-pushed the UIE-8090-dbaas-settings-landing branch from 7394385 to 05ea8b4 Compare October 25, 2024 20:57

smans-akamai requested a review from bnussman-akamai October 25, 2024 20:58

cpathipa requested changes Oct 28, 2024

View reviewed changes

bnussman-akamai approved these changes Nov 1, 2024

View reviewed changes

rodonnel-akamai and others added 2 commits November 1, 2024 14:48

UIE-8090: Add suspend and resume to Landing Page and details Settings…

d4f0e27

… view for GA

Merge pull request #1 from rodonnel-akamai/UIE-8090-dbaas-settings-la…

713f1fe

…nding UIE-8090: Add suspend and resume to Landing Page and details Settings…

cpathipa approved these changes Nov 4, 2024

View reviewed changes

cpathipa added the Approved Multiple approvals and ready to merge! label Nov 4, 2024

smans-akamai and others added 2 commits November 4, 2024 13:24

feat: [UIE-8090] - DBaaS Settings and Landing Page

e0da958

UIE-8090: Add suspend and resume to Landing Page and details Settings…

f2dc179

… view for GA

rodonnel-akamai and others added 4 commits November 4, 2024 14:31

Merge branch 'UIE-8090-dbaas-settings-landing' into UIE-8090-dbaas-se…

795cc9e

…ttings-landing

Merge pull request #2 from rodonnel-akamai/UIE-8090-dbaas-settings-la…

ccfd1d4

…nding UIE-8090 dbaas settings landing - updating from develop

feat: [UIE-8090] - DBaaS Settings and Landing Page

3e673f4

UIE-8090: Add suspend and resume to Landing Page and details Settings…

d8dd5d0

… view for GA

rodonnel-akamai added 3 commits November 5, 2024 14:08

fix tests that broke as result of merge mistake

e027bc7

Merge branch 'UIE-8090-dbaas-settings-landing' into UIE-8090-dbaas-se…

56e6863

…ttings-landing

Merge pull request #3 from rodonnel-akamai/UIE-8090-dbaas-settings-la…

3a3b411

…nding UIE 8090 dbaas settings landing - update to catch up with develop and fix merge problem

cpathipa merged commit f8d33b5 into linode:develop Nov 5, 2024
23 checks passed

jaalah-akamai mentioned this pull request Nov 7, 2024

Release v1.132.0 - release → staging #11220

Merged

hana-akamai mentioned this pull request Nov 12, 2024

Release v1.132.0 - staging → master #11249

Merged

		const [error, setError] = React.useState('');
		const [isLoading, setIsLoading] = React.useState(false);

-export const DatabaseSettingsSuspendClusterDialog: React.FC<SuspendDialogProps> = (
-  props
-) => {
+export const DatabaseSettingsSuspendClusterDialog = (
+  props: SuspendDialogProps
+) => {

	import { APIError } from '@linode/api-v4/src/types';
	import { APIError } from '@linode/api-v4';

feat: [UIE-8090] - DBaaS Settings and Landing Page #11152

feat: [UIE-8090] - DBaaS Settings and Landing Page #11152

Conversation

smans-akamai commented Oct 23, 2024 • edited Loading

Description 📝

Changes 🔄

Target release date 🗓️

Preview 📷

How to test 🧪

Prerequisites

Verification steps

As an Author I have considered 🤔

Commit message and pull request title format standards

github-actions bot commented Oct 23, 2024 • edited Loading

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

bnussman-akamai 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

cpathipa 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

bnussman-akamai 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

cpathipa left a comment

Choose a reason for hiding this comment

cpathipa commented Nov 5, 2024

cpathipa commented Nov 5, 2024

linode-gh-bot commented Nov 5, 2024

Cloud Manager UI test results

🎉 445 passing tests on test run #13 ↗︎

cpathipa commented Nov 5, 2024

smans-akamai commented Oct 23, 2024 •

edited

Loading

github-actions bot commented Oct 23, 2024 •

edited

Loading