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.

Sign up for GitHub

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

Jump to bottom

fix: fix get_resources not adding usps_mail_delivered to configuration selection #966

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

fix: fix get_resources not adding usps_mail_delivered to configuration selection #966

wants to merge 3 commits into from

Conversation

gpeavy
Copy link

@gpeavy gpeavy commented Aug 9, 2024 •
edited
Loading

Proposed change

Fix bug in delivered USPS mail implementation.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests
  • Documentation update
  • Adds a new shipper
  • Update existing shipper

Additional information

fetch() is never called with sensor="usps_mail_delivered" in process_emails() as resources does not contain the BinarySensors.
This is probably just a temporary fix.

@gpeavy gpeavy changed the title Fix usps_mail_delivered fix: usps_mail_delivered Aug 9, 2024
firstof9
firstof9 reviewed Aug 9, 2024
View reviewed changes
custom_components/mail_and_packages/helpers.py Outdated
@@ -217,6 +217,12 @@ def process_emails(hass: HomeAssistant, config: ConfigEntry) -> dict:
except Exception as err:
_LOGGER.error("Error updating sensor: %s reason: %s", sensor, err)

# Update usps_mail_delivered
try:
fetch(hass, config, account, data, "usps_mail_delivered")
Copy link
Collaborator

Choose a reason for hiding this comment

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

The sensor would already be in the resources and searched for twice.

Copy link
Author

Choose a reason for hiding this comment

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

I added some additional logging in my testing and I don't think it's in resources. If resources is set by get_resources(), SENSOR_TYPES does not contain the BinarySensors. I think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

resources is set here:

Home-Assistant-Mail-And-Packages/custom_components/mail_and_packages/helpers.py

Line 177 in 4a39728

resources = config.get(CONF_RESOURCES)

The list comes from the configuration.

Copy link
Author

@gpeavy gpeavy Aug 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Please correct me in I am wrong. This is my first time digging into HA. Does config.get(CONF_RESOURCES) get set by:

Home-Assistant-Mail-And-Packages/custom_components/mail_and_packages/config_flow.py

Lines 211 to 213 in 4a39728

vol.Required(
CONF_RESOURCES, default=_get_default(CONF_RESOURCES)
): cv.multi_select(get_resources()),

If so get_resources() here:

Home-Assistant-Mail-And-Packages/custom_components/mail_and_packages/helpers.py

Lines 94 to 103 in 4a39728

def get_resources() -> dict:
"""Resource selection schema.
Returns dict of user selected sensors
"""
known_available_resources = {
sensor_id: sensor.name for sensor_id, sensor in SENSOR_TYPES.items()
}
return known_available_resources

uses SENSOR_TYPES defined here:

Home-Assistant-Mail-And-Packages/custom_components/mail_and_packages/const.py

Line 651 in 4a39728

SENSOR_TYPES: Final[dict[str, SensorEntityDescription]] = {

which does not include usps_mail_delivered defined here:

Home-Assistant-Mail-And-Packages/custom_components/mail_and_packages/const.py

Lines 1157 to 1161 in 4a39728

"usps_mail_delivered": BinarySensorEntityDescription(
name="USPS Mail Delivered",
key="usps_mail_delivered",
entity_registry_enabled_default=False,
),

I must be missing something. :)

Copy link
Collaborator

@firstof9 firstof9 Aug 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah you are correct, it's been excluded from the resources in error. That's where the bug needs to be fixed.
get_resources() needs the fixing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Feel free to give it a run on a test HA instance if you like too, all the sensors will now be sorted alphabetically.

Copy link
Author

Choose a reason for hiding this comment

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

I've finally started to test it now. Running into a weird situation where the Logbook and History show the sensor turning on, but the sensor still shows off.
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

if you refresh does it show "on"?
if so, this will be an bug in HA not updating the front end.

Copy link
Author

Choose a reason for hiding this comment

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

Interesting. Yep. Issue with HA front end. I had clicked around to different views in HA and even restarted the Mail and Packages integration, but it wasn't until I refreshed the browser that it updated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've noticed this issue after the 2024.8.2 update.

firstof9 and others added 2 commits August 10, 2024 11:57
@firstof9
fix: fix get_resources not adding usps_mail_delivered to configuratio…
5199ed4 
…n selection
@gpeavy
Merge pull request #1 from firstof9/mail-delivered-fix
a1437ce 
fix: fix get_resources not adding usps_mail_delivered to configuration selection
@firstof9 firstof9 changed the title fix: usps_mail_delivered fix: fix get_resources not adding usps_mail_delivered to configuration selection Aug 10, 2024
firstof9
firstof9 requested changes Aug 10, 2024
View reviewed changes
Copy link
Collaborator

@firstof9 firstof9 left a comment

Choose a reason for hiding this comment

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

This should fix the import linting errors.

custom_components/mail_and_packages/const.py
BinarySensorDeviceClass,
BinarySensorEntityDescription,
)
from .entity import MailandPackagesBinarySensorEntityDescription
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
from .entity import MailandPackagesBinarySensorEntityDescription
from homeassistant.components.binary_sensor import BinarySensorDeviceClass

custom_components/mail_and_packages/const.py
BinarySensorEntityDescription,
)
from .entity import MailandPackagesBinarySensorEntityDescription
from homeassistant.components.binary_sensor import BinarySensorDeviceClass
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
from homeassistant.components.binary_sensor import BinarySensorDeviceClass
from homeassistant.components.sensor import SensorDeviceClass, SensorEntityDescription

custom_components/mail_and_packages/const.py
BinarySensorEntityDescription,
)
from .entity import MailandPackagesBinarySensorEntityDescription
from homeassistant.components.binary_sensor import BinarySensorDeviceClass
from homeassistant.components.sensor import SensorDeviceClass, SensorEntityDescription
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
from homeassistant.components.sensor import SensorDeviceClass, SensorEntityDescription
from homeassistant.helpers.entity import EntityCategory

custom_components/mail_and_packages/const.py
BinarySensorEntityDescription,
)
from .entity import MailandPackagesBinarySensorEntityDescription
from homeassistant.components.binary_sensor import BinarySensorDeviceClass
from homeassistant.components.sensor import SensorDeviceClass, SensorEntityDescription
from homeassistant.helpers.entity import EntityCategory
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
from homeassistant.helpers.entity import EntityCategory

custom_components/mail_and_packages/const.py
BinarySensorEntityDescription,
)
from .entity import MailandPackagesBinarySensorEntityDescription
from homeassistant.components.binary_sensor import BinarySensorDeviceClass
from homeassistant.components.sensor import SensorDeviceClass, SensorEntityDescription
from homeassistant.helpers.entity import EntityCategory

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
from .entity import MailandPackagesBinarySensorEntityDescription

@firstof9 firstof9 added code-quality Improves code quality or adds/updates tests work-in-progress labels Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@firstof9 firstof9 firstof9 requested changes

Assignees
No one assigned
Labels
code-quality Improves code quality or adds/updates tests work-in-progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gpeavy @firstof9