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

Add FireServiceRota/BrandweerRooster integration #38206

Merged
merged 85 commits into from
Nov 25, 2020
Merged

Add FireServiceRota/BrandweerRooster integration #38206

merged 85 commits into from
Nov 25, 2020

Conversation

cyberjunky
Copy link
Contributor

@cyberjunky cyberjunky commented Jul 25, 2020 •
edited
Loading

Breaking change

Proposed change

This PR adds code which integrate functionality provided by https://www.fireservicerota.co.uk and https://www.brandweerrooster,nl
"FireServiceRota is a powerful and flexible availability, scheduling and dispatching system for firefighters."

This integration implements the following entities:

In this PR:

  • sensor.incidents: list the latest incident with all relevant data as attributes gather via threaded web-socket client connection

Next PR's:

  • binary_sensor.duty: shows if user is available for duty (as entered via the FireServiceRota mobile app and/or website)
  • switch.incident_response: user can acknowledge or reject an incident response for active incident

Practical use can be sounding an alarm and/or blinking lights when an emergency incident is received, use text to speech to play incident details while getting dressed, respond with an acknowledge using a door-sensor leaving house or a button or something else connected to home assistant to let your teammates know you are underway.

You need to have a FireServiceRota or BrandweerRooster user account to be able to use this integration (this mostly mean you are a firefighter and your team is using one of those web services for scheduling/alerting)

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)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Configuration/setup is done via Integration menu asking for user credentials once, and then storing only oauth token information, so a new token can be requested if needed using the refresh token.

It is using python module https://pypi.org/project/pyfireservicerota/ https://github.com/cyberjunky/python-fireservicerota for communication with the API.

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
  • 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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@MartinHjelmare MartinHjelmare marked this pull request as draft August 16, 2020 14:25
@cyberjunky
Copy link
Contributor Author

cyberjunky commented Aug 19, 2020 •
edited
Loading

After a major rewrite of code and pyservicerota package I'm ready to iron out any left overs.
Documentation is being written.
Moved it from WIP/DRAFT to ready to be reviewed, thanks in advance!

@cyberjunky cyberjunky marked this pull request as ready for review August 19, 2020 18:40
@cyberjunky cyberjunky changed the title WIP: Adding FireServiceRota/BrandweerRooster integration Adding FireServiceRota/BrandweerRooster integration Aug 19, 2020
@MartinHjelmare MartinHjelmare changed the title Adding FireServiceRota/BrandweerRooster integration Add FireServiceRota/BrandweerRooster integration Aug 20, 2020
austinmroczek
austinmroczek reviewed Aug 28, 2020
View reviewed changes
Copy link
Contributor

@austinmroczek austinmroczek left a comment

Choose a reason for hiding this comment

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

It looks pretty good to me @cyberjunky. Just a couple of minor things.

This is my first attempt at a code review, so someone from the core team should look over my shoulder.

@cyberjunky
Copy link
Contributor Author

cyberjunky commented Sep 15, 2020 •
edited
Loading

@austinmroczek I completely overlooked your reviews/audit, didn't see any notification there were audits posted, so I came here to write a small bump post, to get it started ;-) I will work through your advises in the next hours. Thanks!

@cyberjunky cyberjunky mentioned this pull request Sep 19, 2020
Closed
8 tasks
@cyberjunky
Copy link
Contributor Author

@austinmroczek Hi Austin, I have fulfilled all requests, if you have some time please review again if you want, and put it in the next queue. ;D

@cyberjunky
Copy link
Contributor Author

@frenck can this be moved to 'follow up review queue' please, it has been there during two HA version releases now. Would like to know what to fix/change and move on some day. In exchange I can do some Hacktoberfest PR's ;-D

@cyberjunky
Copy link
Contributor Author

It sounds silly but this integration could potentially save lives. In practice we have seen that the firefighter using this service and integration gets warned of an emergency seconds before their beeper goes off ⏰

austinmroczek
austinmroczek approved these changes Oct 3, 2020
View reviewed changes
Copy link
Contributor

@austinmroczek austinmroczek 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 to me. Thanks

@cyberjunky cyberjunky mentioned this pull request Oct 4, 2020
Merged
8 tasks
Bre77
Bre77 approved these changes Oct 15, 2020
View reviewed changes
Copy link
Contributor

@Bre77 Bre77 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. Suggest inheriting the base entity class to reduce boilerplate properties.

Kane610
Kane610 requested changes Oct 15, 2020
View reviewed changes
Copy link
Member

@Kane610 Kane610 left a comment

Choose a reason for hiding this comment

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

There are a few things to fix, but off to a good start

@cyberjunky cyberjunky requested a review from Kane610 October 15, 2020 16:20
Bre77
Bre77 approved these changes Oct 15, 2020
View reviewed changes
Copy link
Contributor

@Bre77 Bre77 left a comment

Choose a reason for hiding this comment

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

Nice Work

@balloob balloob merged commit ea52ffc into home-assistant:dev Nov 25, 2020
@cyberjunky cyberjunky deleted the fireservicerota branch November 25, 2020 15:47
MartinHjelmare
MartinHjelmare reviewed Nov 28, 2020
View reviewed changes
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.

Please address the comments in a new PR. Thanks!

homeassistant/components/fireservicerota/config_flow.py
extra_inputs = user_input

if self._existing_entry:
extra_inputs = self._existing_entry
Copy link
Member

Choose a reason for hiding this comment

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

We seem to be missing test cases in the config flow. It needs 100% test coverage.

MartinHjelmare pushed a commit that referenced this pull request Nov 29, 2020
@cyberjunky
Address FireServiceRota late code review (#43741)
eab6a05 
* Address review comment from PR #38206

* Address review comment from PR #43638

* Address review comment from PR #43700

* isort fixed

* Better code for duty entity update

* Removed all pylint relative-beyond-top-level

* Removed logger entry from entity state method
rccoleman pushed a commit to rccoleman/core that referenced this pull request Dec 4, 2020
@cyberjunky @Kane610
@balloob @rccoleman
Add FireServiceRota/BrandweerRooster integration (home-assistant#38206)
9072667 
Co-authored-by: Robert Svensson <Kane610@users.noreply.github.com>
Co-authored-by: Paulus Schoutsen <paulus@home-assistant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@balloob balloob balloob left review comments

@MartinHjelmare MartinHjelmare MartinHjelmare left review comments

@Bre77 Bre77 Bre77 approved these changes

@austinmroczek austinmroczek austinmroczek approved these changes

@Kane610 Kane610 Kane610 approved these changes

Assignees
No one assigned
Labels
cla-signed has-tests integration: fireservicerota new-integration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@cyberjunky @Kane610 @balloob @Bre77 @MartinHjelmare @austinmroczek @homeassistant