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 channel update API & channel acl update API #35

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

minghan9456
Copy link

@minghan9456 minghan9456 commented Oct 29, 2020

Hey @alfg
This is awesome project.
This is very helpful for me, so I want to share my commit with you.

I add 2 new API about channel

  1. PATCH /servers/:serverid/channels/:channelid
  2. PATCH /servers/:serverid/channels/:channelid/acl

Please check them.

@alfg
Copy link
Owner

alfg commented Oct 29, 2020

Thank you! Will review soon.

Copy link
Owner

@alfg alfg left a comment

Choose a reason for hiding this comment

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

Left a few comments. I don't think this is working with the latest build from master, if you can check it out. Thanks.

app/api.py Outdated Show resolved Hide resolved
app/api.py Outdated Show resolved Hide resolved
app/api.py Outdated
temporary = request.form.get('temporary')

if name is not None:
channel.name = name.encode('utf-8')
Copy link
Owner

Choose a reason for hiding this comment

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

Hey, have you tried running this with the latest from master? I tried passing name in the form and it errors when trying to set the channel state. It works if I remove the .encode('utf-8) part, though.

Copy link
Author

@minghan9456 minghan9456 Oct 30, 2020

Choose a reason for hiding this comment

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

oh
If don't use .encdoe('utf-8'), will get AttributeError: 'unicode' object has no attribute '__dict__'.
But channel name will be updated on mumble client.

My testing command :
curl --location --request PATCH 'http://{ip}:8080/servers/1/channels/13' --form 'name=test'

Copy link
Author

Choose a reason for hiding this comment

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

I try to use str(name), after updating will getChannelState(channel_id) again for response.
It works.

@minghan9456 minghan9456 requested a review from alfg October 30, 2020 09:55
Copy link
Owner

@alfg alfg left a comment

Choose a reason for hiding this comment

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

Sorry for the delayed response.

The update_channel endpoint looks good. I have some questions on the ACL endpoint, though. Thanks.

Comment on lines +641 to +643
update_acls = origin_acl[0]
update_groups = origin_acl[1]
update_inherit = origin_acl[2]
Copy link
Owner

Choose a reason for hiding this comment

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

Can you explain what's going on here with the ACL settings. It's been a while since I've worked on Murmur Slice API, so I'm a bit rusty. :)

Copy link
Author

Choose a reason for hiding this comment

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

oh this part, I want to follow RESTful PATCH.
Example :
If this request only pass acls, not pass groups and inherit, will only update acls.
update_groups will be groups of origin_acl, update_inherit will be inherit of origin_acl.
And update_acls will be new_acls

on server.setACL(channel_id, update_acls, update_groups, update_inherit)

update_groups = origin_acl[1]
update_inherit = origin_acl[2]

params = request.get_json()
Copy link
Owner

Choose a reason for hiding this comment

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

I see you're reading the request JSON body here. However, all of the methods read via multipart form data, so I would rather not change it if possible.

Looking back at the project, I do wish I went with reading the JSON body instead though. 🙂

Copy link
Author

@minghan9456 minghan9456 Nov 2, 2020

Choose a reason for hiding this comment

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

It is ok.
I just think json is easy to pass array data like acls or groups.
Because if want to get acls array I need to via request.form.get(acls[][applyHere]) & request.form.get(acls[][applySubs]) and .......
Or have other way can easy to get params.

Can back to use form data.

Copy link
Author

Choose a reason for hiding this comment

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

here form data with array, you have any idea?

app/api.py Outdated Show resolved Hide resolved
@alfg alfg changed the title add channel update API & channel acl udpate API Add channel update API & channel acl update API Nov 2, 2020
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