Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle empty IP Address field in Network Interface create flow #1854

Merged
merged 7 commits into from
Jan 24, 2024

Conversation

charliepark
Copy link
Contributor

@charliepark charliepark commented Dec 12, 2023

We were looking at some modifications to the NumberField in #1812, and were going to fix #1438 with this changes, but we can decouple that work. This PR focuses on handling the issue in #1438 where a typed-in-and-then-deleted field was getting passed through as an empty string, when we actually want for that field to be undefined.

We'll revisit the NumberField migration separately.

Copy link

vercel bot commented Dec 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Jan 24, 2024 10:19pm

@charliepark
Copy link
Contributor Author

This screenshot was in #1851 :
image

I'm not seeing that same error when I try entering in a non-IP-address syntax. Did we lose something in the recent fixes to that form?

@charliepark
Copy link
Contributor Author

Screenshot 2023-12-12 at 3 30 09 PM

@david-crespo
Copy link
Collaborator

david-crespo commented Dec 12, 2023

This screenshot was in #1851 : image

I'm not seeing that same error when I try entering in a non-IP-address syntax. Did we lose something in the recent fixes to that form?

Real or mock API? I believe that's a server-side error the mock API won't imitate properly.

https://github.com/rust-lang/rust/blob/3340d49d22b1aba425779767278c40781826c2f5/library/core/src/net/parser.rs#L490

@charliepark
Copy link
Contributor Author

That makes sense, then!

@charliepark
Copy link
Contributor Author

There are a few other changes in #1812 from what I have here that I'll want to take another look at, just to make sure I'm not missing something.

@charliepark charliepark marked this pull request as draft December 13, 2023 06:29

// it should error out
// todo: improve error message from API
await expect(sidebar.getByText('Unknown server error')).toBeVisible()
Copy link
Collaborator

@david-crespo david-crespo Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because we don't handle MSW errors properly in the UI. One thing we could do, it just occurred to me, is make them look more like real API errors so that the UI would handle them naturally. As in, where right now we just return the Zod error directly as JSON, we could stick it somewhere in an object that looks like { "error_code": "InvalidRequest", "message": "..." }. The Zod error would probably look like garbage stringified, but maybe we put a short version of it in message (with "Zod" in there to be clear it's a mock API error) and then log the full thing to the console.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@charliepark charliepark changed the title Add onBlur to better handle empty IP Address field Handle empty IP Address field in Network Interface create flow Dec 16, 2023
@@ -56,6 +56,8 @@ export interface TextFieldProps<
units?: string
validate?: Validate<FieldPathValue<TFieldValues, TName>, TFieldValues>
control: Control<TFieldValues>
/** Alters the value of the input during the field's onChange event. */
transform?: (value: string) => FieldPathValue<TFieldValues, TName>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the return type from string | undefined to this, which effectively says it returns whatever the type of the field in question is. The explicit string | undefined was fine for now, but we'd run into a type error later when we tried to use this on a field with a different type.

// it should use the default value
await expect(lastRow.getByText('123.45.68.8')).toBeVisible()
// ip address is auto-assigned
await expectRowVisible(page.getByRole('table'), { name: 'nic-2', ip: '123.45.68.8' })
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handy row assert util

Copy link
Collaborator

@david-crespo david-crespo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work

@david-crespo david-crespo merged commit 7c6f53d into main Jan 24, 2024
8 checks passed
@david-crespo david-crespo deleted the fix-empty-nic-field-issue branch January 24, 2024 22:34
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request Jan 31, 2024
oxidecomputer/console@b9013a3...1a4f5d8

* [1a4f5d81](oxidecomputer/console@1a4f5d81) oxidecomputer/console#1910
* [34596e33](oxidecomputer/console@34596e33) oxidecomputer/console#1928
* [ff7cdb28](oxidecomputer/console@ff7cdb28) oxidecomputer/console#1926
* [efa39789](oxidecomputer/console@efa39789) oxidecomputer/console#1867
* [4bfadc02](oxidecomputer/console@4bfadc02) oxidecomputer/console#1927
* [695d3671](oxidecomputer/console@695d3671) oxidecomputer/console#1925
* [30070292](oxidecomputer/console@30070292) better path filter for local files in msw handler
* [5cf4339c](oxidecomputer/console@5cf4339c) oxidecomputer/console#1916
* [a26f7c1e](oxidecomputer/console@a26f7c1e) oxidecomputer/console#1922
* [231b93ed](oxidecomputer/console@231b93ed) oxidecomputer/console#1923
* [c8364638](oxidecomputer/console@c8364638) better msw warning filter so we don't get warning noise in console
* [764f7310](oxidecomputer/console@764f7310) oxidecomputer/console#1908
* [945619eb](oxidecomputer/console@945619eb) oxidecomputer/console#1921
* [e2d82a4c](oxidecomputer/console@e2d82a4c) oxidecomputer/console#1887
* [d6a67bd5](oxidecomputer/console@d6a67bd5) oxidecomputer/console#1918
* [1fb746f4](oxidecomputer/console@1fb746f4) oxidecomputer/console#1899
* [ca7c85de](oxidecomputer/console@ca7c85de) oxidecomputer/console#1917
* [28598e1d](oxidecomputer/console@28598e1d) oxidecomputer/console#1914
* [34eb478d](oxidecomputer/console@34eb478d) oxidecomputer/console#1912
* [4d693088](oxidecomputer/console@4d693088) bump vite-plugin-html to stop getting deprecation warning
* [d5c39549](oxidecomputer/console@d5c39549) oxidecomputer/console#1909
* [7c6f53db](oxidecomputer/console@7c6f53db) oxidecomputer/console#1854
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request Jan 31, 2024
oxidecomputer/console@b9013a3...1a4f5d8

* [1a4f5d81](oxidecomputer/console@1a4f5d81)
oxidecomputer/console#1910
* [34596e33](oxidecomputer/console@34596e33)
oxidecomputer/console#1928
* [ff7cdb28](oxidecomputer/console@ff7cdb28)
oxidecomputer/console#1926
* [efa39789](oxidecomputer/console@efa39789)
oxidecomputer/console#1867
* [4bfadc02](oxidecomputer/console@4bfadc02)
oxidecomputer/console#1927
* [695d3671](oxidecomputer/console@695d3671)
oxidecomputer/console#1925
* [30070292](oxidecomputer/console@30070292)
better path filter for local files in msw handler
* [5cf4339c](oxidecomputer/console@5cf4339c)
oxidecomputer/console#1916
* [a26f7c1e](oxidecomputer/console@a26f7c1e)
oxidecomputer/console#1922
* [231b93ed](oxidecomputer/console@231b93ed)
oxidecomputer/console#1923
* [c8364638](oxidecomputer/console@c8364638)
better msw warning filter so we don't get warning noise in console
* [764f7310](oxidecomputer/console@764f7310)
oxidecomputer/console#1908
* [945619eb](oxidecomputer/console@945619eb)
oxidecomputer/console#1921
* [e2d82a4c](oxidecomputer/console@e2d82a4c)
oxidecomputer/console#1887
* [d6a67bd5](oxidecomputer/console@d6a67bd5)
oxidecomputer/console#1918
* [1fb746f4](oxidecomputer/console@1fb746f4)
oxidecomputer/console#1899
* [ca7c85de](oxidecomputer/console@ca7c85de)
oxidecomputer/console#1917
* [28598e1d](oxidecomputer/console@28598e1d)
oxidecomputer/console#1914
* [34eb478d](oxidecomputer/console@34eb478d)
oxidecomputer/console#1912
* [4d693088](oxidecomputer/console@4d693088)
bump vite-plugin-html to stop getting deprecation warning
* [d5c39549](oxidecomputer/console@d5c39549)
oxidecomputer/console#1909
* [7c6f53db](oxidecomputer/console@7c6f53db)
oxidecomputer/console#1854
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add NIC to instance: validation problems with IP address field
2 participants