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 additional API params to Jobs > importUsers #354

Merged
merged 5 commits into from
Sep 10, 2019
Merged

Conversation

pinodex
Copy link
Contributor

@pinodex pinodex commented Aug 19, 2019

Changes

Added upsert and send_completion_email parameters to Jobs::importUsers.

References

Closes #353

Testing

Functionality can be tested by setting the upsert and send_completion_email boolean parameters for Jobs::importUsers. This should create a request which is compatible with the API docs of Auth0 Management V2 API: https://auth0.com/docs/api/management/v2#!/Jobs/post_users_imports

  • This change adds test coverage

  • This change has been tested on the latest version of PHP

Checklist

@pinodex pinodex requested a review from a team August 19, 2019 09:45
@joshcanhelp joshcanhelp requested review from joshcanhelp and removed request for a team August 21, 2019 15:18
@joshcanhelp
Copy link
Contributor

Thanks for this @pinodex! I'll pull this down and test it locally since we don't have tests written for this endpoint yet. Stay tuned!

Copy link
Contributor

@joshcanhelp joshcanhelp left a comment

Choose a reason for hiding this comment

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

Works great, just a change to the function signature and adding another option. Thank you!

* @return mixed
*/
public function importUsers($file_path, $connection_id)
public function importUsers($file_path, $connection_id, $upsert = false, $send_completion_email = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

We've got another optional parameter we can send, external_id. At this point, I think it would be better to pass those optional params in as an array to disambiguate the call and provide a way to add new params if that endpoints begins supporting more.

Copy link
Contributor Author

@pinodex pinodex Aug 23, 2019

Choose a reason for hiding this comment

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

I've added the array parameters. Can you see if it works?

->call();
->addFormParam('connection_id', $connection_id);

foreach ($params as $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.

Getting closer but I think we can do a better job of looking for the params we want out of this and making sure we have a reasonable value. Check if the allowed parameters - upsert, send_completion_email, external_id - are included and then get them to a useable value. This will help document expected values and hopefully avoid issues. Something like:

if ( isset( $params['upsert'] ) ) {
  $request->addFormParam('upsert', filter_var( $params['upsert'], FILTER_VALIDATE_BOOLEAN ));
}

if ( isset( $params['send_completion_email'] ) ) {
  $request->addFormParam('send_completion_email', filter_var( $params['send_completion_email'], FILTER_VALIDATE_BOOLEAN ));
}

if ( ! empty( $params['external_id'] ) && is_string( $params['external_id'] ) ) {
  $request->addFormParam('external_id', $params['external_id']);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With that approach, would we still allow for new params to be passed if the endpoint begins supporting more like you mentioned earlier?

Copy link
Contributor

Choose a reason for hiding this comment

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

They would need to be added but our team is notified of any changes to the public API and will add them as they come up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I've added the requested changes.

@joshcanhelp
Copy link
Contributor

Thanks again, @pinodex! I'll pull this down tomorrow, test, and merge!

Copy link
Contributor

@joshcanhelp joshcanhelp left a comment

Choose a reason for hiding this comment

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

One last thing here and then I'll merge this in.

One thing to note ... the \Auth0\SDK\API\Helpers\RequestBuilder::addFormParam() method is not converting the boolean values to the string value that's required by the API (it's sending 1 instead, which the API is not interpreting as true). I'll need to make a change to the request builder for this to work properly, which I'll do a in separate PR.

$request->addFormParam('send_completion_email', filter_var($params['send_completion_email'], FILTER_VALIDATE_BOOLEAN));
}

if (!empty($params['external_id']) && is_string($params['external_id'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It occurred to me that this is too restrictive and would lead to omitting int values. Let's remove that is_string check here.

Copy link
Contributor Author

@pinodex pinodex Aug 29, 2019

Choose a reason for hiding this comment

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

But shouldn't the external_id parameter supposed to be a string as defined in the API docs? https://auth0.com/docs/api/management/v2#!/Jobs/post_users_imports

Copy link
Contributor

Choose a reason for hiding this comment

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

The form is post as multipart, not JSON, so string and int are treated basically the same. I tested it out locally just to be sure and it submits an int external ID value just fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. It still submit the parameter as a string. I've removed the check.

@joshcanhelp joshcanhelp added this to the 5.6.0 milestone Aug 28, 2019
Copy link
Contributor

@joshcanhelp joshcanhelp left a comment

Choose a reason for hiding this comment

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

Thank you @pinodex!

@joshcanhelp joshcanhelp changed the title Add upsert and send_completion_email params in importUsers Add additional API params to Jobs > importUsers Sep 3, 2019
@damieng damieng merged commit b3888fa into auth0:master Sep 10, 2019
@joshcanhelp joshcanhelp mentioned this pull request Sep 17, 2019
1 task
@github-actions
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bulk User Imports - I can't Use upsert as a paramater for the importUsers feature
3 participants