Add Amazon Product API "ItemLookup" endpoint #173
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In preparation for rubyforgood#62, this commit pulls the endpoint-specific information out of the HTTPClient class and into and endpoint-specific class. Now HTTPClient is responsible only for managing the environment information and directing the user to the relevant endpoint. Adding a new endpoint is as simple as adding a method (ex. `item_search`) and corresponding class (ex. `ItemSearchEndpoint). The interface is *not* in its final form yet. This is just a good breaking point for a commit.
Building on the last commit, this adds a new ItemLookup endpoint. Now we can pull details on an individual item based on the ASIN. The endpoints and responses include a lot of duplication. Some refactoring will probably be needed.
When the item search endpoint was extracted, the specs weren't extracted with it; this means that the HTTPClientSpec was testing everything relating to the item search endpoint. This commit extracts all specs relating to the endpoint into a new file. This can now be refactored and copied over for the `ItemLookupEndpointSpec`.
This is a bit of a hack solution, but I don't feel comfortable doing any major abstraction here yet; I don't think we have enough information. Hopefully this'll clear up the CodeClimate complaints about duplicated code!
2 tasks
yarmiganosca
suggested changes
Oct 27, 2017
| describe AmazonProductAPI::ItemLookupEndpoint do | ||
| AWSTestCredentials = Struct.new(:access_key, :secret_key, :associate_tag) | ||
|
|
||
| let(:env) { |
There was a problem hiding this comment.
this should probably be named credentials or aws_credentials
| describe AmazonProductAPI::ItemSearchEndpoint do | ||
| AWSTestCredentials = Struct.new(:access_key, :secret_key, :associate_tag) | ||
|
|
||
| let(:env) { |
There was a problem hiding this comment.
this should probably be named credentials or aws_credentials
There have been enough changes that a merge sounds like a good idea. There are rubocop violations in this code, but I didn't want to tackle too much in one commit. I'll deal with that in a bit.
Two big points:
1. This only fixes easy rubocop issues related to this PR. This
doesn't touch any of the new capistrano code; I want the diff for
this PR to be fairly contained.
2. This includes two rubocop config changes:
i. Exclude the vendor directory from linting
ii. Allow multiline braces in tests (for exception checks, let, etc.)
There are a few more risky/controversial rubocop changes I omitted from
this commit. Those are coming next.
Fixes rubyforgood#56 This commit changes all the `URI.escape` calls to `CGI.escape`. All tests pass and the app still works. As a bonus, this seems to resolve issue rubyforgood#56 too-apostrophes are now properly escaped. This resolves all rubocop issues relating to this PR.
Following the review suggestions. This renames `env` to `aws_credentials`. The latter is a better name because it represents a credentials object (built from env vars), not the ENV object itself. Hopefully this will be clearer!
|
This is ready for review! I updated this PR to match Sorry about all the churn here. Let me know if you've got any questions/comments! |
leesharma
added a commit
to leesharma/playtime
that referenced
this pull request
Nov 26, 2017
When I fixed the code style for Code Climate in rubyforgood#173, I used the wrong hash leading to no update attributes being found. This fixes the bug, adds a test, and renames some of the variables.
seanmarcia
approved these changes
Nov 26, 2017
When I fixed the code style for Code Climate in rubyforgood#173, I used the wrong hash leading to no update attributes being found. This fixes the bug, adds a test, and renames some of the variables.
|
Hah, thanks, flattery's always appreciated. 🙂 I found a bug and pushed a new commit to fix it. Since you hit "approve", I'm going to go ahead and self-merge. 😬 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Works towards #62 (part 1/3)
Resolves #56 (bonus!)
Description
I broke #62 into three PRs for easier review: this is part 1.
This issue requires us to look up an Amazon item by its ASIN, so the first step is to add a new endpoint.
This PR starts with extracting the search-endpoint-specific logic out of the
HTTPClientclass and into a newItemSearchEndpointclass. TheHTTPClientis now responsible for routing between the different endpoints; each endpoint class is responsible for building/performing the actual queries.This PR involves a lot of code duplication between endpoints; the next PR is focused on cleaning that up.
Type of change
How Has This Been Tested?
Specs are in
spec/lib/amazon_product_api/*_endpoint_spec.rb, and the test suite is green.Nothing else is needed to verify these changes.