Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/prestashopper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@
require 'rest-client'
require 'nokogiri'
require 'prestashopper/api'
require 'prestashopper/product'
require 'prestashopper/uri_handler'
require 'active_support/core_ext/string'
require 'active_support/core_ext/hash'

# @author Alfredo Amatriain <geralt@gmail.com>
#
Expand Down
82 changes: 57 additions & 25 deletions lib/prestashopper/api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,33 +22,65 @@ def initialize(url, key)
# List resources that the API key can access
# @return [Array<Symbol>] list of resources the API can access
def resources
xml = @resources_res.get.body
xml_doc = Nokogiri::XML xml
nodes = xml_doc.xpath '/prestashop/api/*'
resources_list = []
nodes.each{|n| resources_list << n.name.to_sym}
return resources_list
end

# Get all products data
# @return [Array<Hash>] list of products. Each product is represented by a hash with all its attributes.
def get_products
# /api/products returns XML with the IDs of each individual product
xml_products = @resources_res['products'].get.body
xml_products_doc = Nokogiri::XML xml_products
products_nodes = xml_products_doc.xpath '/prestashop/products/*/@id'
ids_list = []
products_nodes.each{|n| ids_list << n.value}

# GET each individual product to get the whole data
products = []
ids_list.each do |id|
xml_product = @resources_res["products/#{id}"].get.body
product = Product.xml2hash xml_product
products << product
@resources ||= Nokogiri::XML(@resources_res.get.body).xpath('/prestashop/api/*').collect { |resource| resource.name.to_sym }
Copy link
Author

Choose a reason for hiding this comment

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

This was mostly merged into one line to make it more clear that no get or parsing is done if @resoures is already set.

Copy link
Author

Choose a reason for hiding this comment

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

This was mostly merged into one line to make it more clear that no get or parsing is done if @resoures is already set.

Copy link
Owner

@amatriain amatriain Sep 17, 2016

Choose a reason for hiding this comment

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

Problem with memoization of @resources is that permissions assigned to an API key can be changed dynamically from the Prestashop administration. If an Api instance remembers the @resources array forever, this makes it necessary to get a new instance of the class after a change in permissions. I'd prefer if the list of resources accessible by the API key was not memoized, so that clients can get an up to date list of accessible resources at any time.

This would introduce a performance issue, however. You're using the @resources list in method_missing to see if the method being invoked matches an accessible resource. In this case we do want to get the memoized list of resources, so we don't make an extra HTTP call every time. Perhaps it would be best to create a new public get_resources method, that does an HTTP request every time, and make the resources method (that invokes get_resources only if @resources still has no value) private.

end

def method_missing(method, *args, &block)
Copy link
Author

Choose a reason for hiding this comment

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

This will support getting resource id list, a single resource or an array of resources:

api = Prestashopper::API.new url, key # => #<Prestashopper::API>
api.get_orders # => ["1", "2", "3", "4", "5"]
api.get_orders(1,2,3) # => [#<Prestashopper::Order id="1" ..>, #<Prestashopper::Order id="2"..> ..]
api.get_orders(1) # => [#<Prestashopper::Order id="1" ..>]
api.get_order(1) # => [#<Prestashopper::Order id="1" ..>]

Copy link
Author

Choose a reason for hiding this comment

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

I believe that api.get_orders or similar methods should only make a single API call for the ids instead of collecting all resources into an array.

Perhaps if it's needed I could add an option for something like api.get_orders(:all)? If you have a preference on this let me know and I can update the pull request.

Copy link
Author

Choose a reason for hiding this comment

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

This will support getting resource id list, a single resource or an array of resources:

api = Prestashopper::API.new url, key # => #<Prestashopper::API>
api.get_orders # => ["1", "2", "3", "4", "5"]
api.get_orders(1,2,3) # => [#<Prestashopper::Order id="1" ..>, #<Prestashopper::Order id="2"..> ..]
api.get_orders(1) # => [#<Prestashopper::Order id="1" ..>]
api.get_order(1) # => #<Prestashopper::Order id="1" ..>

Copy link
Owner

@amatriain amatriain Sep 17, 2016

Choose a reason for hiding this comment

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

Isn't it a bit inconsistent that get_orders(1, 2, 3) returns an array of objects, each with all the data about an order, while get_orders( ) returns an array of IDs?

I think getting data about e.g. all products from prestashop without knowing their IDs beforehand is an important use case that is not covered here.

if method.to_s.match(/^get_/)
resource = method.to_s.sub(/^get_/,'').pluralize.to_sym
raise RestClient::MethodNotAllowed, 'You do not have access to this resource' unless resources.include?(resource)
Copy link
Owner

@amatriain amatriain Sep 17, 2016

Choose a reason for hiding this comment

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

I don't like the gem raising RestClient errors. The fact that the gem uses RestClient to make HTTP calls should be an implementation detail transparent to an app that uses it.

On the other hand creating a hierarchy of errors only for this gem is overkill. I'm unsure about the best way to do it.


if method.to_s == method.to_s.singularize
get_resource(resource, collect_ids_for_resource(args), true)
else
Copy link
Author

Choose a reason for hiding this comment

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

This method was updated and moved into method_missing

Copy link
Author

Choose a reason for hiding this comment

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

This method was updated and moved into method_missing

ids = collect_ids_for_resources(args)
ids.present? ? get_resources(resource, ids) : get_resource_ids(resource)
end
else
super
end
end

private

def get_resource_ids(resource)
Nokogiri::XML(@resources_res[resource].get.body).xpath("/prestashop/#{resource}/*/@id").collect(&:value)
end

def get_resource(resource, id, raise_not_found_exception = false)
Copy link
Owner

Choose a reason for hiding this comment

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

What use is the raise_not_found_exception argument? The method is always invoked passing a true to this argument. Also there is no equivalent argument in get_resources method.

resource_class_name = resource.to_s.classify
resource_class = "Prestashopper::#{resource_class_name}".safe_constantize || Prestashopper.const_set(resource_class_name, Class.new(OpenStruct))
Copy link
Author

Choose a reason for hiding this comment

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

resource classes are generated on the fly, this will prevent the gem from needing an update when available resources are added.

Additionally, this code will still allow for a the class to be defined in case a custom parser is required or additional methods are added.

Copy link
Author

Choose a reason for hiding this comment

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

Dynamically generated classes inherit from OpenStruct so the dot syntax shown in the Readme will work properly.


begin
response = Nokogiri::XML(@resources_res[resource][id].get.body).remove_namespaces!.xpath("/prestashop/#{resource.to_s.singularize}")
JSON.parse(Hash.from_xml(response.to_s).values.first.to_json, object_class: resource_class)
Copy link
Owner

@amatriain amatriain Sep 17, 2016

Choose a reason for hiding this comment

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

Isn't a require 'active_support/json' necessary? Otherwise I think we'll get an undefined method error trying to invoke Hash.from_xml(response.to_s).values.first.to_json

rescue RestClient::NotFound
raise if raise_not_found_exception
nil
end
end

def get_resources(resource, ids)
ids.uniq.sort.collect { |id| get_resource(resource, id) }.compact
Copy link
Owner

Choose a reason for hiding this comment

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

Is the sort step really necessary?

end

def collect_ids_for_resource(args)
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps it would be better to call this method collect_id_for_resource, seeing that it validates and returns the ID of a single resource being requested :)

raise ArgumentError, "wrong number of arguments (#{args.length} for 1)" unless args.one?
id = args.first
validate_resource_id_type(id)
id
end

def collect_ids_for_resources(args)
return nil if args.none?
raise TypeError, 'Arguments must be a list of resource ids' unless args.is_a?(Array)
args.each { |id| validate_resource_id_type(id) }
args
end

return products
def validate_resource_id_type(id)
raise TypeError, 'Argument must be a valid resource id type' unless id.is_a?(Numeric) || id.is_a?(String)
id
end
end
end
21 changes: 0 additions & 21 deletions lib/prestashopper/product.rb

This file was deleted.

40 changes: 28 additions & 12 deletions test/api_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,12 @@ def setup
@url = 'http://my.prestashop.com'
@url_regex = %r{my[.]prestashop[.]com.*}
@key = 'VALID_KEY'
end
@resources = [:customers, :orders, :products]
@product_ids = ['1', '2']

def test_get_resources
xml = File.open(File.join __dir__, 'xml', 'resources.xml').read
stub_request(:any, @url_regex).to_return body: xml

resources = Prestashopper::API.new(@url, @key).resources
assert_equal 3, resources.length
[:customers, :orders, :products].each {|s| assert_includes resources, s}
end

def test_get_products
xml_products = File.open(File.join __dir__, 'xml', 'products.xml').read
stub_request(:any, %r{my[.]prestashop[.]com/api/products}).to_return body: xml_products

Expand All @@ -26,10 +20,32 @@ def test_get_products

Copy link
Author

Choose a reason for hiding this comment

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

I attempted to keep the existing functionality and complete some additional functionality as closely to the documentation as possible. New tests have been added for new functionality.

xml_product_2 = File.open(File.join __dir__, 'xml', 'product_2.xml').read
stub_request(:any, %r{my[.]prestashop[.]com/api/products/2}).to_return body: xml_product_2
end

def test_get_resources
Copy link
Author

Choose a reason for hiding this comment

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

I now see that this works differently than the original documentation, but matches the functionality of the code before my pull request. I can revert the test changes and fix the code if this preferable, let me know.

resources = Prestashopper::API.new(@url, @key).resources
assert_equal @resources.length, resources.length
@resources.each { |resource| assert_includes resources, resource }
end

def test_get_products
products_ids = Prestashopper::API.new(@url, @key).get_products
assert_equal @product_ids.length, products_ids.length
@product_ids.each { |id| assert_includes products_ids, id }
end

def test_get_products_for_ids
products = Prestashopper::API.new(@url, @key).get_products(*@product_ids)
assert_equal @product_ids.length, products.length
products_ids = products.collect(&:id)
@product_ids.each { |id| assert_includes products_ids, id }
products.each { |product| assert_kind_of Prestashopper::Product, product }
end

products = Prestashopper::API.new(@url, @key).get_products
assert_equal 2, products.length
product_ids = products.map{|p| p['id']}
['1','2'].each {|id| assert_includes product_ids, id}
def test_get_product_for_id
product_id = @product_ids.first
product = Prestashopper::API.new(@url, @key).get_product(product_id)
assert_kind_of Prestashopper::Product, product
assert_equal product.id, product_id
end
end