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 Ituran integration #129067

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

Conversation

shmuelzon
Copy link
Contributor

@shmuelzon shmuelzon commented Oct 24, 2024

Proposed change

This PR implements a new integration for the Ituran vehicle tracking service. It currently adds a device_tracker platform and, once merged, I will add a couple of sensors as well.

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

To help with the load of incoming pull requests:

homeassistant/components/ituran/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/entity.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/manifest.json Outdated Show resolved Hide resolved
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft October 24, 2024 10:17
Copy link
Contributor Author

@shmuelzon shmuelzon left a comment

Choose a reason for hiding this comment

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

@joostlek thank you for the prompt review!

I've made the requested changes and responded to your questions. Please let me know if you'd like me to change anything else.

homeassistant/components/ituran/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/manifest.json Outdated Show resolved Hide resolved
homeassistant/components/ituran/entity.py Outdated Show resolved Hide resolved
@shmuelzon shmuelzon marked this pull request as ready for review October 24, 2024 13:52
@shmuelzon
Copy link
Contributor Author

Hey @joostlek, have you had a chance to look at my updates? Is there anything else you'd like me to change? Thanks!

homeassistant/components/ituran/device_tracker.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/device_tracker.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/entity.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/device_tracker.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/entity.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/config_flow.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@shmuelzon shmuelzon left a comment

Choose a reason for hiding this comment

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

Hey @zweckj, thanks for your review. I changed some things according to your review but there were a few follow-up questions. Please let me know if you'd like me to change anything else

homeassistant/components/ituran/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/device_tracker.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/device_tracker.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/device_tracker.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/entity.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/entity.py Outdated Show resolved Hide resolved
@shmuelzon
Copy link
Contributor Author

@zweckj I finished addressing your latest review comments. Please let me know if there's anything else. Thanks in advance!

BTW - There's a test here that fails but I don't think it's related to any of my changes.

@shmuelzon
Copy link
Contributor Author

@joostlek @zweckj If there's nothing else, please approve this pull request so I can prepare the following one for additional sensors. Thanks!

homeassistant/components/ituran/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/coordinator.py Outdated Show resolved Hide resolved
tests/components/ituran/__init__.py Outdated Show resolved Hide resolved
tests/components/ituran/conftest.py Show resolved Hide resolved
tests/components/ituran/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/ituran/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/ituran/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/ituran/test_coordinator.py Outdated Show resolved Hide resolved
@shmuelzon
Copy link
Contributor Author

@zweckj I fixed the tests as best as I could. Now might be a good time to state the I have no idea what I'm doing :)

Please let me know if there's anything else I can do to get this approved

@shmuelzon shmuelzon force-pushed the ituran-new-integration branch 2 times, most recently from df92c17 to 12bee73 Compare November 21, 2024 23:09
Copy link
Contributor

@zweckj zweckj left a comment

Choose a reason for hiding this comment

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

I think your tests need some more work. I'd suggest to take a look at some other integrations, e.g. lamarzocco, or palazetti for a shorter one

homeassistant/components/ituran/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/entity.py Outdated Show resolved Hide resolved
homeassistant/components/ituran/quality_scale.yaml Outdated Show resolved Hide resolved
homeassistant/components/ituran/strings.json Outdated Show resolved Hide resolved
homeassistant/components/ituran/strings.json Outdated Show resolved Hide resolved
tests/components/ituran/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/ituran/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/ituran/test_coordinator.py Outdated Show resolved Hide resolved
@shmuelzon
Copy link
Contributor Author

shmuelzon commented Nov 23, 2024

I think your tests need some more work. I'd suggest to take a look at some other integrations, e.g. lamarzocco, or palazetti for a shorter one

I rewrote the tests, highly inspired by the palazzetti integration.

Thanks again for all the guidance, learning a lot (and more than I bargained for 😄)

common-modules: done
docs-high-level-description: done
docs-installation-instructions: done
docs-removal-instructions: todo
Copy link
Contributor

Choose a reason for hiding this comment

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

you'll need to add that to your docs PR because you'll need to reach Bronze with this PR

"config": {
"step": {
"user": {
"data": {
Copy link
Contributor

Choose a reason for hiding this comment

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

those and otp are missing data_descriptions

Choose a reason for hiding this comment

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

I feel like the user step strings are descriptive enough as is. For the OTP I can add a description to wait for a text message or something

await __do_successful_otp_step(hass, result3, mock_ituran)


async def test_cannot_connect(
Copy link
Contributor

Choose a reason for hiding this comment

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

you should be at a minimum able to group the tests that have side effects on otp usung parametrized tests (@pytest.mark.parametrize(), but probably even all failure tests with

    func = getattr(mock_ituran, "authenticate")
    func.side_effect = IturanApiError

@zweckj
Copy link
Contributor

zweckj commented Nov 25, 2024

oh and i'd also at least a snapshot test for your platform, this can be done quickly with the helper await snapshot_platform(hass, entity_registry, snapshot, mock_config_entry.entry_id)

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

Successfully merging this pull request may close these issues.

4 participants