Skip to content

Add WFM connection class and specs IGNT-3539#1

Merged
abreckner merged 4 commits intomasterfrom
add_connection
Apr 24, 2020
Merged

Add WFM connection class and specs IGNT-3539#1
abreckner merged 4 commits intomasterfrom
add_connection

Conversation

@abreckner
Copy link
Contributor

Connect to WorkFlow max using a connection object.

I am initially adding a get method. post, put and delete to follow.

You can initialise it with the account_key, api_key and api_url

connection = XpmRuby::Connection.new(api_key: "api_key", api_url: "api.workflowmax.com", account_key: "account_key")

connection.get(endpoint: "staff.api/list")

We create the basic_auth key on instantiation. Also we are using Faraday as the library to handle HTTP requests.

Copy link
Contributor

@bedrock-adam bedrock-adam left a comment

Choose a reason for hiding this comment

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

Nice Tony!

We're putting these ones out for a wider team approval before merge too, right @lankz?

@abreckner abreckner requested review from a team and wizardofosmium April 24, 2020 03:52
end

def get(endpoint:)
Faraday.new("https://#{@api_url}/v3/#{endpoint}", headers: { "Authorization" => @basic_auth }).get

Choose a reason for hiding this comment

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

This is the guts of the PR. My feeling (my general feeling) is to split these out as much as possible. Something like:

    ENDPOINTS = {
      staff_list:    'staff.api/list'
    }.freeze
      
    def setup(endpoint:)
      @setup = @connector.new(url(endpoint), headers: headers)
    end
      
    def call
      @response = @setup.get
    end
      
    def url(endpoint)
      "https://#{@api_url}/v3/#{ENDPOINTS[endpoint]}"
    end
      
    def headers
      {
          "Authorization" => @basic_auth
      }
    end

What do you think of such a generalised approach? I'm thinking to take that approach because it's a gem.

BTW, this is great! 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I am still inclined to generalise later rather than sooner. For example, we can't set call to a get because sometimes it will have to be a post or a put or a delete. The less you do up front, the less you have to undo as the specifications become clearer (when the new endpoints get added).

Choose a reason for hiding this comment

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

I'm thinking if XPM ever changes its endpoints, you only have to make a change in the gem (any code using the gem is insulated).

connection.get(endpoint: "staff.api/list")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also think about the perspective of the service calling this. It will have to do a new, setup and then call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also not sure that a connector should have the list of endpoints on it...

Choose a reason for hiding this comment

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

I don't really want to derail you; I'm just bringing up generalisation because I think early generalisation (that is, just insulation really) will save time later (because it's a gem).

Choose a reason for hiding this comment

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

I'm just thinking maybe something like:

connection = XpmRuby::Connection.new(api_key: "api_key", api_url: "api.workflowmax.com", account_key: "account_key")

connection.staff_list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment we are going to let the caller specify the endpoint (e.g. Xpm::Staff will have a method called list_all which will call connection.get('staff.api/list'))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I discussed this with @wizardofosmium and made appropriate changes. :)

Copy link

@wizardofosmium wizardofosmium left a comment

Choose a reason for hiding this comment

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

Nice one! 👍

@abreckner abreckner merged commit 36d61b7 into master Apr 24, 2020
@bedrock-adam bedrock-adam changed the title Add WFM connection class and specs Add WFM connection class and specs IGNT-3539 Apr 26, 2020
@abreckner abreckner deleted the add_connection branch May 1, 2020 05:15
lankz pushed a commit that referenced this pull request Jun 10, 2025
Add WFM connection class and specs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants