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

upcoming: [M3-9355] - Fix LKE-E provisioning placeholder when filtering by status #11745

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

hana-akamai
Copy link
Contributor

Description πŸ“

Fix provisioning placeholder incorrectly displaying when filtering a newly created Cluster by Running/Offline status

Target release date πŸ—“οΈ

3/11

Preview πŸ“·

Before After
before.mp4
after.mov

How to test πŸ§ͺ

Prerequisites

(How to setup test environment)

  • Ensure your account has LKE-E customer tags (see project tracker) or verify the fix via e2e test

Reproduction steps

You can test via e2e or create an actual LKE-E cluster and wait ~5mins for it to start provisioning

  • Test via e2e:
    • In NodeTable.tsx replace rowData.length back with count on lines 192 & 218
    • yarn cy:debug and run the lke-update.spec.ts test
    • The filters the node tables based on selected status filter test should fail
  • Test via actual LKE-E cluster:
    • Create a LKE-E cluster and wait ~5mins for the nodes to start provisioning
    • Filter by Running/Offline status, notice the provisioning placeholder incorrectly displaying

Verification steps

(How to verify changes)

  • Test via e2e:
    • yarn cy:debug and run the lke-update.spec.ts test
    • filters the node tables based on selected status filter should pass
  • Test via actual LKE-E cluster:
    • Create a LKE-E cluster and wait ~5mins for the nodes to start provisioning
    • Filter by Running/Offline status, the provisioning placeholder should not be displaying anymore
Author Checklists

As an Author, to speed up the review process, I 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

@hana-akamai hana-akamai added Bug Fixes for regressions or bugs LKE-Enterprise labels Feb 26, 2025
@hana-akamai hana-akamai self-assigned this Feb 26, 2025
@hana-akamai hana-akamai marked this pull request as ready for review February 27, 2025 16:12
@hana-akamai hana-akamai requested review from a team as code owners February 27, 2025 16:12
@hana-akamai hana-akamai requested review from dmcintyr-akamai, hkhalil-akamai, cliu-akamai and mjac0bs and removed request for a team February 27, 2025 16:12
</TableRow>
)}
{(count > 0 ||
{rowData.length === 0 &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to check the length of the original data, not the filtered data bc the filtered data could be zero and this will incorrectly show

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense - thanks for the fix!

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.

Looking good now! Glad we caught this one; it was a bit of a hidden issue. 🚒

Screen.Recording.2025-02-27.at.8.29.13.AM.mov

</TableRow>
)}
{(count > 0 ||
{rowData.length === 0 &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense - thanks for the fix!

@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Feb 27, 2025
@mjac0bs
Copy link
Contributor

mjac0bs commented Feb 27, 2025

@hana-akamai Just merged the copy PR, which is going to create a small merge conflict with that placeholder text.

Copy link

Coverage Report: ❌
Base Coverage: 80.18%
Current Coverage: 80.17%

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

πŸŽ‰ 530 passing tests on test run #3 β†—οΈŽ

❌ Failingβœ… Passingβ†ͺ️ SkippedπŸ• Duration
0 Failing530 Passing3 Skipped115m 37s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Add'tl Approval Needed Waiting on another approval! Bug Fixes for regressions or bugs LKE-Enterprise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants