Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

added new Admin class for the AdminService.asmx #1

Merged
merged 1 commit into from Apr 20, 2012

Conversation

Projects
None yet
2 participants
Contributor

ttdonovan commented Apr 18, 2012

I'm looking into integration options the ChannelAdvisor API and found your gem. To get a better understanding of your code structure and test coverage I implemented a new class for the AdminService and fixed a few of your failing specs from the 'refactor' branch. As the code is now it appears that there is going to be lots of duplication and I have some suggestions for possibly drying up the code. My first incline is to create a services module and a class for each service inside. Each request to the API would be a from a service instance. Something like this:

>> os = ChannelAdvisor::Services::OrderService.new
>> os.ping
>> # sends soap API ping request, setting and last_response and returning the response
>> os.last_request
>> # soap request object
>> os.last_response
>> # soap response object

and since #ping is implement in all services it could possibly be moved into a Pingable module.

It's an idea I'm toying around with and would like to hear your feedback. I could create an new feature branch 'service-instances' and implement some of the code for a better example.

Cheers,
Tanner

@ttdonovan ttdonovan Added AdminService .ping .request_access(local_id) and .get_authoriza…
…tion_list(local_id)

Includes AdminService RSpec spec
Fixed few failing specs and moved stub_* wsdl and response into spec helper module
Added SimpleCov for Ruby 1.9 when running specs
fe2dc55
Owner

dnunez24 commented Apr 20, 2012

This is very much in the early stages. I'm in need of it myself for a current project and am focusing on the order service first as it is most pertinent to the project at hand. My plan was to create a Base class from which the other classes can inherit shared functionality so if there appears to be potential for a lot of duplication it is largely because I haven't focused effort on DRYing things up yet.

As for the API design, this is more what I had in mind because it seemed to me more Rubyesque and/or intuitive in the context of an object oriented API:

module ChannelAdvisor
  class Base
     def self.ping
       # do some pingy stuff
     end
  end
end

module ChannelAdvisor
  class Order < Base
    def self.list
      # list orders from ChannelAdvisor
    end

    def submit
      # submit order instance to ChannelAdvisor
    end
  end
end

# Call the Order.ping method
ChannelAdvisor::Order.ping

ChannelAdvisor::Order.list # returns an array of ChannelAdvisor::Order objects

# Perhaps it makes the most sense in this context
order = ChannelAdvisor::Order.new(
  # some attributes here
)
order.submit

I understand your reasoning behind keeping the classes/modules named after the ChannelAdvisor API services (e.g., OrderService vs Order) but I like the simplicity of having methods like Order.list and order.submit. That being said, I very much welcome your feedback, contribution, criticism, etc. and I will try to provide as much information/documentation as possible as I continue developing the gem. I will review your additions to the code shortly and try to get us on the same page.

Owner

dnunez24 commented Apr 20, 2012

@ttdonovan, I dig your contributions so far. I think there are a few things I might want to clean up for consistency but I like the modifications you've made. Also, I just realized the other day that I can probably get a lot of the WSDL and web service response stubbing accomplished much faster with the VCR gem. Any objections? I'm going to pull your changes. Feel free to supply any other feedback you feel is necessary. Two heads are better than one, they say . . .

@dnunez24 dnunez24 added a commit that referenced this pull request Apr 20, 2012

@dnunez24 dnunez24 Merge pull request #1 from sheetmusicplus/admin-service
Add Initial Admin Service Implementation
eb19012

@dnunez24 dnunez24 merged commit eb19012 into dnunez24:refactor Apr 20, 2012

Contributor

ttdonovan commented Apr 20, 2012

@dnunez24 I think I understand what you are trying to do. Below is something I think could meet both our needs and provide a lot of flexibility for this gem. The idea behind the Services modules is to hide all the SOAPy stuff but if needed a developer could instantiate those clasess and interacte/parse the responses themselves. As for VCR I haven't used the gem extensively but it looks like a good fit for this project.

module ChannelAdvisor
  module Services
    class BaseService
      attr_accessor :last_request, :last_response

      def ping
        # do some pingy stuff
      end

      def client
        # Savon client
      end
    end

    class OrderService < BaseService

      def get_order_list(options={})
        # all the soapy client stuff, could extract the building of header/credentials into a BaseService method...
        # set last_request
        # send the API request
        # set last_response
        # return true (?) # not sure what would be returned...
      end

      # the other ChannelAdvisor order service endpoints
    end
  end
end

module ChannelAdvisor
  class Order

    def self.ping
      order_service.new.ping
    end

    def self.list
      os = order_service.new
      os.get_order_list
      # parse the os.last_response, create new orders instances and return the array
    end

    def submit
      os = order_service.new
      # pass in attributes and submit order instance to ChannelAdvisor using the OrderService method
    end

    private

    def order_service
      ChannelAdvisor::Services::OrderService
    end
  end
end

# Call the Order.ping method
ChannelAdvisor::Order.ping

ChannelAdvisor::Order.list # returns an array of ChannelAdvisor::Order objects

# Perhaps it makes the most sense in this context
order = ChannelAdvisor::Order.new(
  # some attributes here
)
order.submit
Owner

dnunez24 commented Apr 20, 2012

Yeah, that's interesting. I considered a combination after your initial suggestion. Does it really make sense to add that extra layer of abstraction though? Take a look at this--I've been playing around with an example of the Base class:

module ChannelAdvisor
  class Base
    class << self

    private

      def client
        @client ||= Client.new(const_get(:WSDL))
      end

      def config(attribute)
        ChannelAdvisor.configuration.send(attribute.to_sym)
      end
    end # self
  end # Base
end # ChannelAdvisor

And here's what the Order class would look like:

module ChannelAdvisor
  class Order < Base
    # Unique subclass WSDL gets used in the inherited Base.client method
    WSDL = "https://api.channeladvisor.com/ChannelAdvisorAPI/v6/OrderService.asmx?WSDL"   
...

Is the end game of using your method for a separation of concerns? I can certainly see your point, especially in the Order.list method. One thing to note...the ping method doesn't actually exist in all services in v6 of the API (the Marketplace Ad Service, for example). I can definitely be convinced, I'm just not sure we're quite there yet.

Contributor

ttdonovan commented Apr 20, 2012

Yes, you could say my solution address a issue of separation of concerns. I guess what concerns me is at the level of abstraction you are purposing there will be mix of class where some subclass the 'Base' and others that don't. For example what about an 'Address' or 'LineItem' class? These type of classes have no interactions with the API. The way that I am purposing it's very clear that are specific classes that interact with the CA service API and the method names have a direct correlation. We can always start with your method and if it becomes necessary we can create that extra layer of abstraction that I am purposing (or at least I can branch off your work to demonstrate).

Owner

dnunez24 commented Apr 20, 2012

A very good point. There certainly would be no need for Address or LineItem to inherit from Base. You've won me over. Since there's still very little code written we might as well start off on the right foot instead of implementing half of it my way and then doing a rewrite. I figure we can just use undef_method on any classes that inherit from BaseService to solve the issue with something like the ping method unless you have a better suggestion.

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