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

Validate Session Title with Yup #8236

Merged
merged 7 commits into from
Nov 26, 2024
Merged

Conversation

jrjohnson
Copy link
Member

First step in making a transition away from our current classic decorator valdidation approach and into something more modern and less prone to blowing up horribly when decorator support lands in the language officially.

This has some significant advantages over our current approach:

  1. We can declare many smaller validation subsets if we want as each is simple an object we can manipulate
  2. The API on the component is simplified (I think it can get even better, but this was a good place to stop)
  3. Less magic with creating and setup as you can see it all in the class

I'm not a huge fan of the setupErrorMessages function on file load invocation as I don't really know why that works and doesn't get called on every import, I wasn't able to get something going with an initializer, but optimization there may be possible.

This seemed like a natural stopping place for comments. I can either follow up in this PR with migrations for everything else, or more likely we'll tackle them one at a time.

This is the validation library suggested by Mainmatter, let's give it a
try.
This is a very simple case, but validations work and are translated.
Doing this in an initializer isn't a good idea, it breaks everything
when we try and get to the application in tests, but this is a starting
place.
The initializer was a no go, it blew up tests. Plus this was everything
is in the same file. When the file is loaded so are the error message
translations.

We extract any needed information for the translation from the error and
send the entire thing to the message component. That component then does
the translation and subs in the description when needed.
By calling validate when the error display is triggered we ensure that
errors will be present and that properties will get autotracked that are
validated. This, in turn, let's us remove the asynchronous code from a
component and just access the validation object's errors as they are
needed.
Wait 250 milliseconds each time we validate in order to not validate
every single keystroke, but still respond with errors quickly for users.
@michaelchadwick
Copy link
Contributor

How would I test a "NewSession" to see if the validation works or not?

@jrjohnson
Copy link
Member Author

Testing:
Visit a course, scroll to the sessions list, click the plus to open the new session form, have at it!

@michaelchadwick
Copy link
Contributor

For some reason, I was thinking of a "NewSession" as a new user session, like when you log in /facepalm

@jrjohnson
Copy link
Member Author

Failing test is for real, I need to swap the way it debounces, it works for user input, but the test fails because async is hard and there seems to be a ordering issue with calling it this way.

Should be a pretty minor fix, this is still ready for comments.

@michaelchadwick
Copy link
Contributor

First thing I notice is that the form actually seems to get submitted before the validations are run now. Click submit button, see spinner, see validations. Before it would show up before spinning. Not sure if that's better or worse, but it does incur a minor speed decrease.

The validator was running out of sync so things would appear as valid
even if they weren't. Now things run in the right order.

I removed the validation run from the all fields error display, this
should really be used exclusively with saving and at that point the
consumer should run a full validation and check the result.
It's probably fixed now, but the new session save was doing a boolean
check which could be undefined and causing and issue. I think this is a
better API for the validations.
@jrjohnson
Copy link
Member Author

the form actually seems to get submitted before the validations are run now.

I think you may have been seeing the same async weirdness that was breaking in the test, I've moved some stuff around.

I also reduced the debounce because I think that 250ms was actually too long and felt janky.

@michaelchadwick
Copy link
Contributor

@jrjohnson Your change fixed what I was seeing :D

@jrjohnson jrjohnson marked this pull request as ready for review November 20, 2024 19:13
Copy link
Member

@stopfstedt stopfstedt left a comment

Choose a reason for hiding this comment

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

i'm digging this. left a few comments.

@stopfstedt stopfstedt assigned stopfstedt and dartajax and unassigned stopfstedt Nov 22, 2024
@dartajax dartajax added the run ui tests Run the expensive UI tests label Nov 25, 2024
@dartajax dartajax merged commit 4384a3d into ilios:master Nov 26, 2024
43 checks passed
@jrjohnson jrjohnson deleted the validate-with-yup branch November 26, 2024 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run ui tests Run the expensive UI tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants