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

Support Bring! recently list #109854

Merged
merged 2 commits into from Feb 21, 2024
Merged

Conversation

miaucl
Copy link
Contributor

@miaucl miaucl commented Feb 7, 2024

Proposed change

Since version 2.1.0 (and as we are currently on 3.0.0), the bring api supports a method called complete and completeAsync, which allows to check off items instead of just deleting them. In addition to the purchase list available in the getItems and getItemsAsync, a recently list is now available to, letting us perform following mapping purchase->NEEDS_ACTION and recently->COMPLETED and use the full functionality of the todo lists. A small detail, the bring api does not at all support any uncomplete method and basic create method can be used instead, resulting in the removal from the item from the recently list and getting moved to purchase list.

DISCLAIMER: In the meantime we switched the lib to bring-api@0.1.1 which is essentially the same as 3.0.0. Have a look at #110355 for more details about it.

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

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

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 Ruff (ruff format 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

home-assistant bot commented Feb 7, 2024

Hey there @tr4nt0r, mind taking a look at this pull request as it has been labeled with an integration (bring) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of bring 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 bring Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

Copy link
Contributor

@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.

Reviewed the change and it makes sense generally. However, i suspect removing completed Todo items doesn't do anything here? If so, that gives me a little pause since it makes this part of the Home Assistant UI inconsistent and odd/broken?

@tr4nt0r
Copy link
Contributor

tr4nt0r commented Feb 18, 2024

Reviewed the change and it makes sense generally. However, i suspect removing completed Todo items doesn't do anything here? If so, that gives me a little pause since it makes this part of the Home Assistant UI inconsistent and odd/broken?

there was a small bug, fixed it localy and it worked as expected but after this PR #110769 is merged it has to be updated again anyway

@miaucl
Copy link
Contributor Author

miaucl commented Feb 18, 2024

Reviewed the change and it makes sense generally. However, i suspect removing completed Todo items doesn't do anything here? If so, that gives me a little pause since it makes this part of the Home Assistant UI inconsistent and odd/broken?

Thank you @allenporter for the review. I just have small question for unterstanding. What do you mean with inconsistancy?
In general, when completing an item, in Bring it goes from the purchase list to the recently list for easy reuse in the app. But when I delete an item, regardeless of purchase or recently list/completed or pending list, the item is deleted in Bring. Therefore I my opinion, the behaviour should be consistent with home assistant. This also has been confirmed in my tests, but if you have seen different behaviour, please let me know!

@allenporter
Copy link
Contributor

Ah yes I'm talking about how this appears in the home assistant UI

@miaucl
Copy link
Contributor Author

miaucl commented Feb 18, 2024

Ah yes I'm talking about how this appears in the home assistant UI

Ok, so you mean completed=recently and pending=purchase? Indeed, this is a bit odd but the best we can do for now😅

@allenporter
Copy link
Contributor

Yes, I am saying this is not following the spec. This integration support deleting Todo items by setting DELETE_TODO_ITEM, however it is now returning items that are not deletable and so the delete call doesn't work, and the servicetodo.remove_completed_items doesn't work either.

A way to support the recently list is to expose as a read-only todo list with recent items, and it won't claim in the UI to support deletion. (Users can then hide that entity if they don't want it to show up.)

@miaucl
Copy link
Contributor Author

miaucl commented Feb 18, 2024

Ok, I think I get it know.

All the items are deletable in bring, including the recently items (it is just the list of completed items). If that is not the case and items show up again when deleted, it is a bug. I will go over it again and verify the behaviour.

homeassistant/components/bring/todo.py Show resolved Hide resolved
homeassistant/components/bring/todo.py Show resolved Hide resolved
@miaucl
Copy link
Contributor Author

miaucl commented Feb 19, 2024

Hello,

I went again through the code and tested the use cases. Please tell me if that is not how it should work as I would see it the closest to the home assistant way of working:

For Bring! following is true:
pending=purchase
completed=recently
Recently is not really "recently" as it does not discard items based on a time of life.

Home Assistant Action Bring Action Comment
Create an new item Create a new purchase item with item summary as name and item description as specification If it corresponds to something known to Bring, it will auto-translate it!
Update an item description Update the specification of the item
Update an item name Recreate a new item with the new name Name works as primary key at the moment, which means, the old item gets deleted and a new item will be created
Complete an item The item will be moved from the purchase to the recently list No changes are made to the item otherwise
Incomplete an item The item will be moved from the recently to the purchase list Important: There is no possibility to do this explicitly with the bring api, we rather use the same way as the app, which just uses the create endpoint. The create endpoint does nothing when an item with the same name is already in the purchase list and when an item with the same name is in the recently list, it will get moved to the purchase list
Delete an item The item will be deleted, regardless of whether it is in the purchase or recently list

In my opinion, this aligns quite good with the behaviour of home assistant.

@miaucl
Copy link
Contributor Author

miaucl commented Feb 19, 2024

@allenporter If there is still a mismatch from your point of view, feel free to contact me on discord (@miaucl) to dive deeper ;)

Copy link
Contributor

@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.

Yes, thanks for checking, looks good to me.

@miaucl
Copy link
Contributor Author

miaucl commented Feb 21, 2024

@allenporter I had to rebase and align with version 0.3.1 after pep8 changes from this PR: #110769

@allenporter allenporter merged commit 6e20cc8 into home-assistant:dev Feb 21, 2024
23 checks passed
@miaucl miaucl deleted the bring_complete_list branch February 21, 2024 16:26
@frenck frenck added the noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear) label Feb 21, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 22, 2024
@home-assistant home-assistant unlocked this conversation Feb 23, 2024
@miaucl miaucl mentioned this pull request Feb 24, 2024
20 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Feb 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
by-code-owner cla-signed integration: bring new-feature noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear) Quality Scale: No score
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants