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

change: [M3-6838] - Better "Backups Enabled" default when cloning Linode #10959

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

Conversation

pmakode-akamai
Copy link
Contributor

@pmakode-akamai pmakode-akamai commented Sep 18, 2024

Description 📝

  • When cloning a Linode with backups enabled, the "Backups Enabled" checkbox should be checked by default (near the bottom of the Create flow)
  • When cloning a Linode with backups disabled, the "Backups Enabled" checkbox should be unchecked by default (near the bottom of the Create flow)

Changes 🔄

  • When cloning a Linode with backups enabled, the "Backups Enabled" checkbox is checked by default.
  • When cloning a Linode with backups disabled, the "Backups Enabled" checkbox is unchecked by default.

Target release date 🗓️

N/A

How to test 🧪

Reproduction steps

  • When cloning a Linode with backups enabled/disabled, the "Backups Enabled" checkbox is unchecked.

Verification steps

  • When cloning a Linode with backups enabled:
    • Verify the "Backups Enabled" checkbox should be checked by default.
    • Without changing the state of the "Backups Enabled" checkbox, the newly cloned Linode should have backups enabled.
  • When cloning a Linode with backups disabled:
    • the "Backups Enabled" checkbox should be unchecked by default.
    • Without changing the state of the "Backups Enabled" checkbox, the newly cloned Linode should have backups disabled.

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

@pmakode-akamai pmakode-akamai added Linodes Dealing with the Linodes section of the app Missing Changeset labels Sep 18, 2024
@pmakode-akamai pmakode-akamai self-assigned this Sep 18, 2024
@pmakode-akamai pmakode-akamai marked this pull request as ready for review September 18, 2024 11:48
@pmakode-akamai pmakode-akamai requested a review from a team as a code owner September 18, 2024 11:48
@pmakode-akamai pmakode-akamai requested review from bnussman-akamai and coliu-akamai and removed request for a team September 18, 2024 11:48
@pmakode-akamai pmakode-akamai marked this pull request as draft September 18, 2024 13:50
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.

Rather than calling setValue in a useEffect, we can set the value at the time the Linode is selected

I think this code will need to be updated:

const handleSelect = async (linode: Linode) => {
const hasPrivateIP = linode.ipv4.some((ipv4) => privateIPRegex.test(ipv4));
reset((prev) => ({
...prev,
backup_id: null,
linode,
private_ip: hasPrivateIP,
region: linode.region,
type: linode.type ?? '',
}));

We'll also want to update the form's initial values function to account for initializing values when the user deep links with a Linode preselected

@pmakode-akamai pmakode-akamai marked this pull request as ready for review September 18, 2024 15:25
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.

Looks good!

Confirmed that backups were enabled automatically when selecting a Linode that backups enabled ✅

@pmakode-akamai pmakode-akamai added the Add'tl Approval Needed Waiting on another approval! label Sep 18, 2024
Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

thanks Purvesh 🚀

✅ confirmed cloning a linode with backups enabled = backups is checked and cloned linode has backups enabled
✅ confirmed cloning a linode without backups = backups is unchecked and cloned linode doesn't have backups

Unrelated to this PR - I had Managed enabled, which automatically creates backups for Linodes (and backups can't be disabled while you have Managed) - should we consider creating tickets to

  • disable the Cancel Backups button
    image

  • have this checkbox automatically be checked (+ not able to be unchecked)
    image

for when Managed is enabled? (edit: + maybe a tooltip explaining why they're disabled)

@coliu-akamai coliu-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Sep 19, 2024
@pmakode-akamai
Copy link
Contributor Author

pmakode-akamai commented Sep 19, 2024

thanks Purvesh 🚀

✅ confirmed cloning a linode with backups enabled = backups is checked and cloned linode has backups enabled ✅ confirmed cloning a linode without backups = backups is unchecked and cloned linode doesn't have backups

Unrelated to this PR - I had Managed enabled, which automatically creates backups for Linodes (and backups can't be disabled while you have Managed) - should we consider creating tickets to

  • disable the Cancel Backups button
    image
  • have this checkbox automatically be checked (+ not able to be unchecked)
    image

for when Managed is enabled? (edit: + maybe a tooltip explaining why they're disabled)

Nice catch, Connie! 🙌🏻 I wasn’t aware of the Managed Linode functionality until your comment 😅.

After enabling Managed, I tried to cancel the Backups but encountered an error in the dialog. It’s subjective whether we should prevent users from clicking the Disable Backups button or allow them to see the error message when they click it in the dialog. I’m fine with keeping the "Cancel Backups" button disabled in the Backups tab, even though users won't be able to disable backups from the dialog.
Screenshot 2024-09-19 at 7 35 44 PM

Also, when cloning any Linode with Managed enabled, the checkbox will be checked by default for all Linodes. If a user unchecks it while cloning, the cloned Linode will still have backups enabled. In this case, it makes sense to keep the checkbox disabled 👍, even though the functionality doesn’t affect the cloned Linode when the user tries to clone it unchecked.

edit: Yes, we can have a separate ticket for this one. 💯

@coliu-akamai
Copy link
Contributor

coliu-akamai commented Sep 19, 2024

Ah apologies I should have specified 😅 - I was referring to the checkbox not being checked when creating a Linode. Everything works as expected when cloning 🎉

@pmakode-akamai
Copy link
Contributor Author

pmakode-akamai commented Sep 19, 2024

Ah apologies I should have specified 😅 - I was referring to the checkbox not being checked when creating a Linode. Everything works as expected when cloning 🎉

Yes, Everything works as expected when cloning.

I was referring to the checkbox not being checked when creating a Linode.

However, when 'Managed' is enabled, checkbox will always remain checked, right? Take note of the time when you uncheck it and click the 'Create Linode' button - it still clones the Linode with backup enabled (which is expected). I thought checked checkbox should be disabled when Managed is enabled.

@coliu-akamai
Copy link
Contributor

coliu-akamai commented Sep 19, 2024

messaging you off github to continue chatting!
edit: resolved async - clarified that this PR is good to merge as is, and that my other comments were referring to creating a new linode + is a nice to haves for later tickets

@pmakode-akamai pmakode-akamai force-pushed the M3-6838-better-backups-enabled-default-when-cloning-linode branch from e805a81 to 950a6e2 Compare September 20, 2024 06:25
Copy link

Coverage Report:
Base Coverage: 86.94%
Current Coverage: 86.94%

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! Linodes Dealing with the Linodes section of the app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants