Skip to content

Add manual config flow for Plex#34476

Merged
balloob merged 7 commits intohome-assistant:devfrom
jjlawren:re-add_plex_manual_config
May 5, 2020
Merged

Add manual config flow for Plex#34476
balloob merged 7 commits intohome-assistant:devfrom
jjlawren:re-add_plex_manual_config

Conversation

@jjlawren
Copy link
Copy Markdown
Contributor

@jjlawren jjlawren commented Apr 20, 2020

Proposed change

In preparation for a potential removal of YAML configuration, this will bring functional parity to the config flow.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • 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.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@jjlawren
Copy link
Copy Markdown
Contributor Author

I'm adding in the functionality to configure with only a token to match the YAML feature.

@jjlawren jjlawren marked this pull request as ready for review April 21, 2020 17:11
@jjlawren jjlawren mentioned this pull request Apr 22, 2020
20 tasks
@jjlawren jjlawren marked this pull request as draft April 25, 2020 13:09
@jjlawren
Copy link
Copy Markdown
Contributor Author

Waiting for home-assistant/frontend#5612 as I'd like to use it here.

@jjlawren jjlawren force-pushed the re-add_plex_manual_config branch from 28f8a7d to 9486d17 Compare April 26, 2020 04:22
@jjlawren jjlawren marked this pull request as ready for review April 26, 2020 04:28
@jjlawren jjlawren force-pushed the re-add_plex_manual_config branch from 9486d17 to a6c4662 Compare May 3, 2020 19:49
"step": {
"user": {
"title": "Plex Media Server",
"description": "Continue to <a href=\"https://plex.tv\" target=\"_blank\">plex.tv</a> to link a Plex server.",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This description seems weird. Can we make it so that manual_setup is not a checkbox but that we offer 2 options:

  • Configure by logging in to Plex.tv
  • Manually enter server details

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we use vol.In. We can look at ha-form to see if we can render radio buttons if < 5 options or so.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The manual setup option will only be visible to those with advanced mode enabled. Most users will only see the description and a button to continue.

I'm trying to suggest in the language that proceeding is the recommended option and that a manual config is not.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, the description is shared between normal and advanced mode :( We could change that by using different steps ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be doable.

@jjlawren jjlawren force-pushed the re-add_plex_manual_config branch from a6c4662 to 6facde0 Compare May 5, 2020 21:52
data_schema = vol.Schema(
{
vol.Required("setup_method", default=AUTOMATIC_SETUP_STRING): vol.In(
[AUTOMATIC_SETUP_STRING, MANUAL_SETUP_STRING]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh idea. We could call async_get_translations from helpers/translation.py

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I guess we don't have the used language, never mind.

Copy link
Copy Markdown
Member

@balloob balloob May 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need a better solution for translating a vol.In (in a future PR)

@balloob balloob merged commit 740d9c5 into home-assistant:dev May 5, 2020
@jjlawren jjlawren deleted the re-add_plex_manual_config branch May 6, 2020 14:44
@lock lock Bot locked and limited conversation to collaborators May 21, 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.

3 participants