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

Unsubscribe API responses different behaviour and missing params #390

Open
shakaran opened this Issue Aug 29, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@shakaran
Contributor

shakaran commented Aug 29, 2017

I am trying two ways to get the list of unsuscribes for a domain, but I found diferent behaviours calling to different ways (which is pretty inconsistent):

If you check the unsubscribe API index in API agnostic documentation:

  1. It should allow two parameters: skip and limit
  2. The limit says 100 as maximum

But if you check the source code in PHP API you will see:

  1. It only allows limit parameter (you cannot use the skip parameter)
  2. The limit as default value is 100, but, there is an assert between 1 and 10000 ( 'Limit parameter must be between 1 and 10000' ), which confuse me even more.

But there are even more problems, which is is the way of how is returned the data from Mailgun\Model\Suppression\Unsubscribe\IndexResponse

For example:

use Mailgun\Model\Suppression\Unsubscribe\IndexResponse;

$result = $mailgun->suppressions()->unsubscribes()->index('example.com');

if($result instanceof IndexResponse)
{
    $total_unsubscribes = $result->getTotalCount(); // Missing param total_count
    $unsubscribes       = $result->getItems();
}

Instead if I call via:

$result = $mailgun->get('example.com/unsubscribes');

if(isset($result->http_response_code) && $result->http_response_code === 200)
{
    $total_unsubscribes = $result->http_response_body->total_count; // This record is available
    $unsubscribes       = $result->http_response_body->items;
}

Also since each item returned is an object of Mailgun/Model/Suppression/Unsubscribe/Unsubscribe, I notice which is missing the "id" fields which appears if you get only as json object via $mailgun->get('example.com/unsubscribes'); and the tag is returned as NULL instead "*" (which means all).

Conclusion:

  1. It is needed clarify if the limit is 100, 1000 or other, why this limit? what if I overpass that limit, how I get the remaining if no skip? Update the docs regading to unsubscribe suppresions properly with that info.
  2. Enable the skip parameter in index() PHP-SDK. Probably the proper way is allow params as array and not use another second parameter
  3. Implement method getTotalCount() in IndexResponse, the following could be valid:
   /**
    * @var integer
    */
    private $total_count = 0;


    /**
     * @return integer
     */
    public function getTotalCount()
    {
        return count($this->items);
    }
  1. In Unsubscribe implement id and getId() method or clarify why is displaying in normal call. All if NULL will means * should be reported always NULL or as *

Waiting your reply, thanks

@Nyholm

This comment has been minimized.

Show comment
Hide comment
@Nyholm

Nyholm Aug 30, 2017

Collaborator

About the limit/skip: I guess the api server has updated their specification. We should update the api client so it aligns with the docs.

Yes, we should have totalcount and getid.

Collaborator

Nyholm commented Aug 30, 2017

About the limit/skip: I guess the api server has updated their specification. We should update the api client so it aligns with the docs.

Yes, we should have totalcount and getid.

@shakaran

This comment has been minimized.

Show comment
Hide comment
@shakaran

shakaran Aug 30, 2017

Contributor

@Nyholm For point 1, I still need know which is the really limit number by the server specification, could you ask to the server guys? Which worry me more, it is how Mailgun flow is when the server spec changes things and the SDK clients receive the notification with changes for update properly? If not a clear way, a lot could be missing with the time if nobody detects

Contributor

shakaran commented Aug 30, 2017

@Nyholm For point 1, I still need know which is the really limit number by the server specification, could you ask to the server guys? Which worry me more, it is how Mailgun flow is when the server spec changes things and the SDK clients receive the notification with changes for update properly? If not a clear way, a lot could be missing with the time if nobody detects

@Nyholm

This comment has been minimized.

Show comment
Hide comment
@Nyholm

Nyholm Aug 30, 2017

Collaborator

For point 1, I still need know which is the really limit number by the server specification, could you ask to the server guys?

No, I have as much contact with "the server guys" as you have. ;) I do not work at Mailgun.

If not a clear way, a lot could be missing with the time if nobody detects

True, I would argue that it is a BC break to reduce the upper limit. Calls that previously returned 200 will now return 400. That is no good..

What do you suggest we should do? Maybe we should remove the client verification for this endpoint at let the server return the failure?

Collaborator

Nyholm commented Aug 30, 2017

For point 1, I still need know which is the really limit number by the server specification, could you ask to the server guys?

No, I have as much contact with "the server guys" as you have. ;) I do not work at Mailgun.

If not a clear way, a lot could be missing with the time if nobody detects

True, I would argue that it is a BC break to reduce the upper limit. Calls that previously returned 200 will now return 400. That is no good..

What do you suggest we should do? Maybe we should remove the client verification for this endpoint at let the server return the failure?

@shakaran

This comment has been minimized.

Show comment
Hide comment
@shakaran

shakaran Aug 30, 2017

Contributor

No, I have as much contact with "the server guys" as you have. ;) I do not work at Mailgun.

That's not a good thing :( Communication is the key. I was thinking that as Collaborator you was in contact with some representative, even if you are a freelance hired for them, the probably should help or designate to someone to speak with server guy-sdk relation.

True, I would argue that it is a BC break to reduce the upper limit. Calls that previously returned 200 will now return 400. That is no good..

Exactly. A silent BC from server

What do you suggest we should do? Maybe we should remove the client verification for this endpoint at let the server return the failure?

The should provide a changelog or show from specs or server changes in behaviour. So all SDK could adapt quickly based on that server changelog. The right thing should know the exact limit and raise an exception when the limits are reached previously to make the call. Imagine if you call 1000 times to that, and exception could stop you and avoid 999 calls more to the server, even you can catch the exception and treat properly if needed

Contributor

shakaran commented Aug 30, 2017

No, I have as much contact with "the server guys" as you have. ;) I do not work at Mailgun.

That's not a good thing :( Communication is the key. I was thinking that as Collaborator you was in contact with some representative, even if you are a freelance hired for them, the probably should help or designate to someone to speak with server guy-sdk relation.

True, I would argue that it is a BC break to reduce the upper limit. Calls that previously returned 200 will now return 400. That is no good..

Exactly. A silent BC from server

What do you suggest we should do? Maybe we should remove the client verification for this endpoint at let the server return the failure?

The should provide a changelog or show from specs or server changes in behaviour. So all SDK could adapt quickly based on that server changelog. The right thing should know the exact limit and raise an exception when the limits are reached previously to make the call. Imagine if you call 1000 times to that, and exception could stop you and avoid 999 calls more to the server, even you can catch the exception and treat properly if needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment