fix: [M3-7004] - Allow IPv6 ranges transfers #10156

abailly-akamai · 2024-02-06T22:38:21Z

Description 📝

This PR aims to fix a regression where users weren't able to transfer IPv6 ranges in the Linode Network tab.

I am frankly unsure how that was a regression considering we weren't pulling the ranges in the modal whatsoever, but perhaps someone has better context about this. Long story short, I hope my fix is the right one considering my lack of context. One thing I know is that adding those back to the modal works fine, and the transfer goes through.

This component is gross. It is a dirty ramda rat fest and the UI is questionable. While I wanted to only address the problem at heart, I couldn't help but attempt to improve the UI and make things at least look better for our users in the modal and add IP drawer.

Changes 🔄

Add (back?) IPvs ranges to the modal
Improve layout and styling in modal and drawer
Invalidate an extra query to improve the experience
Refactor default export

Preview 📷

Before	After

Before	After

Before	After

How to test 🧪

Prerequisites

Have two linodes in the same region without any extra IP address added (linode1, linode2)

Reproduction steps

Navigate to /linodes/{linode1.id}/networking
Add an IPv6 range (either /64 or /56)
Click on the "IP Transfer" Button
Notice your IPv6 range is not present

Verification steps

Stay on /linodes/{linode1.id}/networking
Pull code locally
Click on the "IP Transfer" Button
Notice your IPv6 range is present and can be transferred to linode2
Verify it gets removed from the IPs table without a refresh

Overall regression testing

Verify styling at all breakpoints
Verify IPv4 can be transferred just the same

As an Author I have considered 🤔

Check all that apply

abailly-akamai · 2024-02-08T03:39:26Z

packages/manager/src/features/Linodes/LinodesDetail/LinodeNetworking/IPTransfer.tsx

+    ...publicIPv4Addresses,
+    ...privateIPv4Addresses,
+    ...ipv6Addresses,
+  ];


So that's the gist of it. The report mentions a regression but curious to know how it ever worked without iterating over those 🤔

There's a chance I caused a regression with #9097, but I'm not sure this was working in the first place.

abailly-akamai · 2024-02-08T03:43:38Z

packages/manager/src/features/Linodes/LinodesDetail/LinodeNetworking/AddIPDrawer.tsx

+              />
+            ))}
+          </Box>
+        </StyledRadioGroup>


I frankly did not feel like abstracting the radio groups though they are pretty much identical. I just went for consolidated styling and I am at peace with it.

I changed the structure and added accessible support based on html recommendations which were slightly broken: https://www.w3.org/WAI/ARIA/apg/patterns/radio/examples/radio-rating/

packages/manager/src/queries/linodes/networking.ts

github-actions · 2024-02-08T03:57:08Z

Coverage Report: ✅
Base Coverage: 81.12%
Current Coverage: 81.12%

abailly-akamai · 2024-02-08T13:53:05Z

@jdamore-linode thoughts on covering this one?

bnussman-akamai · 2024-02-08T14:02:23Z

packages/manager/src/features/Linodes/LinodesDetail/LinodeNetworking/IPTransfer.tsx

+    ...publicIPv4Addresses,
+    ...privateIPv4Addresses,
+    ...ipv6Addresses,
+  ];


There's a chance I caused a regression with #9097, but I'm not sure this was working in the first place.

bnussman-akamai · 2024-02-08T14:06:23Z

packages/manager/src/queries/linodes/networking.ts

@@ -107,6 +108,7 @@ export const useLinodeShareIPMutation = () => {

 export const useAssignAdressesMutation = () => {
  const queryClient = useQueryClient();
+  const { location } = useHistory();


This approach might be fine but we may want to explore more robust ways.

If we ever use this mutation on a different page, it might not make the correct invalidations.

If we keep this, useParams might be better

const { linodeId } = useParams<{ linodeId: string }>();

You're right - I added a currentLinodeId required param to pass to useAssignAdressesMutation which solved our problem and is more robust. The mutation is only used in the IPTransfer modal but if eventually used elsewhere, i don't a reason why we wouldn't have access to the current linode ID we're migrating the IP address from.

packages/manager/src/queries/linodes/networking.ts

bnussman-akamai

Awesome. Good fixes and changes!

packages/manager/src/queries/linodes/networking.ts

dwiley-akamai

Able to successfully transfer IPv4 addresses and IPv6 ranges ✅
"IP Addresses" table reflects updates w/o refreshing the page ✅
Mobile view ✅

This is an existing issue in prod, but I noticed that if you fill out the "IP Transfer" (or "IP Sharing") dialog, exit it before submitting, and re-open it, it will be populated with the data you didn't submit. Not sure if we're doing that intentionally for some reason (we normally don't throughout the app)

jdamore-linode · 2024-02-08T18:22:58Z

@jdamore-linode thoughts on covering this one?

@abailly-akamai We definitely should! I think we should have Cypress coverage for pretty much every flow involving the Linode Details page (if not e2e, at least with mock data) and I don't believe we have any for IP management right now.

I wouldn't worry about handling that as part of this PR (unless you're feeling ambitious) -- I can write up some tickets for this and a few other things later this afternoon.

hana-akamai

I'm seeing a gray background color for the mobile view in the IP Transfer dialog

abailly-akamai · 2024-02-08T18:39:41Z

@dwiley-akamai This is indeed intentional (and not consistent with the rest of the app), mostly because we had gotten feedback users go back and forth between the table and modals to see additional details.

@hana-linode intentional as well. This modal has a unique layout and the mobile version needed a bit of TLC. Again, not really consistent but neither is this feature. Gonna leave it in there cause it adds clarity between rows.

@jdamore-linode tickets would be great, thank you!

abailly-akamai self-assigned this Feb 6, 2024

abailly-akamai added 2 commits February 7, 2024 14:04

Saving work

c95b9f1

Responsive changes

676ea62

abailly-akamai force-pushed the M3-7004 branch from 3d3c101 to 676ea62 Compare February 7, 2024 19:04

Query invalidation improvement

b2e678d

abailly-akamai changed the title ~~fix: [M3-7004] - Allow for IPv6 addresses transfer~~ fix: [M3-7004] - Allow for IPv6 ranges transfer Feb 7, 2024

abailly-akamai added 2 commits February 7, 2024 17:17

Improve add IP drawer

025c06a

Improve add IP drawer: styling

e015773

abailly-akamai marked this pull request as ready for review February 8, 2024 03:34

abailly-akamai requested a review from a team as a code owner February 8, 2024 03:34

abailly-akamai requested review from jdamore-linode and dwiley-akamai and removed request for a team February 8, 2024 03:34

Added changeset: Allow IPv6 ranges transfers

ec5078b

abailly-akamai changed the title ~~fix: [M3-7004] - Allow for IPv6 ranges transfer~~ fix: [M3-7004] - Allow IPv6 ranges transfers Feb 8, 2024

abailly-akamai commented Feb 8, 2024

View reviewed changes

abailly-akamai requested a review from hana-akamai February 8, 2024 13:52

bnussman-akamai reviewed Feb 8, 2024

View reviewed changes

Feedback

ab95206

bnussman-akamai approved these changes Feb 8, 2024

View reviewed changes

packages/manager/src/queries/linodes/networking.ts Outdated Show resolved Hide resolved

bnussman-akamai added Add'tl Approval Needed Waiting on another approval! Linodes Dealing with the Linodes section of the app labels Feb 8, 2024

Cleanup comment

676a038

dwiley-akamai approved these changes Feb 8, 2024

View reviewed changes

hana-akamai reviewed Feb 8, 2024

View reviewed changes

bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Feb 8, 2024

hana-akamai approved these changes Feb 8, 2024

View reviewed changes

abailly-akamai merged commit 48f3c13 into linode:develop Feb 8, 2024
17 of 18 checks passed

cpathipa mentioned this pull request Feb 12, 2024

Release v1.112.0 - release → staging #10174

Closed

This was referenced Feb 20, 2024

Release v1.113.0 - release → staging #10206

Merged

Release v1.113.0 – staging → master #10209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: [M3-7004] - Allow IPv6 ranges transfers #10156

fix: [M3-7004] - Allow IPv6 ranges transfers #10156

abailly-akamai commented Feb 6, 2024 •

edited

Loading

abailly-akamai Feb 8, 2024

bnussman-akamai Feb 8, 2024

abailly-akamai Feb 8, 2024 •

edited

Loading

github-actions bot commented Feb 8, 2024 •

edited

Loading

abailly-akamai commented Feb 8, 2024

bnussman-akamai Feb 8, 2024

bnussman-akamai Feb 8, 2024

bnussman-akamai Feb 8, 2024

abailly-akamai Feb 8, 2024 •

edited

Loading

bnussman-akamai left a comment

dwiley-akamai left a comment

jdamore-linode commented Feb 8, 2024

hana-akamai left a comment

abailly-akamai commented Feb 8, 2024

fix: [M3-7004] - Allow IPv6 ranges transfers #10156

fix: [M3-7004] - Allow IPv6 ranges transfers #10156

Conversation

abailly-akamai commented Feb 6, 2024 • edited Loading

Description 📝

Changes 🔄

Preview 📷

How to test 🧪

Prerequisites

Reproduction steps

Verification steps

Overall regression testing

As an Author I have considered 🤔

abailly-akamai Feb 8, 2024

Choose a reason for hiding this comment

bnussman-akamai Feb 8, 2024

Choose a reason for hiding this comment

abailly-akamai Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Feb 8, 2024 • edited Loading

abailly-akamai commented Feb 8, 2024

bnussman-akamai Feb 8, 2024

Choose a reason for hiding this comment

bnussman-akamai Feb 8, 2024

Choose a reason for hiding this comment

bnussman-akamai Feb 8, 2024

Choose a reason for hiding this comment

abailly-akamai Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

bnussman-akamai left a comment

Choose a reason for hiding this comment

dwiley-akamai left a comment

Choose a reason for hiding this comment

jdamore-linode commented Feb 8, 2024

hana-akamai left a comment

Choose a reason for hiding this comment

abailly-akamai commented Feb 8, 2024

abailly-akamai commented Feb 6, 2024 •

edited

Loading

abailly-akamai Feb 8, 2024 •

edited

Loading

github-actions bot commented Feb 8, 2024 •

edited

Loading

abailly-akamai Feb 8, 2024 •

edited

Loading