maorcc / citymind_water_meter

Home Assistant Integration with cp.city-mind.com, an Israeli water meters reading service
Apache License 2.0
40 stars 2 forks source link

Change base url #63

Closed Arielgordon123 closed 3 months ago

Arielgordon123 commented 3 months ago

closes #62

elad-bar commented 3 months ago

@Arielgordon123 thanks for the PR, have you checked it to work? Reason I'm asking because the endpoints both old and new changed to the latest?

elad-bar commented 3 months ago

In addition, can you pls change the version number in manifest and update the changelog as part of this PR?

Thanks again

Arielgordon123 commented 3 months ago

@Arielgordon123 thanks for the PR, have you checked it to work? Reason I'm asking because the endpoints both old and new changed to the latest?

as far as i can see this is how it's working from the web-ui except for the ENDPOINT_MY_MESSAGES that I've didnt see, and not working with the new url

mikeage commented 3 months ago

I came here to propose this change, but I see people have already found it :-)

[edit: ignore the deleted part; I see you fixed the url that I was complaining about, in a better way that I was going to. Sorry!]

elad-bar commented 3 months ago

@Arielgordon123 thank you for the efforts to make it work again, had few minutes to review the changes, it seems that you changed the base url without fully testing that all endpoints are working, to understand the range of supported endpoints, i strongly suggest to create a postman project and investigate the set of new endpoints within RYM pro website.

it's not that straight forward as it seems, some of the endpoints are not aligned with the changes, including monthly consumption and some of the endpoints of consumer / municipal,

if it's too much effort, i will try to get to it during the weekend and align it

thanks

mikeage commented 3 months ago

Is it worth merging this and then fixing the remaining broken URLs when possible? For those who don't take the update manually, the entire integration is now broken; this changes restores the vast majority of the most important functionality, which is a lot better than the current situation!

elad-bar commented 3 months ago

in addition to the change above for monthly consumption, the message endpoint was removed, need to remove from dictionary the following value ENDPOINT_DATA_UPDATE.API_DATA_SECTION_MY_MESSAGES:

to:

ENDPOINT_DATA_UPDATE = {
    API_DATA_SECTION_LAST_READ: ENDPOINT_LAST_READ,
    API_DATA_SECTION_VACATIONS: ENDPOINT_VACATIONS,
    API_DATA_SECTION_MY_ALERTS: ENDPOINT_MY_ALERTS,
    API_DATA_SECTION_SETTINGS: ENDPOINT_MY_ALERTS_SETTINGS,
}
elad-bar commented 3 months ago

Is it worth merging this and then fixing the remaining broken URLs when possible? For those who don't take the update manually, the entire integration is now broken; this changes restores the vast majority of the most important functionality, which is a lot better than the current situation!

i prefer to release a version that is working and not one that partially working, monthly sensor wouldn't work properly if it was just released as suggested... for you it will be fine, for others it will not be fine, i prefer to be bit more strict, hope it make sense

elad-bar commented 3 months ago

Thanks a lot! Please let me know when I can merge it

Arielgordon123 commented 3 months ago

I think I'm done with this Change if you don't have any suggestions

elad-bar commented 3 months ago

Looks good, assume you have tested it