Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: [UIE-8007] - DBaaS enhancements summary and settings #10939

Merged
merged 1 commit into from
Sep 18, 2024
Merged

feat: [UIE-8007] - DBaaS enhancements summary and settings #10939

merged 1 commit into from
Sep 18, 2024

Conversation

corya-akamai
Copy link
Contributor

@corya-akamai corya-akamai commented Sep 13, 2024 •
edited
Loading

Description 📝

DBaaS enhancements to the Summary and Settings tabs

Changes 🔄

List any change relevant to the reviewer.

  • Readonly host displayed
  • Hide monthly maintenance window for V2

Target release date 🗓️

9/30/24

How to test 🧪

Prerequisites

(How to setup test environment)

  • Account must have capability "Managed Databases Beta"

Reproduction steps

(How to reproduce the issue, if applicable)

  • Use mock data to view fields in both V1 and V2

Verification steps

(How to verify changes)

  • Text Copy is different for V2
  • Monthly/weekly radio is hidden for V2

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@corya-akamai corya-akamai requested a review from a team as a code owner September 13, 2024 21:36
@corya-akamai corya-akamai requested review from bnussman-akamai and coliu-akamai and removed request for a team September 13, 2024 21:36
@mjac0bs mjac0bs added DBaaS Relates to Database as a Service Ready for Review labels Sep 16, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 16, 2024 •
edited
Loading

Coverage Report: ✅
Base Coverage: 86.93%
Current Coverage: 86.93%

bnussman-akamai
bnussman-akamai approved these changes Sep 17, 2024
View reviewed changes
Copy link
Member

@bnussman-akamai bnussman-akamai left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, left a few minor points of feedback. I do think the readOnly vs read_only should be addressed.

...ges/manager/src/features/Databases/DatabaseDetail/DatabaseSettings/DatabaseSettings.test.tsx
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job on this test. Clean and concise 🧼

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

const deleteClusterCopy =
'Deleting a database cluster is permanent and cannot be undone.';
let deleteClusterCopy = 'Permanently remove an unused database cluster.';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the legacy copy is a bit better.

What is the user wants to delete an in-use database cluster? Is the API really going to stop them? The copy makes it sound like the database must be unused to delete it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know the copy came from tech writing, but I can check.

packages/manager/src/features/Databases/DatabaseDetail/DatabaseSettings/MaintenanceWindow.tsx
Maintenance Window
{isLegacy
? 'Maintenance Window'
: 'Set a Weekly Maintenance Window'}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All other headings on this page don't use verbs. I'd prefer if we keep headings consistent, but that's just my opinion.

Suggested change
: 'Set a Weekly Maintenance Window'}
: 'Weekly Maintenance Window'}

packages/manager/src/features/Databases/DatabaseDetail/DatabaseSettings/MaintenanceWindow.tsx Outdated Show resolved Hide resolved
...r/src/features/Databases/DatabaseDetail/DatabaseSummary/DatabaseSummaryConnectionDetails.tsx Outdated
@@ -372,6 +372,17 @@ export const DatabaseSummaryConnectionDetails = (props: Props) => {
/>
</Box>
) : null}
{database.platform === 'rdbms-default' && database.hosts.readOnly ? (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional:

Suggested change
{database.platform === 'rdbms-default' && database.hosts.readOnly ? (
{database.platform === 'rdbms-default' && database.hosts.readOnly && (

coliu-akamai
coliu-akamai approved these changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ confirmed copy updates
✅ confirm radios hidden for v2

thanks for the added test coverage! 🎉
left a few small, optional comments below

...ges/manager/src/features/Databases/DatabaseDetail/DatabaseSettings/DatabaseSettings.test.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Databases/DatabaseDetail/DatabaseSettings/DatabaseSettings.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Databases/DatabaseDetail/DatabaseSettings/MaintenanceWindow.tsx
@@ -173,22 +171,30 @@ export const MaintenanceWindow = (props: Props) => {
onSubmit: handleSaveMaintenanceWindow,
});

const isLegacy = database.platform === 'rdbms-legacy';

const typographyDatabase =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: I think we normally use capitalized snake case for copy constants, but someone correct me if I'm wrong!

Suggested change
const typographyDatabase =
const TYPOGRAPHY_DATABASE =

these constants could also be moved to the top of the scope, outside the component. Alternatively, you could do something like you did with the copies in DatabaseSettings:

const typographyDatabase = isLegacy ? ... : ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, other places have camelCase for text copy. Personally, I'd do CAPS for const but not sure the standard.

@coliu-akamai coliu-akamai added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Sep 17, 2024
@coliu-akamai coliu-akamai merged commit 9734976 into linode:develop Sep 18, 2024
17 of 20 checks passed
This was referenced Sep 18, 2024
Merged
Merged
This was referenced Sep 25, 2024
Merged
Merged
@corya-akamai corya-akamai deleted the DBaaS-v2 branch September 26, 2024 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coliu-akamai coliu-akamai coliu-akamai approved these changes

@bnussman-akamai bnussman-akamai bnussman-akamai approved these changes

Assignees

@corya-akamai corya-akamai

Labels
Approved Multiple approvals and ready to merge! DBaaS Relates to Database as a Service
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@corya-akamai @bnussman-akamai @coliu-akamai @mjac0bs