Bug 1897276: Vsphere provider creation/edition certificate validation

[gildub]

Addresses #833

[konveyor-preview-bot]

🚀 Deployed Preview: http://konveyor-forklift-ui-pr-845-preview.surge.sh ✨

Compare with current main branch: http://konveyor-forklift-ui-preview.surge.sh

[mturley]

Looking good, but there are a few state things we can clean up here.

Also, I wonder if we should be showing other certificate details in addition to just the fingerprint, like issuer and expiration date. That would give the user more info to go by to know if the cert is valid. @vconzola what do you think? I left a similar comment on slide 7 of the mockup. The cert details could be in a little info box and the checkbox could say something more generic like "I trust the authenticity of this certificate" if we are showing more details.

[mturley]

whoa!! I was not aware of this TypeScript feature, I really wish I had discovered this sooner! That's really nice. I think we could use it in a few places where we have logic based on providerType and mappingType with a lot of as assertions. Future cleanup :)

[mturley]

As you suggested on Slack, I do think we should probably move this stuff to a useCertificateQuery hook so we're not cluttering this component. assertIsCertificate and getCertificate could be moved into there. I think we should also have the query result as a single variable instead of destructuring it, so we can use certificateQuery.status instead of status so the usage is more readable below.

[mturley]

I think we can simplify things and reduce the amount of state we're using here.

• I think we should define isCertificateValid as a required field in the vsphere section of useAddProviderFormState rather than in its own useState. We may also be able to just remove the fingerprint and fingerprintFilename fields from there, but we'd need to pass the fingerprint data (from the query) as an additional param when we call mutateProvider (which comes from useCreateProviderMutation and usePatchProviderMutation). In general I think it is helpful to have the useFormState stuff match the form fields we display as much as possible so we can leverage all its built in stuff.
• By default (without the custom onBlur here), the fields.hostname.isTouched boolean will not be true until the blur event happens (because that's what triggers the validation errors to appear). Currently the validation on the hostname field is broken here because you're not calling setIsTouched in your custom onBlur. But I think we can restore the default behavior by removing the custom onBlur and isCertificateQueryEnabled state entirely, because:
  • If we replace the setCertificateQueryFlag(false) in onFocus with fields.hostname.setIsTouched(false), we can use fields.hostname.isTouched as our value for whether the query should be enabled.
• If we do that, we also don't need the hostname/setHostname state, because we already have fields.hostname.value.

So in short, I think all we really need is the isCertificateValid to be part of the form state (so isFormValid works properly), and to manage the isTouched value of the hostname field as our indicator of whether the query should run, and we can get rid of all the other new state you've introduced here. Everything below that uses isCertificateValid can use fields.isCertificateValid.value, and your useCertificateQuery can take fields.hostname.value as a hostname param and fields.hostname.isTouched as an enabled param.

[gildub]

@mturley,

Thank you for your thorough review.
With latest commit is as close I could get to the expected behavior.
The reason I keep using a state hostname is to make benefit of having a button triggering the fetch and avoid trying to fetch the certificate at each key stroke when hostname is entering.

[mturley]

Per the mockups, this field should still be rendered, but just disabled if the certificate checkbox is unchecked.

[mturley]

Same here.

[vconzola]

Looking good, but there are a few state things we can clean up here.

Also, I wonder if we should be showing other certificate details in addition to just the fingerprint, like issuer and expiration date. That would give the user more info to go by to know if the cert is valid. @vconzola what do you think? I left a similar comment on slide 7 of the mockup. The cert details could be in a little info box and the checkbox could say something more generic like "I trust the authenticity of this certificate" if we are showing more details.

@mturley I really like that idea. I think it'd be more helpful to the user than just seeing the fingerprint. @gildub Lemme update the mockups this afternoon and send them to you.

[mturley]

Another note: when it comes time to deal with editing, I think we just need to be sure to modify useEditProviderPrefillEffect to match the changes you make to useAddProviderFormState here (including making sure the hostname's isTouched gets set to true so the form fetches the certificate right away). Then I think we will need something that runs after the fingerprint is loaded (a useEffect, or maybe just an onSuccess param we pass to the certificate query?) with the following logic:

• if the fingerprint we loaded matches the fingerprint on the providerBeingEdited, we can check the validated box.
• else, we need to leave that box unchecked and display an alert that the fingerprint changed and the user should make sure they trust the new certificate.

[mturley]

One more thought: @vconzola was mentioning that it might be weird to have the cert fetch on blur, because what if the user types and doesn't click away from the field because the rest of them are disabled, and they are confused about why nothing is happening? We could have a custom onChange and in addition to setting the value, it could clear/set a timeout (stored in a ref) so that it sets isTouched when they stop typing for X amount of time rather than when they blur the field.

Edit: onChange, not onBlur

Edit 2: hmm, we'd also have to set isTouched to false in onChange as well so if they continue typing it fetches again.

[mturley]

For reference, these are the default text field props that you have to replicate if you override them with additional custom behavior. We should document that better in lib-ui maybe.

export const getTextFieldProps = (
  field: IValidatedFormField<string> | IValidatedFormField<string | undefined>
): Pick<TextInputProps | TextAreaProps, 'value' | 'onChange' | 'onBlur' | 'validated'> => ({
  value: field.value,
  onChange: field.setValue,
  onBlur: () => field.setIsTouched(true),
  validated: field.isValid ? 'default' : 'error',
});

[gildub]

In reply of your latest comment in regards of getTextFieldProps, do we have to override all the props?

[mturley]

In reply of your latest comment in regards of getTextFieldProps, do we have to override all the props?

No, those are just the default values provided by ValidatedTextInput, so if you do override onBlur for example you need to make sure you are calling field.setIsTouched(true) because it will no longer happen for you automatically. I think maybe this indicates a flaw in my component 😆

[gildub]

@vconzola, please review.

[gildub]

@mturley, I have no idea why but the field validation part of the isDisabled prop of the "Verify certificate" button fails the test even though the hostname field contains a valid url.

[mturley]

@gildub that's quite strange. Looking into it now

[mturley]

See slack for my findings :)

[github-actions]

Valid bug 1897276

[gildub]

@vconzola, per your question on Slack,

Not sure there’s anything we can do about this, but the Verify button is hidden below the fold when either the Add or Edit modals opened. Can we auto-scroll the form when the user clicks into either the user name or password field?

Moving the focus between different parts of the modal could be done but what about moving the hostname to be after the other input fields?
This way it's just before the "verify certificate" and then the certificate information and "Validate certificate" checkbox are next to each others.
Latest commit shows such approach. Please let me know what you think.
If that feel wrong then we can try moving focus or look at other solutions such as a dedicated pop-up for the certificate.

[gildub]

Having the hostname is really weird and unusual. So I reverted it.
Let's discuss further.

[mturley]

Hey Gilles. I pushed a few commits here, feel free to revert if you need to, but I had it working in troubleshooting so I figured I may as well commit.

• Fixed the verify button enable issue in edit mode by calling setIsTouched on the hostname field when prefilling the form
• Replaced destructuring with a single variable for certificateQuery to make later code more readable (having variables simply called status and data was unclear imo)
• Added a warning when editing a provider if the loaded fingerprint is different from the one stored in the provider
• Per @vconzola's request, made the modal scroll down to show the verify button if it is offscreen when focusing the username/password fields

[gildub]

Mike,

That's great, no need to revert !

Thanks for troubleshooting the validation field issue. Fixing it will be great although we'll a fair amount of testing to do ;)
The rest is nice and makes sense.

I added a patch to mock scrollIntoView to be able to pass the tests. This is because scrollIntoView is missing from jsdom: jsdom/jsdom#1695

[mturley]

@seanforyou23 @gildub are we happy with this?

[gildub]

@mturley, I'm happy with this now.

[mturley]

Discussing things with Mike S, I pushed a few more commits to surface error messages from fetching the certificate all the way to the UI. I think we can merge this and follow up in another PR perhaps, but the main thing remaining here I think is that if the certificate request fails, the button should reappear so it can be retried.

[mturley]

Good to merge, we can address the above in a followup.

[mturley]

Followup issue: https://github.com/konveyor/forklift-ui/issues/868

[mturley]

Hmm... CI is failing on createEvent here again. Not sure why

[mturley]

@seanforyou23 @gildub I'm pretty baffled as to why this error is coming up when running tests in CI but not locally. Any ideas?

[gildub]

@mturley, yeah I know, really frustrating I already looked at it and I've no idea why.

[gildub]

@mturley, I've started investigation...

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
8 Code Smells

No Coverage information
2.0% Duplication

[gildub]

@mturley,

I was able to reproduce CI error locally using Ubuntu image and following same pipeline.
Meanwhile I'm also getting the same error when running CI locally on my machine using Nodejs 16 LTS.
So maybe my env was out of sync (forgot to run npm install).
So the positive side is that if anything like that (a CI mismatch) occurs again then we can simply run the Ubuntu env to compare. Although there is no reason this should happen because Ubuntu or Fedora or are all running on same Node.js LTS latest version.

Also I worked around the error itself by using scrollIntoView with React.useRef().

[mturley]

Good find, thanks @gildub ! The ref solution is interesting, I guess that just means something is different about that global document in jsdom or something like that. Glad it's working!