Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add update on classes #125

Closed
wants to merge 4 commits into from
Closed

Add update on classes #125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
425efe3
Add Company.update! on class
fonji Jul 10, 2018
00c65ca
Add Contact.update! on class
fonji Jul 10, 2018
d2f8ea3
Add Deal.update! on class
fonji Jul 11, 2018
361a75b
Recreate spec for contact and deal update 404
fonji Jul 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions lib/hubspot/company.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ def batch_update!(companies)
end
Hubspot::Connection.post_json(BATCH_UPDATE_PATH, params: {}, body: query)
end

# Adds contact to a company
# {http://developers.hubspot.com/docs/methods/companies/add_contact_to_company}
# @param company_vid [Integer] The ID of a company to add a contact to
Expand All @@ -165,6 +165,18 @@ def add_contact!(company_vid, contact_vid)
},
body: nil)
end

# Updates the properties of a company
# {http://developers.hubspot.com/docs/methods/companies/update_company}
# @param vid [Integer] hubspot company vid
# @param params [Hash] hash of properties to update
# @return [Hubspot::Company] Company record
def update!(vid, params)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

params.stringify_keys!
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to call stringify_keys here?

Copy link

Choose a reason for hiding this comment

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

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

query = {"properties" => Hubspot::Utils.hash_to_properties(params, key_name: "name")}
Copy link
Contributor

@SViccari SViccari Nov 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

response = Hubspot::Connection.put_json(UPDATE_COMPANY_PATH, params: { company_id: vid }, body: query)
new(response)
end
end

attr_reader :properties
Expand All @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@SViccari SViccari Nov 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link

@jesseclark jesseclark Jun 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

self
end
Expand Down
11 changes: 9 additions & 2 deletions lib/hubspot/contact.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,14 @@ def merge!(primary_contact_vid, secondary_contact_vid)
body: { vidToMerge: secondary_contact_vid }
)
end

# Updates the properties of a contact
# {https://developers.hubspot.com/docs/methods/contacts/update_contact}
# @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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

end
end

attr_reader :properties, :vid, :is_new
Expand Down Expand Up @@ -197,8 +205,7 @@ def is_new=(val)
# @param params [Hash] hash of properties to update
# @return [Hubspot::Contact] self
def update!(params)
query = {"properties" => Hubspot::Utils.hash_to_properties(params.stringify_keys!)}
Hubspot::Connection.post_json(UPDATE_CONTACT_PATH, params: { contact_id: vid }, body: query)
self.class.update!(vid, params)
@properties.merge!(params)
self
end
Expand Down
15 changes: 13 additions & 2 deletions lib/hubspot/deal.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,18 @@ def find_by_association(object)
response = Hubspot::Connection.get_json(path, params)
response["results"].map { |deal_id| find(deal_id) }
end

# Updates the properties of a deal
# {http://developers.hubspot.com/docs/methods/deals/update_deal}
# @param deal_id [Integer] hubspot deal_id
# @param params [Hash] hash of properties to update
# @return [Hubspot::Deal] Deal record
def update!(deal_id, params)
params.stringify_keys!
query = {"properties" => Hubspot::Utils.hash_to_properties(params, key_name: "name")}
response = Hubspot::Connection.put_json(UPDATE_DEAL_PATH, params: { deal_id: deal_id }, body: query)
new(response)
end
end

# Archives the contact in hubspot
Expand All @@ -132,8 +144,7 @@ def [](property)
# @param params [Hash] hash of properties to update
# @return [Hubspot::Deal] self
def update!(params)
query = {"properties" => Hubspot::Utils.hash_to_properties(params.stringify_keys!, key_name: 'name')}
Hubspot::Connection.put_json(UPDATE_DEAL_PATH, params: { deal_id: deal_id }, body: query)
self.class.update!(deal_id, params)
@properties.merge!(params)
self
end
Expand Down
429 changes: 0 additions & 429 deletions spec/fixtures/vcr_cassettes/company_update.yml
View file
Open in desktop

This file was deleted.

136 changes: 136 additions & 0 deletions spec/fixtures/vcr_cassettes/company_update_class.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

136 changes: 136 additions & 0 deletions spec/fixtures/vcr_cassettes/company_update_instance.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading