Skip to content

Allow passing the current value to config flow input fields#5603

Merged
bramkragten merged 5 commits intohome-assistant:devfrom
OnFreund:current_values_in_config_flow
May 1, 2020
Merged

Allow passing the current value to config flow input fields#5603
bramkragten merged 5 commits intohome-assistant:devfrom
OnFreund:current_values_in_config_flow

Conversation

@OnFreund
Copy link
Copy Markdown
Contributor

@OnFreund OnFreund commented Apr 23, 2020

Proposed change

This PR creates a mechanism for the backend to pass the current value of config flow fields. The existing way of doing this is to set the default parameter in the schema, but that has the adverse effect of not being able to remove an optional value, since it will be populated with the default if left empty.
After this PR is merged, the backend can pass the current value without setting a default. An example:

vol.Schema({
  vol.Optional("port", description={"suggested_value": 1234}): int
})

If the current value is not present, the default will still be used, so existing integrations would not be broken as a result of this change.

You can also mix and match the current value and the default - the dialog will show the current value, but if left empty, the default will be used.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Comment thread src/components/ha-form/ha-form.ts Outdated
Comment thread src/dialogs/config-flow/step-flow-form.ts Outdated
@bramkragten
Copy link
Copy Markdown
Member

This should also be added to all the ha-form-* elements.

Co-Authored-By: Bram Kragten <mail@bramkragten.nl>
@OnFreund
Copy link
Copy Markdown
Contributor Author

Thanks @bramkragten.

This should also be added to all the ha-form-* elements.

Not sure what you mean. The ha-form-* elements see the current value already pre-filled, so I'm not sure what change is required if at all.

@OnFreund OnFreund requested a review from bramkragten April 30, 2020 07:32
@bramkragten
Copy link
Copy Markdown
Member

I have to check how this all works, but you now do it in only one place. The device automations might also need this? And at least ha-form-integer now uses default so should be updated?

@bramkragten
Copy link
Copy Markdown
Member

Looks like device automations now don't display the default at all, only the set value if it has it. I personally would like to remove the logic of setting the data based on default/current_value in step-flow-form and move it to ha-form.

But that might be out of scope for this PR.

@OnFreund
Copy link
Copy Markdown
Contributor Author

OnFreund commented Apr 30, 2020

  1. I renamed current_value to suggested_value - I think it reflects more uses cases (e.g. the one in Wake on LAN for UI-configured Samsung TVs core#33621.

  2. Device Automations - I don't have much experience there, so I don't really know, but I believe that enabling this for config and options flows is already great value on its own. As for moving it into ha-form, it sounds right, but TBH I don't believe I understand this code well enough to pull this off. I agree with you it could be out of scope here.

  3. I'm not sure why ha-form-integer has its own handling of default, but it only kicks in when there's no data, which will not happen if there's a default or suggested_value in a config/options flow. I assumed that this might be used by other clients (e.g. device automations that you brought up), so I preferred not to touch it and break something else.

@bramkragten
Copy link
Copy Markdown
Member

It is because ha-form-integer already supports optional based on the optional flag

@bramkragten
Copy link
Copy Markdown
Member

This does not depend on a backend change right?

@OnFreund
Copy link
Copy Markdown
Contributor Author

OnFreund commented May 1, 2020

No backend change required - this is completely backwards compatible.
However, integrations that currently use default for optional values, can migrate to suggested_value if it makes sense for them.

@bramkragten bramkragten merged commit 1687d90 into home-assistant:dev May 1, 2020
@OnFreund OnFreund deleted the current_values_in_config_flow branch May 1, 2020 08:45
@OnFreund
Copy link
Copy Markdown
Contributor Author

OnFreund commented May 1, 2020

@bramkragten I think there might be a bug here that I missed because my backend was covering up for it - if the user changes nothing in the form and submits, the suggested value isn't used. My best suggestion on how to fix this is to end _stepDataProcessed() with:

this._stepData = data;
return data;

But I'm assuming there's a reason it's not there? Maybe we can set it only in case a suggested value was picked up?

@OnFreund
Copy link
Copy Markdown
Contributor Author

OnFreund commented May 1, 2020

Something like:

private get _stepDataProcessed() {
    if (this._stepData !== undefined) {
      return this._stepData;
    }

    const data = {};
    var suggestion_used = false;
    this.step.data_schema.forEach((field) => {
      if (field.description?.suggested_value) {
        data[field.name] = field.description.suggested_value;
        suggestion_used = true;
      } else if ("default" in field) {
        data[field.name] = field.default;
      }
    });

    if (suggestion_used) {
      this._stepData = data;
    }
    return data;
  }

@bramkragten
Copy link
Copy Markdown
Member

I think we can just set it, it only means the frontend is doing the backends work of setting defaults.

@OnFreund OnFreund mentioned this pull request May 1, 2020
9 tasks
@OnFreund
Copy link
Copy Markdown
Contributor Author

OnFreund commented May 1, 2020

OK, new PR here: #5688

@bramkragten bramkragten mentioned this pull request May 5, 2020
@lock lock Bot locked and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allowing optional values in options flow to be removed

3 participants