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

Display of client- vs. server-side form errors is inconsistent #8058

Closed
sol1-matt opened this issue Dec 14, 2021 · 10 comments
Closed

Display of client- vs. server-side form errors is inconsistent #8058

sol1-matt opened this issue Dec 14, 2021 · 10 comments
Assignees
Labels
status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application

Comments

@sol1-matt
Copy link

sol1-matt commented Dec 14, 2021

NetBox version

v3.1.0

Python version

3.7

Steps to Reproduce

  1. Create custom validators for device and virtual machine
  2. Do something that will trigger the validator

Expected Behavior

  1. A pop up dialogue appears
  2. At the top of the Form a Errors section appear
  3. OK fields have green ticks and/or a green border
  4. The field in error should have a red cross and/or red border

Observed Behavior

For Device
3. A pop up dialogue appears
4. At the top of the form a Errors section appear
5. Some fields have green ticks and a green border but they disappear
6. The field in error doesn't have a green tick but has a green border

For VM
3. A pop up dialogue appears
4. Some fields have green ticks and a green border but they disappear
5. The field in error doesn't have a green tick but has a green border

@sol1-matt sol1-matt added the type: bug A confirmed report of unexpected behavior in the application label Dec 14, 2021
@sol1-matt
Copy link
Author

This is a minor UI issue

@jeremystretch
Copy link
Member

  1. Create custom validators for device and virtual machine
  2. Do something that will trigger the validator

Please specify exactly what you're doing so that someone else might attempt to replicate the behavior.

@jeremystretch jeremystretch added the status: revisions needed This issue requires additional information to be actionable label Dec 14, 2021
@sol1-matt
Copy link
Author

sol1-matt commented Dec 14, 2021

Test setup 1

  1. Add validators to configuration.py and restart netbox
CUSTOM_VALIDATORS  = {
    "dcim.device": [
        {
            "name": {
                "min_length": 6,
                "regex": "^[a-z0-9_\-. ]*$"
            }
        }
    ],
    "virtualization.virtualmachine": [
        {
            "name": {
                "min_length": 6,
                "regex": "^[a-z0-9_\-. ]*$"
            }
        }
    ]
}

2a. Add device with missing default required field, eg site, and save.
2b. Add device with name test? and all required fields and save
3. Add virtual machine with name test? and all required fields and save

2a result is similar to this

  • good fields have green border
  • bad fields have red border
    image

2b result is similar to this

  • borders appear initially but disappear
  • Error box above form with error
  • Error pop up in bottom right
    image

3 result is similar to this

  • borders appear initially but disappear
  • No Error box above form
  • Error pop up in bottom right
    image

Note the actual validators I'm using from the screen shots is different to the test case shown, I did get the same results with this validator though, while the text of the error differs the UI layout from them is the same.

The validator used for the screen shots is a Custom Validator python script which is substantially more complex, both the config validator python script and config based customer validator behave the same.

@sol1-matt
Copy link
Author

I also noticed green ticks but they only appear to be on text fields, I'd assume are required text fields in error have a red X or something similar. The ticks disappear at the same time the borders disappear.
example of ticks
image

@sol1-matt
Copy link
Author

sol1-matt commented Dec 14, 2021

Test setup 2
CustomValidator class in netbox/custom_validators.py.
This is something I'm experimenting with and requires further testing.

For UI purposes the results using this customer validator class are exactly the same for device and virtual machine. Site and tenant which wasn't tested using the config based validators behave the same as virtual machine.

  • Borders appear initially but disappear
  • No Error box above form
  • Error pop up in bottom right
from extras.validators import CustomValidator
from dcim.models import Device, Site
from virtualization.models import VirtualMachine
from tenancy.models import Tenant
import re

class MyValidatorHelper():

    @staticmethod
    def automationSlugify(s):
        slug = s.lower()
        # Things to replace with nothing
        for c in ["+", "*", "'", '"']:
            slug = slug.replace(c, "")
        # Everything else
        slug = re.sub("[^0-9a-zA-Z_]+", '_', slug)
        # Replace repeating underscores
        slug = re.sub("__+", '_', slug)
        return slug

    @staticmethod
    def monitoringSlugify(s):
        slug = s
        # A-z, 0-9, dot, underscore, dash, space
        slug = re.sub("[^0-9a-zA-Z_\-. ]+", '_', slug)
        # Replace repeating underscores
        slug = re.sub("__+", '_', slug)
        return slug

    @staticmethod
    def lengthValidator(length, val):
        fail_message = []
        if len(val) <= length:
            fail_message.append("must be {} or more characters long".format(length))
        else:
            if len(MyValidatorHelper.automationSlugify(val)) <= length:
                fail_message.append("group for automation ({}) must be {} or more characters long".format(MyValidatorHelper.automationSlugify(val), length))
            if len(MyValidatorHelper.monitoringSlugify(val)) <= length:
                fail_message.append("name for monitoring ({}) must be {} or more characters long".format(MyValidatorHelper.automationSlugify(val), length))
        return fail_message

    @staticmethod
    def duplicateValidator(needle, haystack):
        fail_message = []
        for hay in haystack:
            if needle.id != hay.id:
                if needle.name.lower() == hay.name.lower():
                    fail_message.append("duplicate name detected '{}'".format(hay.name))
                if MyValidatorHelper.automationSlugify(needle.name.lower()) == MyValidatorHelper.automationSlugify(hay.name.lower()):
                    fail_message.append("duplicate automation name detected '{}'".format(MyValidatorHelper.automationSlugify(hay.name)))
                if MyValidatorHelper.monitoringSlugify(needle.name.lower()) == MyValidatorHelper.monitoringSlugify(hay.name.lower()):
                    fail_message.append("duplicate monitoring name detected '{}'".format(MyValidatorHelper.monitoringSlugify(hay.name)))
        return fail_message

# This is to validate anything that isn't a piece of plastic, aka stuff that may interact with automation and monitoring
class DeviceValidator(CustomValidator):

    def validate(self, instance):
        fail_message = []
        # Exclude bits of plastic we want to document but don't interact with
        if instance.device_role not in ['Patch panel']:
            # Name format
            if not re.match('^[a-zA-Z0-9_\-. ]*$', instance.name):
                fail_message.append("must match regex '^[a-zA-Z0-9_\-. ]*$'")
            # Duplicates
            fail_message += MyValidatorHelper.duplicateValidator(instance, Device.objects.all())
        # Length
        fail_message += MyValidatorHelper.lengthValidator(6, instance.name)
        if fail_message:
           self.fail("'name' field {}".format(" and ".join(fail_message)), field='name')

class VirtualMachineValidator(CustomValidator):

    def validate(self, instance):
        fail_message = []
        # Name format
        if not re.match('^[a-zA-Z0-9_\-. ]*$', instance.name):
            fail_message.append("must match regex '^[a-zA-Z0-9_\-. ]*$'")
        # Duplicates
        fail_message += MyValidatorHelper.duplicateValidator(instance, VirtualMachine.objects.all())
        # Length
        fail_message += MyValidatorHelper.lengthValidator(6, instance.name)
        if fail_message:
           self.fail("'name' field {}".format(" and ".join(fail_message)), field='name')

class SiteValidator(CustomValidator):

    def validate(self, instance):
        fail_message = []
        # Name format
        if not re.match('^[a-zA-Z0-9_\-. ]*$', instance.name):
            fail_message.append("must match regex '^[a-zA-Z0-9_\-. ]*$'")
        # Duplicates
        fail_message += MyValidatorHelper.duplicateValidator(instance, Site.objects.all())
        # Length
        fail_message += MyValidatorHelper.lengthValidator(6, instance.name)
        if fail_message:
           self.fail("'name' field {}".format(" and ".join(fail_message)), field='name')

class TenantValidator(CustomValidator):

    def validate(self, instance):
        fail_message = []
        # Name format
        if not re.match('^[a-zA-Z0-9_\-. ]*$', instance.name):
            fail_message.append("must match regex '^[a-zA-Z0-9_\-. ]*$'")
        # Duplicates
        fail_message += MyValidatorHelper.duplicateValidator(instance, Tenant.objects.all())
        # Length
        fail_message += MyValidatorHelper.lengthValidator(6, instance.name)
        if fail_message:
           self.fail("'name' field {}".format(" and ".join(fail_message)), field='name')

configuration.py

CUSTOM_VALIDATORS  = {
    "dcim.device": (
        'custom_validators.DeviceValidator',
    ),
    "virtualization.virtualmachine": (
        'custom_validators.VirtualMachineValidator',
    ),
    "dcim.site": (
        'custom_validators.SiteValidator',
    ),
    "tenancy.tenant": (
        'custom_validators.TenantValidator',
    )
}

@jeremystretch jeremystretch removed the status: revisions needed This issue requires additional information to be actionable label Dec 15, 2021
@DanSheps
Copy link
Member

DanSheps commented Dec 15, 2021

So, the green boxes come from the form validation that happens on submit. There currently is no way to maintain them during submit when the custom validation happens.

The error could be clarified. Instead of Enter a valid value. we could maybe clean that up with a Data entered into field failed custom validation

However, one option might be to instead inject some of the Custom validation into the form if there is a regex, required, min, max, min_length, max_length, required or prohibited set. This possibly falls down when you have more then 1 validator running for a specific model with the same fields set (regex, required, etc) or with a custom validation class.

@sol1-matt
Copy link
Author

sol1-matt commented Dec 16, 2021

So, the green boxes come from the form validation that happens on submit. There currently is no way to maintain them during submit when the custom validation happens.

From reading this and looking at the behavior the custom validator run after form validation, ie they only trigger if the form doesn't have default problems so this make sense.

With this bug if there is no easy way to add the green/red field boarders then I'd suggest aiming for all custom validation to behave as device customer validation errors do

  • Pop up
  • Error text area above the form

Other Netbox objects I've tested (site, tenant and virtual machine) only have the pop up, the text area is absent.

This is a very minor issue, is just stood out to me.

@DanSheps
Copy link
Member

DanSheps commented Dec 17, 2021

I wonder if we could do a post using htmx now. That might allow us to retain the errors and validation.

@jeremystretch
Copy link
Member

The red & green field borders are added on the client side: The missing field is highlighted because the browser knows that it's a required field but empty. The validation logic is applied on the server side, and when it fails, an entire new form is rendered with the appropriate messages. (Incidentally, I have an open item under #7449 to remove the green border from valid fields as it can be distracting.)

I think the root issue here is that server-side form errors are not being displayed inline with the form fields as the client-side errors are. This isn't specifically related to custom validators; the same seems to be true for any server-side validation errors. We probably just need to tweak the form rendering templates to apply field-specific errors more consistently.

@jeremystretch jeremystretch added the status: under review Further discussion is needed to determine this issue's scope and/or implementation label Dec 22, 2021
@jeremystretch jeremystretch changed the title Custom Validator UI is inconsistent Display of client- vs. server-side form errors is inconsistent Dec 22, 2021
@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation and removed status: under review Further discussion is needed to determine this issue's scope and/or implementation labels Jan 13, 2022
@jeremystretch jeremystretch self-assigned this Jan 13, 2022
@jeremystretch
Copy link
Member

I've modified how form field errors are rendered under f56e3eb for NetBox v3.4. Error messages are now embedded inline beneath each field rather than being displayed as "toasts" in the bottom right corner. I believe this satisfies at least the bulk of the concerns raised in this issue.

I'm open to further discussion of ways to improve form rendering in general, however any further work in this area should be captured as separate feature requests.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application
Projects
None yet
Development

No branches or pull requests

3 participants