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 Roles and Users endpoints integration tests #174

Merged
merged 5 commits into from
Jul 2, 2019

Conversation

joshcanhelp
Copy link
Contributor

@joshcanhelp joshcanhelp commented Jun 27, 2019

Changes

  • Add integration tests (recorded with VCR) for new Roles and Users endpoints.
  • Rename unlink_users_account to unlink_user_account, added alias and test
  • Add delete_with_body method to send a body rather than params with a DELETE request

Note for reviewers:

  • New VCR recordings were add in spec/fixtures/vcr_cassettes/Auth0_Api_V2_Users/ and spec/fixtures/vcr_cassettes/Auth0_Api_V2_Roles/. Sensitive information (API token) is automatically filtered out.
  • The other YML recording files have either a changed format, changed date, or removed dummy API token (the one that was there is not valid but makes more sense for them all to have API_TOKEN).
  • In general, YML files can be ignored. Without those, review is ~360 changed lines total.

References

Testing

  • This change adds unit test coverage (added in original PR)
  • This change adds integration test coverage
  • This change has been tested on the latest version of Ruby (2.6.0)

@joshcanhelp joshcanhelp changed the title Add roles and users integration tests [WIP] Add roles and users integration tests Jun 27, 2019
@joshcanhelp joshcanhelp force-pushed the add-roles-and-users-integration-tests branch from 6170d8a to 579cf77 Compare June 28, 2019 19:11
@joshcanhelp joshcanhelp force-pushed the add-roles-and-users-integration-tests branch from 579cf77 to 7ff2375 Compare June 28, 2019 19:22
@joshcanhelp joshcanhelp changed the title [WIP] Add roles and users integration tests Add Roles and Users endpoints integration tests Jun 28, 2019
@joshcanhelp joshcanhelp force-pushed the add-roles-and-users-integration-tests branch from 7ff2375 to 6278b66 Compare June 28, 2019 19:25
@joshcanhelp joshcanhelp added this to the v4.8.0 milestone Jun 28, 2019
@joshcanhelp joshcanhelp marked this pull request as ready for review June 28, 2019 20:11
@joshcanhelp joshcanhelp requested a review from a team June 28, 2019 20:11
@@ -220,19 +221,19 @@ def remove_user_roles(user_id, roles)
raise Auth0::MissingUserId, 'Must supply a valid user_id' if user_id.to_s.empty?
validate_strings_array roles
path = "#{users_path}/#{user_id}/roles"
delete(path, { roles: roles })
delete_with_body path, roles: roles
Copy link
Contributor

Choose a reason for hiding this comment

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

I see inconsistencies on the syntax when calling a function with parameters.

delete(path, {param: value} vs delete path, param:value.

Should we add a linter as a precommit for this repo as well or is this change on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a linter that I run, rubocop, but it doesn't pick this up

@@ -20,6 +20,8 @@ module HTTPProxy
call(:get, url(safe_path), timeout, get_headers)
elsif method == :delete
call(:delete, url(safe_path), timeout, add_headers({params: body}))
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this params:body header for? I guess you meant to send a query instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oddly, sending it as params: converts it to a query ... one of the many quirks of this HTTP library

@joshcanhelp joshcanhelp merged commit 6ec34e6 into master Jul 2, 2019
@joshcanhelp joshcanhelp deleted the add-roles-and-users-integration-tests branch July 2, 2019 20:02
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