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 support for 17track.net package sensors #18038

Merged
merged 8 commits into from Nov 8, 2018

Conversation

Projects
None yet
4 participants
@bachya
Contributor

bachya commented Oct 31, 2018

Description:

This PR adds support for package tracking via 17track.net. The platform provides sensors denoting the number packages in a current state ("In Transit", etc.) as well as individual sensors for each package that include tracking number, etc.

Related issue (if applicable): N/A

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#7303

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: seventeentrack
    username: <USERNAME>
    password: <PASSWORD>
    show_archived: true
    show_delivered: true

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

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

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

@bachya bachya self-assigned this Oct 31, 2018

@wafflebot wafflebot bot added the in progress label Oct 31, 2018

@bachya bachya changed the title from WIP: Add support for 17track.net package sensors to Add support for 17track.net package sensors Oct 31, 2018

@bachya bachya referenced this pull request Oct 31, 2018

Merged

Add docs for 17track.net sensor platform #7303

2 of 2 tasks complete
login_result = await client.profile.login(
config[CONF_USERNAME], config[CONF_PASSWORD])
if login_result is False:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 5, 2018

Member
if not login_result:
if login_result is False:
_LOGGER.error('Invalid username and password provided')
return False

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 5, 2018

Member

Nothing is checking this return value. Just return.

return False
except SeventeenTrackError as err:
_LOGGER.error('There was an error while logging in: %s', err)
return False

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 5, 2018

Member

See above.

return False
if CONF_SCAN_INTERVAL in config:
scan_interval = config[CONF_SCAN_INTERVAL]

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 5, 2018

Member
scan_interval = config.get(CONF_SCAN_INTERVAL, DEFAULT_SCAN_INTERVAL)
"""Initialize."""
self._attrs = {ATTR_ATTRIBUTION: DEFAULT_ATTRIBUTION}
self._data = data
self._name = name

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 5, 2018

Member

Isn't this more of a type than a name? It's different status of packages right?

This comment has been minimized.

@bachya

bachya Nov 5, 2018

Contributor

You got it. I'll change the attribute to be more descriptive.

def unit_of_measurement(self):
"""Return the unit the value is expressed in."""
if self._state == 1:
return 'package'

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 5, 2018

Member

Changing unit of measurement depending on sensor value is really unfriendly if using this with 3rd party data loggers.

This comment has been minimized.

@bachya

bachya Nov 5, 2018

Contributor

Got it. I'll forgo my English preferences in favor of data cleanliness. 😄

async def async_update(self):
"""Update the sensor."""
await self._data.async_update()
[package] = [

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 5, 2018

Member

What happens if self._data.packages is an empty list?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 5, 2018

Member

An empty list should probably result in state set to None.

ATTR_TRACKING_INFO_LANGUAGE: package.tracking_info_language,
}
self._data = data
self._name = package.tracking_number

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 5, 2018

Member

So these entities for package tracking are only added once when home assistant starts and then after they are delivered they are never updated again?

This comment has been minimized.

@bachya

bachya Nov 5, 2018

Contributor

Hmm, this is a great point: although "Delivered" packages will be potentially ignored on HASS start (based on configuration), as soon as a package becomes "Delivered", that entity will remain. Not a great user experience. Suggestions? Is there a nice, approved way to periodically scrub these dead entities out (again assuming that's in configuration)?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 5, 2018

Member

Adding new entities for new packages that should be tracked is straight forward, just keep a reference to async_add_entities, and add entities as needed after each new scan.

Removing is not hard per se, we have entity method to do so, but question is when should that be done. There is some value in knowing that the package reached delivered status, right? So we can't remove immediately when that happens.

This comment has been minimized.

@bachya

bachya Nov 5, 2018

Contributor

Right.

@tjorim, you've been following this one – what would you ideally see happen? Wait some configurable time before removing delivered packages?

This comment has been minimized.

@tjorim

tjorim Nov 5, 2018

Contributor

What about creating a persistent notification once it's delivered? Then the user knows about it and the entity can be removed.
It's not ideal since the history is gone, but good enough since users just have this for tracking. (Maybe add a link to the 17track-site in the notification, to track that specific package afterwards.)

This comment has been minimized.

@bachya

bachya Nov 5, 2018

Contributor

Ooh, that’s interesting. @MartinHjelmare, opinion?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 6, 2018

Member

Are packages ever cleared from the 17track api? An alternative could be to just return the result from the api, and remove the entity when the package is no longer present in the result.

Notification could work. We fire a state change event when removing the entity, so automations are possible too.

This comment has been minimized.

@bachya

bachya Nov 6, 2018

Contributor

Are packages ever cleared from the 17track api? An alternative could be to just return the result from the api, and remove the entity when the package is no longer present in the result.

Indeterminate. @tjorim has been generous to offer his account for testing and in its results, there are packages back from April of this year. @tjorim: your account appears to return 11 total packages; does that sound like the total number of packages that you've tracked (all time) through 17track.net, or, to your knowledge, is it just a subset?

bachya added some commits Oct 31, 2018

@bachya bachya force-pushed the bachya:17track branch from 28c401c to 37aad82 Nov 6, 2018

try:
package = next(
iter([

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 8, 2018

Member

Make this a generator directly by using parenthesis instead of brackets.

(p for p in self._data.packages if ...)

Then we don't need iter.

if not self.show_delivered:
packages = [p for p in packages if p.status != VALUE_DELIVERED]
# Add new packates:

This comment has been minimized.

@MartinHjelmare
packages = [p for p in packages if p.status != VALUE_DELIVERED]
# Add new packates:
to_add = list(set(packages) - set(self.packages))

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 8, 2018

Member

We don't need to copy to list it seems. We can keep to_add as a set.

"""Update the sensor."""
await self._data.async_update()
self._state = self._data.summary[self._status]

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 8, 2018

Member

If there's an error with the data update, self._data.summary is set to empty dict, and we will get a KeyError here.

We should mark entity as unavailable instead.

This comment has been minimized.

@bachya

bachya Nov 8, 2018

Contributor

I went about this slightly differently in 61a19d7 – let me know what you think (and if I missed something).

Show resolved Hide resolved homeassistant/components/sensor/seventeentrack.py

bachya added some commits Nov 8, 2018

Revert "Member comments"
This reverts commit 61a19d7.
@MartinHjelmare

Cool! Just a small comment below.

@property
def available(self):
"""Return whether the entity is available."""
return self._data.summary.get(self._status) is not None

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 8, 2018

Member
return self._state is not None
@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Nov 8, 2018

Can be merged when build passes and comment addressed.

@bachya bachya merged commit 954191c into home-assistant:dev Nov 8, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 93.027%
Details

@wafflebot wafflebot bot removed the in progress label Nov 8, 2018

@bachya bachya deleted the bachya:17track branch Nov 8, 2018

zxdavb added a commit to zxdavb/home-assistant that referenced this pull request Nov 13, 2018

Add support for 17track.net package sensors (home-assistant#18038)
* Add support for 17track.net package sensors

* Updated CODEOWNERS

* Addressing comments

* Fixed requirements

* Member comments

* Revert "Member comments"

This reverts commit 61a19d7.

* Member comments

* Member comments

@balloob balloob referenced this pull request Nov 29, 2018

Merged

0.83 #18776

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment