feat: [M3-7254] - Add AGLB Routes - Rule Edit Drawer #9778

bnussman-akamai · 2023-10-10T17:26:12Z

Description 📝

Allows users to edit AGLB Route rules ✏️

Preview 📷

How to test 🧪

Prerequisites

Turn on the MSW

Verification steps

Go to http://localhost:3000/loadbalancers/0/routes
Test the general functionality of editing a rule
Verify you can still add a rule
Verify you can still re-order rules

As an Author I have considered 🤔

Check all that apply

bnussman-akamai · 2023-10-10T18:18:12Z

packages/manager/src/components/Notice/Notice.styles.ts

    alignItems: 'center',
-    backgroundColor: theme.bg.bgPaper,


This caused a UI regression. I don't think we want notices to have a background unless they are important

bnussman-akamai · 2023-10-10T18:20:40Z

packages/manager/src/features/LoadBalancers/LoadBalancerDetail/RulesTable.tsx

-                            rule.match_condition.hostname ??
-                            `Rule ${rule.match_condition.hostname}`
-                          }
+                          aria-label={`Rule ${index}`}


Is this change okay for accessibility? Rules don't have a solid way to uniquely identify them so I think index is best here

Since order matters and hostname isn't unique, then I think the change makes sense.

bnussman-akamai · 2023-10-10T18:20:56Z

packages/manager/src/features/LoadBalancers/LoadBalancerDetail/RulesTable.tsx

@@ -253,7 +250,7 @@ export const RulesTable = ({ loadbalancerId, route }: Props) => {
                                }}
                              >
                                {rule.match_condition
-                                  .session_stickiness_cookie &&
+                                  .session_stickiness_cookie ||


Fixes incorrect logic

mjac0bs

Overall, adding, editing, and reordering rules looks good. The drawer works in add and edit mode. The UI updates accordingly, populates the form with data in edit mode, and displays the updates in the table after the request is submitted.

A few questions (sorry if these have already been addressed in past PRs -- would appreciate a reminder):

Related directly to this PR:

Is the API spec for the request payload for PUT /v4beta/aglb/{id}/routes/{id} out of date? I noticed some slight discrepancies.

API spec includes route label in the request; our request payload in Cloud does not.
API spec does not include service target label in the request; our payload in Cloud does.

Not directly related to this PR:

Have we talked about expanding the width of this tooltip? It's a lot of text and a little hard to read with how thin it is.

Should we add some logic to not display the tooltip on rules where there are no service targets? Currently, we display a blank tooltip popover, which is a little weird.

mjac0bs · 2023-10-11T18:00:05Z

packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Routes/RuleDrawer.tsx

Can we fix the typo in the helper text for TCP rules?

"A TCP rule consists a percent allocation to a Service Target." -> "A TCP rule consists of a percent allocation to a Service Target."

mjac0bs · 2023-10-11T19:32:28Z

packages/manager/src/features/LoadBalancers/LoadBalancerDetail/RulesTable.tsx

-                            rule.match_condition.hostname ??
-                            `Rule ${rule.match_condition.hostname}`
-                          }
+                          aria-label={`Rule ${index}`}


Since order matters and hostname isn't unique, then I think the change makes sense.

bnussman-akamai · 2023-10-11T20:01:16Z

@mjac0bs

We do have label in the PUT request type but we don't need it in the actual requets. All Linode API PUTs assume all fields are optional. You are correct about the fact service target should not have label as a type but I hesitate to fix it as part of this PR because it will add a significant amount of code.
I wanted to expand the tooltip but at the time of its creation it wasn't possible. @abailly-akamai said I should be able to widen it after feat: [M3-7114] - Allow user to choose resize migration type when resizing Linode #9677 but I don't think this is true because feat: [M3-7114] - Allow user to choose resize migration type when resizing Linode #9677 didn't touch the TextField component
I'll see if I can add something for the empty state or just hide the tooltip

mjac0bs

@bnussman-akamai
Approving, pending the typo I mentioned is addressed and follow up tickets are created.

We do have label in the PUT request type but we don't need it in the actual requets. All Linode API PUTs assume all fields are optional.

Oops, yeah, you're right!

You are correct about the fact service target should not have label as a type but I hesitate to fix it as part of this PR because it will add a significant amount of code.

That's fair and a good call about not wanting to expand the scope of this PR. Can we make a follow up ticket for this?

I wanted to expand the tooltip but at the time of its creation it wasn't possible. @abailly-akamai said I should be able to widen it after feat: [M3-7114] - Allow user to choose resize migration type when resizing Linode #9677 but I don't think this is true because feat: [M3-7114] - Allow user to choose resize migration type when resizing Linode #9677 didn't touch the TextField component

Good context -- thank you for looking into and linking. I felt like this had been discussed before, but didn't remember exactly where. TBH, I don't understand how this styled component is working, but I see it in Storybook... a template literal tacked on after the styled function? Didn't know you could do that.

It looks like TextfField has a tooltipClasses prop and we've used that in the past to pass in a minWidth... but from what I can tell, it seems like this is not actually working on the label field Database Create form.

Anyway, this is minor and not worth holding up the PR over or expanding the scope too much. (Unless @jaalah-akamai can help do this cleanly.) A follow up ticket would be nice to address at some point in the future since it sounds like we're in consensus that wider is better.

I'll see if I can add something for the empty state or just hide the tooltip

Empty state of "None" looks good now; thanks for the add.

cpathipa

Overall LGTM!

Verified the functionality of editing a rule
Verified add rule
Verified re-order rules

* initial edit flow and e2e test * fix notice regression * a few small fixes * Added changeset: Add AGLB Routes - Rule Edit Drawer * add empty state for no service targets --------- Co-authored-by: Banks Nussman <banks@nussman.us>

bnussman added 2 commits October 10, 2023 12:56

initial edit flow and e2e test

5d8d563

fix notice regression

97a00a7

bnussman-akamai added the ACLB Relating to the Akamai Cloud Load Balancer label Oct 10, 2023

bnussman-akamai self-assigned this Oct 10, 2023

a few small fixes

b9cf65a

bnussman-akamai marked this pull request as ready for review October 10, 2023 18:06

bnussman-akamai requested a review from a team as a code owner October 10, 2023 18:06

bnussman-akamai requested review from mjac0bs and hana-akamai and removed request for a team October 10, 2023 18:06

Added changeset: Add AGLB Routes - Rule Edit Drawer

e0d7b0f

bnussman-akamai commented Oct 10, 2023

View reviewed changes

mjac0bs reviewed Oct 11, 2023

View reviewed changes

bnussman added 2 commits October 11, 2023 16:21

Merge branch 'develop' into M3-7254-aglb-edit-rule-drawer

7a8505a

add empty state for no service targets

b5ca24f

mjac0bs approved these changes Oct 11, 2023

View reviewed changes

mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Oct 12, 2023

Merge branch 'develop' into M3-7254-aglb-edit-rule-drawer

acf7c93

bnussman-akamai requested a review from cpathipa October 17, 2023 14:24

cpathipa approved these changes Oct 17, 2023

View reviewed changes

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

bnussman-akamai merged commit 609c64b into linode:develop Oct 17, 2023

This was referenced Oct 26, 2023

Release v1.106.0 - release → staging #9842

Merged

Release v1.106.0 - staging → master #9850

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [M3-7254] - Add AGLB Routes - Rule Edit Drawer #9778

feat: [M3-7254] - Add AGLB Routes - Rule Edit Drawer #9778

bnussman-akamai commented Oct 10, 2023 •

edited

Loading

bnussman-akamai Oct 10, 2023

bnussman-akamai Oct 10, 2023

mjac0bs Oct 11, 2023

bnussman-akamai Oct 10, 2023

mjac0bs left a comment •

edited

Loading

mjac0bs Oct 11, 2023

mjac0bs Oct 11, 2023

bnussman-akamai commented Oct 11, 2023 •

edited

Loading

mjac0bs left a comment

cpathipa left a comment

feat: [M3-7254] - Add AGLB Routes - Rule Edit Drawer #9778

feat: [M3-7254] - Add AGLB Routes - Rule Edit Drawer #9778

Conversation

bnussman-akamai commented Oct 10, 2023 • edited Loading

Description 📝

Preview 📷

How to test 🧪

Prerequisites

Verification steps

As an Author I have considered 🤔

bnussman-akamai Oct 10, 2023

Choose a reason for hiding this comment

bnussman-akamai Oct 10, 2023

Choose a reason for hiding this comment

mjac0bs Oct 11, 2023

Choose a reason for hiding this comment

bnussman-akamai Oct 10, 2023

Choose a reason for hiding this comment

mjac0bs left a comment • edited Loading

Choose a reason for hiding this comment

mjac0bs Oct 11, 2023

Choose a reason for hiding this comment

mjac0bs Oct 11, 2023

Choose a reason for hiding this comment

bnussman-akamai commented Oct 11, 2023 • edited Loading

mjac0bs left a comment

Choose a reason for hiding this comment

cpathipa left a comment

Choose a reason for hiding this comment

bnussman-akamai commented Oct 10, 2023 •

edited

Loading

mjac0bs left a comment •

edited

Loading

bnussman-akamai commented Oct 11, 2023 •

edited

Loading