Add update on classes #125

fonji · 2018-07-11T08:51:06Z

It's kind of like update_all, except just for one element.
Allows to update a company, contact or deal without the need of instanciating it.
Usefull if you have an application which knows vids and you want to avoid fetching from hubspot to update known fields.

Also adds specs for Deal#update!

cbisnett · 2018-10-18T17:20:32Z

Can you rebase this on master and fix any conflicts? Thanks.

cbisnett · 2018-11-21T17:52:47Z

I've rebased this onto the current master and fixed the conflicts. Tests are passing and this PR is ready for review.

cbisnett · 2018-11-21T18:09:26Z

lib/hubspot/company.rb

@@ -185,8 +197,7 @@ def [](property)
    # @param params [Hash] hash of properties to update
    # @return [Hubspot::Company] self
    def update!(params)
-      query = {"properties" => Hubspot::Utils.hash_to_properties(params.stringify_keys!, key_name: "name")}
-      Hubspot::Connection.put_json(UPDATE_COMPANY_PATH, params: { company_id: vid }, body: query)
+      self.class.update!(vid, params)
      @properties.merge!(params)


I don't think this is a blocker for this PR since we didn't have this behavior already but we're only updating the properties with the changes from params which means we're not updating dynamic properties such as timestamp and versions. I think the best approach is to copy the properties from the returned object to the current object which is what ActiveRecord reload() does.

Thoughts?

Interesting catch 🎣 I agree that this isn't a blocker for this PR.

Based on your description, I'm imagining something like:

response = self.class.update!(vid, params) # Re-initializes the object based on a hash of values from the API call # and updates the state of the object refresh_from(response)

** Updated **
I've created a ticket to capture this concern: https://trello.com/c/0nFLC7xU.
As someone who hasn't used the HubSpot API, do you think it's typical that users will want access to the version information?

As noted by @fonji below, the HubSpot just returns a 204 so there isn't really anything in the response to refresh from is there?

The Hubspot APIs are inconsistent with their return results from updates. Some only return a 204, some return a partial record with the changed fields, and others return the entire record.

I’ve mostly worked this functionality out on the master branch but I’ve not yet pushed it since I didn’t finish the tests yet.

cbisnett · 2018-11-21T18:12:16Z

lib/hubspot/contact.rb

+      # @param params [Hash] hash of properties to update
+      def update!(vid, params = {})
+        query = {"properties" => Hubspot::Utils.hash_to_properties(params.stringify_keys!)}
+        Hubspot::Connection.post_json(UPDATE_CONTACT_PATH, params: { contact_id: vid }, body: query)


This is not consistent with the other added update!() methods in that it doesn't return a new instance of the updated resource. This should return a new instance of the object.

The problem is that this endpoint

Returns a 204 No Content response on success.

https://developers.hubspot.com/docs/methods/contacts/update_contact

So if you want to return a new instance, you'd have to fetch it, which makes two calls instead of one.

@fonji Good point. For reasons like this, I think we are headed in the direction of always returning a Hubspot::Response object instead of attempting to return an instance of the class. PR #171 moves us in that direction, starting with some of the update actions. Once @cbisnett has time to review and approve this approach (and given it's the holidays, that may not be until after the new year) we can continue updating all the actions to either raise or return a Hubspot::Response, with the goal of releasing these changes as part of the v1 release.

SViccari · 2018-11-21T19:50:01Z

lib/hubspot/company.rb

+      # @param params [Hash] hash of properties to update
+      # @return [Hubspot::Company] Company record
+      def update!(vid, params)
+        params.stringify_keys!


Do we need to call stringify_keys here?

Seems unnecessary as it looks like Hubspot::Utils::hash_to_properties calls to_s on the keys:
https://github.com/adimichele/hubspot-ruby/blob/945a8717f260fc064ef6b34f427fc3333f28aaf4/lib/hubspot/utils.rb#L20

SViccari · 2018-11-21T20:04:31Z

lib/hubspot/company.rb

+      # @return [Hubspot::Company] Company record
+      def update!(vid, params)
+        params.stringify_keys!
+        query = {"properties" => Hubspot::Utils.hash_to_properties(params, key_name: "name")}


Interesting, I've only looked at a handful of examples but it seems the HubSpot v1 endpoints use "property" and the v2 endpoints use "name". Good to know.

SViccari · 2018-11-21T20:30:21Z

lib/hubspot/company.rb

+      # @param vid [Integer] hubspot company vid
+      # @param params [Hash] hash of properties to update
+      # @return [Hubspot::Company] Company record
+      def update!(vid, params)


This is a bit trivial but what do you think of calling this method update without the !? I tend to avoid the use of ! unless there is a safe version of the function. For example, stringify_keys! has the alternative stringify_keys.

I find that avoiding defining bang (!) methods leads to more descriptive function names. Since ! is a rather debated topic, I find it's use doesn't hold much value, except in the event where ! means there's an alternative option.

Better Naming example: Sending an invite email to a user. Instead of writing invite! to warn the reader something will happen, send_invite is a clearer message that informs the reader that an invite will be sent.

Other classes in the gem seem to use the ! on persistence style method calls. I was assuming this was following the ActiveRecord convention of bang versions of persistence methods raising exceptions whereas the non-bang version returns true/false.

SViccari · 2018-11-21T21:44:39Z

spec/lib/hubspot/company_spec.rb

@@ -49,6 +49,24 @@
    its(['num_associated_contacts']) { should eql '1' }
  end

+  describe ".update!" do
+    cassette "company_update_class"
+    let(:company) { Hubspot::Company.create!("New Company #{Time.now.to_i}") }


What do you think of avoiding the use of its, since its is deprecated in RSpec3, and the use of lets?

In regards to the use of lets, I find that avoiding the use of lets results in:

easier to read tests because all of the actors referenced in the test are declared in the setup step within the it block.

less brittle tests, because each example only specifies the information it needs.

faster tests, because each example is running with a smaller data set.

There's a great robots blog post here, if you'd like to read more: https://robots.thoughtbot.com/lets-not.

Alternative to lets:

describe ".update!" do it "updates the record" do VCR.use_cassette("company_update_class") do company = Hubspot::Company.create!("New Company #{Time.now.to_i}") ...any other test setup... Hubspot::Company.update!(company.vid, params) ...expectations... end end end

The DealPipeline spec follows a similar pattern.

SViccari · 2018-11-21T21:47:56Z

@cbisnett Thank you so much for pushing this along! I left some comments and I'll be happy to take another look when you're ready.

fonji · 2018-11-22T12:30:30Z

Hello!
Thank you for all the work and comments!
I'm sorry as I don't have much time to change this, can you guys handle it from now on? 😥

cbisnett · 2018-11-29T15:29:17Z

@fonji Don't worry about making these updates. I'll finish them and get this merged. Thanks for your work on the initial PR.

SViccari · 2018-12-26T15:45:24Z

Before pushing this along further, I think it's worth settling on the direction that's proposed in PR #171. PR #171 is just pending on @cbisnett's approval.

jesseclark · 2019-06-18T00:35:50Z

The company I am working for needs this functionality too. We wrote our own class method to achieve this which re-uses the utils class and the global connection object.

I was checking to see if it would make sense for us to contribute that work back to the gem and found this PR. Our implementation looks almost identical to what @fonji came up with here.

We cache HubSpot 'vids' that correspond to objects in our system so having to retrieve the object before updating adds an unnecessary API call to the process. This would be a useful option to have on all the resource classes.

fonji · 2019-11-08T15:47:06Z

Replaced by #209

fonji added 4 commits November 21, 2018 12:47

Add Company.update! on class

425efe3

Add Contact.update! on class

00c65ca

Add Deal.update! on class

d2f8ea3

Recreate spec for contact and deal update 404

361a75b

cbisnett force-pushed the add_update_on_classes branch from 90c7351 to 361a75b Compare November 21, 2018 17:51

cbisnett requested a review from SViccari November 21, 2018 17:52

cbisnett self-assigned this Nov 21, 2018

cbisnett reviewed Nov 21, 2018

View reviewed changes

cbisnett requested changes Nov 21, 2018

View reviewed changes

SViccari reviewed Nov 21, 2018

View reviewed changes

SViccari mentioned this pull request Dec 20, 2018

Add test coverage for PUT requests #170

Merged

fonji closed this Nov 8, 2019

fonji deleted the add_update_on_classes branch November 8, 2019 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add update on classes #125

Add update on classes #125

fonji commented Jul 11, 2018 •

edited

Loading

cbisnett commented Oct 18, 2018

cbisnett commented Nov 21, 2018

cbisnett Nov 21, 2018

SViccari Nov 21, 2018 •

edited

Loading

jesseclark Jun 18, 2019 •

edited

Loading

cbisnett Jun 18, 2019

cbisnett Nov 21, 2018

fonji Dec 26, 2018

SViccari Dec 26, 2018

SViccari Nov 21, 2018

jesseclark Jun 18, 2019

SViccari Nov 21, 2018 •

edited

Loading

SViccari Nov 21, 2018

jesseclark Jun 18, 2019

SViccari Nov 21, 2018 •

edited

Loading

SViccari commented Nov 21, 2018

fonji commented Nov 22, 2018

cbisnett commented Nov 29, 2018

SViccari commented Dec 26, 2018

jesseclark commented Jun 18, 2019

fonji commented Nov 8, 2019

Add update on classes #125

Add update on classes #125

Conversation

fonji commented Jul 11, 2018 • edited Loading

cbisnett commented Oct 18, 2018

cbisnett commented Nov 21, 2018

Choose a reason for hiding this comment

SViccari Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

jesseclark Jun 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SViccari Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SViccari Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

SViccari commented Nov 21, 2018

fonji commented Nov 22, 2018

cbisnett commented Nov 29, 2018

SViccari commented Dec 26, 2018

jesseclark commented Jun 18, 2019

fonji commented Nov 8, 2019

fonji commented Jul 11, 2018 •

edited

Loading

SViccari Nov 21, 2018 •

edited

Loading

jesseclark Jun 18, 2019 •

edited

Loading

SViccari Nov 21, 2018 •

edited

Loading

SViccari Nov 21, 2018 •

edited

Loading