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

feat: [M3-8099] - Improve Linode Clone Power Off based on initial data #10508

Merged
merged 10 commits into from
May 24, 2024

Conversation

mjac0bs
Copy link
Contributor

@mjac0bs mjac0bs commented May 22, 2024

Description 📝

We'd like to increase the percentage of users powering down their linodes before cloning to reduce the chance of data corruption. We'd also like to capture more analytics data to find out how many users are clicking the new "Power Off" button.

Changes 🔄

  • For both the v1 and v2 Clone flows:
    • Switched the order of the bullet points so that "To help avoid data corruption during the cloning process, we recommend powering off your Compute Instance prior to cloning." comes first in the list.
    • Bolded the text "avoid data corruption".
    • Added an analytics event firing on click of the Power Off button.
    • Users can see their linode's status and power it off in tablet and mobile view in the Linode Clone flow.
    • Added unit test coverage for SelectLinodeCard changes

Target release date 🗓️

6/10

Preview 📷

Before After
cloud linode com_linodes_create_linodeID=59147305 regionID=us-sea type=Clone+Linode typeID=g6-nanode-1 (1) localhost_3000_linodes_create_type=Backups
Screenshot 2024-05-22 at 2 09 53 PM Screenshot 2024-05-22 at 2 10 06 PM
N/A Screenshot 2024-05-22 at 2 05 35 PM

How to test 🧪

Prerequisites

(How to setup test environment)

  • Have at least two linodes on your account - one powered on and one powered off.
  • Have analytics logging enabled in the browser console.

Reproduction steps

(How to reproduce the issue, if applicable)

  • Go to https://cloud.linode.com/linodes/create?type=Clone%20Linode.
  • Change the screen to mobile view.
  • Observe there is no way to power off a selected, running linode when viewing selection cards.
  • Change the screen back to desktop view and select your powered on linode.
  • Click the Power Off button and observe in the browser console that there is no analytics event fired when the button is clicked.
  • Observe the notice copy has two bullets and may be easy to skim/ignore information about possible corruption.

Verification steps

(How to verify changes)

  • With this PR checked out, go to http://localhost:3000/linodes/create?type=Clone%20Linode.
  • Change the screen to mobile view.
  • Confirm that each linode's status is now displaying on the cards.
  • Select a powered on linode. Confirm that the selected card shows a Power Off button, and that clicking it powers off the linode and fires an analytics event.
  • Change the screen back to desktop view and select your powered on linode.
  • Click the Power Off button and confirm in the browser console that is also an analytics event fired when the button is clicked in the table view.
  • Observe the notice copy has switched the order of the bullets and bolded text to encourage users to read it.
  • Go to http://localhost:3000/linodes/create?type=Backups. (Turn the MSW on if you don't have any backups.) Change the screen to mobile view and confirm that status displays on the cards, but selecting a card of a powered on linode will not display the Power Off button.
  • Turn on the Linode Create v2 flag in the dev tools and confirm all the above changes are also present for the new version of the create flow.

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 May 22, 2024
@@ -197,21 +197,6 @@ describe('SelectLinodePanel (cards, mobile)', () => {
).toHaveTextContent(defaultProps.linodes[0].label);
});

it('displays the heading, notices and error', () => {
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 was a duplicate test the linter notified me of. See L99-L112.

@mjac0bs mjac0bs marked this pull request as ready for review May 22, 2024 22:08
@mjac0bs mjac0bs requested a review from a team as a code owner May 22, 2024 22:08
@mjac0bs mjac0bs requested review from jdamore-linode and abailly-akamai and removed request for a team May 22, 2024 22:08
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.

Nice and clean PR! Checks added underneath. I possibly did not test the right things?

  • go to http://localhost:3000/linodes/create?type=Clone%20Linode, Change the screen to mobile view. Confirm that each linode's status is now displaying on the cards. ✅
  • Select a powered on linode. Confirm that the selected card shows a Power Off button, and that clicking it powers off the linode and fires an analytics event. (❌ (maybe intentional?) it does work for both v1 and v2 flows on mobile, but on desktop the event does not fire in v2)
  • Go to http://localhost:3000/linodes/create?type=Backups. (Turn the MSW on if you don't have any backups.) Change the screen to mobile view and confirm that status displays on the cards, but selecting a card of a powered on linode will not display the Power Off button. ✅
  • Change the screen back to desktop view and select your powered on linode. Click the Power Off button and confirm in the browser console that is also an analytics event fired when the button is clicked in the table view. ❌ is that for Backups?? I don't see a power button at all in the backups flow)
  • Observe the notice copy has switched the order of the bullets and bolded text to encourage users to read it.
  • Turn on the Linode Create v2 flag in the dev tools and confirm all the above changes are also present for the new version of the create flow. ❌ see comment in bullet 2

Copy link

github-actions bot commented May 23, 2024

Coverage Report:
Base Coverage: 81.74%
Current Coverage: 81.73%

@mjac0bs
Copy link
Contributor Author

mjac0bs commented May 23, 2024

@abailly-akamai Thanks for the review:

  • Select a powered on linode. Confirm that the selected card shows a Power Off button, and that clicking it powers off the linode and fires an analytics event. (❌ (maybe intentional?) it does work for both v1 and v2 flows on mobile, but on desktop the event does not fire in v2)

This was a mistake - thank you for catching! The Power Off button has been tagged in v2 flow in a1e0053.

Screen.Recording.2024-05-23.at.8.12.08.AM.mov
  • Change the screen back to desktop view and select your powered on linode. Click the Power Off button and confirm in the browser console that is also an analytics event fired when the button is clicked in the table view. ❌ is that for Backups?? I don't see a power button at all in the backups flow)

My bad, sorry - my testing instructions were unclear because I went back and added the bullet about Backups after the fact and put it out of order. You were observing the intended behavior - no Power Off option (or analytics) for Backups, yes Power Off option (and analytics) for Clone. This should be the case for v1 and v2 flows. I've corrected the testing instructions.

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.

All good after fix and confirming all steps ✅

Nit: the alignment of the status icon/button feels a bit off
Screenshot 2024-05-23 at 10 49 26

@mjac0bs mjac0bs requested a review from jaalah-akamai May 23, 2024 15:37
@mjac0bs
Copy link
Contributor Author

mjac0bs commented May 24, 2024

@abailly-akamai How does this alignment look?

localhost_3000_linodes_create_type=Backups (6)

@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label May 24, 2024
@mjac0bs mjac0bs requested a review from hkhalil-akamai May 24, 2024 14:06
Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

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

👏

@jaalah-akamai jaalah-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels May 24, 2024
@abailly-akamai
Copy link
Contributor

Looks great @mjac0bs thx for the fixes - ship it 🚢

@mjac0bs mjac0bs merged commit 432909f into linode:develop May 24, 2024
18 checks passed
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.

3 participants