fix: [M3-7313] - Migrated Linode dropdown to Autocomplete in Firewall Landing > Create Firewall drawer by tyler-akamai · Pull Request #9837 · linode/manager

tyler-akamai · 2023-10-24T18:20:40Z

Description 📝

Migrated the Linode dropdown in Firewall Landing > Create Firewall drawer from the LinodeSelect component to the Autocomplete component

Changes 🔄

Migrated Linode Dropdown from LinodeSelect to the Autocomplete component
Fixed NodeBalancer dropdown incorrect highlighted options
Added successful toast notification for firewall creation

Preview 📷

Before	After
Screen.Recording.2023-10-24.at.2.47.30.PM.mov	Screen.Recording.2023-10-24.at.2.35.56.PM.mov

How to test 🧪

Prerequisites

Checkout the feature branch (feature/firewall-nodebalancer)

Reproduction steps

Navigate to http://localhost:3000/firewalls/create
Select a NodeBalancer, the correct NodeBalancer is created, but all of the options are highlighted as if they were chosen.
Create the firewall, there should not be a successful toast notification

Verification steps

Navigate to http://localhost:3000/firewalls/create

Ensure the Linode dropdown and NodeBalancer dropdown are consistent
Ensure the dropdowns work as expected
Ensure there is a successful toast notification when the Firewall is created
Ensure the firewall is assigned to the selected NodeBalancers and Linodes

As an Author I have considered 🤔

Check all that apply

…, added successfull toast notification

…deSelect component instead of the Autocomplete component

tyler-akamai · 2023-10-24T18:29:43Z

packages/manager/.changeset/pr-9785-fixed-1697059201989.md

 ---

-Firewall Create Drawer opens in the same tab in the Linode Create Flow ([#9785](https://github.com/linode/manager/pull/9785))
+Firewall Create Drawer opening in a new tab in the Linode Create Flow ([#9785](https://github.com/linode/manager/pull/9785))


@mjac0bs made a good suggestion with the changeset message on my last PR, but I forgot to fix it.

tyler-akamai · 2023-10-24T18:32:12Z

packages/manager/src/features/Firewalls/FirewallLanding/CreateFirewallDrawer.tsx

      isLoading: nodebalancerIsLoading,
    } = useAllNodeBalancersQuery();

+    const {


This query used to be called in the LinodeSelect component but now since we are using the universal Autocomplete component, we need to call it in this component instead.

tyler-akamai · 2023-10-24T18:33:32Z

packages/manager/src/features/Firewalls/FirewallLanding/CreateFirewallDrawer.tsx


-    const optionsFilter = (nodebalancer: NodeBalancer) => {
-      const selectedNodeBalancersIds = selectedNodeBalancers.map((nb) => nb.id);
+    const optionsFilter = (


I updated this optionsFilter function to filter NodeBalancers and Linodes

carrillo-erik · 2023-10-25T16:26:23Z

Looks great and the NodeBalancer selection bug has been addressed as well. Nice job ✅

hana-akamai

Functionality LGTM, noticed one small thing. The selected linodes don't get cleared after clicking cancel

Screen.Recording.2023-10-26.at.12.06.11.PM.mov

tyler-akamai · 2023-10-26T18:47:14Z

Functionality LGTM, noticed one small thing. The selected linodes don't get cleared after clicking cancel

Screen.Recording.2023-10-26.at.12.06.11.PM.mov

Good catch! just fixed

bnussman-akamai · 2023-10-26T20:34:51Z

packages/manager/src/features/Firewalls/FirewallLanding/CreateFirewallDrawer.tsx

      NodeBalancer[]
    >([]);

+    const [selectedLinodes, setSelectedLinodes] = React.useState<Linode[]>([]);


This seems like duplicate state. We have the selected Linode IDs in formik so we probably don't want it duplicated here.

The issue that I was running into was that values.devices.linodes is a list of IDs (numbers). For the Autocomplete value attribute, you need a list of labels ( string ), or a list of objects (Linodes) with a label: String field.

So in order to use values.devices.linodes, you would have to convert it from a list of numbers to a list of Linodes (a list of Linodes gives us access to both the ID and Label).

One way to do this is to take the already existing linodes list used for the Autocomplete's options attribute. We can then take linodes and then filter it so only the linodes that contain the selected IDs (values.devices.linodes) remain. It would look something like this:

const selectedLinodes = linodes.filter(linode => values.devices.linodes.includes(linode.id));

But I thought just adding a state 'selectedLinodes' would be a cleaner option.

Any thoughts @bnussman-akamai

I like the filter idea!

tyler-akamai · 2023-10-30T14:39:40Z

packages/manager/src/features/Firewalls/FirewallLanding/CreateFirewallDrawer.tsx

      optionsFilter(linode.id, 'linode')
    );

+    const linodeIdSet = new Set(values?.devices?.linodes);


If we use two lists, Big O would be O(n*m) where n is the number of linodes for a given account and m is the number that is selected from that list of linodes

By converting the values.devices.linodes (selectedLinodes) array into a Set, Big O goes from O(n*m) to just O(n)

packages/manager/src/features/Firewalls/FirewallLanding/CreateFirewallDrawer.tsx

bnussman-akamai

Functionality looks good!

bnussman-akamai · 2023-10-30T16:24:35Z

packages/manager/src/features/Firewalls/FirewallLanding/CreateFirewallDrawer.tsx

+    const selectedLinodes: Linode[] =
+      linodes?.filter((linode) => linodeIdSet.has(linode.id)) || [];
+
+    const nodebalancerIdSet = new Set(values?.devices?.nodebalancers);


I wonder how expensive it is to create these Sets on every render?

Just did some research. Sets are relatively inexpensive if we put it behind a useMemo and only update it if the user selects or deselects a Linode/NodeBalancer.

However, I was comparing using a List vs a Set and it seems like a Set is a better option if the list is continuously changing and a List is better if there are not many changes or its a one-off change. Since the user would most likely be either selecting limited number of Linodes/NodeBalancers at once, or hitting the Select All button, I think the usage better aligns with a List instead of a Set.

bnussman-akamai · 2023-10-30T16:25:48Z

packages/manager/src/features/Firewalls/FirewallLanding/CreateFirewallDrawer.tsx

+            })}
+            disabled={userCannotAddFirewall || !!linodeError}
            errorText={errors['devices.linodes']}
-            helperText={FIREWALL_HELPER_TEXT}


Are we loosing this helper text?

yeah I compared it to the mockups, and realized the mockups dont have it.

bnussman-akamai · 2023-10-30T16:26:37Z

packages/manager/src/features/Firewalls/FirewallLanding/CreateFirewallDrawer.tsx

              {learnMoreLink}
            </Typography>
          </Box>
-          <LinodeSelect


I personally like the LinodeSelect abstraction as opposed to using Autcompeletes directly, but I don't want to block this work.

This may need a cafe discussion

Yeah this is a good topic, I will add it to the agenda.

hana-akamai

Ensure the Linode dropdown and NodeBalancer dropdown are consistent
Ensure the dropdowns work as expected
Ensure there is a successful toast notification when the Firewall is created
Ensure the firewall is assigned to the selected NodeBalancers and Linodes

* feat: [M3 7015] - Create nodebalancer tab in firewalls landing (#9590) * initial changes * finished the firewall device landing updates * Added changeset: Create nodebalancer tab in firewalls landing * fixed breadcrumb and variable names * added FirewallDeviceLanding test file * added initial unit tests * updated jest.mock function, still not working * swapped from jest.mock to MSW, still need some work * fixed unit tests * fixed tests for FirewallDeviceLanding --------- Co-authored-by: TylerWJ <tylerwjones99@gmail.com> * feat: [M3-7019] - Create add Nodebalancer to Firewall drawer (#9608) * initial drawer commit * finished the add nodebalancer drawer * Added changeset: Created 'Add Nodebalancer' to Firewall drawer * updated drawer and started to add event handlers * added toast notification * added multiple toast notifications * separated nodebalancer and linode drawer * added infinite scrolling * fixed pr suggestions * partially eliminated type definitions in LinodeSelect * eliminated type definitions in LinodeSelect * changed type definition of onSelectionChange in NodeBalancerSelect * eliminated type declarations in NodeBalancerSelect * erased commented out code * initial round of fixes * fixed linode error drawer not closing * eliminated event message * added todo comment * added todo comment * fixed toast notification, error reset, and error text * fixed toast notification, error reset, and error text for Linode Drawer * fixed nodebalancer drawer error notices * merged with develop * initial migration to new autocomplete component, still some errors * can select linodes now, but the linodes arent showing as selected * fixed selection issue with autocomplete * migrated to new autocomplete component * remove NodeBalancerSelect file changes * added banks PR suggestions --------- Co-authored-by: TylerWJ <tylerwjones99@gmail.com> * feat: [M3-7052] - Add NodeBalancer table under Firewall / Devices (#9664) * initial nodebalancer table commit * Added changeset: Added NodeBalancer table under Firewall/Devices * removed unnecessary prop extension --------- Co-authored-by: TylerWJ <tylerwjones99@gmail.com> * feat: [M3-7016] - Add Basic Filtering for Firewall Devices (#9626) * initial firewall device filter commit * Added changeset: Added Basic Filtering for Firewall Devices * cleaned up label content in TextField * initial ActionsPanel Storybook commit * reverted ActionsPanel changes * updated storybook * Fixed storybook * test commit - eslint issues * test commit * fixed text field * removed unnecessary code * added Alban's changes * test commit * removed test comment * fixed filtering error --------- Co-authored-by: TylerWJ <tylerwjones99@gmail.com> Co-authored-by: Hana Xu <hxu@akamai.com> * feat: [M3-7033] - Update Firewall Landing Device Column (#9668) * initial update to device cell wrap functionality * added extra console line for width * still not working * updated the code, still not working * changes linodes column to devices column * fixed styling file * Added changeset: Updated Firewall Landing Device Column * removed unnecessary styling * fixed test results * added Connie's pr suggestions * updated styled row file * added line clamp to styled component * removed old test case * updated row spacing * fixed spacing --------- Co-authored-by: TylerWJ <tylerwjones99@gmail.com> * feat: [M3-7017] - Update firewall events (#9693) * feat: [M3-7017] - Update firewall events * Fix e2e test and secondary entity formatting * Add changeset * feat: [M3-7020] - Update Create Firewall drawer (#9630) * feat: [M3-7020] - Update Create Firewall drawer * Update firewall e2e tests for NodeBalancer UI improvements * Integrate new Autocomplete component * Change test description * Add UI indicator for errors and reset nb values * Add changeset * Move static strings outside component * Implement PR feedback suggestions * Remove unnecessary check and change var name --------- Co-authored-by: Joe D'Amore <jdamore@linode.com> * feat: [M3-7021] - Added Firewall panel to create NodeBalancer flow (#9733) * merged with upstream * Added changeset: Added Firewall Panel to NodeBalancer Create flow --------- Co-authored-by: TylerWJ <tylerwjones99@gmail.com> * fix: [M3-7245] - Replaced 'Devices' with 'Services' in Firewall Page (#9775) * replaced visible devices with services * fixed routing for drawers * Added changeset: Swapped 'Devices' to 'Services' in Firewall and updated Add Service to Firewall routing * updated unit tests, still need work * updated unit tests * updated capitalization and unit tests --------- Co-authored-by: TylerWJ <tylerwjones99@gmail.com> * fixed merge conflict * fix: [M3-7313] - Migrated Linode dropdown to Autocomplete in Firewall Landing > Create Firewall drawer (#9837) * migrated Linode dropdown to Autocomplete, fixed NodeBalancer dropdown, added successfull toast notification * Added changeset: Linode dropdown in Create Firewall drawer using LinodeSelect component instead of the Autocomplete component * fixed changeset message from last pr * updated unit tests * addressed Hana's feedback * removed helper text * initial changes to unnecessary state * addresses Banks feedback * addressed additional feedback * fixed syntax of optionsFilter * switched set to list for autocomplete --------- Co-authored-by: TylerWJ <tylerwjones99@gmail.com> * feat: [M3-7163] - Add Firewall Section to NodeBalancer Details Page (#9831) * first commit * Code cleanup * Fix quick bug * PR feedback changes * RQ changes * fixed toast notification errors * fixed toast notifications * PR feedback --------- Co-authored-by: TylerWJ <tylerwjones99@gmail.com> * refactor: [M3-7377] - Migrate dopdowns in Create Firewall Drawer from Autocomplete to LinodeSelect and NodeBalancerSelect (#9886) * initial commit * updated font bold to use mui theme * migrated autocomplete to nodebalancer select * small naming changes * Updated CreateFirewallDrawer and SelectFirewallPanel * removed comments * Added changeset: Dropdowns in Create Firewall Drawer using Autocomplete instead of updated LinodeSelect and NodeBalancerSelect * fixed LinodeSelect unit tests * added NodeBalancer unit tests * fixed SelectFirewallPanel unit tests * added pr feedback * updated dropdown labels * fixed unit tests * fixed unit tests * fixed unit tests * working to fix timeout error * rerunning unit tests * test debug * merged with develop * testing unit tests * testing last unit test * testing last unit test * removed line to see if unit tests will pass * Replace calls to `jest.fn()` with `vi.fn()` * rerunning tests * fixed e2e tests * resolved pr suggestions --------- Co-authored-by: TylerWJ <tylerwjones99@gmail.com> Co-authored-by: Banks Nussman <banks@nussman.us> Co-authored-by: Joe D'Amore <jdamore@linode.com> * feature: [M3-7442] - Put Firewall-Nodebalancer behind feature flag (#9931) * initial feature flag migration * Added remaining code under feature flag * updated text in drawer * Added changeset: Added feature flag to Firewall-NodeBalancer * fixed unit test * fixed unit tests * added feature flag to dev tools * test commit * test commit * fixed border issue in firewall's table * partially fixed border issue * partially fixed border issue * fixed border issue * feat: [M3-7018] - Add documentation links for Firewall NodeBalancer (#9926) * added links * fixed nodebalancer create flow text * made link format more consistent * added changeset * fixed filter issue * removed comment * started to address Alban's feedback * fixed other instance of unnecessary else * addressed feedback for Firewall Drawer Props * added clamp-js for firewall landing * added clamp-js library * addressed some comments * addressed onService required prop feedback * addressed test feedbacl for FirewallDeviceLanding * addressed test feedback for FirewallDeviceLanding * added a todo in FirewallRulesLanding * added unit tests for addNodebalancerDrawer and addLinodeDrawer * addressed Connie's feedback ab aria labels * sanitized error messages in add drawers * added interface for secondaryEntityTypeObj * fixed link in create NodeBalancer flow * fixed header in create drawer * Update packages/manager/.changeset/pr-9926-upcoming-features-1700672014523.md Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com> * added period after Learn more in create firewall drawer * fixed nodebalancer create flow issue * NodeBalancer is now assigned to Firewall * removed unnecessary invalidate query * added doc link to NodeBalancer's details page * improved organization of the AddLinodeDrawer and AddNodeBalancerDrawer * added a constants file in FirewallLanding directory * Update packages/manager/.changeset/pr-9926-upcoming-features-1700672014523.md Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com> * Update packages/manager/.changeset/pr-9931-upcoming-features-1700686098973.md Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com> * Update packages/manager/src/features/Firewalls/FirewallDetail/Devices/FirewallDeviceTable.test.tsx Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com> * Update packages/manager/src/features/Firewalls/FirewallDetail/Devices/FirewallDeviceTable.test.tsx Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com> * Update packages/manager/src/features/Firewalls/FirewallDetail/Devices/AddNodebalancerDrawer.test.tsx Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com> * Update packages/manager/src/features/Firewalls/FirewallDetail/Devices/AddNodebalancerDrawer.test.tsx Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com> * Update packages/manager/src/features/Firewalls/FirewallDetail/Devices/AddNodebalancerDrawer.test.tsx Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com> * Update packages/manager/src/features/NodeBalancers/NodeBalancerDetail/NodeBalancerFirewalls.tsx Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com> * Update packages/api-v4/src/nodebalancers/nodebalancers.ts Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com> * Update packages/manager/src/features/NodeBalancers/NodeBalancerDetail/NodeBalancerFirewalls.tsx Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com> * Update packages/manager/src/features/Firewalls/FirewallDetail/Devices/AddLinodeDrawer.test.tsx Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com> * Update packages/manager/src/features/Firewalls/FirewallDetail/Devices/AddNodebalancerDrawer.test.tsx Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com> * Update packages/manager/src/features/Firewalls/FirewallDetail/Devices/AddLinodeDrawer.test.tsx Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com> * Update packages/manager/src/features/Firewalls/FirewallDetail/Devices/AddLinodeDrawer.test.tsx Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com> * Update packages/manager/src/features/Firewalls/FirewallDetail/Devices/AddLinodeDrawer.test.tsx Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com> * fixed import error * mapIdsToLinodes to mapIdsToDevices * fixed Linodes Network tab Firewall Table error state * fixed Linodes Network tab Firewall Table error state * fixed Create NodeBalancer add firewall flow * removed firewall_id forom NodeBalancerConfigNode * added firewall_id to the payload * fixed spelling issue * Fix darkmode link color discrepancy * added tags to CreateNodeBalancerPayload * Replace html anchor tag with Link component * change: [M3-7546] - Revert FW table row changes (#9974) * change: [M3-7546] - Revert NB table row changes * Fix linking for NodeBalancers from the table cell * Remove the `clamp-js` package * Add changeset * added links to constants file * changed SecondaryEntityType from interface to type * changed improved mapIdsToDevices tests to cover nodebalancers * Update packages/manager/src/features/Events/eventMessageGenerator.ts Co-authored-by: Banks Nussman <115251059+bnussman-akamai@users.noreply.github.com> * eliminated ternary in Summary Panel * cleaned up NodeBalancerSettings displayFirewallInfotext * Fix dark mode color issue on NB settings page * invalidated device queries after firewall has been deleted --------- Co-authored-by: tyler-akamai <139489745+tyler-akamai@users.noreply.github.com> Co-authored-by: TylerWJ <tylerwjones99@gmail.com> Co-authored-by: Hana Xu <hxu@akamai.com> Co-authored-by: Joe D'Amore <jdamore@linode.com> Co-authored-by: Tyler Jones <tjones@akamai.com> Co-authored-by: Banks Nussman <banks@nussman.us> Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com> Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com> Co-authored-by: Banks Nussman <115251059+bnussman-akamai@users.noreply.github.com>

migrated Linode dropdown to Autocomplete, fixed NodeBalancer dropdown…

1caac25

…, added successfull toast notification

tyler-akamai self-assigned this Oct 24, 2023

TylerWJ added 2 commits October 24, 2023 14:26

Added changeset: Linode dropdown in Create Firewall drawer using Lino…

d9fd4ef

…deSelect component instead of the Autocomplete component

fixed changeset message from last pr

307ff5c

tyler-akamai commented Oct 24, 2023

View reviewed changes

updated unit tests

5f5729e

tyler-akamai marked this pull request as ready for review October 24, 2023 19:01

tyler-akamai requested a review from a team as a code owner October 24, 2023 19:01

tyler-akamai requested review from bnussman-akamai, carrillo-erik and hana-akamai and removed request for a team October 24, 2023 19:01

tyler-akamai added Ready for Review Nodebalancer Firewall UX labels Oct 25, 2023

carrillo-erik approved these changes Oct 25, 2023

View reviewed changes

mjac0bs added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Oct 25, 2023

tyler-akamai added the Ready for Review label Oct 26, 2023

hana-akamai reviewed Oct 26, 2023

View reviewed changes

tyler-akamai removed the Ready for Review label Oct 26, 2023

addressed Hana's feedback

a9edfe0

tyler-akamai requested a review from hana-akamai October 26, 2023 18:47

removed helper text

4320d1c

bnussman-akamai requested changes Oct 26, 2023

View reviewed changes

initial changes to unnecessary state

26dc92b

addresses Banks feedback

bef2f53

tyler-akamai commented Oct 30, 2023

View reviewed changes

bnussman-akamai reviewed Oct 30, 2023

View reviewed changes

packages/manager/src/features/Firewalls/FirewallLanding/CreateFirewallDrawer.tsx Outdated Show resolved Hide resolved

TylerWJ added 2 commits October 30, 2023 11:04

addressed additional feedback

4b439f9

fixed syntax of optionsFilter

10cb9c1

tyler-akamai requested a review from bnussman-akamai October 30, 2023 15:31

bnussman-akamai approved these changes Oct 30, 2023

View reviewed changes

switched set to list for autocomplete

879ca8b

hana-akamai approved these changes Oct 30, 2023

View reviewed changes

tyler-akamai removed the Add'tl Approval Needed Waiting on another approval! label Oct 30, 2023

tyler-akamai merged commit c8124fc into linode:feature/firewall-nodebalancer Oct 30, 2023

Conversation

tyler-akamai commented Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description 📝

Changes 🔄

Preview 📷

How to test 🧪

Prerequisites

Reproduction steps

Verification steps

As an Author I have considered 🤔

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyler-akamai Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyler-akamai Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carrillo-erik commented Oct 25, 2023

Uh oh!

hana-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

tyler-akamai commented Oct 26, 2023

Uh oh!

bnussman-akamai Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyler-akamai Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bnussman-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hana-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tyler-akamai commented Oct 24, 2023 •

edited

Loading

tyler-akamai Oct 24, 2023 •

edited

Loading

tyler-akamai Oct 24, 2023 •

edited

Loading

bnussman-akamai Oct 26, 2023 •

edited

Loading

tyler-akamai Oct 27, 2023 •

edited

Loading