sarahhenkens / home-assistant-iocare

27 stars 15 forks source link

Config flow field labels are missing #15

Open emcniece opened 3 years ago

emcniece commented 3 years ago

When adding the integration via HA config flow there is no label or description on the username and password fields. I recently fixed this in my bchydro repo - you just have to add some translation strings. I'll post a fix in the next day or so!

RobertD502 commented 3 years ago

Good catch!

RobertD502 commented 3 years ago

@emcniece Have you been able to get this fixed? I just tried and followed the documentation, but it doesn't seem to be working.

The config_flow.py file contains:

"""Config flow of our component"""

import voluptuous as vol
from homeassistant import config_entries
from homeassistant.const import (
    CONF_PASSWORD,
    CONF_USERNAME
)
from .const import DOMAIN

class IoCareConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
    """Handle our config flow."""

    VERSION = 1
    CONNECTION_CLASS = config_entries.CONN_CLASS_CLOUD_POLL

    async def async_step_user(self, user_input=None):
        """Handle a flow start."""

        if user_input is not None:
            return self.async_create_entry(
                title="Coway IoCare Config Entry",
                data=user_input,
            )

        return self.async_show_form(
            step_id="user", 
            data_schema=vol.Schema(
                {
                    vol.Required("CONF_USERNAME"): str,
                    vol.Required("CONF_PASSWORD"): str,
                }
            )
        )

The strings.json file contains:

{
  "config": {
    "step": {
      "user": {
        "data": {
          "username": "Coway Username",
          "password": "Coway Password"
        },
        "title": "Fill in your Coway IoCare credentials",
        "description": "See <documentation> for more information"
      }
    },
    "error": {
      "option_error": "IoCare validation failed. Are your credentials correct?"
    },
    "abort": {
      "already_configured": "IoCare is already configured"
    }
  }
}

The .translations folder contains an en.json file that has the same content as the strings.json file. Of note, I have also tried setting the "username" and "password" fields to "[%key:common::config_flow::data::username%]" and "[%key:common::config_flow::data::password%]" respectively in the strings.json file, but no luck. Not sure what is going on as the current format seems to be following HA dev documentation.

Not sure if perhaps init needs to be defined after CONNECTION_CLASS?

emcniece commented 3 years ago

Apologies for not following up sooner. I haven't had a chance to test this out yet. As mentioned I did fix this in another repo recently, the changes aren't merged into master but here's the comparison that makes the labels work: https://github.com/emcniece/hass-bchydro/compare/feat/updatecoordinator

Maybe we can look at this comparison and see if there's anything missing from the iocare repo. I find the official documentation to be lacking in some crucial details at times.

RobertD502 commented 3 years ago

Thanks for the quick response! Besides the form fields, I noticed that the IOCare integration also doesn't check for any errors. It flew under the radar when I started using this integration since I entered my credentials in correctly. As it stands right now, you can enter anything into the fields and it will be accepted/an entry will be made, but obviously fails to work. The config flow should probably be rewritten to include this for the sake of anyone new. I'll see what I can make of the differences between your config flow and the one on this integration.

RobertD502 commented 3 years ago

@emcniece Ended up figuring out the problem. The en.json file indentation needs to be different from the strings.json file- the en.json file "config" needs to start 2 tab indentations in. I didn't realize this as the current file isn't like that and all the documentation I've seen said the en.json file is just a copy and rename of the strings.json file. I also changed the folder from ".translations" to "translations" - what the difference in naming does I couldn't figure out. All I was told on the dev discord channel was that the name of the directory was changed. Perhaps you have some insight into this?

Also, thank you for providing the changes in your code. They helped me a lot when it came to rewriting the config_flow. As of now the python-iocare API doesn't have exceptions written for the login phase. I'll have to look into what changes I can make in the API to get correct exceptions, but with the way I've rewritten the config_flow, new users will not be able to add the integration with a wrong username and password- this wasn't the case before. I'll be adding the changes soon to the current pull request I have open.

emcniece commented 3 years ago

Excellent work! The only insight I can provide is that HA development moves very fast and things get changed quickly and without notice or documentation. It's up to us devs to try and figure out why and how things don't work, as often the official documentation is outdated or sparse. To figure this particular problem out I looked through a dozen official components to see how they did it, then picked out the bits that were relevant. Not an easy process as there seems to be several implementation patterns at work across those components, likely not updated because of tech debt and breakneck development speed.

Anyway, glad I could offer some help. Thanks for your efforts!

RobertD502 commented 3 years ago

I probably should have stuck to looking at official components. I looked at several custom components and their implementation patterns were all over the place. The changes + more are now in my fork of this repo and added to the current pull request, but I'm not sure when they will be reviewed. If you want the newly added eco mode, use of the updated fan entity model (If you're tired of seeing errors in your log), rounded AQI, and use of the new Services UI feel free to use my fork until the pull request is approved!

I also added the icon and logo for IOCare in the homeassistant brands repo, so we should be seeing those once the pull request is reviewed (Yay! no more blank icons on the integrations page).