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

Device Config form can be saved with required fields missing #2209

Closed
justjam2013 opened this issue Oct 21, 2024 · 4 comments
Closed

Device Config form can be saved with required fields missing #2209

justjam2013 opened this issue Oct 21, 2024 · 4 comments
Labels

Comments

@justjam2013
Copy link
Contributor

justjam2013 commented Oct 21, 2024

Describe The Bug

I have the following defined in my config schema, where both the name and hasCellular fields are required :

    ....
    "name": {
        "title": "My iPad",
        "type": "string",
        "required": true
    },
    "hasCellular": {
        "title": "Has Cellular Capabilities",
        "type": "boolean",
        "required": true,
        "condition": {
            "functionBody": "return ['switch'].includes(model.devices[arrayIndices].accessoryType);"
        }
    },
    ...

In Config UI, I go ahead and configure in the Plugin Config window my iPad. Without entering a name for my iPad, I can click the Save button, which results in a valid json structure-wise, but not a valid iPad device, as I expect the field name to be present in the config.
I can check in the code to see if the name is present or not, then either fail to allow Homebridge to startup, or fail to load the malformed device and display an error in the log.

  1. I fail to allow Homebridge to startup. My plugin is not being a "good citizen" and I is holding the entire Homebridge hostage until the plugin json is fixed or deleted.
  2. I fail to load the plugin and display an error message in the log. The log scrolls by very quickly in the Homebridge Logs widget on the Status screen, so the user may not realize that there was an error until they try to use the device and it doesn't show up.

While option 2 works and I can check in code if the configuration is valid, it is not a good user experience. It would be reasonable for the user to expect that config ui would not save an invalid configuration.

My expectation is that

  1. Required && visible fields would be actually required, not just marked as required
  2. The Save button would be disabled until all required && visible fields are populated

Note: if a field is required, but not visible, it wouldn't affect the status of the Save button and wouldn't be saved in the configuration json

Note:
If I go to the Status screen right after I press the Save button, then I see the logs scroll by, albeit very quickly, so I might still miss an error message displayed. But iIf I wait for the spinner to stop after I press the Savebutton before I go to theStatusscreen, this is what myHomebridge Logs` widget looks like. It looks like Homebridge started up fine and everything is good.
Screenshot 2024-10-21 at 1 54 39 PM

Logs

No response

Config

No response

Homebridge UI Version

4.62.0

Homebridge Version

1.8.4

Node.js Version

22.9.0

Operating System

macOS

Environment Info

Using hb-service

Raspberry Pi Model

None

@bwp91
Copy link
Contributor

bwp91 commented Oct 23, 2024

Hi @justjam2013 please install the beta version of config-ui-x where I have added a validation check for the config schema form.

The UI (in the beta version) uses ng-formworks for displaying the forms.

You might be able to create some custom validation messages from the docs here:

although it is not something that I have really explored myself.

Note that ideally plugins should still perform some checks on the user's config to avoid any type errors and plugin restart loops which may bring homebridge into a restart loop too.

Let me know how it goes with any custom validation you try out!

@justjam2013
Copy link
Contributor Author

justjam2013 commented Oct 23, 2024

Hi @bwp91, I have updated to the beta version of config-ui-x and it works like a peach!

When the config form starts up I see a red ! symbol at the bottom of the popup and she `Save button is grayed out:
Screenshot 2024-10-23 at 5 04 00 PM

When all required fields have values, the red ! turns into a green check mark and the Save button is enabled:
Screenshot 2024-10-23 at 5 04 12 PM

I fiddled around with making different fields required and non-required.

The only debatable issue I found is that if a checkbox is required, then the form is blocked. To unblock form submission, I had to select and unselect the checkbox.
So I added

  "default": false,

to the schema for the checkbox and that removes the need to select-select.

In my opinion this is not a bug, but correct behavior. The schema specifically indicates that it is a required field, so the value must be explicit. undefined does not meet the requirement of required. The value can either be set in the schema by using default, or it can be left to the user will have to check/uncheck the checkbox. The latter though is bad user experience.

So tried out the following validations:

  "minLength": 5,
  "pattern": "^[A-Fa-f0-9]$",
  "minLength": 5,
  "pattern": "^[A-Fa-f0-9]$",
  "pattern": "^[A-Fa-f0-9]{5,}$",

In all cases, the validations prevented the green check mark and the Save button from being enabled.

I would say that it fully works as expected. Thank you for making the changes.

Update: I also verified that if a field is required, but not visible, it does not affect the status of the Save button.

@justjam2013
Copy link
Contributor Author

justjam2013 commented Oct 23, 2024

Note that ideally plugins should still perform some checks on the user's config to avoid any type errors and plugin restart loops which may bring homebridge into a restart loop too.

I fully agree that plugins should perform checks. However, the plugin the plugin should be able to reasonably expect that the UI would honor the schema constraints, like type, required, pattern, etc. Especially given that this task is fully performed by config-ui and the plugin does not have a chance to validate the values before they are stored.

In fact, it would be nice if config-ui called a validate(pluginConfig) method in the plugin. ATM the only way to create a stateful uuid for a virtual device is to create a field on the form for the user to fill in. And a user could enter the same uuid for two devices
If the plugin got a chance to validate the data, then it could catch the problem and fix it. Or even better, the plugin would assign uuids to newly created virtual devices and the UI field would be read-only.

Update:

it would be nice if config-ui called a validate(pluginConfig) method in the plugin
Opened a new ticket here as an suggested enhancement.

@justjam2013
Copy link
Contributor Author

Closing this bug as this now works as expected in the latest beta versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants