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-7746] - Fix $0 region price error in "Enable All Backups" drawer #10161

Conversation

jdamore-linode
Copy link
Contributor

Description πŸ“

This fixes a couple issues in the "Enable All Backups" drawer when one or more Linodes are in a $0-priced region.

  • Fixes calculation of total backup cost
  • Removes error indicator

Changes πŸ”„

List any change relevant to the reviewer.

  • Fixes calculation of total backup price when one or more Linodes is in a $0 region
  • Removes error indicator from backup rows for Linodes in $0 regions
  • Adds and updates tests to account for these states

Preview πŸ“·

Include a screenshot or screen recording of the change

πŸ’‘ Use <video src="" /> tag when including recordings in table.

Before After
Screenshot 2024-02-07 at 1 33 36 PM Screenshot 2024-02-07 at 4 18 30 PM

How to test πŸ§ͺ

We should be able to rely on the unit tests and Cypress tests to cover this, since getting the "Enable All Linode Backups" drawer to appear can be difficult depending on the state of your account.

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

@jdamore-linode jdamore-linode self-assigned this Feb 7, 2024
@jdamore-linode jdamore-linode requested review from a team as code owners February 7, 2024 21:21
@jdamore-linode jdamore-linode requested review from cliu-akamai, bnussman-akamai and jaalah-akamai and removed request for a team February 7, 2024 21:21
@mjac0bs mjac0bs self-requested a review February 7, 2024 21:21
getMonthlyBackupsPrice({
region: linode.region,
type,
}) || 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.

This || undefined caused monthly prices of 0 to get returned as undefined here

@jdamore-linode jdamore-linode changed the title fix: [M3-7746] - Fix $0 region price error indicator in "Enable All Backups" drawer fix: [M3-7746] - Fix $0 region price error in "Enable All Backups" drawer Feb 7, 2024
Copy link
Contributor

@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.

Nice!

Love the unit tests 🧼

Copy link

github-actions bot commented Feb 7, 2024

Coverage Report: βœ…
Base Coverage: 81.11%
Current Coverage: 81.12%

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 saw a case where the total wasn't be displayed as expected if the totalBackupsPrice is 0 in BackupDrawer.tsx.

If we remove the check for totalBackupsPrice and just go with:

            price={
              isNumber(totalBackupsPrice) ? totalBackupsPrice : UNKNOWN_PRICE
            }

we do see $0 for a total.

Before After
Screenshot 2024-02-07 at 2 45 06 PM Screenshot 2024-02-07 at 2 46 09 PM

packages/manager/src/features/Backups/BackupLinodeRow.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Backups/BackupLinodeRow.tsx Outdated Show resolved Hide resolved
@jdamore-linode
Copy link
Contributor Author

jdamore-linode commented Feb 7, 2024

I saw a case where the total wasn't be displayed as expected if the totalBackupsPrice is 0 in BackupDrawer.tsx.

Oh awesome catch, thanks @mjac0bs! I think this even warrants its own test case, so I'll handle that all at once tomorrow.

Also totally agreed on that const name, I'll take care of that too πŸ‘ Thank you!

@jdamore-linode
Copy link
Contributor Author

@mjac0bs Just pushed a fix for that $0 total price issue, as well as some unit tests for the BackupDrawer component

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.

Awesome, thanks for the unit test coverage of the drawer! Manually confirmed the total of the drawer now displays $0.00 for a total price of backups, as well. 🚒

@mjac0bs mjac0bs removed Add'tl Approval Needed Waiting on another approval! Ready for Review labels Feb 8, 2024
@mjac0bs mjac0bs added the Approved Multiple approvals and ready to merge! label Feb 8, 2024
@jdamore-linode jdamore-linode merged commit b31f6b1 into linode:develop Feb 8, 2024
18 checks passed
jdamore-linode added a commit to jdamore-linode/manager that referenced this pull request Feb 12, 2024
…awer (linode#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
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.

None yet

3 participants