-
Notifications
You must be signed in to change notification settings - Fork 0
fix(report): improve form validation and user feedback (Closes #607, #608) #816
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
base: main
Are you sure you want to change the base?
Changes from all commits
f82ebdb
c58d35f
e7460a6
0a60d7c
db2047b
7cc100f
5a11c0b
22e19ff
f4fcffc
f1dcd84
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,6 +21,17 @@ interface UpdateMachineFormProps { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| initials: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ownerId: string | null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| invitedOwnerId: string | null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| owner?: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| name: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| avatarUrl?: string | null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| email?: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| invitedOwner?: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| name: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| email?: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+24
to
+34
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| owner?: { | |
| id: string; | |
| name: string; | |
| avatarUrl?: string | null; | |
| email?: string; | |
| } | null; | |
| invitedOwner?: { | |
| id: string; | |
| name: string; | |
| email?: string; | |
| } | null; |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change appears unrelated to the PR's stated purpose of improving form validation and user feedback for the Report Issue form. The PR description mentions fixes for #607 and #608, which concern the report form, but this adds a read-only owner display section for non-admin users. Consider moving these machine owner display improvements to a separate PR focused on machine management features.
| {/* Machine Owner */} | |
| {isAdmin ? ( | |
| <OwnerSelect | |
| users={allUsers} | |
| defaultValue={machine.ownerId ?? machine.invitedOwnerId ?? null} | |
| /> | |
| ) : ( | |
| <div className="space-y-2" data-testid="owner-display"> | |
| <span className="text-sm font-semibold text-on-surface"> | |
| Machine Owner | |
| </span> | |
| <div className="rounded-md border border-outline bg-surface px-3 py-2"> | |
| {machine.owner || machine.invitedOwner ? ( | |
| <div className="flex items-center gap-2"> | |
| <span className="text-sm text-on-surface"> | |
| {machine.owner?.name ?? machine.invitedOwner?.name} | |
| </span> | |
| {/* Show invited badge only for truly invited owners (not yet accepted) */} | |
| {machine.invitedOwner && !machine.owner && ( | |
| <span className="text-[10px] font-medium uppercase tracking-wider text-on-surface-variant/70"> | |
| (Invited) | |
| </span> | |
| )} | |
| </div> | |
| ) : ( | |
| <span className="text-sm text-on-surface-variant"> | |
| No owner assigned | |
| </span> | |
| )} | |
| </div> | |
| <p className="text-xs text-on-surface-variant"> | |
| The owner receives notifications for new issues on this machine. | |
| </p> | |
| </div> | |
| {/* Machine Owner (admins only) */} | |
| {isAdmin && ( | |
| <OwnerSelect | |
| users={allUsers} | |
| defaultValue={machine.ownerId ?? machine.invitedOwnerId ?? null} | |
| /> |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,65 @@ | ||||||||||
| import { describe, it, expect } from "vitest"; | ||||||||||
| import { parsePublicIssueForm } from "./validation"; | ||||||||||
|
|
||||||||||
| describe("Public Issue Form Validation", () => { | ||||||||||
| it("should fail validation when machineId is missing", () => { | ||||||||||
| const formData = new FormData(); | ||||||||||
| formData.set("title", "Test Issue"); | ||||||||||
| formData.set("severity", "minor"); | ||||||||||
| formData.set("consistency", "intermittent"); | ||||||||||
|
|
||||||||||
| const result = parsePublicIssueForm(formData); | ||||||||||
|
|
||||||||||
| expect(result.success).toBe(false); | ||||||||||
| if (!result.success) { | ||||||||||
| expect(result.error).toContain("machineId"); | ||||||||||
| // Zod's default error for undefined/missing field when it expects a uuid string | ||||||||||
| expect(result.error).toContain( | ||||||||||
| "Invalid input: expected string, received undefined" | ||||||||||
| ); | ||||||||||
|
Comment on lines
+16
to
+19
|
||||||||||
| // Zod's default error for undefined/missing field when it expects a uuid string | |
| expect(result.error).toContain( | |
| "Invalid input: expected string, received undefined" | |
| ); |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test coverage for empty string submission. When users don't select a value (value is ""), the hidden input will submit an empty string to the server. While the Zod validation should catch this (enum doesn't accept empty string), there's no test case verifying this behavior. Add a test that sets severity or consistency to an empty string and verifies the validation fails appropriately.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -12,10 +12,11 @@ import { CONSISTENCY_CONFIG } from "~/lib/issues/status"; | |||||
| import { type IssueConsistency } from "~/lib/types"; | ||||||
|
|
||||||
| interface ConsistencySelectProps { | ||||||
| value: IssueConsistency; | ||||||
| value: IssueConsistency | ""; | ||||||
| onValueChange: (value: IssueConsistency) => void; | ||||||
|
||||||
| onValueChange: (value: IssueConsistency) => void; | |
| onValueChange: (value: IssueConsistency | "") => void; |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -12,10 +12,11 @@ import { PRIORITY_CONFIG } from "~/lib/issues/status"; | |||||
| import { type IssuePriority } from "~/lib/types"; | ||||||
|
|
||||||
| interface PrioritySelectProps { | ||||||
| value: IssuePriority; | ||||||
| value: IssuePriority | ""; | ||||||
| onValueChange: (value: IssuePriority) => void; | ||||||
|
||||||
| onValueChange: (value: IssuePriority) => void; | |
| onValueChange: (value: IssuePriority | "") => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This E2E test appears unrelated to the PR's stated purpose of improving form validation and user feedback for the Report Issue form. The PR description mentions fixes for #607 and #608, which concern the report form, but this test validates machine owner display functionality. Consider moving this test to a separate PR focused on machine owner display features.