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-7739] - Fix error when enabling backups for Linodes in regions with $0 price #10153

Conversation

jdamore-linode
Copy link
Contributor

@jdamore-linode jdamore-linode commented Feb 6, 2024

Description 📝

This fixes an error message that appears when enabling Linode backups in regions with $0 prices.

Changes 🔄

  • Only show the error message if the retrieved backup price is null or undefined, but allow 0

How to test 🧪

Automated Tests

Serve Cloud with yarn && yarn build && yarn start:manager:ci, and then:

CY_TEST_REGION='es-mad' yarn cy:run -s "cypress/e2e/core/linodes/backup-linode.spec.ts"

(Note: This will only work on accounts that have access to the es-mad region, and won't be properly validated by our CI)

Manual

  1. Create a Linode in the es-mad region
  2. Navigate to its details page, then click the "Backups" tab
  3. Attempt to enable backups and confirm that an error message does not appear on the confirmation dialog and that you're able to enable backups successfully

(Edit: You can also test that the error message still appears when expected by blocking the request to the /linode/types/:type endpoint via Chrome's dev tools)

Reproduction steps

You can reproduce the error with the tests if develop is checked out. yarn && yarn build && yarn start:manager:ci, and then:

CY_TEST_REGION='es-mad' yarn cy:run -s "cypress/e2e/core/linodes/backup-linode.spec.ts"

(Note: This will only work on accounts that have access to the es-mad region)

Similarly, you can follow the steps above to reproduce the issue manually.

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 6, 2024
@jdamore-linode jdamore-linode requested a review from a team as a code owner February 6, 2024 19:54
@jdamore-linode jdamore-linode requested review from mjac0bs and carrillo-erik and removed request for a team February 6, 2024 19:54
Comment on lines +50 to +51
const hasBackupsMonthlyPriceError =
!backupsMonthlyPrice && backupsMonthlyPrice !== 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.

Wasn't 100% sure what the clearest way to express this is

Opted for this since it telegraphs that 0 is a valid value, but if something else would be clearer to the team, like checking explicitly for null or undefined, I'm happy to tweak this.

Copy link
Contributor

Choose a reason for hiding this comment

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

No objections from me.

Copy link

github-actions bot commented Feb 6, 2024

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

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.

Soooo, about that test coverage. 😆

Thanks again for the catch and the fix here, Joe. Let's cover the $0 case in backup-linode.spec in your open test PR?

✅ I can enable backups for a Madrid Linode:
Screenshot 2024-02-07 at 2 04 05 PM

Comment on lines +50 to +51
const hasBackupsMonthlyPriceError =
!backupsMonthlyPrice && backupsMonthlyPrice !== 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

No objections from me.

@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Feb 7, 2024
@jdamore-linode
Copy link
Contributor Author

Soooo, about that test coverage. 😆

Shakes fist

jdamore-linode and others added 2 commits February 7, 2024 16:27
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
@jaalah-akamai jaalah-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Feb 9, 2024
@jdamore-linode jdamore-linode merged commit e68bbb0 into linode:develop Feb 9, 2024
18 checks passed
jdamore-linode added a commit to jdamore-linode/manager that referenced this pull request Feb 12, 2024
…ns with $0 price (linode#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>
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! Ready for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants