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

fix: [UIE-8259] dbaas summary blank read-only host should be N/A #11265

Conversation

smans-akamai
Copy link
Contributor

@smans-akamai smans-akamai commented Nov 14, 2024

Description 📝

Fixing DBaaS summary so that when the read-only host field is displayed and backend does not return a read-only host., N/A is displayed

Changes 🔄

When read-only host field has no value (secondary/standby), it displays N/A

Target release date 🗓️

12/10/2024

Preview 📷

Before After
summary-before summary-after

How to test 🧪

Prerequisites

  • Create a new database with 1 node which provides this state where the backend does not return a read-only host.

Reproduction steps

  • Create a new database with 1 node.
  • Navigate to the database summary and view the empty read-only host field

Verification steps

  • Navigate to the summary new db with 1 node and see that N/A is displayed for read-only hosts field when there is no secondary or standby host for the database from the backend.

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

  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@smans-akamai smans-akamai requested a review from a team as a code owner November 14, 2024 21:22
@smans-akamai smans-akamai requested review from bnussman-akamai and carrillo-erik and removed request for a team November 14, 2024 21:22
@smans-akamai smans-akamai force-pushed the UIE-8259-dbaas-summary-blank-read-only-host-fix branch from 60e2218 to b6dd438 Compare November 14, 2024 21:29
@cpathipa cpathipa requested review from cpathipa and removed request for carrillo-erik November 14, 2024 21:36
@cpathipa cpathipa added the DBaaS Relates to Database as a Service label Nov 14, 2024
Copy link

github-actions bot commented Nov 14, 2024

Coverage Report:
Base Coverage: 87.32%
Current Coverage: 87.32%

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Looks good, but the test should probably be cleaned up a bit

Comment on lines 88 to 99
const database = databaseFactory.build({
engine: POSTGRESQL,
hosts: {
primary: DEFAULT_PRIMARY,
},
id: 99,
platform: 'rdbms-default',
port: 22496,
ssl_connection: true,
}) as Database;
// Overwrite hosts so that only primary is available
database.hosts = { primary: DEFAULT_PRIMARY };
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 this might be a more simple way to express this:

  • We don't need as Database
  • Rather than overwriting the hosts after the factory has been created, I think we can just do it in the factory
Suggested change
const database = databaseFactory.build({
engine: POSTGRESQL,
hosts: {
primary: DEFAULT_PRIMARY,
},
id: 99,
platform: 'rdbms-default',
port: 22496,
ssl_connection: true,
}) as Database;
// Overwrite hosts so that only primary is available
database.hosts = { primary: DEFAULT_PRIMARY };
const database = databaseFactory.build({
engine: POSTGRESQL,
hosts: {
primary: DEFAULT_PRIMARY,
secondary: undefined,
standby: undefined,
},
id: 99,
platform: 'rdbms-default',
port: 22496,
ssl_connection: true,
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, that is simpler. I've applied this change!

Comment on lines 81 to 86
queryMocks.useDatabaseCredentialsQuery.mockReturnValue({
data: {
password: 'abc123',
username: AKMADMIN,
},
});
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this? This unit test has nothing to do with credentials

Suggested change
queryMocks.useDatabaseCredentialsQuery.mockReturnValue({
data: {
password: 'abc123',
username: AKMADMIN,
},
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this and the expect block for it below.

Comment on lines 110 to 112
await waitFor(() => {
expect(queryAllByText('N/A')).toHaveLength(1);
});
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the waitFor? This test doesn't seem to do anything async so it may not be needed

Suggested change
await waitFor(() => {
expect(queryAllByText('N/A')).toHaveLength(1);
});
expect(queryAllByText('N/A')).toHaveLength(1);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's not necessary, so I've removed it.

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 read-only host value.

image

@cpathipa cpathipa added the Approved Multiple approvals and ready to merge! label Nov 15, 2024
@smans-akamai smans-akamai force-pushed the UIE-8259-dbaas-summary-blank-read-only-host-fix branch from b6dd438 to 2bba2be Compare November 15, 2024 20:32
@smans-akamai smans-akamai force-pushed the UIE-8259-dbaas-summary-blank-read-only-host-fix branch from 2bba2be to d8ff309 Compare November 15, 2024 20:36
@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🔺 1 failing test on test run #4 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
1 Failing451 Passing2 Skipped98m 13s

Details

Failing Tests
SpecTest
clone-linode.spec.tsclone linode » can clone a Linode from Linode details page

Troubleshooting

Use this command to re-run the failing tests:

yarn cy:run -s "cypress/e2e/core/linodes/clone-linode.spec.ts"

@cpathipa cpathipa merged commit bea1ccc into linode:develop Nov 18, 2024
22 of 23 checks passed
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.

4 participants