SuffolkLITLab / docassemble-ALToolbox

Collection of small utility functions, classes, and web components for Docassemble interviews
https://suffolklitlab.org/docassemble-AssemblyLine-documentation/docs/framework/altoolbox
MIT License
14 stars 2 forks source link

Allow custom min/max/etc. and messages for date CustomDataTypes #158

Closed plocket closed 1 year ago

plocket commented 1 year ago

Our ThreePartsDate and BirthDate may now be able to have some custom validation using the parameters or code_parameters attributes of the CustomDataType class. For example, some kind of min and max values (almin, almax?) which could be python code.

We'd have to specify to users that it would have to be python code because we have to choose between a string value or code value when we make the parameters. Also, I can't yet find a way to automatically parse Mako.

Another aspect is allowing custom validation messages. I think those would need to be their own "top level" keys/directives because of where we can access those values in the custom data type class. For example:

fields:
  - "Birtdate": users[0].birthdate
    datatype: BirthDate
    almin: 01/01/0001
    alminmessage: "The answer needs to be an AD date"

Whether we can somehow work in custom validation code is a different question. From what I can tell so far, evaluating that client-side would involve using eval(), which is unsafe to call on arbitrary code. I'm not sure how we'd get the value server-side, though, given the tools we have.

plocket commented 1 year ago

I had forgotten the details of these custom datatypes which makes it more complicated. It's a bit hard to explain.

The current situation: The only validation we have right now is server-side validation when the user submits.

That's because we cannot use the jQuery validation plugin because we hide the element on which we would evaluate the validation. The plugin validation doesn't work for hidden elements.

That means I'm not sure what all we need to do to make everything work properly, like the validation message. I can try to hack it, but it may be unreliable.

parameters get added to the DOM. They're supposed to be used client side, not server side. I don't know of a way to get parameters into the server validation values.

What can we do? Other than hacking our own validation, I'm not sure. We avoided that in the past. Maybe it's time to dig into it.

plocket commented 1 year ago

I'm trying to see if we can somehow make a validation rule for the visible elements, but I'm not sure how the jq_rule custom datatype attribute is connected to the jquery validation item class name. That is, I've determined that the only things that need to match each other are those two things. It certainly isn't connected to any classname on the input element that I see when looking at the DOM. For example, this gets validated correctly:

from docassemble.base.util import CustomDataType, DAValidationError, word
import re

# The only parts that _need_ to match are the jquery rule and the
# validator method classname

class CCC(CustomDataType):
    name = 'aaa'
    container_class = 'da-eee-container'
    input_class = 'da-ddd'
    javascript = """\
$.validator.addMethod('bbb', function(value, element, params){
  return value == '' || /^[0-9]{3}-?[0-9]{2}-?[0-9]{4}$/.test(value);
});
"""
    code_parameters = ['kind']
    jq_rule = 'bbb'
    jq_message = 'You need to enter a valid AAA.'
    @classmethod
    def validate(cls, item):
        item = str(item).strip()
        m = re.search(r'^[0-9]{3}-?[0-9]{2}-?[0-9]{4}$', item)
        if item == '' or m:
            return True
        raise DAValidationError("A SSN needs to be in the form xxx-xx-xxxx")
    @classmethod
    def transform(cls, item):
        item = str(item).strip()
        m = re.search(r'^([0-9]{3})-?([0-9]{2})-?([0-9]{4})$', item)
        if m:
            return m.group(1) + '-' + m.group(2) + '-' + m.group(3)
        else:
            return item
plocket commented 1 year ago

Ok, now that I've dug into the jQuery validation plugin for the 5th time, I think I've got this. We're going to do our own jQuery validation using the plugin. This is a minimal example of a .py file with custom jQuery validation (see caveats below):

from docassemble.base.util import CustomDataType, DAValidationError, word
import re

# The parts that need to match:
# 1. `name` and the `datatype` used in the question
# 2. `input_class` and the selector used in the for loop
# 3. The `rules` key, the `rules.messages` key, and the first argument to `.addMethod()`

class AAA(CustomDataType):
    name = 'altestvalidation'
    input_class = 'da-ccc'
    javascript = """\
// da's try/catch doesn't log the error
try {
  $('#daform').validate({});

  $('.da-ccc').each(function() {
    $(this).rules('add', {
      ddd: true,
      messages: {
        ddd: "A SSN needs to be in the form xxx-xx-xxxx",
      },
    })
  });  // ends for every .da-ccc

  $.validator.addMethod('ddd', function(value, element, params){
    // Whatever code we want
    return value == '' || /^[0-9]{3}-?[0-9]{2}-?[0-9]{4}$/.test(value);
  });
} catch (error) {
  console.error( error );
}
"""

The letters are there to show what names need to match with what other names.

The interview .yml file:

---
question: A custom data type
fields:
  - Test: test_cdt
    datatype: altestvalidation
---

If we just want to do minlength and other pretty simple stuff like that, I think it can be even more simple.

~Caveats: Because we're modifying elements, this code may have to be done within a whole other js function that manipulates the elements. That or we'll need to create some global variables.~

Just fyi, we need the rules depends property to for our classes dynamically. https://jqueryvalidation.org/validate/. I doesn't need to be in .validate, it can be in .rules('add', function () {}).

plocket commented 1 year ago

This is where I'm working on this for now: https://github.com/plocket/docassemble-CDTCustomValidation/blob/main/docassemble/CDTCustomValidation/ALCustomDateTestValidation.py

plocket commented 1 year ago

Validation errors: The error message is, I think, probably fine - I'm having one message show up under all the fields containing the most recent error. Also, though, each field currently gets a red exclamation mark icon. Also, it only only when that field is focused and then blurred. For example, if the date is less than the minimal date, the exclamation point will show up in the field I just blurred, but there's no exclamation mark in the other two fields. If I click on another field and then click out, the exclamation mark will appear in that one now too. And so on. Same type of behavior when the date becomes valid. It's obviously not the behavior we want, but how do we want to handle this?

  1. One exclamation mark for all of them? Where should it go?
  2. Each have an individual exclamation mark, but make sure they all appear or disappear together?
  3. What about when one field in particular is wrong, like when the day value is above 31? Should just that field have an exclamation point? Where should the error show up? Just under that field (which looked terrible in the past)? Or in the now "regular" spot that's under all the fields? Making special behavior per field is more brittle, but we can give it a shot.
plocket commented 1 year ago

@mnewsted - would love your thoughts on date validation behavior.

plocket commented 1 year ago

[environment: Interview has 1 3-part date field and 1 birthdate field. (and 2 normal date fields)]

Note to ignore for now: I had a bug I can't replicate, so maybe I got rid of it. Putting it here for posterity. The input was invalid, the message was showing in one place under all three elements, but then another message appeared just under the element itself that I had just blurred from - that there was no message set for that invalid input. It subsequently appeared under each of the others as I interacted with them.

[Another one that's inconsistent: 3-part date sometimes gets default 'birthdate needs to be in the past' message. Race condition about which one gets created first?]

plocket commented 1 year ago

Feedback notes here:

plocket commented 1 year ago

Got pointed at UK gov's decisions on how to show errors for 3-part dates! Says it needs more user research [regarding "the extent to which users struggle to enter months as numbers, and whether allowing them to enter months as text is helpful"], but it seems to me to make some sense at the very least: https://design-system.service.gov.uk/components/date-input/#error-messages. Maybe a good resource for other inputs too!

nonprofittechy commented 1 year ago

Yes, the gov.uk guidance is really helpful. Adding 1 dozen validation error messages that are each custom per screen sounds very difficult, but using it when we can easily makes sense.

plocket commented 1 year ago

Do we want to require 4 digits for the year? Even if it's just something to put in a future issue.

nonprofittechy commented 1 year ago

Sure. There's no good reason to use this date input for a non-4 digit year

plocket commented 1 year ago

Are you expecting custom messages for min and max date type stuff, or just one message for every occasion, @mnewsted?

plocket commented 1 year ago

Give this interview a shot and see if it fits the MVP. [Would also appreciate feedback on names/attribute names if possible. The customdatatype has a ridiculous name for now, of course. See full module code in practice repo. ]

Also, custom messages turn out to be trivial. They can come along with the validation easily. Just the other parts are nuts.

plocket commented 1 year ago

Another question while folks are here: One type of invalid input is when a date crosses boundaries. For example 1/32/2000 would become, in js, 2/1/2000. We can see how that can be done by putting in the wrong day value. It's impossible for the user to put in the wrong month value. Is there any value other than the day value where a user could make a mistake that would cause the date to cross a date boundary in this way? [Changing 1/30 to 2/30 doesn't count because the day is still the value that's crossing the boundary.]

mnewsted commented 1 year ago

Thanks @plocket ! This is looking good. I'm happy that passing the validation message was trivial. A few things. 1) Is it possible to have the leap year/February date validations work the same way that other non-existent dates do? In the below screenshot, I get the helpful message that Sep 2000 doesn't have 39 days, and I was expecting to get the same treatment for February. Maybe it would always say February YYYY doesn't have 30/31 days, and on non-leap-years, it would say Feb YYYY doesn't have 29 days. 2) Maybe this is just the font we are using, but is it possible to further widen the Month text field so the entire month fits in the drop down? Day can be narrowed considerably (never > 31) and year can also be limited to 4 numbers.

In response to your date boundary question, I'm wondering at which point in time/action the validation occurs. Are there multiple? Would it make sense to hard-limit day to 31, but then do a full check of whether the month/day/year combo is valid after all 3 are entered?

image

plocket commented 1 year ago

Thanks for the feedback @mnewsted! You have some great points! I was a bit unclear about what was going on in the demo, so I think that led to some confusion.

I was expecting to get the same treatment for February

The "Three parts" date field there is actually showing a custom validation message, not our own default one. I wanted to make sure those were getting through. When I've tested the lower "Birthday" one, it does work for February. I've changed the labels in the interview now to hopefully make it a bit more clear.

is it possible to further widen the Month text field so the entire month fits in the drop down?

Totally! I saw that issue (#143) and it'll be trivial to add it to this PR so I'll go ahead and do that.

In response to your date boundary question, I'm wondering at which point in time/action the validation occurs. Are there multiple? Would it make sense to hard-limit day to 31, but then do a full check of whether the month/day/year combo is valid after all 3 are entered?

That's exactly what I went with and I'm glad if it aligns with what other people are thinking. That is, 31 is always invalid. Anything at or below 31 stays valid until the full date is put in and then it gets evaluated again 👍

plocket commented 1 year ago

Notes from deep dive:

[conclusion: no current way to access attributes, rules, and messages. Currently da doesn't add them to the DOM] Additional note: Now that we know how to access settings, can we find a way to access attributes, rules, and messages?

Just so it's here, draft work as of 2023/03/05 is in this codepen.

plocket commented 1 year ago

These are the validation requirements I've collected from our discussions

MVP validation rules

The below is not part of the test and not accessibility friendly.

Note: if a required field with default value has its default value erased, it stays valid :P jQuery validation plugin doesn't seem to deal with that right now.

Possible post MVP improvements

Other notes

plocket commented 1 year ago

Bugs in their own comment

Bugs

Submission bugs I've noticed that I haven't been able to affect

These notes mention add_to_group(). That's a function of ours.

(TODO: Watch deep dive vid for 2023/03/13 for lots of replication. Start at beginning of vid.)

Bad things happen when a user submits invalid fields (upstream?). Note that no other invalidation highlighting appears. If any, it's only our new stuff:

More important:

Less important: