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

Add todo component #100019

Merged
merged 35 commits into from Oct 23, 2023
Merged

Add todo component #100019

merged 35 commits into from Oct 23, 2023

Conversation

allenporter
Copy link
Contributor

@allenporter allenporter commented Sep 10, 2023

Proposed change

Add todo component. Updates the Shopping List component to implement the todo platform. This platform was also manually tested with a local todo implementation (removed as it is a larger scope PR, so local todo list based on rfc5545 will come in followups).

The to-do list platform registers the existing shopping list frontend panel (can be updated based on frontend PR direction)

An alternative can be to import shopping list into a local todo list component, though we can probably do that in followup PRs if desired.

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)
  • Deprecation (breaking change to happen in the future)
  • 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
  • I have followed the perfect PR recommendations
  • 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:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant
Copy link

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (demo) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of demo can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign demo Removes the current integration label and assignees on the pull request, add the integration domain after the command.

@allenporter allenporter marked this pull request as draft October 8, 2023 15:26
@allenporter
Copy link
Contributor Author

Marking as draft until we complete a round of user research to make sure the proposal has all the requirements. I think the APIs are still mostly correct here, but marking to indicate the actual status.

@allenporter allenporter force-pushed the todo-component branch 3 times, most recently from ffc4784 to 1fedcb8 Compare October 10, 2023 04:23
homeassistant/components/demo/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/shopping_list/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/shopping_list/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/shopping_list/__init__.py Outdated Show resolved Hide resolved
"""Add an item to the To-do list."""
raise NotImplementedError()

async def async_delete_todo_items(self, uids: set[str]) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Do we use a set parameter with multiple uids to follow the rfc spec or why do we use multiple instead of a single item for this action?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is purely for practical performance concerns, not related to the rfc spec.

When reviewing the shopping list API, one observation was that we currently require a "clear completed items" for the shopping list. This is also part of the new spec for todo when I looked at the wireframes shared by Bram that Madelena made. (This is mentioned in home-assistant/architecture#962 in passing under "Consequences" as a performance topic)

An alternative could be to rename this "clear completed items" rather than supporting delete of arbitrary items, however then i believe it requires more custom logic by the integration to implement. I liked simplifying this by pushing this to be a frontend concern about which items it is deleting (delete is needed anyway) allowing cloud integrations to have a more straight forward API.

Open to suggestions.

homeassistant/components/todo/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/todo/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/todo/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/todo/__init__.py Outdated Show resolved Hide resolved
tests/components/todo/test_init.py Show resolved Hide resolved
@allenporter allenporter marked this pull request as draft October 16, 2023 14:55
Copy link
Contributor Author

@allenporter allenporter left a comment

Choose a reason for hiding this comment

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

Made two API changes based on offline discussion for FE concerns:

  • The move service was reverted back to a websocket to be frontend focused (also lower priority for end user service)
  • The move service was simplified to take a position rather than a previous item. This pushes complexity into integrations that need to handle relative positions rather than in the frontend.

@allenporter allenporter marked this pull request as ready for review October 16, 2023 15:55
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Looks good!

Is codecov correct that some lines are not covered? Can we test those in that case?

@allenporter
Copy link
Contributor Author

Looks good!

Is codecov correct that some lines are not covered? Can we test those in that case?

Looks legit. I updated todo component with additional tests.

The shopping list case for unloaded is not tested, but the code is still there. One option is to just remove this code since shopping list does not yet support unload, though it feels weird to leave it incomplete.

@MartinHjelmare
Copy link
Member

I don't see the code regarding unload in the shopping list integration.

@allenporter
Copy link
Contributor Author

I don't see the code regarding unload in the shopping list integration.

There is code in shopping_list/todo.py:

    async def async_added_to_hass(self) -> None:
        """Entity has been added to hass."""
        self.async_on_remove(self._data.async_add_listener(self.async_write_ha_state))

And so the add listener unsub code in shopping list turned out to be not actually used given shopping list todo entities cannot be unloaded.

(I thought the code I just added added coverage in todo, but it didn't so i reverted it. the code coverage is similar to event now at least)

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

We should add back the unload tests for todo integration. Add a mock entity that should be removed or unavailable when the config entry is unloaded. That way we know that we've hooked up the todo integration to the entity component helper correctly.

Example:

async def test_lawn_mower_setup(hass: HomeAssistant) -> None:
"""Test setup and tear down of lawn mower platform and entity."""
async def async_setup_entry_init(
hass: HomeAssistant, config_entry: ConfigEntry
) -> bool:
"""Set up test config entry."""
await hass.config_entries.async_forward_entry_setup(
config_entry, Platform.LAWN_MOWER
)
return True
async def async_unload_entry_init(
hass: HomeAssistant, config_entry: ConfigEntry
) -> bool:
"""Unload up test config entry."""
await hass.config_entries.async_unload_platforms(
config_entry, [Platform.LAWN_MOWER]
)
return True
mock_platform(hass, f"{TEST_DOMAIN}.config_flow")
mock_integration(
hass,
MockModule(
TEST_DOMAIN,
async_setup_entry=async_setup_entry_init,
async_unload_entry=async_unload_entry_init,
),
)
entity1 = MockLawnMowerEntity()
entity1.entity_id = "lawn_mower.mock_lawn_mower"
async def async_setup_entry_platform(
hass: HomeAssistant,
config_entry: ConfigEntry,
async_add_entities: AddEntitiesCallback,
) -> None:
"""Set up test platform via config entry."""
async_add_entities([entity1])
mock_platform(
hass,
f"{TEST_DOMAIN}.{LAWN_MOWER_DOMAIN}",
MockPlatform(async_setup_entry=async_setup_entry_platform),
)
config_entry = MockConfigEntry(domain=TEST_DOMAIN)
config_entry.add_to_hass(hass)
assert await hass.config_entries.async_setup(config_entry.entry_id)
await hass.async_block_till_done()
assert config_entry.state == ConfigEntryState.LOADED
assert hass.states.get(entity1.entity_id)
assert await hass.config_entries.async_unload(config_entry.entry_id)
await hass.async_block_till_done()
assert config_entry.state == ConfigEntryState.NOT_LOADED
entity_state = hass.states.get(entity1.entity_id)
assert entity_state
assert entity_state.state == STATE_UNAVAILABLE

@home-assistant home-assistant bot marked this pull request as draft October 17, 2023 09:54
@allenporter
Copy link
Contributor Author

Thank you, I was missing the call to async_unload_platforms.

@allenporter allenporter marked this pull request as ready for review October 17, 2023 14:47
@MartinHjelmare
Copy link
Member

Can be merged when there's a linked and approved frontend PR.

@bramkragten bramkragten merged commit 5d430f5 into home-assistant:dev Oct 23, 2023
54 of 55 checks passed
@allenporter allenporter mentioned this pull request Oct 24, 2023
20 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Oct 24, 2023
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.

None yet

4 participants