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.

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

change: [UIE-8220] - Settings text update #11166

Merged

Conversation

smans-akamai
Copy link
Contributor

@smans-akamai smans-akamai commented Oct 25, 2024

Description 📝

Updating text for database settings

Changes 🔄

List any change relevant to the reviewer.

  • Updating Settings text and labels

Target release date 🗓️

11/12/2024

Preview 📷

Before After
Before-DB-settings-update After-DB-settings

How to test 🧪

Prerequisites

(How to setup test environment)

  • Have access to databases to view database settings tab

Verification steps

  • Access database settings and view updated text
yarn test DatabaseSummary DatabaseSettings AccessControls AddAccessControlDrawer

As an Author I have considered 🤔

  • 👀 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

@smans-akamai smans-akamai requested a review from a team as a code owner October 25, 2024 21:32
@smans-akamai smans-akamai requested review from bnussman-akamai and jaalah-akamai and removed request for a team October 25, 2024 21:32
@smans-akamai smans-akamai force-pushed the UIE-8204-dbaas-settings-text-update branch from d76c819 to b1ebdde Compare October 25, 2024 21:36
@abailly-akamai
Copy link
Contributor

@smans-akamai Heads up you still have failing units as a result of this PR

@cpathipa cpathipa requested review from cpathipa and removed request for bnussman-akamai October 28, 2024 14:03
@cpathipa cpathipa added the DBaaS Relates to Database as a Service label Oct 28, 2024
Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

The things I mention below aren't included in this PR, but they are text on the Settings page so it may make sense to address them in this PR.

Couple questions about Weekly Maintenance Window copy:

  • Can we add a space before the sentence that starts For non-HA plans after the period of the preceding sentence?
  • Is the DBaaS team intentionally using the term DB Engine here? Throughout the rest of Cloud Manager, I believe we spell out database rather than useDB, so I'm curious to know if this is changing or if we want to continue without the abbreviation for consistency?

Screenshot 2024-10-28 at 7 13 31 AM

@cpathipa cpathipa removed the request for review from jaalah-akamai October 29, 2024 16:00
@mpolotsk-akamai mpolotsk-akamai force-pushed the UIE-8204-dbaas-settings-text-update branch from 595a1cc to 55fcfd5 Compare October 30, 2024 14:51
Copy link

github-actions bot commented Oct 30, 2024

Coverage Report:
Base Coverage: 87.23%
Current Coverage: 87.23%

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

I'm seeing test failures in CI that seem relevant. Unit test updates look good, but you'll want to run

yarn cy:run -s "cypress/e2e/core/databases/update-database.spec.ts,cypress/e2e/core/databases/resize-database.spec.ts"

and make sure the labels are updated.

Screenshot 2024-10-30 at 9 06 06 AM
Screenshot 2024-10-30 at 9 06 31 AM

@@ -198,7 +198,7 @@ export const MaintenanceWindow = (props: Props) => {
<Typography className={classes.sectionText}>
{isLegacy ? typographyLegacyDatabase : typographyDatabase}
{database.cluster_size !== 3
? 'For non-HA plans, expect downtime during this window.'
? ' For non-HA plans, expect downtime during this window.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
? ' For non-HA plans, expect downtime during this window.'
{isLegacy ? typographyLegacyDatabase : typographyDatabase}{' '}
{database.cluster_size !== 3
? 'For non-HA plans, expect downtime during this window.'
: null}

Nit: how we tend to add spaces to avoid modifying the actual copy.

Copy link
Contributor

@cpathipa cpathipa left a comment

Choose a reason for hiding this comment

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

Confirming on the text updates, This is good to go with cypress test fix.

@mjac0bs
Copy link
Contributor

mjac0bs commented Oct 30, 2024

Okay, it was just the update spec that is relevant here. Looks like the resize spec was due to a feature flag toggle as Joe mentions here: #11190

@mpolotsk-akamai mpolotsk-akamai requested a review from a team as a code owner October 31, 2024 11:04
@mpolotsk-akamai mpolotsk-akamai requested review from jdamore-linode and removed request for a team October 31, 2024 11:04
@mpolotsk-akamai mpolotsk-akamai force-pushed the UIE-8204-dbaas-settings-text-update branch from df23346 to bf4d30f Compare October 31, 2024 11:12
@mpolotsk-akamai
Copy link
Contributor

Okay, it was just the update spec that is relevant here. Looks like the resize spec was due to a feature flag toggle as Joe mentions here: #11190

@mjac0bs , done

Copy link
Contributor

@cpathipa cpathipa left a comment

Choose a reason for hiding this comment

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

Thank you @smans-akamai LGTM!

@cpathipa cpathipa added the Add'tl Approval Needed Waiting on another approval! label Oct 31, 2024
@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Oct 31, 2024
@cpathipa cpathipa merged commit 5bb7839 into linode:develop Oct 31, 2024
23 checks passed
Copy link

cypress bot commented Oct 31, 2024

Cloud Manager E2E    Run #6762

Run Properties:  status check passed Passed #6762  •  git commit 5bb78393ad: change: [UIE-8220] - Settings text update (#11166)
Project Cloud Manager E2E
Branch Review develop
Run status status check passed Passed #6762
Run duration 24m 59s
Commit git commit 5bb78393ad: change: [UIE-8220] - Settings text update (#11166)
Committer smans-akamai
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 445
View all changes introduced in this branch ↗︎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! DBaaS Relates to Database as a Service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants