Allow specifying a site id on a per-request basis

[wingrunr21]

Right now a user must swap out the site ids specified in the configuration in order for a specific request to target that site. Investigate if it is feasible to have requests target a subset of these site ids.

[stevehanson]

I'd also love to see this. It doesn't seem thread-safe to modify the global configuration for each request.

My initial naive solution was to modify Client to pass site_ids to auth_params if it is passed in as a local for any of the operations. That enabled me to do:

MindBody::Services::SiteService.get_sites(site_ids: [1,2,3])

but it fell apart when I realized that some operations don't allow locals to be passed in :( would love to hear any ideas from contributors to this project. Even just a little direction, and I could implement something and submit a PR.

[stevehanson]

I ended up monkey-patching some of the classes in my app to allow passing site_id or site_ids to any of the operations. Some of the operations disallow passing params, so you have to monkey-patch those as well. I just monkey-patched the get_locations operation in my code below:

# config/initializers/mind_body
MindBody.configure do |config|
  config.site_ids    = -99
  config.source_key  = 'xxxxxxxxx'
  config.source_name = 'xxxxx'
  config.log_level   = :debug
end

# Monkey-patching
Rails.configuration.to_prepare do
  MindBody::Services::Client.class_eval do

    # By default, the gem has the site IDs set up as global configuration.
    # Monkey-patch the call method to allow passing
    # `site_ids` to any operation and to pass it through in the request
    def call(operation_name, locals = {}, &block)
      @globals.open_timeout(MindBody.configuration.open_timeout)
      @globals.read_timeout(MindBody.configuration.read_timeout)
      @globals.log_level(MindBody.configuration.log_level)
      locals = locals.has_key?(:message) ? locals[:message] : locals
      locals = fixup_locals(locals)
      site_ids = locals[:site_ids] || [locals[:site_id]]
      params = {:message => {'Request' => auth_params(site_ids).merge(locals)}}

      # Run the request
      response = super(operation_name, params, &block)
      MindBody::Services::Response.new(response)
    end

    # did not previously take any params
    def auth_params(site_ids)
      site_ids = site_ids || MindBody.configuration.site_ids

      {
        'SourceCredentials' => {
          'SourceName' => MindBody.configuration.source_name,
          'Password' => MindBody.configuration.source_key,
          'SiteIDs'=> { 'int' => site_ids }
        }
      }
    end
  end

  MindBody::Services::SiteService.class_eval do
    # re-declare this operation, allowing locals
    operation :get_locations
  end

  # monkey-patch any other operations that need to allow locals
end

@wingrunr21 if you are ok with this approach, i'd be happy to submit a PR with the changes.

[wingrunr21]

My problem with passing site ids into every call is then you need to haul your site ids around your codebase. I'd rather have a global configuration for site ids (like it is now) and then allow a certain request to optionally target only a subset of those ids.

[stevehanson]

@wingrunr21 interesting. So basically, allow targeting a specific site_ids per call, but only if the site_ids are predefined in the configuration?

In my case, I have potentially hundreds of studios that I might be making calls to, and new studios can be added dynamically through the database at any time. Just trying to think through how I could make that work since the global list of site ID's are typically defined in a configuration block in an initializer.

[wingrunr21]

Ah, interesting. So, I wonder if it would be preferential to also support creating client instances and performing operations on them ala faraday vs the global method.

As an aside, how are you handling timestamps? Is part of your ingestion process to ask for the timezone of the given site?

[stevehanson]

The site I work with also integrates with a different booking platform, and I made a simple little Faraday client for that one and do what you described—one off instances of the client each time I need to make a call.

Regarding timezones, we are currently only in one timezone, so I am just stripping the timezone that comes back and parsing it in the server's timezone. I'm about to have to refactor though since we're expanding to other timezones soon.

[wingrunr21]

Ok, that sounds like a viable option. The services would need to be modified to support an optional client being passed in (and could simply default to the globally configured client otherwise).

#4 deals with the timestamp problem.

[stevehanson]

That sounds good to me. I'm reminded of Gibbon, the Ruby Mailchimp client, which uses a similar pattern.

With Gibbon, you can just make requests using class methods, which use the global configuration, like we currently do with MindBody:

lists = Gibbon::Request.lists.retrieve

Or, you can instantiate a client using custom parameters (like your proposed approach):

client = Gibbon::Request.new(api_key: "your_api_key", symbolize_keys: true) 
lists = client.lists.retrieve

I don't know much about Savon or what the limitations might be, but a syntax like this (basically copying what the Gibbon client does) seems like it could be nice:

client = MindBody::Services.new(client_options)
client.sale.checkout_shopping_cart(params)
client.staff.get_staff
# etc..

# or, to use the global config, it's the same as it currently is:
MindBody::Services::SaleService.checkout_shopping_cart(params)

# alternatively, for consistency with the client syntax, though would be a breaking change:
MindBody::Services.sale.checkout_shopping_cart(params)

Alternatively, the specific services themselves could be instantiated, though this would require a new client for each service that is used (not a big deal to me):

sale_client = MindBody::Services::SaleService.new(client_options)
sale_client.checkout_shopping_cart(params)

Sorry for the ramble. Just throwing out ideas as they come to mind, partly just to clear the thoughts in my head :)

Also -- thanks for pointing me in the right direction with the timestamp logic. I literally just had a conversation with my client about that yesterday.

[daidekman]

@wingrunr21 @stevehanson Thanks so much for all of the great contributions here and for this very helpful gem.

I was checking to learn if there was any further resolution on the recommended approach to handle this circumstance.

I am new to the mindbody-api and have encountered the same issue described by @stevehanson in his March 7 comments: we have potentially hundreds of studios that I might be making calls to, and new studios can be added dynamically through the database at any time.

Is there a suggested way to pass in the site_id as a parameter to get the response?

Many thanks for any assistance on this!

[stevehanson]

@daidekman this gem currently hard-codes the Site ID to whatever was specified in your configuration block, so it will not work for this use-case without modification.

The only solutions right now are to either monkey-patch the Client class as I demonstrated above in this thread or to fork the repo and update the Client class that way. I've been successfully using this gem with the monkey-patched changes for almost two years now with hundreds of studios, so it can absolutely be done with some modifications. Let me know if you have any questions or difficulties figuring it out.

[wingrunr21]

@stevehanson if you've solved the problem and maintained backwards compatibility for people, I'd love a PR

[stevehanson]

@wingrunr21 The reason I haven't submitted a PR yet and opened this thread is because I doubt my current approach is the best approach universally and was wanting to determine the right approach before submitting anything.

My current solution treats site_id and site_ids params as having special meaning. Passing them in for a request results in the client using them in the SourceCredentials. To do this, I've also had to remove locals: false, from every operation that specified this (since we have to allow the site_id or site_ids locals). I'm guessing that's not ideal.

If you approve, I could take a stab at adding something along the lines of what I proposed on Mar 7:

client_params = { site_id: "123456" }
sale_client = MindBody::Services::SaleService.new(client_params)
sale_client.checkout_shopping_cart(params)

[stevehanson]

I have a proof of concept working, will try to have a PR opened later this week or early next week.

The idea is to allow instantiating any of the services with passed in parameters that override the global defaults:

sale_service = MindBody::Services::SaleService.new(
  site_id: "123456",
  source_name: "my_source_name",
  source_key: "my_password",
  include_user_credentials: true
)

sale_service.checkout_shopping_cart(...)

which would result in an API call with the following parameters:

<Request>
  <SourceCredentials>
    <SourceName>my_source_name</SourceName>
    <Password>my_password</Password>
    <SiteIDs>
      <int>216536</int>
    </SiteIDs>
  </SourceCredentials>
  <UserCredentials>
    <Username>_my_source_name</Username>
    <Password>my_password</Password>
    <SiteIDs>
      <int>123456</int>
    </SiteIDs>
  </UserCredentials>
</Request>

Let me know what you think, @wingrunr21.

The include_user_credentials option was something I added because it seems useful. If set, include_user_credentials would include UserCredentials in the request, as shown above. I'm not sure if that makes sense on the class level like I have it, though. Would it make more sense to allow passing include_user_credentials on individual requests, and we just filter it out of the locals in the client?