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

Aftership #1354

Merged
merged 21 commits into from
Mar 21, 2016
Merged

Aftership #1354

merged 21 commits into from
Mar 21, 2016

Conversation

Jngai
Copy link
Contributor

@Jngai Jngai commented Mar 15, 2016

@cantino The spec is not ready but the agent is. I was a bit into this before I realized that you need a paying plan for the api. I can probably do more api integration with aftership but I think I should stop here. Its working but I would love some input.

@Jngai
Copy link
Contributor Author

Jngai commented Mar 15, 2016

I tested it locally as well.

@Jngai
Copy link
Contributor Author

Jngai commented Mar 15, 2016

@cantino The spec is good as well. I just put it in earlier.

You can use this agent to retrieve tracking data. You have to provide a specific `get` request and its associated option.

To get all trackings for your packages please enter `get` for key and `trackings` for the option.
To get tracking for a specific tracking number, add the extra keys `slug`, `tracking_number` and its associated values.
Copy link
Member

Choose a reason for hiding this comment

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

", and their associated values."

@Jngai
Copy link
Contributor Author

Jngai commented Mar 16, 2016

@cantino ok I think I am good. I tested all 3 endpoints locally and addressed the feedbacks. I also removed a bug and put in something that I left out.


event_description <<-MD
A typical tracking event has 3 objects (attributes, tracking, and checkpoint) and it looks like this
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I meant 4 spaces further then the line overhead (you might also need a empty line in between). You can check the rendered markdown by creating a new agent and selecting the AftershipAgent as a source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its rendered correctly now but I removed the attributes object because its not returning anything useful. Compared to other agents, its a bit on the long side. Hope this works out.

@Jngai Jngai mentioned this pull request Mar 16, 2016
To get the last checkpoint of a package set key to `path` and option to `last_checkpoint`. Please provide `slug` and `tracking_number`. Set `last_checkpoint_request` to true.

`slug` is a unique courier code.

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused about the relationship of single_tracking_request and last_checkpoint_request, and when you use them. Perhaps it would be cleaner to just have people provide the full path, with a link to the API?

All trackings:

{
  'path': '/tracking',
}

Specific:

{
  'path': '/tracking/fedex/123,
}

checkpoint:

{
  'path': '/last_checkpoint/fedex/123'
}

Then the docs could say:

Provide the path for the API endpoint that you'd like to hit. For example, for all active packages, enter /trackings (see https://www.aftership.com/docs/api/4/trackings), for a specific package, use /trackings/SLUG/TRACKING_NUMBER and replace SLUG with a courier code and TRACKING_NUMBER with the tracking number, or use Liquid (with a link).

Whatcha think?

@Jngai
Copy link
Contributor Author

Jngai commented Mar 17, 2016

@cantino I understand. I wasn't sure how to explain the agent. Once I put your suggestions in I removed the unnecessary methods and modified the spec accordingly. I tested it locally as well.

@Jngai
Copy link
Contributor Author

Jngai commented Mar 18, 2016

just double checked this locally

@coveralls
Copy link

Coverage Status

Coverage increased (+1.02%) to 90.72% when pulling a011af5 on Jngai:aftership into c1b2496 on cantino:master.

@cantino
Copy link
Member

cantino commented Mar 21, 2016

Looks great, thanks @Jngai!

cantino added a commit that referenced this pull request Mar 21, 2016
@cantino cantino merged commit 7a4ebee into huginn:master Mar 21, 2016
@Jngai Jngai deleted the aftership branch March 23, 2016 15:29
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.

4 participants