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 grants, custom domains, rules_configs to API #177

Merged
merged 17 commits into from
Jan 4, 2019

Conversation

sagnew-dg
Copy link
Contributor

@sagnew-dg sagnew-dg commented Jan 3, 2019

Following up on #160, I wanted to help get the new APIs listed in the title available in the client. I've addressed the comments I see on that PR and made a few other changes I saw.

@sagnew-dg sagnew-dg changed the title Rubel changes WIP: Add grants, custom domains, rules_configs to API Jan 3, 2019
@sagnew-dg sagnew-dg changed the title WIP: Add grants, custom domains, rules_configs to API Add grants, custom domains, rules_configs to API Jan 3, 2019
Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

Thanks @sagnew-dg for taking the time to update this up. I've left you a few small comments. Looking forward for your answer.

return url + '/' + id
return url

def all(self, extra_params=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

this endpoint doesn't accept extra params, at least by reading the api explorer. Would you remove it please?

return url + '/' + id
return url

def all(self, extra_params=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

this endpoint doesn't accept extra params, at least by reading the api explorer. Would you remove it please?

to be included in the result, False otherwise.

extra_params (dictionary, optional): The extra parameters to add to
the request. The fields, include_fields, page and per_page values
Copy link
Contributor

Choose a reason for hiding this comment

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

The fields, include_fields, page and per_page values should be The page, per_page and include_totals values. Probably copy paste issue.

Suggested change
the request. The fields, include_fields, page and per_page values
the request. The page, per_page and include_totals values

@@ -40,6 +41,7 @@ def __init__(self, domain, token):
self.logs = Logs(domain, token)
self.resource_servers = ResourceServers(domain, token)
self.rules = Rules(domain, token)
self.rules_configs = RulesConfigs(domain, token)
self.stats = Stats(domain, token)
Copy link
Contributor

Choose a reason for hiding this comment

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

Custom Domains and Grants also should be added here. Please try to keep the alphabetical order 💯

"""Removes the rules config for a given key.


See: https://auth0.com/docs/api/management/v2#!/Rules_Configs/delete_rules_configs_by_key
Copy link
Contributor

Choose a reason for hiding this comment

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

It's missing the key parameter doc line

"""Sets the rules config for a given key.


See: https://auth0.com/docs/api/management/v2#!/Rules_Configs/put_rules_configs_by_key
Copy link
Contributor

Choose a reason for hiding this comment

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

It's missing the key and value parameters doc line

)

@mock.patch('auth0.v3.management.rules_configs.RestClient')
def test_update_factor_providers(self, mock_rc):
Copy link
Contributor

Choose a reason for hiding this comment

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

the name of the test is misleading, please update it

}
return self.client.delete(self._url(), params=params)

def create(self, key, value):
Copy link
Contributor

Choose a reason for hiding this comment

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

Here's a question for you. What about calling this one "set" and the delete one "unset"? I think it would follow more accordingly the api explorer docs. I tend to relate "create" with POST HTTP method, and this is a PUT sadly. :/

Copy link
Contributor Author

@sagnew-dg sagnew-dg Jan 4, 2019

Choose a reason for hiding this comment

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

One of the comments on PR #160 was to name it create, so that's what I started with. I like set / unset better as well because of the HTTP method; I'll change it to that.

…it's a PUT, fix docstrings, remove extra_params where not needed
@sagnew-dg
Copy link
Contributor Author

Thanks for the review; latest commit should address the comments. 😀

@lbalmaceda lbalmaceda merged commit e09d69d into auth0:master Jan 4, 2019
@lbalmaceda
Copy link
Contributor

lbalmaceda commented Jan 4, 2019

Thanks again! I just released 3.6.0 with this changes.

@lbalmaceda lbalmaceda modified the milestones: v3-Next, 3.6.0 Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants