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

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

Closed
ftrotter opened this issue Oct 30, 2019 · 8 comments
Closed

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

ftrotter opened this issue Oct 30, 2019 · 8 comments
Assignees
Labels
1.0 requirement Things we need in initial release bug Ready for QA

Comments

@ftrotter
Copy link
Contributor

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:

  • Either you have to enter the data that is called for....
  • or you have to setup a DEFAULT value for the database to use when the value is not set.

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.

@ftrotter ftrotter added bug 1.0 requirement Things we need in initial release labels Oct 30, 2019
@kchapple
Copy link
Contributor

@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
Copy link
Contributor Author

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
Copy link
Contributor

kchapple commented Nov 13, 2019

@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
Copy link
Contributor Author

ftrotter commented Nov 17, 2019

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 added a commit that referenced this issue Nov 20, 2019
@kchapple
Copy link
Contributor

kchapple commented Nov 20, 2019

@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
Copy link
Contributor Author

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
Copy link
Contributor

@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

kchapple added a commit that referenced this issue Jan 31, 2020
intermediate

intermediate

intermediate

intermediate

intermediate

Finished testing

add test table
kchapple added a commit that referenced this issue Feb 5, 2020
Add null checkbox and behavior for null and default values (#48)(#28)
@seanccsmith
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 requirement Things we need in initial release bug Ready for QA
Projects
None yet
Development

No branches or pull requests

3 participants