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: [M3-7741] - Hide error notices for $0 regions in Resize Pool and Add a Node Pool drawers #10157

Conversation

mjac0bs
Copy link
Contributor

@mjac0bs mjac0bs commented Feb 7, 2024

Description 📝

This PR supports $0 region pricing by correctly showing pricing without errors on the LKE Details page. It also fixes the UI for invalid (null or undefined) prices in the Resize Pool drawer.

Currently, once a user creates a LKE cluster in a region where pricing is $0 (currently: Madrid, which is in LA):

When trying to resize pool, the UI displays a confusing 0 in place of the "Current pool" and "Resized pool" calculations, the pricing error is displayed, but a user is actually still able to complete the resize because the enhanced number input is still clickable and can be incremented, at which point the user can save changes.

When trying to add a node pool, after adding at lease one node, the UI displays a pricing error and pricing display text with $--.-- placeholder pricing in it. The user can still save changes.

Changes 🔄

Resize Pool:

  • The user is able to resize pool for an LKE cluster in a region where pricing is $0 and sees no error notice.
  • The user is not able to resize pool for an LKE cluster with pricing that is null or undefined.
  • If pricing is null or undefined, correctly display placeholder copy
    • Current pool: $--.–-/month (N node(s) at $--.--/month)
    • Resized pool: $--.–-/month (N node(s) at $--.--/month)
      and the pricing error rather than just 0.

Add a Node Pool:

  • The user is able to add nodes to an LKE cluster in a region where pricing is $0 and sees no error notice.
  • The user is not able to add nodes to an LKE cluster with pricing that is null or undefined.
  • If pricing is null or undefined, correctly display placeholder copy
    • This pool will add $--.--/month (N node(s) at $--.--/month) to this cluster.

Preview 📷

Description Before After
Resize Pool: Region with $0 pricing (currently Madrid) Screenshot 2024-02-06 at 3 04 04 PM Screenshot 2024-02-07 at 8 19 57 AM
Resize Pool: undefined or null pricing Screenshot 2024-02-07 at 9 28 16 AM Screenshot 2024-02-07 at 9 28 57 AM
Add a Node Pool: Region with $0 pricing (currently Madrid) Screenshot 2024-02-06 at 5 18 32 PM Screenshot 2024-02-07 at 8 20 11 AM

How to test 🧪

Prerequisites

(How to setup test environment)

  • Check out this PR and yarn dev.
  • Create a LKE cluster with es-madrid as its region.

Reproduction steps

(How to reproduce the issue, if applicable)

Resize Pool:

  • Navigate to the cluster's details page and click Resize Pool.
  • Observe the described/screenshotted issues with the drawer.

Add a Node Pool:

  • Navigate to the cluster's details page and click Add a Node Pool.
  • Add at least one node to the pool (press the + icon)
  • Observe the described/screenshotted issues with the drawer.

Verification steps

(How to verify changes)

  • On this branch, follow the repro steps and observe that there are no errors for a region with $0 pricing when Resizing Pool or Adding a Node Pool.
  • To test behavior with pricing that is undefined (same behavior as null), block network requests to the linode type (example: v4/linode/types/g6-standard-1) or manually edit the values of pricePerNode and totalMonthlyPrice (here, here).
  • Confirm test passes with added coverage for resizing and adding to node pool in $0 regions:
yarn cy:run -s "cypress/e2e/core/kubernetes/lke-update.spec.ts"

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

@mjac0bs mjac0bs self-assigned this Feb 7, 2024
<Typography className={classes.summary}>
{`Resized pool: $${renderMonthlyPriceToCorrectDecimalPlace(
updatedCount * pricePerNode
!isInvalidPricePerNode ? updatedCount * pricePerNode : undefined
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the pricePerNode could be null or undefined, we can't multiply a non-number. renderMonthlyPriceToCorrectDecimalPlace() has logic to render a non-numeric price as $--.--.

variant="error"

{nodePool.count &&
(isInvalidPricePerNode || isInvalidTotalMonthlyPrice) && (
Copy link
Contributor Author

@mjac0bs mjac0bs Feb 7, 2024

Choose a reason for hiding this comment

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

Only real change in this section is the updated bools to determine whether to display the pricing error notice or not - the rest of the diff is due to indentation.

Comment on lines +120 to +129
{isLoadingTypes ? (
<CircleProgress />
) : (
<form
onSubmit={(e: React.ChangeEvent<HTMLFormElement>) => {
e.preventDefault();
handleSubmit();
}}
>
<div className={classes.section}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of this diff is an indentation change caused by fixing an issue where the loading spinner was displaying above the form, instead of in its place, while isLoadingTypes is true. This was visible when I blocked network requests as described in the testing steps.

Before After
Screen.Recording.2024-02-07.at.9.01.57.AM.mov
Screen.Recording.2024-02-07.at.9.01.12.AM.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes in the file are to account for the loading state that is now present before form data is rendered.

Comment on lines 110 to 113
const isInvalidPricePerNode = !pricePerNode && pricePerNode !== 0;
const isInvalidTotalMonthlyPrice =
!totalMonthlyPrice && totalMonthlyPrice !== 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.

undefined and null prices are invalid, but 0 is allowed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to have this clarification as a comment in the code

Copy link
Contributor

Choose a reason for hiding this comment

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

I second this. Am also thinking you could consolidate this logic with the addNodePool drawer logic in a util with your comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 6b86e9a.

@mjac0bs mjac0bs changed the title fix: [M3-7741] - Allow $0 LKE prices without error notices in Resize Pool and Add a Node Pool drawers fix: [M3-7741] - Hide error notices for $0 regions in Resize Pool and Add a Node Pool drawers Feb 7, 2024
@mjac0bs mjac0bs marked this pull request as ready for review February 7, 2024 18:00
@mjac0bs mjac0bs requested review from a team as code owners February 7, 2024 18:00
@mjac0bs mjac0bs requested review from cliu-akamai, dwiley-akamai and abailly-akamai and removed request for a team February 7, 2024 18:00
@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Feb 7, 2024
Copy link

github-actions bot commented Feb 7, 2024

Coverage Report:
Base Coverage: 81.11%
Current Coverage: 81.11%

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Observed the behavior described in the Changes section for each circumstance (opted to manually modify the values of pricePerNode and totalMonthlyPrice) ✅

lke-update.spec.ts passed ✅

Comment on lines 110 to 113
const isInvalidPricePerNode = !pricePerNode && pricePerNode !== 0;
const isInvalidTotalMonthlyPrice =
!totalMonthlyPrice && totalMonthlyPrice !== 0;

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to have this clarification as a comment in the code

Copy link
Contributor

@abailly-akamai abailly-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 and confirmed the fixes shown in the description ✅

Thanks for the improved coverage as well!

Bonus point if you wanna do that minor refactor

Comment on lines 110 to 113
const isInvalidPricePerNode = !pricePerNode && pricePerNode !== 0;
const isInvalidTotalMonthlyPrice =
!totalMonthlyPrice && totalMonthlyPrice !== 0;

Copy link
Contributor

Choose a reason for hiding this comment

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

I second this. Am also thinking you could consolidate this logic with the addNodePool drawer logic in a util with your comments.

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Feb 8, 2024
@mjac0bs mjac0bs merged commit 49ea132 into linode:develop Feb 8, 2024
18 checks passed
jdamore-linode pushed a commit to jdamore-linode/manager that referenced this pull request Feb 12, 2024
… Add a Node Pool drawers (linode#10157)

* Allow -zsh LKE prices without error notices in Resize Pool and Add Pool drawers

* Fix loading spinner displaying above what was supposed to be loading

* Fix conditional to render notice if either price is invalid

* Add test coverage

* Added changeset: Hide error notices for /bin/sh regions for LKE Resize and Add Node Pools

* Fix changeset wording

* Address feedback: use invalid price util
jdamore-linode added a commit that referenced this pull request Feb 12, 2024
…xes for release (#10177)

* fix: [M3-7741] - Hide error notices for $0 regions in Resize Pool and Add a Node Pool drawers (#10157)

* Allow -zsh LKE prices without error notices in Resize Pool and Add Pool drawers

* Fix loading spinner displaying above what was supposed to be loading

* Fix conditional to render notice if either price is invalid

* Add test coverage

* Added changeset: Hide error notices for /bin/sh regions for LKE Resize and Add Node Pools

* Fix changeset wording

* Address feedback: use invalid price util

* fix: [M3-7746] - Fix $0 region price error in "Enable All Backups" drawer (#10161)

* Remove error indicator for Linodes in $0 regions

* Fix $0 total price display issue

* Cover $0 pricing cases in Cypress backup tests

* Add BackupLinodeRow tests to account for error states and $0 regions

* Add unit tests for BackupDrawer component

* fix: [M3-7747] - Fix Linode Migration dialog hidden $0 price (#10166)

* Add unit tests for MigrationPricing component

* Accounting for $0 prices in MigrationPricing component

* fix: [M3-7739] - Fix error when enabling backups for Linodes in regions with $0 price (#10153)

* Fix error when enabling backups for Linodes in regions with $0 price

* Add unit tests for EnableBackupsDialog

---------

Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>

* Replace "toBeDisabled" with "toHaveAttribute" assertion

---------

Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants