Change API behaviour

[arieboven]

All the API calls are using the GET method, even the calls that preform actions. This change divide the API in GET and POST commands, for better implementation in other software.

Tested in OctoPrint 1.10.2 + python 3.9.2 + OctoPi 1.0.0.

API tested with CURL, and Home Assistant.

[thomst08]

Hey @arieboven,

This is awesome! Your right, they should be different, I like what you've done. My only argument would be, shouldn't the lights new status be returned after any post command too? When I have a chance to test everything then merge it :)

[thomst08]

I have had a chance to look over this, good work, everything looks good to me. I agree with you that changing the light status should be a POST request, however, this does mean a big change for anyone currently using OctoLight through the API.

This breaks the current setup, I would prefer to fix it but don't want to upset too many people. I have also noticed this does not work with OctoApp as it supports the current API and will need to be updated to work with the new version. (I am unaware of any other apps like this atm, besides anyone setting up a widget/call from Home Assistant).

At the moment I am going to leave this PR alone and reach out to OctoApp and maybe the OctoPrint community first before merging. When merged, I will also make sure to add a notice to OctoPrint's plugins that all older versions have a different API and this new one may/will break compatibility with current setups if used. I might also change the POST request to return the new status of the light when changed as I feel that wouldn't hurt and allows the requester to validate the state if needed.

If you have any comments, ideas, feedback, etc, please let me know. Awesome work again.

[arieboven]

Thank you for your response and consideration.

I understand the concern, as it crossed my mind as well. Everyone using the API will need to update their implementation. I don't know the best approach how to push this upgrade, other than making an announcement before the update becomes available.

The change to return the state after the POST request wouldn't hurt. It is a matter of adding one line. I am also happy to make this change if you haven't the solution for it yet.

It's not my intention to put you in a difficult position. I believe this is a better implementation, but it may cause some inconvenience for current users who will need to update their setup. However, the longer you wait, the harder it will be to implement this change. If you decide not to proceed with this update due to the potential hassle, that's perfectly fine with me.

[thomst08]

All good @arieboven, I have talked with some devs on the OctoPrint discord server, we should update the implementation as it's been this way for a long time. I have done some changes my end and will update this PR and merge it when everything is ready.

Basically we discussed the changes and I will add back some of the old api calls, that way going forward it will work with the old setup while having the new setup. But if the old calls are used, it will trigger a warning, in the future, I will remove the old calls (end of the year maybe). The developer of OctoApp will update the app too to work with the new setup.

Basically, everything is sorted and I will update the PR over the next week :)

[thomst08]

Hey @arieboven,

I have had a moment and looked over everything and tested it. I have updated the code to include the old setup and tested it, working fine. I setup a warning that will log to the logs when used, I have also change the permissions just a little and tweaked the requests, all seems to work fine from my point of view. I have also updated the version number, and README.

If you are happy with everything, I will pull it in, please, if you have any issues, let me know, I might have missed something (or done something stupid) :) If I don't hear from you, I will merge it in a few days, create a release and update the information on OctoPrints plugin page as well.

Thank you again for your help. :)

[arieboven]

Hey @thomst08,

I have reviewed and tested the the plugin with the latest changes you made and did not find any issues with either the new or the old API method.

However, I'm uncertain about the change in permissions. Here’s my understanding of how permissions work (please correct me if I’m wrong): OctoPrint has predefined permissions (such as CONTROL and STATUS), which can be assigned by group or user through Access Control.

For plugins that don't comply with the predefined permissions, you can create additional permissions using the get_additional_permissions function. This creates permissions at startup that can be used by OctoPrint. With the latest change, you now have two additional permissions (OctoLight Control and OctoLight Status) that will be created, as seen in the log:

2024-07-07 16:31:10,397 - octoprint.server - INFO - Added new permission from plugin octolight: PLUGIN_OCTOLIGHT_CONTROL (needs: "Need(method='role', value='plugin_octolight_admin')")
2024-07-07 16:31:10,397 - octoprint.server - INFO - Added new permission from plugin octolight: PLUGIN_OCTOLIGHT_STATUS (needs: "Need(method='role', value='plugin_octolight_admin')")

However, these additional permissions are not used now. For the GET request, the predefined STATUS permission is used, and for the POST request, the predefined CONTROL permission is used. These additional permissions can either be removed or update the GET and POST requests to utilize them (see initial pull request).

Aside from this, great implementation in supporting both the new and (deprecated) old method.

[thomst08]

Oh, I see what you mean with the permissions, sorry, I miss understood how they worked. I will correct them and retest.

[thomst08]

You are 100% correct with the permission, sorry, this was a miss understanding on my end. The new commit should correct that issue. :)

[thomst08]

Merged and released. I have also updated the info on the OctoPrint plugin page and will just wait for that info to be updated, I have also scheduled a notice for old versions stating about the changes for the API. Thanks again for your help :)