CareSet / DURC

DURC is reverse CRUD
MIT License
3 stars 2 forks source link

column X cannot be null needs a custom error screen. #48

Closed ftrotter closed 4 years ago

ftrotter commented 4 years ago

The current error screen when a field is left blank in DURC looks like this:

Screen Shot 2019-10-30 at 10 49 54 AM

The problem with that is that it does not clearly explicate the decision that a user of DURC faces. The decision is simple:

More importantyl, this errors screen makes it seem like DURC is broken... but it is not.. it is working exactly as it is intended. When data is not filled in correctly.. the DURC is correct in refusing to save the data.

So the above choice needs to presented to the user in a manner that makes it clear that there is a user choice here.. and not actually a problem.

This relates to #26 which addresses the issue of not having the back button re-populate the form correctly from this error screen.

kchapple commented 4 years ago

@ftrotter I've done a bunch of work on this, and want to check in with your thoughts, and make sure they're aligned with mine.

Currently, there is no validation on DURC forms and a DURC model/controller won't allow a default value as set by the database. My idea to resolve this is to use a set of rules that will govern how default values are implemented, and use Bootstrap Validation and some informative validation messages to indicate to the user what is happening. So far, I have implemented this so that the DURC mine command will also mine a column's default value, if it's nullable, and if it's auto-increment. Then we put that information through these rules to determine the validation scheme.

Here are the rules that seem appropriate :

  1. If DB column is NOT nullable, and there is a default value, use the default value. Put the default value in the "placeholder" field.
  2. If DB column is NOT nullable and an empty string is provided, inform user that value is required column (see screenshot)
  3. If the DB column IS nullable, and an empty string is entered, use empty string (or NULL??) when saving whether there is default or not
  4. If a value is passed, use the value
  5. If the field is auto-increment, allow an empty string which will kick-in the auto-generator, but also allow a valid int value

Screen Shot 2019-11-13 at 10 02 35 AM

ftrotter commented 4 years ago

You are taking a more expansive approach to this and i like that but not a as a 1.0 feature.

For 1.0 i just want to make sure that you do not see innappropriate error messages. So i want a general error view that says "you are still working with an operational system that has not crashes, but you are entering data wrong".

Later we will make the front end smarter, but bear in mind that people can replace the frontend forms but still use the backend POST saving capability. So the backend cannot appear to be broken when it refuses to save wrong data..

kchapple commented 4 years ago

@ftrotter I think that what you describe can be mostly achieved by catching the exception and redirecting back to the form with a generic error message. It's fairly simple to add the validation to the front end, though (and mostly done, just need to add to the couple other form components.)

I guess one main thing that got me down this path, and was the more difficult part that required mining the default value and nullable meta-data from the database, was this line from your original post:

"or you have to setup a DEFAULT value for the database to use when the value is not set"

... because even if there is a default value, the models and controllers are not set up to know to use a default value.

So, if we do a simple catch-exception and redirect, even if there is a default value in the DB, if the user submits an empty string on a non-nullable field, the exception will be thrown. Unless we know a default value to set up the model, this cannot be implemented in eloquent. I have implemented default values by using the meta-data to set the built-in $attributes property (where you can set default values) on the eloquent model, and keeping and array of non-nullable-fields.

ftrotter commented 4 years ago

ok, I think i understand what you are saying, and that is the right place to start from. Lets get rid of the scary error screen using the catch-exception and redirect, and then lets discuss the more complex underlying concerns about "what happens when the user intends a blank" for the next version.

Lets say that the "catch-exception and redirect to error message" functionality should close this ticket, and lets record a new ticket taken from the front end work that you described....

kchapple commented 4 years ago

@ftrotter I have pushed the simplest solution to this. The error message is generic, and the color uses the default info message color. Let me know if this works for the immediate need.

Screen Shot 2019-11-20 at 1 05 39 PM
ftrotter commented 4 years ago

As of now, the current dev-master still craps out when a field saved without a value that cannot be NULL.

Also, I think it is fine to send the actual error message back instead of the generic error message and I also think it is fine to have the error be the "warning" color and not the default color.

Database errors are a part of what makes DURC tick, and having one is not an indication that DURC is broken, it is an indication that the the data is broken. That means if the saved data comes back with "Integrity constraint violation" anywhere in the error message, then it should be shown in DURCs front end view. And if it does not have that, then it should use the current error view.

In the future, we will want other specific error messages from the database to surface as "data errors" rather than "DURC errors". So we should have an array of strings that (when found in an error string) we are going to classify as "user errors" (which should all bounce back to the DURC interface as you are suggesting). Any error that is novel and not in our "list" of things we classify as user errors should get the current error view.

Does this work?

-FT

kchapple commented 4 years ago

@ftrotter The try/catch block is in the generated controller in the store() method. Can you verify that your controller has the try/catch block in store() method around the Eloquent model save(), otherwise, I might be doing something wrong checking in code/testing. The other stuff makes sense. Here's example from appstring:

Screen Shot 2019-12-10 at 8 40 42 AM
seanccsmith commented 4 years ago

Trying to save null values in non-nullable fields now throws a non-scary error. Closing issue.