Define a consistent return value for `update` actions #171

SViccari · 2018-12-20T17:42:37Z

What does an update action return?

Depending on which class you're using, calling .update! or #update! will vary between returning a hash or an instance of the class.

How can we make this consistent and give our users the best experience?
I'd like to suggest that we always return a Hubspot::Response object that contains the parsed JSON
and avoid parsing the HubSpot API response to a domain object, (ex: Deal).

My thought process behind this approach:

We want to return all the fields that are returned by the HubSpot API. I've used API wrappers in the past that limit the fields the gem user could access and it made for a poor user experience.
Keep the interface simple. Define one class level .update! action and remove existing instance #update! methods.
Adding support for new endpoints should be easy. We want maintainers and contributors to enjoy adding to this project, keeping this project useful and up-to-date with the HubSpot API.

If everyone likes this approach, next steps include:

Document this change in History.md
We can merge this PR as is because the update actions that return a class instance are not using the response from Connection.put_json
Alter the update actions that return a class instance to be class level methods that return a Hubspot::Response.
Apply this approach to GET, POST, and DELETE actions

SViccari · 2018-12-20T17:51:43Z

lib/hubspot/company.rb

+          params: { company_id: vid },
+          body: { "properties" => properties }
+        )
+      end


Example of adding a new .update! endpoint that returns a Hubspot::Response.

SViccari · 2018-12-20T17:53:10Z

lib/hubspot/connection.rb

@@ -7,7 +7,8 @@ def get_json(path, opts)
        url = generate_url(path, opts)
        response = get(url, format: :json)
        log_request_and_response url, response
-        handle_response(response)
+        raise(Hubspot::RequestError.new(response)) unless response.success?
+        response.parsed_response


To keep these changes isolated, get_json still returns a hash as we focus on making updates consistent.

SViccari · 2018-12-20T17:56:02Z

spec/lib/hubspot/company_spec.rb

-    let(:company){ Hubspot::Company.new(example_company_hash) }
-    let(:params){ {name: "Acme Cogs", domain: "abccogs.com"} }
-    subject{ company.update!(params) }
+  describe ".update!" do


These changes:

updates the test to remove the use of its (which is deprecated but still available to us as we've pulled in the rspec-its gem)

updates the test to be self-reliant by creating and deleting any necessary data.

Nice, how far is the code base from removing rspec-its as a dependency?

There are about 10 spec files that use its heavily. It will take some time to get there.

MattMSumner

This seems like a step in the right direction.

MattMSumner · 2018-12-20T18:01:55Z

spec/lib/hubspot/company_spec.rb

-    let(:company){ Hubspot::Company.new(example_company_hash) }
-    let(:params){ {name: "Acme Cogs", domain: "abccogs.com"} }
-    subject{ company.update!(params) }
+  describe ".update!" do


Nice, how far is the code base from removing rspec-its as a dependency?

adimichele · 2018-12-20T19:12:33Z

Sounds like a good approach!

SViccari · 2018-12-20T19:59:09Z

@adimichele @MattMSumner Excellent, thank you! I'll wait for @cbisnett to add his thoughts before proceeding any further.

SViccari · 2018-12-29T16:08:25Z

@adimichele Is it okay if I proceed with these changes? This change is blocking work as we'll want future work to follow this model of returning a Hubspot::Response.

Or, do you know when @cbisnett will be back from holiday? I'd prefer to have his buy-in as well unless you're comfortable with moving forward.

adimichele · 2018-12-29T16:13:02Z

Yes, don’t want you blocked so please go ahead!

On Sat, Dec 29, 2018 at 11:08 AM Stephanie ***@***.***> wrote: @adimichele <https://github.com/adimichele> Is it okay if I proceed with these changes? This change is blocking work as we'll want future work to follow this model of returning a Hubspot::Response. Or, do you know when @cbisnett <https://github.com/cbisnett> will be back from holiday? I'd prefer to have his buy-in as well unless you're comfortable with moving forward. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#171 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABIm-CRuc_kj5eoeDhR7FPR5ol1_dIGcks5u95N6gaJpZM4ZcoY0> .

-- Sent from my mobile device

cbisnett · 2019-01-04T15:07:52Z

Sorry for the delayed response.

I don't think returning the parsed JSON is a good approach for two reasons:

It adds a leak to the abstraction. The whole point of the library is to add an abstraction on top of the API so it's less like an external service where you have to make a bunch of API calls and parse the returned data and more like a local resource where you interact with it like any other Ruby object. I fully agree with the sentiment that not being able to access all of the fields returned from the API is a poor experience and an issue with the abstraction but I don't think that simply returning the parsed JSON is the answer. I think the better answer is to make a base class that all resource objects inherit from where the properties are stored and can be accessed through a method_missing handler. This way we can also provide getter methods that convert response properties to idiomatic Ruby objects. For example when the API returns a timestamp in Unix epoch format, the library can convert it to a DateTime object which would be expected from a Ruby instance.
Idiomatic Ruby generally supports method chaining and this would break that. For example when a method modifies an instance and returns the instance (or the modified copy) you can do things like this:

irb(main):003:0> 'hello world'.split.map{|x| x.titleize }.join(' ')
=> "Hello World"

Returning a Hash with the result of the parsed JSON means that you can't chain any methods to the end of update.

For these reasons (and some other minor reasons) I think the correct implementation here is to return the instance. I've got an idea for how to implement a base resource class that would support being able to read all of the properties returned by the API which seems like the main reason for returning the JSON. I'll take a few minutes today to build this and submit a PR so everyone can review it.

cbisnett · 2019-01-05T20:33:04Z

I created PR #176 with a basic implementation for how I fee much of this should be refactored.

SViccari · 2019-02-22T03:19:30Z

Closing this PR in favor of pursuing a design that returns an object that represents the resource (ex: Hubspot::Contact) instead of returning a Hubspot::Response object.

SViccari added 3 commits December 20, 2018 11:17

remove Company#update

7957754

add Company.update

8be28c0

address failing tests

2b179d7

SViccari changed the title ~~update a company~~ Define a consistent return value for update actions Dec 20, 2018

SViccari commented Dec 20, 2018

View reviewed changes

MattMSumner approved these changes Dec 20, 2018

View reviewed changes

This was referenced Dec 26, 2018

Add update on classes #125

Closed

Define a separate method for form submission #173

Open

Define a consistent return value for each action #174

Open

SViccari closed this Feb 22, 2019

SViccari deleted the sv-update-a-company branch February 22, 2019 03:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define a consistent return value for `update` actions #171

Define a consistent return value for `update` actions #171

SViccari commented Dec 20, 2018 •

edited

Loading

SViccari Dec 20, 2018

SViccari Dec 20, 2018 •

edited

Loading

SViccari Dec 20, 2018

MattMSumner Dec 20, 2018

SViccari Dec 20, 2018

MattMSumner left a comment

MattMSumner Dec 20, 2018

adimichele commented Dec 20, 2018

SViccari commented Dec 20, 2018

SViccari commented Dec 29, 2018

adimichele commented Dec 29, 2018 via email

cbisnett commented Jan 4, 2019

cbisnett commented Jan 5, 2019

SViccari commented Feb 22, 2019

Define a consistent return value for update actions #171

Define a consistent return value for update actions #171

Conversation

SViccari commented Dec 20, 2018 • edited Loading

SViccari Dec 20, 2018

Choose a reason for hiding this comment

SViccari Dec 20, 2018 • edited Loading

Choose a reason for hiding this comment

SViccari Dec 20, 2018

Choose a reason for hiding this comment

MattMSumner Dec 20, 2018

Choose a reason for hiding this comment

SViccari Dec 20, 2018

Choose a reason for hiding this comment

MattMSumner left a comment

Choose a reason for hiding this comment

MattMSumner Dec 20, 2018

Choose a reason for hiding this comment

adimichele commented Dec 20, 2018

SViccari commented Dec 20, 2018

SViccari commented Dec 29, 2018

adimichele commented Dec 29, 2018 via email

cbisnett commented Jan 4, 2019

cbisnett commented Jan 5, 2019

SViccari commented Feb 22, 2019

Define a consistent return value for `update` actions #171

Define a consistent return value for `update` actions #171

SViccari commented Dec 20, 2018 •

edited

Loading

SViccari Dec 20, 2018 •

edited

Loading