posit-dev / publisher

MIT License
3 stars 0 forks source link

Credential Validation with enhanced multi-stepper validation #1882

Closed sagerb closed 4 days ago

sagerb commented 5 days ago

Intent

Resolves #1611 Resolves #1612

This PR introduces validation when creating a credential, for the server URL and the API KEY associated with that server URL.

The ask was that validation be added while maintaining the best user experience. In particular, we did not want to surface errors or perform validation on every keystroke, since these validations require non-trivial API calls to the agent which then has to call into remote Connect servers.

Type of Change

Approach

  1. The multi-stepper framework (for input boxes) now supports a final validation callback, which is called when the user presses the Enter key to move forward to the next step. It functions in the same way as the original validation callbacks, in which a promise is resolved to undefined if there is no error, or to a string or InputBoxValidationMessage if there is an error.
  2. Progress indicators have been added to be active while validation is pending.
  3. The multi-stepper framework has also been changed to make the validation callback optional.
  4. Existing validation checks have been moved into the finalValidation checks, when they are not immediate failures or could represent an intermediate stage of the user entering in the info. This improves the user experience by hiding validation errors when the user is not yet finished interacting with the input box.
  5. The new credential validation methods have been added into the API and types.
  6. New finalValidation methods have been implemented which provide for validation of the server URLs and API KEYs.
  7. Error messages have been standardized in format across all of the multi-steppers.
  8. Duplicate code between newDeployment.ts and newCredential.ts have been synced, with a few changes that allow for easier syncing in the future.

The following screenshots should provide for a flavor of the new experience:

NewCredential Flow:

2024-06-26 at 3 25 PM 2024-06-26 at 3 26 PM 2024-06-26 at 3 28 PM 2024-06-26 at 3 29 PM 2024-06-26 at 3 31 PM

Automated Tests

Directions for Reviewers

The new deployment and new credential flows were affected by these changes. Please execute happy and unhappy paths through these multi-steppers to validate the changes.

Checklist

sagerb commented 4 days ago

Thanks for the responses. With all of that answered this looks good to me.

One thing I did notice is if the server isn't up you cannot make a credential. I'm not sure if we want to discuss that at all, but if you are, for example, making a credential for your Yeti server you won't be able to unless your Yeti instance is up. That is frequently my go to for making a new credential when testing.

I can see still wanting to see the error, but maybe being able to skip validation as a follow-up? I'm not even sure if that is a possibility with VSCode's inputs.

Yes, that is worth calling out. I also wondered if we wanted to allow a "continue anyways", but in the end, I decided it was probably better to force the validation here. For now, we can wait on user feedback I think.

If we do get some in that area, I think we have a few options: 1) add a button that would allow a "Continue Anyways", shown when the error is active. 2) modify the steps/flow to still move to a continue anyways step if the validation fails, rather than stay on the input page.

But let's wait...

dotNomad commented 4 days ago

Yes, that is worth calling out. I also wondered if we wanted to allow a "continue anyways", but in the end, I decided it was probably better to force the validation here. For now, we can wait on user feedback I think.

If we do get some in that area, I think we have a few options:

  1. add a button that would allow a "Continue Anyways", shown when the error is active.
  2. modify the steps/flow to still move to a continue anyways step if the validation fails, rather than stay on the input page.

But let's wait...

Sounds like a plan 🫡

kgartland-rstudio commented 4 days ago

nit: it takes exactly 30 seconds to fail when there's a bad URL, is there a reason for that? Can we fail sooner?

sagerb commented 4 days ago

nit: it takes exactly 30 seconds to fail when there's a bad URL, is there a reason for that? Can we fail sooner?

The backend is responsible for that, so in this case, the code within this PR is simply exposing that backend behavior. For a completely bogus URL, I'm seeing an immediate failure. (like https://sager2.org). @kgartland-rstudio what bad URL are you using?

kgartland-rstudio commented 4 days ago

Looks good tested in VSCode, Positron on Windows/MacOS/Ubuntu22

mmarchetti commented 4 days ago

I actually added a timeout parameter to the API in case the front end wanted to customize. If this is too short, valid URLs can fail; if too long, it is annoying or looks broken.