Skip to content
This repository has been archived by the owner on May 26, 2022. It is now read-only.

Adding support for onetouch endpoints #47

Closed
wants to merge 1 commit into from

Conversation

luisuribe
Copy link
Contributor

No description provided.

Copy link
Contributor

@chrisdeeming chrisdeeming left a comment

Choose a reason for hiding this comment

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

Lots of phpDoc comments added to methods with no content; they should be removed or have comments provided, shouldn't they?

Copy link
Contributor

@chrisdeeming chrisdeeming left a comment

Choose a reason for hiding this comment

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

Unfortunately there are major issues with the new code, the approach this class has taken (generally) and arguably the endpoint URL structure.

The constructor sets up a base_uri like this:

'base_uri'      => "{$api_url}/protected/json/"

The request URL passed into the OneTouch endpoints is similar to:

onetouch/json/users/{$authy_id}/approval_requests

So this produces a request URI of:

"{$api_url}/protected/json/onetouch/json/users/{$authy_id}/approval_requests"

This is incorrect.

The only realistic way forward, I think, could be to change the base URI to simply:

"{$api_url}/"

And then update every request URI in this class, and others, to be, e.g.

'protected/json/users/new'

I need this code anyway, so I may submit a new pull request.

As I side note, I sort of question the logic behind the OneTouch stuff not being under, e.g. protected/json/onetouch in the first place, but I guess that's too difficult to change now.

@luisuribe
Copy link
Contributor Author

@chrisdeeming I'll take a look at your comments and modify the PR according to them. Hopefully it will be ready this weekend.

Thanks for your help!

@luisuribe
Copy link
Contributor Author

PR to be replaced by #55

@luisuribe luisuribe closed this Aug 27, 2018
@luisuribe luisuribe deleted the onetouch-support branch August 27, 2018 23:41
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.

None yet

2 participants