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

(For Yoav) Support returned actions #30

Merged
merged 1 commit into from
Mar 17, 2016
Merged

Conversation

nickhurlburt
Copy link
Contributor

@yschatzberg this addresses #23 as well as #12926 in the main repository.

cc @safoo @aaronTufts @jburnim

return self::urlPrefix() . '/events' . ($returnScore ? '?return_score=true' : '');
private static function restApiUrl($returnScore, $returnAction) {
return self::urlPrefix() . '/events' . ($returnScore ? '?return_score=true' : '')
. ($returnAction ? '?return_action=true' : '');
Copy link
Contributor

Choose a reason for hiding this comment

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

If both returnScore and returnAction are true, the url string generated would be:

https://api.siftscience.com/v203/events?return_score=true?return_action=true

Which is invalid. It should be:
https://api.siftscience.com/v203/events?return_score=true&return_action=true

@yschatzberg
Copy link
Contributor

@nickhurlburt commented on an issue, otherwise LGTM

@nickhurlburt
Copy link
Contributor Author

That's an important issue! Thanks for spotting that. I changed the code to handle multiple parameters correctly. Note that in our API service currently, if return_score=true is present, we actually ignore returnAction (that happens in L192 here). I think it's the right thing to pass the client's wishes through to the service so that the logic is only in the service, so the client can still pass in ?return_score=true&return_action=true, even if return_action will be ignored. I am not sure what additional documentation we should have to reflect that.

Let me know if you have more comments, otherwise I'll merge this in near the end of the day.

@yschatzberg
Copy link
Contributor

While we're making changes to the Client library, perhaps if we modify the description of $returnScore to indicate that it is depreciated and $returnAction should be used instead. Other than that LGTM.

@nickhurlburt
Copy link
Contributor Author

Comments have been added. I'll merge after the integration build runs.

nickhurlburt added a commit that referenced this pull request Mar 17, 2016
@nickhurlburt nickhurlburt merged commit eb8d88e into master Mar 17, 2016
@ehrmann ehrmann deleted the nick_add_return_action branch May 2, 2019 01:08
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.

None yet

2 participants