upcoming: [M3-7944] - Linode Create Refactor - User Data - Part 7 #10331

bnussman-akamai · 2024-03-29T15:46:54Z

Description 📝

Adds User Data section to the new Linode Create page 👤📑 🎉

Note

The User Data section will only show up if you select an Image that is cloud-init capable and a region that supports Metadata. I don't necessarily agree with this UX, but this matches the existing behavior.

Preview 📷

How to test 🧪

Turn on the Linode Create v2 feature flag using load dev tools
Verify you see the Metadata section if you select a cloud-init capable image and a Region that supports metadata
Verify you can specify metadata
Observe that the base64 encoded user data is in the payload

As an Author I have considered 🤔

github-actions · 2024-03-29T16:09:10Z

Coverage Report: ✅
Base Coverage: 81.72%
Current Coverage: 81.72%

mjac0bs

Looks good from my review and testing! I didn't see test coverage for our help icons/tooltips in any e2es, so that could be something worth adding to these component unit tests. And we need a changeset, please.

✅ I see the Metadata section if I select a cloud-init capable image and a Region that supports metadata -- though I'm with you on the poor visibility of this feature. I had to google our Metadata availability because I never remember.
✅ I can specify metadata and if it's not in the intended formats, I see the warning
✅ Observed that the base64 encoded user data is in the payload
✅ Observed the warnings for backups and cloning

packages/manager/src/features/Linodes/LinodeCreatev2/UserData/UserDataHeading.tsx

mjac0bs · 2024-03-29T18:07:17Z

packages/manager/src/features/Linodes/LinodeCreatev2/UserData/UserDataHeading.test.tsx

Great test, thank you!

We could add test coverage to check that the help icon displays as expected.

mjac0bs · 2024-03-29T18:08:07Z

packages/manager/src/features/Linodes/LinodeCreatev2/UserData/UserData.test.tsx

We could add test coverage to check that the help icon displays as expected.

(One of them would actually be in UserDataHeading, I know.)

hkhalil-akamai

Noticed no regressions after manual testing. Left a few suggestions. Nice work, really appreciate the bite-sized PRs! 🙏🏽

hkhalil-akamai · 2024-03-29T19:24:43Z

packages/manager/src/features/Linodes/LinodeCreatev2/UserData/UserData.tsx

+                hasInputValueChanged: true,
+                userData: e.target.value,
+              });
+              field.onChange(utoa(e.target.value));


Instead of performing the utoa conversion on change, could we store the original value and only do the conversion on submit? This would also save us from having to convert back on line 106.

I'm considering both options. At first, when building this new create flow, I wanted to try to keep the form data in the format the API expects, but now I'm realizing it might make more sense to perform some transformation on submit.

I might merge as-is, but might come back and change this.

FWIW, i agree with a submit data transformation. This looks a bit expensive especially without debouncing - you also transform the value

Fair enough. I will change this ✅

Added a transform step in 1803a36

hkhalil-akamai · 2024-03-29T19:26:01Z

packages/manager/src/features/Linodes/LinodeCreatev2/UserData/UserData.test.tsx

+import { UserData } from './UserData';
+
+describe('Linode Create v2 UserData', () => {
+  it('should render if the selected image supports cloud-init and the region supports metadata', async () => {


Should we have a test for the opposite (i.e., doesn't appear when the image or regions doesn't support metadata)?

Yes, but I don't really know how to reliably do this because the unit test waits for API data to load

@bnussman-akamai This is a good candidate to mock the useImages and useRegions hooks so you can ignore the HTTP stuff and test directly against the state you want, but you can also do this kind of hacky thing to wait for the requests before testing the state of the component:

const image = imageFactory.build({ capabilities: [] }); const region = regionFactory.build({ capabilities: ['Metadata'] }); let [hasLoadedImages, hasLoadedRegions] = [false, false]; server.use( http.get('*/v4/images/*', () => { hasLoadedImages = true; return HttpResponse.json(image); }), http.get('*/v4/regions', () => { hasLoadedRegions = true; return HttpResponse.json(makeResourcePage([region])); }) ); const { queryByText } = renderWithThemeAndHookFormContext({ component: <UserData />, useFormOptions: { defaultValues: { image: image.id, region: region.id } }, }); // Wait for requests... await waitFor(() => { expect(hasLoadedImages && hasLoadedRegions).toBe(true); }); // Assert that user data is hidden here.

yeah good call. Mocking useImages and useRegions would probably be best. Thank you for the tip 🙏

hkhalil-akamai · 2024-03-29T19:27:40Z

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

+/**
+ * ASCII to Unicode (decode Base64 to original data)
+ */
+export function atou(b64: string) {
+  return decodeURIComponent(escape(atob(b64)));
+}
+


It might be worth adding a test for this utility (and its counterpart) with some common examples and edge cases (like hex escape codes).

packages/manager/src/features/Linodes/LinodeCreatev2/UserData/UserData.tsx

…UserData.tsx Co-authored-by: Hussain Khalil <122488130+hkhalil-akamai@users.noreply.github.com>

abailly-akamai

Looks great and appreciate the small, well tested PR

Prob an existing behavior - we should prevent event propagation on the accordion header tooltip so it does not collapses it onclick. This is prob very annoying on mobile

Screen.Recording.2024-04-01.at.10.08.20.mov

abailly-akamai · 2024-04-01T14:09:40Z

packages/manager/src/features/Linodes/LinodeCreatev2/UserData/UserData.tsx

+                hasInputValueChanged: true,
+                userData: e.target.value,
+              });
+              field.onChange(utoa(e.target.value));


FWIW, i agree with a submit data transformation. This looks a bit expensive especially without debouncing - you also transform the value

abailly-akamai · 2024-04-01T14:11:54Z

packages/manager/src/features/Linodes/LinodeCreatev2/UserData/UserDataHeading.tsx

+          {warningMessage}
+        </Notice>
+      )}
+    </Stack>


nit, let's not add another div if not needed and use a fragment instead

Kind of addressed in cf72f9f

abailly-akamai · 2024-04-01T14:17:22Z

packages/manager/src/features/Linodes/LinodeCreatev2/UserData/UserData.tsx

+import { useImageQuery } from 'src/queries/images';
+import { useRegionsQuery } from 'src/queries/regions/regions';
+
+import { atou, utoa } from '../../LinodesCreate/utilities';


I assume you're planning to move utils eventually? wondering about duplicating them but then we'd miss potential updates

I'll do all of the moving when we start deleting the old create flow code. It will be a pain, but TS should help me identify the utils that need to be kept and moved when I delete a file and see an error

bnussman-akamai · 2024-04-01T14:35:37Z

we should prevent event propagation on the accordion header tooltip

Good point @abailly-akamai . Do you think this should be a global change to the TooltipIcon component?

abailly-akamai · 2024-04-01T14:44:36Z

@bnussman-akamai i can't really think of a time we'd want the click to propagate so I'd say yes?

bnussman-akamai · 2024-04-01T14:50:35Z

Sounds good, I agree! I also can't think of any instances where we'd want the click to propagate. Thanks @abailly-akamai

abailly-akamai

Thanks for the changes!

…node#10331) * initial work * unit testing * clean up * fix placement of `.` * Added changeset: Linode Create Refactor v2 - User Data - Part 7 * Update packages/manager/src/features/Linodes/LinodeCreatev2/UserData/UserData.tsx Co-authored-by: Hussain Khalil <122488130+hkhalil-akamai@users.noreply.github.com> * add some bais testing to encode and decode * improve spacing * use flex spacing insted of margin * stop propogation of tooltip icon button click * add a transform step in the onSubmit --------- Co-authored-by: Banks Nussman <banks@nussman.us> Co-authored-by: Hussain Khalil <122488130+hkhalil-akamai@users.noreply.github.com>

bnussman added 3 commits March 29, 2024 10:15

initial work

4414688

unit testing

016866a

clean up

53dc6b5

bnussman-akamai added the Linode Create Refactor label Mar 29, 2024

bnussman-akamai self-assigned this Mar 29, 2024

bnussman-akamai requested a review from a team as a code owner March 29, 2024 15:46

bnussman-akamai requested review from jdamore-linode, mjac0bs, hana-linode, hkhalil-akamai, jaalah-akamai and abailly-akamai and removed request for a team March 29, 2024 15:46

bnussman-akamai changed the title ~~refactor: [M3-7944] - Linode Create Refactor v2 - User Data - Part 7~~ upcoming: [M3-7944] - Linode Create Refactor v2 - User Data - Part 7 Mar 29, 2024

mjac0bs added the Missing Changeset label Mar 29, 2024

mjac0bs approved these changes Mar 29, 2024

View reviewed changes

bnussman added 2 commits March 29, 2024 14:51

fix placement of .

1af467c

Added changeset: Linode Create Refactor v2 - User Data - Part 7

bb166c8

bnussman-akamai removed the Missing Changeset label Mar 29, 2024

hkhalil-akamai approved these changes Mar 29, 2024

View reviewed changes

bnussman-akamai and others added 2 commits March 29, 2024 16:49

Update packages/manager/src/features/Linodes/LinodeCreatev2/UserData/…

3b7a440

…UserData.tsx Co-authored-by: Hussain Khalil <122488130+hkhalil-akamai@users.noreply.github.com>

add some bais testing to encode and decode

ef3c277

abailly-akamai reviewed Apr 1, 2024

View reviewed changes

bnussman added 2 commits April 1, 2024 10:19

improve spacing

6304ee6

use flex spacing insted of margin

cf72f9f

stop propogation of tooltip icon button click

01902d0

bnussman-akamai changed the title ~~upcoming: [M3-7944] - Linode Create Refactor v2 - User Data - Part 7~~ upcoming: [M3-7944] - Linode Create Refactor - User Data - Part 7 Apr 1, 2024

add a transform step in the onSubmit

1803a36

bnussman-akamai requested a review from abailly-akamai April 1, 2024 16:26

abailly-akamai approved these changes Apr 1, 2024

View reviewed changes

bnussman-akamai merged commit 0e0e5a5 into linode:develop Apr 1, 2024
17 of 18 checks passed

This was referenced Apr 11, 2024

Release v1.117.0 - release → staging #10368

Merged

Release v1.117.0 - staging → master #10377

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upcoming: [M3-7944] - Linode Create Refactor - User Data - Part 7 #10331

upcoming: [M3-7944] - Linode Create Refactor - User Data - Part 7 #10331

bnussman-akamai commented Mar 29, 2024 •

edited

github-actions bot commented Mar 29, 2024 •

edited

mjac0bs left a comment •

edited

mjac0bs Mar 29, 2024

mjac0bs Mar 29, 2024

hkhalil-akamai left a comment

hkhalil-akamai Mar 29, 2024

bnussman-akamai Apr 1, 2024 •

edited

abailly-akamai Apr 1, 2024

bnussman-akamai Apr 1, 2024

bnussman-akamai Apr 1, 2024

hkhalil-akamai Mar 29, 2024

bnussman-akamai Mar 29, 2024

jdamore-linode Mar 29, 2024

bnussman-akamai Mar 29, 2024

hkhalil-akamai Mar 29, 2024

abailly-akamai left a comment

abailly-akamai Apr 1, 2024

abailly-akamai Apr 1, 2024

bnussman-akamai Apr 1, 2024

abailly-akamai Apr 1, 2024

bnussman-akamai Apr 1, 2024

bnussman-akamai commented Apr 1, 2024 •

edited

abailly-akamai commented Apr 1, 2024

bnussman-akamai commented Apr 1, 2024

abailly-akamai left a comment

upcoming: [M3-7944] - Linode Create Refactor - User Data - Part 7 #10331

upcoming: [M3-7944] - Linode Create Refactor - User Data - Part 7 #10331

Conversation

bnussman-akamai commented Mar 29, 2024 • edited

Description 📝

Preview 📷

How to test 🧪

As an Author I have considered 🤔

github-actions bot commented Mar 29, 2024 • edited

mjac0bs left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkhalil-akamai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnussman-akamai Apr 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abailly-akamai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnussman-akamai commented Apr 1, 2024 • edited

abailly-akamai commented Apr 1, 2024

bnussman-akamai commented Apr 1, 2024

abailly-akamai left a comment

Choose a reason for hiding this comment

bnussman-akamai commented Mar 29, 2024 •

edited

github-actions bot commented Mar 29, 2024 •

edited

mjac0bs left a comment •

edited

bnussman-akamai Apr 1, 2024 •

edited

bnussman-akamai commented Apr 1, 2024 •

edited