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.

Sign up for GitHub

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

Decouple min/max validations from required for integer, float and string fields #1464

Merged
merged 14 commits into from
Jul 6, 2018
Merged

Decouple min/max validations from required for integer, float and string fields #1464

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
251b085
Decouple min/max validations from required for integer, float and string
houmark Jul 1, 2018
912a6c3
Correct wrong launder from copy/paste
houmark Jul 1, 2018
2a9f97b
Missing callback
houmark Jul 1, 2018
80cd559
Improve min and max filtering
houmark Jul 2, 2018
4039a75
Add required validation
houmark Jul 2, 2018
3f7bd0f
Add tests to cover integer / float fields focused on min / max converts
houmark Jul 2, 2018
c7f2b54
Add missing required check for integer
houmark Jul 2, 2018
43f17f6
Fix required validations
houmark Jul 2, 2018
e2eb48c
Fix wrong comparison, _.isEmpty is no good here
houmark Jul 2, 2018
da2d0f1
Added unit tests to cover the passthrough of a 0 value
houmark Jul 2, 2018
08435cd
Remove comments that should not have been there in the first place
houmark Jul 2, 2018
19528e6
Fix required validation for zero / negative values
houmark Jul 3, 2018
e2d00cb
Add many more tests to cover most of string / number / float
houmark Jul 3, 2018
2a4b1ac
Add parseFloat and parseInt on comparison checks
houmark Jul 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions lib/modules/apostrophe-schemas/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -977,14 +977,21 @@ module.exports = {
converters: {
string: function(req, data, name, object, field, callback) {
object[name] = self.apos.launder.string(data[name], field.def);
if (field.min && (object[name].length < field.min)) {
if (object[name] && field.min && (object[name].length < field.min)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's also possible I have to change object[name] to data[name] because launder may change the value to the default value and that was not what the user actually input. Which means that it will never fail if the configuration is min: 2, max: 5 and def: 3 but the user submitted 1 or null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a whole lot of things to consider here and this is fairly deep inside stuff and not so easy to test manually. I did do many tests to try to cover use cases, but due to my limited insight in Apostrophe I may not be the best person to do this all the way through. So I would for sure appreciate a second set of eyes (or more) and more testing with knowledge of how to provoke this and test it properly. I understand that can delay this PR getting merged and that's okay. I think decoupling this is a good base for doing more converters and validators, for exampel to compare two fields in the same form against each other, conditional so the check is only done if both are shown, with value etc.

// Would be unpleasant, but shouldn't happen since the browser
// also implements this. We're just checking for naughty scripts
return callback('min');
}
if (field.max && (object[name].length > field.max)) {
// If max is longer than allowed, trim the value down to the max length
if (object[name] && field.max && (object[name].length > field.max)) {
object[name] = object[name].substr(0, field.max);
}
// If field is required but empty (and client side didn't catch that)
// This is new and until now if JS client side failed, then it would
// allow the save with empty values -Lars
if (field.required && !data[name]) {
return callback('required');
}
return setImmediate(callback);
},
form: 'string'
Expand Down Expand Up @@ -1402,6 +1409,15 @@ module.exports = {
converters: {
string: function(req, data, name, object, field, callback) {
object[name] = self.apos.launder.integer(data[name], field.def, field.min, field.max);
if (field.required && !data[name]) {
Copy link
Member

Choose a reason for hiding this comment

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

This would reject 0 whenever required, which is not the same as no entry and should satisfy "required" for an integer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good point. This went from complex to simple and back to complex for me. I just realized late last night that this creating more issues in the core than I first expected after my initial commits. I will go over this once again and take care of the 0 situation and also try to address negative max.

return callback('required');
}
// This makes it possible to have a field that is not required, but min / max defined
// This allows the form to be saved and sets the value to null if no value was given by
// the user.
if (!data[name] && data[name] !== 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks good

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, same thought: can't you simplify this by looking at object[name] which contains the output of launder, guaranteed to be an integer or whatever field.def is?

object[name] = null;
}
return setImmediate(callback);
},
form: 'string'
Expand Down Expand Up @@ -1437,6 +1453,12 @@ module.exports = {
converters: {
string: function(req, data, name, object, field, callback) {
object[name] = self.apos.launder.float(data[name], field.def, field.min, field.max);
if (field.required && !data[name]) {
Copy link
Member

Choose a reason for hiding this comment

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

Same concern re: 0

return callback('required');
}
if (!data[name] && data[name] !== 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks good

object[name] = null;
}
return setImmediate(callback);
},
form: 'string'
Expand Down
1 change: 1 addition & 0 deletions lib/modules/apostrophe-schemas/public/css/user.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
&::after {
margin-left: 10px;
content: "(" attr(data-apos-error-message) ")";
color: inherit;
}
}
}
Expand Down
24 changes: 16 additions & 8 deletions lib/modules/apostrophe-schemas/public/js/user.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,8 +191,16 @@ apos.define('apostrophe-schemas', {
field: field,
type: type
};
if (type === 'required') {
error.message = 'Required';
switch (type) {
case 'required':
error.message = 'Required';
break;
case 'min':
error.message = 'Min. permitted value is: ' + field.min;
Copy link
Member

Choose a reason for hiding this comment

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

Great. The thinking was "red is better than nothing, we'll add in the messages later." About time. (:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to keep the text short as it has to fit above a text field after the label for the field, including on smaller screens hence "Min.". Open for suggestions for synonyms for "permitted" or a full rewrite of this error message.

break;
case 'max':
error.message = 'Max. permitted value is: ' + field.max;
break;
}
return error;
};
Expand Down Expand Up @@ -864,12 +872,12 @@ apos.define('apostrophe-schemas', {
err = self.error(field, 'required');
return setImmediate(_.partial(callback, err));
}
if (field.max && (data[name].length > field.max)) {
if (data[name] && field.max && (data[name].length > field.max)) {
err = self.error(field, 'max');
err.message = 'Maximum of ' + field.max + ' characters';
return setImmediate(_.partial(callback, err));
}
if (field.min && (data[name].length < field.min)) {
if (data[name] && field.min && (data[name].length < field.min)) {
err = self.error(field, 'min');
err.message = 'Minimum of ' + field.min + ' characters';
return setImmediate(_.partial(callback, err));
Expand Down Expand Up @@ -1039,10 +1047,10 @@ apos.define('apostrophe-schemas', {
if (field.required && (!(data[name] && data[name].length))) {
return setImmediate(_.partial(callback, 'required'));
}
if ((field.max !== undefined) && (data[name] > field.max)) {
if (data[name] && field.max !== undefined && data[name] > field.max) {
Copy link
Member

Choose a reason for hiding this comment

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

Same issue with 0 (the max could be less than 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tested with negative values and it works (after I added the parseFlot / parseInt) inside the comparison check. It also worked before (which I don't really understand why).

Checking if min is larger than max etc. is not done in any way, but that should be handled elsewhere, in the schema validation more than here so that it would error in the console if a field is configured in a way that wouldn't work (I did not look around to see if that's already done).

But the overall resume here is that min and max should be working fine on UI level, including negative min max, so should 0 values (including for validation) and so this should be good to go unless I missed something crazy here.

return setImmediate(_.partial(callback, 'max'));
}
if ((field.min !== undefined) && (data[name] < field.min)) {
if (data[name] && field.min !== undefined && data[name] < field.min) {
Copy link
Member

Choose a reason for hiding this comment

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

Same issue with 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0 is not an issue here. The value is a string and is checked as a string so the length of 0 is 1 for the required validation at least. What is a concern, though, is that the the comparison of min and max is done as string (input) vs. number (min/max configuration) and so I will add a parseFloat around the value, but inside the check if, because I don't want any more side effects that I cannot foresee by modifying the value on the outside.

Let me know what you think about this. Code coming in shortly.

return setImmediate(_.partial(callback, 'min'));
}
return setImmediate(callback);
Expand All @@ -1060,10 +1068,10 @@ apos.define('apostrophe-schemas', {
if (field.required && (!(data[name] && data[name].length))) {
return setImmediate(_.partial(callback, 'required'));
}
if ((field.max !== undefined) && (data[name] > field.max)) {
if (data[name] && field.max !== undefined && data[name] > field.max) {
Copy link
Member

Choose a reason for hiding this comment

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

same re: 0

return setImmediate(_.partial(callback, 'max'));
}
if ((field.min !== undefined) && (data[name] < field.min)) {
if (data[name] && field.min !== undefined && data[name] < field.min) {
Copy link
Member

Choose a reason for hiding this comment

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

same re: 0

return setImmediate(_.partial(callback, 'min'));
}
return setImmediate(callback);
Expand Down
Loading