[Waiting on #174] WIP - Add rake task to sync items with Amazon #175
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
leesharma
force-pushed
the
items-sync
branch
3 times, most recently
from
74c3b05
to
b0a691b
Compare
I'm pretty torn on this commit-an abstract superclass doesn't seem like
a very ruby thing to do. Maybe I've been doing too much Java?
There are two main goals of this commit:
1. It should be trivial to add new endpoints. We're going to have to
add a few more, and being able to customize only the specialized
bits will save a ton of time and prevent bugs.
2. Shared code should exist in one location. Already, we've got a bug
in our signing process, and keeping this all in a superclass means
we only need to fix it in one place.
I don't like how I'm handling `@aws_credentials` at the moment
(requiring each subclass to initialize it); any ideas of a better way to
handle it?
Since the instructions are getting more complicated, this commit adds a README (with new endpoint instructions) to the Amazon Product API. This has three benefits: * It keeps the root README clean and relevant to most people. * It allows us to go into more documentation detail. * It facilitates an eventual extraction of the Amazon Product API lib. It might be worth extracting more from the "Getting Amazon Product Advertising API Working Locally" section into the lib README, but for now there's just endpoint information.
Whenever the tests run, the following error appeared:
warning: already initialized constant AWSTestCredentials
This moves the AWSTestCredentials init from the individual specs to a
spec helper class: `spec/support/helpers/amazon_helpers.rb`.
`rake items:sync` will sync all items in the database. Some chunking was done in `ItemSyncJob` in order to get around Amazon's rate limitations. TODO: * Batch the Amazon query (ItemLookup can take up to 10 ASINs) * Add a delayed job queue (new issue?) * Add specs
A couple notes:
1. I added a new exception to rubocop to allow for the
`expect { action_a }.to change { dynamic_attr }` form.
2. The 2 sec Amazon rate limit delay doesn't apply to tests.
All tests are green and no new rubocop violations are added.
As a first step for batching the requests, this commit makes the client endpoint take a list of asins instead of just an individual one. Other files are changed in order to work with the new interface (asins -> items) Next up: bulk-searching with the items sync job!
This commit takes advantage of the multi-asin item lookup requests and performs the item sync job in batches of 10.
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.
Resolves #62 (part 3/3)
Description
I broke #62 into three PRs for easier review: this is the final piece. (part 1: #173, part 2: #174)
Now that we have the new endpoint, I can add a rake task (
rake items:sync) to go through and sync the local items with Amazon. The real work happens inItemSyncJob. In order to get around Amazon rate limits, the requests are a little spaced out (seeItemSyncJob#sync_all!).Risks/Tradeoffs
I decided against putting any of these methods on
Item–to me, it seems cleaner to have an outside object doing the API calls.The rake task calls
#perform_nowbecause we don't have any sort of messaging queue set up. Given our low traffic, this should be fine for now.TODO
This PR is actually in progress. Here are the things I want to do before this gets merged:
Type of change
How Has This Been Tested?
So far, it hasn't! (Eventually it will be through
spec/jobs/items_sync_job_spec.rb.)