Respect Client API options

[pat]

This pull request rejigs the code slightly so that the options provided to a Validic::Client instance (e.g. organization_id, access_token) are respected over the global defaults. If specific API requests use a custom organization_id or access_token as an option, then that takes priority over the client options.

[allcentury]

Hi @pat - Thanks for the PR! Can you talk to us a little more about your use case here? When we wrote the gem, we tried to take into account organizations and their sub organizations, is that the case you're trying to help with?

If so, I'm curious if you considered something like:

my_parent_og = Validic::Client.new(parent_org_options)
my_other_org = Validic::Client.new(other_org_options)

[pat]

Hi Anthony - code like that is precisely what we're doing, but it wasn't using the options appropriately.

client = Validic::Client.new(
  organization_id: our_org_id,
  access_token: our_access_token
)

# ignores the client's own value, uses Validic.organization_id, which is nil:
client.provision_user uid: '1'

# uses the specified organization id:
client.provision_user uid: '1', organization_id: our_org_id

[pat]

The first provision_user call above actually works as expected (using the client's organization_id value) with the 0.4.1 release, but we're currently using the latest commits from your master branch so we can pass through a specific user's authentication token to get_org_apps (which 0.4.1 does not support), and somewhere after 0.4.1, the behaviour changed/broke.

[allcentury]

Hi Pat - thank you for the quick reply, I can confirm we introduced this bug as part of PR #25 . Originally when we instantiated the client, we ran reload_config which was a setter for those module methods.

It worked as such:

 class Client
    def initialize(options={})
      @api_url        = options[:api_url].nil? ? 'https://api.validic.com' : options[:api_url]
      @api_version    = options[:api_version].nil? ? 'v1' : options[:api_version]
      @access_token   = options[:access_token].nil? ? Validic.access_token : options[:access_token]
      @organization_id = options[:organization_id].nil? ? Validic.organization_id : options[:organization_id]
      reload_config
    end

    ...

    def reload_config
      Validic.api_url = api_url
      Validic.api_version = api_version
      Validic.access_token = access_token
      Validic.organization_id = organization_id
    end
  end

Moving that one line back appears to fix the problem though it does conflict with your PR since some of the code was removed.

@jjlangholtz @markphelps I'm fine to do away with the reload_config method, I actually think this PR is more explicit but would like your guys input before proceeding.

@pat do you mind squashing your commits down for us?

[pat]

Squashed and force-pushed.

[markphelps]

lgtm. thanks @pat for the PR!

[allcentury]

Hi @pat - Your branch has the right ideas but unfortunately while the REST methods converge on construct_path, we're not consistent with the options passed in so methods like provision_user will need to be redefined. That will require we relook at how all the methods are interfacing and I don't want to delay a fix for the gem.

I've pushed up a hotfix that reenables the reload_config option so for the time being, please continue to use this convention:

my_parent_og = Validic::Client.new(parent_org_options)
my_other_org = Validic::Client.new(other_org_options)

I would not pass the optional organization_id into any of the methods, or use configure with a block but instead instantiate a new client for each organization (as illustrated above). We'll look at making this more consistent though so the interface is clear and so is the documentation.

I'm going to leave this issue open until a more widespread fix is in place.