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

feat: Add Integration through UI without breaking old config #7

Merged
merged 2 commits into from
Jan 4, 2025

Conversation

sayam93
Copy link
Contributor

@sayam93 sayam93 commented Dec 15, 2024

Allow users to add this integration using UI without breaking old configuration. Any notify service previously created through manual code in configuration.yaml remains functional. It gives the user choice to setup through UI if they want or continue with the old method.

While setup through UI, the user enters the Whatsapp number (without country code) and country code separately.

If the user's number is '+919876556789' where country code is '+91', then the user shall to enter 9876556789 as the WhatsApp Phone Number and +91 as the Country Code during setup. A notification service name notify.whatspie_919876556789 will be saved in such case.

When calling the notify service (like notify.whatspie_919876556789), the user has to mention the full phone number of the recipient with country code.

An updated readme is also available for users.

@sayam93
Copy link
Contributor Author

sayam93 commented Dec 15, 2024

@arifwn could you kindly take a look at this?

@arifwn
Copy link
Owner

arifwn commented Dec 16, 2024

Thank you @sayam93 , I'll review your pull request soon.

@sayam93
Copy link
Contributor Author

sayam93 commented Dec 16, 2024

Thank you @sayam93 , I'll review your pull request soon.

@arifwn You're welcome. Let me know if there's any issue.

@arifwn
Copy link
Owner

arifwn commented Dec 17, 2024

Hi @sayam93,

I found two issues in the pull requests when I tested it on homeassistant version 2024.12.3 :

The first one happen when adding device via configflow which results in auth_error. This is fixed by updating the config_flow.py file line 39, by changing await self._test_credentials(user_input[CONF_API_TOKEN], full_number, country_code) to await self._test_credentials(user_input[CONF_API_TOKEN], full_number) . For context, I have removed my previous settings from my configuration.yaml file before trying to setup a new device.

The second one happens when the device has been configured. Attempting to configure the device again via ui will result in http 500. The error go away by updating async_get_options_flow method to return WhatsPieOptionsFlowHandler() instead of WhatsPieOptionsFlowHandler(config_entry), but then it'll cause the phone number and country code fields to be empty. For context, I have removed my previous settings from my configuration.yaml file (same config with the previous error).

Other than the two issues above, the update works on homeassistant version 2024.12.3. Existing config is still used if it exists, and the new device added from the ui can be used via notify.whatsie_<phone number> notification. This is a great update, thank you!

@sayam93 sayam93 marked this pull request as draft December 17, 2024 14:24
@sayam93 sayam93 marked this pull request as ready for review December 17, 2024 14:25
@sayam93
Copy link
Contributor Author

sayam93 commented Dec 17, 2024

Hi @sayam93,

I found two issues in the pull requests when I tested it on homeassistant version 2024.12.3 :

The first one happen when adding device via configflow which results in auth_error. This is fixed by updating the config_flow.py file line 39, by changing await self._test_credentials(user_input[CONF_API_TOKEN], full_number, country_code) to await self._test_credentials(user_input[CONF_API_TOKEN], full_number) . For context, I have removed my previous settings from my configuration.yaml file before trying to setup a new device.

The second one happens when the device has been configured. Attempting to configure the device again via ui will result in http 500. The error go away by updating async_get_options_flow method to return WhatsPieOptionsFlowHandler() instead of WhatsPieOptionsFlowHandler(config_entry), but then it'll cause the phone number and country code fields to be empty. For context, I have removed my previous settings from my configuration.yaml file (same config with the previous error).

Other than the two issues above, the update works on homeassistant version 2024.12.3. Existing config is still used if it exists, and the new device added from the ui can be used via notify.whatsie_<phone number> notification. This is a great update, thank you!

@arifwn code updated now. this should fix both the issues pointed out by you.

@sayam93 sayam93 marked this pull request as draft December 17, 2024 14:28
@sayam93 sayam93 marked this pull request as ready for review December 17, 2024 14:41
@sayam93 sayam93 marked this pull request as draft December 17, 2024 18:53
@sayam93 sayam93 marked this pull request as ready for review December 17, 2024 18:54
@sayam93
Copy link
Contributor Author

sayam93 commented Dec 18, 2024

Screenshot 2024-12-18 at 8 51 50 PM Screenshot 2024-12-18 at 8 52 05 PM

@arifwn Above changes made to the initial PR to fix the issues mentioned by you.

@arifwn
Copy link
Owner

arifwn commented Dec 18, 2024

Thank you @sayam93 , I'll test it again soon.

@arifwn
Copy link
Owner

arifwn commented Dec 20, 2024

@sayam93 the edit form is no longer crashing, but the phone number and country code field are empty:

Screencast.From.2024-12-20.21-33-09.mp4

@sayam93 sayam93 marked this pull request as draft December 20, 2024 15:41
@sayam93 sayam93 marked this pull request as ready for review December 20, 2024 15:42
Allow users to add this integration using UI without breaking old configuration. Any notify service previously created through manual code in configuration.yaml remains functional. It gives the user choice to setup through UI if they want or continue with the old method.

While setup through UI, the user enters the Whatsapp number (without country code) and country code separately.

If the user's number is '+919876556789' where country code is '+91', then the user shall to enter 9876556789 as the WhatsApp Phone Number and +91 as the Country Code during setup. A notification service name notify.whatspie_919876556789 will be saved in such case.

When calling the notify service (like notify.whatspie_919876556789), the user has to mention the full phone number of the recipient with country code.

An updated readme is also available for users.
@sayam93 sayam93 marked this pull request as draft December 20, 2024 16:58
@sayam93 sayam93 marked this pull request as ready for review December 20, 2024 16:59
@sayam93
Copy link
Contributor Author

sayam93 commented Dec 20, 2024

@sayam93 the edit form is no longer crashing, but the phone number and country code field are empty:

Screencast.From.2024-12-20.21-33-09.mp4

@arifwn I have updated the PR to fix this by adding two new constants to const.py in addition to the existing 4.

I have also imported these constants in the config_flow.py to store unsanitised value from user input while setting up the configuration entry using UI.

These values are then showed up while editing the config and when edited, correctly combined to rebuild the number to be stored in the config.

See the below screenshots for changes.

Screenshot 2024-12-20 at 10 33 45 PM Screenshot 2024-12-20 at 10 33 57 PM Screenshot 2024-12-20 at 10 34 47 PM Screenshot 2024-12-20 at 10 35 03 PM Screenshot 2024-12-20 at 10 35 28 PM

@sayam93
Copy link
Contributor Author

sayam93 commented Dec 26, 2024

@arifwn Let me know how the updated PR works for you. I am unable to test the latest changes for myself since my trial subscription expired.

@arifwn
Copy link
Owner

arifwn commented Dec 27, 2024

Thank you @sayam93 , I'll do some more testing this weekend.

@sayam93
Copy link
Contributor Author

sayam93 commented Jan 1, 2025

Thank you @sayam93 , I'll do some more testing this weekend.

@arifwn Do let me know if there are any issues.

@arifwn
Copy link
Owner

arifwn commented Jan 2, 2025

@sayam93 featurewise, I found no issue, but the manifest check below produce some error:

image

https://github.com/arifwn/homeassistant-whatspie-integration/actions/runs/12435355722

@sayam93 sayam93 marked this pull request as draft January 2, 2025 16:50
@sayam93 sayam93 marked this pull request as ready for review January 2, 2025 16:52
@sayam93
Copy link
Contributor Author

sayam93 commented Jan 2, 2025

@sayam93 featurewise, I found no issue, but the manifest check below produce some error:

image

https://github.com/arifwn/homeassistant-whatspie-integration/actions/runs/12435355722

@arifwn The latest commit should fix all these errors.

@arifwn
Copy link
Owner

arifwn commented Jan 3, 2025

Thank you @sayam93! There is some minor manifest issue left, but those are minor issues and I can fix that when I merge this pr soon.

@arifwn arifwn merged commit 29ca41d into arifwn:main Jan 4, 2025
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants