#So the new aidot integration PR has an

1 messages · Page 1 of 1 (latest)

manic shoal
#

Currently they use the region + username

#

I pushed back on this because I saw a user id in the tests, so there was something better

#

now apparently there can only be a single session active at a time, so trying to login to get the ID would sign out the existing entry

magic rain
#

Would you mind telling us what the proposed unique ID is? 😄

floral lintel
magic rain
#

I thought that's what Joost rejected?

manic shoal
#
  • Would it be dirty to keep the resuilt of the second login and update the existing entry?
#

oh

#

just a user ID

#

like ID ID

#

123123

#
  • And now that I was typing this, maybe it'd make sense to do an initial check with _async_abort_entries_match
magic rain
#

apparently there can only be a single session active at a time, so trying to login to get the ID would sign out the existing entry
And what's the problem with that? Does it regenerate the ID?

manic shoal
#

Nope, it invalidates the existing tokens

#

So you'd setup the entry, everything works fine

#

you'd try to set it up again, it fails because it was setup

#

then the old entry stops working

#

(you'd need to reauth)

magic rain
#

what old integration?

#

The PR you're reviewing is a perfect example of when an empty PR description simply is not acceptable

#

The author should link to the existing integration, explain why the new one is better, link to the library etc.

manic shoal
#

I meant entry

#

there's no old integration

magic rain
#

ah, ok

jovial grail
#

What if the config flow updates the token of the existing entry before aborting?

magic rain
#

so what's the problem then? the problem is that trying to set up the same entry again will log out the existing one?

manic shoal
#

yes

manic shoal
#

like I consider updatign existing entries because you add a new one a bit meh

magic rain
#

what's the problem?

#

since they tried to login again, a new token was generated. hence we need to update the existing entry

magic rain
#

But it's off topic for the unique id / reauth issue of course

manic shoal
magic rain
#

I mean it as a general comment

#

Did you get the answer you wanted here btw?

manic shoal
#

I think we can check with _async_abort_entries_match first to do matching, and then try

#

If they don't match because the username changed, then I think a reauth makes sense as there's data to change I guess

magic rain
#

if that means they need to store some extra data in the entry only for that, I think it's a bad idea

#

I'd just keep it very simple, do the auth, check if there's a collision and if there is abort and update the existing entry

#

you're over thinking this

manic shoal
#

It'd only be the region they need more

magic rain
#

so, it's a no then

#

IMO

manic shoal
#

no wait huh

#

no their tests are busted

#

thanks

#

their reauth flow is fetching the country code from the entry data

#

so it's in there, it's just not in their mocks

# 
    async def async_step_reauth_confirm(
        self, user_input: dict[str, Any] | None = None
    ) -> ConfigFlowResult:
        """Handle reauth confirmation."""
        errors: dict[str, str] = {}
        if user_input is not None:
            login_info = self._get_reauth_entry().data[CONF_LOGIN_INFO]
            client = AidotClient(
                session=async_get_clientsession(self.hass),
                country_code=login_info.get(CONF_COUNTRY_CODE, DEFAULT_COUNTRY_CODE),
                username=user_input[CONF_USERNAME],
                password=user_input[CONF_PASSWORD],
            )
#

I am not sure why it uses a .get(), but in any case, even if the country code would not be in there, I think it'd still be a good practice to keep the country code in the entry data, if it doesn't change and we need it for reauth

#

Otherwise we'd need to ask again when you reauth