fix: [M3-7262] - Create Firewall Drawer opens in the same tab in the Linode Create Flow by tyler-akamai · Pull Request #9785 · linode/manager

tyler-akamai · 2023-10-11T21:19:22Z

Description 📝

Currently there is a 'Create Firewall' button in the Firewall Panel that when clicked, opens the Create Firewall Drawer in another tab. This should open the Create Firewall drawer in the same tab, in the same page.

Changes 🔄

List any change relevant to the reviewer.

Firewall Panel's 'Create Firewall' button opens the Firewall Create Drawer in the same tab
Once the Firewall is selected, it becomes the value of the dropdown
Dropdown header in Create Firewall Drawer 'Linodes' --> 'Additional Linodes (Optional)'

Before	After
Screen.Recording.2023-10-12.at.10.02.53.AM.mov	Screen.Recording.2023-10-12.at.9.56.37.AM.mov

How to test 🧪

Prerequisites

No required prerequisites

Reproduction steps

Navigate to http://localhost:3000/linodes/create
Scroll down to the Firewall Panel
Click on the 'Create VPC' link
Ensure that it brings you to a new tab

Verification steps

Navigate to http://localhost:3000/linodes/create
Ensure that:

The default Firewall is still 'None'
When you choose a Firewall, the summary section at the bottom gets updated ('Firewall Assigned' gets added)
When clicking on 'None', the 'Firewall Assigned' gets removed from the summary section
When clicking on the 'Create Firewall' link, the Create Firewall Drawer appears in the same tab
The Create Firewall Drawer has the text 'Additional Linodes (Optional)', while the Create Firewall Drawer from the Firewall Landing page has the text 'Linodes'
Add an additional Linode and ensure it is added when the Firewall is created
After creating a new Firewall, that Firewall is selected in the dropdown (in the firewall panel) and the summary section got updated
After creating the Linode, the Linode is properly assigned to the Firewall in the Firewall Landing page. Test this in development, it is blocked in prod by an api feature flag

As an Author I have considered 🤔

Check all that apply

…Linode Create Flow

tyler-akamai · 2023-10-11T21:23:31Z

packages/manager/src/components/SelectFirewallPanel/SelectFirewallPanel.tsx

+  };
+
+  const handleFirewallCreated = (id: number, label: string) => {
+    setDropdownValue({ label, value: id });


When the user creates a new Firewall, this code is responsible for setting the value of the dropdown in the Firewall Panel to the recently created Firewall.

packages/manager/src/components/SelectFirewallPanel/SelectFirewallPanel.tsx

tyler-akamai · 2023-10-17T17:51:58Z

packages/manager/src/components/SelectFirewallPanel/SelectFirewallPanel.tsx

  helperText: JSX.Element;
 }

+export const createFirewallLabel = 'Additional Linodes (Optional)';


UX wanted to ensure the user:

Wasnt looking for current Linode in the dropdown --> 'Additional'

Knew that the input was optional --> '(Optional)'

mjac0bs

Thank you for this awesome testing checklist!

I verified:

The default Firewall is still 'None'
When you choose a Firewall, the summary section at the bottom gets updated ('Firewall Assigned' gets added)
When clicking on 'None', the 'Firewall Assigned' gets removed from the summary section
When clicking on the 'Create Firewall' link, the Create Firewall Drawer appears in the same tab
The Create Firewall Drawer has the text 'Additional Linodes (Optional)', while the Create Firewall Drawer from the Firewall Landing page has the text 'Linodes'
Add an additional Linode and ensure it is added when the Firewall is created
After creating a new Firewall, that Firewall is selected in the dropdown (in the firewall panel) and the summary section got updated
After creating the Linode, the Linode is properly assigned to the Firewall in the Firewall Landing page.

For the last item, I'm not seeing the linode in the create flow be assigned to the firewall -- just the "additional linode" is assigned. Based on your screencap of this branch and my understanding of the acceptance criteria, I would have expected to see both longview-linode-do-not-delete and debian-us-sea listed as assigned to firewall testing123, but debian-us-sea was not. Have you observed any issues with handleFirewallChange?

Screen.Recording.2023-10-18.at.10.07.57.AM.mov

mjac0bs · 2023-10-18T16:20:43Z

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

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


Suggested change

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))

Similar suggestion here with describing the thing that was fixed.

tyler-akamai · 2023-10-18T21:40:47Z

Thank you for this awesome testing checklist!

I verified:

The default Firewall is still 'None'

When you choose a Firewall, the summary section at the bottom gets updated ('Firewall Assigned' gets added)

When clicking on 'None', the 'Firewall Assigned' gets removed from the summary section

When clicking on the 'Create Firewall' link, the Create Firewall Drawer appears in the same tab

The Create Firewall Drawer has the text 'Additional Linodes (Optional)', while the Create Firewall Drawer from the Firewall Landing page has the text 'Linodes'

Add an additional Linode and ensure it is added when the Firewall is created

After creating a new Firewall, that Firewall is selected in the dropdown (in the firewall panel) and the summary section got updated

After creating the Linode, the Linode is properly assigned to the Firewall in the Firewall Landing page.

For the last item, I'm not seeing the linode in the create flow be assigned to the firewall -- just the "additional linode" is assigned. Based on your screencap of this branch and my understanding of the acceptance criteria, I would have expected to see both longview-linode-do-not-delete and debian-us-sea listed as assigned to firewall testing123, but debian-us-sea was not. Have you observed any issues with handleFirewallChange?

Screen.Recording.2023-10-18.at.10.07.57.AM.mov

@mjac0bs Good catch! I had to ask @dwiley-akamai on this one. From my understanding this functionality is blocked by an API feature flag. You can test this on your dev environment locally since the feature flag is turned on, but it is not turned on in prod yet.

packages/manager/src/components/SelectFirewallPanel/SelectFirewallPanel.tsx

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

jaalah-akamai · 2023-10-20T13:11:52Z

I verified the checklist above and the last item as well. This works in dev, not locally though. ✅

jaalah-akamai

Looks good pending changes requested from @bnussman-akamai

…-create-drawer-opens-in-same-tab

bnussman-akamai · 2023-10-20T18:48:04Z

packages/manager/src/components/SelectFirewallPanel/SelectFirewallPanel.tsx

+  const [dropdownValue, setDropdownValue] = React.useState<{
+    label: string;
+    value: number;
+  } | null>(null);


I just realized, we shouldn't need state here. We should have a prop on this component called selectedFirewallId: number which we should be using in place of the state

Suggested change

const [dropdownValue, setDropdownValue] = React.useState<{

label: string;

value: number;

} | null>(null);

mjac0bs

After creating the Linode, the Linode is properly assigned to the Firewall in the Firewall Landing page.

Verified in dev environment, thanks for the clarification!

Functionality looks good. You caught the commented code I was about to note. Other than Banks' feedback and my earlier changelog suggestion, this LGTM.

bnussman-akamai

Thanks for addressing the feedback! Great change!

packages/manager/src/components/SelectFirewallPanel/SelectFirewallPanel.tsx

packages/manager/src/features/Linodes/LinodesCreate/LinodeCreate.tsx

…-create-drawer-opens-in-same-tab

TylerWJ added 3 commits October 11, 2023 17:07

initial commit

670386d

resolved merge conflicts

bc1e043

Added changeset: Firewall Create Drawer opens in the same tab in the …

0b9b3a8

…Linode Create Flow

tyler-akamai commented Oct 11, 2023

View reviewed changes

Added 'Additional Linodes (Optional)' to Firewall Drawer

d06ae5c

tyler-akamai self-assigned this Oct 11, 2023

tyler-akamai changed the title ~~fix: [M3-7262] - Firewall Create Drawer opens in the same tab in the Linode Create Flow~~ fix: [M3-7262] - Create Firewall Drawer opens in the same tab in the Linode Create Flow Oct 12, 2023

tyler-akamai commented Oct 12, 2023

View reviewed changes

packages/manager/src/components/SelectFirewallPanel/SelectFirewallPanel.tsx Outdated Show resolved Hide resolved

TylerWJ added 2 commits October 17, 2023 13:46

added unit test

3118f7d

updated unit tests

a6c7d51

tyler-akamai marked this pull request as ready for review October 17, 2023 17:49

tyler-akamai requested a review from a team as a code owner October 17, 2023 17:49

tyler-akamai requested review from jaalah-akamai and mjac0bs and removed request for a team October 17, 2023 17:49

tyler-akamai commented Oct 17, 2023

View reviewed changes

mjac0bs reviewed Oct 18, 2023

View reviewed changes

bnussman-akamai requested changes Oct 19, 2023

View reviewed changes

jaalah-akamai approved these changes Oct 20, 2023

View reviewed changes

TylerWJ added 2 commits October 20, 2023 13:45

addressed PR feedback

bb627e8

Merge remote-tracking branch 'upstream/develop' into M3-7262-firewall…

d268aa2

…-create-drawer-opens-in-same-tab

tyler-akamai requested review from bnussman-akamai and mjac0bs October 20, 2023 18:12

bnussman-akamai requested changes Oct 20, 2023

View reviewed changes

removed commented code

0120421

mjac0bs approved these changes Oct 20, 2023

View reviewed changes

TylerWJ added 2 commits October 20, 2023 15:56

addressed PR feedback

577011b

improved selectedFirewall functionality

836a827

bnussman-akamai approved these changes Oct 20, 2023

View reviewed changes

packages/manager/src/components/SelectFirewallPanel/SelectFirewallPanel.tsx Outdated Show resolved Hide resolved

packages/manager/src/features/Linodes/LinodesCreate/LinodeCreate.tsx Outdated Show resolved Hide resolved

TylerWJ added 2 commits October 23, 2023 10:20

addressed feedback

7b41447

Merge remote-tracking branch 'upstream/develop' into M3-7262-firewall…

da2cf0a

…-create-drawer-opens-in-same-tab

tyler-akamai merged commit 86ba6e8 into linode:develop Oct 23, 2023

This was referenced Oct 26, 2023

Release v1.106.0 - release → staging #9842

Merged

Release v1.106.0 - staging → master #9850

Merged

	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))

Conversation

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

Uh oh!

Description 📝

Changes 🔄

How to test 🧪

Prerequisites

Reproduction steps

Verification steps

As an Author I have considered 🤔

Uh oh!

tyler-akamai Oct 11, 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!

Uh oh!

tyler-akamai Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

mjac0bs left a comment

Choose a reason for hiding this comment

Uh oh!

mjac0bs Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

jaalah-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

bnussman-akamai Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

mjac0bs left a comment

Choose a reason for hiding this comment

Uh oh!

bnussman-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tyler-akamai commented Oct 11, 2023 •

edited by cpathipa

Loading

tyler-akamai Oct 11, 2023 •

edited

Loading

tyler-akamai commented Oct 18, 2023 •

edited

Loading

jaalah-akamai commented Oct 20, 2023 •

edited

Loading