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

docs: [M3-8932] - Add Form Validation Best Practices #11298

Merged
merged 5 commits into from
Nov 22, 2024

Conversation

jaalah-akamai
Copy link
Contributor

@jaalah-akamai jaalah-akamai commented Nov 20, 2024

Description 📝

Currently, form validation implementations vary across the application. We need to establish a pattern for extending API validation schemas that maintains a clear separation between API contract validation and UI-specific validation requirements.

Changes 🔄

Create documentation explaining:

  • When to use simple schema extension vs. full resolver pattern
  • How to extend API validation schemas
  • Best practices for async validation using QueryClient
  • Examples of common validation patterns
  • Establish pattern for simple validation extensions

Target release date 🗓️

12/10

@jaalah-akamai jaalah-akamai self-assigned this Nov 20, 2024
@jaalah-akamai jaalah-akamai added the Documentation Improving / adding to our documentation label Nov 20, 2024
@jaalah-akamai jaalah-akamai marked this pull request as ready for review November 20, 2024 20:46
@jaalah-akamai jaalah-akamai requested a review from a team as a code owner November 20, 2024 20:46
@jaalah-akamai jaalah-akamai requested review from mjac0bs and abailly-akamai and removed request for a team November 20, 2024 20:46
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Copy looks good and clear to me

Not sure why this test is failing, also you'll want to add a changeset

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Looks good! I do worry that the Linode Create example will unintentionally cause inexperienced developers to add unnecessary complexity, but can't say for sure.

We could consider using this example

const resolver: Resolver<UpdateImageRegionsPayload, Context> = async (
values,
context
) => {
const availableRegionIds = context?.imageRegions
?.filter((r) => r.status === 'available')
.map((r) => r.region);
const isMissingAvailableRegion = !values.regions.some((regionId) =>
availableRegionIds?.includes(regionId)
);
const availableRegionLabels = context?.regions
?.filter((r) => availableRegionIds?.includes(r.id))
.map((r) => r.label);
if (isMissingAvailableRegion) {
const message = `At least one available region must be present (${availableRegionLabels?.join(
', '
)}).`;
return { errors: { regions: { message, type: 'validate' } }, values };
}
return { errors: {}, values };
};

but it may not be complex enough 😅

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Approving pending changeset is added.

re: Banks' concern:

worry that the Linode Create example will unintentionally cause inexperienced developers to add unnecessary complexity, but can't say for sure.

What if we used Images for the code snippet, but then said something like: 'for a more complex example, see Linode Create' and linked?

@mjac0bs mjac0bs added the Approved Multiple approvals and ready to merge! label Nov 21, 2024
Copy link

Coverage Report:
Base Coverage: 86.81%
Current Coverage: 86.81%

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🔺 2 failing tests on test run #3 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
2 Failing451 Passing2 Skipped112m 49s

Details

Failing Tests
SpecTest
linode-storage.spec.tslinode storage tab » delete disk
linode-storage.spec.tslinode storage tab » resize disk

Troubleshooting

Use this command to re-run the failing tests:

yarn cy:run -s "cypress/e2e/core/linodes/linode-storage.spec.ts,cypress/e2e/core/linodes/linode-storage.spec.ts"

@jaalah-akamai jaalah-akamai merged commit a52c2ba into linode:develop Nov 22, 2024
21 of 23 checks passed
Copy link

cypress bot commented Nov 22, 2024

Cloud Manager E2E    Run #6869

Run Properties:  status check failed Failed #6869  •  git commit a52c2ba9bc: docs: [M3-8932] - Add Form Validation Best Practices (#11298)
Project Cloud Manager E2E
Branch Review develop
Run status status check failed Failed #6869
Run duration 33m 10s
Commit git commit a52c2ba9bc: docs: [M3-8932] - Add Form Validation Best Practices (#11298)
Committer Jaalah Ramos
View all properties for this run ↗︎

Test results
Tests that failed  Failures 4
Tests that were flaky  Flaky 3
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 451
View all changes introduced in this branch ↗︎

Tests for review

Failed  cypress/e2e/core/images/machine-image-upload.spec.ts • 4 failed tests

View Output Video

Test Artifacts
machine image > uploads machine image, mock finish event Screenshots Video
machine image > uploads machine image, mock upload canceled failed event Screenshots Video
machine image > uploads machine image, mock failed to decompress failed event Screenshots Video
machine image > uploads machine image, mock expired upload event Screenshots Video
Flakiness  linodes/linode-config.spec.ts • 1 flaky test

View Output Video

Test Artifacts
Linode Config management > End-to-End > Clones a config Screenshots Video
Flakiness  linodes/update-linode-labels.spec.ts • 1 flaky test

View Output Video

Test Artifacts
update linode label > updates a linode label from details page Screenshots Video
Flakiness  parentChild/account-switching.spec.ts • 1 flaky test

View Output Video

Test Artifacts
Parent/Child account switching > From Parent to Child > can switch from Parent account user to Proxy account user from Billing page Screenshots Video

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Documentation Improving / adding to our documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants