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

Wizrad: add condition to useHostnameValidation in case of empty string #2872

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mgold1234
Copy link
Collaborator

@mgold1234 mgold1234 commented Feb 10, 2025

this adds condition to useHostnameValidation in case of empty string if user decide to delete the value in hostname field
FIX ISSUE: 2917

Copy link

codecov bot commented Feb 10, 2025

Codecov Report

Attention: Patch coverage is 90.32258% with 3 lines in your changes missing coverage. Please review.

Project coverage is 82.09%. Comparing base (06f4ce4) to head (c18a152).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/Components/CreateImageWizard/validators.ts 83.33% 2 Missing ⚠️
...rc/Components/CreateImageWizard/ValidatedInput.tsx 93.75% 1 Missing ⚠️

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2872   +/-   ##
=======================================
  Coverage   82.09%   82.09%           
=======================================
  Files         207      207           
  Lines       23291    23313   +22     
  Branches     2285     2290    +5     
=======================================
+ Hits        19120    19139   +19     
- Misses       4144     4147    +3     
  Partials       27       27           
Files with missing lines Coverage Δ
...Components/CreateImageWizard/CreateImageWizard.tsx 97.24% <ø> (-0.01%) ⬇️
...mponents/CreateImageWizard/steps/Details/index.tsx 100.00% <100.00%> (ø)
...teImageWizard/steps/FileSystem/FileSystemTable.tsx 71.24% <100.00%> (+0.07%) ⬆️
...ents/CreateImageWizard/utilities/useValidation.tsx 84.70% <100.00%> (ø)
...rc/Components/CreateImageWizard/ValidatedInput.tsx 91.56% <93.75%> (+0.06%) ⬆️
src/Components/CreateImageWizard/validators.ts 92.56% <83.33%> (-1.25%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06f4ce4...c18a152. Read the comment docs.

Copy link
Collaborator

@lucasgarfield lucasgarfield left a comment

Choose a reason for hiding this comment

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

Nice catch, the validation should not appear when there is no hostname.

I’m not sure about the approach, though. In this PR we’re hiding the validation by returning an error object with a fieldname of ‘default’. That technically works and is supported by the HookValidatedInput but it just feels kind of surprising/counter-intuitive.

I think it feels counter-intuitive because <HookValidatedInput> does not surface validation on pristine state components. So there is already an existing pattern for not displaying validation – that’s what I intuitively would expect to use. In other words, set isPristine to true when the field’s value returns to the default state (in this case, ‘’).

This might be a good opportunity to refactor a little bit. The double nested ternary operator is hard to understand. It definitely does support this ‘feature’ where if the fieldName is ‘default’ then but let’s take a step back and ask… should it? Maybe we can get rid of the nested ternary and the concept of a ‘default’ state. Having to manage both ‘default’ and ‘isPristine’ feels duplicitous.

In all the validation hooks, it only seems to be used in one other place, and there is a comment specifically calling it out as a hacky solution…
image

Therefore, I am a little hesitant to introduce this new pattern to the code base. I’d prefer refactoring a bit. What do you think?

@mgold1234
Copy link
Collaborator Author

after some investigation, the solution that I did here https://github.com/osbuild/image-builder-frontend/pull/2844/files#diff-5206f54a601e8c56290e1a412fbf3f35172f16b892c7674852c91b4ea846bcd9R62
will fix that problem, so I can close this pr

@mgold1234 mgold1234 closed this Feb 11, 2025
@mgold1234 mgold1234 reopened this Feb 11, 2025
@mgold1234
Copy link
Collaborator Author

I saw your comment here #2844
so reopen this pr and refactor the code now

@mgold1234 mgold1234 force-pushed the hostname branch 5 times, most recently from 7f3c4c0 to 32f8bba Compare February 11, 2025 13:51
@mgold1234 mgold1234 force-pushed the hostname branch 8 times, most recently from d861369 to 61d3608 Compare February 18, 2025 09:47
@mgold1234 mgold1234 requested a review from regexowl February 19, 2025 12:49
@regexowl
Copy link
Collaborator

regexowl commented Feb 20, 2025

One small bug in FSC - the input doesn't get validated without a value
image

this is how the error for invalid value looks like:
image

There's also the same problem with Blueprint name which should be required:
image

this adds condition to useHostnameValidation in case of empty string
if user decide to delete the value in hostname field
@mgold1234
Copy link
Collaborator Author

mgold1234 commented Feb 20, 2025

nice catch @regexowl , I am looking at stage, if user delete the value in Blueprint name we should see an error be cecause it's mandatory field, am I wrong?
I fixed the FSC respectivally

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.

3 participants