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

upcoming: [M3-8364] - Use React Hook Form instead of Formik for Create Bucket #10699

Merged
merged 14 commits into from
Jul 27, 2024

Conversation

jaalah-akamai
Copy link
Contributor

@jaalah-akamai jaalah-akamai commented Jul 22, 2024

Description 📝

Switching from Formik to React Hook Form before updating the drawer with the new UI / API requirements.

Changes 🔄

  • Using isFeatureEnabledV2 instead of deprecated isFeatureEnabled
  • Unique label validation is happening in the schema now
  • Reset of values is now happening onExited

Target release date 🗓️

N/A

Preview 📷

N/A

How to test 🧪

Note

Ensure you test both legacy and multi-cluster (gen1) by toggling flag

Verification steps

  • Create a new bucket and ensure the creation process still works
  • Try creating a bucket with a label that's already used and ensure the validation works
    • You should see an error with helper text under label input field
  • The bucket creation form should be more responsive, with real-time validation feedback
  • Form fields should validate input as the user types, not just on submission
  • The form should prevent submission if there are any validation errors

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • [ ] 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@jaalah-akamai jaalah-akamai self-assigned this Jul 22, 2024
@jaalah-akamai jaalah-akamai requested a review from a team as a code owner July 22, 2024 17:27
@jaalah-akamai jaalah-akamai requested review from carrillo-erik and AzureLatte and removed request for a team July 22, 2024 17:27
@jaalah-akamai jaalah-akamai changed the title upcoming: [M3-8301] - Use React Hook Form instead of Formik for Create Bucket upcoming: [M3-8364] - Use React Hook Form instead of Formik for Create Bucket Jul 22, 2024
Copy link

github-actions bot commented Jul 22, 2024

Coverage Report:
Base Coverage: 82.46%
Current Coverage: 82.46%

@@ -11,7 +11,7 @@ interface Props {
onBlur: (e: any) => void;
onChange: (value: string) => void;
required?: boolean;
selectedCluster: string;
selectedCluster: string | undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The RegionSelect for which this is a wrapper has the value as string | undefined which I believe to be correct.

@jaalah-akamai jaalah-akamai marked this pull request as ready for review July 23, 2024 21:28
@jaalah-akamai
Copy link
Contributor Author

@bnussman I would love a review for the React Hook refactor / @cpathipa for overall sanity check that functionality still works for OBJ.

Copy link
Contributor

@cpathipa cpathipa left a comment

Choose a reason for hiding this comment

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

Create Bucket functionality is working as expected for Gen1 and legacy.

  • Able to create a new bucket for Gen1 and legacy.
  • Validation is working as expected for label that's already used.
  • Confirming on error with helper text under label input field
  • - Confirming form prevents submission if there are any validation errors.

Note: Since we are using V2 flag, we should enable the flag in LD for Gen1 in respective environment when this is shipped to production.

@jaalah-akamai jaalah-akamai added the Add'tl Approval Needed Waiting on another approval! label Jul 24, 2024
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.

On an account without Object Storage enabled, I wasn't able to enable Object Storage

Screen.Recording.2024-07-25.at.9.10.11.AM.mov

setError,
watch,
} = useForm<CreateObjectStorageBucketPayload>({
context: { buckets: bucketsData?.buckets ?? [] },
Copy link
Member

Choose a reason for hiding this comment

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

Neat. Did not know about context

@jaalah-akamai
Copy link
Contributor Author

I need to fix the OBJ test since I'm now disabling Create Bucket unless they provide a label and a region, and the tests aren't accounting for that.

@hana-linode The clone-linode.spec.ts is looking for the old region format, surprised this wasn't caught in the PR where we changed this? 🤔

clone-linode.spec.ts.mp4

@carrillo-erik
Copy link
Contributor

I still need to test this with the multi-cluster feature flag turned on. The legacy experience worked as expected and did not see any visual regressions. Nothing in the code stood out on first inspection.

@jaalah-akamai jaalah-akamai requested a review from a team as a code owner July 25, 2024 21:46
@jaalah-akamai jaalah-akamai requested review from cliu-akamai and removed request for a team July 25, 2024 21:46
@jaalah-akamai jaalah-akamai added SDET Approval Needed and removed Add'tl Approval Needed Waiting on another approval! labels Jul 25, 2024
Copy link
Contributor

@AzureLatte AzureLatte 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!

@hana-akamai
Copy link
Contributor

The clone-linode.spec.ts is looking for the old region format, surprised this wasn't caught in the PR where we changed this? 🤔

@jaalah tests were not updated since we're still going to display the old format for now since Gecko LA/GA got pushed 2 months (new format is feature flagged under Gecko GA). The tests passed on my PR 🤔

@carrillo-erik
Copy link
Contributor

The clone-linode.spec.ts is looking for the old region format, surprised this wasn't caught in the PR where we changed this? 🤔

@jaalah tests were not updated since we're still going to display the old format for now since Gecko LA/GA got pushed 2 months (new format is feature flagged under Gecko GA). The tests passed on my PR 🤔

@jaalah-akamai @hana-linode
I experienced the region label issue in the clone-linode.spec.ts when I tried to merge M3-8142. In order to have the tests pass without updating the tests, I had to comment out the gecko2 feature flag from the useFlags.ts file.

  return {
    ...flags,
    ...mockFlags,
    // gecko2: {
    //   enabled: true,
    //   ga: true,
    // },
  };

@jaalah-akamai
Copy link
Contributor Author

@carrillo-erik Yea this issue is stemming from #10702. We are looking into a patch to fix this without having to revert the work. Until then if you need to get your tests passing, you can disable the gecko2 flag in your mock test. Something like this should work

mockAppendFeatureFlags({
   gecko2: makeFeatureFlagData(false),
}).as('getFeatureFlags');

Comment on lines +13 to +31
.test(
'unique-label',
'A bucket with this label already exists in your selected region',
(value, context) => {
const { cluster, region } = context.parent;
const buckets = context.options.context?.buckets;

if (!Array.isArray(buckets)) {
// If buckets is not an array, assume the label is unique
return true;
}

return !buckets.some(
(bucket) =>
bucket.label === value &&
(bucket.cluster === cluster || bucket.region === region)
);
}
),
Copy link
Member

Choose a reason for hiding this comment

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

In my opinion, I think we should just remove this logic and rely on API errors for uniqueness, but this seems fine!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I haven't looked too deeply into this yet, but it seems that no API error is caught in the case of a duplicate. Instead, it silently reverts the optimistic UI rendering of the bucket or something similar. I'll investigate this further and try to address it in my next PR. For now, I think the validation is useful. 👍

Copy link
Member

Choose a reason for hiding this comment

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

Ohh, got it! Thanks for the investigation 🙏

Comment on lines 219 to 220
!watchLabel ||
!watchRegion ||
Copy link
Member

Choose a reason for hiding this comment

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

From previous cafe discussion, UX decided that we should to keep form submit buttons enabled globally so that the user can submit at any time and receive validation errors.

Not a huge deal here because this is a small form, but this will diverge from our pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok no problem, I might have forgotten we talked about this 👍

@carrillo-erik
Copy link
Contributor

Tested and verified all workflows related to these changes. In addition, all tests are now ✅.

@jaalah-akamai
Copy link
Contributor Author

This appears to be a flaky e2e test looking at detail/PR-10699/9/pipeline.

Screenshot 2024-07-26 at 9 13 00 PM

@jaalah-akamai jaalah-akamai merged commit 62b5b7b into linode:develop Jul 27, 2024
18 of 19 checks passed
@jaalah-akamai jaalah-akamai deleted the M3-8301-v2 branch July 27, 2024 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants